usb: dwc3: Stop active transfers before halting the controller
Subsystems (2)
PATCH v2 1/1 usb: dwc3: Stop active transfers before halting the controller
In the DWC3 databook, for a device initiated disconnect or bus reset, the driver is required to send dependxfer commands for any pending transfers. In addition, before the controller can move to the halted state, the SW needs to acknowledge any pending events. If the controller is not halted properly, there is a chance the controller will continue accessing stale or freed TRBs and buffers.
---
Changes in v2:
- Moved cleanup code to the pullup() API to differentiate between device disconnect and hibernation.
- Added cleanup code to the bus reset case as well.
- Verified the move to pullup() did not reproduce the problen using the same test sequence.
Verified fix by adding a check for ETIMEDOUT during the run stop call. Shell script writing to the configfs UDC file to trigger disconnect and connect. Batch script to have PC execute data transfers over adb (ie adb push) After a few iterations, we'd run into a scenario where the controller wasn't halted. With the following change, no failed halts after many iterations.
--- drivers/usb/dwc3/ep0.c | 2 +- drivers/usb/dwc3/gadget.c | 52 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 2 deletions(-)
drivers/usb/dwc3/ep0.c +1 -1
@@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,int ret;spin_lock_irqsave (&dwc->lock, flags); -if (!dep->endpoint.desc) { +if (!dep->endpoint.desc || !dwc->pullups_connected) {dev_err (dwc->dev,"%s: can't queue to disabled endpoint\n" , dep->name); ret = -ESHUTDOWN ;
drivers/usb/dwc3/gadget.c +51 -1
79 lines not on this page · raw message
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Thinh Nguyen
maintainerQuoted 61 lines · Wesley Cheng
In the DWC3 databook, for a device initiated disconnect or bus reset, the driver is required to send dependxfer commands for any pending transfers. In addition, before the controller can move to the halted state, the SW needs to acknowledge any pending events. If the controller is not halted properly, there is a chance the controller will continue accessing stale or freed TRBs and buffers.
Signed-off-by Wesley Cheng <[email protected]>---
Changes in v2:
- Moved cleanup code to thepullup()API to differentiate between device disconnect and hibernation.
- Added cleanup code to the bus reset case as well.
- Verified the move topullup()did not reproduce the problen using the same test sequence.Verified fix by adding a check for ETIMEDOUT during the run stop call. Shell script writing to the configfs UDC file to trigger disconnect and connect. Batch script to have PC execute data transfers over adb (ie adb push) After a few iterations, we'd run into a scenario where the controller wasn't halted. With the following change, no failed halts after many iterations.
--- drivers/usb/dwc3/ep0.c | 2 +- drivers/usb/dwc3/gadget.c | 52 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 2 deletions(-)drivers/usb/dwc3/ep0.c +1
-1@@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,int ret;spin_lock_irqsave (&dwc->lock, flags); -if (!dep->endpoint.desc) { +if (!dep->endpoint.desc || !dwc->pullups_connected) {dev_err (dwc->dev,"%s: can't queue to disabled endpoint\n" , dep->name); ret = -ESHUTDOWN ;drivers/usb/dwc3/gadget.c +4
-1@@ -1516,7 +1516,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) {struct dwc3 *dwc = dep->dwc; -if (!dep->endpoint.desc) { +if (!dep->endpoint.desc || !dwc->pullups_connected) {dev_err (dwc->dev,"%s: can't queue to disabled endpoint\n" , dep->name);return -ESHUTDOWN ;@@ -1926,6 +1926,24 @@ static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,return 0 ; } +static void dwc3_stop_active_transfers (struct dwc3 *dwc) +{ +u32 epnum;
drivers/usb/dwc3/gadget.c static int dwc3_gadget_set_selfpowered(struct usb_gadget *g, · line 1941
Quoted 10 lines
drivers/usb/dwc3/gadget.c +10
-0+ +for (epnum =2 ; epnum <DWC3_ENDPOINTS_NUM ; epnum++) { +struct dwc3_ep *dep; + + dep = dwc->eps[epnum]; +if (!dep) +continue ; + +if (!(dep->flags &DWC3_EP_ENABLED )) +continue ;
Don't do the enabled check here. Let the dwc3_stop_active_transfer() do that checking.
Quoted 9 lines
drivers/usb/dwc3/gadget.c +5
-0+ +dwc3_remove_requests (dwc, dep); + } +} +static int dwc3_gadget_run_stop (struct dwc3 *dwc,int is_on,int suspend) {u32 reg;
drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2019
Quoted 8 lines
drivers/usb/dwc3/gadget.c +5
-0@@ -1994,9 +2012,39 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) } } +/* +* Synchronize and disable any further event handling while controller +* is being enabled/disabled. +*/ +disable_irq (dwc->irq_gadget);
I think it's better to do dwc3_gadget_disable_irq(). This only stops handling events. Although it's unlikely, the controller may still generate events before it's halted.
drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2027
Quoted 8 lines
drivers/usb/dwc3/gadget.c +7
-0spin_lock_irqsave (&dwc->lock, flags); + +/* Controller is not halted until pending events are acknowledged */ +if (!is_on) { +u32 reg; + +__dwc3_gadget_ep_disable (dwc->eps[0 ]); +__dwc3_gadget_ep_disable (dwc->eps[1 ]);
You can just do __dwc3_gadget_stop(), and do that after dwc3_stop_active_transfers().
drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2037
Quoted 10 lines
drivers/usb/dwc3/gadget.c +10
-0+ +/* +* The databook explicitly mentions for a device-initiated +* disconnect sequence, the SW needs to ensure that it ends any +* active transfers. +*/ +dwc3_stop_active_transfers (dwc); + + reg =dwc3_readl (dwc->regs,DWC3_GEVNTCOUNT (0 )); + reg &=DWC3_GEVNTCOUNT_MASK ;
Can we use another variable "count" instead of reusing reg to make it a little clearer?
Quoted 14 lines
drivers/usb/dwc3/gadget.c +8
-0+if (reg >0 ) { +dwc3_writel (dwc->regs,DWC3_GEVNTCOUNT (0 ), reg); + dwc->ev_buf->lpos = (dwc->ev_buf->lpos + reg) % + dwc->ev_buf->length; + } + } + ret =dwc3_gadget_run_stop (dwc, is_on, false);spin_unlock_irqrestore (&dwc->lock, flags); +enable_irq (dwc->irq_gadget);return ret; }
drivers/usb/dwc3/gadget.c static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) · line 3155
Quoted 8 lines
drivers/usb/dwc3/gadget.c +2
-0@@ -3100,6 +3148,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) }dwc3_reset_gadget (dwc); +/* Stop any active/pending transfers when receiving bus reset */ +dwc3_stop_active_transfers (dwc); reg =dwc3_readl (dwc->regs,DWC3_DCTL ); reg &= ~DWC3_DCTL_TSTCTRL_MASK ;
Looks good to me.
Thanks,
Thinh
Quoted 59 lines · Thinh Nguyen
Quoted 58 lines · Wesley Cheng
In the DWC3 databook, for a device initiated disconnect or bus reset, the driver is required to send dependxfer commands for any pending transfers. In addition, before the controller can move to the halted state, the SW needs to acknowledge any pending events. If the controller is not halted properly, there is a chance the controller will continue accessing stale or freed TRBs and buffers.
Signed-off-by Wesley Cheng <[email protected]>---
Changes in v2:
- Moved cleanup code to thepullup()API to differentiate between device disconnect and hibernation.
- Added cleanup code to the bus reset case as well.
- Verified the move topullup()did not reproduce the problen using the same test sequence.Verified fix by adding a check for ETIMEDOUT during the run stop call. Shell script writing to the configfs UDC file to trigger disconnect and connect. Batch script to have PC execute data transfers over adb (ie adb push) After a few iterations, we'd run into a scenario where the controller wasn't halted. With the following change, no failed halts after many iterations.
--- drivers/usb/dwc3/ep0.c | 2 +- drivers/usb/dwc3/gadget.c | 52 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 2 deletions(-)drivers/usb/dwc3/ep0.c +1
-1@@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,int ret;spin_lock_irqsave (&dwc->lock, flags); -if (!dep->endpoint.desc) { +if (!dep->endpoint.desc || !dwc->pullups_connected) {dev_err (dwc->dev,"%s: can't queue to disabled endpoint\n" , dep->name); ret = -ESHUTDOWN ;drivers/usb/dwc3/gadget.c +1
-1@@ -1516,7 +1516,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) {struct dwc3 *dwc = dep->dwc; -if (!dep->endpoint.desc) { +if (!dep->endpoint.desc || !dwc->pullups_connected) {dev_err (dwc->dev,"%s: can't queue to disabled endpoint\n" , dep->name);return -ESHUTDOWN ;@@ -1926,6 +1926,24 @@ static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,return 0 ; }
Quoted 17 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_set_selfpowered(struct usb_gadget *g, · line 1941
Quoted 13 lines
drivers/usb/dwc3/gadget.c +13
-0+static void dwc3_stop_active_transfers (struct dwc3 *dwc) +{ +u32 epnum; + +for (epnum =2 ; epnum <DWC3_ENDPOINTS_NUM ; epnum++) { +struct dwc3_ep *dep; + + dep = dwc->eps[epnum]; +if (!dep) +continue ; + +if (!(dep->flags &DWC3_EP_ENABLED )) +continue ;Don't do the enabled check here. Let the
dwc3_stop_active_transfer()do that checking.
Hi Thinh,
Thanks for the detailed review, as always. Got it, we can allow that to catch it based off the DWC3_EP_TRANSFER_STARTED.
Quoted 12 lines
Quoted 12 lines
drivers/usb/dwc3/gadget.c +5
-0+ +dwc3_remove_requests (dwc, dep); + } +} +static int dwc3_gadget_run_stop (struct dwc3 *dwc,int is_on,int suspend) {u32 reg;@@ -1994,9 +2012,39 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) } }
Quoted 10 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2019
Quoted 5 lines
drivers/usb/dwc3/gadget.c +5
-0+/* +* Synchronize and disable any further event handling while controller +* is being enabled/disabled. +*/ +disable_irq (dwc->irq_gadget);I think it's better to do
dwc3_gadget_disable_irq(). This only stops handling events. Although it's unlikely, the controller may still generate events before it's halted.
I think its better if we can do both. At least with the disable_irq() call present, we can ensure the irq handlers are complete, or we can do as Felipe suggested, and first disable the controller events (using dwc3_gadget_disable_irq()) and then calling synchronize_irq().
The concern I had is the pullup() API updating the lpos, and the hardirq handler referencing it to update the evt buf cache and waking up the threaded irq handler. (since we don't clear the evt->count explicitly, it may reference empty/stale events)
Quoted 12 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2027
Quoted 8 lines
drivers/usb/dwc3/gadget.c +7
-0spin_lock_irqsave (&dwc->lock, flags); + +/* Controller is not halted until pending events are acknowledged */ +if (!is_on) { +u32 reg; + +__dwc3_gadget_ep_disable (dwc->eps[0 ]); +__dwc3_gadget_ep_disable (dwc->eps[1 ]);You can just do
__dwc3_gadget_stop(), and do that afterdwc3_stop_active_transfers().
Got it.
Quoted 5 lines
Quoted 5 lines
drivers/usb/dwc3/gadget.c +5
-0+ +/* +* The databook explicitly mentions for a device-initiated +* disconnect sequence, the SW needs to ensure that it ends any +* active transfers.
Quoted 9 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2037
Quoted 5 lines
drivers/usb/dwc3/gadget.c +5
-0+*/ +dwc3_stop_active_transfers (dwc); + + reg =dwc3_readl (dwc->regs,DWC3_GEVNTCOUNT (0 )); + reg &=DWC3_GEVNTCOUNT_MASK ;Can we use another variable "count" instead of reusing reg to make it a little clearer?
Sure, I'll add another variable.
Thanks
Wesley
Quoted 28 lines
Quoted 14 lines
drivers/usb/dwc3/gadget.c +8
-0+if (reg >0 ) { +dwc3_writel (dwc->regs,DWC3_GEVNTCOUNT (0 ), reg); + dwc->ev_buf->lpos = (dwc->ev_buf->lpos + reg) % + dwc->ev_buf->length; + } + } + ret =dwc3_gadget_run_stop (dwc, is_on, false);spin_unlock_irqrestore (&dwc->lock, flags); +enable_irq (dwc->irq_gadget);return ret; }drivers/usb/dwc3/gadget.c static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) · line 3155
Quoted 8 lines
drivers/usb/dwc3/gadget.c +2
-0@@ -3100,6 +3148,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) }dwc3_reset_gadget (dwc); +/* Stop any active/pending transfers when receiving bus reset */ +dwc3_stop_active_transfers (dwc); reg =dwc3_readl (dwc->regs,DWC3_DCTL ); reg &= ~DWC3_DCTL_TSTCTRL_MASK ;Looks good to me.
Thanks,
Thinh
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Quoted 131 lines · Wesley Cheng
Quoted 59 lines · Thinh Nguyen
Quoted 58 lines · Wesley Cheng
In the DWC3 databook, for a device initiated disconnect or bus reset, the driver is required to send dependxfer commands for any pending transfers. In addition, before the controller can move to the halted state, the SW needs to acknowledge any pending events. If the controller is not halted properly, there is a chance the controller will continue accessing stale or freed TRBs and buffers.
Signed-off-by Wesley Cheng <[email protected]>---
Changes in v2:
- Moved cleanup code to thepullup()API to differentiate between device disconnect and hibernation.
- Added cleanup code to the bus reset case as well.
- Verified the move topullup()did not reproduce the problen using the same test sequence.Verified fix by adding a check for ETIMEDOUT during the run stop call. Shell script writing to the configfs UDC file to trigger disconnect and connect. Batch script to have PC execute data transfers over adb (ie adb push) After a few iterations, we'd run into a scenario where the controller wasn't halted. With the following change, no failed halts after many iterations.
--- drivers/usb/dwc3/ep0.c | 2 +- drivers/usb/dwc3/gadget.c | 52 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 2 deletions(-)drivers/usb/dwc3/ep0.c +1
-1@@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,int ret;spin_lock_irqsave (&dwc->lock, flags); -if (!dep->endpoint.desc) { +if (!dep->endpoint.desc || !dwc->pullups_connected) {dev_err (dwc->dev,"%s: can't queue to disabled endpoint\n" , dep->name); ret = -ESHUTDOWN ;drivers/usb/dwc3/gadget.c +1
-1@@ -1516,7 +1516,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) {struct dwc3 *dwc = dep->dwc; -if (!dep->endpoint.desc) { +if (!dep->endpoint.desc || !dwc->pullups_connected) {dev_err (dwc->dev,"%s: can't queue to disabled endpoint\n" , dep->name);return -ESHUTDOWN ;@@ -1926,6 +1926,24 @@ static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,return 0 ; }Quoted 17 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_set_selfpowered(struct usb_gadget *g, · line 1941
Quoted 13 lines
drivers/usb/dwc3/gadget.c +13
-0+static void dwc3_stop_active_transfers (struct dwc3 *dwc) +{ +u32 epnum; + +for (epnum =2 ; epnum <DWC3_ENDPOINTS_NUM ; epnum++) { +struct dwc3_ep *dep; + + dep = dwc->eps[epnum]; +if (!dep) +continue ; + +if (!(dep->flags &DWC3_EP_ENABLED )) +continue ;Don't do the enabled check here. Let the
dwc3_stop_active_transfer()do that checking.Hi Thinh,
Thanks for the detailed review, as always. Got it, we can allow that to catch it based off the
DWC3_EP_TRANSFER_STARTED.Quoted 12 lines
Quoted 12 lines
drivers/usb/dwc3/gadget.c +5
-0+ +dwc3_remove_requests (dwc, dep); + } +} +static int dwc3_gadget_run_stop (struct dwc3 *dwc,int is_on,int suspend) {u32 reg;@@ -1994,9 +2012,39 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) } }Quoted 10 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2019
Quoted 5 lines
drivers/usb/dwc3/gadget.c +5
-0+/* +* Synchronize and disable any further event handling while controller +* is being enabled/disabled. +*/ +disable_irq (dwc->irq_gadget);I think it's better to do
dwc3_gadget_disable_irq(). This only stops handling events. Although it's unlikely, the controller may still generate events before it's halted.I think its better if we can do both. At least with the
disable_irq()call present, we can ensure the irq handlers are complete, or we can do as Felipe suggested, and first disable the controller events (usingdwc3_gadget_disable_irq()) and then callingsynchronize_irq().The concern I had is the
pullup()API updating the lpos, and the hardirq handler referencing it to update the evt buf cache and waking up the threaded irq handler. (since we don't clear theevt->countexplicitly, it may reference empty/stale events)Quoted 12 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2027
Quoted 8 lines
drivers/usb/dwc3/gadget.c +7
-0spin_lock_irqsave (&dwc->lock, flags); + +/* Controller is not halted until pending events are acknowledged */ +if (!is_on) { +u32 reg; + +__dwc3_gadget_ep_disable (dwc->eps[0 ]); +__dwc3_gadget_ep_disable (dwc->eps[1 ]);You can just do
__dwc3_gadget_stop(), and do that afterdwc3_stop_active_transfers().
Quoted 2 lines
Got it.
Hi Thinh,
Maybe we can ignore calling dwc3_gadget_disable_irq() separately if we are going to use __dwc3_gadget_stop(), since gadget stop will call dwc3_gadget_disable_irq(). Also, it would be executed before the event count clearing, so if there was an event (unlikely) that was generated, we would discard it.
Something like:
disable_irq(dwc->irq_gadget);
spin_lock_irqsave(&dwc->lock, flags);
if (!is_on) {
u32 count;
dwc3_stop_active_transfers(dwc);
__dwc3_gadget_stop(dwc);
count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
count &= DWC3_GEVNTCOUNT_MASK;Thanks
Wesley
Quoted 49 lines
Quoted 5 lines
Quoted 5 lines
drivers/usb/dwc3/gadget.c +5
-0+ +/* +* The databook explicitly mentions for a device-initiated +* disconnect sequence, the SW needs to ensure that it ends any +* active transfers. Quoted 9 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2037
Quoted 5 lines
drivers/usb/dwc3/gadget.c +5
-0+*/ +dwc3_stop_active_transfers (dwc); + + reg =dwc3_readl (dwc->regs,DWC3_GEVNTCOUNT (0 )); + reg &=DWC3_GEVNTCOUNT_MASK ;Can we use another variable "count" instead of reusing reg to make it a little clearer?
Sure, I'll add another variable.
Thanks
WesleyQuoted 28 lines
Quoted 14 lines
drivers/usb/dwc3/gadget.c +8
-0+if (reg >0 ) { +dwc3_writel (dwc->regs,DWC3_GEVNTCOUNT (0 ), reg); + dwc->ev_buf->lpos = (dwc->ev_buf->lpos + reg) % + dwc->ev_buf->length; + } + } + ret =dwc3_gadget_run_stop (dwc, is_on, false);spin_unlock_irqrestore (&dwc->lock, flags); +enable_irq (dwc->irq_gadget);return ret; }drivers/usb/dwc3/gadget.c static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) · line 3155
Quoted 8 lines
drivers/usb/dwc3/gadget.c +2
-0@@ -3100,6 +3148,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc) }dwc3_reset_gadget (dwc); +/* Stop any active/pending transfers when receiving bus reset */ +dwc3_stop_active_transfers (dwc); reg =dwc3_readl (dwc->regs,DWC3_DCTL ); reg &= ~DWC3_DCTL_TSTCTRL_MASK ;Looks good to me.
Thanks,
Thinh
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Thinh Nguyen
maintainerQuoted 110 lines · Wesley Cheng
Quoted 108 lines · Wesley Cheng
Quoted 59 lines · Thinh Nguyen
Quoted 58 lines · Wesley Cheng
In the DWC3 databook, for a device initiated disconnect or bus reset, the driver is required to send dependxfer commands for any pending transfers. In addition, before the controller can move to the halted state, the SW needs to acknowledge any pending events. If the controller is not halted properly, there is a chance the controller will continue accessing stale or freed TRBs and buffers.
Signed-off-by Wesley Cheng <[email protected]>---
Changes in v2:
- Moved cleanup code to thepullup()API to differentiate between device disconnect and hibernation.
- Added cleanup code to the bus reset case as well.
- Verified the move topullup()did not reproduce the problen using the same test sequence.Verified fix by adding a check for ETIMEDOUT during the run stop call. Shell script writing to the configfs UDC file to trigger disconnect and connect. Batch script to have PC execute data transfers over adb (ie adb push) After a few iterations, we'd run into a scenario where the controller wasn't halted. With the following change, no failed halts after many iterations.
--- drivers/usb/dwc3/ep0.c | 2 +- drivers/usb/dwc3/gadget.c | 52 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 2 deletions(-)drivers/usb/dwc3/ep0.c +1
-1@@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,int ret;spin_lock_irqsave (&dwc->lock, flags); -if (!dep->endpoint.desc) { +if (!dep->endpoint.desc || !dwc->pullups_connected) {dev_err (dwc->dev,"%s: can't queue to disabled endpoint\n" , dep->name); ret = -ESHUTDOWN ;drivers/usb/dwc3/gadget.c +1
-1@@ -1516,7 +1516,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) {struct dwc3 *dwc = dep->dwc; -if (!dep->endpoint.desc) { +if (!dep->endpoint.desc || !dwc->pullups_connected) {dev_err (dwc->dev,"%s: can't queue to disabled endpoint\n" , dep->name);return -ESHUTDOWN ;@@ -1926,6 +1926,24 @@ static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,return 0 ; }Quoted 16 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_set_selfpowered(struct usb_gadget *g, · line 1941
Quoted 13 lines
drivers/usb/dwc3/gadget.c +13
-0+static void dwc3_stop_active_transfers (struct dwc3 *dwc) +{ +u32 epnum; + +for (epnum =2 ; epnum <DWC3_ENDPOINTS_NUM ; epnum++) { +struct dwc3_ep *dep; + + dep = dwc->eps[epnum]; +if (!dep) +continue ; + +if (!(dep->flags &DWC3_EP_ENABLED )) +continue ;Don't do the enabled check here. Let the
dwc3_stop_active_transfer()do that checking.Hi Thinh,
Thanks for the detailed review, as always. Got it, we can allow that to catch it based off the
DWC3_EP_TRANSFER_STARTED.Quoted 12 lines
Quoted 12 lines
drivers/usb/dwc3/gadget.c +5
-0+ +dwc3_remove_requests (dwc, dep); + } +} +static int dwc3_gadget_run_stop (struct dwc3 *dwc,int is_on,int suspend) {u32 reg;@@ -1994,9 +2012,39 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) } }Quoted 9 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2019
Quoted 5 lines
drivers/usb/dwc3/gadget.c +5
-0+/* +* Synchronize and disable any further event handling while controller +* is being enabled/disabled. +*/ +disable_irq (dwc->irq_gadget);I think it's better to do
dwc3_gadget_disable_irq(). This only stops handling events. Although it's unlikely, the controller may still generate events before it's halted.I think its better if we can do both. At least with the
disable_irq()call present, we can ensure the irq handlers are complete, or we can do as Felipe suggested, and first disable the controller events (usingdwc3_gadget_disable_irq()) and then callingsynchronize_irq().
Quoted 4 lines
Quoted 4 lines
The concern I had is the
pullup()API updating the lpos, and the hardirq handler referencing it to update the evt buf cache and waking up the threaded irq handler. (since we don't clear theevt->countexplicitly, it may reference empty/stale events)
I see, then we should do both.
Quoted 14 lines
Quoted 12 lines
Quoted 12 lines
drivers/usb/dwc3/gadget.c static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on) · line 2027
Quoted 9 lines
drivers/usb/dwc3/gadget.c +7
-0spin_lock_irqsave (&dwc->lock, flags); + +/* Controller is not halted until pending events are acknowledged */ +if (!is_on) { +u32 reg; + +__dwc3_gadget_ep_disable (dwc->eps[0 ]); +__dwc3_gadget_ep_disable (dwc->eps[1 ]);You can just do
__dwc3_gadget_stop(), and do that afterdwc3_stop_active_transfers().Quoted 2 lines
Got it.
Quoted 7 lines
Hi Thinh,
Maybe we can ignore calling
dwc3_gadget_disable_irq()separately if we are going to use__dwc3_gadget_stop(), since gadget stop will calldwc3_gadget_disable_irq(). Also, it would be executed before the event count clearing, so if there was an event (unlikely) that was generated, we would discard it.
That was my intention. Just make sure to re-enable the events on pull_up(on)
Quoted 15 lines
Something like:
disable_irq(dwc->irq_gadget); spin_lock_irqsave(&dwc->lock, flags); if (!is_on) { u32 count; dwc3_stop_active_transfers(dwc); __dwc3_gadget_stop(dwc); count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0)); count &= DWC3_GEVNTCOUNT_MASK;
Quoted 4 lines
Thanks
Wesley
Thanks,
Thinh