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

[Dirk Behme]

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.

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)
>>