[PATCH v4 0/3] dwc3: gadget: properly fix cache operations

[PATCH v4 0/3] dwc3: gadget: properly fix cache operations

[Neil Armstrong]

We experience huge problems with cache handling on Qualcomm
systems, and it appears the dcache handling in the DWC3 gadget
code is quite wrong and causes operational issues.

This serie fixes the dcache operations on unaligned data,
and properly invalidate buffers when reading back data from
hardware.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Changes in v4:
- Go back to CACHELINE_SIZE, and do not use DMA_MINALIGN since it's not valid for all platforms
- Link to v3: https://lore.kernel.org/r/20241002-u-boot-dwc3-gadget-dcache-fixup-v3-0-5398088ef93c@linaro.org

Changes in v3:
- Cast addresses to (unsigned long) when calling invalidate_dcache_range()
- Drop unused CACHELINE_SIZE
- Fix warning by casting ctrl to uintptr_r when calling dwc3_invalidate_cache()
- Link to v2: https://lore.kernel.org/r/20240724-u-boot-dwc3-gadget-dcache-fixup-v2-0-65836d699a71@linaro.org

Changes in v2:
- Fix typo in drivers/usb/dwc3/core.h and rewrite patch 1 commit message
- Link to v1: https://lore.kernel.org/r/20240719-u-boot-dwc3-gadget-dcache-fixup-v1-0-58a5f026ea8e@linaro.org

---
Neil Armstrong (3):
      usb: dwc3: allocate setup_buf with dma_alloc_coherent()
      usb: dwc3: fix dcache flush range calculation
      usb: dwc3: invalidate dcache on buffer used in interrupt handling

 drivers/usb/dwc3/core.h   |  2 ++
 drivers/usb/dwc3/ep0.c    |  6 ++++--
 drivers/usb/dwc3/gadget.c | 10 ++++++----
 drivers/usb/dwc3/io.h     | 13 ++++++++++++-
 4 files changed, 24 insertions(+), 7 deletions(-)
---
base-commit: ddbcafeb53e7093c58488596bfce6d8823777c3a
change-id: 20240719-u-boot-dwc3-gadget-dcache-fixup-ea1e92758663

Best regards,
-- 
Neil Armstrong <neil.armstrong@linaro.org>

[PATCH v4 1/3] usb: dwc3: allocate setup_buf with dma_alloc_coherent()

[Neil Armstrong]

Since setup_buf is also consumed by hardware DMA, aligns it's
allocation like other hardware buffers by introduce setup_buf_addr
populated by dma_alloc_coherent(), and use it to pass the physical
address of the buffer to the hardware.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Reviewed-by: Marek Vasut <marex@denx.de>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/usb/dwc3/core.h   | 2 ++
 drivers/usb/dwc3/ep0.c    | 4 ++--
 drivers/usb/dwc3/gadget.c | 8 ++++----
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 7374ce950da..b572ea340c8 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -670,6 +670,7 @@ struct dwc3_scratchpad_array {
  * @ep0_trb: dma address of ep0_trb
  * @ep0_usb_req: dummy req used while handling STD USB requests
  * @ep0_bounce_addr: dma address of ep0_bounce
+ * @setup_buf_addr: dma address of setup_buf
  * @scratch_addr: dma address of scratchbuf
  * @lock: for synchronizing
  * @dev: pointer to our struct device
@@ -757,6 +758,7 @@ struct dwc3 {
 	dma_addr_t		ep0_trb_addr;
 	dma_addr_t		ep0_bounce_addr;
 	dma_addr_t		scratch_addr;
+	dma_addr_t		setup_buf_addr;
 	struct dwc3_request	ep0_usb_req;
 
 	/* device lock */
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 24f516a131b..8ba5fcd5312 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -380,7 +380,7 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
 	dep = dwc->eps[0];
 	dwc->ep0_usb_req.dep = dep;
 	dwc->ep0_usb_req.request.length = sizeof(*response_pkt);
-	dwc->ep0_usb_req.request.buf = dwc->setup_buf;
+	dwc->ep0_usb_req.request.buf = (void *)dwc->setup_buf_addr;
 	dwc->ep0_usb_req.request.complete = dwc3_ep0_status_cmpl;
 
 	return __dwc3_gadget_ep0_queue(dep, &dwc->ep0_usb_req);
@@ -662,7 +662,7 @@ static int dwc3_ep0_set_sel(struct dwc3 *dwc, struct usb_ctrlrequest *ctrl)
 	dep = dwc->eps[0];
 	dwc->ep0_usb_req.dep = dep;
 	dwc->ep0_usb_req.request.length = dep->endpoint.maxpacket;
-	dwc->ep0_usb_req.request.buf = dwc->setup_buf;
+	dwc->ep0_usb_req.request.buf = (void *)dwc->setup_buf_addr;
 	dwc->ep0_usb_req.request.complete = dwc3_ep0_set_sel_cmpl;
 
 	return __dwc3_gadget_ep0_queue(dep, &dwc->ep0_usb_req);
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index fe33e307d3e..19c3a5f5e58 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2653,8 +2653,8 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 		goto err1;
 	}
 
-	dwc->setup_buf = memalign(CONFIG_SYS_CACHELINE_SIZE,
-				  DWC3_EP0_BOUNCE_SIZE);
+	dwc->setup_buf = dma_alloc_coherent(DWC3_EP0_BOUNCE_SIZE,
+					(unsigned long *)&dwc->setup_buf_addr);
 	if (!dwc->setup_buf) {
 		ret = -ENOMEM;
 		goto err2;
@@ -2701,7 +2701,7 @@ err4:
 	dma_free_coherent(dwc->ep0_bounce);
 
 err3:
-	kfree(dwc->setup_buf);
+	dma_free_coherent(dwc->setup_buf);
 
 err2:
 	dma_free_coherent(dwc->ep0_trb);
@@ -2723,7 +2723,7 @@ void dwc3_gadget_exit(struct dwc3 *dwc)
 
 	dma_free_coherent(dwc->ep0_bounce);
 
-	kfree(dwc->setup_buf);
+	dma_free_coherent(dwc->setup_buf);
 
 	dma_free_coherent(dwc->ep0_trb);
 

-- 
2.34.1

[PATCH v4 2/3] usb: dwc3: fix dcache flush range calculation

[Neil Armstrong]

The current flush operation will omit doing a flush/invalidate on
the first and last bytes if the base address and size are not aligned
with CACHELINE_SIZE.

This causes operation failures Qualcomm platforms.

Take in account the alignment and size of the buffer and also
flush the previous and last cacheline.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/usb/dwc3/io.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
index 04791d4c9be..0ede323671b 100644
--- a/drivers/usb/dwc3/io.h
+++ b/drivers/usb/dwc3/io.h
@@ -50,6 +50,9 @@ static inline void dwc3_writel(void __iomem *base, u32 offset, u32 value)
 
 static inline void dwc3_flush_cache(uintptr_t addr, int length)
 {
-	flush_dcache_range(addr, addr + ROUND(length, CACHELINE_SIZE));
+	uintptr_t start_addr = (uintptr_t)addr & ~(CACHELINE_SIZE - 1);
+	uintptr_t end_addr = ALIGN((uintptr_t)addr + length, CACHELINE_SIZE);
+
+	flush_dcache_range((unsigned long)start_addr, (unsigned long)end_addr);
 }
 #endif /* __DRIVERS_USB_DWC3_IO_H */

-- 
2.34.1

Re: [PATCH v4 2/3] usb: dwc3: fix dcache flush range calculation

[Marek Vasut]

On 10/11/24 4:38 PM, Neil Armstrong wrote:
> The current flush operation will omit doing a flush/invalidate on
> the first and last bytes if the base address and size are not aligned
> with CACHELINE_SIZE.
> 
> This causes operation failures Qualcomm platforms.
> 
> Take in account the alignment and size of the buffer and also
> flush the previous and last cacheline.
> 
> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>   drivers/usb/dwc3/io.h | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
> index 04791d4c9be..0ede323671b 100644
> --- a/drivers/usb/dwc3/io.h
> +++ b/drivers/usb/dwc3/io.h
> @@ -50,6 +50,9 @@ static inline void dwc3_writel(void __iomem *base, u32 offset, u32 value)
>   
>   static inline void dwc3_flush_cache(uintptr_t addr, int length)
>   {
> -	flush_dcache_range(addr, addr + ROUND(length, CACHELINE_SIZE));
> +	uintptr_t start_addr = (uintptr_t)addr & ~(CACHELINE_SIZE - 1);
> +	uintptr_t end_addr = ALIGN((uintptr_t)addr + length, CACHELINE_SIZE);
> +
> +	flush_dcache_range((unsigned long)start_addr, (unsigned long)end_addr);
>   }
>   #endif /* __DRIVERS_USB_DWC3_IO_H */

You likely want a max(CONFIG_SYS_CACHELINE_SIZE, 
dwc3-buffer-alignment-requirement) to really correctly align the buffer.

Re: [PATCH v4 2/3] usb: dwc3: fix dcache flush range calculation

[Neil Armstrong]

Hi,

Le 12/10/2024 à 05:37, Marek Vasut a écrit :
> On 10/11/24 4:38 PM, Neil Armstrong wrote:
>> The current flush operation will omit doing a flush/invalidate on
>> the first and last bytes if the base address and size are not aligned
>> with CACHELINE_SIZE.
>>
>> This causes operation failures Qualcomm platforms.
>>
>> Take in account the alignment and size of the buffer and also
>> flush the previous and last cacheline.
>>
>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>>   drivers/usb/dwc3/io.h | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
>> index 04791d4c9be..0ede323671b 100644
>> --- a/drivers/usb/dwc3/io.h
>> +++ b/drivers/usb/dwc3/io.h
>> @@ -50,6 +50,9 @@ static inline void dwc3_writel(void __iomem *base, u32 offset, u32 value)
>>   static inline void dwc3_flush_cache(uintptr_t addr, int length)
>>   {
>> -    flush_dcache_range(addr, addr + ROUND(length, CACHELINE_SIZE));
>> +    uintptr_t start_addr = (uintptr_t)addr & ~(CACHELINE_SIZE - 1);
>> +    uintptr_t end_addr = ALIGN((uintptr_t)addr + length, CACHELINE_SIZE);
>> +
>> +    flush_dcache_range((unsigned long)start_addr, (unsigned long)end_addr);
>>   }
>>   #endif /* __DRIVERS_USB_DWC3_IO_H */
> 
> You likely want a max(CONFIG_SYS_CACHELINE_SIZE, dwc3-buffer-alignment-requirement) to really correctly align the buffer.

No this change is related to the system cacheline to properly flush/invalidate the data
to the system memory, the dwc3 buffer alignment (if there's one) should be handled
when allocating the memory, but here we're using dma_alloc_coherent() which
use DMA_MINALIGN.

So it's another unrelated story.

Neil

Re: [PATCH v4 2/3] usb: dwc3: fix dcache flush range calculation

[Marek Vasut]

On 10/13/24 6:35 PM, Neil Armstrong wrote:
> Hi,
> 
> Le 12/10/2024 à 05:37, Marek Vasut a écrit :
>> On 10/11/24 4:38 PM, Neil Armstrong wrote:
>>> The current flush operation will omit doing a flush/invalidate on
>>> the first and last bytes if the base address and size are not aligned
>>> with CACHELINE_SIZE.
>>>
>>> This causes operation failures Qualcomm platforms.
>>>
>>> Take in account the alignment and size of the buffer and also
>>> flush the previous and last cacheline.
>>>
>>> Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>> ---
>>>   drivers/usb/dwc3/io.h | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/io.h b/drivers/usb/dwc3/io.h
>>> index 04791d4c9be..0ede323671b 100644
>>> --- a/drivers/usb/dwc3/io.h
>>> +++ b/drivers/usb/dwc3/io.h
>>> @@ -50,6 +50,9 @@ static inline void dwc3_writel(void __iomem *base, 
>>> u32 offset, u32 value)
>>>   static inline void dwc3_flush_cache(uintptr_t addr, int length)
>>>   {
>>> -    flush_dcache_range(addr, addr + ROUND(length, CACHELINE_SIZE));
>>> +    uintptr_t start_addr = (uintptr_t)addr & ~(CACHELINE_SIZE - 1);
>>> +    uintptr_t end_addr = ALIGN((uintptr_t)addr + length, 
>>> CACHELINE_SIZE);
>>> +
>>> +    flush_dcache_range((unsigned long)start_addr, (unsigned 
>>> long)end_addr);
>>>   }
>>>   #endif /* __DRIVERS_USB_DWC3_IO_H */
>>
>> You likely want a max(CONFIG_SYS_CACHELINE_SIZE, dwc3-buffer- 
>> alignment-requirement) to really correctly align the buffer.
> 
> No this change is related to the system cacheline to properly flush/ 
> invalidate the data
> to the system memory, the dwc3 buffer alignment (if there's one) should 
> be handled
> when allocating the memory, but here we're using dma_alloc_coherent() which
> use DMA_MINALIGN.

OK

Reviewed-by: Marek Vasut <marex@denx.de>

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

[Neil Armstrong]

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));
+
 	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 */

-- 
2.34.1

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

[Marek Vasut]

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>

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

[Ahmad Fatoum]

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 */
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Neil Armstrong]

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 */
>>
> 
> 

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

[Ahmad Fatoum]

Hi Neil,

On 30.12.24 11:35, Neil Armstrong wrote:
> On 28/12/2024 14:46, Ahmad Fatoum wrote:
>> 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.

:/

No wonder these bugs happen. Thanks for clarifying.

Cheers,
Ahmad

> 
> 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 */
>>>
>>
>>
> 
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v4 0/3] dwc3: gadget: properly fix cache operations

[Mattijs Korpershoek]

Hi,

On Fri, 11 Oct 2024 16:38:23 +0200, Neil Armstrong wrote:
> We experience huge problems with cache handling on Qualcomm
> systems, and it appears the dcache handling in the DWC3 gadget
> code is quite wrong and causes operational issues.
> 
> This serie fixes the dcache operations on unaligned data,
> and properly invalidate buffers when reading back data from
> hardware.
> 
> [...]

Thanks, Applied to https://source.denx.de/u-boot/custodians/u-boot-dfu (u-boot-dfu)

[1/3] usb: dwc3: allocate setup_buf with dma_alloc_coherent()
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/1f12fc7e3350b179d17efaf5ba00fc3683cf33ec
[2/3] usb: dwc3: fix dcache flush range calculation
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/502a50ab1f7e32e3e90056597e8ce6a0931789ba
[3/3] usb: dwc3: invalidate dcache on buffer used in interrupt handling
      https://source.denx.de/u-boot/custodians/u-boot-dfu/-/commit/3e47302dd71267b85e5ec65c5b6d881c23cce6cb

--
Mattijs

Re: [PATCH v4 0/3] dwc3: gadget: properly fix cache operations

[Tom Rini]

On Fri, 11 Oct 2024 16:38:23 +0200, Neil Armstrong wrote:

> We experience huge problems with cache handling on Qualcomm
> systems, and it appears the dcache handling in the DWC3 gadget
> code is quite wrong and causes operational issues.
> 
> This serie fixes the dcache operations on unaligned data,
> and properly invalidate buffers when reading back data from
> hardware.
> 
> [...]

Applied to u-boot/master, thanks!

-- 
Tom

Re: [PATCH v4 0/3] dwc3: gadget: properly fix cache operations

[Mattijs Korpershoek]

Hi Tom,

On lun., oct. 21, 2024 at 17:31, Tom Rini <trini@konsulko.com> wrote:

> On Fri, 11 Oct 2024 16:38:23 +0200, Neil Armstrong wrote:
>
>> We experience huge problems with cache handling on Qualcomm
>> systems, and it appears the dcache handling in the DWC3 gadget
>> code is quite wrong and causes operational issues.
>> 
>> This serie fixes the dcache operations on unaligned data,
>> and properly invalidate buffers when reading back data from
>> hardware.
>> 
>> [...]
>
> Applied to u-boot/master, thanks!

I think you already pulled this via:

https://patchwork.ozlabs.org/project/uboot/patch/87iktr2l9t.fsf@baylibre.com/

>
> -- 
> Tom

Re: [PATCH v4 0/3] dwc3: gadget: properly fix cache operations

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]

On Tue, Oct 22, 2024 at 11:10:27AM +0200, Mattijs Korpershoek wrote:
> Hi Tom,
> 
> On lun., oct. 21, 2024 at 17:31, Tom Rini <trini@konsulko.com> wrote:
> 
> > On Fri, 11 Oct 2024 16:38:23 +0200, Neil Armstrong wrote:
> >
> >> We experience huge problems with cache handling on Qualcomm
> >> systems, and it appears the dcache handling in the DWC3 gadget
> >> code is quite wrong and causes operational issues.
> >> 
> >> This serie fixes the dcache operations on unaligned data,
> >> and properly invalidate buffers when reading back data from
> >> hardware.
> >> 
> >> [...]
> >
> > Applied to u-boot/master, thanks!
> 
> I think you already pulled this via:
> 
> https://patchwork.ozlabs.org/project/uboot/patch/87iktr2l9t.fsf@baylibre.com/

Ah, hunh. OK. So, I should try and run the script to update patchwork
status based on patch hashes more often. And I think b4 applied it to
the original merge base, then merged to master as a no-op.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]