[PATCH v2 0/5] Add function suspend/resume and remote wakeup support

[PATCH v2 0/5] Add function suspend/resume and remote wakeup support

[Elson Roy Serrao]

Changes in v2
 - Added a flag to indicate whether the device is remote wakeup capable.
 - Added an async parameter to _dwc3_gadget_wakeup() API and few cosmetic
   changes.
 - Added flags to reflect the state of  function suspend and function remote
   wakeup to usb_function struct rather than function specific struct (f_ecm).
 - Changed the dwc3_gadget_func__wakeup() API to run synchronously by first
   checking the link state and then sending the device notification. Also
   added debug log for DEVICE_NOTIFICATION generic cmd.
 - Added changes to arm the device for remotewakeup/function remotewakeup
   only if device is capable.

An usb device can initate a remote wakeup and bring the link out of
suspend as dictated by the DEVICE_REMOTE_WAKEUP feature selector.
To achieve this an interface can invoke gadget_wakeup op and wait for the
device to come out of LPM. But the current polling based implementation
fails if the host takes a long time to drive the resume signaling specially
in high speed capable devices. Switching to an interrupt based approach is
more robust and efficient. This can be leveraged by enabling link status
change events and triggering a gadget resume when the link comes to active
state.

If the device is enhanced super-speed capable, individual interfaces can
also be put into suspend state. An interface can be in function suspend
state even when the device is not in suspend state. Function suspend state
is retained throughout the device suspend entry and exit process.
A function can be put to function suspend through FUNCTION_SUSPEND feature
selector sent by the host. This setup packet also decides whether that
function is capable of initiating a function remote wakeup. When the
function sends a wakeup notification to the host the link must be first
brought to a non-U0 state and then this notification is sent.

This change adds the infrastructure needed to support the above
functionalities.

Elson Roy Serrao (5):
  usb: gadget: Add remote wakeup capable flag
  usb: dwc3: Add remote wakeup handling
  usb: gadget: Add function wakeup support
  usb: dwc3: Add function suspend and function wakeup support
  usb: gadget: f_ecm: Add suspend/resume and remote wakeup support

 drivers/usb/dwc3/core.h               |  4 ++
 drivers/usb/dwc3/debug.h              |  2 +
 drivers/usb/dwc3/ep0.c                | 16 +++---
 drivers/usb/dwc3/gadget.c             | 93 ++++++++++++++++++++++++++++++++---
 drivers/usb/gadget/composite.c        | 29 +++++++++++
 drivers/usb/gadget/function/f_ecm.c   | 69 ++++++++++++++++++++++++++
 drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++
 drivers/usb/gadget/function/u_ether.h |  4 ++
 drivers/usb/gadget/udc/core.c         | 19 +++++++
 include/linux/usb/composite.h         |  6 +++
 include/linux/usb/gadget.h            |  4 ++
 11 files changed, 294 insertions(+), 15 deletions(-)

-- 
2.7.4

[PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Elson Roy Serrao]

Add a flag to indicate whether the gadget is capable
of sending remote wakeup to the host.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/gadget/composite.c | 3 +++
 include/linux/usb/gadget.h     | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 403563c..b83963a 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -965,6 +965,9 @@ static int set_config(struct usb_composite_dev *cdev,
 	else
 		usb_gadget_clear_selfpowered(gadget);
 
+	if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes)
+		gadget->rw_capable = 1;
+
 	usb_gadget_vbus_draw(gadget, power);
 	if (result >= 0 && cdev->delayed_status)
 		result = USB_GADGET_DELAYED_STATUS;
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index dc3092c..15785f8 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -385,6 +385,7 @@ struct usb_gadget_ops {
  *	indicates that it supports LPM as per the LPM ECN & errata.
  * @irq: the interrupt number for device controller.
  * @id_number: a unique ID number for ensuring that gadget names are distinct
+ * @rw_capable: True if the gadget is capable of sending remote wakeup.
  *
  * Gadgets have a mostly-portable "gadget driver" implementing device
  * functions, handling all usb configurations and interfaces.  Gadget
@@ -446,6 +447,7 @@ struct usb_gadget {
 	unsigned			lpm_capable:1;
 	int				irq;
 	int				id_number;
+	unsigned			rw_capable:1;
 };
 #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
 
-- 
2.7.4

[PATCH v2 2/5] usb: dwc3: Add remote wakeup handling

[Elson Roy Serrao]

An usb device can initate a remote wakeup and bring the link out of
suspend as dictated by the DEVICE_REMOTE_WAKEUP feature selector.
Add support to handle this packet and set the remote wakeup capability.

Some hosts may take longer time to initiate the resume signaling after
device triggers a remote wakeup. So add async support to the wakeup API
by enabling link status change events.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/dwc3/core.h   |  3 +++
 drivers/usb/dwc3/ep0.c    |  4 ++++
 drivers/usb/dwc3/gadget.c | 57 ++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 8f9959b..53ded08 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1117,6 +1117,8 @@ struct dwc3_scratchpad_array {
  *		     address.
  * @num_ep_resized: carries the current number endpoints which have had its tx
  *		    fifo resized.
+ * @rw_armed: Indicates whether the host has armed the device for remote
+ *	      wakeup.
  */
 struct dwc3 {
 	struct work_struct	drd_work;
@@ -1332,6 +1334,7 @@ struct dwc3 {
 	int			max_cfg_eps;
 	int			last_fifo_depth;
 	int			num_ep_resized;
+	bool			rw_armed;
 };
 
 #define INCRX_BURST_MODE 0
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 61de693..0c1203d 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -356,6 +356,8 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
 				usb_status |= 1 << USB_DEV_STAT_U1_ENABLED;
 			if (reg & DWC3_DCTL_INITU2ENA)
 				usb_status |= 1 << USB_DEV_STAT_U2_ENABLED;
+		} else {
+			usb_status |= dwc->rw_armed << USB_DEVICE_REMOTE_WAKEUP;
 		}
 
 		break;
@@ -476,6 +478,8 @@ static int dwc3_ep0_handle_device(struct dwc3 *dwc,
 
 	switch (wValue) {
 	case USB_DEVICE_REMOTE_WAKEUP:
+		if (dwc->gadget->rw_capable)
+			dwc->rw_armed = set;
 		break;
 	/*
 	 * 9.4.1 says only for SS, in AddressState only for
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 89dcfac..db4b438 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -258,7 +258,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
 	return ret;
 }
 
-static int __dwc3_gadget_wakeup(struct dwc3 *dwc);
+static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async);
 
 /**
  * dwc3_send_gadget_ep_cmd - issue an endpoint command
@@ -325,7 +325,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
 
 			fallthrough;
 		case DWC3_LINK_STATE_U3:
-			ret = __dwc3_gadget_wakeup(dwc);
+			ret = __dwc3_gadget_wakeup(dwc, false);
 			dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n",
 					ret);
 			break;
@@ -2269,6 +2269,19 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
 
 /* -------------------------------------------------------------------------- */
 
+static void dwc3_gadget_enable_linksts_evts(struct dwc3 *dwc, bool set)
+{
+	u32 reg;
+
+	reg = dwc3_readl(dwc->regs, DWC3_DEVTEN);
+	if (set)
+		reg |= DWC3_DEVTEN_ULSTCNGEN;
+	else
+		reg &= ~DWC3_DEVTEN_ULSTCNGEN;
+
+	dwc3_writel(dwc->regs, DWC3_DEVTEN, reg);
+}
+
 static int dwc3_gadget_get_frame(struct usb_gadget *g)
 {
 	struct dwc3		*dwc = gadget_to_dwc(g);
@@ -2276,7 +2289,7 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g)
 	return __dwc3_gadget_get_frame(dwc);
 }
 
-static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
+static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
 {
 	int			retries;
 
@@ -2307,9 +2320,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
 		return -EINVAL;
 	}
 
+	if (async)
+		dwc3_gadget_enable_linksts_evts(dwc, true);
+
 	ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
 	if (ret < 0) {
 		dev_err(dwc->dev, "failed to put link in Recovery\n");
+		dwc3_gadget_enable_linksts_evts(dwc, false);
 		return ret;
 	}
 
@@ -2321,6 +2338,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
 		dwc3_writel(dwc->regs, DWC3_DCTL, reg);
 	}
 
+	/*
+	 * 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;
 
@@ -2347,7 +2371,13 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
 	int			ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
-	ret = __dwc3_gadget_wakeup(dwc);
+	if (!dwc->rw_armed) {
+		dev_err(dwc->dev, "%s:remote wakeup not enabled\n", __func__);
+		spin_unlock_irqrestore(&dwc->lock, flags);
+		return -EINVAL;
+	}
+	ret = __dwc3_gadget_wakeup(dwc, true);
+
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return ret;
@@ -2813,6 +2843,8 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	dwc->gadget_driver	= driver;
+	dwc3_gadget_enable_linksts_evts(dwc, false);
+	dwc->rw_armed = false;
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return 0;
@@ -2832,6 +2864,8 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	dwc->gadget_driver	= NULL;
+	dwc3_gadget_enable_linksts_evts(dwc, false);
+	dwc->rw_armed = false;
 	dwc->max_cfg_eps = 0;
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
@@ -3821,6 +3855,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
 
 	dwc->gadget->speed = USB_SPEED_UNKNOWN;
 	dwc->setup_packet_pending = false;
+	dwc3_gadget_enable_linksts_evts(dwc, false);
+	dwc->rw_armed = false;
 	usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
 
 	if (dwc->ep0state != EP0_SETUP_PHASE) {
@@ -3920,6 +3956,9 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
 	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
 	reg &= ~(DWC3_DCFG_DEVADDR_MASK);
 	dwc3_writel(dwc->regs, DWC3_DCFG, reg);
+
+	dwc3_gadget_enable_linksts_evts(dwc, false);
+	dwc->rw_armed = false;
 }
 
 static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
@@ -4066,7 +4105,7 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
 	 */
 }
 
-static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
+static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo)
 {
 	/*
 	 * TODO take core out of low power mode when that's
@@ -4078,6 +4117,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
 		dwc->gadget_driver->resume(dwc->gadget);
 		spin_lock(&dwc->lock);
 	}
+
+	dwc->link_state = evtinfo & DWC3_LINK_STATE_MASK;
 }
 
 static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
@@ -4159,6 +4200,10 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
 	}
 
 	switch (next) {
+	case DWC3_LINK_STATE_U0:
+		dwc3_gadget_enable_linksts_evts(dwc, false);
+		dwc3_resume_gadget(dwc);
+		break;
 	case DWC3_LINK_STATE_U1:
 		if (dwc->speed == USB_SPEED_SUPER)
 			dwc3_suspend_gadget(dwc);
@@ -4227,7 +4272,7 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc,
 		dwc3_gadget_conndone_interrupt(dwc);
 		break;
 	case DWC3_DEVICE_EVENT_WAKEUP:
-		dwc3_gadget_wakeup_interrupt(dwc);
+		dwc3_gadget_wakeup_interrupt(dwc, event->event_info);
 		break;
 	case DWC3_DEVICE_EVENT_HIBER_REQ:
 		if (dev_WARN_ONCE(dwc->dev, !dwc->has_hibernation,
-- 
2.7.4

[PATCH v2 3/5] usb: gadget: Add function wakeup support

[Elson Roy Serrao]

A function which is in function suspend state has to send a
function wake notification to the host in case it needs to
exit from this state and resume data transfer. Add support to
handle such requests by exposing a new gadget op.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/gadget/composite.c | 26 ++++++++++++++++++++++++++
 drivers/usb/gadget/udc/core.c  | 19 +++++++++++++++++++
 include/linux/usb/composite.h  |  6 ++++++
 include/linux/usb/gadget.h     |  2 ++
 4 files changed, 53 insertions(+)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index b83963a..1672513 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -490,6 +490,32 @@ int usb_interface_id(struct usb_configuration *config,
 }
 EXPORT_SYMBOL_GPL(usb_interface_id);
 
+int usb_func_wakeup(struct usb_function *func)
+{
+	int ret, id;
+
+	if (!func->func_rw_armed) {
+		ERROR(func->config->cdev, "func remote wakeup not enabled\n");
+		return -EINVAL;
+	}
+
+	DBG(func->config->cdev, "%s function wakeup\n", func->name);
+
+	for (id = 0; id < MAX_CONFIG_INTERFACES; id++)
+		if (func->config->interface[id] == func)
+			break;
+
+	if (id == MAX_CONFIG_INTERFACES) {
+		ERROR(func->config->cdev, "Invalid function id:%d\n", id);
+		return -EINVAL;
+	}
+
+	ret = usb_gadget_func_wakeup(func->config->cdev->gadget, id);
+
+	return ret;
+}
+EXPORT_SYMBOL(usb_func_wakeup);
+
 static u8 encode_bMaxPower(enum usb_device_speed speed,
 		struct usb_configuration *c)
 {
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 23b0629..73c2346 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -819,6 +819,25 @@ int usb_gadget_activate(struct usb_gadget *gadget)
 }
 EXPORT_SYMBOL_GPL(usb_gadget_activate);
 
+/**
+ * usb_gadget_func_wakeup - sends function wake notification to the host.
+ * If the link is in low power state, first brings the link to active state.
+ * @gadget: controller used to wake up the host
+ * @interface_id: interface on which function wake notification is sent.
+ *
+ * Returns zero on success, else negative error code if the hardware
+ * doesn't support such attempts. Drivers must return device descriptors that
+ * report their ability to support this, or hosts won't enable it.
+ */
+int usb_gadget_func_wakeup(struct usb_gadget *gadget, int interface_id)
+{
+	if (!gadget->ops->func_wakeup)
+		return -EOPNOTSUPP;
+
+	return gadget->ops->func_wakeup(gadget, interface_id);
+}
+EXPORT_SYMBOL_GPL(usb_gadget_func_wakeup);
+
 /* ------------------------------------------------------------------------- */
 
 #ifdef	CONFIG_HAS_DMA
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 43ac3fa..b2cac0a 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -149,6 +149,9 @@ struct usb_os_desc_table {
  *	GetStatus() request when the recipient is Interface.
  * @func_suspend: callback to be called when
  *	SetFeature(FUNCTION_SUSPEND) is reseived
+ * @func_suspended: Indicates whether the function is in function suspend state.
+ * @func_rw_armed: Indicates whether the function is armed by the host for
+ *	wakeup signaling.
  *
  * A single USB function uses one or more interfaces, and should in most
  * cases support operation at both full and high speeds.  Each function is
@@ -219,6 +222,8 @@ struct usb_function {
 	int			(*get_status)(struct usb_function *);
 	int			(*func_suspend)(struct usb_function *,
 						u8 suspend_opt);
+	bool			func_suspended;
+	bool			func_rw_armed;
 	/* private: */
 	/* internals */
 	struct list_head		list;
@@ -240,6 +245,7 @@ int config_ep_by_speed_and_alt(struct usb_gadget *g, struct usb_function *f,
 
 int config_ep_by_speed(struct usb_gadget *g, struct usb_function *f,
 			struct usb_ep *_ep);
+int usb_func_wakeup(struct usb_function *func);
 
 #define	MAX_CONFIG_INTERFACES		16	/* arbitrary; max 255 */
 
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 15785f8..47441b0 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -309,6 +309,7 @@ struct usb_udc;
 struct usb_gadget_ops {
 	int	(*get_frame)(struct usb_gadget *);
 	int	(*wakeup)(struct usb_gadget *);
+	int	(*func_wakeup)(struct usb_gadget *gadget, int interface_id);
 	int	(*set_selfpowered) (struct usb_gadget *, int is_selfpowered);
 	int	(*vbus_session) (struct usb_gadget *, int is_active);
 	int	(*vbus_draw) (struct usb_gadget *, unsigned mA);
@@ -612,6 +613,7 @@ int usb_gadget_disconnect(struct usb_gadget *gadget);
 int usb_gadget_deactivate(struct usb_gadget *gadget);
 int usb_gadget_activate(struct usb_gadget *gadget);
 int usb_gadget_check_config(struct usb_gadget *gadget);
+int usb_gadget_func_wakeup(struct usb_gadget *gadget, int interface_id);
 #else
 static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
 { return 0; }
-- 
2.7.4

[PATCH v2 4/5] usb: dwc3: Add function suspend and function wakeup support

[Elson Roy Serrao]

USB host sends function suspend and function resume notifications to
the interface through SET_FEATURE/CLEAR_FEATURE setup packets.
Add support to handle these packets by delegating the requests to
composite layer. Also add support to handle function wake notification
requests to exit from function suspend state.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/dwc3/core.h   |  1 +
 drivers/usb/dwc3/debug.h  |  2 ++
 drivers/usb/dwc3/ep0.c    | 12 ++++--------
 drivers/usb/dwc3/gadget.c | 36 +++++++++++++++++++++++++++++++++++-
 4 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 53ded08..5cda645 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -526,6 +526,7 @@
 #define DWC3_DGCMD_SET_ENDPOINT_NRDY	0x0c
 #define DWC3_DGCMD_SET_ENDPOINT_PRIME	0x0d
 #define DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK	0x10
+#define DWC3_DGCMD_DEV_NOTIFICATION	0x07
 
 #define DWC3_DGCMD_STATUS(n)		(((n) >> 12) & 0x0F)
 #define DWC3_DGCMD_CMDACT		BIT(10)
diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
index 48b44b8..0897d9d 100644
--- a/drivers/usb/dwc3/debug.h
+++ b/drivers/usb/dwc3/debug.h
@@ -72,6 +72,8 @@ dwc3_gadget_generic_cmd_string(u8 cmd)
 		return "Set Endpoint Prime";
 	case DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK:
 		return "Run SoC Bus Loopback Test";
+	case DWC3_DGCMD_DEV_NOTIFICATION:
+		return "Device Notification";
 	default:
 		return "UNKNOWN";
 	}
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 0c1203d..e05c2b9 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -30,6 +30,8 @@
 static void __dwc3_ep0_do_control_status(struct dwc3 *dwc, struct dwc3_ep *dep);
 static void __dwc3_ep0_do_control_data(struct dwc3 *dwc,
 		struct dwc3_ep *dep, struct dwc3_request *req);
+static int dwc3_ep0_delegate_req(struct dwc3 *dwc,
+				 struct usb_ctrlrequest *ctrl);
 
 static void dwc3_ep0_prepare_one_trb(struct dwc3_ep *dep,
 		dma_addr_t buf_dma, u32 len, u32 type, bool chain)
@@ -367,7 +369,7 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
 		 * Function Remote Wake Capable	D0
 		 * Function Remote Wakeup	D1
 		 */
-		break;
+		return dwc3_ep0_delegate_req(dwc, ctrl);
 
 	case USB_RECIP_ENDPOINT:
 		dep = dwc3_wIndex_to_dep(dwc, ctrl->wIndex);
@@ -514,13 +516,7 @@ static int dwc3_ep0_handle_intf(struct dwc3 *dwc,
 
 	switch (wValue) {
 	case USB_INTRF_FUNC_SUSPEND:
-		/*
-		 * REVISIT: Ideally we would enable some low power mode here,
-		 * however it's unclear what we should be doing here.
-		 *
-		 * For now, we're not doing anything, just making sure we return
-		 * 0 so USB Command Verifier tests pass without any errors.
-		 */
+		ret = dwc3_ep0_delegate_req(dwc, ctrl);
 		break;
 	default:
 		ret = -EINVAL;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index db4b438..1c958c4 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2346,7 +2346,7 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
 		return 0;
 
 	/* poll until Link State changes to ON */
-	retries = 20000;
+	retries = 30000;
 
 	while (retries--) {
 		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
@@ -2383,6 +2383,39 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
 	return ret;
 }
 
+static void dwc3_resume_gadget(struct dwc3 *dwc);
+
+static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int interface_id)
+{
+	struct  dwc3		*dwc = gadget_to_dwc(g);
+	unsigned long		flags;
+	int			ret;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	/*
+	 * If the link is in LPM, first bring the link to U0
+	 * before triggering function remote wakeup.
+	 */
+	if (dwc->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->link_state = DWC3_LINK_STATE_U0;
+	}
+
+	ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
+					       0x1 | (interface_id << 4));
+	if (ret)
+		dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
+
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	return ret;
+}
+
 static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
 		int is_selfpowered)
 {
@@ -3012,6 +3045,7 @@ static void dwc3_gadget_async_callbacks(struct usb_gadget *g, bool enable)
 static const struct usb_gadget_ops dwc3_gadget_ops = {
 	.get_frame		= dwc3_gadget_get_frame,
 	.wakeup			= dwc3_gadget_wakeup,
+	.func_wakeup		= dwc3_gadget_func_wakeup,
 	.set_selfpowered	= dwc3_gadget_set_selfpowered,
 	.pullup			= dwc3_gadget_pullup,
 	.udc_start		= dwc3_gadget_start,
-- 
2.7.4

[PATCH v2 5/5] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support

[Elson Roy Serrao]

When host sends a suspend notification to the device, handle
the suspend callbacks in the function driver. Enhanced super
speed devices can support function suspend feature to put the
function in suspend state. Handle function suspend callback.

Depending on the remote wakeup capability the device can either
trigger a remote wakeup or wait for the host initiated resume to
start data transfer again.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/gadget/function/f_ecm.c   | 69 +++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++++++
 drivers/usb/gadget/function/u_ether.h |  4 ++
 3 files changed, 136 insertions(+)

diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
index a7ab30e..34fe091 100644
--- a/drivers/usb/gadget/function/f_ecm.c
+++ b/drivers/usb/gadget/function/f_ecm.c
@@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
 
 	usb_ep_disable(ecm->notify);
 	ecm->notify->desc = NULL;
+	f->func_suspended = false;
+	f->func_rw_armed = false;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -885,6 +887,69 @@ static struct usb_function_instance *ecm_alloc_inst(void)
 	return &opts->func_inst;
 }
 
+static void ecm_suspend(struct usb_function *f)
+{
+	struct f_ecm *ecm = func_to_ecm(f);
+	struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
+
+	if (f->func_suspended) {
+		DBG(cdev, "Function already suspended\n");
+		return;
+	}
+
+	DBG(cdev, "ECM Suspend\n");
+
+	gether_suspend(&ecm->port);
+}
+
+static void ecm_resume(struct usb_function *f)
+{
+	struct f_ecm *ecm = func_to_ecm(f);
+	struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
+
+	/*
+	 * If the function is in USB3 Function Suspend state, resume is
+	 * canceled. In this case resume is done by a Function Resume request.
+	 */
+	if (f->func_suspended)
+		return;
+
+	DBG(cdev, "ECM Resume\n");
+
+	gether_resume(&ecm->port);
+}
+
+static int ecm_get_status(struct usb_function *f)
+{
+	return (f->func_rw_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
+		USB_INTRF_STAT_FUNC_RW_CAP;
+}
+
+static int ecm_func_suspend(struct usb_function *f, u8 options)
+{
+	struct f_ecm *ecm = func_to_ecm(f);
+	struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
+
+	DBG(cdev, "func susp %u cmd\n", options);
+
+	if (cdev->gadget->rw_capable)
+		f->func_rw_armed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8));
+
+	if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
+		if (!f->func_suspended) {
+			ecm_suspend(f);
+			f->func_suspended = true;
+		}
+	} else {
+		if (f->func_suspended) {
+			f->func_suspended = false;
+			ecm_resume(f);
+		}
+	}
+
+	return 0;
+}
+
 static void ecm_free(struct usb_function *f)
 {
 	struct f_ecm *ecm;
@@ -952,6 +1017,10 @@ static struct usb_function *ecm_alloc(struct usb_function_instance *fi)
 	ecm->port.func.setup = ecm_setup;
 	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.func_suspend = ecm_func_suspend;
+	ecm->port.func.resume = ecm_resume;
 
 	return &ecm->port.func;
 }
diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index 8f12f3f..4e54089 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -471,6 +471,20 @@ static inline int is_promisc(u16 cdc_filter)
 	return cdc_filter & USB_CDC_PACKET_TYPE_PROMISCUOUS;
 }
 
+static int ether_wakeup_host(struct gether *port)
+{
+	int			ret;
+	struct usb_function	*func = &port->func;
+	struct usb_gadget	*gadget = func->config->cdev->gadget;
+
+	if (func->func_suspended)
+		ret = usb_func_wakeup(func);
+	else
+		ret = usb_gadget_wakeup(gadget);
+
+	return ret;
+}
+
 static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 					struct net_device *net)
 {
@@ -490,6 +504,15 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 		in = NULL;
 		cdc_filter = 0;
 	}
+
+	if (dev->port_usb->is_suspend) {
+		DBG(dev, "Port suspended. Triggering wakeup\n");
+		netif_stop_queue(net);
+		spin_unlock_irqrestore(&dev->lock, flags);
+		ether_wakeup_host(dev->port_usb);
+		return NETDEV_TX_BUSY;
+	}
+
 	spin_unlock_irqrestore(&dev->lock, flags);
 
 	if (!in) {
@@ -1046,6 +1069,45 @@ int gether_set_ifname(struct net_device *net, const char *name, int len)
 }
 EXPORT_SYMBOL_GPL(gether_set_ifname);
 
+void gether_suspend(struct gether *link)
+{
+	struct eth_dev *dev = link->ioport;
+	unsigned long flags;
+
+	if (!dev)
+		return;
+
+	if (atomic_read(&dev->tx_qlen)) {
+		/*
+		 * There is a transfer in progress. So we trigger a remote
+		 * wakeup to inform the host.
+		 */
+		ether_wakeup_host(dev->port_usb);
+		return;
+	}
+	spin_lock_irqsave(&dev->lock, flags);
+	link->is_suspend = true;
+	spin_unlock_irqrestore(&dev->lock, flags);
+}
+EXPORT_SYMBOL_GPL(gether_suspend);
+
+void gether_resume(struct gether *link)
+{
+	struct eth_dev *dev = link->ioport;
+	unsigned long flags;
+
+	if (!dev)
+		return;
+
+	if (netif_queue_stopped(dev->net))
+		netif_start_queue(dev->net);
+
+	spin_lock_irqsave(&dev->lock, flags);
+	link->is_suspend = false;
+	spin_unlock_irqrestore(&dev->lock, flags);
+}
+EXPORT_SYMBOL_GPL(gether_resume);
+
 /*
  * gether_cleanup - remove Ethernet-over-USB device
  * Context: may sleep
@@ -1208,6 +1270,7 @@ void gether_disconnect(struct gether *link)
 
 	spin_lock(&dev->lock);
 	dev->port_usb = NULL;
+	link->is_suspend = false;
 	spin_unlock(&dev->lock);
 }
 EXPORT_SYMBOL_GPL(gether_disconnect);
diff --git a/drivers/usb/gadget/function/u_ether.h b/drivers/usb/gadget/function/u_ether.h
index 4014454..851ee10 100644
--- a/drivers/usb/gadget/function/u_ether.h
+++ b/drivers/usb/gadget/function/u_ether.h
@@ -79,6 +79,7 @@ struct gether {
 	/* called on network open/close */
 	void				(*open)(struct gether *);
 	void				(*close)(struct gether *);
+	bool				is_suspend;
 };
 
 #define	DEFAULT_FILTER	(USB_CDC_PACKET_TYPE_BROADCAST \
@@ -258,6 +259,9 @@ int gether_set_ifname(struct net_device *net, const char *name, int len);
 
 void gether_cleanup(struct eth_dev *dev);
 
+void gether_suspend(struct gether *link);
+void gether_resume(struct gether *link);
+
 /* connect/disconnect is handled by individual functions */
 struct net_device *gether_connect(struct gether *);
 void gether_disconnect(struct gether *);
-- 
2.7.4

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Thinh Nguyen]

On Tue, Jan 17, 2023, Elson Roy Serrao wrote:
> Add a flag to indicate whether the gadget is capable
> of sending remote wakeup to the host.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  drivers/usb/gadget/composite.c | 3 +++
>  include/linux/usb/gadget.h     | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 403563c..b83963a 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -965,6 +965,9 @@ static int set_config(struct usb_composite_dev *cdev,
>  	else
>  		usb_gadget_clear_selfpowered(gadget);
>  
> +	if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes)
> +		gadget->rw_capable = 1;

Some device may not support remote wakeup. gadget->rw_capable should be
set and reported by the UDC. May need a gadget ops to enable remote
wakeup here.

Thanks,
Thinh

> +
>  	usb_gadget_vbus_draw(gadget, power);
>  	if (result >= 0 && cdev->delayed_status)
>  		result = USB_GADGET_DELAYED_STATUS;
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index dc3092c..15785f8 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -385,6 +385,7 @@ struct usb_gadget_ops {
>   *	indicates that it supports LPM as per the LPM ECN & errata.
>   * @irq: the interrupt number for device controller.
>   * @id_number: a unique ID number for ensuring that gadget names are distinct
> + * @rw_capable: True if the gadget is capable of sending remote wakeup.
>   *
>   * Gadgets have a mostly-portable "gadget driver" implementing device
>   * functions, handling all usb configurations and interfaces.  Gadget
> @@ -446,6 +447,7 @@ struct usb_gadget {
>  	unsigned			lpm_capable:1;
>  	int				irq;
>  	int				id_number;
> +	unsigned			rw_capable:1;
>  };
>  #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
>  
> -- 
> 2.7.4
> 

Re: [PATCH v2 4/5] usb: dwc3: Add function suspend and function wakeup support

[Thinh Nguyen]

On Tue, Jan 17, 2023, Elson Roy Serrao wrote:
> USB host sends function suspend and function resume notifications to
> the interface through SET_FEATURE/CLEAR_FEATURE setup packets.
> Add support to handle these packets by delegating the requests to
> composite layer. Also add support to handle function wake notification
> requests to exit from function suspend state.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  drivers/usb/dwc3/core.h   |  1 +
>  drivers/usb/dwc3/debug.h  |  2 ++
>  drivers/usb/dwc3/ep0.c    | 12 ++++--------
>  drivers/usb/dwc3/gadget.c | 36 +++++++++++++++++++++++++++++++++++-
>  4 files changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 53ded08..5cda645 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -526,6 +526,7 @@
>  #define DWC3_DGCMD_SET_ENDPOINT_NRDY	0x0c
>  #define DWC3_DGCMD_SET_ENDPOINT_PRIME	0x0d
>  #define DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK	0x10
> +#define DWC3_DGCMD_DEV_NOTIFICATION	0x07
>  
>  #define DWC3_DGCMD_STATUS(n)		(((n) >> 12) & 0x0F)
>  #define DWC3_DGCMD_CMDACT		BIT(10)
> diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
> index 48b44b8..0897d9d 100644
> --- a/drivers/usb/dwc3/debug.h
> +++ b/drivers/usb/dwc3/debug.h
> @@ -72,6 +72,8 @@ dwc3_gadget_generic_cmd_string(u8 cmd)
>  		return "Set Endpoint Prime";
>  	case DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK:
>  		return "Run SoC Bus Loopback Test";
> +	case DWC3_DGCMD_DEV_NOTIFICATION:
> +		return "Device Notification";
>  	default:
>  		return "UNKNOWN";
>  	}
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 0c1203d..e05c2b9 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -30,6 +30,8 @@
>  static void __dwc3_ep0_do_control_status(struct dwc3 *dwc, struct dwc3_ep *dep);
>  static void __dwc3_ep0_do_control_data(struct dwc3 *dwc,
>  		struct dwc3_ep *dep, struct dwc3_request *req);
> +static int dwc3_ep0_delegate_req(struct dwc3 *dwc,
> +				 struct usb_ctrlrequest *ctrl);
>  
>  static void dwc3_ep0_prepare_one_trb(struct dwc3_ep *dep,
>  		dma_addr_t buf_dma, u32 len, u32 type, bool chain)
> @@ -367,7 +369,7 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
>  		 * Function Remote Wake Capable	D0
>  		 * Function Remote Wakeup	D1
>  		 */
> -		break;
> +		return dwc3_ep0_delegate_req(dwc, ctrl);
>  
>  	case USB_RECIP_ENDPOINT:
>  		dep = dwc3_wIndex_to_dep(dwc, ctrl->wIndex);
> @@ -514,13 +516,7 @@ static int dwc3_ep0_handle_intf(struct dwc3 *dwc,
>  
>  	switch (wValue) {
>  	case USB_INTRF_FUNC_SUSPEND:
> -		/*
> -		 * REVISIT: Ideally we would enable some low power mode here,
> -		 * however it's unclear what we should be doing here.
> -		 *
> -		 * For now, we're not doing anything, just making sure we return
> -		 * 0 so USB Command Verifier tests pass without any errors.
> -		 */
> +		ret = dwc3_ep0_delegate_req(dwc, ctrl);
>  		break;
>  	default:
>  		ret = -EINVAL;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index db4b438..1c958c4 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2346,7 +2346,7 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>  		return 0;
>  
>  	/* poll until Link State changes to ON */
> -	retries = 20000;
> +	retries = 30000;

This seems like a fix that needs to be splitted to a separate change.
read_poll_timeout_atomic() would be useful here (as a separate/unrelated
change for status polling).

>  
>  	while (retries--) {
>  		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> @@ -2383,6 +2383,39 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>  	return ret;
>  }
>  
> +static void dwc3_resume_gadget(struct dwc3 *dwc);
> +
> +static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int interface_id)
> +{
> +	struct  dwc3		*dwc = gadget_to_dwc(g);
> +	unsigned long		flags;
> +	int			ret;
> +
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	/*
> +	 * If the link is in LPM, first bring the link to U0

Reword LMP -> low power.

> +	 * before triggering function remote wakeup.
> +	 */
> +	if (dwc->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->link_state = DWC3_LINK_STATE_U0;
> +	}
> +
> +	ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
> +					       0x1 | (interface_id << 4));

Create a macro for 0x1. Something like DWC3_DGCMDPAR_DN_FUNC_WAKE. Also,
create an macro for interface selection. Maybe
DWC3_DGCMDPAR_INTF_SEL(interface_id).

> +	if (ret)
> +		dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
> +
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +	return ret;
> +}
> +
>  static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
>  		int is_selfpowered)
>  {
> @@ -3012,6 +3045,7 @@ static void dwc3_gadget_async_callbacks(struct usb_gadget *g, bool enable)
>  static const struct usb_gadget_ops dwc3_gadget_ops = {
>  	.get_frame		= dwc3_gadget_get_frame,
>  	.wakeup			= dwc3_gadget_wakeup,
> +	.func_wakeup		= dwc3_gadget_func_wakeup,
>  	.set_selfpowered	= dwc3_gadget_set_selfpowered,
>  	.pullup			= dwc3_gadget_pullup,
>  	.udc_start		= dwc3_gadget_start,
> -- 
> 2.7.4
> 

Thanks,
Thinh

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Greg KH]

On Tue, Jan 17, 2023 at 01:55:03PM -0800, Elson Roy Serrao wrote:
> Add a flag to indicate whether the gadget is capable
> of sending remote wakeup to the host.
> 
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>  drivers/usb/gadget/composite.c | 3 +++
>  include/linux/usb/gadget.h     | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 403563c..b83963a 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -965,6 +965,9 @@ static int set_config(struct usb_composite_dev *cdev,
>  	else
>  		usb_gadget_clear_selfpowered(gadget);
>  
> +	if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes)
> +		gadget->rw_capable = 1;
> +
>  	usb_gadget_vbus_draw(gadget, power);
>  	if (result >= 0 && cdev->delayed_status)
>  		result = USB_GADGET_DELAYED_STATUS;
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index dc3092c..15785f8 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -385,6 +385,7 @@ struct usb_gadget_ops {
>   *	indicates that it supports LPM as per the LPM ECN & errata.
>   * @irq: the interrupt number for device controller.
>   * @id_number: a unique ID number for ensuring that gadget names are distinct
> + * @rw_capable: True if the gadget is capable of sending remote wakeup.
>   *
>   * Gadgets have a mostly-portable "gadget driver" implementing device
>   * functions, handling all usb configurations and interfaces.  Gadget
> @@ -446,6 +447,7 @@ struct usb_gadget {
>  	unsigned			lpm_capable:1;
>  	int				irq;
>  	int				id_number;
> +	unsigned			rw_capable:1;

Why not put this by the other bitfields in this structure?

thanks,

greg k-h

Re: [PATCH v2 4/5] usb: dwc3: Add function suspend and function wakeup support

[Elson Serrao]

On 1/18/2023 6:18 PM, Thinh Nguyen wrote:
> On Tue, Jan 17, 2023, Elson Roy Serrao wrote:
>> USB host sends function suspend and function resume notifications to
>> the interface through SET_FEATURE/CLEAR_FEATURE setup packets.
>> Add support to handle these packets by delegating the requests to
>> composite layer. Also add support to handle function wake notification
>> requests to exit from function suspend state.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>   drivers/usb/dwc3/core.h   |  1 +
>>   drivers/usb/dwc3/debug.h  |  2 ++
>>   drivers/usb/dwc3/ep0.c    | 12 ++++--------
>>   drivers/usb/dwc3/gadget.c | 36 +++++++++++++++++++++++++++++++++++-
>>   4 files changed, 42 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 53ded08..5cda645 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -526,6 +526,7 @@
>>   #define DWC3_DGCMD_SET_ENDPOINT_NRDY	0x0c
>>   #define DWC3_DGCMD_SET_ENDPOINT_PRIME	0x0d
>>   #define DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK	0x10
>> +#define DWC3_DGCMD_DEV_NOTIFICATION	0x07
>>   
>>   #define DWC3_DGCMD_STATUS(n)		(((n) >> 12) & 0x0F)
>>   #define DWC3_DGCMD_CMDACT		BIT(10)
>> diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
>> index 48b44b8..0897d9d 100644
>> --- a/drivers/usb/dwc3/debug.h
>> +++ b/drivers/usb/dwc3/debug.h
>> @@ -72,6 +72,8 @@ dwc3_gadget_generic_cmd_string(u8 cmd)
>>   		return "Set Endpoint Prime";
>>   	case DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK:
>>   		return "Run SoC Bus Loopback Test";
>> +	case DWC3_DGCMD_DEV_NOTIFICATION:
>> +		return "Device Notification";
>>   	default:
>>   		return "UNKNOWN";
>>   	}
>> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
>> index 0c1203d..e05c2b9 100644
>> --- a/drivers/usb/dwc3/ep0.c
>> +++ b/drivers/usb/dwc3/ep0.c
>> @@ -30,6 +30,8 @@
>>   static void __dwc3_ep0_do_control_status(struct dwc3 *dwc, struct dwc3_ep *dep);
>>   static void __dwc3_ep0_do_control_data(struct dwc3 *dwc,
>>   		struct dwc3_ep *dep, struct dwc3_request *req);
>> +static int dwc3_ep0_delegate_req(struct dwc3 *dwc,
>> +				 struct usb_ctrlrequest *ctrl);
>>   
>>   static void dwc3_ep0_prepare_one_trb(struct dwc3_ep *dep,
>>   		dma_addr_t buf_dma, u32 len, u32 type, bool chain)
>> @@ -367,7 +369,7 @@ static int dwc3_ep0_handle_status(struct dwc3 *dwc,
>>   		 * Function Remote Wake Capable	D0
>>   		 * Function Remote Wakeup	D1
>>   		 */
>> -		break;
>> +		return dwc3_ep0_delegate_req(dwc, ctrl);
>>   
>>   	case USB_RECIP_ENDPOINT:
>>   		dep = dwc3_wIndex_to_dep(dwc, ctrl->wIndex);
>> @@ -514,13 +516,7 @@ static int dwc3_ep0_handle_intf(struct dwc3 *dwc,
>>   
>>   	switch (wValue) {
>>   	case USB_INTRF_FUNC_SUSPEND:
>> -		/*
>> -		 * REVISIT: Ideally we would enable some low power mode here,
>> -		 * however it's unclear what we should be doing here.
>> -		 *
>> -		 * For now, we're not doing anything, just making sure we return
>> -		 * 0 so USB Command Verifier tests pass without any errors.
>> -		 */
>> +		ret = dwc3_ep0_delegate_req(dwc, ctrl);
>>   		break;
>>   	default:
>>   		ret = -EINVAL;
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index db4b438..1c958c4 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2346,7 +2346,7 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
>>   		return 0;
>>   
>>   	/* poll until Link State changes to ON */
>> -	retries = 20000;
>> +	retries = 30000;
> 
> This seems like a fix that needs to be splitted to a separate change.
> read_poll_timeout_atomic() would be useful here (as a separate/unrelated
> change for status polling).
> 
Done.

>>   
>>   	while (retries--) {
>>   		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>> @@ -2383,6 +2383,39 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>>   	return ret;
>>   }
>>   
>> +static void dwc3_resume_gadget(struct dwc3 *dwc);
>> +
>> +static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int interface_id)
>> +{
>> +	struct  dwc3		*dwc = gadget_to_dwc(g);
>> +	unsigned long		flags;
>> +	int			ret;
>> +
>> +	spin_lock_irqsave(&dwc->lock, flags);
>> +	/*
>> +	 * If the link is in LPM, first bring the link to U0
> 
> Reword LMP -> low power.
> 
Sure.

>> +	 * before triggering function remote wakeup.
>> +	 */
>> +	if (dwc->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->link_state = DWC3_LINK_STATE_U0;
>> +	}
>> +
>> +	ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
>> +					       0x1 | (interface_id << 4));
> 
> Create a macro for 0x1. Something like DWC3_DGCMDPAR_DN_FUNC_WAKE. Also,
> create an macro for interface selection. Maybe
> DWC3_DGCMDPAR_INTF_SEL(interface_id).
> 
Sure.

>> +	if (ret)
>> +		dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
>> +
>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>> +
>> +	return ret;
>> +}
>> +
>>   static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
>>   		int is_selfpowered)
>>   {
>> @@ -3012,6 +3045,7 @@ static void dwc3_gadget_async_callbacks(struct usb_gadget *g, bool enable)
>>   static const struct usb_gadget_ops dwc3_gadget_ops = {
>>   	.get_frame		= dwc3_gadget_get_frame,
>>   	.wakeup			= dwc3_gadget_wakeup,
>> +	.func_wakeup		= dwc3_gadget_func_wakeup,
>>   	.set_selfpowered	= dwc3_gadget_set_selfpowered,
>>   	.pullup			= dwc3_gadget_pullup,
>>   	.udc_start		= dwc3_gadget_start,
>> -- 
>> 2.7.4
>>
> 
> Thanks,
> Thinh

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Elson Serrao]

On 1/19/2023 5:02 AM, Greg KH wrote:
> On Tue, Jan 17, 2023 at 01:55:03PM -0800, Elson Roy Serrao wrote:
>> Add a flag to indicate whether the gadget is capable
>> of sending remote wakeup to the host.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>   drivers/usb/gadget/composite.c | 3 +++
>>   include/linux/usb/gadget.h     | 2 ++
>>   2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index 403563c..b83963a 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -965,6 +965,9 @@ static int set_config(struct usb_composite_dev *cdev,
>>   	else
>>   		usb_gadget_clear_selfpowered(gadget);
>>   
>> +	if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes)
>> +		gadget->rw_capable = 1;
>> +
>>   	usb_gadget_vbus_draw(gadget, power);
>>   	if (result >= 0 && cdev->delayed_status)
>>   		result = USB_GADGET_DELAYED_STATUS;
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index dc3092c..15785f8 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -385,6 +385,7 @@ struct usb_gadget_ops {
>>    *	indicates that it supports LPM as per the LPM ECN & errata.
>>    * @irq: the interrupt number for device controller.
>>    * @id_number: a unique ID number for ensuring that gadget names are distinct
>> + * @rw_capable: True if the gadget is capable of sending remote wakeup.
>>    *
>>    * Gadgets have a mostly-portable "gadget driver" implementing device
>>    * functions, handling all usb configurations and interfaces.  Gadget
>> @@ -446,6 +447,7 @@ struct usb_gadget {
>>   	unsigned			lpm_capable:1;
>>   	int				irq;
>>   	int				id_number;
>> +	unsigned			rw_capable:1;
> 
> Why not put this by the other bitfields in this structure?
> 
> thanks,
> 
> greg k-h

Done. I will make that change

Thanks,
Elson

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Elson Serrao]

On 1/18/2023 5:44 PM, Thinh Nguyen wrote:
> On Tue, Jan 17, 2023, Elson Roy Serrao wrote:
>> Add a flag to indicate whether the gadget is capable
>> of sending remote wakeup to the host.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>   drivers/usb/gadget/composite.c | 3 +++
>>   include/linux/usb/gadget.h     | 2 ++
>>   2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index 403563c..b83963a 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -965,6 +965,9 @@ static int set_config(struct usb_composite_dev *cdev,
>>   	else
>>   		usb_gadget_clear_selfpowered(gadget);
>>   
>> +	if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes)
>> +		gadget->rw_capable = 1;
> 
> Some device may not support remote wakeup. gadget->rw_capable should be
> set and reported by the UDC. May need a gadget ops to enable remote
> wakeup here.
> 
> Thanks,
> Thinh
> 
Not exactly clear on which parameter in UDC decides whether a device 
supports remote wakeup. Here I have this flag just to indicate whether 
the connected device is rw capable based on the bmAttributes populated 
in the config descriptor. If the UDC doesnt have a callback for remote 
wakeup we have that check when calling the gadget op in udc/core.c (have 
added a similar check in usb_func_wakeup() also ).

int usb_gadget_wakeup(struct usb_gadget *gadget)
{
	int ret = 0;

	if (!gadget->ops->wakeup) {
		ret = -EOPNOTSUPP;
		goto out;

Thanks
Elson

>> +
>>   	usb_gadget_vbus_draw(gadget, power);
>>   	if (result >= 0 && cdev->delayed_status)
>>   		result = USB_GADGET_DELAYED_STATUS;
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index dc3092c..15785f8 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -385,6 +385,7 @@ struct usb_gadget_ops {
>>    *	indicates that it supports LPM as per the LPM ECN & errata.
>>    * @irq: the interrupt number for device controller.
>>    * @id_number: a unique ID number for ensuring that gadget names are distinct
>> + * @rw_capable: True if the gadget is capable of sending remote wakeup.
>>    *
>>    * Gadgets have a mostly-portable "gadget driver" implementing device
>>    * functions, handling all usb configurations and interfaces.  Gadget
>> @@ -446,6 +447,7 @@ struct usb_gadget {
>>   	unsigned			lpm_capable:1;
>>   	int				irq;
>>   	int				id_number;
>> +	unsigned			rw_capable:1;
>>   };
>>   #define work_to_gadget(w)	(container_of((w), struct usb_gadget, work))
>>   
>> -- 
>> 2.7.4

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Thinh Nguyen]

On Thu, Jan 19, 2023, Elson Serrao wrote:
> 
> 
> On 1/18/2023 5:44 PM, Thinh Nguyen wrote:
> > On Tue, Jan 17, 2023, Elson Roy Serrao wrote:
> > > Add a flag to indicate whether the gadget is capable
> > > of sending remote wakeup to the host.
> > > 
> > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> > > ---
> > >   drivers/usb/gadget/composite.c | 3 +++
> > >   include/linux/usb/gadget.h     | 2 ++
> > >   2 files changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> > > index 403563c..b83963a 100644
> > > --- a/drivers/usb/gadget/composite.c
> > > +++ b/drivers/usb/gadget/composite.c
> > > @@ -965,6 +965,9 @@ static int set_config(struct usb_composite_dev *cdev,
> > >   	else
> > >   		usb_gadget_clear_selfpowered(gadget);
> > > +	if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes)
> > > +		gadget->rw_capable = 1;
> > 
> > Some device may not support remote wakeup. gadget->rw_capable should be
> > set and reported by the UDC. May need a gadget ops to enable remote
> > wakeup here.
> > 
> > Thanks,
> > Thinh
> > 
> Not exactly clear on which parameter in UDC decides whether a device
> supports remote wakeup. Here I have this flag just to indicate whether the
> connected device is rw capable based on the bmAttributes populated in the
> config descriptor. If the UDC doesnt have a callback for remote wakeup we
> have that check when calling the gadget op in udc/core.c (have added a
> similar check in usb_func_wakeup() also ).

That flag describes the gadget's capability, not the device
configuration. However, it's being used as a configuration flag.

Thanks,
Thinh

> 
> int usb_gadget_wakeup(struct usb_gadget *gadget)
> {
> 	int ret = 0;
> 
> 	if (!gadget->ops->wakeup) {
> 		ret = -EOPNOTSUPP;
> 		goto out;
> 
> Thanks
> Elson
> 
> > > +
> > >   	usb_gadget_vbus_draw(gadget, power);
> > >   	if (result >= 0 && cdev->delayed_status)
> > >   		result = USB_GADGET_DELAYED_STATUS;
> > > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> > > index dc3092c..15785f8 100644
> > > --- a/include/linux/usb/gadget.h
> > > +++ b/include/linux/usb/gadget.h
> > > @@ -385,6 +385,7 @@ struct usb_gadget_ops {
> > >    *	indicates that it supports LPM as per the LPM ECN & errata.
> > >    * @irq: the interrupt number for device controller.
> > >    * @id_number: a unique ID number for ensuring that gadget names are distinct
> > > + * @rw_capable: True if the gadget is capable of sending remote wakeup.
> > >    *

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Elson Serrao]

On 1/19/2023 5:15 PM, Thinh Nguyen wrote:
> On Thu, Jan 19, 2023, Elson Serrao wrote:
>>
>>
>> On 1/18/2023 5:44 PM, Thinh Nguyen wrote:
>>> On Tue, Jan 17, 2023, Elson Roy Serrao wrote:
>>>> Add a flag to indicate whether the gadget is capable
>>>> of sending remote wakeup to the host.
>>>>
>>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>>> ---
>>>>    drivers/usb/gadget/composite.c | 3 +++
>>>>    include/linux/usb/gadget.h     | 2 ++
>>>>    2 files changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>>>> index 403563c..b83963a 100644
>>>> --- a/drivers/usb/gadget/composite.c
>>>> +++ b/drivers/usb/gadget/composite.c
>>>> @@ -965,6 +965,9 @@ static int set_config(struct usb_composite_dev *cdev,
>>>>    	else
>>>>    		usb_gadget_clear_selfpowered(gadget);
>>>> +	if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes)
>>>> +		gadget->rw_capable = 1;
>>>
>>> Some device may not support remote wakeup. gadget->rw_capable should be
>>> set and reported by the UDC. May need a gadget ops to enable remote
>>> wakeup here.
>>>
>>> Thanks,
>>> Thinh
>>>
>> Not exactly clear on which parameter in UDC decides whether a device
>> supports remote wakeup. Here I have this flag just to indicate whether the
>> connected device is rw capable based on the bmAttributes populated in the
>> config descriptor. If the UDC doesnt have a callback for remote wakeup we
>> have that check when calling the gadget op in udc/core.c (have added a
>> similar check in usb_func_wakeup() also ).
> 
> That flag describes the gadget's capability, not the device
> configuration. However, it's being used as a configuration flag.
> 
> Thanks,
> Thinh
> 

Thank you for the clarification. Please let me know if below approach 
where we consider both gadget's capability and device configuration fine?

if (gadget->ops->wakeup || gadget->ops->func_wakeup)
    gadget->rw_capable = USB_CONFIG_ATT_WAKEUP & c->bmAttributes ? 1: 0;

Thanks
Elson
>>
>> int usb_gadget_wakeup(struct usb_gadget *gadget)
>> {
>> 	int ret = 0;
>>
>> 	if (!gadget->ops->wakeup) {
>> 		ret = -EOPNOTSUPP;
>> 		goto out;
>>
>> Thanks
>> Elson
>>
>>>> +
>>>>    	usb_gadget_vbus_draw(gadget, power);
>>>>    	if (result >= 0 && cdev->delayed_status)
>>>>    		result = USB_GADGET_DELAYED_STATUS;
>>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>>> index dc3092c..15785f8 100644
>>>> --- a/include/linux/usb/gadget.h
>>>> +++ b/include/linux/usb/gadget.h
>>>> @@ -385,6 +385,7 @@ struct usb_gadget_ops {
>>>>     *	indicates that it supports LPM as per the LPM ECN & errata.
>>>>     * @irq: the interrupt number for device controller.
>>>>     * @id_number: a unique ID number for ensuring that gadget names are distinct
>>>> + * @rw_capable: True if the gadget is capable of sending remote wakeup.
>>>>     *

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Thinh Nguyen]

On Fri, Jan 20, 2023, Elson Serrao wrote:
> 
> 
> On 1/19/2023 5:15 PM, Thinh Nguyen wrote:
> > On Thu, Jan 19, 2023, Elson Serrao wrote:
> > > 
> > > 
> > > On 1/18/2023 5:44 PM, Thinh Nguyen wrote:
> > > > On Tue, Jan 17, 2023, Elson Roy Serrao wrote:
> > > > > Add a flag to indicate whether the gadget is capable
> > > > > of sending remote wakeup to the host.
> > > > > 
> > > > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> > > > > ---
> > > > >    drivers/usb/gadget/composite.c | 3 +++
> > > > >    include/linux/usb/gadget.h     | 2 ++
> > > > >    2 files changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> > > > > index 403563c..b83963a 100644
> > > > > --- a/drivers/usb/gadget/composite.c
> > > > > +++ b/drivers/usb/gadget/composite.c
> > > > > @@ -965,6 +965,9 @@ static int set_config(struct usb_composite_dev *cdev,
> > > > >    	else
> > > > >    		usb_gadget_clear_selfpowered(gadget);
> > > > > +	if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes)
> > > > > +		gadget->rw_capable = 1;
> > > > 
> > > > Some device may not support remote wakeup. gadget->rw_capable should be
> > > > set and reported by the UDC. May need a gadget ops to enable remote
> > > > wakeup here.
> > > > 
> > > > Thanks,
> > > > Thinh
> > > > 
> > > Not exactly clear on which parameter in UDC decides whether a device
> > > supports remote wakeup. Here I have this flag just to indicate whether the
> > > connected device is rw capable based on the bmAttributes populated in the
> > > config descriptor. If the UDC doesnt have a callback for remote wakeup we
> > > have that check when calling the gadget op in udc/core.c (have added a
> > > similar check in usb_func_wakeup() also ).
> > 
> > That flag describes the gadget's capability, not the device
> > configuration. However, it's being used as a configuration flag.
> > 
> > Thanks,
> > Thinh
> > 
> 
> Thank you for the clarification. Please let me know if below approach where
> we consider both gadget's capability and device configuration fine?
> 
> if (gadget->ops->wakeup || gadget->ops->func_wakeup)
>    gadget->rw_capable = USB_CONFIG_ATT_WAKEUP & c->bmAttributes ? 1: 0;
> 

The way gadget->rw_capable is named and described, it's a capability
flag. That is, its value shouldn't change from the user config. Perhaps
we don't need that in the usb_gadget, and we can have something that
looks like this:

if (gadget->ops->wakeup && (c->bmAttributes & USB_CONFIG_ATT_WAKEUP))
	usb_gadget_enable_remote_wakeup(g);
else
	usb_gadget_disable_remote_wakeup(g);

The setting of the remote wakeup configuration can be tracked internally
by the dwc3 driver based on the usb_gadget_enable_remote_wakeup call.

Thanks,
Thinh

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Alan Stern]

On Sat, Jan 21, 2023 at 12:21:10AM +0000, Thinh Nguyen wrote:
> The way gadget->rw_capable is named and described, it's a capability
> flag. That is, its value shouldn't change from the user config. Perhaps
> we don't need that in the usb_gadget, and we can have something that
> looks like this:
> 
> if (gadget->ops->wakeup && (c->bmAttributes & USB_CONFIG_ATT_WAKEUP))
> 	usb_gadget_enable_remote_wakeup(g);
> else
> 	usb_gadget_disable_remote_wakeup(g);
> 
> The setting of the remote wakeup configuration can be tracked internally
> by the dwc3 driver based on the usb_gadget_enable_remote_wakeup call.

A UDC design might have multiple versions, some supporting remote wakeup 
and others not.  But drivers generally use a single static 
usb_gadget_ops structure, and they don't modify it at runtime to account 
for hardware differences.  So if a single driver controls those multiple 
versions, you can't rely on the presence of gadget->ops->wakeup to 
indicate whether there actually is hardware remote wakeup support.

Ideally, the usb_gadget structure should have a wakeup_capable flag 
which the UDC driver would set appropriately (probably during its probe 
routine).

Alan Stern

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Thinh Nguyen]

On Fri, Jan 20, 2023, Alan Stern wrote:
> On Sat, Jan 21, 2023 at 12:21:10AM +0000, Thinh Nguyen wrote:
> > The way gadget->rw_capable is named and described, it's a capability
> > flag. That is, its value shouldn't change from the user config. Perhaps
> > we don't need that in the usb_gadget, and we can have something that
> > looks like this:
> > 
> > if (gadget->ops->wakeup && (c->bmAttributes & USB_CONFIG_ATT_WAKEUP))
> > 	usb_gadget_enable_remote_wakeup(g);
> > else
> > 	usb_gadget_disable_remote_wakeup(g);
> > 
> > The setting of the remote wakeup configuration can be tracked internally
> > by the dwc3 driver based on the usb_gadget_enable_remote_wakeup call.
> 
> A UDC design might have multiple versions, some supporting remote wakeup 
> and others not.  But drivers generally use a single static 
> usb_gadget_ops structure, and they don't modify it at runtime to account 
> for hardware differences.  So if a single driver controls those multiple 
> versions, you can't rely on the presence of gadget->ops->wakeup to 
> indicate whether there actually is hardware remote wakeup support.
> 
> Ideally, the usb_gadget structure should have a wakeup_capable flag 
> which the UDC driver would set appropriately (probably during its probe 
> routine).
> 

I was thinking that it can be handled by the
usb_gadget_enable_remote_wakeup() so we can do away with the
wakeup_capable flag.

BR,
Thinh

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Alan Stern]

On Sat, Jan 21, 2023 at 02:02:36AM +0000, Thinh Nguyen wrote:
> On Fri, Jan 20, 2023, Alan Stern wrote:
> > A UDC design might have multiple versions, some supporting remote wakeup 
> > and others not.  But drivers generally use a single static 
> > usb_gadget_ops structure, and they don't modify it at runtime to account 
> > for hardware differences.  So if a single driver controls those multiple 
> > versions, you can't rely on the presence of gadget->ops->wakeup to 
> > indicate whether there actually is hardware remote wakeup support.
> > 
> > Ideally, the usb_gadget structure should have a wakeup_capable flag 
> > which the UDC driver would set appropriately (probably during its probe 
> > routine).
> > 
> 
> I was thinking that it can be handled by the
> usb_gadget_enable_remote_wakeup() so we can do away with the
> wakeup_capable flag.

usb_gadget_enable_remote_wakeup() gets called when the gadget or 
function is suspended, right?  But a gadget driver may want to know long 
before that whether the UDC supports remote wakeup, in order to set up 
its config descriptor correctly.

Alan Stern

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Thinh Nguyen]

On Fri, Jan 20, 2023, Alan Stern wrote:
> On Sat, Jan 21, 2023 at 02:02:36AM +0000, Thinh Nguyen wrote:
> > On Fri, Jan 20, 2023, Alan Stern wrote:
> > > A UDC design might have multiple versions, some supporting remote wakeup 
> > > and others not.  But drivers generally use a single static 
> > > usb_gadget_ops structure, and they don't modify it at runtime to account 
> > > for hardware differences.  So if a single driver controls those multiple 
> > > versions, you can't rely on the presence of gadget->ops->wakeup to 
> > > indicate whether there actually is hardware remote wakeup support.
> > > 
> > > Ideally, the usb_gadget structure should have a wakeup_capable flag 
> > > which the UDC driver would set appropriately (probably during its probe 
> > > routine).
> > > 
> > 
> > I was thinking that it can be handled by the
> > usb_gadget_enable_remote_wakeup() so we can do away with the
> > wakeup_capable flag.
> 
> usb_gadget_enable_remote_wakeup() gets called when the gadget or 
> function is suspended, right?  But a gadget driver may want to know long 
> before that whether the UDC supports remote wakeup, in order to set up 
> its config descriptor correctly.
> 

No, this is to be called during set configuration. If the configuration
doesn't support remote wakeup, the device should not be able to send
remote wakeup.

BR,
Thinh

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Thinh Nguyen]

On Sat, Jan 21, 2023, Thinh Nguyen wrote:
> On Fri, Jan 20, 2023, Alan Stern wrote:
> > On Sat, Jan 21, 2023 at 02:02:36AM +0000, Thinh Nguyen wrote:
> > > On Fri, Jan 20, 2023, Alan Stern wrote:
> > > > A UDC design might have multiple versions, some supporting remote wakeup 
> > > > and others not.  But drivers generally use a single static 
> > > > usb_gadget_ops structure, and they don't modify it at runtime to account 
> > > > for hardware differences.  So if a single driver controls those multiple 
> > > > versions, you can't rely on the presence of gadget->ops->wakeup to 
> > > > indicate whether there actually is hardware remote wakeup support.
> > > > 
> > > > Ideally, the usb_gadget structure should have a wakeup_capable flag 
> > > > which the UDC driver would set appropriately (probably during its probe 
> > > > routine).
> > > > 
> > > 
> > > I was thinking that it can be handled by the
> > > usb_gadget_enable_remote_wakeup() so we can do away with the
> > > wakeup_capable flag.
> > 
> > usb_gadget_enable_remote_wakeup() gets called when the gadget or 
> > function is suspended, right?  But a gadget driver may want to know long 
> > before that whether the UDC supports remote wakeup, in order to set up 
> > its config descriptor correctly.
> > 
> 
> No, this is to be called during set configuration. If the configuration
> doesn't support remote wakeup, the device should not be able to send
> remote wakeup.
> 

On second thought, you're right about the descriptor. It's better to
warn and prevent the remote wakeup bit from being set in the descriptor
if the UDC doesn't support remote wakeup. Warning the user at set
configuration is too late.

So, we need both rw_capable flag and usb_gadget_enable_remote_wakeup().

Thanks,
Thinh

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Elson Serrao]

On 1/23/2023 11:33 AM, Thinh Nguyen wrote:
> On Sat, Jan 21, 2023, Thinh Nguyen wrote:
>> On Fri, Jan 20, 2023, Alan Stern wrote:
>>> On Sat, Jan 21, 2023 at 02:02:36AM +0000, Thinh Nguyen wrote:
>>>> On Fri, Jan 20, 2023, Alan Stern wrote:
>>>>> A UDC design might have multiple versions, some supporting remote wakeup
>>>>> and others not.  But drivers generally use a single static
>>>>> usb_gadget_ops structure, and they don't modify it at runtime to account
>>>>> for hardware differences.  So if a single driver controls those multiple
>>>>> versions, you can't rely on the presence of gadget->ops->wakeup to
>>>>> indicate whether there actually is hardware remote wakeup support.
>>>>>
>>>>> Ideally, the usb_gadget structure should have a wakeup_capable flag
>>>>> which the UDC driver would set appropriately (probably during its probe
>>>>> routine).
>>>>>
>>>>
>>>> I was thinking that it can be handled by the
>>>> usb_gadget_enable_remote_wakeup() so we can do away with the
>>>> wakeup_capable flag.
>>>
>>> usb_gadget_enable_remote_wakeup() gets called when the gadget or
>>> function is suspended, right?  But a gadget driver may want to know long
>>> before that whether the UDC supports remote wakeup, in order to set up
>>> its config descriptor correctly.
>>>
>>
>> No, this is to be called during set configuration. If the configuration
>> doesn't support remote wakeup, the device should not be able to send
>> remote wakeup.
>>
> 
> On second thought, you're right about the descriptor. It's better to
> warn and prevent the remote wakeup bit from being set in the descriptor
> if the UDC doesn't support remote wakeup. Warning the user at set
> configuration is too late.
> 
> So, we need both rw_capable flag and usb_gadget_enable_remote_wakeup().
> 
> Thanks,
> Thinh

Do we need usb_gadget_enable_remote_wakeup() gadget-op ?
As per the discussion, we can have rw_capable flag in usb_gadget struct 
and set it during gagdet init/probe if the UDC supports resume 
signalling OR wants the remote wakeup feature to be enabled.
This flag now represents UDC capability to initiate resume signalling.

During enumeration phase, when preparing the config descriptor we can 
use gadget->rw_capable flag to rightly modify the remote wakeup
bit. Based on this, host will decide whether to arm the device for 
remote wakeup or not.

For gadget->ops->wakeup callback support we already have explicit checks 
when invoking this gadget op and device wont be able to send remote 
wakeup if callback support doesn't exist.
Please let me know if I am missing something.

Thanks
Elson

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Thinh Nguyen]

On Mon, Jan 23, 2023, Elson Serrao wrote:
> 
> 
> On 1/23/2023 11:33 AM, Thinh Nguyen wrote:
> > On Sat, Jan 21, 2023, Thinh Nguyen wrote:
> > > On Fri, Jan 20, 2023, Alan Stern wrote:
> > > > On Sat, Jan 21, 2023 at 02:02:36AM +0000, Thinh Nguyen wrote:
> > > > > On Fri, Jan 20, 2023, Alan Stern wrote:
> > > > > > A UDC design might have multiple versions, some supporting remote wakeup
> > > > > > and others not.  But drivers generally use a single static
> > > > > > usb_gadget_ops structure, and they don't modify it at runtime to account
> > > > > > for hardware differences.  So if a single driver controls those multiple
> > > > > > versions, you can't rely on the presence of gadget->ops->wakeup to
> > > > > > indicate whether there actually is hardware remote wakeup support.
> > > > > > 
> > > > > > Ideally, the usb_gadget structure should have a wakeup_capable flag
> > > > > > which the UDC driver would set appropriately (probably during its probe
> > > > > > routine).
> > > > > > 
> > > > > 
> > > > > I was thinking that it can be handled by the
> > > > > usb_gadget_enable_remote_wakeup() so we can do away with the
> > > > > wakeup_capable flag.
> > > > 
> > > > usb_gadget_enable_remote_wakeup() gets called when the gadget or
> > > > function is suspended, right?  But a gadget driver may want to know long
> > > > before that whether the UDC supports remote wakeup, in order to set up
> > > > its config descriptor correctly.
> > > > 
> > > 
> > > No, this is to be called during set configuration. If the configuration
> > > doesn't support remote wakeup, the device should not be able to send
> > > remote wakeup.
> > > 
> > 
> > On second thought, you're right about the descriptor. It's better to
> > warn and prevent the remote wakeup bit from being set in the descriptor
> > if the UDC doesn't support remote wakeup. Warning the user at set
> > configuration is too late.
> > 
> > So, we need both rw_capable flag and usb_gadget_enable_remote_wakeup().
> > 
> > Thanks,
> > Thinh
> 
> Do we need usb_gadget_enable_remote_wakeup() gadget-op ?
> As per the discussion, we can have rw_capable flag in usb_gadget struct and
> set it during gagdet init/probe if the UDC supports resume signalling OR
> wants the remote wakeup feature to be enabled.
> This flag now represents UDC capability to initiate resume signalling.
> 
> During enumeration phase, when preparing the config descriptor we can use
> gadget->rw_capable flag to rightly modify the remote wakeup
> bit. Based on this, host will decide whether to arm the device for remote
> wakeup or not.
> 

If the configuration doesn't allow remote wakeup, the device should not
be able to send signal to wake up the host regardless whether the host
armed the device for remote wake or not.

The rw_capable flag will inform the composite layer whether the UDC is
capable of remote wakeup. The composite layer needs to tell the UDC
whether the configuration allows for remote wakeup.

Whether it's usb_gadget_enable_remote_wakeup() or the remote wakeup bit
in the bmAttribute, the composite layer needs to communicate that to the
controller driver. But we should not expect the UDC driver to parse for
that bit. The prepared control transfer from the the composite layer
should be abstracted from the UDC driver.

> For gadget->ops->wakeup callback support we already have explicit checks
> when invoking this gadget op and device wont be able to send remote wakeup
> if callback support doesn't exist.
> Please let me know if I am missing something.

As mentioned previously, we should also warn the user if the UDC doesn't
support remote wakeup and prevent the remote wakeup bit being set in the
descriptor.

Let me know if it makes sense.

Thanks,
Thinh

Re: [PATCH v2 1/5] usb: gadget: Add remote wakeup capable flag

[Elson Serrao]

On 1/23/2023 3:02 PM, Thinh Nguyen wrote:
> On Mon, Jan 23, 2023, Elson Serrao wrote:
>>
>>
>> On 1/23/2023 11:33 AM, Thinh Nguyen wrote:
>>> On Sat, Jan 21, 2023, Thinh Nguyen wrote:
>>>> On Fri, Jan 20, 2023, Alan Stern wrote:
>>>>> On Sat, Jan 21, 2023 at 02:02:36AM +0000, Thinh Nguyen wrote:
>>>>>> On Fri, Jan 20, 2023, Alan Stern wrote:
>>>>>>> A UDC design might have multiple versions, some supporting remote wakeup
>>>>>>> and others not.  But drivers generally use a single static
>>>>>>> usb_gadget_ops structure, and they don't modify it at runtime to account
>>>>>>> for hardware differences.  So if a single driver controls those multiple
>>>>>>> versions, you can't rely on the presence of gadget->ops->wakeup to
>>>>>>> indicate whether there actually is hardware remote wakeup support.
>>>>>>>
>>>>>>> Ideally, the usb_gadget structure should have a wakeup_capable flag
>>>>>>> which the UDC driver would set appropriately (probably during its probe
>>>>>>> routine).
>>>>>>>
>>>>>>
>>>>>> I was thinking that it can be handled by the
>>>>>> usb_gadget_enable_remote_wakeup() so we can do away with the
>>>>>> wakeup_capable flag.
>>>>>
>>>>> usb_gadget_enable_remote_wakeup() gets called when the gadget or
>>>>> function is suspended, right?  But a gadget driver may want to know long
>>>>> before that whether the UDC supports remote wakeup, in order to set up
>>>>> its config descriptor correctly.
>>>>>
>>>>
>>>> No, this is to be called during set configuration. If the configuration
>>>> doesn't support remote wakeup, the device should not be able to send
>>>> remote wakeup.
>>>>
>>>
>>> On second thought, you're right about the descriptor. It's better to
>>> warn and prevent the remote wakeup bit from being set in the descriptor
>>> if the UDC doesn't support remote wakeup. Warning the user at set
>>> configuration is too late.
>>>
>>> So, we need both rw_capable flag and usb_gadget_enable_remote_wakeup().
>>>
>>> Thanks,
>>> Thinh
>>
>> Do we need usb_gadget_enable_remote_wakeup() gadget-op ?
>> As per the discussion, we can have rw_capable flag in usb_gadget struct and
>> set it during gagdet init/probe if the UDC supports resume signalling OR
>> wants the remote wakeup feature to be enabled.
>> This flag now represents UDC capability to initiate resume signalling.
>>
>> During enumeration phase, when preparing the config descriptor we can use
>> gadget->rw_capable flag to rightly modify the remote wakeup
>> bit. Based on this, host will decide whether to arm the device for remote
>> wakeup or not.
>>
> 
> If the configuration doesn't allow remote wakeup, the device should not
> be able to send signal to wake up the host regardless whether the host
> armed the device for remote wake or not.
> 
> The rw_capable flag will inform the composite layer whether the UDC is
> capable of remote wakeup. The composite layer needs to tell the UDC
> whether the configuration allows for remote wakeup.
> 
> Whether it's usb_gadget_enable_remote_wakeup() or the remote wakeup bit
> in the bmAttribute, the composite layer needs to communicate that to the
> controller driver. But we should not expect the UDC driver to parse for
> that bit. The prepared control transfer from the the composite layer
> should be abstracted from the UDC driver.
> 
>> For gadget->ops->wakeup callback support we already have explicit checks
>> when invoking this gadget op and device wont be able to send remote wakeup
>> if callback support doesn't exist.
>> Please let me know if I am missing something.
> 
> As mentioned previously, we should also warn the user if the UDC doesn't
> support remote wakeup and prevent the remote wakeup bit being set in the
> descriptor.
> 
> Let me know if it makes sense.
> 
> Thanks,
> Thinh

Sure. That makes sense.
As discussed I will modify this patch like below
1.)gadget->rw_capable flag to represent the UDC wakeup capability. Used 
to configure/modify rw bit in bmAtrributes before sending ConfigDesc. 
Also warn the user if rw bit is set but UDC doesnt support it.

2.) usb_gadget_enable_remote_wakeup() gadget op for composite layer to 
inform UDC layer. Maintain an internal flag in UDC driver to guard 
against invoking wakeup callbacks when user has not configured for 
remote wakeup.

Thanks
Elson