* [PATCH v12 1/6] usb: gadget: Properly configure the device for remote wakeup
2023-03-16 23:38 [PATCH v12 0/6] Add function suspend/resume and remote wakeup support Elson Roy Serrao
@ 2023-03-16 23:38 ` Elson Roy Serrao
2023-03-16 23:38 ` [PATCH v12 2/6] usb: dwc3: Add remote wakeup handling Elson Roy Serrao
` (4 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Elson Roy Serrao @ 2023-03-16 23:38 UTC (permalink / raw)
To: gregkh, Thinh.Nguyen
Cc: linux-kernel, linux-usb, quic_wcheng, quic_jackp, Elson Roy Serrao
The wakeup bit in the bmAttributes field indicates whether the device
is configured for remote wakeup. But this field should be allowed to
set only if the UDC supports such wakeup mechanism. So configure this
field based on UDC capability. Also inform the UDC whether the device
is configured for remote wakeup by implementing a gadget op.
Reviewed-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
drivers/usb/gadget/composite.c | 18 ++++++++++++++++++
drivers/usb/gadget/configfs.c | 3 +++
drivers/usb/gadget/udc/core.c | 27 +++++++++++++++++++++++++++
drivers/usb/gadget/udc/trace.h | 5 +++++
include/linux/usb/composite.h | 2 ++
include/linux/usb/gadget.h | 8 ++++++++
6 files changed, 63 insertions(+)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 36add18..c9c983e 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -513,6 +513,19 @@ static u8 encode_bMaxPower(enum usb_device_speed speed,
return min(val, 900U) / 8;
}
+void check_remote_wakeup_config(struct usb_gadget *g,
+ struct usb_configuration *c)
+{
+ if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes) {
+ /* Reset the rw bit if gadget is not capable of it */
+ if (!g->wakeup_capable && g->ops->set_remote_wakeup) {
+ WARN(c->cdev, "Clearing wakeup bit for config c.%d\n",
+ c->bConfigurationValue);
+ c->bmAttributes &= ~USB_CONFIG_ATT_WAKEUP;
+ }
+ }
+}
+
static int config_buf(struct usb_configuration *config,
enum usb_device_speed speed, void *buf, u8 type)
{
@@ -994,6 +1007,11 @@ static int set_config(struct usb_composite_dev *cdev,
power = min(power, 500U);
else
power = min(power, 900U);
+
+ if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes)
+ usb_gadget_set_remote_wakeup(gadget, 1);
+ else
+ usb_gadget_set_remote_wakeup(gadget, 0);
done:
if (power <= USB_SELF_POWER_VBUS_MAX_DRAW)
usb_gadget_set_selfpowered(gadget);
diff --git a/drivers/usb/gadget/configfs.c b/drivers/usb/gadget/configfs.c
index b9f1136..4c639e9 100644
--- a/drivers/usb/gadget/configfs.c
+++ b/drivers/usb/gadget/configfs.c
@@ -1761,6 +1761,9 @@ static int configfs_composite_bind(struct usb_gadget *gadget,
if (gadget_is_otg(gadget))
c->descriptors = otg_desc;
+ /* Properly configure the bmAttributes wakeup bit */
+ check_remote_wakeup_config(gadget, c);
+
cfg = container_of(c, struct config_usb_cfg, c);
if (!list_empty(&cfg->string_list)) {
i = 0;
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 23b0629..3dcbba7 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -514,6 +514,33 @@ int usb_gadget_wakeup(struct usb_gadget *gadget)
EXPORT_SYMBOL_GPL(usb_gadget_wakeup);
/**
+ * usb_gadget_set_remote_wakeup - configures the device remote wakeup feature.
+ * @gadget:the device being configured for remote wakeup
+ * @set:value to be configured.
+ *
+ * set to one to enable remote wakeup feature and zero to disable it.
+ *
+ * returns zero on success, else negative errno.
+ */
+int usb_gadget_set_remote_wakeup(struct usb_gadget *gadget, int set)
+{
+ int ret = 0;
+
+ if (!gadget->ops->set_remote_wakeup) {
+ ret = -EOPNOTSUPP;
+ goto out;
+ }
+
+ ret = gadget->ops->set_remote_wakeup(gadget, set);
+
+out:
+ trace_usb_gadget_set_remote_wakeup(gadget, ret);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(usb_gadget_set_remote_wakeup);
+
+/**
* usb_gadget_set_selfpowered - sets the device selfpowered feature.
* @gadget:the device being declared as self-powered
*
diff --git a/drivers/usb/gadget/udc/trace.h b/drivers/usb/gadget/udc/trace.h
index abdbcb1..a5ed26f 100644
--- a/drivers/usb/gadget/udc/trace.h
+++ b/drivers/usb/gadget/udc/trace.h
@@ -91,6 +91,11 @@ DEFINE_EVENT(udc_log_gadget, usb_gadget_wakeup,
TP_ARGS(g, ret)
);
+DEFINE_EVENT(udc_log_gadget, usb_gadget_set_remote_wakeup,
+ TP_PROTO(struct usb_gadget *g, int ret),
+ TP_ARGS(g, ret)
+);
+
DEFINE_EVENT(udc_log_gadget, usb_gadget_set_selfpowered,
TP_PROTO(struct usb_gadget *g, int ret),
TP_ARGS(g, ret)
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index 608dc96..d949e91 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -413,6 +413,8 @@ extern int composite_dev_prepare(struct usb_composite_driver *composite,
extern int composite_os_desc_req_prepare(struct usb_composite_dev *cdev,
struct usb_ep *ep0);
void composite_dev_cleanup(struct usb_composite_dev *cdev);
+void check_remote_wakeup_config(struct usb_gadget *g,
+ struct usb_configuration *c);
static inline struct usb_composite_driver *to_cdriver(
struct usb_gadget_driver *gdrv)
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 00750f7..1d79612 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -310,6 +310,7 @@ struct usb_udc;
struct usb_gadget_ops {
int (*get_frame)(struct usb_gadget *);
int (*wakeup)(struct usb_gadget *);
+ int (*set_remote_wakeup)(struct usb_gadget *, int set);
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);
@@ -384,6 +385,8 @@ struct usb_gadget_ops {
* @connected: True if gadget is connected.
* @lpm_capable: If the gadget max_speed is FULL or HIGH, this flag
* indicates that it supports LPM as per the LPM ECN & errata.
+ * @wakeup_capable: True if gadget is capable of sending remote wakeup.
+ * @wakeup_armed: True if gadget is armed by the host for remote wakeup.
* @irq: the interrupt number for device controller.
* @id_number: a unique ID number for ensuring that gadget names are distinct
*
@@ -445,6 +448,8 @@ struct usb_gadget {
unsigned deactivated:1;
unsigned connected:1;
unsigned lpm_capable:1;
+ unsigned wakeup_capable:1;
+ unsigned wakeup_armed:1;
int irq;
int id_number;
};
@@ -601,6 +606,7 @@ static inline int gadget_is_otg(struct usb_gadget *g)
#if IS_ENABLED(CONFIG_USB_GADGET)
int usb_gadget_frame_number(struct usb_gadget *gadget);
int usb_gadget_wakeup(struct usb_gadget *gadget);
+int usb_gadget_set_remote_wakeup(struct usb_gadget *gadget, int set);
int usb_gadget_set_selfpowered(struct usb_gadget *gadget);
int usb_gadget_clear_selfpowered(struct usb_gadget *gadget);
int usb_gadget_vbus_connect(struct usb_gadget *gadget);
@@ -616,6 +622,8 @@ static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
{ return 0; }
static inline int usb_gadget_wakeup(struct usb_gadget *gadget)
{ return 0; }
+static inline int usb_gadget_set_remote_wakeup(struct usb_gadget *gadget, int set)
+{ return 0; }
static inline int usb_gadget_set_selfpowered(struct usb_gadget *gadget)
{ return 0; }
static inline int usb_gadget_clear_selfpowered(struct usb_gadget *gadget)
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v12 2/6] usb: dwc3: Add remote wakeup handling
2023-03-16 23:38 [PATCH v12 0/6] Add function suspend/resume and remote wakeup support Elson Roy Serrao
2023-03-16 23:38 ` [PATCH v12 1/6] usb: gadget: Properly configure the device for remote wakeup Elson Roy Serrao
@ 2023-03-16 23:38 ` Elson Roy Serrao
2023-03-16 23:38 ` [PATCH v12 3/6] usb: gadget: Add function wakeup support Elson Roy Serrao
` (3 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Elson Roy Serrao @ 2023-03-16 23:38 UTC (permalink / raw)
To: gregkh, Thinh.Nguyen
Cc: linux-kernel, linux-usb, quic_wcheng, quic_jackp, Elson Roy Serrao
An usb device can initate a remote wakeup and bring the link out of
suspend as dictated by the DEVICE_REMOTE_WAKEUP feature selector.
Add support to handle this packet and set the remote wakeup capability.
Some hosts may take longer time to initiate the resume signaling after
device triggers a remote wakeup. So add async support to the wakeup API
by enabling link status change events.
Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
drivers/usb/dwc3/core.h | 2 ++
drivers/usb/dwc3/ep0.c | 7 +++++
drivers/usb/dwc3/gadget.c | 76 +++++++++++++++++++++++++++++++++++++++++++----
3 files changed, 79 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index b1bd631..416e0ef 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1113,6 +1113,7 @@ struct dwc3_scratchpad_array {
* 3 - Reserved
* @dis_metastability_quirk: set to disable metastability quirk.
* @dis_split_quirk: set to disable split boundary.
+ * @wakeup_configured: set if the device is configured for remote wakeup.
* @imod_interval: set the interrupt moderation interval in 250ns
* increments or 0 to disable.
* @max_cfg_eps: current max number of IN eps used across all USB configs.
@@ -1331,6 +1332,7 @@ struct dwc3 {
unsigned dis_split_quirk:1;
unsigned async_callbacks:1;
+ unsigned wakeup_configured:1;
u16 imod_interval;
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 61de693..8aa14a5 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -356,6 +356,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->gadget->wakeup_armed <<
+ USB_DEVICE_REMOTE_WAKEUP;
}
break;
@@ -476,6 +479,10 @@ static int dwc3_ep0_handle_device(struct dwc3 *dwc,
switch (wValue) {
case USB_DEVICE_REMOTE_WAKEUP:
+ if (dwc->wakeup_configured)
+ dwc->gadget->wakeup_armed = set;
+ else
+ ret = -EINVAL;
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 a1ebb30..c322c70 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -258,7 +258,7 @@ int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned int cmd,
return ret;
}
-static int __dwc3_gadget_wakeup(struct dwc3 *dwc);
+static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async);
/**
* dwc3_send_gadget_ep_cmd - issue an endpoint command
@@ -325,7 +325,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned int cmd,
fallthrough;
case DWC3_LINK_STATE_U3:
- ret = __dwc3_gadget_wakeup(dwc);
+ ret = __dwc3_gadget_wakeup(dwc, false);
dev_WARN_ONCE(dwc->dev, ret, "wakeup failed --> %d\n",
ret);
break;
@@ -2269,6 +2269,22 @@ static const struct usb_ep_ops dwc3_gadget_ep_ops = {
/* -------------------------------------------------------------------------- */
+static void dwc3_gadget_enable_linksts_evts(struct dwc3 *dwc, bool set)
+{
+ u32 reg;
+
+ if (DWC3_VER_IS_PRIOR(DWC3, 250A))
+ return;
+
+ reg = dwc3_readl(dwc->regs, DWC3_DEVTEN);
+ if (set)
+ reg |= DWC3_DEVTEN_ULSTCNGEN;
+ else
+ reg &= ~DWC3_DEVTEN_ULSTCNGEN;
+
+ dwc3_writel(dwc->regs, DWC3_DEVTEN, reg);
+}
+
static int dwc3_gadget_get_frame(struct usb_gadget *g)
{
struct dwc3 *dwc = gadget_to_dwc(g);
@@ -2276,7 +2292,7 @@ static int dwc3_gadget_get_frame(struct usb_gadget *g)
return __dwc3_gadget_get_frame(dwc);
}
-static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
+static int __dwc3_gadget_wakeup(struct dwc3 *dwc, bool async)
{
int retries;
@@ -2307,9 +2323,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
return -EINVAL;
}
+ if (async)
+ dwc3_gadget_enable_linksts_evts(dwc, true);
+
ret = dwc3_gadget_set_link_state(dwc, DWC3_LINK_STATE_RECOV);
if (ret < 0) {
dev_err(dwc->dev, "failed to put link in Recovery\n");
+ dwc3_gadget_enable_linksts_evts(dwc, false);
return ret;
}
@@ -2321,6 +2341,13 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc)
dwc3_writel(dwc->regs, DWC3_DCTL, reg);
}
+ /*
+ * Since link status change events are enabled we will receive
+ * an U0 event when wakeup is successful. So bail out.
+ */
+ if (async)
+ return 0;
+
/* poll until Link State changes to ON */
retries = 20000;
@@ -2346,13 +2373,36 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
unsigned long flags;
int ret;
+ if (!dwc->wakeup_configured) {
+ dev_err(dwc->dev, "remote wakeup not configured\n");
+ return -EINVAL;
+ }
+
spin_lock_irqsave(&dwc->lock, flags);
- ret = __dwc3_gadget_wakeup(dwc);
+ if (!dwc->gadget->wakeup_armed) {
+ dev_err(dwc->dev, "not armed for remote wakeup\n");
+ spin_unlock_irqrestore(&dwc->lock, flags);
+ return -EINVAL;
+ }
+ ret = __dwc3_gadget_wakeup(dwc, true);
+
spin_unlock_irqrestore(&dwc->lock, flags);
return ret;
}
+static int dwc3_gadget_set_remote_wakeup(struct usb_gadget *g, int set)
+{
+ struct dwc3 *dwc = gadget_to_dwc(g);
+ unsigned long flags;
+
+ spin_lock_irqsave(&dwc->lock, flags);
+ dwc->wakeup_configured = !!set;
+ spin_unlock_irqrestore(&dwc->lock, flags);
+
+ return 0;
+}
+
static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
int is_selfpowered)
{
@@ -2978,6 +3028,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,
+ .set_remote_wakeup = dwc3_gadget_set_remote_wakeup,
.set_selfpowered = dwc3_gadget_set_selfpowered,
.pullup = dwc3_gadget_pullup,
.udc_start = dwc3_gadget_start,
@@ -3819,6 +3870,8 @@ static void dwc3_gadget_disconnect_interrupt(struct dwc3 *dwc)
dwc->gadget->speed = USB_SPEED_UNKNOWN;
dwc->setup_packet_pending = false;
+ dwc->gadget->wakeup_armed = false;
+ dwc3_gadget_enable_linksts_evts(dwc, false);
usb_gadget_set_state(dwc->gadget, USB_STATE_NOTATTACHED);
if (dwc->ep0state != EP0_SETUP_PHASE) {
@@ -3912,6 +3965,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
reg &= ~DWC3_DCTL_TSTCTRL_MASK;
dwc3_gadget_dctl_write_safe(dwc, reg);
dwc->test_mode = false;
+ dwc->gadget->wakeup_armed = false;
+ dwc3_gadget_enable_linksts_evts(dwc, false);
dwc3_clear_stall_all_ep(dwc);
/* Reset device address to zero */
@@ -4064,7 +4119,7 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
*/
}
-static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
+static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc, unsigned int evtinfo)
{
/*
* TODO take core out of low power mode when that's
@@ -4076,6 +4131,8 @@ static void dwc3_gadget_wakeup_interrupt(struct dwc3 *dwc)
dwc->gadget_driver->resume(dwc->gadget);
spin_lock(&dwc->lock);
}
+
+ dwc->link_state = evtinfo & DWC3_LINK_STATE_MASK;
}
static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
@@ -4157,6 +4214,12 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
}
switch (next) {
+ case DWC3_LINK_STATE_U0:
+ if (dwc->gadget->wakeup_armed) {
+ dwc3_gadget_enable_linksts_evts(dwc, false);
+ dwc3_resume_gadget(dwc);
+ }
+ break;
case DWC3_LINK_STATE_U1:
if (dwc->speed == USB_SPEED_SUPER)
dwc3_suspend_gadget(dwc);
@@ -4225,7 +4288,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,
@@ -4473,6 +4536,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
dwc->gadget->sg_supported = true;
dwc->gadget->name = "dwc3-gadget";
dwc->gadget->lpm_capable = !dwc->usb2_gadget_lpm_disable;
+ dwc->gadget->wakeup_capable = true;
/*
* FIXME We might be setting max_speed to <SUPER, however versions
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v12 3/6] usb: gadget: Add function wakeup support
2023-03-16 23:38 [PATCH v12 0/6] Add function suspend/resume and remote wakeup support Elson Roy Serrao
2023-03-16 23:38 ` [PATCH v12 1/6] usb: gadget: Properly configure the device for remote wakeup Elson Roy Serrao
2023-03-16 23:38 ` [PATCH v12 2/6] usb: dwc3: Add remote wakeup handling Elson Roy Serrao
@ 2023-03-16 23:38 ` Elson Roy Serrao
2023-03-16 23:38 ` [PATCH v12 4/6] usb: dwc3: Add function suspend and " Elson Roy Serrao
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Elson Roy Serrao @ 2023-03-16 23:38 UTC (permalink / raw)
To: gregkh, Thinh.Nguyen
Cc: linux-kernel, linux-usb, quic_wcheng, quic_jackp, Elson Roy Serrao
USB3.2 spec section 9.2.5.4 quotes that 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. Add an api in composite layer that can be used
by the function drivers to support this feature. Also expose
a gadget op so that composite layer can trigger a wakeup request
to the UDC driver.
Reviewed-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
drivers/usb/gadget/composite.c | 40 ++++++++++++++++++++++++++++++++++++++++
include/linux/usb/composite.h | 6 ++++++
include/linux/usb/gadget.h | 1 +
3 files changed, 47 insertions(+)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index c9c983e..2111732 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -492,6 +492,46 @@ int usb_interface_id(struct usb_configuration *config,
}
EXPORT_SYMBOL_GPL(usb_interface_id);
+/**
+ * usb_func_wakeup - sends function wake notification to the host.
+ * @func: function that sends the remote wakeup notification.
+ *
+ * Applicable to devices operating at enhanced superspeed when usb
+ * functions are put in function suspend state and armed for function
+ * remote wakeup. On completion, function wake notification is sent. If
+ * the device is in low power state it tries to bring the device to active
+ * state before sending the wake notification. Since it is a synchronous
+ * call, caller must take care of not calling it in interrupt context.
+ * For devices operating at lower speeds returns negative errno.
+ *
+ * Returns zero on success, else negative errno.
+ */
+int usb_func_wakeup(struct usb_function *func)
+{
+ struct usb_gadget *gadget = func->config->cdev->gadget;
+ int id;
+
+ if (!gadget->ops->func_wakeup)
+ return -EOPNOTSUPP;
+
+ if (!func->func_wakeup_armed) {
+ ERROR(func->config->cdev, "not armed for func remote wakeup\n");
+ return -EINVAL;
+ }
+
+ 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\n");
+ return -EINVAL;
+ }
+
+ return gadget->ops->func_wakeup(gadget, id);
+}
+EXPORT_SYMBOL_GPL(usb_func_wakeup);
+
static u8 encode_bMaxPower(enum usb_device_speed speed,
struct usb_configuration *c)
{
diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
index d949e91..a2448e9 100644
--- a/include/linux/usb/composite.h
+++ b/include/linux/usb/composite.h
@@ -150,6 +150,9 @@ struct usb_os_desc_table {
* GetStatus() request when the recipient is Interface.
* @func_suspend: callback to be called when
* SetFeature(FUNCTION_SUSPEND) is reseived
+ * @func_suspended: Indicates whether the function is in function suspend state.
+ * @func_wakeup_armed: Indicates whether the function is armed by the host for
+ * wakeup signaling.
*
* A single USB function uses one or more interfaces, and should in most
* cases support operation at both full and high speeds. Each function is
@@ -220,6 +223,8 @@ struct usb_function {
int (*get_status)(struct usb_function *);
int (*func_suspend)(struct usb_function *,
u8 suspend_opt);
+ bool func_suspended;
+ bool func_wakeup_armed;
/* private: */
/* internals */
struct list_head list;
@@ -241,6 +246,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 1d79612..75bda078 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -310,6 +310,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 intf_id);
int (*set_remote_wakeup)(struct usb_gadget *, int set);
int (*set_selfpowered) (struct usb_gadget *, int is_selfpowered);
int (*vbus_session) (struct usb_gadget *, int is_active);
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v12 4/6] usb: dwc3: Add function suspend and function wakeup support
2023-03-16 23:38 [PATCH v12 0/6] Add function suspend/resume and remote wakeup support Elson Roy Serrao
` (2 preceding siblings ...)
2023-03-16 23:38 ` [PATCH v12 3/6] usb: gadget: Add function wakeup support Elson Roy Serrao
@ 2023-03-16 23:38 ` Elson Roy Serrao
2023-03-16 23:38 ` [PATCH v12 5/6] usb: gadget: arm the function for triggering remote wakeup Elson Roy Serrao
2023-03-16 23:38 ` [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support Elson Roy Serrao
5 siblings, 0 replies; 19+ messages in thread
From: Elson Roy Serrao @ 2023-03-16 23:38 UTC (permalink / raw)
To: gregkh, Thinh.Nguyen
Cc: linux-kernel, linux-usb, quic_wcheng, quic_jackp, Elson Roy Serrao
USB host sends function suspend and function resume notifications to
the interface through SET_FEATURE/CLEAR_FEATURE setup packets.
Add support to handle these packets by delegating the requests to
composite layer. Also add support to handle function wake notification
requests to exit from function suspend state.
Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
drivers/usb/dwc3/core.h | 3 +++
drivers/usb/dwc3/debug.h | 2 ++
drivers/usb/dwc3/ep0.c | 12 ++++--------
drivers/usb/dwc3/gadget.c | 42 ++++++++++++++++++++++++++++++++++++++++++
4 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 416e0ef..3412ebb 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -527,6 +527,7 @@
#define DWC3_DGCMD_SET_ENDPOINT_NRDY 0x0c
#define DWC3_DGCMD_SET_ENDPOINT_PRIME 0x0d
#define DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK 0x10
+#define DWC3_DGCMD_DEV_NOTIFICATION 0x07
#define DWC3_DGCMD_STATUS(n) (((n) >> 12) & 0x0F)
#define DWC3_DGCMD_CMDACT BIT(10)
@@ -539,6 +540,8 @@
#define DWC3_DGCMDPAR_TX_FIFO BIT(5)
#define DWC3_DGCMDPAR_LOOPBACK_DIS (0 << 0)
#define DWC3_DGCMDPAR_LOOPBACK_ENA BIT(0)
+#define DWC3_DGCMDPAR_DN_FUNC_WAKE BIT(0)
+#define DWC3_DGCMDPAR_INTF_SEL(n) ((n) << 4)
/* Device Endpoint Command Register */
#define DWC3_DEPCMD_PARAM_SHIFT 16
diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
index 8bb2c9e..09d7038 100644
--- a/drivers/usb/dwc3/debug.h
+++ b/drivers/usb/dwc3/debug.h
@@ -72,6 +72,8 @@ dwc3_gadget_generic_cmd_string(u8 cmd)
return "Set Endpoint Prime";
case DWC3_DGCMD_RUN_SOC_BUS_LOOPBACK:
return "Run SoC Bus Loopback Test";
+ case DWC3_DGCMD_DEV_NOTIFICATION:
+ return "Device Notification";
default:
return "UNKNOWN";
}
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 8aa14a5..953b752 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)
@@ -368,7 +370,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);
@@ -517,13 +519,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 c322c70..9c3d619 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2391,6 +2391,47 @@ static int dwc3_gadget_wakeup(struct usb_gadget *g)
return ret;
}
+static void dwc3_resume_gadget(struct dwc3 *dwc);
+
+static int dwc3_gadget_func_wakeup(struct usb_gadget *g, int intf_id)
+{
+ struct dwc3 *dwc = gadget_to_dwc(g);
+ unsigned long flags;
+ int ret;
+ int link_state;
+
+ if (!dwc->wakeup_configured) {
+ dev_err(dwc->dev, "remote wakeup not configured\n");
+ return -EINVAL;
+ }
+
+ spin_lock_irqsave(&dwc->lock, flags);
+ /*
+ * If the link is in U3, signal for remote wakeup and wait for the
+ * link to transition to U0 before sending device notification.
+ */
+ link_state = dwc3_gadget_get_link_state(dwc);
+ if (link_state == DWC3_LINK_STATE_U3) {
+ ret = __dwc3_gadget_wakeup(dwc, false);
+ if (ret) {
+ spin_unlock_irqrestore(&dwc->lock, flags);
+ return -EINVAL;
+ }
+ dwc3_resume_gadget(dwc);
+ dwc->link_state = DWC3_LINK_STATE_U0;
+ }
+
+ ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
+ DWC3_DGCMDPAR_DN_FUNC_WAKE |
+ DWC3_DGCMDPAR_INTF_SEL(intf_id));
+ if (ret)
+ dev_err(dwc->dev, "function remote wakeup failed, ret:%d\n", ret);
+
+ spin_unlock_irqrestore(&dwc->lock, flags);
+
+ return ret;
+}
+
static int dwc3_gadget_set_remote_wakeup(struct usb_gadget *g, int set)
{
struct dwc3 *dwc = gadget_to_dwc(g);
@@ -3028,6 +3069,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_remote_wakeup = dwc3_gadget_set_remote_wakeup,
.set_selfpowered = dwc3_gadget_set_selfpowered,
.pullup = dwc3_gadget_pullup,
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v12 5/6] usb: gadget: arm the function for triggering remote wakeup
2023-03-16 23:38 [PATCH v12 0/6] Add function suspend/resume and remote wakeup support Elson Roy Serrao
` (3 preceding siblings ...)
2023-03-16 23:38 ` [PATCH v12 4/6] usb: dwc3: Add function suspend and " Elson Roy Serrao
@ 2023-03-16 23:38 ` Elson Roy Serrao
2023-03-16 23:38 ` [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support Elson Roy Serrao
5 siblings, 0 replies; 19+ messages in thread
From: Elson Roy Serrao @ 2023-03-16 23:38 UTC (permalink / raw)
To: gregkh, Thinh.Nguyen
Cc: linux-kernel, linux-usb, quic_wcheng, quic_jackp, Elson Roy Serrao
When host sends function suspend feature selector to the device,
arm the function for remote wakeup based on the received packet.
Also host queries the function wakeup capability through a get
status request. Handle such requests in composite layer for cases
where function driver has not exposed a get_status callback.
Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
drivers/usb/gadget/composite.c | 26 +++++++++++++++++++++++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 2111732..2c1100e 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2006,9 +2006,20 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
f = cdev->config->interface[intf];
if (!f)
break;
- status = f->get_status ? f->get_status(f) : 0;
- if (status < 0)
- break;
+
+ if (f->get_status) {
+ status = f->get_status(f);
+ if (status < 0)
+ break;
+ } else {
+ /* Set D0 and D1 bits based on func wakeup capability */
+ if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
+ status |= USB_INTRF_STAT_FUNC_RW_CAP;
+ if (f->func_wakeup_armed)
+ status |= USB_INTRF_STAT_FUNC_RW;
+ }
+ }
+
put_unaligned_le16(status & 0x0000ffff, req->buf);
break;
/*
@@ -2029,6 +2040,15 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
f = cdev->config->interface[intf];
if (!f)
break;
+
+ if (w_index & USB_INTRF_FUNC_SUSPEND_RW) {
+ if (!(f->config->bmAttributes &
+ USB_CONFIG_ATT_WAKEUP))
+ break;
+ f->func_wakeup_armed = (ctrl->bRequest ==
+ USB_REQ_SET_FEATURE);
+ }
+
value = 0;
if (f->func_suspend)
value = f->func_suspend(f, w_index >> 8);
--
2.7.4
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
2023-03-16 23:38 [PATCH v12 0/6] Add function suspend/resume and remote wakeup support Elson Roy Serrao
` (4 preceding siblings ...)
2023-03-16 23:38 ` [PATCH v12 5/6] usb: gadget: arm the function for triggering remote wakeup Elson Roy Serrao
@ 2023-03-16 23:38 ` Elson Roy Serrao
2023-03-17 0:11 ` Thinh Nguyen
2023-03-21 8:12 ` Dan Carpenter
5 siblings, 2 replies; 19+ messages in thread
From: Elson Roy Serrao @ 2023-03-16 23:38 UTC (permalink / raw)
To: gregkh, Thinh.Nguyen
Cc: linux-kernel, linux-usb, quic_wcheng, quic_jackp, Elson Roy Serrao
When host sends a suspend notification to the device, handle
the suspend callbacks in the function driver. Enhanced super
speed devices can support function suspend feature to put the
function in suspend state. Handle function suspend callback.
Depending on the remote wakeup capability the device can either
trigger a remote wakeup or wait for the host initiated resume to
start data transfer again.
Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
---
drivers/usb/gadget/function/f_ecm.c | 71 +++++++++++++++++++++++++++++++++++
drivers/usb/gadget/function/u_ether.c | 63 +++++++++++++++++++++++++++++++
drivers/usb/gadget/function/u_ether.h | 4 ++
3 files changed, 138 insertions(+)
diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
index a7ab30e..c43cd557 100644
--- a/drivers/usb/gadget/function/f_ecm.c
+++ b/drivers/usb/gadget/function/f_ecm.c
@@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
usb_ep_disable(ecm->notify);
ecm->notify->desc = NULL;
+ f->func_suspended = false;
+ f->func_wakeup_armed = false;
}
/*-------------------------------------------------------------------------*/
@@ -885,6 +887,71 @@ static struct usb_function_instance *ecm_alloc_inst(void)
return &opts->func_inst;
}
+static void ecm_suspend(struct usb_function *f)
+{
+ struct f_ecm *ecm = func_to_ecm(f);
+ struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
+
+ if (f->func_suspended) {
+ DBG(cdev, "Function already suspended\n");
+ return;
+ }
+
+ DBG(cdev, "ECM Suspend\n");
+
+ gether_suspend(&ecm->port);
+}
+
+static void ecm_resume(struct usb_function *f)
+{
+ struct f_ecm *ecm = func_to_ecm(f);
+ struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
+
+ /*
+ * If the function is in USB3 Function Suspend state, resume is
+ * canceled. In this case resume is done by a Function Resume request.
+ */
+ if (f->func_suspended)
+ return;
+
+ DBG(cdev, "ECM Resume\n");
+
+ gether_resume(&ecm->port);
+}
+
+static int ecm_get_status(struct usb_function *f)
+{
+ struct usb_configuration *c = f->config;
+
+ /* D0 and D1 bit set to 0 if device is not wakeup capable */
+ if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
+ return 0;
+
+ return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
+ USB_INTRF_STAT_FUNC_RW_CAP;
+}
+
+static int ecm_func_suspend(struct usb_function *f, u8 options)
+{
+ struct usb_composite_dev *cdev = f->config->cdev;
+
+ DBG(cdev, "func susp %u cmd\n", options);
+
+ if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
+ if (!f->func_suspended) {
+ ecm_suspend(f);
+ f->func_suspended = true;
+ }
+ } else {
+ if (f->func_suspended) {
+ f->func_suspended = false;
+ ecm_resume(f);
+ }
+ }
+
+ return 0;
+}
+
static void ecm_free(struct usb_function *f)
{
struct f_ecm *ecm;
@@ -952,6 +1019,10 @@ static struct usb_function *ecm_alloc(struct usb_function_instance *fi)
ecm->port.func.setup = ecm_setup;
ecm->port.func.disable = ecm_disable;
ecm->port.func.free_func = ecm_free;
+ ecm->port.func.suspend = ecm_suspend;
+ ecm->port.func.get_status = ecm_get_status;
+ ecm->port.func.func_suspend = ecm_func_suspend;
+ ecm->port.func.resume = ecm_resume;
return &ecm->port.func;
}
diff --git a/drivers/usb/gadget/function/u_ether.c b/drivers/usb/gadget/function/u_ether.c
index f259975..8eba018 100644
--- a/drivers/usb/gadget/function/u_ether.c
+++ b/drivers/usb/gadget/function/u_ether.c
@@ -437,6 +437,20 @@ static inline int is_promisc(u16 cdc_filter)
return cdc_filter & USB_CDC_PACKET_TYPE_PROMISCUOUS;
}
+static int ether_wakeup_host(struct gether *port)
+{
+ int ret;
+ struct usb_function *func = &port->func;
+ struct usb_gadget *gadget = func->config->cdev->gadget;
+
+ if (func->func_suspended)
+ ret = usb_func_wakeup(func);
+ else
+ ret = usb_gadget_wakeup(gadget);
+
+ return ret;
+}
+
static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
struct net_device *net)
{
@@ -456,6 +470,15 @@ static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
in = NULL;
cdc_filter = 0;
}
+
+ if (dev->port_usb->is_suspend) {
+ DBG(dev, "Port suspended. Triggering wakeup\n");
+ netif_stop_queue(net);
+ spin_unlock_irqrestore(&dev->lock, flags);
+ ether_wakeup_host(dev->port_usb);
+ return NETDEV_TX_BUSY;
+ }
+
spin_unlock_irqrestore(&dev->lock, flags);
if (!in) {
@@ -1014,6 +1037,45 @@ int gether_set_ifname(struct net_device *net, const char *name, int len)
}
EXPORT_SYMBOL_GPL(gether_set_ifname);
+void gether_suspend(struct gether *link)
+{
+ struct eth_dev *dev = link->ioport;
+ unsigned long flags;
+
+ if (!dev)
+ return;
+
+ if (atomic_read(&dev->tx_qlen)) {
+ /*
+ * There is a transfer in progress. So we trigger a remote
+ * wakeup to inform the host.
+ */
+ ether_wakeup_host(dev->port_usb);
+ return;
+ }
+ spin_lock_irqsave(&dev->lock, flags);
+ link->is_suspend = true;
+ spin_unlock_irqrestore(&dev->lock, flags);
+}
+EXPORT_SYMBOL_GPL(gether_suspend);
+
+void gether_resume(struct gether *link)
+{
+ struct eth_dev *dev = link->ioport;
+ unsigned long flags;
+
+ if (!dev)
+ return;
+
+ if (netif_queue_stopped(dev->net))
+ netif_start_queue(dev->net);
+
+ spin_lock_irqsave(&dev->lock, flags);
+ link->is_suspend = false;
+ spin_unlock_irqrestore(&dev->lock, flags);
+}
+EXPORT_SYMBOL_GPL(gether_resume);
+
/*
* gether_cleanup - remove Ethernet-over-USB device
* Context: may sleep
@@ -1176,6 +1238,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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
2023-03-16 23:38 ` [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support Elson Roy Serrao
@ 2023-03-17 0:11 ` Thinh Nguyen
2023-03-17 17:59 ` Elson Serrao
2023-03-21 8:12 ` Dan Carpenter
1 sibling, 1 reply; 19+ messages in thread
From: Thinh Nguyen @ 2023-03-17 0:11 UTC (permalink / raw)
To: Elson Roy Serrao
Cc: gregkh, Thinh Nguyen, linux-kernel, linux-usb, quic_wcheng, quic_jackp
On Thu, Mar 16, 2023, Elson Roy Serrao wrote:
> When host sends a suspend notification to the device, handle
> the suspend callbacks in the function driver. Enhanced super
> speed devices can support function suspend feature to put the
> function in suspend state. Handle function suspend callback.
>
> Depending on the remote wakeup capability the device can either
> trigger a remote wakeup or wait for the host initiated resume to
> start data transfer again.
>
> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> ---
> drivers/usb/gadget/function/f_ecm.c | 71 +++++++++++++++++++++++++++++++++++
> drivers/usb/gadget/function/u_ether.c | 63 +++++++++++++++++++++++++++++++
> drivers/usb/gadget/function/u_ether.h | 4 ++
> 3 files changed, 138 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
> index a7ab30e..c43cd557 100644
> --- a/drivers/usb/gadget/function/f_ecm.c
> +++ b/drivers/usb/gadget/function/f_ecm.c
> @@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
>
> usb_ep_disable(ecm->notify);
> ecm->notify->desc = NULL;
> + f->func_suspended = false;
> + f->func_wakeup_armed = false;
> }
>
> /*-------------------------------------------------------------------------*/
> @@ -885,6 +887,71 @@ static struct usb_function_instance *ecm_alloc_inst(void)
> return &opts->func_inst;
> }
>
> +static void ecm_suspend(struct usb_function *f)
> +{
> + struct f_ecm *ecm = func_to_ecm(f);
> + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> +
> + if (f->func_suspended) {
> + DBG(cdev, "Function already suspended\n");
> + return;
> + }
> +
> + DBG(cdev, "ECM Suspend\n");
> +
> + gether_suspend(&ecm->port);
> +}
> +
> +static void ecm_resume(struct usb_function *f)
> +{
> + struct f_ecm *ecm = func_to_ecm(f);
> + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> +
> + /*
> + * If the function is in USB3 Function Suspend state, resume is
> + * canceled. In this case resume is done by a Function Resume request.
> + */
> + if (f->func_suspended)
> + return;
> +
> + DBG(cdev, "ECM Resume\n");
> +
> + gether_resume(&ecm->port);
> +}
> +
> +static int ecm_get_status(struct usb_function *f)
> +{
> + struct usb_configuration *c = f->config;
> +
> + /* D0 and D1 bit set to 0 if device is not wakeup capable */
> + if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
> + return 0;
> +
> + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
> + USB_INTRF_STAT_FUNC_RW_CAP;
> +}
Why do we need to implement ecm_get_status if it's already handled in
composite.c now?
> +
> +static int ecm_func_suspend(struct usb_function *f, u8 options)
> +{
> + struct usb_composite_dev *cdev = f->config->cdev;
> +
> + DBG(cdev, "func susp %u cmd\n", options);
> +
> + if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
This feature selector doesn't indicate whether it's SetFeature or
ClearFeature request. ecm_func_suspend is supposed to be for
SetFeature(suspend) only. Perhaps we may have to define func_resume()
for ClearFeature(suspend)?
Thanks,
Thinh
> + if (!f->func_suspended) {
> + ecm_suspend(f);
> + f->func_suspended = true;
> + }
> + } else {
> + if (f->func_suspended) {
> + f->func_suspended = false;
> + ecm_resume(f);
> + }
> + }
> +
> + return 0;
> +}
> +
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
2023-03-17 0:11 ` Thinh Nguyen
@ 2023-03-17 17:59 ` Elson Serrao
2023-03-17 21:28 ` Thinh Nguyen
0 siblings, 1 reply; 19+ messages in thread
From: Elson Serrao @ 2023-03-17 17:59 UTC (permalink / raw)
To: Thinh Nguyen; +Cc: gregkh, linux-kernel, linux-usb, quic_wcheng, quic_jackp
On 3/16/2023 5:11 PM, Thinh Nguyen wrote:
> On Thu, Mar 16, 2023, Elson Roy Serrao wrote:
>> When host sends a suspend notification to the device, handle
>> the suspend callbacks in the function driver. Enhanced super
>> speed devices can support function suspend feature to put the
>> function in suspend state. Handle function suspend callback.
>>
>> Depending on the remote wakeup capability the device can either
>> trigger a remote wakeup or wait for the host initiated resume to
>> start data transfer again.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>> drivers/usb/gadget/function/f_ecm.c | 71 +++++++++++++++++++++++++++++++++++
>> drivers/usb/gadget/function/u_ether.c | 63 +++++++++++++++++++++++++++++++
>> drivers/usb/gadget/function/u_ether.h | 4 ++
>> 3 files changed, 138 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
>> index a7ab30e..c43cd557 100644
>> --- a/drivers/usb/gadget/function/f_ecm.c
>> +++ b/drivers/usb/gadget/function/f_ecm.c
>> @@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
>>
>> usb_ep_disable(ecm->notify);
>> ecm->notify->desc = NULL;
>> + f->func_suspended = false;
>> + f->func_wakeup_armed = false;
>> }
>>
>> /*-------------------------------------------------------------------------*/
>> @@ -885,6 +887,71 @@ static struct usb_function_instance *ecm_alloc_inst(void)
>> return &opts->func_inst;
>> }
>>
>> +static void ecm_suspend(struct usb_function *f)
>> +{
>> + struct f_ecm *ecm = func_to_ecm(f);
>> + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
>> +
>> + if (f->func_suspended) {
>> + DBG(cdev, "Function already suspended\n");
>> + return;
>> + }
>> +
>> + DBG(cdev, "ECM Suspend\n");
>> +
>> + gether_suspend(&ecm->port);
>> +}
>> +
>> +static void ecm_resume(struct usb_function *f)
>> +{
>> + struct f_ecm *ecm = func_to_ecm(f);
>> + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
>> +
>> + /*
>> + * If the function is in USB3 Function Suspend state, resume is
>> + * canceled. In this case resume is done by a Function Resume request.
>> + */
>> + if (f->func_suspended)
>> + return;
>> +
>> + DBG(cdev, "ECM Resume\n");
>> +
>> + gether_resume(&ecm->port);
>> +}
>> +
>> +static int ecm_get_status(struct usb_function *f)
>> +{
>> + struct usb_configuration *c = f->config;
>> +
>> + /* D0 and D1 bit set to 0 if device is not wakeup capable */
>> + if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
>> + return 0;
>> +
>> + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
>> + USB_INTRF_STAT_FUNC_RW_CAP;
>> +}
>
> Why do we need to implement ecm_get_status if it's already handled in
> composite.c now?
>
Yes this can be removed now. Will modify accordingly.
>> +
>> +static int ecm_func_suspend(struct usb_function *f, u8 options)
>> +{
>> + struct usb_composite_dev *cdev = f->config->cdev;
>> +
>> + DBG(cdev, "func susp %u cmd\n", options);
>> +
>> + if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
>
> This feature selector doesn't indicate whether it's SetFeature or
> ClearFeature request. ecm_func_suspend is supposed to be for
> SetFeature(suspend) only. Perhaps we may have to define func_resume()
> for ClearFeature(suspend)?
>
> Thanks,
> Thinh
>
Host uses the same feature selector FUNCTION_SUSPEND for function
suspend and function resume and func_suspend() callback can be used to
handle both the cases ? The distinction comes whether it is a
SetFeature(FUNCTION_SUSPEND) or ClearFeature(FUNCTION_SUSPEND) which can
be easily done in the func_suspend callback itself. We can add another
callback func_resume specific to ClearFeature(FUNCTION_SUSPEND) but wont
that be redundant and more callback handling on function
driver/composite side as well? Please let me know your opinion.
Thanks
Elson
>> + if (!f->func_suspended) {
>> + ecm_suspend(f);
>> + f->func_suspended = true;
>> + }
>> + } else {
>> + if (f->func_suspended) {
>> + f->func_suspended = false;
>> + ecm_resume(f);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
2023-03-17 17:59 ` Elson Serrao
@ 2023-03-17 21:28 ` Thinh Nguyen
2023-03-17 23:20 ` Thinh Nguyen
0 siblings, 1 reply; 19+ messages in thread
From: Thinh Nguyen @ 2023-03-17 21:28 UTC (permalink / raw)
To: Elson Serrao
Cc: Thinh Nguyen, gregkh, linux-kernel, linux-usb, quic_wcheng, quic_jackp
On Fri, Mar 17, 2023, Elson Serrao wrote:
>
>
> On 3/16/2023 5:11 PM, Thinh Nguyen wrote:
> > On Thu, Mar 16, 2023, Elson Roy Serrao wrote:
> > > When host sends a suspend notification to the device, handle
> > > the suspend callbacks in the function driver. Enhanced super
> > > speed devices can support function suspend feature to put the
> > > function in suspend state. Handle function suspend callback.
> > >
> > > Depending on the remote wakeup capability the device can either
> > > trigger a remote wakeup or wait for the host initiated resume to
> > > start data transfer again.
> > >
> > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> > > ---
> > > drivers/usb/gadget/function/f_ecm.c | 71 +++++++++++++++++++++++++++++++++++
> > > drivers/usb/gadget/function/u_ether.c | 63 +++++++++++++++++++++++++++++++
> > > drivers/usb/gadget/function/u_ether.h | 4 ++
> > > 3 files changed, 138 insertions(+)
> > >
> > > diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
> > > index a7ab30e..c43cd557 100644
> > > --- a/drivers/usb/gadget/function/f_ecm.c
> > > +++ b/drivers/usb/gadget/function/f_ecm.c
> > > @@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
> > > usb_ep_disable(ecm->notify);
> > > ecm->notify->desc = NULL;
> > > + f->func_suspended = false;
> > > + f->func_wakeup_armed = false;
> > > }
> > > /*-------------------------------------------------------------------------*/
> > > @@ -885,6 +887,71 @@ static struct usb_function_instance *ecm_alloc_inst(void)
> > > return &opts->func_inst;
> > > }
> > > +static void ecm_suspend(struct usb_function *f)
> > > +{
> > > + struct f_ecm *ecm = func_to_ecm(f);
> > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> > > +
> > > + if (f->func_suspended) {
> > > + DBG(cdev, "Function already suspended\n");
> > > + return;
> > > + }
> > > +
> > > + DBG(cdev, "ECM Suspend\n");
> > > +
> > > + gether_suspend(&ecm->port);
> > > +}
> > > +
> > > +static void ecm_resume(struct usb_function *f)
> > > +{
> > > + struct f_ecm *ecm = func_to_ecm(f);
> > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> > > +
> > > + /*
> > > + * If the function is in USB3 Function Suspend state, resume is
> > > + * canceled. In this case resume is done by a Function Resume request.
> > > + */
> > > + if (f->func_suspended)
> > > + return;
> > > +
> > > + DBG(cdev, "ECM Resume\n");
> > > +
> > > + gether_resume(&ecm->port);
> > > +}
> > > +
> > > +static int ecm_get_status(struct usb_function *f)
> > > +{
> > > + struct usb_configuration *c = f->config;
> > > +
> > > + /* D0 and D1 bit set to 0 if device is not wakeup capable */
> > > + if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
> > > + return 0;
> > > +
> > > + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
> > > + USB_INTRF_STAT_FUNC_RW_CAP;
> > > +}
> >
> > Why do we need to implement ecm_get_status if it's already handled in
> > composite.c now?
> >
>
> Yes this can be removed now. Will modify accordingly.
> > > +
> > > +static int ecm_func_suspend(struct usb_function *f, u8 options)
> > > +{
> > > + struct usb_composite_dev *cdev = f->config->cdev;
> > > +
> > > + DBG(cdev, "func susp %u cmd\n", options);
> > > +
> > > + if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
> >
> > This feature selector doesn't indicate whether it's SetFeature or
> > ClearFeature request. ecm_func_suspend is supposed to be for
> > SetFeature(suspend) only. Perhaps we may have to define func_resume()
> > for ClearFeature(suspend)?
> >
> Host uses the same feature selector FUNCTION_SUSPEND for function suspend
> and function resume and func_suspend() callback can be used to
> handle both the cases ? The distinction comes whether it is a
How do you plan to handle that? Pass this info in some unused/reserved
bit of the "options" argument? Introduce a new parameter to the
func_suspend()?
If that's the case, then you need to update the document on
func_suspend() to also support ClearFeature(suspend). Right now it's
documented for SetFeature only. Also, make sure that other existing
function drivers will not break because of the change of the
func_suspend behavior.
> SetFeature(FUNCTION_SUSPEND) or ClearFeature(FUNCTION_SUSPEND) which can be
> easily done in the func_suspend callback itself. We can add another callback
> func_resume specific to ClearFeature(FUNCTION_SUSPEND) but wont that be
> redundant and more callback handling on function driver/composite side as
> well? Please let me know your opinion.
>
We actually didn't properly define func_suspend and its counter part. It
seems cleaner to me to introduce func_resume as it seems more intuitive
and easier to read. Let me know how you plan to use func_suspend() for
both cases.
Thanks,
Thinh
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
2023-03-17 21:28 ` Thinh Nguyen
@ 2023-03-17 23:20 ` Thinh Nguyen
2023-03-18 0:23 ` Elson Serrao
0 siblings, 1 reply; 19+ messages in thread
From: Thinh Nguyen @ 2023-03-17 23:20 UTC (permalink / raw)
To: Elson Serrao; +Cc: gregkh, linux-kernel, linux-usb, quic_wcheng, quic_jackp
On Fri, Mar 17, 2023, Thinh Nguyen wrote:
> On Fri, Mar 17, 2023, Elson Serrao wrote:
> >
> >
> > On 3/16/2023 5:11 PM, Thinh Nguyen wrote:
> > > On Thu, Mar 16, 2023, Elson Roy Serrao wrote:
> > > > When host sends a suspend notification to the device, handle
> > > > the suspend callbacks in the function driver. Enhanced super
> > > > speed devices can support function suspend feature to put the
> > > > function in suspend state. Handle function suspend callback.
> > > >
> > > > Depending on the remote wakeup capability the device can either
> > > > trigger a remote wakeup or wait for the host initiated resume to
> > > > start data transfer again.
> > > >
> > > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> > > > ---
> > > > drivers/usb/gadget/function/f_ecm.c | 71 +++++++++++++++++++++++++++++++++++
> > > > drivers/usb/gadget/function/u_ether.c | 63 +++++++++++++++++++++++++++++++
> > > > drivers/usb/gadget/function/u_ether.h | 4 ++
> > > > 3 files changed, 138 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
> > > > index a7ab30e..c43cd557 100644
> > > > --- a/drivers/usb/gadget/function/f_ecm.c
> > > > +++ b/drivers/usb/gadget/function/f_ecm.c
> > > > @@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
> > > > usb_ep_disable(ecm->notify);
> > > > ecm->notify->desc = NULL;
> > > > + f->func_suspended = false;
> > > > + f->func_wakeup_armed = false;
> > > > }
> > > > /*-------------------------------------------------------------------------*/
> > > > @@ -885,6 +887,71 @@ static struct usb_function_instance *ecm_alloc_inst(void)
> > > > return &opts->func_inst;
> > > > }
> > > > +static void ecm_suspend(struct usb_function *f)
> > > > +{
> > > > + struct f_ecm *ecm = func_to_ecm(f);
> > > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> > > > +
> > > > + if (f->func_suspended) {
> > > > + DBG(cdev, "Function already suspended\n");
> > > > + return;
> > > > + }
> > > > +
> > > > + DBG(cdev, "ECM Suspend\n");
> > > > +
> > > > + gether_suspend(&ecm->port);
> > > > +}
> > > > +
> > > > +static void ecm_resume(struct usb_function *f)
> > > > +{
> > > > + struct f_ecm *ecm = func_to_ecm(f);
> > > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> > > > +
> > > > + /*
> > > > + * If the function is in USB3 Function Suspend state, resume is
> > > > + * canceled. In this case resume is done by a Function Resume request.
> > > > + */
> > > > + if (f->func_suspended)
> > > > + return;
> > > > +
> > > > + DBG(cdev, "ECM Resume\n");
> > > > +
> > > > + gether_resume(&ecm->port);
> > > > +}
> > > > +
> > > > +static int ecm_get_status(struct usb_function *f)
> > > > +{
> > > > + struct usb_configuration *c = f->config;
> > > > +
> > > > + /* D0 and D1 bit set to 0 if device is not wakeup capable */
> > > > + if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
> > > > + return 0;
> > > > +
> > > > + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
> > > > + USB_INTRF_STAT_FUNC_RW_CAP;
> > > > +}
> > >
> > > Why do we need to implement ecm_get_status if it's already handled in
> > > composite.c now?
> > >
> >
> > Yes this can be removed now. Will modify accordingly.
> > > > +
> > > > +static int ecm_func_suspend(struct usb_function *f, u8 options)
> > > > +{
> > > > + struct usb_composite_dev *cdev = f->config->cdev;
> > > > +
> > > > + DBG(cdev, "func susp %u cmd\n", options);
> > > > +
> > > > + if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
> > >
> > > This feature selector doesn't indicate whether it's SetFeature or
> > > ClearFeature request. ecm_func_suspend is supposed to be for
> > > SetFeature(suspend) only. Perhaps we may have to define func_resume()
> > > for ClearFeature(suspend)?
> > >
>
> > Host uses the same feature selector FUNCTION_SUSPEND for function suspend
> > and function resume and func_suspend() callback can be used to
> > handle both the cases ? The distinction comes whether it is a
>
> How do you plan to handle that? Pass this info in some unused/reserved
> bit of the "options" argument? Introduce a new parameter to the
> func_suspend()?
>
> If that's the case, then you need to update the document on
> func_suspend() to also support ClearFeature(suspend). Right now it's
> documented for SetFeature only. Also, make sure that other existing
> function drivers will not break because of the change of the
> func_suspend behavior.
>
> > SetFeature(FUNCTION_SUSPEND) or ClearFeature(FUNCTION_SUSPEND) which can be
> > easily done in the func_suspend callback itself. We can add another callback
> > func_resume specific to ClearFeature(FUNCTION_SUSPEND) but wont that be
> > redundant and more callback handling on function driver/composite side as
> > well? Please let me know your opinion.
> >
>
> We actually didn't properly define func_suspend and its counter part. It
> seems cleaner to me to introduce func_resume as it seems more intuitive
> and easier to read. Let me know how you plan to use func_suspend() for
> both cases.
>
How about we handle function suspend resume in composite also? I mean
something like this:
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 36add1879ed2..79dc055eb5f7 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -1948,9 +1948,18 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
f = cdev->config->interface[intf];
if (!f)
break;
- status = f->get_status ? f->get_status(f) : 0;
- if (status < 0)
- break;
+
+ if (f->get_status) {
+ status = f->get_status(f);
+ if (status < 0)
+ break;
+ } else {
+ if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
+ status |= USB_INTRF_STAT_FUNC_RW_CAP;
+ if (f->func_wakeup_armed)
+ status |= USB_INTRF_STAT_FUNC_RW;
+ }
+ }
put_unaligned_le16(status & 0x0000ffff, req->buf);
break;
/*
@@ -1971,9 +1980,28 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
f = cdev->config->interface[intf];
if (!f)
break;
+ if (w_index & USB_INTRF_FUNC_SUSPEND_RW) {
+ if (!(f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP))
+ break;
+
+ f->func_wakeup_armed = (ctrl->bRequest == USB_REQ_SET_FEATURE);
+ }
+
value = 0;
- if (f->func_suspend)
+ if (f->func_suspend) {
value = f->func_suspend(f, w_index >> 8);
+ } else if (w_index & USB_INTRF_FUNC_SUSPEND_LP) {
+ if (f->suspend && && !f->func_suspended &&
+ ctrl->bRequest == USB_REQ_SET_FEATURE)) {
+ f->suspend(f);
+ f->func_suspended = true;
+ } else if (f->resume && f->func_suspended &&
+ ctrl->bRequest == USB_REQ_CLEAR_FEATURE_FEATURE)) {
+ f->resume(f);
+ f->func_suspended = false;
+ }
+ }
+
if (value < 0) {
ERROR(cdev,
"func_suspend() returned error %d\n",
Also, do we need the f->func_suspended flag? we'd need the remote wakeup
flag for the status, but when do we need f->func_suspended? It seems
like it can be handled within the function driver's scope.
Thanks,
Thinh
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
2023-03-17 23:20 ` Thinh Nguyen
@ 2023-03-18 0:23 ` Elson Serrao
2023-03-18 2:26 ` Thinh Nguyen
0 siblings, 1 reply; 19+ messages in thread
From: Elson Serrao @ 2023-03-18 0:23 UTC (permalink / raw)
To: Thinh Nguyen; +Cc: gregkh, linux-kernel, linux-usb, quic_wcheng, quic_jackp
On 3/17/2023 4:20 PM, Thinh Nguyen wrote:
> On Fri, Mar 17, 2023, Thinh Nguyen wrote:
>> On Fri, Mar 17, 2023, Elson Serrao wrote:
>>>
>>>
>>> On 3/16/2023 5:11 PM, Thinh Nguyen wrote:
>>>> On Thu, Mar 16, 2023, Elson Roy Serrao wrote:
>>>>> When host sends a suspend notification to the device, handle
>>>>> the suspend callbacks in the function driver. Enhanced super
>>>>> speed devices can support function suspend feature to put the
>>>>> function in suspend state. Handle function suspend callback.
>>>>>
>>>>> Depending on the remote wakeup capability the device can either
>>>>> trigger a remote wakeup or wait for the host initiated resume to
>>>>> start data transfer again.
>>>>>
>>>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>>>> ---
>>>>> drivers/usb/gadget/function/f_ecm.c | 71 +++++++++++++++++++++++++++++++++++
>>>>> drivers/usb/gadget/function/u_ether.c | 63 +++++++++++++++++++++++++++++++
>>>>> drivers/usb/gadget/function/u_ether.h | 4 ++
>>>>> 3 files changed, 138 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
>>>>> index a7ab30e..c43cd557 100644
>>>>> --- a/drivers/usb/gadget/function/f_ecm.c
>>>>> +++ b/drivers/usb/gadget/function/f_ecm.c
>>>>> @@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
>>>>> usb_ep_disable(ecm->notify);
>>>>> ecm->notify->desc = NULL;
>>>>> + f->func_suspended = false;
>>>>> + f->func_wakeup_armed = false;
>>>>> }
>>>>> /*-------------------------------------------------------------------------*/
>>>>> @@ -885,6 +887,71 @@ static struct usb_function_instance *ecm_alloc_inst(void)
>>>>> return &opts->func_inst;
>>>>> }
>>>>> +static void ecm_suspend(struct usb_function *f)
>>>>> +{
>>>>> + struct f_ecm *ecm = func_to_ecm(f);
>>>>> + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
>>>>> +
>>>>> + if (f->func_suspended) {
>>>>> + DBG(cdev, "Function already suspended\n");
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + DBG(cdev, "ECM Suspend\n");
>>>>> +
>>>>> + gether_suspend(&ecm->port);
>>>>> +}
>>>>> +
>>>>> +static void ecm_resume(struct usb_function *f)
>>>>> +{
>>>>> + struct f_ecm *ecm = func_to_ecm(f);
>>>>> + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
>>>>> +
>>>>> + /*
>>>>> + * If the function is in USB3 Function Suspend state, resume is
>>>>> + * canceled. In this case resume is done by a Function Resume request.
>>>>> + */
>>>>> + if (f->func_suspended)
>>>>> + return;
>>>>> +
>>>>> + DBG(cdev, "ECM Resume\n");
>>>>> +
>>>>> + gether_resume(&ecm->port);
>>>>> +}
>>>>> +
>>>>> +static int ecm_get_status(struct usb_function *f)
>>>>> +{
>>>>> + struct usb_configuration *c = f->config;
>>>>> +
>>>>> + /* D0 and D1 bit set to 0 if device is not wakeup capable */
>>>>> + if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
>>>>> + return 0;
>>>>> +
>>>>> + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
>>>>> + USB_INTRF_STAT_FUNC_RW_CAP;
>>>>> +}
>>>>
>>>> Why do we need to implement ecm_get_status if it's already handled in
>>>> composite.c now?
>>>>
>>>
>>> Yes this can be removed now. Will modify accordingly.
>>>>> +
>>>>> +static int ecm_func_suspend(struct usb_function *f, u8 options)
>>>>> +{
>>>>> + struct usb_composite_dev *cdev = f->config->cdev;
>>>>> +
>>>>> + DBG(cdev, "func susp %u cmd\n", options);
>>>>> +
>>>>> + if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
>>>>
>>>> This feature selector doesn't indicate whether it's SetFeature or
>>>> ClearFeature request. ecm_func_suspend is supposed to be for
>>>> SetFeature(suspend) only. Perhaps we may have to define func_resume()
>>>> for ClearFeature(suspend)?
>>>>
>>
>>> Host uses the same feature selector FUNCTION_SUSPEND for function suspend
>>> and function resume and func_suspend() callback can be used to
>>> handle both the cases ? The distinction comes whether it is a
>>
>> How do you plan to handle that? Pass this info in some unused/reserved
>> bit of the "options" argument? Introduce a new parameter to the
>> func_suspend()?
>>
>> If that's the case, then you need to update the document on
>> func_suspend() to also support ClearFeature(suspend). Right now it's
>> documented for SetFeature only. Also, make sure that other existing
>> function drivers will not break because of the change of the
>> func_suspend behavior.
>>
>>> SetFeature(FUNCTION_SUSPEND) or ClearFeature(FUNCTION_SUSPEND) which can be
>>> easily done in the func_suspend callback itself. We can add another callback
>>> func_resume specific to ClearFeature(FUNCTION_SUSPEND) but wont that be
>>> redundant and more callback handling on function driver/composite side as
>>> well? Please let me know your opinion.
>>>
>>
>> We actually didn't properly define func_suspend and its counter part. It
>> seems cleaner to me to introduce func_resume as it seems more intuitive
>> and easier to read. Let me know how you plan to use func_suspend() for
>> both cases.
>>
>
> How about we handle function suspend resume in composite also? I mean
> something like this:
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 36add1879ed2..79dc055eb5f7 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -1948,9 +1948,18 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
> f = cdev->config->interface[intf];
> if (!f)
> break;
> - status = f->get_status ? f->get_status(f) : 0;
> - if (status < 0)
> - break;
> +
> + if (f->get_status) {
> + status = f->get_status(f);
> + if (status < 0)
> + break;
> + } else {
> + if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
> + status |= USB_INTRF_STAT_FUNC_RW_CAP;
> + if (f->func_wakeup_armed)
> + status |= USB_INTRF_STAT_FUNC_RW;
> + }
> + }
> put_unaligned_le16(status & 0x0000ffff, req->buf);
> break;
> /*
> @@ -1971,9 +1980,28 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
> f = cdev->config->interface[intf];
> if (!f)
> break;
> + if (w_index & USB_INTRF_FUNC_SUSPEND_RW) {
> + if (!(f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP))
> + break;
> +
> + f->func_wakeup_armed = (ctrl->bRequest == USB_REQ_SET_FEATURE);
> + }
> +
> value = 0;
> - if (f->func_suspend)
> + if (f->func_suspend) {
> value = f->func_suspend(f, w_index >> 8);
> + } else if (w_index & USB_INTRF_FUNC_SUSPEND_LP) {
> + if (f->suspend && && !f->func_suspended &&
> + ctrl->bRequest == USB_REQ_SET_FEATURE)) {
> + f->suspend(f);
> + f->func_suspended = true;
> + } else if (f->resume && f->func_suspended &&
> + ctrl->bRequest == USB_REQ_CLEAR_FEATURE_FEATURE)) {
> + f->resume(f);
> + f->func_suspended = false;
> + }
> + }
> +
> if (value < 0) {
> ERROR(cdev,
> "func_suspend() returned error %d\n",
>
At individual function driver level there is no need to differentiate
between suspend() and func_suspend() APIs, as both are intended to put
the function in suspend state. So your idea/implementation above makes
it much more clearer. Let composite also handle this and call either
f->suspend() or f->resume() callback based on the setup packet received.
Thank you for this suggestion.
>
> Also, do we need the f->func_suspended flag? we'd need the remote wakeup
> flag for the status, but when do we need f->func_suspended? It seems
> like it can be handled within the function driver's scope.
f->func_suspended flag I had added for below purposes
1.) Function drivers should know the right wakeup() op to be called.
That is if they are in FUNC_SUSPEND then call usb_func_wakeup() and if
they are in device suspend then call usb_gadget_wakeup(). (we can use
f->func_wakeup_armed flag for this purpose as well)
2.) If a function is in USB3 FUNCTION_SUSPEND state then it shouldn't
allow f->resume() called through composite_resume() as the exit from
FUNCTION_SUSPEND state is via ClearFeature(FUNCTION_SUSPEND).
So we need a way to tell function drivers if they are in USB3
FUNCTION_SUSPEND state OR device suspend.
Please let me know if you see any alternative or better approach here.
Thanks
Elson
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
2023-03-18 0:23 ` Elson Serrao
@ 2023-03-18 2:26 ` Thinh Nguyen
2023-03-20 17:42 ` Elson Serrao
0 siblings, 1 reply; 19+ messages in thread
From: Thinh Nguyen @ 2023-03-18 2:26 UTC (permalink / raw)
To: Elson Serrao
Cc: Thinh Nguyen, gregkh, linux-kernel, linux-usb, quic_wcheng, quic_jackp
On Fri, Mar 17, 2023, Elson Serrao wrote:
>
>
> On 3/17/2023 4:20 PM, Thinh Nguyen wrote:
> > On Fri, Mar 17, 2023, Thinh Nguyen wrote:
> > > On Fri, Mar 17, 2023, Elson Serrao wrote:
> > > >
> > > >
> > > > On 3/16/2023 5:11 PM, Thinh Nguyen wrote:
> > > > > On Thu, Mar 16, 2023, Elson Roy Serrao wrote:
> > > > > > When host sends a suspend notification to the device, handle
> > > > > > the suspend callbacks in the function driver. Enhanced super
> > > > > > speed devices can support function suspend feature to put the
> > > > > > function in suspend state. Handle function suspend callback.
> > > > > >
> > > > > > Depending on the remote wakeup capability the device can either
> > > > > > trigger a remote wakeup or wait for the host initiated resume to
> > > > > > start data transfer again.
> > > > > >
> > > > > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> > > > > > ---
> > > > > > drivers/usb/gadget/function/f_ecm.c | 71 +++++++++++++++++++++++++++++++++++
> > > > > > drivers/usb/gadget/function/u_ether.c | 63 +++++++++++++++++++++++++++++++
> > > > > > drivers/usb/gadget/function/u_ether.h | 4 ++
> > > > > > 3 files changed, 138 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
> > > > > > index a7ab30e..c43cd557 100644
> > > > > > --- a/drivers/usb/gadget/function/f_ecm.c
> > > > > > +++ b/drivers/usb/gadget/function/f_ecm.c
> > > > > > @@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
> > > > > > usb_ep_disable(ecm->notify);
> > > > > > ecm->notify->desc = NULL;
> > > > > > + f->func_suspended = false;
> > > > > > + f->func_wakeup_armed = false;
> > > > > > }
> > > > > > /*-------------------------------------------------------------------------*/
> > > > > > @@ -885,6 +887,71 @@ static struct usb_function_instance *ecm_alloc_inst(void)
> > > > > > return &opts->func_inst;
> > > > > > }
> > > > > > +static void ecm_suspend(struct usb_function *f)
> > > > > > +{
> > > > > > + struct f_ecm *ecm = func_to_ecm(f);
> > > > > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> > > > > > +
> > > > > > + if (f->func_suspended) {
> > > > > > + DBG(cdev, "Function already suspended\n");
> > > > > > + return;
> > > > > > + }
> > > > > > +
> > > > > > + DBG(cdev, "ECM Suspend\n");
> > > > > > +
> > > > > > + gether_suspend(&ecm->port);
> > > > > > +}
> > > > > > +
> > > > > > +static void ecm_resume(struct usb_function *f)
> > > > > > +{
> > > > > > + struct f_ecm *ecm = func_to_ecm(f);
> > > > > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> > > > > > +
> > > > > > + /*
> > > > > > + * If the function is in USB3 Function Suspend state, resume is
> > > > > > + * canceled. In this case resume is done by a Function Resume request.
> > > > > > + */
> > > > > > + if (f->func_suspended)
> > > > > > + return;
> > > > > > +
> > > > > > + DBG(cdev, "ECM Resume\n");
> > > > > > +
> > > > > > + gether_resume(&ecm->port);
> > > > > > +}
> > > > > > +
> > > > > > +static int ecm_get_status(struct usb_function *f)
> > > > > > +{
> > > > > > + struct usb_configuration *c = f->config;
> > > > > > +
> > > > > > + /* D0 and D1 bit set to 0 if device is not wakeup capable */
> > > > > > + if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
> > > > > > + return 0;
> > > > > > +
> > > > > > + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
> > > > > > + USB_INTRF_STAT_FUNC_RW_CAP;
> > > > > > +}
> > > > >
> > > > > Why do we need to implement ecm_get_status if it's already handled in
> > > > > composite.c now?
> > > > >
> > > >
> > > > Yes this can be removed now. Will modify accordingly.
> > > > > > +
> > > > > > +static int ecm_func_suspend(struct usb_function *f, u8 options)
> > > > > > +{
> > > > > > + struct usb_composite_dev *cdev = f->config->cdev;
> > > > > > +
> > > > > > + DBG(cdev, "func susp %u cmd\n", options);
> > > > > > +
> > > > > > + if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
> > > > >
> > > > > This feature selector doesn't indicate whether it's SetFeature or
> > > > > ClearFeature request. ecm_func_suspend is supposed to be for
> > > > > SetFeature(suspend) only. Perhaps we may have to define func_resume()
> > > > > for ClearFeature(suspend)?
> > > > >
> > >
> > > > Host uses the same feature selector FUNCTION_SUSPEND for function suspend
> > > > and function resume and func_suspend() callback can be used to
> > > > handle both the cases ? The distinction comes whether it is a
> > >
> > > How do you plan to handle that? Pass this info in some unused/reserved
> > > bit of the "options" argument? Introduce a new parameter to the
> > > func_suspend()?
> > >
> > > If that's the case, then you need to update the document on
> > > func_suspend() to also support ClearFeature(suspend). Right now it's
> > > documented for SetFeature only. Also, make sure that other existing
> > > function drivers will not break because of the change of the
> > > func_suspend behavior.
> > >
> > > > SetFeature(FUNCTION_SUSPEND) or ClearFeature(FUNCTION_SUSPEND) which can be
> > > > easily done in the func_suspend callback itself. We can add another callback
> > > > func_resume specific to ClearFeature(FUNCTION_SUSPEND) but wont that be
> > > > redundant and more callback handling on function driver/composite side as
> > > > well? Please let me know your opinion.
> > > >
> > >
> > > We actually didn't properly define func_suspend and its counter part. It
> > > seems cleaner to me to introduce func_resume as it seems more intuitive
> > > and easier to read. Let me know how you plan to use func_suspend() for
> > > both cases.
> > >
> >
> > How about we handle function suspend resume in composite also? I mean
> > something like this:
> >
> > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> > index 36add1879ed2..79dc055eb5f7 100644
> > --- a/drivers/usb/gadget/composite.c
> > +++ b/drivers/usb/gadget/composite.c
> > @@ -1948,9 +1948,18 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
> > f = cdev->config->interface[intf];
> > if (!f)
> > break;
> > - status = f->get_status ? f->get_status(f) : 0;
> > - if (status < 0)
> > - break;
> > +
> > + if (f->get_status) {
> > + status = f->get_status(f);
> > + if (status < 0)
> > + break;
> > + } else {
> > + if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
> > + status |= USB_INTRF_STAT_FUNC_RW_CAP;
> > + if (f->func_wakeup_armed)
> > + status |= USB_INTRF_STAT_FUNC_RW;
> > + }
> > + }
> > put_unaligned_le16(status & 0x0000ffff, req->buf);
> > break;
> > /*
> > @@ -1971,9 +1980,28 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
> > f = cdev->config->interface[intf];
> > if (!f)
> > break;
> > + if (w_index & USB_INTRF_FUNC_SUSPEND_RW) {
> > + if (!(f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP))
> > + break;
> > +
> > + f->func_wakeup_armed = (ctrl->bRequest == USB_REQ_SET_FEATURE);
> > + }
> > +
> > value = 0;
> > - if (f->func_suspend)
> > + if (f->func_suspend) {
> > value = f->func_suspend(f, w_index >> 8);
> > + } else if (w_index & USB_INTRF_FUNC_SUSPEND_LP) {
> > + if (f->suspend && && !f->func_suspended &&
> > + ctrl->bRequest == USB_REQ_SET_FEATURE)) {
> > + f->suspend(f);
> > + f->func_suspended = true;
> > + } else if (f->resume && f->func_suspended &&
> > + ctrl->bRequest == USB_REQ_CLEAR_FEATURE_FEATURE)) {
> > + f->resume(f);
> > + f->func_suspended = false;
> > + }
> > + }
> > +
> > if (value < 0) {
> > ERROR(cdev,
> > "func_suspend() returned error %d\n",
> >
> At individual function driver level there is no need to differentiate
> between suspend() and func_suspend() APIs, as both are intended to put the
> function in suspend state. So your idea/implementation above makes it much
> more clearer. Let composite also handle this and call either f->suspend() or
> f->resume() callback based on the setup packet received. Thank you for this
> suggestion.
>
> >
> > Also, do we need the f->func_suspended flag? we'd need the remote wakeup
> > flag for the status, but when do we need f->func_suspended? It seems
> > like it can be handled within the function driver's scope.
>
> f->func_suspended flag I had added for below purposes
>
> 1.) Function drivers should know the right wakeup() op to be called.
> That is if they are in FUNC_SUSPEND then call usb_func_wakeup() and if they
> are in device suspend then call usb_gadget_wakeup(). (we can use
> f->func_wakeup_armed flag for this purpose as well)
>
> 2.) If a function is in USB3 FUNCTION_SUSPEND state then it shouldn't allow
> f->resume() called through composite_resume() as the exit from
> FUNCTION_SUSPEND state is via ClearFeature(FUNCTION_SUSPEND).
>
> So we need a way to tell function drivers if they are in USB3
> FUNCTION_SUSPEND state OR device suspend.
Ok. So do you plan to update composite_resume to check that? Perhaps
document this expected behavior also?
>
> Please let me know if you see any alternative or better approach here.
>
If we have a use for it in composite.c, then it should be made
available. Otherwise it should only exist in the function driver (ie. in
f_ecm structure).
Thanks,
Thinh
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
2023-03-18 2:26 ` Thinh Nguyen
@ 2023-03-20 17:42 ` Elson Serrao
2023-03-24 0:12 ` Elson Serrao
0 siblings, 1 reply; 19+ messages in thread
From: Elson Serrao @ 2023-03-20 17:42 UTC (permalink / raw)
To: Thinh Nguyen; +Cc: gregkh, linux-kernel, linux-usb, quic_wcheng, quic_jackp
On 3/17/2023 7:26 PM, Thinh Nguyen wrote:
> On Fri, Mar 17, 2023, Elson Serrao wrote:
>>
>>
>> On 3/17/2023 4:20 PM, Thinh Nguyen wrote:
>>> On Fri, Mar 17, 2023, Thinh Nguyen wrote:
>>>> On Fri, Mar 17, 2023, Elson Serrao wrote:
>>>>>
>>>>>
>>>>> On 3/16/2023 5:11 PM, Thinh Nguyen wrote:
>>>>>> On Thu, Mar 16, 2023, Elson Roy Serrao wrote:
>>>>>>> When host sends a suspend notification to the device, handle
>>>>>>> the suspend callbacks in the function driver. Enhanced super
>>>>>>> speed devices can support function suspend feature to put the
>>>>>>> function in suspend state. Handle function suspend callback.
>>>>>>>
>>>>>>> Depending on the remote wakeup capability the device can either
>>>>>>> trigger a remote wakeup or wait for the host initiated resume to
>>>>>>> start data transfer again.
>>>>>>>
>>>>>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>>>>>> ---
>>>>>>> drivers/usb/gadget/function/f_ecm.c | 71 +++++++++++++++++++++++++++++++++++
>>>>>>> drivers/usb/gadget/function/u_ether.c | 63 +++++++++++++++++++++++++++++++
>>>>>>> drivers/usb/gadget/function/u_ether.h | 4 ++
>>>>>>> 3 files changed, 138 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
>>>>>>> index a7ab30e..c43cd557 100644
>>>>>>> --- a/drivers/usb/gadget/function/f_ecm.c
>>>>>>> +++ b/drivers/usb/gadget/function/f_ecm.c
>>>>>>> @@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
>>>>>>> usb_ep_disable(ecm->notify);
>>>>>>> ecm->notify->desc = NULL;
>>>>>>> + f->func_suspended = false;
>>>>>>> + f->func_wakeup_armed = false;
>>>>>>> }
>>>>>>> /*-------------------------------------------------------------------------*/
>>>>>>> @@ -885,6 +887,71 @@ static struct usb_function_instance *ecm_alloc_inst(void)
>>>>>>> return &opts->func_inst;
>>>>>>> }
>>>>>>> +static void ecm_suspend(struct usb_function *f)
>>>>>>> +{
>>>>>>> + struct f_ecm *ecm = func_to_ecm(f);
>>>>>>> + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
>>>>>>> +
>>>>>>> + if (f->func_suspended) {
>>>>>>> + DBG(cdev, "Function already suspended\n");
>>>>>>> + return;
>>>>>>> + }
>>>>>>> +
>>>>>>> + DBG(cdev, "ECM Suspend\n");
>>>>>>> +
>>>>>>> + gether_suspend(&ecm->port);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void ecm_resume(struct usb_function *f)
>>>>>>> +{
>>>>>>> + struct f_ecm *ecm = func_to_ecm(f);
>>>>>>> + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
>>>>>>> +
>>>>>>> + /*
>>>>>>> + * If the function is in USB3 Function Suspend state, resume is
>>>>>>> + * canceled. In this case resume is done by a Function Resume request.
>>>>>>> + */
>>>>>>> + if (f->func_suspended)
>>>>>>> + return;
>>>>>>> +
>>>>>>> + DBG(cdev, "ECM Resume\n");
>>>>>>> +
>>>>>>> + gether_resume(&ecm->port);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int ecm_get_status(struct usb_function *f)
>>>>>>> +{
>>>>>>> + struct usb_configuration *c = f->config;
>>>>>>> +
>>>>>>> + /* D0 and D1 bit set to 0 if device is not wakeup capable */
>>>>>>> + if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
>>>>>>> + return 0;
>>>>>>> +
>>>>>>> + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
>>>>>>> + USB_INTRF_STAT_FUNC_RW_CAP;
>>>>>>> +}
>>>>>>
>>>>>> Why do we need to implement ecm_get_status if it's already handled in
>>>>>> composite.c now?
>>>>>>
>>>>>
>>>>> Yes this can be removed now. Will modify accordingly.
>>>>>>> +
>>>>>>> +static int ecm_func_suspend(struct usb_function *f, u8 options)
>>>>>>> +{
>>>>>>> + struct usb_composite_dev *cdev = f->config->cdev;
>>>>>>> +
>>>>>>> + DBG(cdev, "func susp %u cmd\n", options);
>>>>>>> +
>>>>>>> + if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
>>>>>>
>>>>>> This feature selector doesn't indicate whether it's SetFeature or
>>>>>> ClearFeature request. ecm_func_suspend is supposed to be for
>>>>>> SetFeature(suspend) only. Perhaps we may have to define func_resume()
>>>>>> for ClearFeature(suspend)?
>>>>>>
>>>>
>>>>> Host uses the same feature selector FUNCTION_SUSPEND for function suspend
>>>>> and function resume and func_suspend() callback can be used to
>>>>> handle both the cases ? The distinction comes whether it is a
>>>>
>>>> How do you plan to handle that? Pass this info in some unused/reserved
>>>> bit of the "options" argument? Introduce a new parameter to the
>>>> func_suspend()?
>>>>
>>>> If that's the case, then you need to update the document on
>>>> func_suspend() to also support ClearFeature(suspend). Right now it's
>>>> documented for SetFeature only. Also, make sure that other existing
>>>> function drivers will not break because of the change of the
>>>> func_suspend behavior.
>>>>
>>>>> SetFeature(FUNCTION_SUSPEND) or ClearFeature(FUNCTION_SUSPEND) which can be
>>>>> easily done in the func_suspend callback itself. We can add another callback
>>>>> func_resume specific to ClearFeature(FUNCTION_SUSPEND) but wont that be
>>>>> redundant and more callback handling on function driver/composite side as
>>>>> well? Please let me know your opinion.
>>>>>
>>>>
>>>> We actually didn't properly define func_suspend and its counter part. It
>>>> seems cleaner to me to introduce func_resume as it seems more intuitive
>>>> and easier to read. Let me know how you plan to use func_suspend() for
>>>> both cases.
>>>>
>>>
>>> How about we handle function suspend resume in composite also? I mean
>>> something like this:
>>>
>>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>>> index 36add1879ed2..79dc055eb5f7 100644
>>> --- a/drivers/usb/gadget/composite.c
>>> +++ b/drivers/usb/gadget/composite.c
>>> @@ -1948,9 +1948,18 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>>> f = cdev->config->interface[intf];
>>> if (!f)
>>> break;
>>> - status = f->get_status ? f->get_status(f) : 0;
>>> - if (status < 0)
>>> - break;
>>> +
>>> + if (f->get_status) {
>>> + status = f->get_status(f);
>>> + if (status < 0)
>>> + break;
>>> + } else {
>>> + if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
>>> + status |= USB_INTRF_STAT_FUNC_RW_CAP;
>>> + if (f->func_wakeup_armed)
>>> + status |= USB_INTRF_STAT_FUNC_RW;
>>> + }
>>> + }
>>> put_unaligned_le16(status & 0x0000ffff, req->buf);
>>> break;
>>> /*
>>> @@ -1971,9 +1980,28 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>>> f = cdev->config->interface[intf];
>>> if (!f)
>>> break;
>>> + if (w_index & USB_INTRF_FUNC_SUSPEND_RW) {
>>> + if (!(f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP))
>>> + break;
>>> +
>>> + f->func_wakeup_armed = (ctrl->bRequest == USB_REQ_SET_FEATURE);
>>> + }
>>> +
>>> value = 0;
>>> - if (f->func_suspend)
>>> + if (f->func_suspend) {
>>> value = f->func_suspend(f, w_index >> 8);
>>> + } else if (w_index & USB_INTRF_FUNC_SUSPEND_LP) {
>>> + if (f->suspend && && !f->func_suspended &&
>>> + ctrl->bRequest == USB_REQ_SET_FEATURE)) {
>>> + f->suspend(f);
>>> + f->func_suspended = true;
>>> + } else if (f->resume && f->func_suspended &&
>>> + ctrl->bRequest == USB_REQ_CLEAR_FEATURE_FEATURE)) {
>>> + f->resume(f);
>>> + f->func_suspended = false;
>>> + }
>>> + }
>>> +
>>> if (value < 0) {
>>> ERROR(cdev,
>>> "func_suspend() returned error %d\n",
>>>
>> At individual function driver level there is no need to differentiate
>> between suspend() and func_suspend() APIs, as both are intended to put the
>> function in suspend state. So your idea/implementation above makes it much
>> more clearer. Let composite also handle this and call either f->suspend() or
>> f->resume() callback based on the setup packet received. Thank you for this
>> suggestion.
>>
>>>
>>> Also, do we need the f->func_suspended flag? we'd need the remote wakeup
>>> flag for the status, but when do we need f->func_suspended? It seems
>>> like it can be handled within the function driver's scope.
>>
>> f->func_suspended flag I had added for below purposes
>>
>> 1.) Function drivers should know the right wakeup() op to be called.
>> That is if they are in FUNC_SUSPEND then call usb_func_wakeup() and if they
>> are in device suspend then call usb_gadget_wakeup(). (we can use
>> f->func_wakeup_armed flag for this purpose as well)
>>
>> 2.) If a function is in USB3 FUNCTION_SUSPEND state then it shouldn't allow
>> f->resume() called through composite_resume() as the exit from
>> FUNCTION_SUSPEND state is via ClearFeature(FUNCTION_SUSPEND).
>>
>> So we need a way to tell function drivers if they are in USB3
>> FUNCTION_SUSPEND state OR device suspend.
>
> Ok. So do you plan to update composite_resume to check that? Perhaps
> document this expected behavior also?
>
Yes I will update composite resume to handle this scenario. I was
handling it in f_ecm till now, but based on this new discussion I will
move all handling to composite layer.
>>
>> Please let me know if you see any alternative or better approach here.
>>
>
> If we have a use for it in composite.c, then it should be made
> available. Otherwise it should only exist in the function driver (ie. in
> f_ecm structure).
>
Yes I will keep it for now. Since we are moving all handling to
composite layer better to have some kind of flag that function
drivers/composite layer can use to check if a given function is in
FUNCTION_SUSPEND state or composite_suspend() state.
Thanks
Elson
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
2023-03-20 17:42 ` Elson Serrao
@ 2023-03-24 0:12 ` Elson Serrao
2023-03-24 0:59 ` Thinh Nguyen
0 siblings, 1 reply; 19+ messages in thread
From: Elson Serrao @ 2023-03-24 0:12 UTC (permalink / raw)
To: Thinh Nguyen; +Cc: gregkh, linux-kernel, linux-usb, quic_wcheng, quic_jackp
On 3/20/2023 10:42 AM, Elson Serrao wrote:
>
>
> On 3/17/2023 7:26 PM, Thinh Nguyen wrote:
>> On Fri, Mar 17, 2023, Elson Serrao wrote:
>>>
>>>
>>> On 3/17/2023 4:20 PM, Thinh Nguyen wrote:
>>>> On Fri, Mar 17, 2023, Thinh Nguyen wrote:
>>>>> On Fri, Mar 17, 2023, Elson Serrao wrote:
>>>>>>
>>>>>>
>>>>>> On 3/16/2023 5:11 PM, Thinh Nguyen wrote:
>>>>>>> On Thu, Mar 16, 2023, Elson Roy Serrao wrote:
>>>>>>>> When host sends a suspend notification to the device, handle
>>>>>>>> the suspend callbacks in the function driver. Enhanced super
>>>>>>>> speed devices can support function suspend feature to put the
>>>>>>>> function in suspend state. Handle function suspend callback.
>>>>>>>>
>>>>>>>> Depending on the remote wakeup capability the device can either
>>>>>>>> trigger a remote wakeup or wait for the host initiated resume to
>>>>>>>> start data transfer again.
>>>>>>>>
>>>>>>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>>>>>>> ---
>>>>>>>> drivers/usb/gadget/function/f_ecm.c | 71
>>>>>>>> +++++++++++++++++++++++++++++++++++
>>>>>>>> drivers/usb/gadget/function/u_ether.c | 63
>>>>>>>> +++++++++++++++++++++++++++++++
>>>>>>>> drivers/usb/gadget/function/u_ether.h | 4 ++
>>>>>>>> 3 files changed, 138 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/usb/gadget/function/f_ecm.c
>>>>>>>> b/drivers/usb/gadget/function/f_ecm.c
>>>>>>>> index a7ab30e..c43cd557 100644
>>>>>>>> --- a/drivers/usb/gadget/function/f_ecm.c
>>>>>>>> +++ b/drivers/usb/gadget/function/f_ecm.c
>>>>>>>> @@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
>>>>>>>> usb_ep_disable(ecm->notify);
>>>>>>>> ecm->notify->desc = NULL;
>>>>>>>> + f->func_suspended = false;
>>>>>>>> + f->func_wakeup_armed = false;
>>>>>>>> }
>>>>>>>>
>>>>>>>> /*-------------------------------------------------------------------------*/
>>>>>>>>
>>>>>>>> @@ -885,6 +887,71 @@ static struct usb_function_instance
>>>>>>>> *ecm_alloc_inst(void)
>>>>>>>> return &opts->func_inst;
>>>>>>>> }
>>>>>>>> +static void ecm_suspend(struct usb_function *f)
>>>>>>>> +{
>>>>>>>> + struct f_ecm *ecm = func_to_ecm(f);
>>>>>>>> + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
>>>>>>>> +
>>>>>>>> + if (f->func_suspended) {
>>>>>>>> + DBG(cdev, "Function already suspended\n");
>>>>>>>> + return;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + DBG(cdev, "ECM Suspend\n");
>>>>>>>> +
>>>>>>>> + gether_suspend(&ecm->port);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static void ecm_resume(struct usb_function *f)
>>>>>>>> +{
>>>>>>>> + struct f_ecm *ecm = func_to_ecm(f);
>>>>>>>> + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
>>>>>>>> +
>>>>>>>> + /*
>>>>>>>> + * If the function is in USB3 Function Suspend state,
>>>>>>>> resume is
>>>>>>>> + * canceled. In this case resume is done by a Function
>>>>>>>> Resume request.
>>>>>>>> + */
>>>>>>>> + if (f->func_suspended)
>>>>>>>> + return;
>>>>>>>> +
>>>>>>>> + DBG(cdev, "ECM Resume\n");
>>>>>>>> +
>>>>>>>> + gether_resume(&ecm->port);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int ecm_get_status(struct usb_function *f)
>>>>>>>> +{
>>>>>>>> + struct usb_configuration *c = f->config;
>>>>>>>> +
>>>>>>>> + /* D0 and D1 bit set to 0 if device is not wakeup capable */
>>>>>>>> + if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
>>>>>>>> + return 0;
>>>>>>>> +
>>>>>>>> + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
>>>>>>>> + USB_INTRF_STAT_FUNC_RW_CAP;
>>>>>>>> +}
>>>>>>>
>>>>>>> Why do we need to implement ecm_get_status if it's already
>>>>>>> handled in
>>>>>>> composite.c now?
>>>>>>>
>>>>>>
>>>>>> Yes this can be removed now. Will modify accordingly.
>>>>>>>> +
>>>>>>>> +static int ecm_func_suspend(struct usb_function *f, u8 options)
>>>>>>>> +{
>>>>>>>> + struct usb_composite_dev *cdev = f->config->cdev;
>>>>>>>> +
>>>>>>>> + DBG(cdev, "func susp %u cmd\n", options);
>>>>>>>> +
>>>>>>>> + if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
>>>>>>>
>>>>>>> This feature selector doesn't indicate whether it's SetFeature or
>>>>>>> ClearFeature request. ecm_func_suspend is supposed to be for
>>>>>>> SetFeature(suspend) only. Perhaps we may have to define
>>>>>>> func_resume()
>>>>>>> for ClearFeature(suspend)?
>>>>>>>
>>>>>
>>>>>> Host uses the same feature selector FUNCTION_SUSPEND for function
>>>>>> suspend
>>>>>> and function resume and func_suspend() callback can be used to
>>>>>> handle both the cases ? The distinction comes whether it is a
>>>>>
>>>>> How do you plan to handle that? Pass this info in some unused/reserved
>>>>> bit of the "options" argument? Introduce a new parameter to the
>>>>> func_suspend()?
>>>>>
>>>>> If that's the case, then you need to update the document on
>>>>> func_suspend() to also support ClearFeature(suspend). Right now it's
>>>>> documented for SetFeature only. Also, make sure that other existing
>>>>> function drivers will not break because of the change of the
>>>>> func_suspend behavior.
>>>>>
>>>>>> SetFeature(FUNCTION_SUSPEND) or ClearFeature(FUNCTION_SUSPEND)
>>>>>> which can be
>>>>>> easily done in the func_suspend callback itself. We can add
>>>>>> another callback
>>>>>> func_resume specific to ClearFeature(FUNCTION_SUSPEND) but wont
>>>>>> that be
>>>>>> redundant and more callback handling on function driver/composite
>>>>>> side as
>>>>>> well? Please let me know your opinion.
>>>>>>
>>>>>
>>>>> We actually didn't properly define func_suspend and its counter
>>>>> part. It
>>>>> seems cleaner to me to introduce func_resume as it seems more
>>>>> intuitive
>>>>> and easier to read. Let me know how you plan to use func_suspend() for
>>>>> both cases.
>>>>>
>>>>
>>>> How about we handle function suspend resume in composite also? I mean
>>>> something like this:
>>>>
>>>> diff --git a/drivers/usb/gadget/composite.c
>>>> b/drivers/usb/gadget/composite.c
>>>> index 36add1879ed2..79dc055eb5f7 100644
>>>> --- a/drivers/usb/gadget/composite.c
>>>> +++ b/drivers/usb/gadget/composite.c
>>>> @@ -1948,9 +1948,18 @@ composite_setup(struct usb_gadget *gadget,
>>>> const struct usb_ctrlrequest *ctrl)
>>>> f = cdev->config->interface[intf];
>>>> if (!f)
>>>> break;
>>>> - status = f->get_status ? f->get_status(f) : 0;
>>>> - if (status < 0)
>>>> - break;
>>>> +
>>>> + if (f->get_status) {
>>>> + status = f->get_status(f);
>>>> + if (status < 0)
>>>> + break;
>>>> + } else {
>>>> + if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
>>>> + status |= USB_INTRF_STAT_FUNC_RW_CAP;
>>>> + if (f->func_wakeup_armed)
>>>> + status |= USB_INTRF_STAT_FUNC_RW;
>>>> + }
>>>> + }
>>>> put_unaligned_le16(status & 0x0000ffff, req->buf);
>>>> break;
>>>> /*
>>>> @@ -1971,9 +1980,28 @@ composite_setup(struct usb_gadget *gadget,
>>>> const struct usb_ctrlrequest *ctrl)
>>>> f = cdev->config->interface[intf];
>>>> if (!f)
>>>> break;
>>>> + if (w_index & USB_INTRF_FUNC_SUSPEND_RW) {
>>>> + if (!(f->config->bmAttributes &
>>>> USB_CONFIG_ATT_WAKEUP))
>>>> + break;
>>>> +
>>>> + f->func_wakeup_armed = (ctrl->bRequest ==
>>>> USB_REQ_SET_FEATURE);
>>>> + }
>>>> +
>>>> value = 0;
>>>> - if (f->func_suspend)
>>>> + if (f->func_suspend) {
>>>> value = f->func_suspend(f, w_index >> 8);
>>>> + } else if (w_index & USB_INTRF_FUNC_SUSPEND_LP) {
>>>> + if (f->suspend && && !f->func_suspended &&
>>>> + ctrl->bRequest == USB_REQ_SET_FEATURE)) {
>>>> + f->suspend(f);
>>>> + f->func_suspended = true;
>>>> + } else if (f->resume && f->func_suspended &&
>>>> + ctrl->bRequest ==
>>>> USB_REQ_CLEAR_FEATURE_FEATURE)) {
Linux based hosts, dont use ClearFeature(FUNCTION_SUSPEND) for function
resume. They use SetFeature(FUNCTION_SUSPEND) itself but with Bit(0) i.e
USB_INTRF_FUNC_SUSPEND_LP reset. So we may not be able to distinguish
based on SET_FEATURE and CLEAR_FEATURE packets for function suspend and
function resume. Instead we can generalize (w_index &
USB_INTRF_FUNC_SUSPEND_LP) for function suspend and all other cases for
function resume.
if (f->func_suspend) {
value = f->func_suspend(f, w_index >> 8);
} else if (w_index & USB_INTRF_FUNC_SUSPEND_LP) {
if (f->suspend && !f->func_suspended) {
f->suspend(f);
f->func_suspended = true;
}
} else {
if (f->resume && f->func_suspended) {
f->resume(f);
f->func_suspended = false;
}
}
Thanks
Elson
>>>> + f->resume(f);
>>>> + f->func_suspended = false;
>>>> + }
>>>> + }
>>>> +
>>>> if (value < 0) {
>>>> ERROR(cdev,
>>>> "func_suspend() returned error %d\n",
>>>>
>>> At individual function driver level there is no need to differentiate
>>> between suspend() and func_suspend() APIs, as both are intended to
>>> put the
>>> function in suspend state. So your idea/implementation above makes it
>>> much
>>> more clearer. Let composite also handle this and call either
>>> f->suspend() or
>>> f->resume() callback based on the setup packet received. Thank you
>>> for this
>>> suggestion.
>>>
>>>>
>>>> Also, do we need the f->func_suspended flag? we'd need the remote
>>>> wakeup
>>>> flag for the status, but when do we need f->func_suspended? It seems
>>>> like it can be handled within the function driver's scope.
>>>
>>> f->func_suspended flag I had added for below purposes
>>>
>>> 1.) Function drivers should know the right wakeup() op to be called.
>>> That is if they are in FUNC_SUSPEND then call usb_func_wakeup() and
>>> if they
>>> are in device suspend then call usb_gadget_wakeup(). (we can use
>>> f->func_wakeup_armed flag for this purpose as well)
>>>
>>> 2.) If a function is in USB3 FUNCTION_SUSPEND state then it shouldn't
>>> allow
>>> f->resume() called through composite_resume() as the exit from
>>> FUNCTION_SUSPEND state is via ClearFeature(FUNCTION_SUSPEND).
>>>
>>> So we need a way to tell function drivers if they are in USB3
>>> FUNCTION_SUSPEND state OR device suspend.
>>
>> Ok. So do you plan to update composite_resume to check that? Perhaps
>> document this expected behavior also?
>>
> Yes I will update composite resume to handle this scenario. I was
> handling it in f_ecm till now, but based on this new discussion I will
> move all handling to composite layer.
>>>
>>> Please let me know if you see any alternative or better approach here.
>>>
>>
>> If we have a use for it in composite.c, then it should be made
>> available. Otherwise it should only exist in the function driver (ie. in
>> f_ecm structure).
>>
> Yes I will keep it for now. Since we are moving all handling to
> composite layer better to have some kind of flag that function
> drivers/composite layer can use to check if a given function is in
> FUNCTION_SUSPEND state or composite_suspend() state.
>
> Thanks
> Elson
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
2023-03-24 0:12 ` Elson Serrao
@ 2023-03-24 0:59 ` Thinh Nguyen
2023-03-24 1:14 ` Thinh Nguyen
0 siblings, 1 reply; 19+ messages in thread
From: Thinh Nguyen @ 2023-03-24 0:59 UTC (permalink / raw)
To: Elson Serrao
Cc: Thinh Nguyen, gregkh, linux-kernel, linux-usb, quic_wcheng, quic_jackp
On Thu, Mar 23, 2023, Elson Serrao wrote:
>
>
> On 3/20/2023 10:42 AM, Elson Serrao wrote:
> >
> >
> > On 3/17/2023 7:26 PM, Thinh Nguyen wrote:
> > > On Fri, Mar 17, 2023, Elson Serrao wrote:
> > > >
> > > >
> > > > On 3/17/2023 4:20 PM, Thinh Nguyen wrote:
> > > > > On Fri, Mar 17, 2023, Thinh Nguyen wrote:
> > > > > > On Fri, Mar 17, 2023, Elson Serrao wrote:
> > > > > > >
> > > > > > >
> > > > > > > On 3/16/2023 5:11 PM, Thinh Nguyen wrote:
> > > > > > > > On Thu, Mar 16, 2023, Elson Roy Serrao wrote:
> > > > > > > > > When host sends a suspend notification to the device, handle
> > > > > > > > > the suspend callbacks in the function driver. Enhanced super
> > > > > > > > > speed devices can support function suspend feature to put the
> > > > > > > > > function in suspend state. Handle function suspend callback.
> > > > > > > > >
> > > > > > > > > Depending on the remote wakeup capability the device can either
> > > > > > > > > trigger a remote wakeup or wait for the host initiated resume to
> > > > > > > > > start data transfer again.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> > > > > > > > > ---
> > > > > > > > > drivers/usb/gadget/function/f_ecm.c |
> > > > > > > > > 71 +++++++++++++++++++++++++++++++++++
> > > > > > > > > drivers/usb/gadget/function/u_ether.c |
> > > > > > > > > 63 +++++++++++++++++++++++++++++++
> > > > > > > > > drivers/usb/gadget/function/u_ether.h | 4 ++
> > > > > > > > > 3 files changed, 138 insertions(+)
> > > > > > > > >
> > > > > > > > > diff --git
> > > > > > > > > a/drivers/usb/gadget/function/f_ecm.c
> > > > > > > > > b/drivers/usb/gadget/function/f_ecm.c
> > > > > > > > > index a7ab30e..c43cd557 100644
> > > > > > > > > --- a/drivers/usb/gadget/function/f_ecm.c
> > > > > > > > > +++ b/drivers/usb/gadget/function/f_ecm.c
> > > > > > > > > @@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
> > > > > > > > > usb_ep_disable(ecm->notify);
> > > > > > > > > ecm->notify->desc = NULL;
> > > > > > > > > + f->func_suspended = false;
> > > > > > > > > + f->func_wakeup_armed = false;
> > > > > > > > > }
> > > > > > > > > /*-------------------------------------------------------------------------*/
> > > > > > > > >
> > > > > > > > > @@ -885,6 +887,71 @@ static struct
> > > > > > > > > usb_function_instance *ecm_alloc_inst(void)
> > > > > > > > > return &opts->func_inst;
> > > > > > > > > }
> > > > > > > > > +static void ecm_suspend(struct usb_function *f)
> > > > > > > > > +{
> > > > > > > > > + struct f_ecm *ecm = func_to_ecm(f);
> > > > > > > > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> > > > > > > > > +
> > > > > > > > > + if (f->func_suspended) {
> > > > > > > > > + DBG(cdev, "Function already suspended\n");
> > > > > > > > > + return;
> > > > > > > > > + }
> > > > > > > > > +
> > > > > > > > > + DBG(cdev, "ECM Suspend\n");
> > > > > > > > > +
> > > > > > > > > + gether_suspend(&ecm->port);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static void ecm_resume(struct usb_function *f)
> > > > > > > > > +{
> > > > > > > > > + struct f_ecm *ecm = func_to_ecm(f);
> > > > > > > > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> > > > > > > > > +
> > > > > > > > > + /*
> > > > > > > > > + * If the function is in USB3 Function
> > > > > > > > > Suspend state, resume is
> > > > > > > > > + * canceled. In this case resume is
> > > > > > > > > done by a Function Resume request.
> > > > > > > > > + */
> > > > > > > > > + if (f->func_suspended)
> > > > > > > > > + return;
> > > > > > > > > +
> > > > > > > > > + DBG(cdev, "ECM Resume\n");
> > > > > > > > > +
> > > > > > > > > + gether_resume(&ecm->port);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static int ecm_get_status(struct usb_function *f)
> > > > > > > > > +{
> > > > > > > > > + struct usb_configuration *c = f->config;
> > > > > > > > > +
> > > > > > > > > + /* D0 and D1 bit set to 0 if device is not wakeup capable */
> > > > > > > > > + if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
> > > > > > > > > + return 0;
> > > > > > > > > +
> > > > > > > > > + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
> > > > > > > > > + USB_INTRF_STAT_FUNC_RW_CAP;
> > > > > > > > > +}
> > > > > > > >
> > > > > > > > Why do we need to implement ecm_get_status if
> > > > > > > > it's already handled in
> > > > > > > > composite.c now?
> > > > > > > >
> > > > > > >
> > > > > > > Yes this can be removed now. Will modify accordingly.
> > > > > > > > > +
> > > > > > > > > +static int ecm_func_suspend(struct usb_function *f, u8 options)
> > > > > > > > > +{
> > > > > > > > > + struct usb_composite_dev *cdev = f->config->cdev;
> > > > > > > > > +
> > > > > > > > > + DBG(cdev, "func susp %u cmd\n", options);
> > > > > > > > > +
> > > > > > > > > + if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
> > > > > > > >
> > > > > > > > This feature selector doesn't indicate whether it's SetFeature or
> > > > > > > > ClearFeature request. ecm_func_suspend is supposed to be for
> > > > > > > > SetFeature(suspend) only. Perhaps we may have to
> > > > > > > > define func_resume()
> > > > > > > > for ClearFeature(suspend)?
> > > > > > > >
> > > > > >
> > > > > > > Host uses the same feature selector FUNCTION_SUSPEND
> > > > > > > for function suspend
> > > > > > > and function resume and func_suspend() callback can be used to
> > > > > > > handle both the cases ? The distinction comes whether it is a
> > > > > >
> > > > > > How do you plan to handle that? Pass this info in some unused/reserved
> > > > > > bit of the "options" argument? Introduce a new parameter to the
> > > > > > func_suspend()?
> > > > > >
> > > > > > If that's the case, then you need to update the document on
> > > > > > func_suspend() to also support ClearFeature(suspend). Right now it's
> > > > > > documented for SetFeature only. Also, make sure that other existing
> > > > > > function drivers will not break because of the change of the
> > > > > > func_suspend behavior.
> > > > > >
> > > > > > > SetFeature(FUNCTION_SUSPEND) or
> > > > > > > ClearFeature(FUNCTION_SUSPEND) which can be
> > > > > > > easily done in the func_suspend callback itself. We
> > > > > > > can add another callback
> > > > > > > func_resume specific to
> > > > > > > ClearFeature(FUNCTION_SUSPEND) but wont that be
> > > > > > > redundant and more callback handling on function
> > > > > > > driver/composite side as
> > > > > > > well? Please let me know your opinion.
> > > > > > >
> > > > > >
> > > > > > We actually didn't properly define func_suspend and its
> > > > > > counter part. It
> > > > > > seems cleaner to me to introduce func_resume as it seems
> > > > > > more intuitive
> > > > > > and easier to read. Let me know how you plan to use func_suspend() for
> > > > > > both cases.
> > > > > >
> > > > >
> > > > > How about we handle function suspend resume in composite also? I mean
> > > > > something like this:
> > > > >
> > > > > diff --git a/drivers/usb/gadget/composite.c
> > > > > b/drivers/usb/gadget/composite.c
> > > > > index 36add1879ed2..79dc055eb5f7 100644
> > > > > --- a/drivers/usb/gadget/composite.c
> > > > > +++ b/drivers/usb/gadget/composite.c
> > > > > @@ -1948,9 +1948,18 @@ composite_setup(struct usb_gadget
> > > > > *gadget, const struct usb_ctrlrequest *ctrl)
> > > > > f = cdev->config->interface[intf];
> > > > > if (!f)
> > > > > break;
> > > > > - status = f->get_status ? f->get_status(f) : 0;
> > > > > - if (status < 0)
> > > > > - break;
> > > > > +
> > > > > + if (f->get_status) {
> > > > > + status = f->get_status(f);
> > > > > + if (status < 0)
> > > > > + break;
> > > > > + } else {
> > > > > + if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
> > > > > + status |= USB_INTRF_STAT_FUNC_RW_CAP;
> > > > > + if (f->func_wakeup_armed)
> > > > > + status |= USB_INTRF_STAT_FUNC_RW;
> > > > > + }
> > > > > + }
> > > > > put_unaligned_le16(status & 0x0000ffff, req->buf);
> > > > > break;
> > > > > /*
> > > > > @@ -1971,9 +1980,28 @@ composite_setup(struct usb_gadget
> > > > > *gadget, const struct usb_ctrlrequest *ctrl)
> > > > > f = cdev->config->interface[intf];
> > > > > if (!f)
> > > > > break;
> > > > > + if (w_index & USB_INTRF_FUNC_SUSPEND_RW) {
> > > > > + if (!(f->config->bmAttributes &
> > > > > USB_CONFIG_ATT_WAKEUP))
> > > > > + break;
> > > > > +
> > > > > + f->func_wakeup_armed = (ctrl->bRequest ==
> > > > > USB_REQ_SET_FEATURE);
> > > > > + }
> > > > > +
> > > > > value = 0;
> > > > > - if (f->func_suspend)
> > > > > + if (f->func_suspend) {
> > > > > value = f->func_suspend(f, w_index >> 8);
> > > > > + } else if (w_index & USB_INTRF_FUNC_SUSPEND_LP) {
> > > > > + if (f->suspend && && !f->func_suspended &&
> > > > > + ctrl->bRequest == USB_REQ_SET_FEATURE)) {
> > > > > + f->suspend(f);
> > > > > + f->func_suspended = true;
> > > > > + } else if (f->resume && f->func_suspended &&
> > > > > + ctrl->bRequest ==
> > > > > USB_REQ_CLEAR_FEATURE_FEATURE)) {
>
> Linux based hosts, dont use ClearFeature(FUNCTION_SUSPEND) for function
> resume. They use SetFeature(FUNCTION_SUSPEND) itself but with Bit(0) i.e
> USB_INTRF_FUNC_SUSPEND_LP reset. So we may not be able to distinguish based
> on SET_FEATURE and CLEAR_FEATURE packets for function suspend and function
> resume. Instead we can generalize (w_index & USB_INTRF_FUNC_SUSPEND_LP) for
> function suspend and all other cases for function resume.
>
> if (f->func_suspend) {
> value = f->func_suspend(f, w_index >> 8);
> } else if (w_index & USB_INTRF_FUNC_SUSPEND_LP) {
> if (f->suspend && !f->func_suspended) {
> f->suspend(f);
> f->func_suspended = true;
> }
> } else {
> if (f->resume && f->func_suspended) {
> f->resume(f);
> f->func_suspended = false;
> }
> }
>
>
Ah.. right. That's possible also. Then can we check for both cases?
Something like this:
@ -1994,8 +2003,34 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
if (!f)
break;
value = 0;
- if (f->func_suspend)
+ if (f->func_suspend) {
value = f->func_suspend(f, w_index >> 8);
+ } else if (ctrl->bRequest == USB_REQ_SET_FEATURE) {
+ if (!(f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) &&
+ (w_index & USB_INTRF_FUNC_SUSPEND_RW))
+ break;
+
+ f->func_wakeup_armed = !!(w_index & USB_INTRF_FUNC_SUSPEND_RW);
+
+ if ((w_index & USB_INTRF_FUNC_SUSPEND_LP) &&
+ f->suspend && !f->func_suspended) {
+ f->suspend(f);
+ f->func_suspended = true;
+ } else if (f->resume && f->func_suspended) {
+ f->resume(f);
+ f->func_suspended = false;
+ }
+ } else if (ctrl->bRequest == USB_REQ_CLEAR_FEATURE_FEATURE)) {
+ if (w_index & USB_INTRF_FUNC_SUSPEND_RW)
+ f->func_wakeup_armed = false;
+
+ if ((w_index & USB_INTRF_FUNC_SUSPEND_LP) &&
+ f->resume && f->func_suspended) {
+ f->resume(f);
+ f->func_suspended = false;
+ }
+ }
+
if (value < 0) {
ERROR(cdev,
"func_suspend() returned error %d\n",
Thanks,
Thinh
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
2023-03-24 0:59 ` Thinh Nguyen
@ 2023-03-24 1:14 ` Thinh Nguyen
0 siblings, 0 replies; 19+ messages in thread
From: Thinh Nguyen @ 2023-03-24 1:14 UTC (permalink / raw)
To: Elson Serrao; +Cc: gregkh, linux-kernel, linux-usb, quic_wcheng, quic_jackp
On Fri, Mar 24, 2023, Thinh Nguyen wrote:
> On Thu, Mar 23, 2023, Elson Serrao wrote:
> >
> >
> > On 3/20/2023 10:42 AM, Elson Serrao wrote:
> > >
> > >
> > > On 3/17/2023 7:26 PM, Thinh Nguyen wrote:
> > > > On Fri, Mar 17, 2023, Elson Serrao wrote:
> > > > >
> > > > >
> > > > > On 3/17/2023 4:20 PM, Thinh Nguyen wrote:
> > > > > > On Fri, Mar 17, 2023, Thinh Nguyen wrote:
> > > > > > > On Fri, Mar 17, 2023, Elson Serrao wrote:
> > > > > > > >
> > > > > > > >
> > > > > > > > On 3/16/2023 5:11 PM, Thinh Nguyen wrote:
> > > > > > > > > On Thu, Mar 16, 2023, Elson Roy Serrao wrote:
> > > > > > > > > > When host sends a suspend notification to the device, handle
> > > > > > > > > > the suspend callbacks in the function driver. Enhanced super
> > > > > > > > > > speed devices can support function suspend feature to put the
> > > > > > > > > > function in suspend state. Handle function suspend callback.
> > > > > > > > > >
> > > > > > > > > > Depending on the remote wakeup capability the device can either
> > > > > > > > > > trigger a remote wakeup or wait for the host initiated resume to
> > > > > > > > > > start data transfer again.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
> > > > > > > > > > ---
> > > > > > > > > > drivers/usb/gadget/function/f_ecm.c |
> > > > > > > > > > 71 +++++++++++++++++++++++++++++++++++
> > > > > > > > > > drivers/usb/gadget/function/u_ether.c |
> > > > > > > > > > 63 +++++++++++++++++++++++++++++++
> > > > > > > > > > drivers/usb/gadget/function/u_ether.h | 4 ++
> > > > > > > > > > 3 files changed, 138 insertions(+)
> > > > > > > > > >
> > > > > > > > > > diff --git
> > > > > > > > > > a/drivers/usb/gadget/function/f_ecm.c
> > > > > > > > > > b/drivers/usb/gadget/function/f_ecm.c
> > > > > > > > > > index a7ab30e..c43cd557 100644
> > > > > > > > > > --- a/drivers/usb/gadget/function/f_ecm.c
> > > > > > > > > > +++ b/drivers/usb/gadget/function/f_ecm.c
> > > > > > > > > > @@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
> > > > > > > > > > usb_ep_disable(ecm->notify);
> > > > > > > > > > ecm->notify->desc = NULL;
> > > > > > > > > > + f->func_suspended = false;
> > > > > > > > > > + f->func_wakeup_armed = false;
> > > > > > > > > > }
> > > > > > > > > > /*-------------------------------------------------------------------------*/
> > > > > > > > > >
> > > > > > > > > > @@ -885,6 +887,71 @@ static struct
> > > > > > > > > > usb_function_instance *ecm_alloc_inst(void)
> > > > > > > > > > return &opts->func_inst;
> > > > > > > > > > }
> > > > > > > > > > +static void ecm_suspend(struct usb_function *f)
> > > > > > > > > > +{
> > > > > > > > > > + struct f_ecm *ecm = func_to_ecm(f);
> > > > > > > > > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> > > > > > > > > > +
> > > > > > > > > > + if (f->func_suspended) {
> > > > > > > > > > + DBG(cdev, "Function already suspended\n");
> > > > > > > > > > + return;
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > + DBG(cdev, "ECM Suspend\n");
> > > > > > > > > > +
> > > > > > > > > > + gether_suspend(&ecm->port);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static void ecm_resume(struct usb_function *f)
> > > > > > > > > > +{
> > > > > > > > > > + struct f_ecm *ecm = func_to_ecm(f);
> > > > > > > > > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
> > > > > > > > > > +
> > > > > > > > > > + /*
> > > > > > > > > > + * If the function is in USB3 Function
> > > > > > > > > > Suspend state, resume is
> > > > > > > > > > + * canceled. In this case resume is
> > > > > > > > > > done by a Function Resume request.
> > > > > > > > > > + */
> > > > > > > > > > + if (f->func_suspended)
> > > > > > > > > > + return;
> > > > > > > > > > +
> > > > > > > > > > + DBG(cdev, "ECM Resume\n");
> > > > > > > > > > +
> > > > > > > > > > + gether_resume(&ecm->port);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static int ecm_get_status(struct usb_function *f)
> > > > > > > > > > +{
> > > > > > > > > > + struct usb_configuration *c = f->config;
> > > > > > > > > > +
> > > > > > > > > > + /* D0 and D1 bit set to 0 if device is not wakeup capable */
> > > > > > > > > > + if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
> > > > > > > > > > + return 0;
> > > > > > > > > > +
> > > > > > > > > > + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
> > > > > > > > > > + USB_INTRF_STAT_FUNC_RW_CAP;
> > > > > > > > > > +}
> > > > > > > > >
> > > > > > > > > Why do we need to implement ecm_get_status if
> > > > > > > > > it's already handled in
> > > > > > > > > composite.c now?
> > > > > > > > >
> > > > > > > >
> > > > > > > > Yes this can be removed now. Will modify accordingly.
> > > > > > > > > > +
> > > > > > > > > > +static int ecm_func_suspend(struct usb_function *f, u8 options)
> > > > > > > > > > +{
> > > > > > > > > > + struct usb_composite_dev *cdev = f->config->cdev;
> > > > > > > > > > +
> > > > > > > > > > + DBG(cdev, "func susp %u cmd\n", options);
> > > > > > > > > > +
> > > > > > > > > > + if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
> > > > > > > > >
> > > > > > > > > This feature selector doesn't indicate whether it's SetFeature or
> > > > > > > > > ClearFeature request. ecm_func_suspend is supposed to be for
> > > > > > > > > SetFeature(suspend) only. Perhaps we may have to
> > > > > > > > > define func_resume()
> > > > > > > > > for ClearFeature(suspend)?
> > > > > > > > >
> > > > > > >
> > > > > > > > Host uses the same feature selector FUNCTION_SUSPEND
> > > > > > > > for function suspend
> > > > > > > > and function resume and func_suspend() callback can be used to
> > > > > > > > handle both the cases ? The distinction comes whether it is a
> > > > > > >
> > > > > > > How do you plan to handle that? Pass this info in some unused/reserved
> > > > > > > bit of the "options" argument? Introduce a new parameter to the
> > > > > > > func_suspend()?
> > > > > > >
> > > > > > > If that's the case, then you need to update the document on
> > > > > > > func_suspend() to also support ClearFeature(suspend). Right now it's
> > > > > > > documented for SetFeature only. Also, make sure that other existing
> > > > > > > function drivers will not break because of the change of the
> > > > > > > func_suspend behavior.
> > > > > > >
> > > > > > > > SetFeature(FUNCTION_SUSPEND) or
> > > > > > > > ClearFeature(FUNCTION_SUSPEND) which can be
> > > > > > > > easily done in the func_suspend callback itself. We
> > > > > > > > can add another callback
> > > > > > > > func_resume specific to
> > > > > > > > ClearFeature(FUNCTION_SUSPEND) but wont that be
> > > > > > > > redundant and more callback handling on function
> > > > > > > > driver/composite side as
> > > > > > > > well? Please let me know your opinion.
> > > > > > > >
> > > > > > >
> > > > > > > We actually didn't properly define func_suspend and its
> > > > > > > counter part. It
> > > > > > > seems cleaner to me to introduce func_resume as it seems
> > > > > > > more intuitive
> > > > > > > and easier to read. Let me know how you plan to use func_suspend() for
> > > > > > > both cases.
> > > > > > >
> > > > > >
> > > > > > How about we handle function suspend resume in composite also? I mean
> > > > > > something like this:
> > > > > >
> > > > > > diff --git a/drivers/usb/gadget/composite.c
> > > > > > b/drivers/usb/gadget/composite.c
> > > > > > index 36add1879ed2..79dc055eb5f7 100644
> > > > > > --- a/drivers/usb/gadget/composite.c
> > > > > > +++ b/drivers/usb/gadget/composite.c
> > > > > > @@ -1948,9 +1948,18 @@ composite_setup(struct usb_gadget
> > > > > > *gadget, const struct usb_ctrlrequest *ctrl)
> > > > > > f = cdev->config->interface[intf];
> > > > > > if (!f)
> > > > > > break;
> > > > > > - status = f->get_status ? f->get_status(f) : 0;
> > > > > > - if (status < 0)
> > > > > > - break;
> > > > > > +
> > > > > > + if (f->get_status) {
> > > > > > + status = f->get_status(f);
> > > > > > + if (status < 0)
> > > > > > + break;
> > > > > > + } else {
> > > > > > + if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
> > > > > > + status |= USB_INTRF_STAT_FUNC_RW_CAP;
> > > > > > + if (f->func_wakeup_armed)
> > > > > > + status |= USB_INTRF_STAT_FUNC_RW;
> > > > > > + }
> > > > > > + }
> > > > > > put_unaligned_le16(status & 0x0000ffff, req->buf);
> > > > > > break;
> > > > > > /*
> > > > > > @@ -1971,9 +1980,28 @@ composite_setup(struct usb_gadget
> > > > > > *gadget, const struct usb_ctrlrequest *ctrl)
> > > > > > f = cdev->config->interface[intf];
> > > > > > if (!f)
> > > > > > break;
> > > > > > + if (w_index & USB_INTRF_FUNC_SUSPEND_RW) {
> > > > > > + if (!(f->config->bmAttributes &
> > > > > > USB_CONFIG_ATT_WAKEUP))
> > > > > > + break;
> > > > > > +
> > > > > > + f->func_wakeup_armed = (ctrl->bRequest ==
> > > > > > USB_REQ_SET_FEATURE);
> > > > > > + }
> > > > > > +
> > > > > > value = 0;
> > > > > > - if (f->func_suspend)
> > > > > > + if (f->func_suspend) {
> > > > > > value = f->func_suspend(f, w_index >> 8);
> > > > > > + } else if (w_index & USB_INTRF_FUNC_SUSPEND_LP) {
> > > > > > + if (f->suspend && && !f->func_suspended &&
> > > > > > + ctrl->bRequest == USB_REQ_SET_FEATURE)) {
> > > > > > + f->suspend(f);
> > > > > > + f->func_suspended = true;
> > > > > > + } else if (f->resume && f->func_suspended &&
> > > > > > + ctrl->bRequest ==
> > > > > > USB_REQ_CLEAR_FEATURE_FEATURE)) {
> >
> > Linux based hosts, dont use ClearFeature(FUNCTION_SUSPEND) for function
> > resume. They use SetFeature(FUNCTION_SUSPEND) itself but with Bit(0) i.e
> > USB_INTRF_FUNC_SUSPEND_LP reset. So we may not be able to distinguish based
> > on SET_FEATURE and CLEAR_FEATURE packets for function suspend and function
> > resume. Instead we can generalize (w_index & USB_INTRF_FUNC_SUSPEND_LP) for
> > function suspend and all other cases for function resume.
> >
> > if (f->func_suspend) {
> > value = f->func_suspend(f, w_index >> 8);
> > } else if (w_index & USB_INTRF_FUNC_SUSPEND_LP) {
> > if (f->suspend && !f->func_suspended) {
> > f->suspend(f);
> > f->func_suspended = true;
> > }
> > } else {
> > if (f->resume && f->func_suspended) {
> > f->resume(f);
> > f->func_suspended = false;
> > }
> > }
> >
> >
>
> Ah.. right. That's possible also. Then can we check for both cases?
> Something like this:
>
Correction, ClearFeature clears the entire suspend option (remote wake +
suspend)
@@ -1994,8 +2003,32 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
if (!f)
break;
value = 0;
- if (f->func_suspend)
+ if (f->func_suspend) {
value = f->func_suspend(f, w_index >> 8);
+ } else if (ctrl->bRequest == USB_REQ_SET_FEATURE) {
+ if (!(f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) &&
+ (w_index & USB_INTRF_FUNC_SUSPEND_RW))
+ break;
+
+ f->func_wakeup_armed = !!(w_index & USB_INTRF_FUNC_SUSPEND_RW);
+
+ if ((w_index & USB_INTRF_FUNC_SUSPEND_LP) &&
+ f->suspend && !f->func_suspended) {
+ f->suspend(f);
+ f->func_suspended = true;
+ } else if (f->resume && f->func_suspended) {
+ f->resume(f);
+ f->func_suspended = false;
+ }
+ } else if (ctrl->bRequest == USB_REQ_CLEAR_FEATURE_FEATURE)) {
+ f->func_wakeup_armed = false;
+
+ if (f->resume && f->func_suspended) {
+ f->resume(f);
+ f->func_suspended = false;
+ }
+ }
+
if (value < 0) {
ERROR(cdev,
"func_suspend() returned error %d\n",
Something like that.
Thanks,
Thinh
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
2023-03-16 23:38 ` [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support Elson Roy Serrao
2023-03-17 0:11 ` Thinh Nguyen
@ 2023-03-21 8:12 ` Dan Carpenter
1 sibling, 0 replies; 19+ messages in thread
From: Dan Carpenter @ 2023-03-21 8:12 UTC (permalink / raw)
To: oe-kbuild, Elson Roy Serrao, gregkh, Thinh.Nguyen
Cc: lkp, oe-kbuild-all, linux-kernel, linux-usb, quic_wcheng,
quic_jackp, Elson Roy Serrao
Hi Elson,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Elson-Roy-Serrao/usb-gadget-Properly-configure-the-device-for-remote-wakeup/20230317-074030
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/1679009888-8239-7-git-send-email-quic_eserrao%40quicinc.com
patch subject: [PATCH v12 6/6] usb: gadget: f_ecm: Add suspend/resume and remote wakeup support
config: riscv-randconfig-m031-20230319 (https://download.01.org/0day-ci/archive/20230321/202303211515.XaO8YKCz-lkp@intel.com/config)
compiler: riscv32-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Link: https://lore.kernel.org/r/202303211515.XaO8YKCz-lkp@intel.com/
New smatch warnings:
drivers/usb/gadget/function/u_ether.c:474 eth_start_xmit() error: we previously assumed 'dev->port_usb' could be null (see line 466)
drivers/usb/gadget/function/u_ether.c:539 eth_start_xmit() warn: variable dereferenced before check 'dev->port_usb' (see line 474)
Old smatch warnings:
drivers/usb/gadget/function/u_ether.c:553 eth_start_xmit() error: we previously assumed 'skb' could be null (see line 491)
vim +474 drivers/usb/gadget/function/u_ether.c
25a79c41ce0ce8 drivers/usb/gadget/u_ether.c Stephen Hemminger 2009-08-31 454 static netdev_tx_t eth_start_xmit(struct sk_buff *skb,
25a79c41ce0ce8 drivers/usb/gadget/u_ether.c Stephen Hemminger 2009-08-31 455 struct net_device *net)
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 456 {
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 457 struct eth_dev *dev = netdev_priv(net);
6d3865f9d41f15 drivers/usb/gadget/u_ether.c Jim Baxter 2014-07-07 458 int length = 0;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 459 int retval;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 460 struct usb_request *req = NULL;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 461 unsigned long flags;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 462 struct usb_ep *in;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 463 u16 cdc_filter;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 464
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 465 spin_lock_irqsave(&dev->lock, flags);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 @466 if (dev->port_usb) {
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 467 in = dev->port_usb->in_ep;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 468 cdc_filter = dev->port_usb->cdc_filter;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 469 } else {
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 470 in = NULL;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 471 cdc_filter = 0;
NULL on this path.
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 472 }
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao 2023-03-16 473
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao 2023-03-16 @474 if (dev->port_usb->is_suspend) {
^^^^^^^^^^^^^^^
Dead. (Both ->port_usb warnings are caused by this dereference).
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao 2023-03-16 475 DBG(dev, "Port suspended. Triggering wakeup\n");
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao 2023-03-16 476 netif_stop_queue(net);
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao 2023-03-16 477 spin_unlock_irqrestore(&dev->lock, flags);
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao 2023-03-16 478 ether_wakeup_host(dev->port_usb);
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao 2023-03-16 479 return NETDEV_TX_BUSY;
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao 2023-03-16 480 }
11f254fc796c87 drivers/usb/gadget/function/u_ether.c Elson Roy Serrao 2023-03-16 481
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 482 spin_unlock_irqrestore(&dev->lock, flags);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 483
8ae01239609b29 drivers/usb/gadget/function/u_ether.c Maciej Żenczykowski 2021-07-01 484 if (!in) {
8ae01239609b29 drivers/usb/gadget/function/u_ether.c Maciej Żenczykowski 2021-07-01 485 if (skb)
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 486 dev_kfree_skb_any(skb);
6ed106549d1747 drivers/usb/gadget/u_ether.c Patrick McHardy 2009-06-23 487 return NETDEV_TX_OK;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 488 }
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 489
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 490 /* apply outgoing CDC or RNDIS filters */
6d3865f9d41f15 drivers/usb/gadget/u_ether.c Jim Baxter 2014-07-07 491 if (skb && !is_promisc(cdc_filter)) {
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 492 u8 *dest = skb->data;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 493
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 494 if (is_multicast_ether_addr(dest)) {
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 495 u16 type;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 496
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 497 /* ignores USB_CDC_PACKET_TYPE_MULTICAST and host
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 498 * SET_ETHERNET_MULTICAST_FILTERS requests
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 499 */
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 500 if (is_broadcast_ether_addr(dest))
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 501 type = USB_CDC_PACKET_TYPE_BROADCAST;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 502 else
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 503 type = USB_CDC_PACKET_TYPE_ALL_MULTICAST;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 504 if (!(cdc_filter & type)) {
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 505 dev_kfree_skb_any(skb);
6ed106549d1747 drivers/usb/gadget/u_ether.c Patrick McHardy 2009-06-23 506 return NETDEV_TX_OK;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 507 }
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 508 }
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 509 /* ignores USB_CDC_PACKET_TYPE_DIRECTED */
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 510 }
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 511
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 512 spin_lock_irqsave(&dev->req_lock, flags);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 513 /*
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 514 * this freelist can be empty if an interrupt triggered disconnect()
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 515 * and reconfigured the gadget (shutting down this queue) after the
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 516 * network stack decided to xmit but before we got the spinlock.
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 517 */
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 518 if (list_empty(&dev->tx_reqs)) {
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 519 spin_unlock_irqrestore(&dev->req_lock, flags);
5b548140225c6b drivers/usb/gadget/u_ether.c Patrick McHardy 2009-06-12 520 return NETDEV_TX_BUSY;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 521 }
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 522
fea14e68ff5e11 drivers/usb/gadget/function/u_ether.c Felipe Balbi 2017-03-22 523 req = list_first_entry(&dev->tx_reqs, struct usb_request, list);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 524 list_del(&req->list);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 525
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 526 /* temporarily stop TX queue when the freelist empties */
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 527 if (list_empty(&dev->tx_reqs))
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 528 netif_stop_queue(net);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 529 spin_unlock_irqrestore(&dev->req_lock, flags);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 530
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 531 /* no buffer copies needed, unless the network stack did it
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 532 * or the hardware can't use skb buffers.
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 533 * or there's not enough space for extra headers we need
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 534 */
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 535 if (dev->wrap) {
9b39e9ddedeef4 drivers/usb/gadget/u_ether.c Brian Niebuhr 2009-08-14 536 unsigned long flags;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 537
9b39e9ddedeef4 drivers/usb/gadget/u_ether.c Brian Niebuhr 2009-08-14 538 spin_lock_irqsave(&dev->lock, flags);
9b39e9ddedeef4 drivers/usb/gadget/u_ether.c Brian Niebuhr 2009-08-14 @539 if (dev->port_usb)
9b39e9ddedeef4 drivers/usb/gadget/u_ether.c Brian Niebuhr 2009-08-14 540 skb = dev->wrap(dev->port_usb, skb);
3a383cc0b8cc33 drivers/usb/gadget/function/u_ether.c Greg Kroah-Hartman 2016-09-19 541 spin_unlock_irqrestore(&dev->lock, flags);
6d3865f9d41f15 drivers/usb/gadget/u_ether.c Jim Baxter 2014-07-07 542 if (!skb) {
6d3865f9d41f15 drivers/usb/gadget/u_ether.c Jim Baxter 2014-07-07 543 /* Multi frame CDC protocols may store the frame for
6d3865f9d41f15 drivers/usb/gadget/u_ether.c Jim Baxter 2014-07-07 544 * later which is not a dropped frame.
6d3865f9d41f15 drivers/usb/gadget/u_ether.c Jim Baxter 2014-07-07 545 */
88c09eacf560c3 drivers/usb/gadget/function/u_ether.c Peter Chen 2016-07-01 546 if (dev->port_usb &&
3a383cc0b8cc33 drivers/usb/gadget/function/u_ether.c Greg Kroah-Hartman 2016-09-19 547 dev->port_usb->supports_multi_frame)
6d3865f9d41f15 drivers/usb/gadget/u_ether.c Jim Baxter 2014-07-07 548 goto multiframe;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 549 goto drop;
6d3865f9d41f15 drivers/usb/gadget/u_ether.c Jim Baxter 2014-07-07 550 }
6d3865f9d41f15 drivers/usb/gadget/u_ether.c Jim Baxter 2014-07-07 551 }
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 552
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 553 length = skb->len;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 554 req->buf = skb->data;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 555 req->context = skb;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 556 req->complete = tx_complete;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 557
5c1168dbc50828 drivers/usb/gadget/u_ether.c Yauheni Kaliuta 2010-12-08 558 /* NCM requires no zlp if transfer is dwNtbInMaxSize */
79775f44183840 drivers/usb/gadget/function/u_ether.c Harish Jenny K N 2016-09-09 559 if (dev->port_usb &&
79775f44183840 drivers/usb/gadget/function/u_ether.c Harish Jenny K N 2016-09-09 560 dev->port_usb->is_fixed &&
5c1168dbc50828 drivers/usb/gadget/u_ether.c Yauheni Kaliuta 2010-12-08 561 length == dev->port_usb->fixed_in_len &&
5c1168dbc50828 drivers/usb/gadget/u_ether.c Yauheni Kaliuta 2010-12-08 562 (length % in->maxpacket) == 0)
5c1168dbc50828 drivers/usb/gadget/u_ether.c Yauheni Kaliuta 2010-12-08 563 req->zero = 0;
5c1168dbc50828 drivers/usb/gadget/u_ether.c Yauheni Kaliuta 2010-12-08 564 else
5c1168dbc50828 drivers/usb/gadget/u_ether.c Yauheni Kaliuta 2010-12-08 565 req->zero = 1;
5c1168dbc50828 drivers/usb/gadget/u_ether.c Yauheni Kaliuta 2010-12-08 566
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 567 /* use zlp framing on tx for strict CDC-Ether conformance,
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 568 * though any robust network rx path ignores extra padding.
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 569 * and some hardware doesn't like to write zlps.
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 570 */
5c1168dbc50828 drivers/usb/gadget/u_ether.c Yauheni Kaliuta 2010-12-08 571 if (req->zero && !dev->zlp && (length % in->maxpacket) == 0)
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 572 length++;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 573
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 574 req->length = length;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 575
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 576 retval = usb_ep_queue(in, req, GFP_ATOMIC);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 577 switch (retval) {
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 578 default:
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 579 DBG(dev, "tx queue err %d\n", retval);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 580 break;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 581 case 0:
860e9538a9482b drivers/usb/gadget/function/u_ether.c Florian Westphal 2016-05-03 582 netif_trans_update(net);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 583 atomic_inc(&dev->tx_qlen);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 584 }
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 585
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 586 if (retval) {
9b39e9ddedeef4 drivers/usb/gadget/u_ether.c Brian Niebuhr 2009-08-14 587 dev_kfree_skb_any(skb);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 588 drop:
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 589 dev->net->stats.tx_dropped++;
6d3865f9d41f15 drivers/usb/gadget/u_ether.c Jim Baxter 2014-07-07 590 multiframe:
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 591 spin_lock_irqsave(&dev->req_lock, flags);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 592 if (list_empty(&dev->tx_reqs))
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 593 netif_start_queue(net);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 594 list_add(&req->list, &dev->tx_reqs);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 595 spin_unlock_irqrestore(&dev->req_lock, flags);
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 596 }
6ed106549d1747 drivers/usb/gadget/u_ether.c Patrick McHardy 2009-06-23 597 return NETDEV_TX_OK;
2b3d942c487808 drivers/usb/gadget/u_ether.c David Brownell 2008-06-19 598 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 19+ messages in thread