On 24/07/2024 17:03, Mattijs Korpershoek wrote:

Hi Neil,

Thank you for the patch.

On ven., juil. 19, 2024 at 15:56, Neil Armstrong neil.armstrong@linaro.org wrote:

...

Also allocate the setup_buf with dma_alloc_coherent() since it's

The subject of the patch says:

"usb: dwc3: allocate setup_buf with dma_alloc_coherent()"

Isn't this line just repeating the title?

...

also consumed by the hardware DMA.

Yeah it's a verbose rewrite of the subject, I'll rewrite it to be less bad!

thanks Neil

...

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..ce35460c405 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 if setup_buf

if -> of

Both remarks being minor, please add:

Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com

...

• @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 16b11ce3d9f..0c7e0123368 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -381,7 +381,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);

@@ -663,7 +663,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 7d6bcc2627f..d41b590afb8 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2622,8 +2622,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;

@@ -2670,7 +2670,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);

@@ -2692,7 +2692,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

Hi Neil,

Thank you for the patch.

On ven., juil. 19, 2024 at 15:56, Neil Armstrong neil.armstrong@linaro.org 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 DMA_MINALIGN.

This causes operation failures Qualcomm platforms.

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

Signed-off-by: Neil Armstrong neil.armstrong@linaro.org

Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com

---

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..1aaf5413c6d 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 & ~(ARCH_DMA_MINALIGN - 1);
• uintptr_t end_addr = ALIGN((uintptr_t)addr + length, ARCH_DMA_MINALIGN);
• flush_dcache_range(start_addr, end_addr);

} #endif /* __DRIVERS_USB_DWC3_IO_H */

-- 2.34.1

Hi Neil,

Thank you for the patch.

On ven., juil. 19, 2024 at 15:56, Neil Armstrong neil.armstrong@linaro.org 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...

Signed-off-by: Neil Armstrong neil.armstrong@linaro.org

Reviewed-by: Mattijs Korpershoek mkorpershoek@baylibre.com

---

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 0c7e0123368..fc1d5892106 100644 --- a/drivers/usb/dwc3/ep0.c +++ b/drivers/usb/dwc3/ep0.c @@ -743,6 +743,8 @@ static void dwc3_ep0_inspect_setup(struct dwc3 *dwc, if (!dwc->gadget_driver) goto out;

• dwc3_invalidate_cache(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 d41b590afb8..0bc9aee4daa 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -2503,6 +2503,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 1aaf5413c6d..7cf05203b0d 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(start_addr, end_addr); }

+static inline void dwc3_invalidate_cache(uintptr_t addr, int length) +{

• uintptr_t start_addr = (uintptr_t)addr & ~(ARCH_DMA_MINALIGN - 1);
• uintptr_t end_addr = ALIGN((uintptr_t)addr + length, ARCH_DMA_MINALIGN);
• invalidate_dcache_range(start_addr, end_addr);

+} #endif /* __DRIVERS_USB_DWC3_IO_H */

-- 2.34.1