| Message ID | 20171117142836.3456-1-dirk.behme@de.bosch.com |
|---|---|
| State | Accepted |
| Commit | b3cbcd902db7019410dfe3729a660abcb1f03ffb |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [U-Boot,RFC] usb: ehci: do not invalidate a NULL buffer | expand |
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@de.bosch.com> Looks OK, what about the other cache ops in EHCI, are they also affected? > --- > 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) >
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@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) >>
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@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) >>>
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)
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@de.bosch.com> --- 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(-)