[U-Boot] [RFC PATCH] usb: ehci: do not invalidate a NULL buffer

[Marek Vasut]

On 11/21/2017 07:30 AM, Dirk Behme wrote:
> On 17.11.2017 16:04, Marek Vasut wrote:
>> On 11/17/2017 03:28 PM, Dirk Behme wrote:
>>> Its a valid use case to call ehci_submit_async() with a NULL buffer
>>> with length 0. E.g. from usb_set_configuration().
>>>
>>> As invalidate_dcache_range() isn't able to judge if the address
>>> NULL is valid or not (depending on the SoC hardware configuration it
>>> might be valid) do the check in ehci_submit_async() as here we know
>>> that we don't have to invalidate such a buffer.
>>>
>>> Signed-off-by: Dirk Behme <dirk.behme at de.bosch.com>
>>
>> Looks OK, 
> 
> 
> Ok, thanks, I'll drop the RFC, then.
> 
> 
>> what about the other cache ops in EHCI, are they also affected?
> 
> 
> This was found based on a MMU configuration excluding address
> 0x00000000. Fixing this one location made a usb start and reading from
> an USB stick work.
> 
> So either only this location is affected, or above use case doesn't
> cover all relevant path. In latter case I don't have enough USB
> knowledge to evaluate all path by review.

Well, I skimmed through the rest of the function and this seems to be
the only spot where it could be problematic if length == 0 , so this is
the correct fix.

Applied, thanks.

> Best regards
> 
> Dirk
> 
> 
>>> ---
>>> Notes:
>>>
>>> This was found on an older vendor specific U-Boot on an ARMv8 SoC
>>> with a MMU configuration where address 0x00000000 is invalid.
>>>
>>> The callstack I've seen is
>>>
>>> usb_set_configuration(), calls
>>>   -> usb_control_msg() with *data == NULL and size == 0
>>>    -> submit_control_msg()
>>>     -> _ehci_submit_control_msg()
>>>      -> ehci_submit_async()
>>>
>>> ehci_submit_async() called  __asm_invalidate_dcache_range() from
>>> arch/arm/cpu/armv8/cache.S, then. Resulting in an exception trying
>>> to use address 0 in x0 (dc ivac, x0).
>>>
>>> I'm slightly unsure why this hasn't been catched or complained about
>>> previously, already. Maybe I missed anything while working with an older
>>> vendor U-Boot? Sorry in this case.
>>>
>>> Best regards
>>>
>>> Dirk
>>> ---
>>>   drivers/usb/host/ehci-hcd.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
>>> index be3e842dcc..32c661b37e 100644
>>> --- a/drivers/usb/host/ehci-hcd.c
>>> +++ b/drivers/usb/host/ehci-hcd.c
>>> @@ -595,8 +595,9 @@ ehci_submit_async(struct usb_device *dev,
>>> unsigned long pipe, void *buffer,
>>>        * dangerous operation, it's responsibility of the calling
>>>        * code to make sure enough space is reserved.
>>>        */
>>> -    invalidate_dcache_range((unsigned long)buffer,
>>> -        ALIGN((unsigned long)buffer + length, ARCH_DMA_MINALIGN));
>>> +    if (buffer != NULL && length > 0)
>>> +        invalidate_dcache_range((unsigned long)buffer,
>>> +            ALIGN((unsigned long)buffer + length, ARCH_DMA_MINALIGN));
>>>         /* Check that the TD processing happened */
>>>       if (QT_TOKEN_GET_STATUS(token) & QT_TOKEN_STATUS_ACTIVE)
>>>


-- 
Best regards,
Marek Vasut