diff mbox series

[v4,3/3] usb: dwc3: invalidate dcache on buffer used in interrupt handling

Message ID 20241011-u-boot-dwc3-gadget-dcache-fixup-v4-3-5f3498d8035b@linaro.org
State New
Headers show
Series dwc3: gadget: properly fix cache operations | expand

Commit Message

Neil Armstrong Oct. 11, 2024, 2:38 p.m. UTC 
On Qualcomm systems, the setup buffer and even buffers are in
a bad state at interrupt handling, so invalidate the dcache lines
for the setup_buf and event buffer to make sure we read correct
data written by the hardware.

This fixes the following error:
dwc3-generic-peripheral usb@a600000: UNKNOWN IRQ type -1
dwc3-generic-peripheral usb@a600000: UNKNOWN IRQ type 4673109

and invalid situation in dwc3_gadget_giveback() because setup_buf content
is read at 0s and leads to fatal crash fixed by [1].

[1] https://lore.kernel.org/all/20240528-topic-sm8x50-dwc3-gadget-crash-fix-v1-1-58434ab4b3d3@linaro.org/

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/usb/dwc3/ep0.c    | 2 ++
 drivers/usb/dwc3/gadget.c | 2 ++
 drivers/usb/dwc3/io.h     | 8 ++++++++
 3 files changed, 12 insertions(+)

Comments

Marek Vasut Oct. 13, 2024, 8:38 p.m. UTC | #1 
On 10/11/24 4:38 PM, Neil Armstrong wrote:
> On Qualcomm systems, the setup buffer and even buffers are in
> a bad state at interrupt handling, so invalidate the dcache lines
> for the setup_buf and event buffer to make sure we read correct
> data written by the hardware.
> 
> This fixes the following error:
> dwc3-generic-peripheral usb@a600000: UNKNOWN IRQ type -1
> dwc3-generic-peripheral usb@a600000: UNKNOWN IRQ type 4673109
> 
> and invalid situation in dwc3_gadget_giveback() because setup_buf content
> is read at 0s and leads to fatal crash fixed by [1].
> 
> [1] https://lore.kernel.org/all/20240528-topic-sm8x50-dwc3-gadget-crash-fix-v1-1-58434ab4b3d3@linaro.org/
> 
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>

Reviewed-by: Marek Vasut <marex@denx.de>
Ahmad Fatoum Dec. 28, 2024, 1:46 p.m. UTC | #2 
Hi,

On 11.10.24 16:38, Neil Armstrong wrote:
> On Qualcomm systems, the setup buffer and even buffers are in
> a bad state at interrupt handling, so invalidate the dcache lines
> for the setup_buf and event buffer to make sure we read correct
> data written by the hardware.
> 
> This fixes the following error:
> dwc3-generic-peripheral usb@a600000: UNKNOWN IRQ type -1
> dwc3-generic-peripheral usb@a600000: UNKNOWN IRQ type 4673109
> 
> and invalid situation in dwc3_gadget_giveback() because setup_buf content
> is read at 0s and leads to fatal crash fixed by [1].
> 
> [1] https://lore.kernel.org/all/20240528-topic-sm8x50-dwc3-gadget-crash-fix-v1-1-58434ab4b3d3@linaro.org/
> 
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  drivers/usb/dwc3/ep0.c    | 2 ++
>  drivers/usb/dwc3/gadget.c | 2 ++
>  drivers/usb/dwc3/io.h     | 8 ++++++++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 8ba5fcd5312..531f0b522af 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -742,6 +742,8 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc,
>  	if (!dwc->gadget_driver)
>  		goto out;
>  
> +	dwc3_invalidate_cache((uintptr_t)ctrl, sizeof(*ctrl));

I believe this to be incorrect. ctrl here is dwc->ctrl_req, which AFAICS
is allocated in dwc3_gadget_init() using dma_alloc_coherent.

Doing cache maintenance on DMA-coherent memory doesn't make sense.
If this makes things better for you, it's probable that something else
is broken.

Additionally, this will break platforms that have DMA-coherent DWC3
and allocate normal memory instead of uncached memory. I am not sure
if there are such U-Boot platforms yet, as searching for dma-coherent DT
property yields no relevant results, but e.g. LS1046A in barebox would've
been broken by such a change.

Cheers,
Ahmad

> +
>  	len = le16_to_cpu(ctrl->wLength);
>  	if (!len) {
>  		dwc->three_stage_setup = false;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 19c3a5f5e58..e5a383407a2 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2534,6 +2534,8 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf)
>  	while (left > 0) {
>  		union dwc3_event event;
>  
> +		dwc3_invalidate_cache((uintptr_t)evt->buf, evt->length);
> +
>  		event.raw = *(u32 *) (evt->buf + evt->lpos);
>  
>  		dwc3_process_event_entry(dwc, &event);
> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
> index 0ede323671b..c1ab0288142 100644
> --- a/drivers/usb/dwc3/io.h
> +++ b/drivers/usb/dwc3/io.h
> @@ -55,4 +55,12 @@ static inline void dwc3_flush_cache(uintptr_t addr, int length)
>  
>  	flush_dcache_range((unsigned long)start_addr, (unsigned long)end_addr);
>  }
> +
> +static inline void dwc3_invalidate_cache(uintptr_t addr, int length)
> +{
> +	uintptr_t start_addr = (uintptr_t)addr & ~(CACHELINE_SIZE - 1);
> +	uintptr_t end_addr = ALIGN((uintptr_t)addr + length, CACHELINE_SIZE);
> +
> +	invalidate_dcache_range((unsigned long)start_addr, (unsigned long)end_addr);
> +}
>  #endif /* __DRIVERS_USB_DWC3_IO_H */
>
Neil Armstrong Dec. 30, 2024, 10:35 a.m. UTC | #3 
On 28/12/2024 14:46, Ahmad Fatoum wrote:
> Hi,
> 
> On 11.10.24 16:38, Neil Armstrong wrote:
>> On Qualcomm systems, the setup buffer and even buffers are in
>> a bad state at interrupt handling, so invalidate the dcache lines
>> for the setup_buf and event buffer to make sure we read correct
>> data written by the hardware.
>>
>> This fixes the following error:
>> dwc3-generic-peripheral usb@a600000: UNKNOWN IRQ type -1
>> dwc3-generic-peripheral usb@a600000: UNKNOWN IRQ type 4673109
>>
>> and invalid situation in dwc3_gadget_giveback() because setup_buf content
>> is read at 0s and leads to fatal crash fixed by [1].
>>
>> [1] https://lore.kernel.org/all/20240528-topic-sm8x50-dwc3-gadget-crash-fix-v1-1-58434ab4b3d3@linaro.org/
>>
>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   drivers/usb/dwc3/ep0.c    | 2 ++
>>   drivers/usb/dwc3/gadget.c | 2 ++
>>   drivers/usb/dwc3/io.h     | 8 ++++++++
>>   3 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 8ba5fcd5312..531f0b522af 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -742,6 +742,8 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc,
>>   	if (!dwc->gadget_driver)
>>   		goto out;
>>   
>> +	dwc3_invalidate_cache((uintptr_t)ctrl, sizeof(*ctrl));
> 
> I believe this to be incorrect. ctrl here is dwc->ctrl_req, which AFAICS
> is allocated in dwc3_gadget_init() using dma_alloc_coherent.
> 
> Doing cache maintenance on DMA-coherent memory doesn't make sense.
> If this makes things better for you, it's probable that something else
> is broken.
> 
> Additionally, this will break platforms that have DMA-coherent DWC3
> and allocate normal memory instead of uncached memory. I am not sure
> if there are such U-Boot platforms yet, as searching for dma-coherent DT
> property yields no relevant results, but e.g. LS1046A in barebox would've
> been broken by such a change.

dma_alloc_coherent() in U-boot allocates "aligned on cacheline" cached memory,
the function reuses the Linux naming but doesn't actually allocate
uncached/coherent memory.

Neil

> 
> Cheers,
> Ahmad
> 
>> +
>>   	len = le16_to_cpu(ctrl->wLength);
>>   	if (!len) {
>>   		dwc->three_stage_setup = false;
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 19c3a5f5e58..e5a383407a2 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2534,6 +2534,8 @@ static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf)
>>   	while (left > 0) {
>>   		union dwc3_event event;
>>   
>> +		dwc3_invalidate_cache((uintptr_t)evt->buf, evt->length);
>> +
>>   		event.raw = *(u32 *) (evt->buf + evt->lpos);
>>   
>>   		dwc3_process_event_entry(dwc, &event);
>> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
>> index 0ede323671b..c1ab0288142 100644
>> --- a/drivers/usb/dwc3/io.h
>> +++ b/drivers/usb/dwc3/io.h
>> @@ -55,4 +55,12 @@ static inline void dwc3_flush_cache(uintptr_t addr, int length)
>>   
>>   	flush_dcache_range((unsigned long)start_addr, (unsigned long)end_addr);
>>   }
>> +
>> +static inline void dwc3_invalidate_cache(uintptr_t addr, int length)
>> +{
>> +	uintptr_t start_addr = (uintptr_t)addr & ~(CACHELINE_SIZE - 1);
>> +	uintptr_t end_addr = ALIGN((uintptr_t)addr + length, CACHELINE_SIZE);
>> +
>> +	invalidate_dcache_range((unsigned long)start_addr, (unsigned long)end_addr);
>> +}
>>   #endif /* __DRIVERS_USB_DWC3_IO_H */
>>
> 
>
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 8ba5fcd5312..531f0b522af 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -742,6 +742,8 @@  static void dwc3_ep0_inspect_setup(struct dwc3 *dwc,
 	if (!dwc->gadget_driver)
 		goto out;
 
+	dwc3_invalidate_cache((uintptr_t)ctrl, sizeof(*ctrl));
+
 	len = le16_to_cpu(ctrl->wLength);
 	if (!len) {
 		dwc->three_stage_setup = false;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 19c3a5f5e58..e5a383407a2 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2534,6 +2534,8 @@  static irqreturn_t dwc3_process_event_buf(struct dwc3 *dwc, u32 buf)
 	while (left > 0) {
 		union dwc3_event event;
 
+		dwc3_invalidate_cache((uintptr_t)evt->buf, evt->length);
+
 		event.raw = *(u32 *) (evt->buf + evt->lpos);
 
 		dwc3_process_event_entry(dwc, &event);
diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
index 0ede323671b..c1ab0288142 100644
--- a/drivers/usb/dwc3/io.h
+++ b/drivers/usb/dwc3/io.h
@@ -55,4 +55,12 @@  static inline void dwc3_flush_cache(uintptr_t addr, int length)
 
 	flush_dcache_range((unsigned long)start_addr, (unsigned long)end_addr);
 }
+
+static inline void dwc3_invalidate_cache(uintptr_t addr, int length)
+{
+	uintptr_t start_addr = (uintptr_t)addr & ~(CACHELINE_SIZE - 1);
+	uintptr_t end_addr = ALIGN((uintptr_t)addr + length, CACHELINE_SIZE);
+
+	invalidate_dcache_range((unsigned long)start_addr, (unsigned long)end_addr);
+}
 #endif /* __DRIVERS_USB_DWC3_IO_H */