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

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

[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.
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 super-speed/super-speed-plus 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: 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
  usb: gadget: f_ecm: Add function suspend and wakeup support

 drivers/usb/dwc3/core.h               |  5 ++
 drivers/usb/dwc3/ep0.c                | 16 +++---
 drivers/usb/dwc3/gadget.c             | 99 +++++++++++++++++++++++++++++++++--
 drivers/usb/gadget/composite.c        | 32 +++++++++++
 drivers/usb/gadget/function/f_ecm.c   | 67 ++++++++++++++++++++++++
 drivers/usb/gadget/function/u_ether.c | 98 ++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/function/u_ether.h |  6 +++
 drivers/usb/gadget/udc/core.c         |  9 ++++
 include/linux/usb/composite.h         |  1 +
 include/linux/usb/gadget.h            |  2 +
 10 files changed, 324 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH 1/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
accordingly.

Some hosts may take longer time to initiate the resume
signaling after device triggers a remote wakeup. So improve the
gadget_wakeup op to interrupt based rather than polling based by
enabling link status change events.

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

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 4fe4287..3306b1c 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1113,6 +1113,8 @@ struct dwc3_scratchpad_array {
  *		     address.
  * @num_ep_resized: carries the current number endpoints which have had its tx
  *		    fifo resized.
+ * @is_remote_wakeup_enabled: remote wakeup status from host perspective
+ * @is_gadget_wakeup: remote wakeup requested via gadget op.
  */
 struct dwc3 {
 	struct work_struct	drd_work;
@@ -1326,6 +1328,8 @@ struct dwc3 {
 	int			max_cfg_eps;
 	int			last_fifo_depth;
 	int			num_ep_resized;
+	bool			is_remote_wakeup_enabled;
+	bool			is_gadget_wakeup;
 };
 
 #define INCRX_BURST_MODE 0
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 197af63..4cc3d3a 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -353,6 +353,9 @@ 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->is_remote_wakeup_enabled <<
+						USB_DEVICE_REMOTE_WAKEUP;
 		}
 
 		break;
@@ -473,6 +476,7 @@ static int dwc3_ep0_handle_device(struct dwc3 *dwc,
 
 	switch (wValue) {
 	case USB_DEVICE_REMOTE_WAKEUP:
+		dwc->is_remote_wakeup_enabled = 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 4366c45..d6697da 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2232,6 +2232,22 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
 
 /* -------------------------------------------------------------------------- */
 
+static void linksts_change_events_set(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);
+
+	/* Required to complete this operation before returning */
+	mb();
+}
+
 static int dwc3_gadget_get_frame(struct usb_gadget *g)
 {
 	struct dwc3		*dwc = gadget_to_dwc(g);
@@ -2270,9 +2286,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
 		return -EINVAL;
 	}
 
+	linksts_change_events_set(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");
+		linksts_change_events_set(dwc, false);
+		dwc->is_gadget_wakeup = false;
 		return ret;
 	}
 
@@ -2284,9 +2304,15 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
 		dwc3_writel(dwc->regs, DWC3_DCTL, reg);
 	}
 
+	/* If remote wakeup is triggered from function driver, bail out.
+	 * Since link status change events are enabled we would receive
+	 * an U0 event when wakeup is successful.
+	 */
+	if (dwc->is_gadget_wakeup)
+		return -EAGAIN;
+
 	/* poll until Link State changes to ON */
 	retries = 20000;
-
 	while (retries--) {
 		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
 
@@ -2295,6 +2321,8 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
 			break;
 	}
 
+	linksts_change_events_set(dwc, false);
+
 	if (DWC3_DSTS_USBLNKST(reg) != DWC3_LINK_STATE_U0) {
 		dev_err(dwc->dev, "failed to send remote wakeup\n");
 		return -EINVAL;
@@ -2310,7 +2338,20 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
 	int			ret;
 
 	spin_lock_irqsave(&dwc->lock, flags);
+	if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled) {
+		dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
+		ret =  -EPERM;
+		goto out;
+	}
+	if (dwc->is_gadget_wakeup) {
+		dev_err(dwc->dev, "%s: remote wakeup in progress\n", __func__);
+		ret = -EINVAL;
+		goto out;
+	}
+	dwc->is_gadget_wakeup = true;
 	ret = __dwc3_gadget_wakeup(dwc);
+
+out:
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return ret;
@@ -2766,6 +2807,9 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	dwc->gadget_driver	= driver;
+	linksts_change_events_set(dwc, false);
+	dwc->is_remote_wakeup_enabled = false;
+	dwc->is_gadget_wakeup = false;
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	return 0;
@@ -2785,6 +2829,9 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
 
 	spin_lock_irqsave(&dwc->lock, flags);
 	dwc->gadget_driver	= NULL;
+	linksts_change_events_set(dwc, false);
+	dwc->is_remote_wakeup_enabled = false;
+	dwc->is_gadget_wakeup = false;
 	dwc->max_cfg_eps = 0;
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
@@ -3768,6 +3815,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
 	usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
 
 	dwc->connected = false;
+	linksts_change_events_set(dwc, false);
+	dwc->is_gadget_wakeup = false;
 }
 
 static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
@@ -3855,6 +3904,10 @@ 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);
+
+	dwc->is_remote_wakeup_enabled = false;
+	linksts_change_events_set(dwc, false);
+	dwc->is_gadget_wakeup = false;
 }
 
 static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
@@ -3998,8 +4051,9 @@ 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)
 {
+	enum dwc3_link_state next = evtinfo & DWC3_LINK_STATE_MASK;
 	/*
 	 * TODO take core out of low power mode when that's
 	 * implemented.
@@ -4010,6 +4064,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
 		dwc->gadget_driver->resume(dwc->gadget);
 		spin_lock(&dwc->lock);
 	}
+
+	dwc->link_state = next;
 }
 
 static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
@@ -4091,6 +4147,13 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
 	}
 
 	switch (next) {
+	case DWC3_LINK_STATE_U0:
+		if (dwc->is_gadget_wakeup) {
+			linksts_change_events_set(dwc, false);
+			dwc3_resume_gadget(dwc);
+			dwc->is_gadget_wakeup = false;
+		}
+		break;
 	case DWC3_LINK_STATE_U1:
 		if (dwc->speed == USB_SPEED_SUPER)
 			dwc3_suspend_gadget(dwc);
@@ -4159,7 +4222,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 2/5] usb: gadget: Add function wakeup support

[Elson Roy Serrao]

An interface which is in function suspend state has to send a function
wakeup notification to the host in case it needs to initate any data
transfer. One notable difference between this and the existing remote
wakeup mechanism is that this can be called per-interface, and a UDC
would need to know the particular interface number to convey in its
Device Notification transaction packet.  Hence, we need to introduce
a new callback in the gadget_ops structure that UDC device drivers
can implement.  Similarly add a convenience function in the composite
driver which function drivers can call. Add support to handle such
requests in the composite layer and invoke the gadget op.

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

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 403563c..6bdce23 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -490,6 +490,38 @@ 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;
+	unsigned long flags;
+
+	if (!func || !func->config || !func->config->cdev ||
+	    !func->config->cdev->gadget)
+		return -EINVAL;
+
+	DBG(func->config->cdev, "%s function wakeup\n", func->name);
+
+	spin_lock_irqsave(&func->config->cdev->lock, flags);
+
+	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);
+		ret = -EINVAL;
+		goto err;
+	}
+
+	ret = usb_gadget_func_wakeup(func->config->cdev->gadget, id);
+
+err:
+	spin_unlock_irqrestore(&func->config->cdev->lock, flags);
+
+	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 7886497..fe5c504 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -816,6 +816,15 @@ int usb_gadget_activate(struct usb_gadget *gadget)
 }
 EXPORT_SYMBOL_GPL(usb_gadget_activate);
 
+int usb_gadget_func_wakeup(struct usb_gadget *gadget, int interface_id)
+{
+	if (gadget->speed < USB_SPEED_SUPER || !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 9d27622..31b35d7 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -254,6 +254,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 3ad58b7..76f9de4 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -311,6 +311,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 3/5] usb: dwc3: Add function suspend and function wakeup support

[Elson Roy Serrao]

USB host sends function suspend and resume notifications to
device through SET_FEATURE setup packet. This packet is directed
to the interface to which function suspend/resume is intended to.
Add support to handle this packet by delegating the request to composite
layer. To exit from function suspend the interface needs to trigger a
function wakeup in case it wants to initate a data transfer. Expose a
new gadget op so that an interface can send function wakeup request to
the host.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/dwc3/core.h   |  1 +
 drivers/usb/dwc3/ep0.c    | 12 ++++--------
 drivers/usb/dwc3/gadget.c | 30 ++++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 3306b1c..e08e522 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -519,6 +519,7 @@
 #define DWC3_DGCMD_SET_SCRATCHPAD_ADDR_LO	0x04
 #define DWC3_DGCMD_SET_SCRATCHPAD_ADDR_HI	0x05
 
+#define DWC3_DGCMD_XMIT_DEV		0x07
 #define DWC3_DGCMD_SELECTED_FIFO_FLUSH	0x09
 #define DWC3_DGCMD_ALL_FIFO_FLUSH	0x0a
 #define DWC3_DGCMD_SET_ENDPOINT_NRDY	0x0c
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 4cc3d3a..cedc890 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)
@@ -365,7 +367,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);
@@ -511,13 +513,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 d6697da..0b2947e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2357,6 +2357,35 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
 	return ret;
 }
 
+static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int interface_id)
+{
+	int	ret = 0;
+	u32	reg;
+	struct	dwc3 *dwc = gadget_to_dwc(g);
+
+	reg = dwc3_readl(dwc->regs, DWC3_DSTS);
+
+	/*
+	 * If the link is in LPM, first bring the link to U0
+	 * before triggering function wakeup. Ideally this
+	 * needs to be expanded to other LPMs as well in
+	 * addition to U3
+	 */
+	if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U3) {
+		dwc3_gadget_wakeup(g);
+		return -EAGAIN;
+	}
+
+	ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_XMIT_DEV,
+					       0x1 | (interface_id << 4));
+
+	if (ret)
+		dev_err(dwc->dev, "Function wakeup HW command failed, ret %d\n",
+			ret);
+
+	return ret;
+}
+
 static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
 		int is_selfpowered)
 {
@@ -2978,6 +3007,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 4/5] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support

[Elson Roy Serrao]

When the host sends a suspend notification to the device, handle
the suspend callbacks in the function driver. 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   | 22 +++++++++++
 drivers/usb/gadget/function/u_ether.c | 72 +++++++++++++++++++++++++++++++++++
 drivers/usb/gadget/function/u_ether.h |  4 ++
 3 files changed, 98 insertions(+)

diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
index ffe2486..fb1dec3 100644
--- a/drivers/usb/gadget/function/f_ecm.c
+++ b/drivers/usb/gadget/function/f_ecm.c
@@ -889,6 +889,26 @@ 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;
+
+	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;
+
+	DBG(cdev, "ECM Resume\n");
+
+	gether_resume(&ecm->port);
+}
+
 static void ecm_free(struct usb_function *f)
 {
 	struct f_ecm *ecm;
@@ -956,6 +976,8 @@ 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.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 7887def..78391de 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -471,6 +471,18 @@ 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 = -EOPNOTSUPP;
+	struct usb_function *func = &port->func;
+	struct usb_gadget *gadget = func->config->cdev->gadget;
+
+	if (gadget->speed < USB_SPEED_SUPER)
+		ret = usb_gadget_wakeup(gadget);
+
+	return ret;
+}
+
 static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
 					struct net_device *net)
 {
@@ -490,6 +502,25 @@ 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");
+		retval = ether_wakeup_host(dev->port_usb);
+		if (retval) {
+			/*
+			 * Either the remote wakeup is not yet complete or
+			 * wakeup is not supported. In either case we cannot
+			 * send data and hence inform the upper layer to stop
+			 * sending data. The queue is re-started in resume
+			 * callback once the remote wakeup is successful or when
+			 * host initiated resume happens.
+			 */
+			netif_stop_queue(net);
+			spin_unlock_irqrestore(&dev->lock, flags);
+			return NETDEV_TX_BUSY;
+		}
+	}
+
 	spin_unlock_irqrestore(&dev->lock, flags);
 
 	if (!in) {
@@ -1050,6 +1081,46 @@ 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;
+
+	spin_lock_irqsave(&dev->lock, flags);
+
+	if (netif_queue_stopped(dev->net))
+		netif_start_queue(dev->net);
+
+	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
@@ -1212,6 +1283,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

[PATCH 5/5] usb: gadget: f_ecm: Add function suspend and wakeup support

[Elson Roy Serrao]

USB host sends function suspend setup packet to an interface when there
is no active communication involved. Handle such requests from host so
that the interface goes to function suspend state. For the device to
resume data transfer it can either send a function wakeup notification or
wait for the host initated function resume depending on the function
wakeup capability of the device. Add support to trigger function wakeup.

Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
 drivers/usb/gadget/function/f_ecm.c   | 49 +++++++++++++++++++++++++++++++++--
 drivers/usb/gadget/function/u_ether.c | 32 ++++++++++++++++++++---
 drivers/usb/gadget/function/u_ether.h |  6 +++--
 3 files changed, 80 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
index fb1dec3..8bb7e3c 100644
--- a/drivers/usb/gadget/function/f_ecm.c
+++ b/drivers/usb/gadget/function/f_ecm.c
@@ -54,6 +54,8 @@ struct f_ecm {
 	u8				notify_state;
 	atomic_t			notify_count;
 	bool				is_open;
+	bool				func_wakeup_allowed;
+	bool				func_is_suspended;
 
 	/* FIXME is_open needs some irq-ish locking
 	 * ... possibly the same as port.ioport
@@ -631,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
 		ecm->port.out_ep->desc = NULL;
 	}
 
+	ecm->func_wakeup_allowed = false;
+	ecm->func_is_suspended = false;
 	usb_ep_disable(ecm->notify);
 	ecm->notify->desc = NULL;
 }
@@ -894,9 +898,14 @@ 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 (ecm->func_is_suspended) {
+		DBG(cdev, "Function already suspended\n");
+		return;
+	}
+
 	DBG(cdev, "ECM Suspend\n");
 
-	gether_suspend(&ecm->port);
+	gether_suspend(&ecm->port, ecm->func_wakeup_allowed);
 }
 
 static void ecm_resume(struct usb_function *f)
@@ -906,7 +915,41 @@ static void ecm_resume(struct usb_function *f)
 
 	DBG(cdev, "ECM Resume\n");
 
-	gether_resume(&ecm->port);
+	gether_resume(&ecm->port, ecm->func_is_suspended);
+}
+
+static int ecm_get_status(struct usb_function *f)
+{
+	struct f_ecm *ecm = func_to_ecm(f);
+
+	return (ecm->func_wakeup_allowed ? USB_INTRF_STAT_FUNC_RW : 0) |
+		USB_INTRF_STAT_FUNC_RW_CAP;
+}
+
+static int ecm_func_suspend(struct usb_function *f, u8 options)
+{
+	bool func_wakeup_allowed;
+	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);
+
+	func_wakeup_allowed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8));
+	if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
+		ecm->func_wakeup_allowed = func_wakeup_allowed;
+		if (!ecm->func_is_suspended) {
+			ecm_suspend(f);
+			ecm->func_is_suspended = true;
+		}
+	} else {
+		if (ecm->func_is_suspended) {
+			ecm->func_is_suspended = false;
+			ecm_resume(f);
+		}
+		ecm->func_wakeup_allowed = func_wakeup_allowed;
+	}
+
+	return 0;
 }
 
 static void ecm_free(struct usb_function *f)
@@ -977,6 +1020,8 @@ 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.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 78391de..f3e1c9b 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -477,8 +477,17 @@ static int ether_wakeup_host(struct gether *port)
 	struct usb_function *func = &port->func;
 	struct usb_gadget *gadget = func->config->cdev->gadget;
 
-	if (gadget->speed < USB_SPEED_SUPER)
+	if (gadget->speed >= USB_SPEED_SUPER) {
+		if (!port->func_wakeup_allowed) {
+			DBG(port->ioport, "Function wakeup not allowed\n");
+			return -EOPNOTSUPP;
+		}
+		ret = usb_func_wakeup(func);
+		if (ret)
+			port->is_wakeup_pending = true;
+	} else {
 		ret = usb_gadget_wakeup(gadget);
+	}
 
 	return ret;
 }
@@ -1081,7 +1090,7 @@ 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)
+void gether_suspend(struct gether *link, bool wakeup_allowed)
 {
 	struct eth_dev *dev = link->ioport;
 	unsigned long flags;
@@ -1099,13 +1108,15 @@ void gether_suspend(struct gether *link)
 	}
 	spin_lock_irqsave(&dev->lock, flags);
 	link->is_suspend = true;
+	link->func_wakeup_allowed = wakeup_allowed;
 	spin_unlock_irqrestore(&dev->lock, flags);
 }
 EXPORT_SYMBOL_GPL(gether_suspend);
 
-void gether_resume(struct gether *link)
+void gether_resume(struct gether *link, bool func_suspend)
 {
 	struct eth_dev *dev = link->ioport;
+	struct usb_function *func = &link->func;
 	unsigned long flags;
 
 	if (!dev)
@@ -1113,6 +1124,19 @@ void gether_resume(struct gether *link)
 
 	spin_lock_irqsave(&dev->lock, flags);
 
+	/*
+	 * If the function is in USB3 Function Suspend state, resume is
+	 * canceled. In this case resume is done by a Function Resume request.
+	 */
+	if (func_suspend) {
+		if (link->is_wakeup_pending) {
+			usb_func_wakeup(func);
+			link->is_wakeup_pending = false;
+		}
+		spin_unlock_irqrestore(&dev->lock, flags);
+		return;
+	}
+
 	if (netif_queue_stopped(dev->net))
 		netif_start_queue(dev->net);
 
@@ -1284,6 +1308,8 @@ void gether_disconnect(struct gether *link)
 	spin_lock(&dev->lock);
 	dev->port_usb = NULL;
 	link->is_suspend = false;
+	link->is_wakeup_pending = false;
+	link->func_wakeup_allowed = 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 851ee10..4a6aa646 100644
--- a/drivers/usb/gadget/function/u_ether.h
+++ b/drivers/usb/gadget/function/u_ether.h
@@ -80,6 +80,8 @@ struct gether {
 	void				(*open)(struct gether *);
 	void				(*close)(struct gether *);
 	bool				is_suspend;
+	bool				is_wakeup_pending;
+	bool				func_wakeup_allowed;
 };
 
 #define	DEFAULT_FILTER	(USB_CDC_PACKET_TYPE_BROADCAST \
@@ -259,8 +261,8 @@ 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);
+void gether_suspend(struct gether *link, bool wakeup_allowed);
+void gether_resume(struct gether *link, bool func_suspend);
 
 /* connect/disconnect is handled by individual functions */
 struct net_device *gether_connect(struct gether *);
-- 
2.7.4

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

[Thinh Nguyen]

On 8/2/2022, Elson Roy Serrao wrote:
> USB host sends function suspend and resume notifications to
> device through SET_FEATURE setup packet. This packet is directed
> to the interface to which function suspend/resume is intended to.
> Add support to handle this packet by delegating the request to composite
> layer. To exit from function suspend the interface needs to trigger a
> function wakeup in case it wants to initate a data transfer. Expose a
> new gadget op so that an interface can send function wakeup request to
> the host.
>
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>   drivers/usb/dwc3/core.h   |  1 +
>   drivers/usb/dwc3/ep0.c    | 12 ++++--------
>   drivers/usb/dwc3/gadget.c | 30 ++++++++++++++++++++++++++++++
>   3 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 3306b1c..e08e522 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -519,6 +519,7 @@
>   #define DWC3_DGCMD_SET_SCRATCHPAD_ADDR_LO	0x04
>   #define DWC3_DGCMD_SET_SCRATCHPAD_ADDR_HI	0x05
>   
> +#define DWC3_DGCMD_XMIT_DEV		0x07

This is device notification, so just name this to 
DWC_DGCMD_DEV_NOTIFICATION. Also update debug.h to print the proper 
string for this command.

>   #define DWC3_DGCMD_SELECTED_FIFO_FLUSH	0x09
>   #define DWC3_DGCMD_ALL_FIFO_FLUSH	0x0a
>   #define DWC3_DGCMD_SET_ENDPOINT_NRDY	0x0c
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 4cc3d3a..cedc890 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)
> @@ -365,7 +367,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);

Why do we need to delegate to gadget driver?

We need to check if the host arms the remote wakeup, and we need to 
check if the gadget is capable of remote wakeup. For example, it's odd 
for mass_storage device to be remote capable.

So we need a flag in usb_gadget for that. Have dwc3 check against that 
flag before setting it.

Something like this:

if (dwc->gadget->rw_capable) {
     usb_status = USB_INTRF_STAT_FUNC_RW_CAP;
     if (dwc->rw_armed)
         usb_status |= USB_INTERF_STAT_FUNC_RW;
}


>   
>   	case USB_RECIP_ENDPOINT:
>   		dep = dwc3_wIndex_to_dep(dwc, ctrl->wIndex);
> @@ -511,13 +513,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);

Again, why are we delegating this to the gadget driver? This can be done 
here as something like this:

if (dwc->gadget->rw_capable && set &&
      (wIndex & USB_INTRF_FUNC_SUSPEND_RW))
         dwc->rw_armed = 1;
else
         dwc->rw_armed = 0;

dwc->rw_selected_interface = wIndex & 0xff;

>   		break;
>   	default:
>   		ret = -EINVAL;
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index d6697da..0b2947e 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2357,6 +2357,35 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>   	return ret;
>   }
>   
> +static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int interface_id)

Don't pass this work to the gadget driver. The upperlayer doesn't know 
when to send the function wakeup. The dwc3 driver should send the 
function wakeup when the device is in the state U0, and the dwc3 driver 
gets that info first hand.

> +{
> +	int	ret = 0;
> +	u32	reg;
> +	struct	dwc3 *dwc = gadget_to_dwc(g);
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> +
> +	/*
> +	 * If the link is in LPM, first bring the link to U0
> +	 * before triggering function wakeup. Ideally this
> +	 * needs to be expanded to other LPMs as well in
> +	 * addition to U3
> +	 */
> +	if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U3) {

You're not doing U0 link check here.

> +		dwc3_gadget_wakeup(g);
> +		return -EAGAIN;
> +	}
> +
> +	ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_XMIT_DEV,
> +					       0x1 | (interface_id << 4));

The interface_id here is the dwc->rw_selected_interface as noted earlier.

> +
> +	if (ret)
> +		dev_err(dwc->dev, "Function wakeup HW command failed, ret %d\n",
> +			ret);
> +
> +	return ret;
> +}
> +
>   static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
>   		int is_selfpowered)
>   {
> @@ -2978,6 +3007,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,

BR,
Thinh

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Thinh Nguyen]

On 8/2/2022, Elson Roy Serrao wrote:
> An interface which is in function suspend state has to send a function
> wakeup notification to the host in case it needs to initate any data
> transfer. One notable difference between this and the existing remote
> wakeup mechanism is that this can be called per-interface, and a UDC
> would need to know the particular interface number to convey in its
> Device Notification transaction packet.  Hence, we need to introduce
> a new callback in the gadget_ops structure that UDC device drivers
> can implement.  Similarly add a convenience function in the composite
> driver which function drivers can call. Add support to handle such
> requests in the composite layer and invoke the gadget op.

Sending the function wake notification should be done in the controller 
driver. The controller driver knows when is the proper link state 
(U0/ON) the device is in and would notify the host then.

What we need to add in the usb_gadget is whether the device is remote 
wakeup capable. Something like a flag usb_gadget->rw_capable.

We would also need some functions like usb_gadget_enable_remote_wakeup() 
and usb_gadget_disable_remote_wakeup() for the gadget driver to notify 
the controller driver when it checks against USB_CONFIG_ATT_WAKEUP in 
the bmAttributes configuration.

BR,
Thinh

> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>   drivers/usb/gadget/composite.c | 32 ++++++++++++++++++++++++++++++++
>   drivers/usb/gadget/udc/core.c  |  9 +++++++++
>   include/linux/usb/composite.h  |  1 +
>   include/linux/usb/gadget.h     |  2 ++
>   4 files changed, 44 insertions(+)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 403563c..6bdce23 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -490,6 +490,38 @@ 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;
> +	unsigned long flags;
> +
> +	if (!func || !func->config || !func->config->cdev ||
> +	    !func->config->cdev->gadget)
> +		return -EINVAL;
> +
> +	DBG(func->config->cdev, "%s function wakeup\n", func->name);
> +
> +	spin_lock_irqsave(&func->config->cdev->lock, flags);
> +
> +	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);
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	ret = usb_gadget_func_wakeup(func->config->cdev->gadget, id);
> +
> +err:
> +	spin_unlock_irqrestore(&func->config->cdev->lock, flags);
> +
> +	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 7886497..fe5c504 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -816,6 +816,15 @@ int usb_gadget_activate(struct usb_gadget *gadget)
>   }
>   EXPORT_SYMBOL_GPL(usb_gadget_activate);
>   
> +int usb_gadget_func_wakeup(struct usb_gadget *gadget, int interface_id)
> +{
> +	if (gadget->speed < USB_SPEED_SUPER || !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 9d27622..31b35d7 100644
> --- a/include/linux/usb/composite.h
> +++ b/include/linux/usb/composite.h
> @@ -254,6 +254,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 3ad58b7..76f9de4 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -311,6 +311,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; }

Re: [PATCH 1/5] usb: dwc3: Add remote wakeup handling

[Thinh Nguyen]

On 8/2/2022, Elson Roy Serrao wrote:
> 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
> accordingly.
>
> Some hosts may take longer time to initiate the resume
> signaling after device triggers a remote wakeup. So improve the
> gadget_wakeup op to interrupt based rather than polling based by
> enabling link status change events.
>
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
>   drivers/usb/dwc3/core.h   |  4 +++
>   drivers/usb/dwc3/ep0.c    |  4 +++
>   drivers/usb/dwc3/gadget.c | 69 ++++++++++++++++++++++++++++++++++++++++++++---
>   3 files changed, 74 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 4fe4287..3306b1c 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1113,6 +1113,8 @@ struct dwc3_scratchpad_array {
>    *		     address.
>    * @num_ep_resized: carries the current number endpoints which have had its tx
>    *		    fifo resized.
> + * @is_remote_wakeup_enabled: remote wakeup status from host perspective
> + * @is_gadget_wakeup: remote wakeup requested via gadget op.
>    */
>   struct dwc3 {
>   	struct work_struct	drd_work;
> @@ -1326,6 +1328,8 @@ struct dwc3 {
>   	int			max_cfg_eps;
>   	int			last_fifo_depth;
>   	int			num_ep_resized;
> +	bool			is_remote_wakeup_enabled;
> +	bool			is_gadget_wakeup;
>   };
>   
>   #define INCRX_BURST_MODE 0
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 197af63..4cc3d3a 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -353,6 +353,9 @@ 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->is_remote_wakeup_enabled <<
> +						USB_DEVICE_REMOTE_WAKEUP;

You need to create a new macro for function remote enabled. Name it 
something like USB_DEV_STAT_FUNC_RW_ENABLED to match with others in ch9.h

>   		}
>   
>   		break;
> @@ -473,6 +476,7 @@ static int dwc3_ep0_handle_device(struct dwc3 *dwc,
>   
>   	switch (wValue) {
>   	case USB_DEVICE_REMOTE_WAKEUP:
> +		dwc->is_remote_wakeup_enabled = 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 4366c45..d6697da 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2232,6 +2232,22 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
>   
>   /* -------------------------------------------------------------------------- */
>   
> +static void linksts_change_events_set(struct dwc3 *dwc, bool set)

It should be named something that's consistent with the other functions. 
Something like dwc3_gadget_enable_linkstate_change_event()

> +{
> +	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);
> +
> +	/* Required to complete this operation before returning */
> +	mb();

Why do we need memory barrier here? It's just register write, and we 
already have io barrier for that.

> +}
> +
>   static int dwc3_gadget_get_frame(struct usb_gadget *g)
>   {
>   	struct dwc3		*dwc = gadget_to_dwc(g);
> @@ -2270,9 +2286,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>   		return -EINVAL;
>   	}
>   
> +	linksts_change_events_set(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");
> +		linksts_change_events_set(dwc, false);
> +		dwc->is_gadget_wakeup = false;
>   		return ret;
>   	}
>   
> @@ -2284,9 +2304,15 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>   		dwc3_writel(dwc->regs, DWC3_DCTL, reg);
>   	}
>   
> +	/* If remote wakeup is triggered from function driver, bail out.

Follow this type of doc

/*
  * xxxx
  * yyyy
  */

> +	 * Since link status change events are enabled we would receive
> +	 * an U0 event when wakeup is successful.
> +	 */
> +	if (dwc->is_gadget_wakeup)
> +		return -EAGAIN;
> +
>   	/* poll until Link State changes to ON */
>   	retries = 20000;
> -
>   	while (retries--) {
>   		reg = dwc3_readl(dwc->regs, DWC3_DSTS);
>   
> @@ -2295,6 +2321,8 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
>   			break;
>   	}
>   
> +	linksts_change_events_set(dwc, false);
> +
>   	if (DWC3_DSTS_USBLNKST(reg) != DWC3_LINK_STATE_U0) {
>   		dev_err(dwc->dev, "failed to send remote wakeup\n");
>   		return -EINVAL;
> @@ -2310,7 +2338,20 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
>   	int			ret;
>   
>   	spin_lock_irqsave(&dwc->lock, flags);
> +	if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled) {
> +		dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
> +		ret =  -EPERM;
> +		goto out;
> +	}
> +	if (dwc->is_gadget_wakeup) {
> +		dev_err(dwc->dev, "%s: remote wakeup in progress\n", __func__);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	dwc->is_gadget_wakeup = true;
>   	ret = __dwc3_gadget_wakeup(dwc);
> +
> +out:
>   	spin_unlock_irqrestore(&dwc->lock, flags);
>   
>   	return ret;
> @@ -2766,6 +2807,9 @@ static int dwc3_gadget_start(struct usb_gadget *g,
>   
>   	spin_lock_irqsave(&dwc->lock, flags);
>   	dwc->gadget_driver	= driver;
> +	linksts_change_events_set(dwc, false);
> +	dwc->is_remote_wakeup_enabled = false;
> +	dwc->is_gadget_wakeup = false;
>   	spin_unlock_irqrestore(&dwc->lock, flags);
>   
>   	return 0;
> @@ -2785,6 +2829,9 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>   
>   	spin_lock_irqsave(&dwc->lock, flags);
>   	dwc->gadget_driver	= NULL;
> +	linksts_change_events_set(dwc, false);
> +	dwc->is_remote_wakeup_enabled = false;
> +	dwc->is_gadget_wakeup = false;
>   	dwc->max_cfg_eps = 0;
>   	spin_unlock_irqrestore(&dwc->lock, flags);
>   
> @@ -3768,6 +3815,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
>   	usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
>   
>   	dwc->connected = false;
> +	linksts_change_events_set(dwc, false);
> +	dwc->is_gadget_wakeup = false;
>   }
>   
>   static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
> @@ -3855,6 +3904,10 @@ 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);
> +
> +	dwc->is_remote_wakeup_enabled = false;
> +	linksts_change_events_set(dwc, false);
> +	dwc->is_gadget_wakeup = false;
>   }
>   
>   static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
> @@ -3998,8 +4051,9 @@ 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)
>   {
> +	enum dwc3_link_state next = evtinfo & DWC3_LINK_STATE_MASK;
>   	/*
>   	 * TODO take core out of low power mode when that's
>   	 * implemented.
> @@ -4010,6 +4064,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
>   		dwc->gadget_driver->resume(dwc->gadget);
>   		spin_lock(&dwc->lock);
>   	}
> +
> +	dwc->link_state = next;
>   }
>   
>   static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
> @@ -4091,6 +4147,13 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>   	}
>   
>   	switch (next) {
> +	case DWC3_LINK_STATE_U0:
> +		if (dwc->is_gadget_wakeup) {
> +			linksts_change_events_set(dwc, false);
> +			dwc3_resume_gadget(dwc);
> +			dwc->is_gadget_wakeup = false;
> +		}
> +		break;
>   	case DWC3_LINK_STATE_U1:
>   		if (dwc->speed == USB_SPEED_SUPER)
>   			dwc3_suspend_gadget(dwc);
> @@ -4159,7 +4222,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,

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Elson Serrao]

On 8/2/2022 4:51 PM, Thinh Nguyen wrote:
> On 8/2/2022, Elson Roy Serrao wrote:
>> An interface which is in function suspend state has to send a function
>> wakeup notification to the host in case it needs to initate any data
>> transfer. One notable difference between this and the existing remote
>> wakeup mechanism is that this can be called per-interface, and a UDC
>> would need to know the particular interface number to convey in its
>> Device Notification transaction packet.  Hence, we need to introduce
>> a new callback in the gadget_ops structure that UDC device drivers
>> can implement.  Similarly add a convenience function in the composite
>> driver which function drivers can call. Add support to handle such
>> requests in the composite layer and invoke the gadget op.
> 
> Sending the function wake notification should be done in the controller
> driver. The controller driver knows when is the proper link state
> (U0/ON) the device is in and would notify the host then.
> 
> What we need to add in the usb_gadget is whether the device is remote
> wakeup capable. Something like a flag usb_gadget->rw_capable.
> 
> We would also need some functions like usb_gadget_enable_remote_wakeup()
> and usb_gadget_disable_remote_wakeup() for the gadget driver to notify
> the controller driver when it checks against USB_CONFIG_ATT_WAKEUP in
> the bmAttributes configuration.
> 
> BR,
> Thinh


If we handle this in controller driver, then it would fail to get the 
right interface id when multiple functions have to send function wake 
notification. As per USB3.0 spec (below snippets) a function can be 
independently placed into function suspend state within a composite 
device and each function in function suspend state has to send a 
function wake notification to exit.

USB 3.0 Spec Section 9.2.5.3
"A function may be placed into Function Suspend independently of other 
functions within a composite device"

USB 3.0 Spec Section 9.2.5.4
"A function may signal that it wants to exit from Function Suspend by 
sending a Function Wake Notification to the host if it is enabled for 
function remote wakeup. This applies to single function devices as well 
as multiple function ( i.e., composite) devices. If the link is in
a non-U0 state, then the device must transition the link to U0 prior to 
sending the remote wake message. If a remote wake event occurs in 
multiple functions, each function shall send a Function Wake Notification"

Best Regards,
Elson

> 
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>    drivers/usb/gadget/composite.c | 32 ++++++++++++++++++++++++++++++++
>>    drivers/usb/gadget/udc/core.c  |  9 +++++++++
>>    include/linux/usb/composite.h  |  1 +
>>    include/linux/usb/gadget.h     |  2 ++
>>    4 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index 403563c..6bdce23 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -490,6 +490,38 @@ 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;
>> +	unsigned long flags;
>> +
>> +	if (!func || !func->config || !func->config->cdev ||
>> +	    !func->config->cdev->gadget)
>> +		return -EINVAL;
>> +
>> +	DBG(func->config->cdev, "%s function wakeup\n", func->name);
>> +
>> +	spin_lock_irqsave(&func->config->cdev->lock, flags);
>> +
>> +	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);
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
>> +	ret = usb_gadget_func_wakeup(func->config->cdev->gadget, id);
>> +
>> +err:
>> +	spin_unlock_irqrestore(&func->config->cdev->lock, flags);
>> +
>> +	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 7886497..fe5c504 100644
>> --- a/drivers/usb/gadget/udc/core.c
>> +++ b/drivers/usb/gadget/udc/core.c
>> @@ -816,6 +816,15 @@ int usb_gadget_activate(struct usb_gadget *gadget)
>>    }
>>    EXPORT_SYMBOL_GPL(usb_gadget_activate);
>>    
>> +int usb_gadget_func_wakeup(struct usb_gadget *gadget, int interface_id)
>> +{
>> +	if (gadget->speed < USB_SPEED_SUPER || !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 9d27622..31b35d7 100644
>> --- a/include/linux/usb/composite.h
>> +++ b/include/linux/usb/composite.h
>> @@ -254,6 +254,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 3ad58b7..76f9de4 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -311,6 +311,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; }
> 

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Thinh Nguyen]

On 8/4/2022, Elson Serrao wrote:
>
>
> On 8/2/2022 4:51 PM, Thinh Nguyen wrote:
>> On 8/2/2022, Elson Roy Serrao wrote:
>>> An interface which is in function suspend state has to send a function
>>> wakeup notification to the host in case it needs to initate any data
>>> transfer. One notable difference between this and the existing remote
>>> wakeup mechanism is that this can be called per-interface, and a UDC
>>> would need to know the particular interface number to convey in its
>>> Device Notification transaction packet.  Hence, we need to introduce
>>> a new callback in the gadget_ops structure that UDC device drivers
>>> can implement.  Similarly add a convenience function in the composite
>>> driver which function drivers can call. Add support to handle such
>>> requests in the composite layer and invoke the gadget op.
>>
>> Sending the function wake notification should be done in the controller
>> driver. The controller driver knows when is the proper link state
>> (U0/ON) the device is in and would notify the host then.
>>
>> What we need to add in the usb_gadget is whether the device is remote
>> wakeup capable. Something like a flag usb_gadget->rw_capable.
>>
>> We would also need some functions like usb_gadget_enable_remote_wakeup()
>> and usb_gadget_disable_remote_wakeup() for the gadget driver to notify
>> the controller driver when it checks against USB_CONFIG_ATT_WAKEUP in
>> the bmAttributes configuration.
>>
>> BR,
>> Thinh
>
>
> If we handle this in controller driver, then it would fail to get the 
> right interface id when multiple functions have to send function wake 
> notification. As per USB3.0 spec (below snippets) a function can be 
> independently placed into function suspend state within a composite 
> device and each function in function suspend state has to send a 
> function wake notification to exit.
>
> USB 3.0 Spec Section 9.2.5.3
> "A function may be placed into Function Suspend independently of other 
> functions within a composite device"
>
> USB 3.0 Spec Section 9.2.5.4
> "A function may signal that it wants to exit from Function Suspend by 
> sending a Function Wake Notification to the host if it is enabled for 
> function remote wakeup. This applies to single function devices as 
> well as multiple function ( i.e., composite) devices. If the link is in
> a non-U0 state, then the device must transition the link to U0 prior 
> to sending the remote wake message. If a remote wake event occurs in 
> multiple functions, each function shall send a Function Wake 
> Notification"
>

Ok, so the issue here is adding the ability to pass the interface number 
to the controller driver when sending the device notification function 
wakeup right? Sounds like the callback should be 
send_wakeup_notification(gadget, func_id) instead.

As for remote wakeup, the spec states that "If remote wake event occurs 
in multiple functions, each function shall send a Function Wake 
Notification."

The SET_FEATURE(FUNCTION_SUSPEND) does not necessarily mean the host 
will put the device in Suspend State for a remote wake event to occur. 
It only places the function in Function Suspend. However often the host 
will put the device in suspend after this. The dwc3 driver can track if 
the host puts the device in suspend state and what interfaces are armed 
for remote wakeup. If a remote wakeup event occurs, the dwc3 driver can 
send Function Wake Notification for each function armed with remote wakeup.

Please correct me if I'm wrong.

Also, make sure that device remote wakeup will still work for highspeed 
(not function remote wakeup). I see this check which doesn't look right 
in one of your patches:
+    if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled)
+        dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
+        ret =  -EPERM;
+        goto out;
+    }

Thanks,
Thinh

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Elson Serrao]

On 8/4/2022 6:26 PM, Thinh Nguyen wrote:
> On 8/4/2022, Elson Serrao wrote:
>>
>>
>> On 8/2/2022 4:51 PM, Thinh Nguyen wrote:
>>> On 8/2/2022, Elson Roy Serrao wrote:
>>>> An interface which is in function suspend state has to send a function
>>>> wakeup notification to the host in case it needs to initate any data
>>>> transfer. One notable difference between this and the existing remote
>>>> wakeup mechanism is that this can be called per-interface, and a UDC
>>>> would need to know the particular interface number to convey in its
>>>> Device Notification transaction packet.  Hence, we need to introduce
>>>> a new callback in the gadget_ops structure that UDC device drivers
>>>> can implement.  Similarly add a convenience function in the composite
>>>> driver which function drivers can call. Add support to handle such
>>>> requests in the composite layer and invoke the gadget op.
>>>
>>> Sending the function wake notification should be done in the controller
>>> driver. The controller driver knows when is the proper link state
>>> (U0/ON) the device is in and would notify the host then.
>>>
>>> What we need to add in the usb_gadget is whether the device is remote
>>> wakeup capable. Something like a flag usb_gadget->rw_capable.
>>>
>>> We would also need some functions like usb_gadget_enable_remote_wakeup()
>>> and usb_gadget_disable_remote_wakeup() for the gadget driver to notify
>>> the controller driver when it checks against USB_CONFIG_ATT_WAKEUP in
>>> the bmAttributes configuration.
>>>
>>> BR,
>>> Thinh
>>
>>
>> If we handle this in controller driver, then it would fail to get the
>> right interface id when multiple functions have to send function wake
>> notification. As per USB3.0 spec (below snippets) a function can be
>> independently placed into function suspend state within a composite
>> device and each function in function suspend state has to send a
>> function wake notification to exit.
>>
>> USB 3.0 Spec Section 9.2.5.3
>> "A function may be placed into Function Suspend independently of other
>> functions within a composite device"
>>
>> USB 3.0 Spec Section 9.2.5.4
>> "A function may signal that it wants to exit from Function Suspend by
>> sending a Function Wake Notification to the host if it is enabled for
>> function remote wakeup. This applies to single function devices as
>> well as multiple function ( i.e., composite) devices. If the link is in
>> a non-U0 state, then the device must transition the link to U0 prior
>> to sending the remote wake message. If a remote wake event occurs in
>> multiple functions, each function shall send a Function Wake
>> Notification"
>>
> 
> Ok, so the issue here is adding the ability to pass the interface number
> to the controller driver when sending the device notification function
> wakeup right? Sounds like the callback should be
> send_wakeup_notification(gadget, func_id) instead.
> 
> As for remote wakeup, the spec states that "If remote wake event occurs
> in multiple functions, each function shall send a Function Wake
> Notification."
> 
> The SET_FEATURE(FUNCTION_SUSPEND) does not necessarily mean the host
> will put the device in Suspend State for a remote wake event to occur.
> It only places the function in Function Suspend. However often the host
> will put the device in suspend after this. The dwc3 driver can track if
> the host puts the device in suspend state and what interfaces are armed
> for remote wakeup. If a remote wakeup event occurs, the dwc3 driver can
> send Function Wake Notification for each function armed with remote wakeup.
> 
> Please correct me if I'm wrong.
> 
> Also, make sure that device remote wakeup will still work for highspeed
> (not function remote wakeup). I see this check which doesn't look right
> in one of your patches:
> +    if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled)
> +        dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
> +        ret =  -EPERM;
> +        goto out;
> +    }
> 
> Thanks,
> Thinh
> 

For superspeed capable devices, when a function is in suspend state and 
wants to
initiate a resume, it has to send a function wake notification to the 
host irrespective
of whether the device is in SUSPEND or not. Like you mentioned the 
device need not be in
suspend state when a function is suspended. If the device is in suspend, 
then first the
controller driver has to transition the link to U0 state before sending 
function wake notification.
Note that the DEVICE_REMOTE_WAKEUP feature is ignored for super-speed 
devices and they
are by default remote wakeup capable if any function within the device 
is armed for
function remote wakeup.

So in my current implementation when the host sends a function suspend 
SET_FEATURE(FUNCTION_SUSPEND),
the device delegates it to the respective function driver. There we 
inspect if it is capable
of initiating a function remote wakeup. If it is, then when a remote 
wakeup event
occurs (in my current implementation when TCP/IP layer wants to send 
data to the host. patch#5) then
we trigger a function wakeup by calling usb_gadget_func_wakeup(gadget, 
id) callback. Controller driver then
checks if the device is in suspend or not. If it is in suspend, it first 
brings the device to U0 state
and then sends a function wake notification (via 
dwc3_send_gadget_generic_command() API) only after an
U0 event has occurred. If the device is not in suspend then it directly 
sends function wake notification
to the host. Once the host receives the function wake notification it 
sends a SET_FEATURE(FUNCTION_SUSPEND)
with suspend bit (BIT 0) reset to signal function resume. The controller 
driver upon receiving this packet
delegates to the respective function driver. Note that at this point the 
device is in U0 state but some other
function within the device may still be in suspend state (if more than 
one function was put to suspend state).
So the only way to exit from function suspend is via function resume 
which is independent of device suspend/resume.

Also the task of finding the interface id is done by composite driver 
because most function drivers have
a transport layer and this layer is the one responsible for issuing a 
function remote wakeup and
this has no direct reference to interface id. For example u_ether 
transport layer can have either f_ecm or f_rndis
as its underlying channel and u_ether has no knowledge of the interface 
id/function driver it is using.

For high speed devices there is no concept of function suspend and there 
is only device suspend. The ability
of a device to send a remote wakeup to exit from suspend is dictated by 
DEVICE_REMOTE_WAKEUP feature selector.
The below snippet controls this aspect and sends remote wakeup for high 
speed devices only if they are remote wakeup capable.
dwc->is_remote_wakeup_enabled flag is set when DEVICE_REMOTE_WAKEUP is 
received.

+    if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled)
+        dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
+        ret =  -EPERM;
+        goto out;
+    }

Please let me know your thoughts on this approach. I will address your 
other comments and rectify the patches accordingly.

Thanks & Regards
Elson

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Thinh Nguyen]

On 8/9/2022, Elson Serrao wrote:
> 
> 
> On 8/4/2022 6:26 PM, Thinh Nguyen wrote:
>> On 8/4/2022, Elson Serrao wrote:
>>>
>>>
>>> On 8/2/2022 4:51 PM, Thinh Nguyen wrote:
>>>> On 8/2/2022, Elson Roy Serrao wrote:
>>>>> An interface which is in function suspend state has to send a function
>>>>> wakeup notification to the host in case it needs to initate any data
>>>>> transfer. One notable difference between this and the existing remote
>>>>> wakeup mechanism is that this can be called per-interface, and a UDC
>>>>> would need to know the particular interface number to convey in its
>>>>> Device Notification transaction packet.  Hence, we need to introduce
>>>>> a new callback in the gadget_ops structure that UDC device drivers
>>>>> can implement.  Similarly add a convenience function in the composite
>>>>> driver which function drivers can call. Add support to handle such
>>>>> requests in the composite layer and invoke the gadget op.
>>>>
>>>> Sending the function wake notification should be done in the controller
>>>> driver. The controller driver knows when is the proper link state
>>>> (U0/ON) the device is in and would notify the host then.
>>>>
>>>> What we need to add in the usb_gadget is whether the device is remote
>>>> wakeup capable. Something like a flag usb_gadget->rw_capable.
>>>>
>>>> We would also need some functions like
>>>> usb_gadget_enable_remote_wakeup()
>>>> and usb_gadget_disable_remote_wakeup() for the gadget driver to notify
>>>> the controller driver when it checks against USB_CONFIG_ATT_WAKEUP in
>>>> the bmAttributes configuration.
>>>>
>>>> BR,
>>>> Thinh
>>>
>>>
>>> If we handle this in controller driver, then it would fail to get the
>>> right interface id when multiple functions have to send function wake
>>> notification. As per USB3.0 spec (below snippets) a function can be
>>> independently placed into function suspend state within a composite
>>> device and each function in function suspend state has to send a
>>> function wake notification to exit.
>>>
>>> USB 3.0 Spec Section 9.2.5.3
>>> "A function may be placed into Function Suspend independently of other
>>> functions within a composite device"
>>>
>>> USB 3.0 Spec Section 9.2.5.4
>>> "A function may signal that it wants to exit from Function Suspend by
>>> sending a Function Wake Notification to the host if it is enabled for
>>> function remote wakeup. This applies to single function devices as
>>> well as multiple function ( i.e., composite) devices. If the link is in
>>> a non-U0 state, then the device must transition the link to U0 prior
>>> to sending the remote wake message. If a remote wake event occurs in
>>> multiple functions, each function shall send a Function Wake
>>> Notification"
>>>
>>
>> Ok, so the issue here is adding the ability to pass the interface number
>> to the controller driver when sending the device notification function
>> wakeup right? Sounds like the callback should be
>> send_wakeup_notification(gadget, func_id) instead.
>>
>> As for remote wakeup, the spec states that "If remote wake event occurs
>> in multiple functions, each function shall send a Function Wake
>> Notification."
>>
>> The SET_FEATURE(FUNCTION_SUSPEND) does not necessarily mean the host
>> will put the device in Suspend State for a remote wake event to occur.
>> It only places the function in Function Suspend. However often the host
>> will put the device in suspend after this. The dwc3 driver can track if
>> the host puts the device in suspend state and what interfaces are armed
>> for remote wakeup. If a remote wakeup event occurs, the dwc3 driver can
>> send Function Wake Notification for each function armed with remote
>> wakeup.
>>
>> Please correct me if I'm wrong.
>>
>> Also, make sure that device remote wakeup will still work for highspeed
>> (not function remote wakeup). I see this check which doesn't look right
>> in one of your patches:
>> +    if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled)
>> +        dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
>> +        ret =  -EPERM;
>> +        goto out;
>> +    }
>>
>> Thanks,
>> Thinh
>>
> 
> For superspeed capable devices, when a function is in suspend state and
> wants to
> initiate a resume, it has to send a function wake notification to the
> host irrespective
> of whether the device is in SUSPEND or not. Like you mentioned the
> device need not be in
> suspend state when a function is suspended. If the device is in suspend,
> then first the
> controller driver has to transition the link to U0 state before sending
> function wake notification.

Was I incorrect? I'm not clear on the point of reiteration above.

> Note that the DEVICE_REMOTE_WAKEUP feature is ignored for super-speed
> devices and they

We're still talking about Enhanced Super Speed here.

> are by default remote wakeup capable if any function within the device
> is armed for
> function remote wakeup.

What you're saying is if the host arms the function for remote wakeup,
then the device is remote capable.

However, the important point here is that the host only arms for remote
wakeup _if_ the device is remote wakeup capable. That needs to be checked.

> 
> So in my current implementation when the host sends a function suspend
> SET_FEATURE(FUNCTION_SUSPEND),
> the device delegates it to the respective function driver. There we
> inspect if it is capable
> of initiating a function remote wakeup. If it is, then when a remote
> wakeup event
> occurs (in my current implementation when TCP/IP layer wants to send
> data to the host. patch#5) then
> we trigger a function wakeup by calling usb_gadget_func_wakeup(gadget,
> id) callback. Controller driver then
> checks if the device is in suspend or not. If it is in suspend, it first
> brings the device to U0 state

"brings the device to U0 state" means the device initiates remote wakeup
here.

> and then sends a function wake notification (via
> dwc3_send_gadget_generic_command() API) only after an

So now the dwc3 tracks which interface(s) were armed for remote here?

I don't recall seeing it in your patches. Did you handle and send device
notification for all the functions armed with remote wakeup after device
wakeup?


> U0 event has occurred. If the device is not in suspend then it directly
> sends function wake notification
> to the host. Once the host receives the function wake notification it
> sends a SET_FEATURE(FUNCTION_SUSPEND)
> with suspend bit (BIT 0) reset to signal function resume. The controller
> driver upon receiving this packet
> delegates to the respective function driver. Note that at this point the
> device is in U0 state but some other

We can't assume that the device is in U0 state. There's also no
mechanism in your change to know that either.

> function within the device may still be in suspend state (if more than
> one function was put to suspend state).
> So the only way to exit from function suspend is via function resume
> which is independent of device suspend/resume.
> 
> Also the task of finding the interface id is done by composite driver
> because most function drivers have
> a transport layer and this layer is the one responsible for issuing a
> function remote wakeup and
> this has no direct reference to interface id. For example u_ether
> transport layer can have either f_ecm or f_rndis
> as its underlying channel and u_ether has no knowledge of the interface
> id/function driver it is using.
> 

> For high speed devices there is no concept of function suspend and there
> is only device suspend. The ability
> of a device to send a remote wakeup to exit from suspend is dictated by
> DEVICE_REMOTE_WAKEUP feature selector.
> The below snippet controls this aspect and sends remote wakeup for high
> speed devices only if they are remote wakeup capable.
> dwc->is_remote_wakeup_enabled flag is set when DEVICE_REMOTE_WAKEUP is
> received.
>

The flag "is_remote_wakeup_enabled" implies that it applies for both
device remote wakeup and function remote wakeup. If it only meant for
function remote wakeup, then rename it. But I think you can use the same
flag for both scenarios.


> +    if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled)
> +        dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
> +        ret =  -EPERM;

Also, don't use -EPERM. Use -EINVAL.

> +        goto out;
> +    }
> 
> Please let me know your thoughts on this approach. I will address your
> other comments and rectify the patches accordingly.
> 


To summarize the points:

1) The host only arms function remote wakeup if the device is capable of
remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and hardware
capability)

2) If the device is in suspend, the device can do remote wakeup (through
LFPS handshake) if the function is armed for remote wakeup (through
SET_FEATURE(FUNC_SUSPEND)).

3) If the link transitions to U0 after the device triggering a remote
wakeup, the device will also send device notification function wake for
all the interfaces armed with remote wakeup.

4) If the device is not in suspend, the device can send device
notification function wake if it's in U0.


Now, remote wakeup and function wake device notification are 2 separate
operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
suggested to maybe add usb_gadget_ops->send_wakeup_notification(gadget,
intf_id) for the device notification. What you did was combining both
operations in usb_gadget_ops->func_wakeup(). That may only work for
point 4) (assuming you fix the U0 check), but not point 3).

To be able to do 3), you can teach the composite layer _when_ to send
device notification function wake and for what functions. This can be
retry sending the notification until send_wakeup_notification() succeed?

I suggested to do that in dwc3 driver to avoid having to add the logic
in composite layer as I think it is simpler in dwc3. However, the
downside is that other UDCs have to handle it like dwc3 also.

Now that I think about it again, it maybe better to do it in the
composite driver for the long run. If you want to handle this in the
composite layer, please document and design the mechanism to handle all
the points above.

Thanks,
Thinh

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Elson Serrao]

On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
> On 8/9/2022, Elson Serrao wrote:
>>
>>
>> On 8/4/2022 6:26 PM, Thinh Nguyen wrote:
>>> On 8/4/2022, Elson Serrao wrote:
>>>>
>>>>
>>>> On 8/2/2022 4:51 PM, Thinh Nguyen wrote:
>>>>> On 8/2/2022, Elson Roy Serrao wrote:
>>>>>> An interface which is in function suspend state has to send a function
>>>>>> wakeup notification to the host in case it needs to initate any data
>>>>>> transfer. One notable difference between this and the existing remote
>>>>>> wakeup mechanism is that this can be called per-interface, and a UDC
>>>>>> would need to know the particular interface number to convey in its
>>>>>> Device Notification transaction packet.  Hence, we need to introduce
>>>>>> a new callback in the gadget_ops structure that UDC device drivers
>>>>>> can implement.  Similarly add a convenience function in the composite
>>>>>> driver which function drivers can call. Add support to handle such
>>>>>> requests in the composite layer and invoke the gadget op.
>>>>>
>>>>> Sending the function wake notification should be done in the controller
>>>>> driver. The controller driver knows when is the proper link state
>>>>> (U0/ON) the device is in and would notify the host then.
>>>>>
>>>>> What we need to add in the usb_gadget is whether the device is remote
>>>>> wakeup capable. Something like a flag usb_gadget->rw_capable.
>>>>>
>>>>> We would also need some functions like
>>>>> usb_gadget_enable_remote_wakeup()
>>>>> and usb_gadget_disable_remote_wakeup() for the gadget driver to notify
>>>>> the controller driver when it checks against USB_CONFIG_ATT_WAKEUP in
>>>>> the bmAttributes configuration.
>>>>>
>>>>> BR,
>>>>> Thinh
>>>>
>>>>
>>>> If we handle this in controller driver, then it would fail to get the
>>>> right interface id when multiple functions have to send function wake
>>>> notification. As per USB3.0 spec (below snippets) a function can be
>>>> independently placed into function suspend state within a composite
>>>> device and each function in function suspend state has to send a
>>>> function wake notification to exit.
>>>>
>>>> USB 3.0 Spec Section 9.2.5.3
>>>> "A function may be placed into Function Suspend independently of other
>>>> functions within a composite device"
>>>>
>>>> USB 3.0 Spec Section 9.2.5.4
>>>> "A function may signal that it wants to exit from Function Suspend by
>>>> sending a Function Wake Notification to the host if it is enabled for
>>>> function remote wakeup. This applies to single function devices as
>>>> well as multiple function ( i.e., composite) devices. If the link is in
>>>> a non-U0 state, then the device must transition the link to U0 prior
>>>> to sending the remote wake message. If a remote wake event occurs in
>>>> multiple functions, each function shall send a Function Wake
>>>> Notification"
>>>>
>>>
>>> Ok, so the issue here is adding the ability to pass the interface number
>>> to the controller driver when sending the device notification function
>>> wakeup right? Sounds like the callback should be
>>> send_wakeup_notification(gadget, func_id) instead.
>>>
>>> As for remote wakeup, the spec states that "If remote wake event occurs
>>> in multiple functions, each function shall send a Function Wake
>>> Notification."
>>>
>>> The SET_FEATURE(FUNCTION_SUSPEND) does not necessarily mean the host
>>> will put the device in Suspend State for a remote wake event to occur.
>>> It only places the function in Function Suspend. However often the host
>>> will put the device in suspend after this. The dwc3 driver can track if
>>> the host puts the device in suspend state and what interfaces are armed
>>> for remote wakeup. If a remote wakeup event occurs, the dwc3 driver can
>>> send Function Wake Notification for each function armed with remote
>>> wakeup.
>>>
>>> Please correct me if I'm wrong.
>>>
>>> Also, make sure that device remote wakeup will still work for highspeed
>>> (not function remote wakeup). I see this check which doesn't look right
>>> in one of your patches:
>>> +    if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled)
>>> +        dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
>>> +        ret =  -EPERM;
>>> +        goto out;
>>> +    }
>>>
>>> Thanks,
>>> Thinh
>>>
>>
>> For superspeed capable devices, when a function is in suspend state and
>> wants to
>> initiate a resume, it has to send a function wake notification to the
>> host irrespective
>> of whether the device is in SUSPEND or not. Like you mentioned the
>> device need not be in
>> suspend state when a function is suspended. If the device is in suspend,
>> then first the
>> controller driver has to transition the link to U0 state before sending
>> function wake notification.
> 
> Was I incorrect? I'm not clear on the point of reiteration above.
> 
>> Note that the DEVICE_REMOTE_WAKEUP feature is ignored for super-speed
>> devices and they
> 
> We're still talking about Enhanced Super Speed here.
> 
>> are by default remote wakeup capable if any function within the device
>> is armed for
>> function remote wakeup.
> 
> What you're saying is if the host arms the function for remote wakeup,
> then the device is remote capable.
> 
> However, the important point here is that the host only arms for remote
> wakeup _if_ the device is remote wakeup capable. That needs to be checked.
> 
Yes. The device would have advertised its remote wakeup capability 
during enumeration stage itself based on which host sets 
SET_FEATURE(FUNCTION_SUSPEND) RW option.


>>
>> So in my current implementation when the host sends a function suspend
>> SET_FEATURE(FUNCTION_SUSPEND),
>> the device delegates it to the respective function driver. There we
>> inspect if it is capable
>> of initiating a function remote wakeup. If it is, then when a remote
>> wakeup event
>> occurs (in my current implementation when TCP/IP layer wants to send
>> data to the host. patch#5) then
>> we trigger a function wakeup by calling usb_gadget_func_wakeup(gadget,
>> id) callback. Controller driver then
>> checks if the device is in suspend or not. If it is in suspend, it first
>> brings the device to U0 state
> 
> "brings the device to U0 state" means the device initiates remote wakeup
> here.
> 
>> and then sends a function wake notification (via
>> dwc3_send_gadget_generic_command() API) only after an
> 
> So now the dwc3 tracks which interface(s) were armed for remote here?
> 
> I don't recall seeing it in your patches. Did you handle and send device
> notification for all the functions armed with remote wakeup after device
> wakeup?
> 

That task is delegated to function driver. In patch#5 the 
function/composite layer sends wakeup notification only if it is armed 
for func remote wakeup.
func_wakeup_allowed flag serves this purpose for f_ecm function driver. 
This flag is set based on SET_FEATURE(FUNCTION_SUSPEND) packet (which in 
turn depends on the remote wakeup capability advertised by the device in 
bmAttributes like mentioned above).
> 
>> U0 event has occurred. If the device is not in suspend then it directly
>> sends function wake notification
>> to the host. Once the host receives the function wake notification it
>> sends a SET_FEATURE(FUNCTION_SUSPEND)
>> with suspend bit (BIT 0) reset to signal function resume. The controller
>> driver upon receiving this packet
>> delegates to the respective function driver. Note that at this point the
>> device is in U0 state but some other
> 
> We can't assume that the device is in U0 state. There's also no
> mechanism in your change to know that either.
> 
>> function within the device may still be in suspend state (if more than
>> one function was put to suspend state).
>> So the only way to exit from function suspend is via function resume
>> which is independent of device suspend/resume.
>>
>> Also the task of finding the interface id is done by composite driver
>> because most function drivers have
>> a transport layer and this layer is the one responsible for issuing a
>> function remote wakeup and
>> this has no direct reference to interface id. For example u_ether
>> transport layer can have either f_ecm or f_rndis
>> as its underlying channel and u_ether has no knowledge of the interface
>> id/function driver it is using.
>>
> 
>> For high speed devices there is no concept of function suspend and there
>> is only device suspend. The ability
>> of a device to send a remote wakeup to exit from suspend is dictated by
>> DEVICE_REMOTE_WAKEUP feature selector.
>> The below snippet controls this aspect and sends remote wakeup for high
>> speed devices only if they are remote wakeup capable.
>> dwc->is_remote_wakeup_enabled flag is set when DEVICE_REMOTE_WAKEUP is
>> received.
>>
> 
> The flag "is_remote_wakeup_enabled" implies that it applies for both
> device remote wakeup and function remote wakeup. If it only meant for
> function remote wakeup, then rename it. But I think you can use the same
> flag for both scenarios.

The flag is_remote_wakeup_enabled is only meant for device remote wakeup 
and is only used in High Speed. In my implementation function remote 
wakeup flag is at function level which is set in the function driver 
based on whether the function is armed for remote wakeup. The 
function/composite layer would send wakeup notification only if it is 
armed for remote wakeup.

Below snippet from patch#5

Set the function_wakeup_allowed flag based on USB_INTRF_FUNC_SUSPEND_RW 
option

+static int ecm_func_suspend(struct usb_function *f, u8 options)
+{
+	bool func_wakeup_allowed;
+	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);
+
+	func_wakeup_allowed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8));

Send wakeup notification only if func_wakeup_allowed flag is set.

+		if (!port->func_wakeup_allowed) {
+			DBG(port->ioport, "Function wakeup not allowed\n");
+			return -EOPNOTSUPP;
+		}
+		ret = usb_func_wakeup(func);

> 
> 
>> +    if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled)
>> +        dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
>> +        ret =  -EPERM;
> 
> Also, don't use -EPERM. Use -EINVAL.

Done.

> 
>> +        goto out;
>> +    }
>>
>> Please let me know your thoughts on this approach. I will address your
>> other comments and rectify the patches accordingly.
>>
> 
> 
> To summarize the points:
> 
> 1) The host only arms function remote wakeup if the device is capable of
> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and hardware
> capability)
> 
> 2) If the device is in suspend, the device can do remote wakeup (through
> LFPS handshake) if the function is armed for remote wakeup (through
> SET_FEATURE(FUNC_SUSPEND)).
> 
> 3) If the link transitions to U0 after the device triggering a remote
> wakeup, the device will also send device notification function wake for
> all the interfaces armed with remote wakeup.
> 
> 4) If the device is not in suspend, the device can send device
> notification function wake if it's in U0.
> 
> 
> Now, remote wakeup and function wake device notification are 2 separate
> operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
> suggested to maybe add usb_gadget_ops->send_wakeup_notification(gadget,
> intf_id) for the device notification. What you did was combining both
> operations in usb_gadget_ops->func_wakeup(). That may only work for
> point 4) (assuming you fix the U0 check), but not point 3).

Thank you for your feedback and summary. I will rename func_wakeup to
send_wakeup_notification to better align with the approach. The reason I 
have combined remote_wakeup and function wake notification in 
usb_gadget_ops->func_wakeup() is because since the implementation is at 
function/composite level it has no knowledge on the link state. So I 
have delegated that task to controller driver to handle the notification 
accordingly. That is do a LFPS handshake first if the device is 
suspended and then send notification (explained below). But we can 
definitely separate this by adding an additional flag in the composite 
layer to set the link state based on the gadget suspend callback called 
when U3 SUSPEND interrupt is received. Let me know if you feel 
separating the two is a better approach.

I have explained below, how the 4 points you mentioned are handled in my 
current implementation.

The function driver will send a wakeup notification only if it is armed 
for remote wakeup.

patch#5
+		if (!port->func_wakeup_allowed) {
+			DBG(port->ioport, "Function wakeup not allowed\n");
+			return -EOPNOTSUPP;
+		}
+		ret = usb_func_wakeup(func);
+		if (ret)
+			port->is_wakeup_pending = true;

If the device is in suspend, we do a LFPS handshake first and return 
-EAGAIN to composite layer which will set the is_wakeup_pending flag.

Patch#3
+static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int interface_id)
+{
+	int	ret = 0;
+	u32	reg;
+	struct	dwc3 *dwc = gadget_to_dwc(g);
+
+	reg = dwc3_readl(dwc->regs, DWC3_DSTS);
+
+	/*
+	 * If the link is in LPM, first bring the link to U0
+	 * before triggering function wakeup. Ideally this
+	 * needs to be expanded to other LPMs as well in
+	 * addition to U3
+	 */
+	if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U3) {
+		dwc3_gadget_wakeup(g);
+		return -EAGAIN;
+	}

The above should take care of Point 2.

After triggering a remote wakeup in Point 2, if the link transitions to 
U0 then we will receive a U0 link state event for the same and that 
would trigger a gadget_resume callback to inform the composite layer 
that device has resumed. As soon as the function/composite layer gets 
this info it will re-send the wakeup notification to the controller 
driver based on the is_wakeup_pending flag

linksts_change_interrupt() in Patch#1
+	case DWC3_LINK_STATE_U0:
+		if (dwc->is_gadget_wakeup) {
+			linksts_change_events_set(dwc, false);
+			dwc3_resume_gadget(dwc);
+			dwc->is_gadget_wakeup = false;
+		}
+		break;


u_ether resume callback in Patch#5
+	if (func_suspend) {
+		if (link->is_wakeup_pending) {
+			usb_func_wakeup(func);
+			link->is_wakeup_pending = false;
+		}

The above should take care of Point 3.

For Point 4 like you mentioned I will add U0 check instead of U3 check.

Point 1 would have resolved in enumeration stage itself (bmAttributes in 
config descriptor) based on which the host sets the 
USB_INTRF_FUNC_SUSPEND_RW option in the SET_FEATURE(FUNCTION_SUSPEND) 
packet. Based on this option the function/composite driver will set 
func_wakeup_allowed flag arming it for remote_wakeup

+static int ecm_func_suspend(struct usb_function *f, u8 options)
+{
+	bool func_wakeup_allowed;
+	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);
+
+	func_wakeup_allowed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8));

Do we need any additional checks for Point 1 ? Please let me know if my 
understanding is incorrect here.




> 
> To be able to do 3), you can teach the composite layer _when_ to send
> device notification function wake and for what functions. This can be
> retry sending the notification until send_wakeup_notification() succeed?
> 
> I suggested to do that in dwc3 driver to avoid having to add the logic
> in composite layer as I think it is simpler in dwc3. However, the
> downside is that other UDCs have to handle it like dwc3 also.
> 
> Now that I think about it again, it maybe better to do it in the
> composite driver for the long run. If you want to handle this in the
> composite layer, please document and design the mechanism to handle all
> the points above. >
> Thanks,
> Thinh
> 

Please let me know if this implementation fails to cover the 4 points 
you mentioned or any other rectification needed to handle these points.

Thanks & Regards
Elson

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Elson Serrao]

On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
> On 8/9/2022, Elson Serrao wrote:
>>
>>
>> On 8/4/2022 6:26 PM, Thinh Nguyen wrote:
>>> On 8/4/2022, Elson Serrao wrote:
>>>>
>>>>
>>>> On 8/2/2022 4:51 PM, Thinh Nguyen wrote:
>>>>> On 8/2/2022, Elson Roy Serrao wrote:
>>>>>> An interface which is in function suspend state has to send a function
>>>>>> wakeup notification to the host in case it needs to initate any data
>>>>>> transfer. One notable difference between this and the existing remote
>>>>>> wakeup mechanism is that this can be called per-interface, and a UDC
>>>>>> would need to know the particular interface number to convey in its
>>>>>> Device Notification transaction packet.  Hence, we need to introduce
>>>>>> a new callback in the gadget_ops structure that UDC device drivers
>>>>>> can implement.  Similarly add a convenience function in the composite
>>>>>> driver which function drivers can call. Add support to handle such
>>>>>> requests in the composite layer and invoke the gadget op.
>>>>>
>>>>> Sending the function wake notification should be done in the controller
>>>>> driver. The controller driver knows when is the proper link state
>>>>> (U0/ON) the device is in and would notify the host then.
>>>>>
>>>>> What we need to add in the usb_gadget is whether the device is remote
>>>>> wakeup capable. Something like a flag usb_gadget->rw_capable.
>>>>>
>>>>> We would also need some functions like
>>>>> usb_gadget_enable_remote_wakeup()
>>>>> and usb_gadget_disable_remote_wakeup() for the gadget driver to notify
>>>>> the controller driver when it checks against USB_CONFIG_ATT_WAKEUP in
>>>>> the bmAttributes configuration.
>>>>>
>>>>> BR,
>>>>> Thinh
>>>>
>>>>
>>>> If we handle this in controller driver, then it would fail to get the
>>>> right interface id when multiple functions have to send function wake
>>>> notification. As per USB3.0 spec (below snippets) a function can be
>>>> independently placed into function suspend state within a composite
>>>> device and each function in function suspend state has to send a
>>>> function wake notification to exit.
>>>>
>>>> USB 3.0 Spec Section 9.2.5.3
>>>> "A function may be placed into Function Suspend independently of other
>>>> functions within a composite device"
>>>>
>>>> USB 3.0 Spec Section 9.2.5.4
>>>> "A function may signal that it wants to exit from Function Suspend by
>>>> sending a Function Wake Notification to the host if it is enabled for
>>>> function remote wakeup. This applies to single function devices as
>>>> well as multiple function ( i.e., composite) devices. If the link is in
>>>> a non-U0 state, then the device must transition the link to U0 prior
>>>> to sending the remote wake message. If a remote wake event occurs in
>>>> multiple functions, each function shall send a Function Wake
>>>> Notification"
>>>>
>>>
>>> Ok, so the issue here is adding the ability to pass the interface number
>>> to the controller driver when sending the device notification function
>>> wakeup right? Sounds like the callback should be
>>> send_wakeup_notification(gadget, func_id) instead.
>>>
>>> As for remote wakeup, the spec states that "If remote wake event occurs
>>> in multiple functions, each function shall send a Function Wake
>>> Notification."
>>>
>>> The SET_FEATURE(FUNCTION_SUSPEND) does not necessarily mean the host
>>> will put the device in Suspend State for a remote wake event to occur.
>>> It only places the function in Function Suspend. However often the host
>>> will put the device in suspend after this. The dwc3 driver can track if
>>> the host puts the device in suspend state and what interfaces are armed
>>> for remote wakeup. If a remote wakeup event occurs, the dwc3 driver can
>>> send Function Wake Notification for each function armed with remote
>>> wakeup.
>>>
>>> Please correct me if I'm wrong.
>>>
>>> Also, make sure that device remote wakeup will still work for highspeed
>>> (not function remote wakeup). I see this check which doesn't look right
>>> in one of your patches:
>>> +    if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled)
>>> +        dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
>>> +        ret =  -EPERM;
>>> +        goto out;
>>> +    }
>>>
>>> Thanks,
>>> Thinh
>>>
>>
>> For superspeed capable devices, when a function is in suspend state and
>> wants to
>> initiate a resume, it has to send a function wake notification to the
>> host irrespective
>> of whether the device is in SUSPEND or not. Like you mentioned the
>> device need not be in
>> suspend state when a function is suspended. If the device is in suspend,
>> then first the
>> controller driver has to transition the link to U0 state before sending
>> function wake notification.
> 
> Was I incorrect? I'm not clear on the point of reiteration above.
> 
>> Note that the DEVICE_REMOTE_WAKEUP feature is ignored for super-speed
>> devices and they
> 
> We're still talking about Enhanced Super Speed here.
> 
>> are by default remote wakeup capable if any function within the device
>> is armed for
>> function remote wakeup.
> 
> What you're saying is if the host arms the function for remote wakeup,
> then the device is remote capable.
> 
> However, the important point here is that the host only arms for remote
> wakeup _if_ the device is remote wakeup capable. That needs to be checked.
> 
>>
>> So in my current implementation when the host sends a function suspend
>> SET_FEATURE(FUNCTION_SUSPEND),
>> the device delegates it to the respective function driver. There we
>> inspect if it is capable
>> of initiating a function remote wakeup. If it is, then when a remote
>> wakeup event
>> occurs (in my current implementation when TCP/IP layer wants to send
>> data to the host. patch#5) then
>> we trigger a function wakeup by calling usb_gadget_func_wakeup(gadget,
>> id) callback. Controller driver then
>> checks if the device is in suspend or not. If it is in suspend, it first
>> brings the device to U0 state
> 
> "brings the device to U0 state" means the device initiates remote wakeup
> here.
> 
>> and then sends a function wake notification (via
>> dwc3_send_gadget_generic_command() API) only after an
> 
> So now the dwc3 tracks which interface(s) were armed for remote here?
> 
> I don't recall seeing it in your patches. Did you handle and send device
> notification for all the functions armed with remote wakeup after device
> wakeup?
> 
> 
>> U0 event has occurred. If the device is not in suspend then it directly
>> sends function wake notification
>> to the host. Once the host receives the function wake notification it
>> sends a SET_FEATURE(FUNCTION_SUSPEND)
>> with suspend bit (BIT 0) reset to signal function resume. The controller
>> driver upon receiving this packet
>> delegates to the respective function driver. Note that at this point the
>> device is in U0 state but some other
> 
> We can't assume that the device is in U0 state. There's also no
> mechanism in your change to know that either.
> 
>> function within the device may still be in suspend state (if more than
>> one function was put to suspend state).
>> So the only way to exit from function suspend is via function resume
>> which is independent of device suspend/resume.
>>
>> Also the task of finding the interface id is done by composite driver
>> because most function drivers have
>> a transport layer and this layer is the one responsible for issuing a
>> function remote wakeup and
>> this has no direct reference to interface id. For example u_ether
>> transport layer can have either f_ecm or f_rndis
>> as its underlying channel and u_ether has no knowledge of the interface
>> id/function driver it is using.
>>
> 
>> For high speed devices there is no concept of function suspend and there
>> is only device suspend. The ability
>> of a device to send a remote wakeup to exit from suspend is dictated by
>> DEVICE_REMOTE_WAKEUP feature selector.
>> The below snippet controls this aspect and sends remote wakeup for high
>> speed devices only if they are remote wakeup capable.
>> dwc->is_remote_wakeup_enabled flag is set when DEVICE_REMOTE_WAKEUP is
>> received.
>>
> 
> The flag "is_remote_wakeup_enabled" implies that it applies for both
> device remote wakeup and function remote wakeup. If it only meant for
> function remote wakeup, then rename it. But I think you can use the same
> flag for both scenarios.
> 
> 
>> +    if (g->speed < USB_SPEED_SUPER && !dwc->is_remote_wakeup_enabled)
>> +        dev_err(dwc->dev, "%s:remote wakeup not supported\n", __func__);
>> +        ret =  -EPERM;
> 
> Also, don't use -EPERM. Use -EINVAL.
> 
>> +        goto out;
>> +    }
>>
>> Please let me know your thoughts on this approach. I will address your
>> other comments and rectify the patches accordingly.
>>
> 
> 
> To summarize the points:
> 
> 1) The host only arms function remote wakeup if the device is capable of
> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and hardware
> capability)
> 
> 2) If the device is in suspend, the device can do remote wakeup (through
> LFPS handshake) if the function is armed for remote wakeup (through
> SET_FEATURE(FUNC_SUSPEND)).
> 
> 3) If the link transitions to U0 after the device triggering a remote
> wakeup, the device will also send device notification function wake for
> all the interfaces armed with remote wakeup.
> 

I am not clear on why device notification function wake should be sent 
for ALL interfaces armed with remote wakeup. Since function 
suspend/wakeup of an interface is independent of other functions in a 
composite device only the interface in which a remote wakeup event 
occurred should send the wake notification right? The other functions 
will continue to remain
in function suspend state.

> 4) If the device is not in suspend, the device can send device
> notification function wake if it's in U0.
> 
> 
> Now, remote wakeup and function wake device notification are 2 separate
> operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
> suggested to maybe add usb_gadget_ops->send_wakeup_notification(gadget,
> intf_id) for the device notification. What you did was combining both
> operations in usb_gadget_ops->func_wakeup(). That may only work for
> point 4) (assuming you fix the U0 check), but not point 3).
> 
> To be able to do 3), you can teach the composite layer _when_ to send
> device notification function wake and for what functions. This can be
> retry sending the notification until send_wakeup_notification() succeed?
> 
> I suggested to do that in dwc3 driver to avoid having to add the logic
> in composite layer as I think it is simpler in dwc3. However, the
> downside is that other UDCs have to handle it like dwc3 also.
> 
> Now that I think about it again, it maybe better to do it in the
> composite driver for the long run. If you want to handle this in the
> composite layer, please document and design the mechanism to handle all
> the points above.
> 
> Thanks,
> Thinh
> 

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Thinh Nguyen]

On 8/11/2022, Elson Serrao wrote:
> 
> 
> On 8/9/2022 6:08 PM, Thinh Nguyen wrote:

<snip>


>> To summarize the points:
>>
>> 1) The host only arms function remote wakeup if the device is capable of
>> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and hardware
>> capability)
>>
>> 2) If the device is in suspend, the device can do remote wakeup (through
>> LFPS handshake) if the function is armed for remote wakeup (through
>> SET_FEATURE(FUNC_SUSPEND)).
>>
>> 3) If the link transitions to U0 after the device triggering a remote
>> wakeup, the device will also send device notification function wake for
>> all the interfaces armed with remote wakeup.
>>
>> 4) If the device is not in suspend, the device can send device
>> notification function wake if it's in U0.
>>
>>
>> Now, remote wakeup and function wake device notification are 2 separate
>> operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
>> suggested to maybe add usb_gadget_ops->send_wakeup_notification(gadget,
>> intf_id) for the device notification. What you did was combining both
>> operations in usb_gadget_ops->func_wakeup(). That may only work for
>> point 4) (assuming you fix the U0 check), but not point 3).
> 
> Thank you for your feedback and summary. I will rename func_wakeup to
> send_wakeup_notification to better align with the approach. The reason I
> have combined remote_wakeup and function wake notification in
> usb_gadget_ops->func_wakeup() is because since the implementation is at
> function/composite level it has no knowledge on the link state. So I
> have delegated that task to controller driver to handle the notification
> accordingly. That is do a LFPS handshake first if the device is
> suspended and then send notification (explained below). But we can
> definitely separate this by adding an additional flag in the composite
> layer to set the link state based on the gadget suspend callback called
> when U3 SUSPEND interrupt is received. Let me know if you feel
> separating the two is a better approach.
> 

The reason I think we need to separate it is because of point 3. As I
note earlier, the spec states that "If remote wake event occurs in
multiple functions, each function shall send a Function Wake Notification."

But if there's no remote wake event, and the host brought the device up
instead, then the function suspend state is retained.

If we separate these 2 operations, the caller can check whether the
operation went through properly. For example, if the wakeup() is
initiated properly, but the function wake device notification didn't go
through. We would only need to resend the device notification rather
than initiate remote wakeup again.


> I have explained below, how the 4 points you mentioned are handled in my
> current implementation.
> 
> The function driver will send a wakeup notification only if it is armed
> for remote wakeup.
> 
> patch#5
> +        if (!port->func_wakeup_allowed) {
> +            DBG(port->ioport, "Function wakeup not allowed\n");
> +            return -EOPNOTSUPP;
> +        }
> +        ret = usb_func_wakeup(func);
> +        if (ret)
> +            port->is_wakeup_pending = true;
> 
> If the device is in suspend, we do a LFPS handshake first and return
> -EAGAIN to composite layer which will set the is_wakeup_pending flag.
> 

I don't see you're checking for -EAGAIN. Also what happens when wakeup()
fails, should we retry?

> Patch#3
> +static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int interface_id)
> +{
> +    int    ret = 0;
> +    u32    reg;
> +    struct    dwc3 *dwc = gadget_to_dwc(g);
> +
> +    reg = dwc3_readl(dwc->regs, DWC3_DSTS);
> +
> +    /*
> +     * If the link is in LPM, first bring the link to U0
> +     * before triggering function wakeup. Ideally this
> +     * needs to be expanded to other LPMs as well in
> +     * addition to U3
> +     */
> +    if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U3) {
> +        dwc3_gadget_wakeup(g);
> +        return -EAGAIN;
> +    }
> 
> The above should take care of Point 2.
> 
> After triggering a remote wakeup in Point 2, if the link transitions to
> U0 then we will receive a U0 link state event for the same and that
> would trigger a gadget_resume callback to inform the composite layer
> that device has resumed. As soon as the function/composite layer gets
> this info it will re-send the wakeup notification to the controller
> driver based on the is_wakeup_pending flag

See comment above.

> 
> linksts_change_interrupt() in Patch#1
> +    case DWC3_LINK_STATE_U0:
> +        if (dwc->is_gadget_wakeup) {
> +            linksts_change_events_set(dwc, false);
> +            dwc3_resume_gadget(dwc);
> +            dwc->is_gadget_wakeup = false;
> +        }
> +        break;
> 
> 
> u_ether resume callback in Patch#5
> +    if (func_suspend) {
> +        if (link->is_wakeup_pending) {
> +            usb_func_wakeup(func);
> +            link->is_wakeup_pending = false;
> +        }
> 
> The above should take care of Point 3.

We should also check for other functions to send device notification.

> 
> For Point 4 like you mentioned I will add U0 check instead of U3 check.
> 
> Point 1 would have resolved in enumeration stage itself (bmAttributes in
> config descriptor) based on which the host sets the
> USB_INTRF_FUNC_SUSPEND_RW option in the SET_FEATURE(FUNCTION_SUSPEND)
> packet. Based on this option the function/composite driver will set
> func_wakeup_allowed flag arming it for remote_wakeup
> 
> +static int ecm_func_suspend(struct usb_function *f, u8 options)
> +{
> +    bool func_wakeup_allowed;
> +    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);
> +
> +    func_wakeup_allowed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8));
> 
> Do we need any additional checks for Point 1 ? Please let me know if my
> understanding is incorrect here.
> 
> 

If the device is not remote wake capable, even if the host tries to arm
the device, the device shouldn't allow it. We're missing that check.

See 3.2 spec 9.1.1.6: "If a device is capable of remote wakeup, the
device shall support the ability of the host to enable and disable this
capability."

> 
> 
>>
>> To be able to do 3), you can teach the composite layer _when_ to send
>> device notification function wake and for what functions. This can be
>> retry sending the notification until send_wakeup_notification() succeed?
>>
>> I suggested to do that in dwc3 driver to avoid having to add the logic
>> in composite layer as I think it is simpler in dwc3. However, the
>> downside is that other UDCs have to handle it like dwc3 also.
>>
>> Now that I think about it again, it maybe better to do it in the
>> composite driver for the long run. If you want to handle this in the
>> composite layer, please document and design the mechanism to handle all
>> the points above. >
>> Thanks,
>> Thinh
>>
> 
> Please let me know if this implementation fails to cover the 4 points
> you mentioned or any other rectification needed to handle these points.
> 

BR,
Thinh

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Thinh Nguyen]

On 8/11/2022, Elson Serrao wrote:
> 
>>
>> 3) If the link transitions to U0 after the device triggering a remote
>> wakeup, the device will also send device notification function wake for
>> all the interfaces armed with remote wakeup.
>>
> 
> I am not clear on why device notification function wake should be sent
> for ALL interfaces armed with remote wakeup. Since function
> suspend/wakeup of an interface is independent of other functions in a
> composite device only the interface in which a remote wakeup event
> occurred should send the wake notification right? The other functions
> will continue to remain
> in function suspend state.
> 

hm... I think you're right here. I think I misread the spec.
We only need to send device notification of the function that triggers
remote wake.

You can ignore the comments related to this. Sorry for the confusion.
However, the other points still stand.

Thanks,
Thinh

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Thinh Nguyen]

On 8/11/2022, Thinh Nguyen wrote:
> On 8/11/2022, Elson Serrao wrote:
>>
>>
>> On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
> 
> <snip>
> 
> 
>>> To summarize the points:
>>>
>>> 1) The host only arms function remote wakeup if the device is capable of
>>> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and hardware
>>> capability)
>>>
>>> 2) If the device is in suspend, the device can do remote wakeup (through
>>> LFPS handshake) if the function is armed for remote wakeup (through
>>> SET_FEATURE(FUNC_SUSPEND)).
>>>
>>> 3) If the link transitions to U0 after the device triggering a remote
>>> wakeup, the device will also send device notification function wake for
>>> all the interfaces armed with remote wakeup.
>>>
>>> 4) If the device is not in suspend, the device can send device
>>> notification function wake if it's in U0.
>>>
>>>
>>> Now, remote wakeup and function wake device notification are 2 separate
>>> operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
>>> suggested to maybe add usb_gadget_ops->send_wakeup_notification(gadget,
>>> intf_id) for the device notification. What you did was combining both
>>> operations in usb_gadget_ops->func_wakeup(). That may only work for
>>> point 4) (assuming you fix the U0 check), but not point 3).
>>
>> Thank you for your feedback and summary. I will rename func_wakeup to
>> send_wakeup_notification to better align with the approach. The reason I
>> have combined remote_wakeup and function wake notification in
>> usb_gadget_ops->func_wakeup() is because since the implementation is at
>> function/composite level it has no knowledge on the link state. So I
>> have delegated that task to controller driver to handle the notification
>> accordingly. That is do a LFPS handshake first if the device is
>> suspended and then send notification (explained below). But we can
>> definitely separate this by adding an additional flag in the composite
>> layer to set the link state based on the gadget suspend callback called
>> when U3 SUSPEND interrupt is received. Let me know if you feel
>> separating the two is a better approach.
>>
> 
> The reason I think we need to separate it is because of point 3. As I
> note earlier, the spec states that "If remote wake event occurs in
> multiple functions, each function shall send a Function Wake Notification."
> 
> But if there's no remote wake event, and the host brought the device up
> instead, then the function suspend state is retained.
> 
> If we separate these 2 operations, the caller can check whether the
> operation went through properly. For example, if the wakeup() is
> initiated properly, but the function wake device notification didn't go
> through. We would only need to resend the device notification rather
> than initiate remote wakeup again.

If we don't have to send device notification for other interfaces, we
can combine the operations here as you did.

BR,
Thinh

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Thinh Nguyen]

On 8/11/2022, Thinh Nguyen wrote:
> On 8/11/2022, Thinh Nguyen wrote:
>> On 8/11/2022, Elson Serrao wrote:
>>>
>>>
>>> On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
>>
>> <snip>
>>
>>
>>>> To summarize the points:
>>>>
>>>> 1) The host only arms function remote wakeup if the device is capable of
>>>> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and hardware
>>>> capability)
>>>>
>>>> 2) If the device is in suspend, the device can do remote wakeup (through
>>>> LFPS handshake) if the function is armed for remote wakeup (through
>>>> SET_FEATURE(FUNC_SUSPEND)).
>>>>
>>>> 3) If the link transitions to U0 after the device triggering a remote
>>>> wakeup, the device will also send device notification function wake for
>>>> all the interfaces armed with remote wakeup.
>>>>
>>>> 4) If the device is not in suspend, the device can send device
>>>> notification function wake if it's in U0.
>>>>
>>>>
>>>> Now, remote wakeup and function wake device notification are 2 separate
>>>> operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
>>>> suggested to maybe add usb_gadget_ops->send_wakeup_notification(gadget,
>>>> intf_id) for the device notification. What you did was combining both
>>>> operations in usb_gadget_ops->func_wakeup(). That may only work for
>>>> point 4) (assuming you fix the U0 check), but not point 3).
>>>
>>> Thank you for your feedback and summary. I will rename func_wakeup to
>>> send_wakeup_notification to better align with the approach. The reason I
>>> have combined remote_wakeup and function wake notification in
>>> usb_gadget_ops->func_wakeup() is because since the implementation is at
>>> function/composite level it has no knowledge on the link state. So I
>>> have delegated that task to controller driver to handle the notification
>>> accordingly. That is do a LFPS handshake first if the device is
>>> suspended and then send notification (explained below). But we can
>>> definitely separate this by adding an additional flag in the composite
>>> layer to set the link state based on the gadget suspend callback called
>>> when U3 SUSPEND interrupt is received. Let me know if you feel
>>> separating the two is a better approach.
>>>
>>
>> The reason I think we need to separate it is because of point 3. As I
>> note earlier, the spec states that "If remote wake event occurs in
>> multiple functions, each function shall send a Function Wake Notification."
>>
>> But if there's no remote wake event, and the host brought the device up
>> instead, then the function suspend state is retained.
>>
>> If we separate these 2 operations, the caller can check whether the
>> operation went through properly. For example, if the wakeup() is
>> initiated properly, but the function wake device notification didn't go
>> through. We would only need to resend the device notification rather
>> than initiate remote wakeup again.
> 
> If we don't have to send device notification for other interfaces, we
> can combine the operations here as you did.
> 

I still think it's better to split up the operations. The way you're
handling it now is not clear.

If the func_awake() returns -EAGAIN, I'd expect that the remote wake did
not go through and expect user to retry again. But here it does initiate
remote wake, but it just does not send device notification yet. This is
confusing.

Also, instead of all the function wake handling coming from the function
driver, now we depend on the controller driver to call function resume()
on state change to U0, which will trigger device notification. What
happen if it doesn't call resume(). There's too many dependencies and it
seems fragile.

I think all this can be handled in the function driver. You can fix the
dwc3 wakeup() and poll for U0/ON state rather than RECOVERY state, which
is what it's supposed to poll. On usb_gadget_wakeup() returns
successful, we'd expect the device is linked up and woken up. then you
can send device notification through a different api such as
usb_gadget_send_wake_notification().

Thanks,
Thinh

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Elson Serrao]

On 8/12/2022 5:46 PM, Thinh Nguyen wrote:
> On 8/11/2022, Thinh Nguyen wrote:
>> On 8/11/2022, Thinh Nguyen wrote:
>>> On 8/11/2022, Elson Serrao wrote:
>>>>
>>>>
>>>> On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
>>>
>>> <snip>
>>>
>>>
>>>>> To summarize the points:
>>>>>
>>>>> 1) The host only arms function remote wakeup if the device is capable of
>>>>> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and hardware
>>>>> capability)
>>>>>
>>>>> 2) If the device is in suspend, the device can do remote wakeup (through
>>>>> LFPS handshake) if the function is armed for remote wakeup (through
>>>>> SET_FEATURE(FUNC_SUSPEND)).
>>>>>
>>>>> 3) If the link transitions to U0 after the device triggering a remote
>>>>> wakeup, the device will also send device notification function wake for
>>>>> all the interfaces armed with remote wakeup.
>>>>>
>>>>> 4) If the device is not in suspend, the device can send device
>>>>> notification function wake if it's in U0.
>>>>>
>>>>>
>>>>> Now, remote wakeup and function wake device notification are 2 separate
>>>>> operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
>>>>> suggested to maybe add usb_gadget_ops->send_wakeup_notification(gadget,
>>>>> intf_id) for the device notification. What you did was combining both
>>>>> operations in usb_gadget_ops->func_wakeup(). That may only work for
>>>>> point 4) (assuming you fix the U0 check), but not point 3).
>>>>
>>>> Thank you for your feedback and summary. I will rename func_wakeup to
>>>> send_wakeup_notification to better align with the approach. The reason I
>>>> have combined remote_wakeup and function wake notification in
>>>> usb_gadget_ops->func_wakeup() is because since the implementation is at
>>>> function/composite level it has no knowledge on the link state. So I
>>>> have delegated that task to controller driver to handle the notification
>>>> accordingly. That is do a LFPS handshake first if the device is
>>>> suspended and then send notification (explained below). But we can
>>>> definitely separate this by adding an additional flag in the composite
>>>> layer to set the link state based on the gadget suspend callback called
>>>> when U3 SUSPEND interrupt is received. Let me know if you feel
>>>> separating the two is a better approach.
>>>>
>>>
>>> The reason I think we need to separate it is because of point 3. As I
>>> note earlier, the spec states that "If remote wake event occurs in
>>> multiple functions, each function shall send a Function Wake Notification."
>>>
>>> But if there's no remote wake event, and the host brought the device up
>>> instead, then the function suspend state is retained.
>>>
>>> If we separate these 2 operations, the caller can check whether the
>>> operation went through properly. For example, if the wakeup() is
>>> initiated properly, but the function wake device notification didn't go
>>> through. We would only need to resend the device notification rather
>>> than initiate remote wakeup again.
>>
>> If we don't have to send device notification for other interfaces, we
>> can combine the operations here as you did.
>>
> 
> I still think it's better to split up the operations. The way you're
> handling it now is not clear.
> 
> If the func_awake() returns -EAGAIN, I'd expect that the remote wake did
> not go through and expect user to retry again. But here it does initiate
> remote wake, but it just does not send device notification yet. This is
> confusing.
> 
> Also, instead of all the function wake handling coming from the function
> driver, now we depend on the controller driver to call function resume()
> on state change to U0, which will trigger device notification. What
> happen if it doesn't call resume(). There's too many dependencies and it
> seems fragile.
> 
> I think all this can be handled in the function driver. You can fix the
> dwc3 wakeup() and poll for U0/ON state rather than RECOVERY state, which
> is what it's supposed to poll.

For transitioning from U3 to U0, the current upstream implementation is 
to poll for U0 state when dwc3_gadget_wakeup() is called and it is a 
blocking call. (this is a common API for both HS and SS)

	/* 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;
	}

In my experiments I found that certain hosts take longer time to drive
HS resume signalling between the remote wakeup and the resume K and this 
time varies across hosts. Hence the above polling timer is not generic 
across hosts. So how do we converge on a polling timer value to work 
across HS/SS and without blocking the system for a long time?

Some data layers like TCP/IP hold a TX lock while sending data (that 
causes a remote wakeup event) and hence this wakeup() may run in atomic 
context.

To make this generic across hosts, I had switched to interrupt based 
approach, enabling link state events and return error value immediately 
from the dwc3_gadget_wakeup() API after doing a LFPS handshake. But 
yeah, then we have to rely on the resume callback as an indication that 
link is transitioned to ON state.



  On usb_gadget_wakeup() returns
> successful, we'd expect the device is linked up and woken up. then you
> can send device notification through a different api such as
> usb_gadget_send_wake_notification().
> 
> Thanks,
> Thinh

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Thinh Nguyen]

On 8/16/2022, Elson Serrao wrote:
> 
> 
> On 8/12/2022 5:46 PM, Thinh Nguyen wrote:
>> On 8/11/2022, Thinh Nguyen wrote:
>>> On 8/11/2022, Thinh Nguyen wrote:
>>>> On 8/11/2022, Elson Serrao wrote:
>>>>>
>>>>>
>>>>> On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
>>>>
>>>> <snip>
>>>>
>>>>
>>>>>> To summarize the points:
>>>>>>
>>>>>> 1) The host only arms function remote wakeup if the device is
>>>>>> capable of
>>>>>> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and
>>>>>> hardware
>>>>>> capability)
>>>>>>
>>>>>> 2) If the device is in suspend, the device can do remote wakeup
>>>>>> (through
>>>>>> LFPS handshake) if the function is armed for remote wakeup (through
>>>>>> SET_FEATURE(FUNC_SUSPEND)).
>>>>>>
>>>>>> 3) If the link transitions to U0 after the device triggering a remote
>>>>>> wakeup, the device will also send device notification function
>>>>>> wake for
>>>>>> all the interfaces armed with remote wakeup.
>>>>>>
>>>>>> 4) If the device is not in suspend, the device can send device
>>>>>> notification function wake if it's in U0.
>>>>>>
>>>>>>
>>>>>> Now, remote wakeup and function wake device notification are 2
>>>>>> separate
>>>>>> operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
>>>>>> suggested to maybe add
>>>>>> usb_gadget_ops->send_wakeup_notification(gadget,
>>>>>> intf_id) for the device notification. What you did was combining both
>>>>>> operations in usb_gadget_ops->func_wakeup(). That may only work for
>>>>>> point 4) (assuming you fix the U0 check), but not point 3).
>>>>>
>>>>> Thank you for your feedback and summary. I will rename func_wakeup to
>>>>> send_wakeup_notification to better align with the approach. The
>>>>> reason I
>>>>> have combined remote_wakeup and function wake notification in
>>>>> usb_gadget_ops->func_wakeup() is because since the implementation
>>>>> is at
>>>>> function/composite level it has no knowledge on the link state. So I
>>>>> have delegated that task to controller driver to handle the
>>>>> notification
>>>>> accordingly. That is do a LFPS handshake first if the device is
>>>>> suspended and then send notification (explained below). But we can
>>>>> definitely separate this by adding an additional flag in the composite
>>>>> layer to set the link state based on the gadget suspend callback
>>>>> called
>>>>> when U3 SUSPEND interrupt is received. Let me know if you feel
>>>>> separating the two is a better approach.
>>>>>
>>>>
>>>> The reason I think we need to separate it is because of point 3. As I
>>>> note earlier, the spec states that "If remote wake event occurs in
>>>> multiple functions, each function shall send a Function Wake
>>>> Notification."
>>>>
>>>> But if there's no remote wake event, and the host brought the device up
>>>> instead, then the function suspend state is retained.
>>>>
>>>> If we separate these 2 operations, the caller can check whether the
>>>> operation went through properly. For example, if the wakeup() is
>>>> initiated properly, but the function wake device notification didn't go
>>>> through. We would only need to resend the device notification rather
>>>> than initiate remote wakeup again.
>>>
>>> If we don't have to send device notification for other interfaces, we
>>> can combine the operations here as you did.
>>>
>>
>> I still think it's better to split up the operations. The way you're
>> handling it now is not clear.
>>
>> If the func_awake() returns -EAGAIN, I'd expect that the remote wake did
>> not go through and expect user to retry again. But here it does initiate
>> remote wake, but it just does not send device notification yet. This is
>> confusing.
>>
>> Also, instead of all the function wake handling coming from the function
>> driver, now we depend on the controller driver to call function resume()
>> on state change to U0, which will trigger device notification. What
>> happen if it doesn't call resume(). There's too many dependencies and it
>> seems fragile.
>>
>> I think all this can be handled in the function driver. You can fix the
>> dwc3 wakeup() and poll for U0/ON state rather than RECOVERY state, which
>> is what it's supposed to poll.
> 
> For transitioning from U3 to U0, the current upstream implementation is
> to poll for U0 state when dwc3_gadget_wakeup() is called and it is a
> blocking call. (this is a common API for both HS and SS)
> 
>     /* 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;
>     }
> 
> In my experiments I found that certain hosts take longer time to drive
> HS resume signalling between the remote wakeup and the resume K and this
> time varies across hosts. Hence the above polling timer is not generic
> across hosts. So how do we converge on a polling timer value to work
> across HS/SS and without blocking the system for a long time?

Can't we take the upper limit of both base on experiment? And it
shouldn't be blocking the whole system.

> 
> Some data layers like TCP/IP hold a TX lock while sending data (that
> causes a remote wakeup event) and hence this wakeup() may run in atomic
> context.

Why hold the lock while waiting for the host to wakeup? The host is
still inactive. Also, the usb_gadget_wakeup() API doesn't specify that
it may run in atomic context.

> 
> To make this generic across hosts, I had switched to interrupt based
> approach, enabling link state events and return error value immediately
> from the dwc3_gadget_wakeup() API after doing a LFPS handshake. But
> yeah, then we have to rely on the resume callback as an indication that
> link is transitioned to ON state.
> 

BR,
Thinh

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Elson Serrao]

On 8/16/2022 4:51 PM, Thinh Nguyen wrote:
> On 8/16/2022, Elson Serrao wrote:
>>
>>
>> On 8/12/2022 5:46 PM, Thinh Nguyen wrote:
>>> On 8/11/2022, Thinh Nguyen wrote:
>>>> On 8/11/2022, Thinh Nguyen wrote:
>>>>> On 8/11/2022, Elson Serrao wrote:
>>>>>>
>>>>>>
>>>>>> On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
>>>>>
>>>>> <snip>
>>>>>
>>>>>
>>>>>>> To summarize the points:
>>>>>>>
>>>>>>> 1) The host only arms function remote wakeup if the device is
>>>>>>> capable of
>>>>>>> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and
>>>>>>> hardware
>>>>>>> capability)
>>>>>>>
>>>>>>> 2) If the device is in suspend, the device can do remote wakeup
>>>>>>> (through
>>>>>>> LFPS handshake) if the function is armed for remote wakeup (through
>>>>>>> SET_FEATURE(FUNC_SUSPEND)).
>>>>>>>
>>>>>>> 3) If the link transitions to U0 after the device triggering a remote
>>>>>>> wakeup, the device will also send device notification function
>>>>>>> wake for
>>>>>>> all the interfaces armed with remote wakeup.
>>>>>>>
>>>>>>> 4) If the device is not in suspend, the device can send device
>>>>>>> notification function wake if it's in U0.
>>>>>>>
>>>>>>>
>>>>>>> Now, remote wakeup and function wake device notification are 2
>>>>>>> separate
>>>>>>> operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
>>>>>>> suggested to maybe add
>>>>>>> usb_gadget_ops->send_wakeup_notification(gadget,
>>>>>>> intf_id) for the device notification. What you did was combining both
>>>>>>> operations in usb_gadget_ops->func_wakeup(). That may only work for
>>>>>>> point 4) (assuming you fix the U0 check), but not point 3).
>>>>>>
>>>>>> Thank you for your feedback and summary. I will rename func_wakeup to
>>>>>> send_wakeup_notification to better align with the approach. The
>>>>>> reason I
>>>>>> have combined remote_wakeup and function wake notification in
>>>>>> usb_gadget_ops->func_wakeup() is because since the implementation
>>>>>> is at
>>>>>> function/composite level it has no knowledge on the link state. So I
>>>>>> have delegated that task to controller driver to handle the
>>>>>> notification
>>>>>> accordingly. That is do a LFPS handshake first if the device is
>>>>>> suspended and then send notification (explained below). But we can
>>>>>> definitely separate this by adding an additional flag in the composite
>>>>>> layer to set the link state based on the gadget suspend callback
>>>>>> called
>>>>>> when U3 SUSPEND interrupt is received. Let me know if you feel
>>>>>> separating the two is a better approach.
>>>>>>
>>>>>
>>>>> The reason I think we need to separate it is because of point 3. As I
>>>>> note earlier, the spec states that "If remote wake event occurs in
>>>>> multiple functions, each function shall send a Function Wake
>>>>> Notification."
>>>>>
>>>>> But if there's no remote wake event, and the host brought the device up
>>>>> instead, then the function suspend state is retained.
>>>>>
>>>>> If we separate these 2 operations, the caller can check whether the
>>>>> operation went through properly. For example, if the wakeup() is
>>>>> initiated properly, but the function wake device notification didn't go
>>>>> through. We would only need to resend the device notification rather
>>>>> than initiate remote wakeup again.
>>>>
>>>> If we don't have to send device notification for other interfaces, we
>>>> can combine the operations here as you did.
>>>>
>>>
>>> I still think it's better to split up the operations. The way you're
>>> handling it now is not clear.
>>>
>>> If the func_awake() returns -EAGAIN, I'd expect that the remote wake did
>>> not go through and expect user to retry again. But here it does initiate
>>> remote wake, but it just does not send device notification yet. This is
>>> confusing.
>>>
>>> Also, instead of all the function wake handling coming from the function
>>> driver, now we depend on the controller driver to call function resume()
>>> on state change to U0, which will trigger device notification. What
>>> happen if it doesn't call resume(). There's too many dependencies and it
>>> seems fragile.
>>>
>>> I think all this can be handled in the function driver. You can fix the
>>> dwc3 wakeup() and poll for U0/ON state rather than RECOVERY state, which
>>> is what it's supposed to poll.
>>
>> For transitioning from U3 to U0, the current upstream implementation is
>> to poll for U0 state when dwc3_gadget_wakeup() is called and it is a
>> blocking call. (this is a common API for both HS and SS)
>>
>>      /* 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;
>>      }
>>
>> In my experiments I found that certain hosts take longer time to drive
>> HS resume signalling between the remote wakeup and the resume K and this
>> time varies across hosts. Hence the above polling timer is not generic
>> across hosts. So how do we converge on a polling timer value to work
>> across HS/SS and without blocking the system for a long time?
> 
> Can't we take the upper limit of both base on experiment? And it
> shouldn't be blocking the whole system.

On the host I was experimenting with, the time it took was around 110ms 
in HS case. That would translate to a retry count of about ~181,000 in 
the above polling loop. Wouldn't that be a very large value for polling?
And not sure if there are other hosts that take even longer time
> 
>>
>> Some data layers like TCP/IP hold a TX lock while sending data (that
>> causes a remote wakeup event) and hence this wakeup() may run in atomic
>> context.
> 
> Why hold the lock while waiting for the host to wakeup? The host is
> still inactive. Also, the usb_gadget_wakeup() API doesn't specify that
> it may run in atomic context.
> 

The lock might be held by upper layers who are unaware/independent of 
underlying transport medium. The above TX lock I was referring to was
that held by Linux networking stack. It just pushes out data the same 
way it would when USB is active. It is the function/composite layer 
being aware of the function suspend would now sense this as a remote 
wake event and perform this additional step of bringing out the link 
from u3 and then sending device wakeup notification.

In our current upstream implementation of dwc3_gadget_wakeup() API we 
hold a spinlock as well. But yeah that can be rectified

static int dwc3_gadget_wakeup(struct usb_gadget *g)
{
	struct dwc3		*dwc = gadget_to_dwc(g);
	unsigned long		flags;
	int			ret;

	spin_lock_irqsave(&dwc->lock, flags);
	ret = __dwc3_gadget_wakeup(dwc);
	spin_unlock_irqrestore(&dwc->lock, flags);

	return ret;
}


>>
>> To make this generic across hosts, I had switched to interrupt based
>> approach, enabling link state events and return error value immediately
>> from the dwc3_gadget_wakeup() API after doing a LFPS handshake. But
>> yeah, then we have to rely on the resume callback as an indication that
>> link is transitioned to ON state.
>>
> 
> BR,
> Thinh
> 

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Elson Serrao]

On 8/18/2022 11:17 AM, Elson Serrao wrote:
> 
> 
> On 8/16/2022 4:51 PM, Thinh Nguyen wrote:
>> On 8/16/2022, Elson Serrao wrote:
>>>
>>>
>>> On 8/12/2022 5:46 PM, Thinh Nguyen wrote:
>>>> On 8/11/2022, Thinh Nguyen wrote:
>>>>> On 8/11/2022, Thinh Nguyen wrote:
>>>>>> On 8/11/2022, Elson Serrao wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
>>>>>>
>>>>>> <snip>
>>>>>>
>>>>>>
>>>>>>>> To summarize the points:
>>>>>>>>
>>>>>>>> 1) The host only arms function remote wakeup if the device is
>>>>>>>> capable of
>>>>>>>> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and
>>>>>>>> hardware
>>>>>>>> capability)
>>>>>>>>
>>>>>>>> 2) If the device is in suspend, the device can do remote wakeup
>>>>>>>> (through
>>>>>>>> LFPS handshake) if the function is armed for remote wakeup (through
>>>>>>>> SET_FEATURE(FUNC_SUSPEND)).
>>>>>>>>
>>>>>>>> 3) If the link transitions to U0 after the device triggering a 
>>>>>>>> remote
>>>>>>>> wakeup, the device will also send device notification function
>>>>>>>> wake for
>>>>>>>> all the interfaces armed with remote wakeup.
>>>>>>>>
>>>>>>>> 4) If the device is not in suspend, the device can send device
>>>>>>>> notification function wake if it's in U0.
>>>>>>>>
>>>>>>>>
>>>>>>>> Now, remote wakeup and function wake device notification are 2
>>>>>>>> separate
>>>>>>>> operations. We have the usb_gadget_ops->wakeup() for remote 
>>>>>>>> wakeup. I
>>>>>>>> suggested to maybe add
>>>>>>>> usb_gadget_ops->send_wakeup_notification(gadget,
>>>>>>>> intf_id) for the device notification. What you did was combining 
>>>>>>>> both
>>>>>>>> operations in usb_gadget_ops->func_wakeup(). That may only work for
>>>>>>>> point 4) (assuming you fix the U0 check), but not point 3).
>>>>>>>
>>>>>>> Thank you for your feedback and summary. I will rename 
>>>>>>> func_wakeup to
>>>>>>> send_wakeup_notification to better align with the approach. The
>>>>>>> reason I
>>>>>>> have combined remote_wakeup and function wake notification in
>>>>>>> usb_gadget_ops->func_wakeup() is because since the implementation
>>>>>>> is at
>>>>>>> function/composite level it has no knowledge on the link state. So I
>>>>>>> have delegated that task to controller driver to handle the
>>>>>>> notification
>>>>>>> accordingly. That is do a LFPS handshake first if the device is
>>>>>>> suspended and then send notification (explained below). But we can
>>>>>>> definitely separate this by adding an additional flag in the 
>>>>>>> composite
>>>>>>> layer to set the link state based on the gadget suspend callback
>>>>>>> called
>>>>>>> when U3 SUSPEND interrupt is received. Let me know if you feel
>>>>>>> separating the two is a better approach.
>>>>>>>
>>>>>>
>>>>>> The reason I think we need to separate it is because of point 3. As I
>>>>>> note earlier, the spec states that "If remote wake event occurs in
>>>>>> multiple functions, each function shall send a Function Wake
>>>>>> Notification."
>>>>>>
>>>>>> But if there's no remote wake event, and the host brought the 
>>>>>> device up
>>>>>> instead, then the function suspend state is retained.
>>>>>>
>>>>>> If we separate these 2 operations, the caller can check whether the
>>>>>> operation went through properly. For example, if the wakeup() is
>>>>>> initiated properly, but the function wake device notification 
>>>>>> didn't go
>>>>>> through. We would only need to resend the device notification rather
>>>>>> than initiate remote wakeup again.
>>>>>
>>>>> If we don't have to send device notification for other interfaces, we
>>>>> can combine the operations here as you did.
>>>>>
>>>>
>>>> I still think it's better to split up the operations. The way you're
>>>> handling it now is not clear.
>>>>
>>>> If the func_awake() returns -EAGAIN, I'd expect that the remote wake 
>>>> did
>>>> not go through and expect user to retry again. But here it does 
>>>> initiate
>>>> remote wake, but it just does not send device notification yet. This is
>>>> confusing.
>>>>
>>>> Also, instead of all the function wake handling coming from the 
>>>> function
>>>> driver, now we depend on the controller driver to call function 
>>>> resume()
>>>> on state change to U0, which will trigger device notification. What
>>>> happen if it doesn't call resume(). There's too many dependencies 
>>>> and it
>>>> seems fragile.
>>>>
>>>> I think all this can be handled in the function driver. You can fix the
>>>> dwc3 wakeup() and poll for U0/ON state rather than RECOVERY state, 
>>>> which
>>>> is what it's supposed to poll.
>>>
>>> For transitioning from U3 to U0, the current upstream implementation is
>>> to poll for U0 state when dwc3_gadget_wakeup() is called and it is a
>>> blocking call. (this is a common API for both HS and SS)
>>>
>>>      /* 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;
>>>      }
>>>
>>> In my experiments I found that certain hosts take longer time to drive
>>> HS resume signalling between the remote wakeup and the resume K and this
>>> time varies across hosts. Hence the above polling timer is not generic
>>> across hosts. So how do we converge on a polling timer value to work
>>> across HS/SS and without blocking the system for a long time?
>>
>> Can't we take the upper limit of both base on experiment? And it
>> shouldn't be blocking the whole system.
> 
> On the host I was experimenting with, the time it took was around 110ms 
> in HS case. That would translate to a retry count of about ~181,000 in 
> the above polling loop. Wouldn't that be a very large value for polling?
> And not sure if there are other hosts that take even longer time

Also we may need additional buffer on the retry value to absorb the run 
to run variation.

Below is the retry count kernel logs from my HS mode experiment when a 
remote wakeup was triggered

[   83.014458][  T191] Start Polling
[   83.147318][  T191] End polling. Retry count:223193


/* poll until Link State changes to ON */
#define COUNT 500000
retries = COUNT;
pr_err("Start Polling\n");
while (retries--) {
		reg = dwc3_readl(dwc->regs, DWC3_DSTS);

		/* in HS, means ON */
		if (DWC3_DSTS_USBLNKST(reg) == DWC3_LINK_STATE_U0)
				break;
}
pr_err("End polling. Retry count:%d\n",(COUNT-retries));

>>
>>>
>>> Some data layers like TCP/IP hold a TX lock while sending data (that
>>> causes a remote wakeup event) and hence this wakeup() may run in atomic
>>> context.
>>
>> Why hold the lock while waiting for the host to wakeup? The host is
>> still inactive. Also, the usb_gadget_wakeup() API doesn't specify that
>> it may run in atomic context.
>>
> 
> The lock might be held by upper layers who are unaware/independent of 
> underlying transport medium. The above TX lock I was referring to was
> that held by Linux networking stack. It just pushes out data the same 
> way it would when USB is active. It is the function/composite layer 
> being aware of the function suspend would now sense this as a remote 
> wake event and perform this additional step of bringing out the link 
> from u3 and then sending device wakeup notification.
> 
> In our current upstream implementation of dwc3_gadget_wakeup() API we 
> hold a spinlock as well. But yeah that can be rectified
> 
> static int dwc3_gadget_wakeup(struct usb_gadget *g)
> {
>      struct dwc3        *dwc = gadget_to_dwc(g);
>      unsigned long        flags;
>      int            ret;
> 
>      spin_lock_irqsave(&dwc->lock, flags);
>      ret = __dwc3_gadget_wakeup(dwc);
>      spin_unlock_irqrestore(&dwc->lock, flags);
> 
>      return ret;
> }
> 
> 
>>>
>>> To make this generic across hosts, I had switched to interrupt based
>>> approach, enabling link state events and return error value immediately
>>> from the dwc3_gadget_wakeup() API after doing a LFPS handshake. But
>>> yeah, then we have to rely on the resume callback as an indication that
>>> link is transitioned to ON state.
>>>
>>
>> BR,
>> Thinh
>>

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Thinh Nguyen]

On Thu, Aug 18, 2022 at 11:17:24AM -0700, Elson Serrao wrote:
> 
> 
> On 8/16/2022 4:51 PM, Thinh Nguyen wrote:
> > On 8/16/2022, Elson Serrao wrote:
> > > 
> > > 
> > > On 8/12/2022 5:46 PM, Thinh Nguyen wrote:
> > > > On 8/11/2022, Thinh Nguyen wrote:
> > > > > On 8/11/2022, Thinh Nguyen wrote:
> > > > > > On 8/11/2022, Elson Serrao wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
> > > > > > 
> > > > > > <snip>
> > > > > > 
> > > > > > 
> > > > > > > > To summarize the points:
> > > > > > > > 
> > > > > > > > 1) The host only arms function remote wakeup if the device is
> > > > > > > > capable of
> > > > > > > > remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and
> > > > > > > > hardware
> > > > > > > > capability)
> > > > > > > > 
> > > > > > > > 2) If the device is in suspend, the device can do remote wakeup
> > > > > > > > (through
> > > > > > > > LFPS handshake) if the function is armed for remote wakeup (through
> > > > > > > > SET_FEATURE(FUNC_SUSPEND)).
> > > > > > > > 
> > > > > > > > 3) If the link transitions to U0 after the device triggering a remote
> > > > > > > > wakeup, the device will also send device notification function
> > > > > > > > wake for
> > > > > > > > all the interfaces armed with remote wakeup.
> > > > > > > > 
> > > > > > > > 4) If the device is not in suspend, the device can send device
> > > > > > > > notification function wake if it's in U0.
> > > > > > > > 
> > > > > > > > 
> > > > > > > > Now, remote wakeup and function wake device notification are 2
> > > > > > > > separate
> > > > > > > > operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
> > > > > > > > suggested to maybe add
> > > > > > > > usb_gadget_ops->send_wakeup_notification(gadget,
> > > > > > > > intf_id) for the device notification. What you did was combining both
> > > > > > > > operations in usb_gadget_ops->func_wakeup(). That may only work for
> > > > > > > > point 4) (assuming you fix the U0 check), but not point 3).
> > > > > > > 
> > > > > > > Thank you for your feedback and summary. I will rename func_wakeup to
> > > > > > > send_wakeup_notification to better align with the approach. The
> > > > > > > reason I
> > > > > > > have combined remote_wakeup and function wake notification in
> > > > > > > usb_gadget_ops->func_wakeup() is because since the implementation
> > > > > > > is at
> > > > > > > function/composite level it has no knowledge on the link state. So I
> > > > > > > have delegated that task to controller driver to handle the
> > > > > > > notification
> > > > > > > accordingly. That is do a LFPS handshake first if the device is
> > > > > > > suspended and then send notification (explained below). But we can
> > > > > > > definitely separate this by adding an additional flag in the composite
> > > > > > > layer to set the link state based on the gadget suspend callback
> > > > > > > called
> > > > > > > when U3 SUSPEND interrupt is received. Let me know if you feel
> > > > > > > separating the two is a better approach.
> > > > > > > 
> > > > > > 
> > > > > > The reason I think we need to separate it is because of point 3. As I
> > > > > > note earlier, the spec states that "If remote wake event occurs in
> > > > > > multiple functions, each function shall send a Function Wake
> > > > > > Notification."
> > > > > > 
> > > > > > But if there's no remote wake event, and the host brought the device up
> > > > > > instead, then the function suspend state is retained.
> > > > > > 
> > > > > > If we separate these 2 operations, the caller can check whether the
> > > > > > operation went through properly. For example, if the wakeup() is
> > > > > > initiated properly, but the function wake device notification didn't go
> > > > > > through. We would only need to resend the device notification rather
> > > > > > than initiate remote wakeup again.
> > > > > 
> > > > > If we don't have to send device notification for other interfaces, we
> > > > > can combine the operations here as you did.
> > > > > 
> > > > 
> > > > I still think it's better to split up the operations. The way you're
> > > > handling it now is not clear.
> > > > 
> > > > If the func_awake() returns -EAGAIN, I'd expect that the remote wake did
> > > > not go through and expect user to retry again. But here it does initiate
> > > > remote wake, but it just does not send device notification yet. This is
> > > > confusing.
> > > > 
> > > > Also, instead of all the function wake handling coming from the function
> > > > driver, now we depend on the controller driver to call function resume()
> > > > on state change to U0, which will trigger device notification. What
> > > > happen if it doesn't call resume(). There's too many dependencies and it
> > > > seems fragile.
> > > > 
> > > > I think all this can be handled in the function driver. You can fix the
> > > > dwc3 wakeup() and poll for U0/ON state rather than RECOVERY state, which
> > > > is what it's supposed to poll.
> > > 
> > > For transitioning from U3 to U0, the current upstream implementation is
> > > to poll for U0 state when dwc3_gadget_wakeup() is called and it is a
> > > blocking call. (this is a common API for both HS and SS)
> > > 
> > >      /* 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;
> > >      }
> > > 
> > > In my experiments I found that certain hosts take longer time to drive
> > > HS resume signalling between the remote wakeup and the resume K and this
> > > time varies across hosts. Hence the above polling timer is not generic
> > > across hosts. So how do we converge on a polling timer value to work
> > > across HS/SS and without blocking the system for a long time?
> > 
> > Can't we take the upper limit of both base on experiment? And it
> > shouldn't be blocking the whole system.
> 
> On the host I was experimenting with, the time it took was around 110ms in
> HS case. That would translate to a retry count of about ~181,000 in the
> above polling loop. Wouldn't that be a very large value for polling?
> And not sure if there are other hosts that take even longer time

We don't want to poll that many times. We shouldn't depend on the delay
of a register read for polling interval. Can't we just sleep in between
interval at a reasonable interval.

> > 
> > > 
> > > Some data layers like TCP/IP hold a TX lock while sending data (that
> > > causes a remote wakeup event) and hence this wakeup() may run in atomic
> > > context.
> > 
> > Why hold the lock while waiting for the host to wakeup? The host is
> > still inactive. Also, the usb_gadget_wakeup() API doesn't specify that
> > it may run in atomic context.
> > 
> 
> The lock might be held by upper layers who are unaware/independent of
> underlying transport medium. The above TX lock I was referring to was
> that held by Linux networking stack. It just pushes out data the same way it
> would when USB is active. It is the function/composite layer being aware of
> the function suspend would now sense this as a remote wake event and perform
> this additional step of bringing out the link from u3 and then sending
> device wakeup notification.
> 
> In our current upstream implementation of dwc3_gadget_wakeup() API we hold a
> spinlock as well. But yeah that can be rectified

Holding a spin_lock for this long is not reasonable. We only need to
lock when setting link recovery request but not while polling for DSTS
and waiting for the link to go up.

BR,
Thinh

> 
> static int dwc3_gadget_wakeup(struct usb_gadget *g)
> {
> 	struct dwc3		*dwc = gadget_to_dwc(g);
> 	unsigned long		flags;
> 	int			ret;
> 
> 	spin_lock_irqsave(&dwc->lock, flags);
> 	ret = __dwc3_gadget_wakeup(dwc);
> 	spin_unlock_irqrestore(&dwc->lock, flags);
> 
> 	return ret;
> }
> 
> 
> > > 
> > > To make this generic across hosts, I had switched to interrupt based
> > > approach, enabling link state events and return error value immediately
> > > from the dwc3_gadget_wakeup() API after doing a LFPS handshake. But
> > > yeah, then we have to rely on the resume callback as an indication that
> > > link is transitioned to ON state.
> > > 
> > 
> > BR,
> > Thinh
> > 

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Elson Serrao]

On 8/22/2022 6:01 PM, Thinh Nguyen wrote:
> On Thu, Aug 18, 2022 at 11:17:24AM -0700, Elson Serrao wrote:
>>
>>
>> On 8/16/2022 4:51 PM, Thinh Nguyen wrote:
>>> On 8/16/2022, Elson Serrao wrote:
>>>>
>>>>
>>>> On 8/12/2022 5:46 PM, Thinh Nguyen wrote:
>>>>> On 8/11/2022, Thinh Nguyen wrote:
>>>>>> On 8/11/2022, Thinh Nguyen wrote:
>>>>>>> On 8/11/2022, Elson Serrao wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
>>>>>>>
>>>>>>> <snip>
>>>>>>>
>>>>>>>
>>>>>>>>> To summarize the points:
>>>>>>>>>
>>>>>>>>> 1) The host only arms function remote wakeup if the device is
>>>>>>>>> capable of
>>>>>>>>> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and
>>>>>>>>> hardware
>>>>>>>>> capability)
>>>>>>>>>
>>>>>>>>> 2) If the device is in suspend, the device can do remote wakeup
>>>>>>>>> (through
>>>>>>>>> LFPS handshake) if the function is armed for remote wakeup (through
>>>>>>>>> SET_FEATURE(FUNC_SUSPEND)).
>>>>>>>>>
>>>>>>>>> 3) If the link transitions to U0 after the device triggering a remote
>>>>>>>>> wakeup, the device will also send device notification function
>>>>>>>>> wake for
>>>>>>>>> all the interfaces armed with remote wakeup.
>>>>>>>>>
>>>>>>>>> 4) If the device is not in suspend, the device can send device
>>>>>>>>> notification function wake if it's in U0.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Now, remote wakeup and function wake device notification are 2
>>>>>>>>> separate
>>>>>>>>> operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
>>>>>>>>> suggested to maybe add
>>>>>>>>> usb_gadget_ops->send_wakeup_notification(gadget,
>>>>>>>>> intf_id) for the device notification. What you did was combining both
>>>>>>>>> operations in usb_gadget_ops->func_wakeup(). That may only work for
>>>>>>>>> point 4) (assuming you fix the U0 check), but not point 3).
>>>>>>>>
>>>>>>>> Thank you for your feedback and summary. I will rename func_wakeup to
>>>>>>>> send_wakeup_notification to better align with the approach. The
>>>>>>>> reason I
>>>>>>>> have combined remote_wakeup and function wake notification in
>>>>>>>> usb_gadget_ops->func_wakeup() is because since the implementation
>>>>>>>> is at
>>>>>>>> function/composite level it has no knowledge on the link state. So I
>>>>>>>> have delegated that task to controller driver to handle the
>>>>>>>> notification
>>>>>>>> accordingly. That is do a LFPS handshake first if the device is
>>>>>>>> suspended and then send notification (explained below). But we can
>>>>>>>> definitely separate this by adding an additional flag in the composite
>>>>>>>> layer to set the link state based on the gadget suspend callback
>>>>>>>> called
>>>>>>>> when U3 SUSPEND interrupt is received. Let me know if you feel
>>>>>>>> separating the two is a better approach.
>>>>>>>>
>>>>>>>
>>>>>>> The reason I think we need to separate it is because of point 3. As I
>>>>>>> note earlier, the spec states that "If remote wake event occurs in
>>>>>>> multiple functions, each function shall send a Function Wake
>>>>>>> Notification."
>>>>>>>
>>>>>>> But if there's no remote wake event, and the host brought the device up
>>>>>>> instead, then the function suspend state is retained.
>>>>>>>
>>>>>>> If we separate these 2 operations, the caller can check whether the
>>>>>>> operation went through properly. For example, if the wakeup() is
>>>>>>> initiated properly, but the function wake device notification didn't go
>>>>>>> through. We would only need to resend the device notification rather
>>>>>>> than initiate remote wakeup again.
>>>>>>
>>>>>> If we don't have to send device notification for other interfaces, we
>>>>>> can combine the operations here as you did.
>>>>>>
>>>>>
>>>>> I still think it's better to split up the operations. The way you're
>>>>> handling it now is not clear.
>>>>>
>>>>> If the func_awake() returns -EAGAIN, I'd expect that the remote wake did
>>>>> not go through and expect user to retry again. But here it does initiate
>>>>> remote wake, but it just does not send device notification yet. This is
>>>>> confusing.
>>>>>
>>>>> Also, instead of all the function wake handling coming from the function
>>>>> driver, now we depend on the controller driver to call function resume()
>>>>> on state change to U0, which will trigger device notification. What
>>>>> happen if it doesn't call resume(). There's too many dependencies and it
>>>>> seems fragile.
>>>>>
>>>>> I think all this can be handled in the function driver. You can fix the
>>>>> dwc3 wakeup() and poll for U0/ON state rather than RECOVERY state, which
>>>>> is what it's supposed to poll.
>>>>
>>>> For transitioning from U3 to U0, the current upstream implementation is
>>>> to poll for U0 state when dwc3_gadget_wakeup() is called and it is a
>>>> blocking call. (this is a common API for both HS and SS)
>>>>
>>>>       /* 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;
>>>>       }
>>>>
>>>> In my experiments I found that certain hosts take longer time to drive
>>>> HS resume signalling between the remote wakeup and the resume K and this
>>>> time varies across hosts. Hence the above polling timer is not generic
>>>> across hosts. So how do we converge on a polling timer value to work
>>>> across HS/SS and without blocking the system for a long time?
>>>
>>> Can't we take the upper limit of both base on experiment? And it
>>> shouldn't be blocking the whole system.
>>
>> On the host I was experimenting with, the time it took was around 110ms in
>> HS case. That would translate to a retry count of about ~181,000 in the
>> above polling loop. Wouldn't that be a very large value for polling?
>> And not sure if there are other hosts that take even longer time
> 
> We don't want to poll that many times. We shouldn't depend on the delay
> of a register read for polling interval. Can't we just sleep in between
> interval at a reasonable interval.
> 

Sleeping is not an option as the upper layers (those beyond 
function/composite layer) may transmit data with a lock held.

I ran into below BUG when remote wakeup was triggered via a ping() 
command and attempted sleep while polling

[   88.676789][  T392] BUG: scheduling while atomic
[   88.900112][  T392] Call trace:
<snip>
[   88.912760][  T392]  __schedule_bug+0x90/0x188
[   88.917335][  T392]  __schedule+0x714/0xb4c
[   88.930568][  T392]  schedule+0x110/0x204
[   88.943620][  T392]  schedule_timeout+0x94/0x134
[   88.948371][  T392]  __dwc3_gadget_wakeup+0x1ac/0x558
[   88.953558][  T392]  dwc3_gadget_wakeup+0x3c/0x8c
[   88.958388][  T392]  usb_gadget_wakeup+0x54/0x1a8
[   88.963222][  T392]  eth_start_xmit+0x130/0x830
[   88.967876][  T392]  xmit_one+0xf0/0x364
[   88.971913][  T392]  sch_direct_xmit+0x188/0x3e4
[   88.976663][  T392]  __dev_xmit_skb+0x480/0x984
[   88.981319][  T392]  __dev_queue_xmit+0x2d4/0x7d8
[   88.986151][  T392]  neigh_resolve_output+0x208/0x2f0
<snip>

The above experiment was done by removing spin_locks if any in the 
wakeup() path of  function/composite/controller drivers. It is running 
in atomic context due to the lock held by linux networking stack/generic 
packet scheduler.

So below are the only two approaches I can think of for 
dwc3_gadget_wakeup() API

1.)Polling based approach: We poll until the link comes up. But cannot 
sleep while polling due to above reasons.

2.)Interrupt based approach (the patches being discussed currently): 
When a remote wakeup is triggered enable link state interrupts and 
return right away. The function/composite drivers are later notified 
about the ON event via resume callback (in a similar way how we notify 
U3 to composite layer via suspend callback).

Please let me know if there is any alternate way that you can think of here.

>>>
>>>>
>>>> Some data layers like TCP/IP hold a TX lock while sending data (that
>>>> causes a remote wakeup event) and hence this wakeup() may run in atomic
>>>> context.
>>>
>>> Why hold the lock while waiting for the host to wakeup? The host is
>>> still inactive. Also, the usb_gadget_wakeup() API doesn't specify that
>>> it may run in atomic context.
>>>
>>
>> The lock might be held by upper layers who are unaware/independent of
>> underlying transport medium. The above TX lock I was referring to was
>> that held by Linux networking stack. It just pushes out data the same way it
>> would when USB is active. It is the function/composite layer being aware of
>> the function suspend would now sense this as a remote wake event and perform
>> this additional step of bringing out the link from u3 and then sending
>> device wakeup notification.
>>
>> In our current upstream implementation of dwc3_gadget_wakeup() API we hold a
>> spinlock as well. But yeah that can be rectified
> 
> Holding a spin_lock for this long is not reasonable. We only need to
> lock when setting link recovery request but not while polling for DSTS
> and waiting for the link to go up.
> 
> BR,
> Thinh
> 
>>
>> static int dwc3_gadget_wakeup(struct usb_gadget *g)
>> {
>> 	struct dwc3		*dwc = gadget_to_dwc(g);
>> 	unsigned long		flags;
>> 	int			ret;
>>
>> 	spin_lock_irqsave(&dwc->lock, flags);
>> 	ret = __dwc3_gadget_wakeup(dwc);
>> 	spin_unlock_irqrestore(&dwc->lock, flags);
>>
>> 	return ret;
>> }
>>
>>
>>>>
>>>> To make this generic across hosts, I had switched to interrupt based
>>>> approach, enabling link state events and return error value immediately
>>>> from the dwc3_gadget_wakeup() API after doing a LFPS handshake. But
>>>> yeah, then we have to rely on the resume callback as an indication that
>>>> link is transitioned to ON state.
>>>>
>>>
>>> BR,
>>> Thinh

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Thinh Nguyen]

On Tue, Aug 23, 2022, Elson Serrao wrote:
> On 8/22/2022 6:01 PM, Thinh Nguyen wrote:
> > On Thu, Aug 18, 2022 at 11:17:24AM -0700, Elson Serrao wrote:
> > > 
> > > 
> > > On 8/16/2022 4:51 PM, Thinh Nguyen wrote:
> > > > On 8/16/2022, Elson Serrao wrote:
> > > > > 
> > > > > 
> > > > > On 8/12/2022 5:46 PM, Thinh Nguyen wrote:
> > > > > > On 8/11/2022, Thinh Nguyen wrote:
> > > > > > > On 8/11/2022, Thinh Nguyen wrote:
> > > > > > > > On 8/11/2022, Elson Serrao wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
> > > > > > > > 
> > > > > > > > <snip>
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > > To summarize the points:
> > > > > > > > > > 
> > > > > > > > > > 1) The host only arms function remote wakeup if the device is
> > > > > > > > > > capable of
> > > > > > > > > > remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and
> > > > > > > > > > hardware
> > > > > > > > > > capability)
> > > > > > > > > > 
> > > > > > > > > > 2) If the device is in suspend, the device can do remote wakeup
> > > > > > > > > > (through
> > > > > > > > > > LFPS handshake) if the function is armed for remote wakeup (through
> > > > > > > > > > SET_FEATURE(FUNC_SUSPEND)).
> > > > > > > > > > 
> > > > > > > > > > 3) If the link transitions to U0 after the device triggering a remote
> > > > > > > > > > wakeup, the device will also send device notification function
> > > > > > > > > > wake for
> > > > > > > > > > all the interfaces armed with remote wakeup.
> > > > > > > > > > 
> > > > > > > > > > 4) If the device is not in suspend, the device can send device
> > > > > > > > > > notification function wake if it's in U0.
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Now, remote wakeup and function wake device notification are 2
> > > > > > > > > > separate
> > > > > > > > > > operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
> > > > > > > > > > suggested to maybe add
> > > > > > > > > > usb_gadget_ops->send_wakeup_notification(gadget,
> > > > > > > > > > intf_id) for the device notification. What you did was combining both
> > > > > > > > > > operations in usb_gadget_ops->func_wakeup(). That may only work for
> > > > > > > > > > point 4) (assuming you fix the U0 check), but not point 3).
> > > > > > > > > 
> > > > > > > > > Thank you for your feedback and summary. I will rename func_wakeup to
> > > > > > > > > send_wakeup_notification to better align with the approach. The
> > > > > > > > > reason I
> > > > > > > > > have combined remote_wakeup and function wake notification in
> > > > > > > > > usb_gadget_ops->func_wakeup() is because since the implementation
> > > > > > > > > is at
> > > > > > > > > function/composite level it has no knowledge on the link state. So I
> > > > > > > > > have delegated that task to controller driver to handle the
> > > > > > > > > notification
> > > > > > > > > accordingly. That is do a LFPS handshake first if the device is
> > > > > > > > > suspended and then send notification (explained below). But we can
> > > > > > > > > definitely separate this by adding an additional flag in the composite
> > > > > > > > > layer to set the link state based on the gadget suspend callback
> > > > > > > > > called
> > > > > > > > > when U3 SUSPEND interrupt is received. Let me know if you feel
> > > > > > > > > separating the two is a better approach.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > The reason I think we need to separate it is because of point 3. As I
> > > > > > > > note earlier, the spec states that "If remote wake event occurs in
> > > > > > > > multiple functions, each function shall send a Function Wake
> > > > > > > > Notification."
> > > > > > > > 
> > > > > > > > But if there's no remote wake event, and the host brought the device up
> > > > > > > > instead, then the function suspend state is retained.
> > > > > > > > 
> > > > > > > > If we separate these 2 operations, the caller can check whether the
> > > > > > > > operation went through properly. For example, if the wakeup() is
> > > > > > > > initiated properly, but the function wake device notification didn't go
> > > > > > > > through. We would only need to resend the device notification rather
> > > > > > > > than initiate remote wakeup again.
> > > > > > > 
> > > > > > > If we don't have to send device notification for other interfaces, we
> > > > > > > can combine the operations here as you did.
> > > > > > > 
> > > > > > 
> > > > > > I still think it's better to split up the operations. The way you're
> > > > > > handling it now is not clear.
> > > > > > 
> > > > > > If the func_awake() returns -EAGAIN, I'd expect that the remote wake did
> > > > > > not go through and expect user to retry again. But here it does initiate
> > > > > > remote wake, but it just does not send device notification yet. This is
> > > > > > confusing.
> > > > > > 
> > > > > > Also, instead of all the function wake handling coming from the function
> > > > > > driver, now we depend on the controller driver to call function resume()
> > > > > > on state change to U0, which will trigger device notification. What
> > > > > > happen if it doesn't call resume(). There's too many dependencies and it
> > > > > > seems fragile.
> > > > > > 
> > > > > > I think all this can be handled in the function driver. You can fix the
> > > > > > dwc3 wakeup() and poll for U0/ON state rather than RECOVERY state, which
> > > > > > is what it's supposed to poll.
> > > > > 
> > > > > For transitioning from U3 to U0, the current upstream implementation is
> > > > > to poll for U0 state when dwc3_gadget_wakeup() is called and it is a
> > > > > blocking call. (this is a common API for both HS and SS)
> > > > > 
> > > > >       /* 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;
> > > > >       }
> > > > > 
> > > > > In my experiments I found that certain hosts take longer time to drive
> > > > > HS resume signalling between the remote wakeup and the resume K and this
> > > > > time varies across hosts. Hence the above polling timer is not generic
> > > > > across hosts. So how do we converge on a polling timer value to work
> > > > > across HS/SS and without blocking the system for a long time?
> > > > 
> > > > Can't we take the upper limit of both base on experiment? And it
> > > > shouldn't be blocking the whole system.
> > > 
> > > On the host I was experimenting with, the time it took was around 110ms in
> > > HS case. That would translate to a retry count of about ~181,000 in the
> > > above polling loop. Wouldn't that be a very large value for polling?
> > > And not sure if there are other hosts that take even longer time
> > 
> > We don't want to poll that many times. We shouldn't depend on the delay
> > of a register read for polling interval. Can't we just sleep in between
> > interval at a reasonable interval.
> > 
> 
> Sleeping is not an option as the upper layers (those beyond
> function/composite layer) may transmit data with a lock held.
> 

You can use mdelay() if it can't sleep. But if the wait is long, it
should be allowed to sleep.

> I ran into below BUG when remote wakeup was triggered via a ping() command
> and attempted sleep while polling
> 
> [   88.676789][  T392] BUG: scheduling while atomic
> [   88.900112][  T392] Call trace:
> <snip>
> [   88.912760][  T392]  __schedule_bug+0x90/0x188
> [   88.917335][  T392]  __schedule+0x714/0xb4c
> [   88.930568][  T392]  schedule+0x110/0x204
> [   88.943620][  T392]  schedule_timeout+0x94/0x134
> [   88.948371][  T392]  __dwc3_gadget_wakeup+0x1ac/0x558
> [   88.953558][  T392]  dwc3_gadget_wakeup+0x3c/0x8c
> [   88.958388][  T392]  usb_gadget_wakeup+0x54/0x1a8
> [   88.963222][  T392]  eth_start_xmit+0x130/0x830
> [   88.967876][  T392]  xmit_one+0xf0/0x364
> [   88.971913][  T392]  sch_direct_xmit+0x188/0x3e4
> [   88.976663][  T392]  __dev_xmit_skb+0x480/0x984
> [   88.981319][  T392]  __dev_queue_xmit+0x2d4/0x7d8
> [   88.986151][  T392]  neigh_resolve_output+0x208/0x2f0
> <snip>
> 
> The above experiment was done by removing spin_locks if any in the wakeup()
> path of  function/composite/controller drivers. It is running in atomic
> context due to the lock held by linux networking stack/generic packet
> scheduler.
> 
> So below are the only two approaches I can think of for dwc3_gadget_wakeup()
> API
> 
> 1.)Polling based approach: We poll until the link comes up. But cannot sleep
> while polling due to above reasons.
> 
> 2.)Interrupt based approach (the patches being discussed currently): When a
> remote wakeup is triggered enable link state interrupts and return right
> away. The function/composite drivers are later notified about the ON event
> via resume callback (in a similar way how we notify U3 to composite layer
> via suspend callback).
> 
> Please let me know if there is any alternate way that you can think of here.
> 

The main issue we're trying to solve is knowing if the host had woken up
and the device notification is sent so that the function driver can
resume().

If we can say that upon usb_gadget_wakeup() successful completion, the
link is in U0/ON, then the function driver can send the wake
notification after and resume(). That is, we're trying to make
usb_gadget_wakeup() synchronous. Whether it's polling or interrupt
based, it's implementation detail.

Unfortunately, the API isn't very clear whether usb_gadget_wakeup() may
sleep or synchronous.

Here are 3 approaches that we can have:

1) Clarify that usb_gadget_wakeup() is synchronous and the link will be
in U0/ON upon successful completion, and clarify whether it can sleep.
Introduce a separate API usb_gadget_send_wake_notification() to send
wake notification separately.

2) Create a new API usb_gadget_function_wakeup() that will combine both
device wakeup and wake notification. The function can sleep,
synchronous, and both wakeup and wake notification are done after the
function completes.

3) Create a new API usb_gadget_function_wakeup() that will combine both
device wakeup and wake notification. The function is asynchronous. We
won't know if the wakeup is successfully sent, but we don't care and
proceed with the function proceed with resume() anyway.

BR,
Thinh


> > > > 
> > > > > 
> > > > > Some data layers like TCP/IP hold a TX lock while sending data (that
> > > > > causes a remote wakeup event) and hence this wakeup() may run in atomic
> > > > > context.
> > > > 
> > > > Why hold the lock while waiting for the host to wakeup? The host is
> > > > still inactive. Also, the usb_gadget_wakeup() API doesn't specify that
> > > > it may run in atomic context.
> > > > 
> > > 
> > > The lock might be held by upper layers who are unaware/independent of
> > > underlying transport medium. The above TX lock I was referring to was
> > > that held by Linux networking stack. It just pushes out data the same way it
> > > would when USB is active. It is the function/composite layer being aware of
> > > the function suspend would now sense this as a remote wake event and perform
> > > this additional step of bringing out the link from u3 and then sending
> > > device wakeup notification.
> > > 
> > > In our current upstream implementation of dwc3_gadget_wakeup() API we hold a
> > > spinlock as well. But yeah that can be rectified
> > 
> > Holding a spin_lock for this long is not reasonable. We only need to
> > lock when setting link recovery request but not while polling for DSTS
> > and waiting for the link to go up.
> > 
> > BR,
> > Thinh
> > 
> > > 
> > > static int dwc3_gadget_wakeup(struct usb_gadget *g)
> > > {
> > > 	struct dwc3		*dwc = gadget_to_dwc(g);
> > > 	unsigned long		flags;
> > > 	int			ret;
> > > 
> > > 	spin_lock_irqsave(&dwc->lock, flags);
> > > 	ret = __dwc3_gadget_wakeup(dwc);
> > > 	spin_unlock_irqrestore(&dwc->lock, flags);
> > > 
> > > 	return ret;
> > > }
> > > 
> > > 
> > > > > 
> > > > > To make this generic across hosts, I had switched to interrupt based
> > > > > approach, enabling link state events and return error value immediately
> > > > > from the dwc3_gadget_wakeup() API after doing a LFPS handshake. But
> > > > > yeah, then we have to rely on the resume callback as an indication that
> > > > > link is transitioned to ON state.
> > > > > 
> > > > 
> > > > BR,
> > > > Thinh

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Elson Serrao]

On 8/25/2022 6:30 PM, Thinh Nguyen wrote:
> On Tue, Aug 23, 2022, Elson Serrao wrote:
>> On 8/22/2022 6:01 PM, Thinh Nguyen wrote:
>>> On Thu, Aug 18, 2022 at 11:17:24AM -0700, Elson Serrao wrote:
>>>>
>>>>
>>>> On 8/16/2022 4:51 PM, Thinh Nguyen wrote:
>>>>> On 8/16/2022, Elson Serrao wrote:
>>>>>>
>>>>>>
>>>>>> On 8/12/2022 5:46 PM, Thinh Nguyen wrote:
>>>>>>> On 8/11/2022, Thinh Nguyen wrote:
>>>>>>>> On 8/11/2022, Thinh Nguyen wrote:
>>>>>>>>> On 8/11/2022, Elson Serrao wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
>>>>>>>>>
>>>>>>>>> <snip>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>> To summarize the points:
>>>>>>>>>>>
>>>>>>>>>>> 1) The host only arms function remote wakeup if the device is
>>>>>>>>>>> capable of
>>>>>>>>>>> remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and
>>>>>>>>>>> hardware
>>>>>>>>>>> capability)
>>>>>>>>>>>
>>>>>>>>>>> 2) If the device is in suspend, the device can do remote wakeup
>>>>>>>>>>> (through
>>>>>>>>>>> LFPS handshake) if the function is armed for remote wakeup (through
>>>>>>>>>>> SET_FEATURE(FUNC_SUSPEND)).
>>>>>>>>>>>
>>>>>>>>>>> 3) If the link transitions to U0 after the device triggering a remote
>>>>>>>>>>> wakeup, the device will also send device notification function
>>>>>>>>>>> wake for
>>>>>>>>>>> all the interfaces armed with remote wakeup.
>>>>>>>>>>>
>>>>>>>>>>> 4) If the device is not in suspend, the device can send device
>>>>>>>>>>> notification function wake if it's in U0.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Now, remote wakeup and function wake device notification are 2
>>>>>>>>>>> separate
>>>>>>>>>>> operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
>>>>>>>>>>> suggested to maybe add
>>>>>>>>>>> usb_gadget_ops->send_wakeup_notification(gadget,
>>>>>>>>>>> intf_id) for the device notification. What you did was combining both
>>>>>>>>>>> operations in usb_gadget_ops->func_wakeup(). That may only work for
>>>>>>>>>>> point 4) (assuming you fix the U0 check), but not point 3).
>>>>>>>>>>
>>>>>>>>>> Thank you for your feedback and summary. I will rename func_wakeup to
>>>>>>>>>> send_wakeup_notification to better align with the approach. The
>>>>>>>>>> reason I
>>>>>>>>>> have combined remote_wakeup and function wake notification in
>>>>>>>>>> usb_gadget_ops->func_wakeup() is because since the implementation
>>>>>>>>>> is at
>>>>>>>>>> function/composite level it has no knowledge on the link state. So I
>>>>>>>>>> have delegated that task to controller driver to handle the
>>>>>>>>>> notification
>>>>>>>>>> accordingly. That is do a LFPS handshake first if the device is
>>>>>>>>>> suspended and then send notification (explained below). But we can
>>>>>>>>>> definitely separate this by adding an additional flag in the composite
>>>>>>>>>> layer to set the link state based on the gadget suspend callback
>>>>>>>>>> called
>>>>>>>>>> when U3 SUSPEND interrupt is received. Let me know if you feel
>>>>>>>>>> separating the two is a better approach.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> The reason I think we need to separate it is because of point 3. As I
>>>>>>>>> note earlier, the spec states that "If remote wake event occurs in
>>>>>>>>> multiple functions, each function shall send a Function Wake
>>>>>>>>> Notification."
>>>>>>>>>
>>>>>>>>> But if there's no remote wake event, and the host brought the device up
>>>>>>>>> instead, then the function suspend state is retained.
>>>>>>>>>
>>>>>>>>> If we separate these 2 operations, the caller can check whether the
>>>>>>>>> operation went through properly. For example, if the wakeup() is
>>>>>>>>> initiated properly, but the function wake device notification didn't go
>>>>>>>>> through. We would only need to resend the device notification rather
>>>>>>>>> than initiate remote wakeup again.
>>>>>>>>
>>>>>>>> If we don't have to send device notification for other interfaces, we
>>>>>>>> can combine the operations here as you did.
>>>>>>>>
>>>>>>>
>>>>>>> I still think it's better to split up the operations. The way you're
>>>>>>> handling it now is not clear.
>>>>>>>
>>>>>>> If the func_awake() returns -EAGAIN, I'd expect that the remote wake did
>>>>>>> not go through and expect user to retry again. But here it does initiate
>>>>>>> remote wake, but it just does not send device notification yet. This is
>>>>>>> confusing.
>>>>>>>
>>>>>>> Also, instead of all the function wake handling coming from the function
>>>>>>> driver, now we depend on the controller driver to call function resume()
>>>>>>> on state change to U0, which will trigger device notification. What
>>>>>>> happen if it doesn't call resume(). There's too many dependencies and it
>>>>>>> seems fragile.
>>>>>>>
>>>>>>> I think all this can be handled in the function driver. You can fix the
>>>>>>> dwc3 wakeup() and poll for U0/ON state rather than RECOVERY state, which
>>>>>>> is what it's supposed to poll.
>>>>>>
>>>>>> For transitioning from U3 to U0, the current upstream implementation is
>>>>>> to poll for U0 state when dwc3_gadget_wakeup() is called and it is a
>>>>>> blocking call. (this is a common API for both HS and SS)
>>>>>>
>>>>>>        /* 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;
>>>>>>        }
>>>>>>
>>>>>> In my experiments I found that certain hosts take longer time to drive
>>>>>> HS resume signalling between the remote wakeup and the resume K and this
>>>>>> time varies across hosts. Hence the above polling timer is not generic
>>>>>> across hosts. So how do we converge on a polling timer value to work
>>>>>> across HS/SS and without blocking the system for a long time?
>>>>>
>>>>> Can't we take the upper limit of both base on experiment? And it
>>>>> shouldn't be blocking the whole system.
>>>>
>>>> On the host I was experimenting with, the time it took was around 110ms in
>>>> HS case. That would translate to a retry count of about ~181,000 in the
>>>> above polling loop. Wouldn't that be a very large value for polling?
>>>> And not sure if there are other hosts that take even longer time
>>>
>>> We don't want to poll that many times. We shouldn't depend on the delay
>>> of a register read for polling interval. Can't we just sleep in between
>>> interval at a reasonable interval.
>>>
>>
>> Sleeping is not an option as the upper layers (those beyond
>> function/composite layer) may transmit data with a lock held.
>>
> 
> You can use mdelay() if it can't sleep. But if the wait is long, it
> should be allowed to sleep.
> 
>> I ran into below BUG when remote wakeup was triggered via a ping() command
>> and attempted sleep while polling
>>
>> [   88.676789][  T392] BUG: scheduling while atomic
>> [   88.900112][  T392] Call trace:
>> <snip>
>> [   88.912760][  T392]  __schedule_bug+0x90/0x188
>> [   88.917335][  T392]  __schedule+0x714/0xb4c
>> [   88.930568][  T392]  schedule+0x110/0x204
>> [   88.943620][  T392]  schedule_timeout+0x94/0x134
>> [   88.948371][  T392]  __dwc3_gadget_wakeup+0x1ac/0x558
>> [   88.953558][  T392]  dwc3_gadget_wakeup+0x3c/0x8c
>> [   88.958388][  T392]  usb_gadget_wakeup+0x54/0x1a8
>> [   88.963222][  T392]  eth_start_xmit+0x130/0x830
>> [   88.967876][  T392]  xmit_one+0xf0/0x364
>> [   88.971913][  T392]  sch_direct_xmit+0x188/0x3e4
>> [   88.976663][  T392]  __dev_xmit_skb+0x480/0x984
>> [   88.981319][  T392]  __dev_queue_xmit+0x2d4/0x7d8
>> [   88.986151][  T392]  neigh_resolve_output+0x208/0x2f0
>> <snip>
>>
>> The above experiment was done by removing spin_locks if any in the wakeup()
>> path of  function/composite/controller drivers. It is running in atomic
>> context due to the lock held by linux networking stack/generic packet
>> scheduler.
>>
>> So below are the only two approaches I can think of for dwc3_gadget_wakeup()
>> API
>>
>> 1.)Polling based approach: We poll until the link comes up. But cannot sleep
>> while polling due to above reasons.
>>
>> 2.)Interrupt based approach (the patches being discussed currently): When a
>> remote wakeup is triggered enable link state interrupts and return right
>> away. The function/composite drivers are later notified about the ON event
>> via resume callback (in a similar way how we notify U3 to composite layer
>> via suspend callback).
>>
>> Please let me know if there is any alternate way that you can think of here.
>>
> 
> The main issue we're trying to solve is knowing if the host had woken up
> and the device notification is sent so that the function driver can
> resume().
> 
> If we can say that upon usb_gadget_wakeup() successful completion, the
> link is in U0/ON, then the function driver can send the wake
> notification after and resume(). That is, we're trying to make
> usb_gadget_wakeup() synchronous. Whether it's polling or interrupt
> based, it's implementation detail.
> 
> Unfortunately, the API isn't very clear whether usb_gadget_wakeup() may
> sleep or synchronous.
> 
> Here are 3 approaches that we can have:
> 
> 1) Clarify that usb_gadget_wakeup() is synchronous and the link will be
> in U0/ON upon successful completion, and clarify whether it can sleep.
> Introduce a separate API usb_gadget_send_wake_notification() to send
> wake notification separately.
> 
> 2) Create a new API usb_gadget_function_wakeup() that will combine both
> device wakeup and wake notification. The function can sleep,
> synchronous, and both wakeup and wake notification are done after the
> function completes.
> 
> 3) Create a new API usb_gadget_function_wakeup() that will combine both
> device wakeup and wake notification. The function is asynchronous. We
> won't know if the wakeup is successfully sent, but we don't care and
> proceed with the function proceed with resume() anyway.
> 
> BR,
> Thinh

Thank you for your suggestions.
For handling function wakeup (applicable to enhanced super-speed) will 
implement a separate API usb_gadget_function_wakeup() that combines 
device-wakeup and wake-notification like below in dwc3 driver and 
operates synchronously.

dwc3_gadget_func_wakeup()
{
	if (link in U3)
		Call dwc3_gadget_wakeup()
		Poll for U0
		
	
	If U0 successful, send wake_notification

}

Once the function completes both device wake and func wakeup 
notification are done.

For high speed use-cases when usb_gadget_wakeup() is called from 
function drivers, considering the possibility of significant delay 
associated with remote wakeup, dwc3_gadget_wakeup() should operate 
asynchronously.
i.e rely on link status change events rather than sleeping/polling.

Please let me know if there are any concerns. If not will upload new 
patch sets with this change and other changes suggested.

Regards
Elson


> 
> 
>>>>>
>>>>>>
>>>>>> Some data layers like TCP/IP hold a TX lock while sending data (that
>>>>>> causes a remote wakeup event) and hence this wakeup() may run in atomic
>>>>>> context.
>>>>>
>>>>> Why hold the lock while waiting for the host to wakeup? The host is
>>>>> still inactive. Also, the usb_gadget_wakeup() API doesn't specify that
>>>>> it may run in atomic context.
>>>>>
>>>>
>>>> The lock might be held by upper layers who are unaware/independent of
>>>> underlying transport medium. The above TX lock I was referring to was
>>>> that held by Linux networking stack. It just pushes out data the same way it
>>>> would when USB is active. It is the function/composite layer being aware of
>>>> the function suspend would now sense this as a remote wake event and perform
>>>> this additional step of bringing out the link from u3 and then sending
>>>> device wakeup notification.
>>>>
>>>> In our current upstream implementation of dwc3_gadget_wakeup() API we hold a
>>>> spinlock as well. But yeah that can be rectified
>>>
>>> Holding a spin_lock for this long is not reasonable. We only need to
>>> lock when setting link recovery request but not while polling for DSTS
>>> and waiting for the link to go up.
>>>
>>> BR,
>>> Thinh
>>>
>>>>
>>>> static int dwc3_gadget_wakeup(struct usb_gadget *g)
>>>> {
>>>> 	struct dwc3		*dwc = gadget_to_dwc(g);
>>>> 	unsigned long		flags;
>>>> 	int			ret;
>>>>
>>>> 	spin_lock_irqsave(&dwc->lock, flags);
>>>> 	ret = __dwc3_gadget_wakeup(dwc);
>>>> 	spin_unlock_irqrestore(&dwc->lock, flags);
>>>>
>>>> 	return ret;
>>>> }
>>>>
>>>>
>>>>>>
>>>>>> To make this generic across hosts, I had switched to interrupt based
>>>>>> approach, enabling link state events and return error value immediately
>>>>>> from the dwc3_gadget_wakeup() API after doing a LFPS handshake. But
>>>>>> yeah, then we have to rely on the resume callback as an indication that
>>>>>> link is transitioned to ON state.
>>>>>>
>>>>>
>>>>> BR,
>>>>> Thinh

Re: [PATCH 2/5] usb: gadget: Add function wakeup support

[Thinh Nguyen]

On Tue, Sep 13, 2022, Elson Serrao wrote:
> 
> 
> On 8/25/2022 6:30 PM, Thinh Nguyen wrote:
> > On Tue, Aug 23, 2022, Elson Serrao wrote:
> > > On 8/22/2022 6:01 PM, Thinh Nguyen wrote:
> > > > On Thu, Aug 18, 2022 at 11:17:24AM -0700, Elson Serrao wrote:
> > > > > 
> > > > > 
> > > > > On 8/16/2022 4:51 PM, Thinh Nguyen wrote:
> > > > > > On 8/16/2022, Elson Serrao wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 8/12/2022 5:46 PM, Thinh Nguyen wrote:
> > > > > > > > On 8/11/2022, Thinh Nguyen wrote:
> > > > > > > > > On 8/11/2022, Thinh Nguyen wrote:
> > > > > > > > > > On 8/11/2022, Elson Serrao wrote:
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > On 8/9/2022 6:08 PM, Thinh Nguyen wrote:
> > > > > > > > > > 
> > > > > > > > > > <snip>
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > > > To summarize the points:
> > > > > > > > > > > > 
> > > > > > > > > > > > 1) The host only arms function remote wakeup if the device is
> > > > > > > > > > > > capable of
> > > > > > > > > > > > remote wakeup (check USB_CONFIG_ATT_WAKEUP in bmAttributes and
> > > > > > > > > > > > hardware
> > > > > > > > > > > > capability)
> > > > > > > > > > > > 
> > > > > > > > > > > > 2) If the device is in suspend, the device can do remote wakeup
> > > > > > > > > > > > (through
> > > > > > > > > > > > LFPS handshake) if the function is armed for remote wakeup (through
> > > > > > > > > > > > SET_FEATURE(FUNC_SUSPEND)).
> > > > > > > > > > > > 
> > > > > > > > > > > > 3) If the link transitions to U0 after the device triggering a remote
> > > > > > > > > > > > wakeup, the device will also send device notification function
> > > > > > > > > > > > wake for
> > > > > > > > > > > > all the interfaces armed with remote wakeup.
> > > > > > > > > > > > 
> > > > > > > > > > > > 4) If the device is not in suspend, the device can send device
> > > > > > > > > > > > notification function wake if it's in U0.
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > Now, remote wakeup and function wake device notification are 2
> > > > > > > > > > > > separate
> > > > > > > > > > > > operations. We have the usb_gadget_ops->wakeup() for remote wakeup. I
> > > > > > > > > > > > suggested to maybe add
> > > > > > > > > > > > usb_gadget_ops->send_wakeup_notification(gadget,
> > > > > > > > > > > > intf_id) for the device notification. What you did was combining both
> > > > > > > > > > > > operations in usb_gadget_ops->func_wakeup(). That may only work for
> > > > > > > > > > > > point 4) (assuming you fix the U0 check), but not point 3).
> > > > > > > > > > > 
> > > > > > > > > > > Thank you for your feedback and summary. I will rename func_wakeup to
> > > > > > > > > > > send_wakeup_notification to better align with the approach. The
> > > > > > > > > > > reason I
> > > > > > > > > > > have combined remote_wakeup and function wake notification in
> > > > > > > > > > > usb_gadget_ops->func_wakeup() is because since the implementation
> > > > > > > > > > > is at
> > > > > > > > > > > function/composite level it has no knowledge on the link state. So I
> > > > > > > > > > > have delegated that task to controller driver to handle the
> > > > > > > > > > > notification
> > > > > > > > > > > accordingly. That is do a LFPS handshake first if the device is
> > > > > > > > > > > suspended and then send notification (explained below). But we can
> > > > > > > > > > > definitely separate this by adding an additional flag in the composite
> > > > > > > > > > > layer to set the link state based on the gadget suspend callback
> > > > > > > > > > > called
> > > > > > > > > > > when U3 SUSPEND interrupt is received. Let me know if you feel
> > > > > > > > > > > separating the two is a better approach.
> > > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > The reason I think we need to separate it is because of point 3. As I
> > > > > > > > > > note earlier, the spec states that "If remote wake event occurs in
> > > > > > > > > > multiple functions, each function shall send a Function Wake
> > > > > > > > > > Notification."
> > > > > > > > > > 
> > > > > > > > > > But if there's no remote wake event, and the host brought the device up
> > > > > > > > > > instead, then the function suspend state is retained.
> > > > > > > > > > 
> > > > > > > > > > If we separate these 2 operations, the caller can check whether the
> > > > > > > > > > operation went through properly. For example, if the wakeup() is
> > > > > > > > > > initiated properly, but the function wake device notification didn't go
> > > > > > > > > > through. We would only need to resend the device notification rather
> > > > > > > > > > than initiate remote wakeup again.
> > > > > > > > > 
> > > > > > > > > If we don't have to send device notification for other interfaces, we
> > > > > > > > > can combine the operations here as you did.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > I still think it's better to split up the operations. The way you're
> > > > > > > > handling it now is not clear.
> > > > > > > > 
> > > > > > > > If the func_awake() returns -EAGAIN, I'd expect that the remote wake did
> > > > > > > > not go through and expect user to retry again. But here it does initiate
> > > > > > > > remote wake, but it just does not send device notification yet. This is
> > > > > > > > confusing.
> > > > > > > > 
> > > > > > > > Also, instead of all the function wake handling coming from the function
> > > > > > > > driver, now we depend on the controller driver to call function resume()
> > > > > > > > on state change to U0, which will trigger device notification. What
> > > > > > > > happen if it doesn't call resume(). There's too many dependencies and it
> > > > > > > > seems fragile.
> > > > > > > > 
> > > > > > > > I think all this can be handled in the function driver. You can fix the
> > > > > > > > dwc3 wakeup() and poll for U0/ON state rather than RECOVERY state, which
> > > > > > > > is what it's supposed to poll.
> > > > > > > 
> > > > > > > For transitioning from U3 to U0, the current upstream implementation is
> > > > > > > to poll for U0 state when dwc3_gadget_wakeup() is called and it is a
> > > > > > > blocking call. (this is a common API for both HS and SS)
> > > > > > > 
> > > > > > >        /* 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;
> > > > > > >        }
> > > > > > > 
> > > > > > > In my experiments I found that certain hosts take longer time to drive
> > > > > > > HS resume signalling between the remote wakeup and the resume K and this
> > > > > > > time varies across hosts. Hence the above polling timer is not generic
> > > > > > > across hosts. So how do we converge on a polling timer value to work
> > > > > > > across HS/SS and without blocking the system for a long time?
> > > > > > 
> > > > > > Can't we take the upper limit of both base on experiment? And it
> > > > > > shouldn't be blocking the whole system.
> > > > > 
> > > > > On the host I was experimenting with, the time it took was around 110ms in
> > > > > HS case. That would translate to a retry count of about ~181,000 in the
> > > > > above polling loop. Wouldn't that be a very large value for polling?
> > > > > And not sure if there are other hosts that take even longer time
> > > > 
> > > > We don't want to poll that many times. We shouldn't depend on the delay
> > > > of a register read for polling interval. Can't we just sleep in between
> > > > interval at a reasonable interval.
> > > > 
> > > 
> > > Sleeping is not an option as the upper layers (those beyond
> > > function/composite layer) may transmit data with a lock held.
> > > 
> > 
> > You can use mdelay() if it can't sleep. But if the wait is long, it
> > should be allowed to sleep.
> > 
> > > I ran into below BUG when remote wakeup was triggered via a ping() command
> > > and attempted sleep while polling
> > > 
> > > [   88.676789][  T392] BUG: scheduling while atomic
> > > [   88.900112][  T392] Call trace:
> > > <snip>
> > > [   88.912760][  T392]  __schedule_bug+0x90/0x188
> > > [   88.917335][  T392]  __schedule+0x714/0xb4c
> > > [   88.930568][  T392]  schedule+0x110/0x204
> > > [   88.943620][  T392]  schedule_timeout+0x94/0x134
> > > [   88.948371][  T392]  __dwc3_gadget_wakeup+0x1ac/0x558
> > > [   88.953558][  T392]  dwc3_gadget_wakeup+0x3c/0x8c
> > > [   88.958388][  T392]  usb_gadget_wakeup+0x54/0x1a8
> > > [   88.963222][  T392]  eth_start_xmit+0x130/0x830
> > > [   88.967876][  T392]  xmit_one+0xf0/0x364
> > > [   88.971913][  T392]  sch_direct_xmit+0x188/0x3e4
> > > [   88.976663][  T392]  __dev_xmit_skb+0x480/0x984
> > > [   88.981319][  T392]  __dev_queue_xmit+0x2d4/0x7d8
> > > [   88.986151][  T392]  neigh_resolve_output+0x208/0x2f0
> > > <snip>
> > > 
> > > The above experiment was done by removing spin_locks if any in the wakeup()
> > > path of  function/composite/controller drivers. It is running in atomic
> > > context due to the lock held by linux networking stack/generic packet
> > > scheduler.
> > > 
> > > So below are the only two approaches I can think of for dwc3_gadget_wakeup()
> > > API
> > > 
> > > 1.)Polling based approach: We poll until the link comes up. But cannot sleep
> > > while polling due to above reasons.
> > > 
> > > 2.)Interrupt based approach (the patches being discussed currently): When a
> > > remote wakeup is triggered enable link state interrupts and return right
> > > away. The function/composite drivers are later notified about the ON event
> > > via resume callback (in a similar way how we notify U3 to composite layer
> > > via suspend callback).
> > > 
> > > Please let me know if there is any alternate way that you can think of here.
> > > 
> > 
> > The main issue we're trying to solve is knowing if the host had woken up
> > and the device notification is sent so that the function driver can
> > resume().
> > 
> > If we can say that upon usb_gadget_wakeup() successful completion, the
> > link is in U0/ON, then the function driver can send the wake
> > notification after and resume(). That is, we're trying to make
> > usb_gadget_wakeup() synchronous. Whether it's polling or interrupt
> > based, it's implementation detail.
> > 
> > Unfortunately, the API isn't very clear whether usb_gadget_wakeup() may
> > sleep or synchronous.
> > 
> > Here are 3 approaches that we can have:
> > 
> > 1) Clarify that usb_gadget_wakeup() is synchronous and the link will be
> > in U0/ON upon successful completion, and clarify whether it can sleep.
> > Introduce a separate API usb_gadget_send_wake_notification() to send
> > wake notification separately.
> > 
> > 2) Create a new API usb_gadget_function_wakeup() that will combine both
> > device wakeup and wake notification. The function can sleep,
> > synchronous, and both wakeup and wake notification are done after the
> > function completes.
> > 
> > 3) Create a new API usb_gadget_function_wakeup() that will combine both
> > device wakeup and wake notification. The function is asynchronous. We
> > won't know if the wakeup is successfully sent, but we don't care and
> > proceed with the function proceed with resume() anyway.
> > 
> > BR,
> > Thinh
> 
> Thank you for your suggestions.
> For handling function wakeup (applicable to enhanced super-speed) will
> implement a separate API usb_gadget_function_wakeup() that combines
> device-wakeup and wake-notification like below in dwc3 driver and operates
> synchronously.
> 
> dwc3_gadget_func_wakeup()
> {
> 	if (link in U3)
> 		Call dwc3_gadget_wakeup()
> 		Poll for U0
> 		
> 	
> 	If U0 successful, send wake_notification
> 
> }
> 
> Once the function completes both device wake and func wakeup notification
> are done.
> 
> For high speed use-cases when usb_gadget_wakeup() is called from function
> drivers, considering the possibility of significant delay associated with
> remote wakeup, dwc3_gadget_wakeup() should operate asynchronously.
> i.e rely on link status change events rather than sleeping/polling.
> 
> Please let me know if there are any concerns. If not will upload new patch
> sets with this change and other changes suggested.
> 

That sounds good to me.

Thanks,
Thinh