Currently gadget_wakeup() waits for U0 synchronously if it was
called from func_wakeup(), this is because we need to send the
function wakeup command soon after the link is active. And the
call is made synchronous by polling DSTS continuosly for 20000
times in __dwc3_gadget_wakeup(). But it observed that sometimes
the link is not active even after polling 20K times, leading to
remote wakeup failures. Adding a small delay between each poll
helps, but that won't guarantee resolution in future. Hence make
the gadget_wakeup completely asynchronous.
Since multiple interfaces can issue a function wakeup at once,
add a new variable func_wakeup_pending which will indicate the
functions that has issued func_wakup, this is represented in a
bitmap format. If the link is in U3, dwc3_gadget_func_wakeup()
will set the bit corresponding to interface_id and bail out.
Once link comes back to U0, linksts_change irq is triggered,
where the function wakeup command is sent based on bitmap.
Cc: stable@kernel.org
Fixes: 92c08a84b53e ("usb: dwc3: Add function suspend and function wakeup support")
Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
---
drivers/usb/dwc3/core.h | 4 +++
drivers/usb/dwc3/gadget.c | 60 ++++++++++++++++-----------------------
2 files changed, 28 insertions(+), 36 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index aaa39e663f60..2cdbbd3236d7 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1164,6 +1164,9 @@ struct dwc3_scratchpad_array {
* @gsbuscfg0_reqinfo: store GSBUSCFG0.DATRDREQINFO, DESRDREQINFO,
* DATWRREQINFO, and DESWRREQINFO value passed from
* glue driver.
+ * @func_wakeup_pending: Indicates whether any interface has requested for
+ * function wakeup. Also represents the interface_id
+ * using bitmap.
*/
struct dwc3 {
struct work_struct drd_work;
@@ -1394,6 +1397,7 @@ struct dwc3 {
int num_ep_resized;
struct dentry *debug_root;
u32 gsbuscfg0_reqinfo;
+ u32 func_wakeup_pending;
};
#define INCRX_BURST_MODE 0
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 89a4dc8ebf94..3289e57471f4 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -276,8 +276,6 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
return ret;
}
-static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async);
-
/**
* dwc3_send_gadget_ep_cmd - issue an endpoint command
* @dep: the endpoint to which the command is going to be issued
@@ -2351,10 +2349,8 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g)
return __dwc3_gadget_get_frame(dwc);
}
-static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
+static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
{
- int retries;
-
int ret;
u32 reg;
@@ -2382,8 +2378,7 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
return -EINVAL;
}
- if (async)
- dwc3_gadget_enable_linksts_evts(dwc, true);
+ dwc3_gadget_enable_linksts_evts(dwc, true);
ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
if (ret < 0) {
@@ -2404,25 +2399,6 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
* Since link status change events are enabled we will receive
* an U0 event when wakeup is successful. So bail out.
*/
- if (async)
- return 0;
-
- /* poll until Link State changes to ON */
- retries = 20000;
-
- while (retries--) {
- reg = dwc3_readl(dwc->regs, DWC3_DSTS);
-
- /* in HS, means ON */
- if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U0)
- break;
- }
-
- if (DWC3_DSTS_USBLNKST(reg) != DWC3_LINK_STATE_U0) {
- dev_err(dwc->dev, "failed to send remote wakeup\n");
- return -EINVAL;
- }
-
return 0;
}
@@ -2443,7 +2419,7 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
spin_unlock_irqrestore(&dwc->lock, flags);
return -EINVAL;
}
- ret = __dwc3_gadget_wakeup(dwc, true);
+ ret = __dwc3_gadget_wakeup(dwc);
spin_unlock_irqrestore(&dwc->lock, flags);
@@ -2471,14 +2447,10 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
*/
link_state = dwc3_gadget_get_link_state(dwc);
if (link_state == DWC3_LINK_STATE_U3) {
- ret = __dwc3_gadget_wakeup(dwc, false);
- if (ret) {
- spin_unlock_irqrestore(&dwc->lock, flags);
- return -EINVAL;
- }
- dwc3_resume_gadget(dwc);
- dwc->suspended = false;
- dwc->link_state = DWC3_LINK_STATE_U0;
+ dwc->func_wakeup_pending |= BIT(intf_id);
+ ret = __dwc3_gadget_wakeup(dwc);
+ spin_unlock_irqrestore(&dwc->lock, flags);
+ return ret;
}
ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
@@ -4300,6 +4272,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
{
enum dwc3_link_state next = evtinfo & DWC3_LINK_STATE_MASK;
unsigned int pwropt;
+ int ret, intf_id = 0;
/*
* WORKAROUND: DWC3 < 2.50a have an issue when configured without
@@ -4375,7 +4348,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
switch (next) {
case DWC3_LINK_STATE_U0:
- if (dwc->gadget->wakeup_armed) {
+ if (dwc->gadget->wakeup_armed || dwc->func_wakeup_pending) {
dwc3_gadget_enable_linksts_evts(dwc, false);
dwc3_resume_gadget(dwc);
dwc->suspended = false;
@@ -4398,6 +4371,21 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
}
dwc->link_state = next;
+
+ /* Proceed with func wakeup if any interfaces that has requested */
+ while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
+ if (dwc->func_wakeup_pending & BIT(0)) {
+ ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
+ DWC3_DGCMDPAR_DN_FUNC_WAKE |
+ DWC3_DGCMDPAR_INTF_SEL(intf_id));
+ if (ret)
+ dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
+ intf_id, ret);
+ }
+ dwc->func_wakeup_pending >>= 1;
+ intf_id++;
+ }
+
}
static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
--
2.25.1On Thu, Apr 03, 2025, Prashanth K wrote:
> Currently gadget_wakeup() waits for U0 synchronously if it was
> called from func_wakeup(), this is because we need to send the
> function wakeup command soon after the link is active. And the
> call is made synchronous by polling DSTS continuosly for 20000
> times in __dwc3_gadget_wakeup(). But it observed that sometimes
> the link is not active even after polling 20K times, leading to
> remote wakeup failures. Adding a small delay between each poll
> helps, but that won't guarantee resolution in future. Hence make
> the gadget_wakeup completely asynchronous.
>
> Since multiple interfaces can issue a function wakeup at once,
> add a new variable func_wakeup_pending which will indicate the
> functions that has issued func_wakup, this is represented in a
> bitmap format. If the link is in U3, dwc3_gadget_func_wakeup()
> will set the bit corresponding to interface_id and bail out.
> Once link comes back to U0, linksts_change irq is triggered,
> where the function wakeup command is sent based on bitmap.
>
> Cc: stable@kernel.org
> Fixes: 92c08a84b53e ("usb: dwc3: Add function suspend and function wakeup support")
> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
> ---
> drivers/usb/dwc3/core.h | 4 +++
> drivers/usb/dwc3/gadget.c | 60 ++++++++++++++++-----------------------
> 2 files changed, 28 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index aaa39e663f60..2cdbbd3236d7 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1164,6 +1164,9 @@ struct dwc3_scratchpad_array {
> * @gsbuscfg0_reqinfo: store GSBUSCFG0.DATRDREQINFO, DESRDREQINFO,
> * DATWRREQINFO, and DESWRREQINFO value passed from
> * glue driver.
> + * @func_wakeup_pending: Indicates whether any interface has requested for
> + * function wakeup. Also represents the interface_id
> + * using bitmap.
> */
> struct dwc3 {
> struct work_struct drd_work;
> @@ -1394,6 +1397,7 @@ struct dwc3 {
> int num_ep_resized;
> struct dentry *debug_root;
> u32 gsbuscfg0_reqinfo;
> + u32 func_wakeup_pending;
Can we rename this to wakeup_pending_funcs to not be mixed with bitmap
vs boolean?
> };
>
> #define INCRX_BURST_MODE 0
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 89a4dc8ebf94..3289e57471f4 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -276,8 +276,6 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
> return ret;
> }
>
> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async);
> -
> /**
> * dwc3_send_gadget_ep_cmd - issue an endpoint command
> * @dep: the endpoint to which the command is going to be issued
> @@ -2351,10 +2349,8 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g)
> return __dwc3_gadget_get_frame(dwc);
> }
>
> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
> {
> - int retries;
> -
> int ret;
> u32 reg;
>
> @@ -2382,8 +2378,7 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
> return -EINVAL;
> }
>
> - if (async)
> - dwc3_gadget_enable_linksts_evts(dwc, true);
> + dwc3_gadget_enable_linksts_evts(dwc, true);
>
> ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
> if (ret < 0) {
> @@ -2404,25 +2399,6 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
> * Since link status change events are enabled we will receive
> * an U0 event when wakeup is successful. So bail out.
> */
> - if (async)
> - return 0;
> -
> - /* poll until Link State changes to ON */
> - retries = 20000;
> -
> - while (retries--) {
> - reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> -
> - /* in HS, means ON */
> - if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U0)
> - break;
> - }
> -
> - if (DWC3_DSTS_USBLNKST(reg) != DWC3_LINK_STATE_U0) {
> - dev_err(dwc->dev, "failed to send remote wakeup\n");
> - return -EINVAL;
> - }
> -
> return 0;
> }
>
> @@ -2443,7 +2419,7 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
> spin_unlock_irqrestore(&dwc->lock, flags);
> return -EINVAL;
> }
> - ret = __dwc3_gadget_wakeup(dwc, true);
> + ret = __dwc3_gadget_wakeup(dwc);
>
> spin_unlock_irqrestore(&dwc->lock, flags);
>
> @@ -2471,14 +2447,10 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
> */
> link_state = dwc3_gadget_get_link_state(dwc);
> if (link_state == DWC3_LINK_STATE_U3) {
> - ret = __dwc3_gadget_wakeup(dwc, false);
> - if (ret) {
> - spin_unlock_irqrestore(&dwc->lock, flags);
> - return -EINVAL;
> - }
> - dwc3_resume_gadget(dwc);
> - dwc->suspended = false;
> - dwc->link_state = DWC3_LINK_STATE_U0;
> + dwc->func_wakeup_pending |= BIT(intf_id);
> + ret = __dwc3_gadget_wakeup(dwc);
> + spin_unlock_irqrestore(&dwc->lock, flags);
> + return ret;
> }
>
> ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
> @@ -4300,6 +4272,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> {
> enum dwc3_link_state next = evtinfo & DWC3_LINK_STATE_MASK;
> unsigned int pwropt;
> + int ret, intf_id = 0;
Can we keep declarations in separate lines?
>
> /*
> * WORKAROUND: DWC3 < 2.50a have an issue when configured without
> @@ -4375,7 +4348,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>
> switch (next) {
> case DWC3_LINK_STATE_U0:
> - if (dwc->gadget->wakeup_armed) {
> + if (dwc->gadget->wakeup_armed || dwc->func_wakeup_pending) {
> dwc3_gadget_enable_linksts_evts(dwc, false);
> dwc3_resume_gadget(dwc);
> dwc->suspended = false;
> @@ -4398,6 +4371,21 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> }
>
> dwc->link_state = next;
> +
> + /* Proceed with func wakeup if any interfaces that has requested */
> + while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
> + if (dwc->func_wakeup_pending & BIT(0)) {
> + ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
> + DWC3_DGCMDPAR_DN_FUNC_WAKE |
> + DWC3_DGCMDPAR_INTF_SEL(intf_id));
> + if (ret)
> + dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
> + intf_id, ret);
> + }
> + dwc->func_wakeup_pending >>= 1;
This would break the bitmap of dwc->func_wakeup_pending. Perhaps we can
use ffs(x) to properly find and clear the interface ID from the bitmap
one at a time.
> + intf_id++;
> + }
> +
> }
>
> static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
> --
> 2.25.1
>
Thanks,
Thinh
On 08-04-25 05:08 am, Thinh Nguyen wrote:
> On Thu, Apr 03, 2025, Prashanth K wrote:
>> Currently gadget_wakeup() waits for U0 synchronously if it was
>> called from func_wakeup(), this is because we need to send the
>> function wakeup command soon after the link is active. And the
>> call is made synchronous by polling DSTS continuosly for 20000
>> times in __dwc3_gadget_wakeup(). But it observed that sometimes
>> the link is not active even after polling 20K times, leading to
>> remote wakeup failures. Adding a small delay between each poll
>> helps, but that won't guarantee resolution in future. Hence make
>> the gadget_wakeup completely asynchronous.
>>
>> Since multiple interfaces can issue a function wakeup at once,
>> add a new variable func_wakeup_pending which will indicate the
>> functions that has issued func_wakup, this is represented in a
>> bitmap format. If the link is in U3, dwc3_gadget_func_wakeup()
>> will set the bit corresponding to interface_id and bail out.
>> Once link comes back to U0, linksts_change irq is triggered,
>> where the function wakeup command is sent based on bitmap.
>>
>> Cc: stable@kernel.org
>> Fixes: 92c08a84b53e ("usb: dwc3: Add function suspend and function wakeup support")
>> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
>> ---
>> drivers/usb/dwc3/core.h | 4 +++
>> drivers/usb/dwc3/gadget.c | 60 ++++++++++++++++-----------------------
>> 2 files changed, 28 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index aaa39e663f60..2cdbbd3236d7 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1164,6 +1164,9 @@ struct dwc3_scratchpad_array {
>> * @gsbuscfg0_reqinfo: store GSBUSCFG0.DATRDREQINFO, DESRDREQINFO,
>> * DATWRREQINFO, and DESWRREQINFO value passed from
>> * glue driver.
>> + * @func_wakeup_pending: Indicates whether any interface has requested for
>> + * function wakeup. Also represents the interface_id
>> + * using bitmap.
>> */
>> struct dwc3 {
>> struct work_struct drd_work;
>> @@ -1394,6 +1397,7 @@ struct dwc3 {
>> int num_ep_resized;
>> struct dentry *debug_root;
>> u32 gsbuscfg0_reqinfo;
>> + u32 func_wakeup_pending;
>
> Can we rename this to wakeup_pending_funcs to not be mixed with bitmap
> vs boolean?
>
ACK
>> };
>>
>> #define INCRX_BURST_MODE 0
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 89a4dc8ebf94..3289e57471f4 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -276,8 +276,6 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
>> return ret;
>> }
>>
>> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async);
>> -
>> /**
>> * dwc3_send_gadget_ep_cmd - issue an endpoint command
>> * @dep: the endpoint to which the command is going to be issued
>> @@ -2351,10 +2349,8 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g)
>> return __dwc3_gadget_get_frame(dwc);
>> }
>>
>> -static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>> +static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>> {
>> - int retries;
>> -
>> int ret;
>> u32 reg;
>>
>> @@ -2382,8 +2378,7 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>> return -EINVAL;
>> }
>>
>> - if (async)
>> - dwc3_gadget_enable_linksts_evts(dwc, true);
>> + dwc3_gadget_enable_linksts_evts(dwc, true);
>>
>> ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
>> if (ret < 0) {
>> @@ -2404,25 +2399,6 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>> * Since link status change events are enabled we will receive
>> * an U0 event when wakeup is successful. So bail out.
>> */
>> - if (async)
>> - return 0;
>> -
>> - /* poll until Link State changes to ON */
>> - retries = 20000;
>> -
>> - while (retries--) {
>> - reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>> -
>> - /* in HS, means ON */
>> - if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U0)
>> - break;
>> - }
>> -
>> - if (DWC3_DSTS_USBLNKST(reg) != DWC3_LINK_STATE_U0) {
>> - dev_err(dwc->dev, "failed to send remote wakeup\n");
>> - return -EINVAL;
>> - }
>> -
>> return 0;
>> }
>>
>> @@ -2443,7 +2419,7 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>> spin_unlock_irqrestore(&dwc->lock, flags);
>> return -EINVAL;
>> }
>> - ret = __dwc3_gadget_wakeup(dwc, true);
>> + ret = __dwc3_gadget_wakeup(dwc);
>>
>> spin_unlock_irqrestore(&dwc->lock, flags);
>>
>> @@ -2471,14 +2447,10 @@ static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
>> */
>> link_state = dwc3_gadget_get_link_state(dwc);
>> if (link_state == DWC3_LINK_STATE_U3) {
>> - ret = __dwc3_gadget_wakeup(dwc, false);
>> - if (ret) {
>> - spin_unlock_irqrestore(&dwc->lock, flags);
>> - return -EINVAL;
>> - }
>> - dwc3_resume_gadget(dwc);
>> - dwc->suspended = false;
>> - dwc->link_state = DWC3_LINK_STATE_U0;
>> + dwc->func_wakeup_pending |= BIT(intf_id);
>> + ret = __dwc3_gadget_wakeup(dwc);
>> + spin_unlock_irqrestore(&dwc->lock, flags);
>> + return ret;
>> }
>>
>> ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
>> @@ -4300,6 +4272,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>> {
>> enum dwc3_link_state next = evtinfo & DWC3_LINK_STATE_MASK;
>> unsigned int pwropt;
>> + int ret, intf_id = 0;
>
> Can we keep declarations in separate lines?
>
OK
>>
>> /*
>> * WORKAROUND: DWC3 < 2.50a have an issue when configured without
>> @@ -4375,7 +4348,7 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>>
>> switch (next) {
>> case DWC3_LINK_STATE_U0:
>> - if (dwc->gadget->wakeup_armed) {
>> + if (dwc->gadget->wakeup_armed || dwc->func_wakeup_pending) {
>> dwc3_gadget_enable_linksts_evts(dwc, false);
>> dwc3_resume_gadget(dwc);
>> dwc->suspended = false;
>> @@ -4398,6 +4371,21 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>> }
>>
>> dwc->link_state = next;
>> +
>> + /* Proceed with func wakeup if any interfaces that has requested */
>> + while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
>> + if (dwc->func_wakeup_pending & BIT(0)) {
>> + ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
>> + DWC3_DGCMDPAR_DN_FUNC_WAKE |
>> + DWC3_DGCMDPAR_INTF_SEL(intf_id));
>> + if (ret)
>> + dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
>> + intf_id, ret);
>> + }
>> + dwc->func_wakeup_pending >>= 1;
>
> This would break the bitmap of dwc->func_wakeup_pending. Perhaps we can
> use ffs(x) to properly find and clear the interface ID from the bitmap
> one at a time.
>
Yes, we can use ffs(x). But I didn't understand how this would break
bitmap of dwc->func_wakeup_pending.
Regards,
Prashanth K
>> + intf_id++;
>> + }
>> +
>> }
>>
>> static void dwc3_gadget_suspend_interrupt(struct dwc3 *dwc,
>> --
>> 2.25.1
>>
>
> Thanks,
> Thinh
On Tue, Apr 08, 2025, Prashanth K wrote:
>
>
> On 08-04-25 05:08 am, Thinh Nguyen wrote:
> > On Thu, Apr 03, 2025, Prashanth K wrote:
> >> @@ -4398,6 +4371,21 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> >> }
> >>
> >> dwc->link_state = next;
> >> +
> >> + /* Proceed with func wakeup if any interfaces that has requested */
> >> + while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
> >> + if (dwc->func_wakeup_pending & BIT(0)) {
> >> + ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
> >> + DWC3_DGCMDPAR_DN_FUNC_WAKE |
> >> + DWC3_DGCMDPAR_INTF_SEL(intf_id));
> >> + if (ret)
> >> + dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
> >> + intf_id, ret);
> >> + }
> >> + dwc->func_wakeup_pending >>= 1;
> >
> > This would break the bitmap of dwc->func_wakeup_pending. Perhaps we can
> > use ffs(x) to properly find and clear the interface ID from the bitmap
> > one at a time.
> >
> Yes, we can use ffs(x). But I didn't understand how this would break
> bitmap of dwc->func_wakeup_pending.
>
Since you're sending device notification to all the wakeup armed
interfaces and not reusing the func_wakeup_pending afterward, the result
of what you're doing here will be the same.
IMHO, for clarity, what I was suggesting is to revise the logic to
preserve the dwc->func_wakeup_pending bitmap instead of shifting and
overwriting it, causing the bitmap to no longer match the intf_id. We
get the intf_id from the bitmap and clear the intf_id bit from the
bitmap as we go.
Thanks,
Thinh
On 09-04-25 06:13 am, Thinh Nguyen wrote:
> On Tue, Apr 08, 2025, Prashanth K wrote:
>>
>>
>> On 08-04-25 05:08 am, Thinh Nguyen wrote:
>>> On Thu, Apr 03, 2025, Prashanth K wrote:
>
>>>> @@ -4398,6 +4371,21 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>>>> }
>>>>
>>>> dwc->link_state = next;
>>>> +
>>>> + /* Proceed with func wakeup if any interfaces that has requested */
>>>> + while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
>>>> + if (dwc->func_wakeup_pending & BIT(0)) {
>>>> + ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
>>>> + DWC3_DGCMDPAR_DN_FUNC_WAKE |
>>>> + DWC3_DGCMDPAR_INTF_SEL(intf_id));
>>>> + if (ret)
>>>> + dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
>>>> + intf_id, ret);
>>>> + }
>>>> + dwc->func_wakeup_pending >>= 1;
>>>
>>> This would break the bitmap of dwc->func_wakeup_pending. Perhaps we can
>>> use ffs(x) to properly find and clear the interface ID from the bitmap
>>> one at a time.
>>>
>> Yes, we can use ffs(x). But I didn't understand how this would break
>> bitmap of dwc->func_wakeup_pending.
>>
>
> Since you're sending device notification to all the wakeup armed
> interfaces and not reusing the func_wakeup_pending afterward, the result
> of what you're doing here will be the same.
>
> IMHO, for clarity, what I was suggesting is to revise the logic to
> preserve the dwc->func_wakeup_pending bitmap instead of shifting and
> overwriting it, causing the bitmap to no longer match the intf_id. We
> get the intf_id from the bitmap and clear the intf_id bit from the
> bitmap as we go.
>
The above logic works, but as you suggested I'll refactor it using
ffs(x) and clear the intf_id directly (instead of shifting).
Does this look good?
/* Proceed with func wakeup if any interfaces that has requested */
while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
intf_id = ffs(dwc->func_wakeup_pending);
if (intf_id)
ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
DWC3_DGCMDPAR_DN_FUNC_WAKE |
DWC3_DGCMDPAR_INTF_SEL(intf_id - 1));
if (ret)
dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
intf_id, ret);
}
dwc->func_wakeup_pending &= ~(1 << (intf_id - 1));
}
Regards,
Prashanth K
On Wed, Apr 09, 2025, Prashanth K wrote:
>
>
> On 09-04-25 06:13 am, Thinh Nguyen wrote:
> > On Tue, Apr 08, 2025, Prashanth K wrote:
> >>
> >>
> >> On 08-04-25 05:08 am, Thinh Nguyen wrote:
> >>> On Thu, Apr 03, 2025, Prashanth K wrote:
> >
> >>>> @@ -4398,6 +4371,21 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> >>>> }
> >>>>
> >>>> dwc->link_state = next;
> >>>> +
> >>>> + /* Proceed with func wakeup if any interfaces that has requested */
> >>>> + while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
> >>>> + if (dwc->func_wakeup_pending & BIT(0)) {
> >>>> + ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
> >>>> + DWC3_DGCMDPAR_DN_FUNC_WAKE |
> >>>> + DWC3_DGCMDPAR_INTF_SEL(intf_id));
> >>>> + if (ret)
> >>>> + dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
> >>>> + intf_id, ret);
> >>>> + }
> >>>> + dwc->func_wakeup_pending >>= 1;
> >>>
> >>> This would break the bitmap of dwc->func_wakeup_pending. Perhaps we can
> >>> use ffs(x) to properly find and clear the interface ID from the bitmap
> >>> one at a time.
> >>>
> >> Yes, we can use ffs(x). But I didn't understand how this would break
> >> bitmap of dwc->func_wakeup_pending.
> >>
> >
> > Since you're sending device notification to all the wakeup armed
> > interfaces and not reusing the func_wakeup_pending afterward, the result
> > of what you're doing here will be the same.
> >
> > IMHO, for clarity, what I was suggesting is to revise the logic to
> > preserve the dwc->func_wakeup_pending bitmap instead of shifting and
> > overwriting it, causing the bitmap to no longer match the intf_id. We
> > get the intf_id from the bitmap and clear the intf_id bit from the
> > bitmap as we go.
> >
> The above logic works, but as you suggested I'll refactor it using
> ffs(x) and clear the intf_id directly (instead of shifting).
>
> Does this look good?
It looks great!
>
> /* Proceed with func wakeup if any interfaces that has requested */
> while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
> intf_id = ffs(dwc->func_wakeup_pending);
> if (intf_id)
> ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
> DWC3_DGCMDPAR_DN_FUNC_WAKE |
> DWC3_DGCMDPAR_INTF_SEL(intf_id - 1));
> if (ret)
> dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
> intf_id, ret);
> }
> dwc->func_wakeup_pending &= ~(1 << (intf_id - 1));
Some minor revision. How does this look? (not tested)
while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
intf_id = ffs(dwc->func_wakeup_pending) - 1;
ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
DWC3_DGCMDPAR_DN_FUNC_WAKE |
DWC3_DGCMDPAR_INTF_SEL(intf_id));
if (ret)
dev_err(dwc->dev, "Failed to send DN wake for intf %d\n", intf_id);
dwc->func_wakeup_pending &= ~BIT(intf_id);
}
Thanks,
Thinh
On 10-04-25 03:21 am, Thinh Nguyen wrote:
> On Wed, Apr 09, 2025, Prashanth K wrote:
>>
>>
>> On 09-04-25 06:13 am, Thinh Nguyen wrote:
>>> On Tue, Apr 08, 2025, Prashanth K wrote:
>>>>
>>>>
>>>> On 08-04-25 05:08 am, Thinh Nguyen wrote:
>>>>> On Thu, Apr 03, 2025, Prashanth K wrote:
>>>
>>>>>> @@ -4398,6 +4371,21 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>>>>>> }
>>>>>>
>>>>>> dwc->link_state = next;
>>>>>> +
>>>>>> + /* Proceed with func wakeup if any interfaces that has requested */
>>>>>> + while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
>>>>>> + if (dwc->func_wakeup_pending & BIT(0)) {
>>>>>> + ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
>>>>>> + DWC3_DGCMDPAR_DN_FUNC_WAKE |
>>>>>> + DWC3_DGCMDPAR_INTF_SEL(intf_id));
>>>>>> + if (ret)
>>>>>> + dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
>>>>>> + intf_id, ret);
>>>>>> + }
>>>>>> + dwc->func_wakeup_pending >>= 1;
>>>>>
>>>>> This would break the bitmap of dwc->func_wakeup_pending. Perhaps we can
>>>>> use ffs(x) to properly find and clear the interface ID from the bitmap
>>>>> one at a time.
>>>>>
>>>> Yes, we can use ffs(x). But I didn't understand how this would break
>>>> bitmap of dwc->func_wakeup_pending.
>>>>
>>>
>>> Since you're sending device notification to all the wakeup armed
>>> interfaces and not reusing the func_wakeup_pending afterward, the result
>>> of what you're doing here will be the same.
>>>
>>> IMHO, for clarity, what I was suggesting is to revise the logic to
>>> preserve the dwc->func_wakeup_pending bitmap instead of shifting and
>>> overwriting it, causing the bitmap to no longer match the intf_id. We
>>> get the intf_id from the bitmap and clear the intf_id bit from the
>>> bitmap as we go.
>>>
>> The above logic works, but as you suggested I'll refactor it using
>> ffs(x) and clear the intf_id directly (instead of shifting).
>>
>> Does this look good?
>
> It looks great!
>
>>
>> /* Proceed with func wakeup if any interfaces that has requested */
>> while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
>> intf_id = ffs(dwc->func_wakeup_pending);
>> if (intf_id)
>> ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
>> DWC3_DGCMDPAR_DN_FUNC_WAKE |
>> DWC3_DGCMDPAR_INTF_SEL(intf_id - 1));
>> if (ret)
>> dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
>> intf_id, ret);
>> }
>> dwc->func_wakeup_pending &= ~(1 << (intf_id - 1));
>
>
> Some minor revision. How does this look? (not tested)
>
> while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
> intf_id = ffs(dwc->func_wakeup_pending) - 1;
> ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
> DWC3_DGCMDPAR_DN_FUNC_WAKE |
> DWC3_DGCMDPAR_INTF_SEL(intf_id));
> if (ret)
> dev_err(dwc->dev, "Failed to send DN wake for intf %d\n", intf_id);
>
> dwc->func_wakeup_pending &= ~BIT(intf_id);
> }
>
> Thanks,
> Thinh
Good suggestion, ideally it should work, I'll test and add it in v2.
Thanks,
Prashanth K
© 2016 - 2025 Red Hat, Inc.