[PATCH v1 0/3] Fixes for USB3 CV Chapter 9.15 tests

[PATCH v1 0/3] Fixes for USB3 CV Chapter 9.15 tests

[Prashanth K]

While performing USB3 Command Verifier Chapter 9 tests, failures
were observed during 9.15 ("Function Remote Wakeup Enabled Test").
This is due to the following reasons,

1. Interfaces were incorrectly reporting as remote wakeup capable
   when host requested GET_STATUS.
2. Remote wakeup failures from DWC3 driver due to timeouts.

Address them through this series.

Prashanth K (3):
  usb: gadget: f_ecm: Add get_status callback
  usb: gadget: Use get_status callback to set remote wakeup capability
  usb: dwc3: gadget: Make gadget_wakeup asynchronous

 drivers/usb/dwc3/core.h             |  4 ++
 drivers/usb/dwc3/gadget.c           | 60 ++++++++++++-----------------
 drivers/usb/gadget/composite.c      | 12 ++----
 drivers/usb/gadget/function/f_ecm.c |  7 ++++
 4 files changed, 39 insertions(+), 44 deletions(-)

-- 
2.25.1

[PATCH v1 1/3] usb: gadget: f_ecm: Add get_status callback

[Prashanth K]

When host sends GET_STATUS to ECM interface, handle the request
from the function driver. Since the interface is wakeup capable,
set the corresponding bit, and set RW bit if the function is
already armed for wakeup by the host.

Cc: stable@kernel.org
Fixes: 481c225c4802 ("usb: gadget: Handle function suspend feature selector")
Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
---
 drivers/usb/gadget/function/f_ecm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
index 80841de845b0..027226325039 100644
--- a/drivers/usb/gadget/function/f_ecm.c
+++ b/drivers/usb/gadget/function/f_ecm.c
@@ -892,6 +892,12 @@ static void ecm_resume(struct usb_function *f)
 	gether_resume(&ecm->port);
 }
 
+static int ecm_get_status(struct usb_function *f)
+{
+	return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
+		USB_INTRF_STAT_FUNC_RW_CAP;
+}
+
 static void ecm_free(struct usb_function *f)
 {
 	struct f_ecm *ecm;
@@ -960,6 +966,7 @@ static struct usb_function *ecm_alloc(struct usb_function_instance *fi)
 	ecm->port.func.disable = ecm_disable;
 	ecm->port.func.free_func = ecm_free;
 	ecm->port.func.suspend = ecm_suspend;
+	ecm->port.func.get_status = ecm_get_status;
 	ecm->port.func.resume = ecm_resume;
 
 	return &ecm->port.func;
-- 
2.25.1

[PATCH v1 2/3] usb: gadget: Use get_status callback to set remote wakeup capability

[Prashanth K]

Currently when the host sends GET_STATUS request for an interface,
we use get_status callbacks to set/clear remote wakeup capability
of that interface. And if get_status callback isn't present for
that interface, then we assume its remote wakeup capability based
on bmAttributes.

Now consider a scenario, where we have a USB configuration with
multiple interfaces (say ECM + ADB), here ECM is remote wakeup
capable and as of now ADB isn't. And bmAttributes will indicate
the device as wakeup capable. With the current implementation,
when host sends GET_STATUS request for both interfaces, we will
set FUNC_RW_CAP for both. This results in USB3 CV Chapter 9.15
(Function Remote Wakeup Test) failures as host expects remote
wakeup from both interfaces.

The above scenario is just an example, and the failure can be
observed if we use configuration with any interface except ECM.
Hence avoid configuring remote wakeup capability from composite
driver based on bmAttributes, instead use get_status callbacks
and let the function drivers decide this.

Cc: stable@kernel.org
Fixes: 481c225c4802 ("usb: gadget: Handle function suspend feature selector")
Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
---
 drivers/usb/gadget/composite.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 869ad99afb48..5c6da360e95b 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2010,16 +2010,12 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
 			break;
 
 		if (f->get_status) {
-			status = f->get_status(f);
+			/* if D5 is not set, then device is not wakeup capable */
+			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP)
+				status = f->get_status(f);
+
 			if (status < 0)
 				break;
-		} else {
-			/* Set D0 and D1 bits based on func wakeup capability */
-			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
-				status |= USB_INTRF_STAT_FUNC_RW_CAP;
-				if (f->func_wakeup_armed)
-					status |= USB_INTRF_STAT_FUNC_RW;
-			}
 		}
 
 		put_unaligned_le16(status & 0x0000ffff, req->buf);
-- 
2.25.1

[PATCH v1 3/3] usb: dwc3: gadget: Make gadget_wakeup asynchronous

[Prashanth K]

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.1

Re: [PATCH v1 3/3] usb: dwc3: gadget: Make gadget_wakeup asynchronous

[Thinh Nguyen]

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?

>  };
>  
>  #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

Re: [PATCH v1 1/3] usb: gadget: f_ecm: Add get_status callback

[Thinh Nguyen]

On Thu, Apr 03, 2025, Prashanth K wrote:
> When host sends GET_STATUS to ECM interface, handle the request
> from the function driver. Since the interface is wakeup capable,
> set the corresponding bit, and set RW bit if the function is
> already armed for wakeup by the host.
> 
> Cc: stable@kernel.org
> Fixes: 481c225c4802 ("usb: gadget: Handle function suspend feature selector")
> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
> ---
>  drivers/usb/gadget/function/f_ecm.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
> index 80841de845b0..027226325039 100644
> --- a/drivers/usb/gadget/function/f_ecm.c
> +++ b/drivers/usb/gadget/function/f_ecm.c
> @@ -892,6 +892,12 @@ static void ecm_resume(struct usb_function *f)
>  	gether_resume(&ecm->port);
>  }
>  
> +static int ecm_get_status(struct usb_function *f)
> +{
> +	return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
> +		USB_INTRF_STAT_FUNC_RW_CAP;
> +}
> +

Why does USB_INTRF_STAT_FUNC_RW_CAP is set regardless of
USB_CONFIG_ATT_WAKEUP?

BR,
Thinh

>  static void ecm_free(struct usb_function *f)
>  {
>  	struct f_ecm *ecm;
> @@ -960,6 +966,7 @@ static struct usb_function *ecm_alloc(struct usb_function_instance *fi)
>  	ecm->port.func.disable = ecm_disable;
>  	ecm->port.func.free_func = ecm_free;
>  	ecm->port.func.suspend = ecm_suspend;
> +	ecm->port.func.get_status = ecm_get_status;
>  	ecm->port.func.resume = ecm_resume;
>  
>  	return &ecm->port.func;
> -- 
> 2.25.1
> 

Re: [PATCH v1 2/3] usb: gadget: Use get_status callback to set remote wakeup capability

[Thinh Nguyen]

On Thu, Apr 03, 2025, Prashanth K wrote:
> Currently when the host sends GET_STATUS request for an interface,
> we use get_status callbacks to set/clear remote wakeup capability
> of that interface. And if get_status callback isn't present for
> that interface, then we assume its remote wakeup capability based
> on bmAttributes.
> 
> Now consider a scenario, where we have a USB configuration with
> multiple interfaces (say ECM + ADB), here ECM is remote wakeup
> capable and as of now ADB isn't. And bmAttributes will indicate
> the device as wakeup capable. With the current implementation,
> when host sends GET_STATUS request for both interfaces, we will
> set FUNC_RW_CAP for both. This results in USB3 CV Chapter 9.15
> (Function Remote Wakeup Test) failures as host expects remote
> wakeup from both interfaces.
> 
> The above scenario is just an example, and the failure can be
> observed if we use configuration with any interface except ECM.
> Hence avoid configuring remote wakeup capability from composite
> driver based on bmAttributes, instead use get_status callbacks
> and let the function drivers decide this.
> 
> Cc: stable@kernel.org
> Fixes: 481c225c4802 ("usb: gadget: Handle function suspend feature selector")
> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
> ---
>  drivers/usb/gadget/composite.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 869ad99afb48..5c6da360e95b 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -2010,16 +2010,12 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>  			break;
>  
>  		if (f->get_status) {
> -			status = f->get_status(f);
> +			/* if D5 is not set, then device is not wakeup capable */
> +			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP)

We should allow function to execute get_status regardless of
USB_CONFIG_ATT_WAKEUP. There are other status beside wakeup.

> +				status = f->get_status(f);
> +
>  			if (status < 0)
>  				break;
> -		} else {
> -			/* Set D0 and D1 bits based on func wakeup capability */
> -			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
> -				status |= USB_INTRF_STAT_FUNC_RW_CAP;


So right now we're not able to configure the function to enable RW
capable? Perhaps we need to update the composite configfs for this?

BR,
Thinh

> -				if (f->func_wakeup_armed)
> -					status |= USB_INTRF_STAT_FUNC_RW;
> -			}
>  		}
>  
>  		put_unaligned_le16(status & 0x0000ffff, req->buf);
> -- 
> 2.25.1
> 

Re: [PATCH v1 1/3] usb: gadget: f_ecm: Add get_status callback

[Prashanth K]

On 08-04-25 06:38 am, Thinh Nguyen wrote:
> On Thu, Apr 03, 2025, Prashanth K wrote:
>> When host sends GET_STATUS to ECM interface, handle the request
>> from the function driver. Since the interface is wakeup capable,
>> set the corresponding bit, and set RW bit if the function is
>> already armed for wakeup by the host.
>>
>> Cc: stable@kernel.org
>> Fixes: 481c225c4802 ("usb: gadget: Handle function suspend feature selector")
>> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
>> ---
>>  drivers/usb/gadget/function/f_ecm.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
>> index 80841de845b0..027226325039 100644
>> --- a/drivers/usb/gadget/function/f_ecm.c
>> +++ b/drivers/usb/gadget/function/f_ecm.c
>> @@ -892,6 +892,12 @@ static void ecm_resume(struct usb_function *f)
>>  	gether_resume(&ecm->port);
>>  }
>>  
>> +static int ecm_get_status(struct usb_function *f)
>> +{
>> +	return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
>> +		USB_INTRF_STAT_FUNC_RW_CAP;
>> +}
>> +
> 
> Why does USB_INTRF_STAT_FUNC_RW_CAP is set regardless of
> USB_CONFIG_ATT_WAKEUP?
>
We check that in composite.c before calling get_status().
Have added some comments there [Patch v1 2/3] regarding same.

Thanks,
Prashanth K

Re: [PATCH v1 2/3] usb: gadget: Use get_status callback to set remote wakeup capability

[Prashanth K]

On 08-04-25 06:48 am, Thinh Nguyen wrote:
> On Thu, Apr 03, 2025, Prashanth K wrote:
>> Currently when the host sends GET_STATUS request for an interface,
>> we use get_status callbacks to set/clear remote wakeup capability
>> of that interface. And if get_status callback isn't present for
>> that interface, then we assume its remote wakeup capability based
>> on bmAttributes.
>>
>> Now consider a scenario, where we have a USB configuration with
>> multiple interfaces (say ECM + ADB), here ECM is remote wakeup
>> capable and as of now ADB isn't. And bmAttributes will indicate
>> the device as wakeup capable. With the current implementation,
>> when host sends GET_STATUS request for both interfaces, we will
>> set FUNC_RW_CAP for both. This results in USB3 CV Chapter 9.15
>> (Function Remote Wakeup Test) failures as host expects remote
>> wakeup from both interfaces.
>>
>> The above scenario is just an example, and the failure can be
>> observed if we use configuration with any interface except ECM.
>> Hence avoid configuring remote wakeup capability from composite
>> driver based on bmAttributes, instead use get_status callbacks
>> and let the function drivers decide this.
>>
>> Cc: stable@kernel.org
>> Fixes: 481c225c4802 ("usb: gadget: Handle function suspend feature selector")
>> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
>> ---
>>  drivers/usb/gadget/composite.c | 12 ++++--------
>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index 869ad99afb48..5c6da360e95b 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -2010,16 +2010,12 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>>  			break;
>>  
>>  		if (f->get_status) {
>> -			status = f->get_status(f);
>> +			/* if D5 is not set, then device is not wakeup capable */
>> +			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP)
> 
> We should allow function to execute get_status regardless of
> USB_CONFIG_ATT_WAKEUP. There are other status beside wakeup.
>
Agree with the first part, I also wanted to remove the explicit check
for USB_CONFIG_ATT_WAKEUP. But anyways kept it since only 2 bits (RW_CAP
and RW) are defined in the spec as the status of GetStatus for an Interface.

Lets do one thing, I'll rearrange it as follows

if (f->get_status) {
	status = f->get_status(f);
	
/* if D5 is not set, then device is not wakeup capable */
if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP)
	status &= ~(USB_INTRF_STAT_FUNC_RW_CAP | USB_INTRF_STAT_FUNC_RW);

>> +				status = f->get_status(f);
>> +
>>  			if (status < 0)
>>  				break;
>> -		} else {
>> -			/* Set D0 and D1 bits based on func wakeup capability */
>> -			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
>> -				status |= USB_INTRF_STAT_FUNC_RW_CAP;
> 
> 
> So right now we're not able to configure the function to enable RW
> capable? Perhaps we need to update the composite configfs for this?
> 

The removed code used to set USB_INTRF_STAT_FUNC_RW_CAP even for
interfaces which doesn't have RW capability. Its better to handle this
from function level than from composite.

Regards,
Prashanth K

Re: [PATCH v1 3/3] usb: dwc3: gadget: Make gadget_wakeup asynchronous

[Prashanth K]

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

Re: [PATCH v1 3/3] usb: dwc3: gadget: Make gadget_wakeup asynchronous

[Thinh Nguyen]

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

Re: [PATCH v1 2/3] usb: gadget: Use get_status callback to set remote wakeup capability

[Thinh Nguyen]

On Tue, Apr 08, 2025, Prashanth K wrote:
> 
> 
> On 08-04-25 06:48 am, Thinh Nguyen wrote:
> > On Thu, Apr 03, 2025, Prashanth K wrote:
> >> Currently when the host sends GET_STATUS request for an interface,
> >> we use get_status callbacks to set/clear remote wakeup capability
> >> of that interface. And if get_status callback isn't present for
> >> that interface, then we assume its remote wakeup capability based
> >> on bmAttributes.
> >>
> >> Now consider a scenario, where we have a USB configuration with
> >> multiple interfaces (say ECM + ADB), here ECM is remote wakeup
> >> capable and as of now ADB isn't. And bmAttributes will indicate
> >> the device as wakeup capable. With the current implementation,
> >> when host sends GET_STATUS request for both interfaces, we will
> >> set FUNC_RW_CAP for both. This results in USB3 CV Chapter 9.15
> >> (Function Remote Wakeup Test) failures as host expects remote
> >> wakeup from both interfaces.
> >>
> >> The above scenario is just an example, and the failure can be
> >> observed if we use configuration with any interface except ECM.
> >> Hence avoid configuring remote wakeup capability from composite
> >> driver based on bmAttributes, instead use get_status callbacks
> >> and let the function drivers decide this.
> >>
> >> Cc: stable@kernel.org
> >> Fixes: 481c225c4802 ("usb: gadget: Handle function suspend feature selector")
> >> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
> >> ---
> >>  drivers/usb/gadget/composite.c | 12 ++++--------
> >>  1 file changed, 4 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> >> index 869ad99afb48..5c6da360e95b 100644
> >> --- a/drivers/usb/gadget/composite.c
> >> +++ b/drivers/usb/gadget/composite.c
> >> @@ -2010,16 +2010,12 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
> >>  			break;
> >>  
> >>  		if (f->get_status) {
> >> -			status = f->get_status(f);
> >> +			/* if D5 is not set, then device is not wakeup capable */
> >> +			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP)
> > 
> > We should allow function to execute get_status regardless of
> > USB_CONFIG_ATT_WAKEUP. There are other status beside wakeup.
> >
> Agree with the first part, I also wanted to remove the explicit check
> for USB_CONFIG_ATT_WAKEUP. But anyways kept it since only 2 bits (RW_CAP
> and RW) are defined in the spec as the status of GetStatus for an Interface.
> 
> Lets do one thing, I'll rearrange it as follows
> 
> if (f->get_status) {
> 	status = f->get_status(f);
> 	
> /* if D5 is not set, then device is not wakeup capable */
> if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP)
> 	status &= ~(USB_INTRF_STAT_FUNC_RW_CAP | USB_INTRF_STAT_FUNC_RW);

Yes, something like this works, but I think you mean this:

	if (!(f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP))
		...

> 
> >> +				status = f->get_status(f);
> >> +
> >>  			if (status < 0)
> >>  				break;
> >> -		} else {
> >> -			/* Set D0 and D1 bits based on func wakeup capability */
> >> -			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
> >> -				status |= USB_INTRF_STAT_FUNC_RW_CAP;
> > 
> > 
> > So right now we're not able to configure the function to enable RW
> > capable? Perhaps we need to update the composite configfs for this?
> > 
> 
> The removed code used to set USB_INTRF_STAT_FUNC_RW_CAP even for
> interfaces which doesn't have RW capability. Its better to handle this
> from function level than from composite.
> 

Not at the gadget level, I mean to create a configfs attribute common
across different functions to allow the user to enable/disable the
function wakeup capability via the configfs when you setup the function.

What do you think?

Thanks,
Thinh

Re: [PATCH v1 1/3] usb: gadget: f_ecm: Add get_status callback

[Thinh Nguyen]

On Tue, Apr 08, 2025, Prashanth K wrote:
> 
> 
> On 08-04-25 06:38 am, Thinh Nguyen wrote:
> > On Thu, Apr 03, 2025, Prashanth K wrote:
> >> When host sends GET_STATUS to ECM interface, handle the request
> >> from the function driver. Since the interface is wakeup capable,
> >> set the corresponding bit, and set RW bit if the function is
> >> already armed for wakeup by the host.
> >>
> >> Cc: stable@kernel.org
> >> Fixes: 481c225c4802 ("usb: gadget: Handle function suspend feature selector")
> >> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
> >> ---
> >>  drivers/usb/gadget/function/f_ecm.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
> >> index 80841de845b0..027226325039 100644
> >> --- a/drivers/usb/gadget/function/f_ecm.c
> >> +++ b/drivers/usb/gadget/function/f_ecm.c
> >> @@ -892,6 +892,12 @@ static void ecm_resume(struct usb_function *f)
> >>  	gether_resume(&ecm->port);
> >>  }
> >>  
> >> +static int ecm_get_status(struct usb_function *f)
> >> +{
> >> +	return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
> >> +		USB_INTRF_STAT_FUNC_RW_CAP;
> >> +}
> >> +
> > 
> > Why does USB_INTRF_STAT_FUNC_RW_CAP is set regardless of
> > USB_CONFIG_ATT_WAKEUP?
> >
> We check that in composite.c before calling get_status().
> Have added some comments there [Patch v1 2/3] regarding same.
> 

Ok, let's discuss on the other thread.

Thanks,
Thinh

Re: [PATCH v1 3/3] usb: dwc3: gadget: Make gadget_wakeup asynchronous

[Prashanth K]

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

Re: [PATCH v1 2/3] usb: gadget: Use get_status callback to set remote wakeup capability

[Prashanth K]

On 09-04-25 06:25 am, Thinh Nguyen wrote:
> On Tue, Apr 08, 2025, Prashanth K wrote:
>>
>>
>> On 08-04-25 06:48 am, Thinh Nguyen wrote:
>>> On Thu, Apr 03, 2025, Prashanth K wrote:
>>>> Currently when the host sends GET_STATUS request for an interface,
>>>> we use get_status callbacks to set/clear remote wakeup capability
>>>> of that interface. And if get_status callback isn't present for
>>>> that interface, then we assume its remote wakeup capability based
>>>> on bmAttributes.
>>>>
>>>> Now consider a scenario, where we have a USB configuration with
>>>> multiple interfaces (say ECM + ADB), here ECM is remote wakeup
>>>> capable and as of now ADB isn't. And bmAttributes will indicate
>>>> the device as wakeup capable. With the current implementation,
>>>> when host sends GET_STATUS request for both interfaces, we will
>>>> set FUNC_RW_CAP for both. This results in USB3 CV Chapter 9.15
>>>> (Function Remote Wakeup Test) failures as host expects remote
>>>> wakeup from both interfaces.
>>>>
>>>> The above scenario is just an example, and the failure can be
>>>> observed if we use configuration with any interface except ECM.
>>>> Hence avoid configuring remote wakeup capability from composite
>>>> driver based on bmAttributes, instead use get_status callbacks
>>>> and let the function drivers decide this.
>>>>
>>>> Cc: stable@kernel.org
>>>> Fixes: 481c225c4802 ("usb: gadget: Handle function suspend feature selector")
>>>> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
>>>> ---
>>>>  drivers/usb/gadget/composite.c | 12 ++++--------
>>>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>>>> index 869ad99afb48..5c6da360e95b 100644
>>>> --- a/drivers/usb/gadget/composite.c
>>>> +++ b/drivers/usb/gadget/composite.c
>>>> @@ -2010,16 +2010,12 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>>>>  			break;
>>>>  
>>>>  		if (f->get_status) {
>>>> -			status = f->get_status(f);
>>>> +			/* if D5 is not set, then device is not wakeup capable */
>>>> +			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP)
>>>
>>> We should allow function to execute get_status regardless of
>>> USB_CONFIG_ATT_WAKEUP. There are other status beside wakeup.
>>>
>> Agree with the first part, I also wanted to remove the explicit check
>> for USB_CONFIG_ATT_WAKEUP. But anyways kept it since only 2 bits (RW_CAP
>> and RW) are defined in the spec as the status of GetStatus for an Interface.
>>
>> Lets do one thing, I'll rearrange it as follows
>>
>> if (f->get_status) {
>> 	status = f->get_status(f);
>> 	
>> /* if D5 is not set, then device is not wakeup capable */
>> if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP)
>> 	status &= ~(USB_INTRF_STAT_FUNC_RW_CAP | USB_INTRF_STAT_FUNC_RW);
> 
> Yes, something like this works, but I think you mean this:
> 
> 	if (!(f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP))
> 		...
> 
Yes right, will update it in next version.
>>
>>>> +				status = f->get_status(f);
>>>> +
>>>>  			if (status < 0)
>>>>  				break;
>>>> -		} else {
>>>> -			/* Set D0 and D1 bits based on func wakeup capability */
>>>> -			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
>>>> -				status |= USB_INTRF_STAT_FUNC_RW_CAP;
>>>
>>>
>>> So right now we're not able to configure the function to enable RW
>>> capable? Perhaps we need to update the composite configfs for this?
>>>
>>
>> The removed code used to set USB_INTRF_STAT_FUNC_RW_CAP even for
>> interfaces which doesn't have RW capability. Its better to handle this
>> from function level than from composite.
>>
> 
> Not at the gadget level, I mean to create a configfs attribute common
> across different functions to allow the user to enable/disable the
> function wakeup capability via the configfs when you setup the function.
> 
> What do you think?
> 
> Thanks,
> Thinh

Thats a good idea, in fact I had the same thought. But thought of doing
it later since its beyond the scope of this patch/series.

We can add a configfs attribute to enable/disable FUNC_RW_CAP, and
functions can return get_status() based on the attribute.

Regards,
Prashanth K

Re: [PATCH v1 3/3] usb: dwc3: gadget: Make gadget_wakeup asynchronous

[Thinh Nguyen]

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

Re: [PATCH v1 2/3] usb: gadget: Use get_status callback to set remote wakeup capability

[Thinh Nguyen]

On Wed, Apr 09, 2025, Prashanth K wrote:
> 
> 
> On 09-04-25 06:25 am, Thinh Nguyen wrote:
> > On Tue, Apr 08, 2025, Prashanth K wrote:
> >>
> >>
> >> On 08-04-25 06:48 am, Thinh Nguyen wrote:
> >>> On Thu, Apr 03, 2025, Prashanth K wrote:
> >>>> Currently when the host sends GET_STATUS request for an interface,
> >>>> we use get_status callbacks to set/clear remote wakeup capability
> >>>> of that interface. And if get_status callback isn't present for
> >>>> that interface, then we assume its remote wakeup capability based
> >>>> on bmAttributes.
> >>>>
> >>>> Now consider a scenario, where we have a USB configuration with
> >>>> multiple interfaces (say ECM + ADB), here ECM is remote wakeup
> >>>> capable and as of now ADB isn't. And bmAttributes will indicate
> >>>> the device as wakeup capable. With the current implementation,
> >>>> when host sends GET_STATUS request for both interfaces, we will
> >>>> set FUNC_RW_CAP for both. This results in USB3 CV Chapter 9.15
> >>>> (Function Remote Wakeup Test) failures as host expects remote
> >>>> wakeup from both interfaces.
> >>>>
> >>>> The above scenario is just an example, and the failure can be
> >>>> observed if we use configuration with any interface except ECM.
> >>>> Hence avoid configuring remote wakeup capability from composite
> >>>> driver based on bmAttributes, instead use get_status callbacks
> >>>> and let the function drivers decide this.
> >>>>
> >>>> Cc: stable@kernel.org
> >>>> Fixes: 481c225c4802 ("usb: gadget: Handle function suspend feature selector")
> >>>> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
> >>>> ---
> >>>>  drivers/usb/gadget/composite.c | 12 ++++--------
> >>>>  1 file changed, 4 insertions(+), 8 deletions(-)
> >>>>
> >>>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> >>>> index 869ad99afb48..5c6da360e95b 100644
> >>>> --- a/drivers/usb/gadget/composite.c
> >>>> +++ b/drivers/usb/gadget/composite.c
> >>>> @@ -2010,16 +2010,12 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
> >>>>  			break;
> >>>>  
> >>>>  		if (f->get_status) {
> >>>> -			status = f->get_status(f);
> >>>> +			/* if D5 is not set, then device is not wakeup capable */
> >>>> +			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP)
> >>>
> >>> We should allow function to execute get_status regardless of
> >>> USB_CONFIG_ATT_WAKEUP. There are other status beside wakeup.
> >>>
> >> Agree with the first part, I also wanted to remove the explicit check
> >> for USB_CONFIG_ATT_WAKEUP. But anyways kept it since only 2 bits (RW_CAP
> >> and RW) are defined in the spec as the status of GetStatus for an Interface.
> >>
> >> Lets do one thing, I'll rearrange it as follows
> >>
> >> if (f->get_status) {
> >> 	status = f->get_status(f);
> >> 	
> >> /* if D5 is not set, then device is not wakeup capable */
> >> if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP)
> >> 	status &= ~(USB_INTRF_STAT_FUNC_RW_CAP | USB_INTRF_STAT_FUNC_RW);
> > 
> > Yes, something like this works, but I think you mean this:
> > 
> > 	if (!(f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP))
> > 		...
> > 
> Yes right, will update it in next version.
> >>
> >>>> +				status = f->get_status(f);
> >>>> +
> >>>>  			if (status < 0)
> >>>>  				break;
> >>>> -		} else {
> >>>> -			/* Set D0 and D1 bits based on func wakeup capability */
> >>>> -			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
> >>>> -				status |= USB_INTRF_STAT_FUNC_RW_CAP;
> >>>
> >>>
> >>> So right now we're not able to configure the function to enable RW
> >>> capable? Perhaps we need to update the composite configfs for this?
> >>>
> >>
> >> The removed code used to set USB_INTRF_STAT_FUNC_RW_CAP even for
> >> interfaces which doesn't have RW capability. Its better to handle this
> >> from function level than from composite.
> >>
> > 
> > Not at the gadget level, I mean to create a configfs attribute common
> > across different functions to allow the user to enable/disable the
> > function wakeup capability via the configfs when you setup the function.
> > 
> > What do you think?
> > 
> > Thanks,
> > Thinh
> 
> Thats a good idea, in fact I had the same thought. But thought of doing
> it later since its beyond the scope of this patch/series.

The way you have it done now forces a usb3x function driver to implement
f->get_status to be able to respond with function wakeup capable.
Previously, we automatically enable function wakeup capability for all
functions if the USB_CONFIG_ATT_WAKEUP is set.

Arguably, this can cause a regression for remote capable devices to
operate in usb3 speeds.

> 
> We can add a configfs attribute to enable/disable FUNC_RW_CAP, and
> functions can return get_status() based on the attribute.
> 

That would be great! This would fit this series perfectly. Let me know
if there's an issue.

Thanks,
Thinh

Re: [PATCH v1 2/3] usb: gadget: Use get_status callback to set remote wakeup capability

[Thinh Nguyen]

On Wed, Apr 09, 2025, Thinh Nguyen wrote:
> On Wed, Apr 09, 2025, Prashanth K wrote:
> > 
> > 
> > On 09-04-25 06:25 am, Thinh Nguyen wrote:
> > > 
> > > Not at the gadget level, I mean to create a configfs attribute common
> > > across different functions to allow the user to enable/disable the
> > > function wakeup capability via the configfs when you setup the function.
> > > 
> > > What do you think?
> > > 
> > > Thanks,
> > > Thinh
> > 
> > Thats a good idea, in fact I had the same thought. But thought of doing
> > it later since its beyond the scope of this patch/series.
> 
> The way you have it done now forces a usb3x function driver to implement
> f->get_status to be able to respond with function wakeup capable.
> Previously, we automatically enable function wakeup capability for all
> functions if the USB_CONFIG_ATT_WAKEUP is set.
> 
> Arguably, this can cause a regression for remote capable devices to
> operate in usb3 speeds.

Sorry typos: I mean arguably, this can cause a regression for remote
wake capable devices to perform remote wakeup in usb3 speed.

BR,
Thinh

> 
> > 
> > We can add a configfs attribute to enable/disable FUNC_RW_CAP, and
> > functions can return get_status() based on the attribute.
> > 
> 
> That would be great! This would fit this series perfectly. Let me know
> if there's an issue.
> 
> Thanks,
> Thinh

Re: [PATCH v1 3/3] usb: dwc3: gadget: Make gadget_wakeup asynchronous

[Prashanth K]

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

Re: [PATCH v1 2/3] usb: gadget: Use get_status callback to set remote wakeup capability

[Prashanth K]

On 10-04-25 03:42 am, Thinh Nguyen wrote:
> On Wed, Apr 09, 2025, Thinh Nguyen wrote:
>> On Wed, Apr 09, 2025, Prashanth K wrote:
>>>
>>>
>>> On 09-04-25 06:25 am, Thinh Nguyen wrote:
>>>>
>>>> Not at the gadget level, I mean to create a configfs attribute common
>>>> across different functions to allow the user to enable/disable the
>>>> function wakeup capability via the configfs when you setup the function.
>>>>
>>>> What do you think?
>>>>
>>>> Thanks,
>>>> Thinh
>>>
>>> Thats a good idea, in fact I had the same thought. But thought of doing
>>> it later since its beyond the scope of this patch/series.
>>
>> The way you have it done now forces a usb3x function driver to implement
>> f->get_status to be able to respond with function wakeup capable.
>> Previously, we automatically enable function wakeup capability for all
>> functions if the USB_CONFIG_ATT_WAKEUP is set.

Currently function wakeup is implemented only on f_ecm and others don't
have the capability, so the expectation is functions should add add the
get_status callbacks while implementing remote/func wakeup and mark
itself and RW/FW capable. And if get_status callback is not there, then
func is not FW capable.

Current implementation sets RW/FW capability to all interfaces if
USB_CONFIG_ATT_WAKEUP is set (which is not right). I have provided an
example in the commit text where we incorrectly set FW capability.
>>
>> Arguably, this can cause a regression for remote capable devices to
>> operate in usb3 speeds.
> 
> Sorry typos: I mean arguably, this can cause a regression for remote
> wake capable devices to perform remote wakeup in usb3 speed.
> 
> BR,
> Thinh
> 
>>
>>>
>>> We can add a configfs attribute to enable/disable FUNC_RW_CAP, and
>>> functions can return get_status() based on the attribute.
>>>
>>
>> That would be great! This would fit this series perfectly. Let me know
>> if there's an issue.
>>
I seriously think we can take it out of this series and do that
separately. The intention of this series was to fix the wakeup
operations. And enable/disable func_wakeup from function driver would be
a new implementation. Ill take it up after this.

Regards,
Prashanth K

Re: [PATCH v1 2/3] usb: gadget: Use get_status callback to set remote wakeup capability

[Thinh Nguyen]

On Thu, Apr 10, 2025, Prashanth K wrote:
> 
> 
> On 10-04-25 03:42 am, Thinh Nguyen wrote:
> > On Wed, Apr 09, 2025, Thinh Nguyen wrote:
> >> On Wed, Apr 09, 2025, Prashanth K wrote:
> >>>
> >>>
> >>> On 09-04-25 06:25 am, Thinh Nguyen wrote:
> >>>>
> >>>> Not at the gadget level, I mean to create a configfs attribute common
> >>>> across different functions to allow the user to enable/disable the
> >>>> function wakeup capability via the configfs when you setup the function.
> >>>>
> >>>> What do you think?
> >>>>
> >>>> Thanks,
> >>>> Thinh
> >>>
> >>> Thats a good idea, in fact I had the same thought. But thought of doing
> >>> it later since its beyond the scope of this patch/series.
> >>
> >> The way you have it done now forces a usb3x function driver to implement
> >> f->get_status to be able to respond with function wakeup capable.
> >> Previously, we automatically enable function wakeup capability for all
> >> functions if the USB_CONFIG_ATT_WAKEUP is set.
> 
> Currently function wakeup is implemented only on f_ecm and others don't
> have the capability, so the expectation is functions should add add the
> get_status callbacks while implementing remote/func wakeup and mark
> itself and RW/FW capable. And if get_status callback is not there, then
> func is not FW capable.
> 
> Current implementation sets RW/FW capability to all interfaces if
> USB_CONFIG_ATT_WAKEUP is set (which is not right). I have provided an
> example in the commit text where we incorrectly set FW capability.
> >>
> >> Arguably, this can cause a regression for remote capable devices to
> >> operate in usb3 speeds.
> > 
> > Sorry typos: I mean arguably, this can cause a regression for remote
> > wake capable devices to perform remote wakeup in usb3 speed.
> > 
> > BR,
> > Thinh
> > 
> >>
> >>>
> >>> We can add a configfs attribute to enable/disable FUNC_RW_CAP, and
> >>> functions can return get_status() based on the attribute.
> >>>
> >>
> >> That would be great! This would fit this series perfectly. Let me know
> >> if there's an issue.
> >>
> I seriously think we can take it out of this series and do that
> separately. The intention of this series was to fix the wakeup
> operations. And enable/disable func_wakeup from function driver would be
> a new implementation. Ill take it up after this.
> 

Fair enough. Thanks for the work!

BR,
Thinh