| Message ID | 1675710806-9735-2-git-send-email-quic_eserrao@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add function suspend/resume and remote wakeup support | expand |
On Mon, Feb 06, 2023 at 11:13:22AM -0800, Elson Roy Serrao wrote: > 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. > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> > --- > drivers/usb/gadget/composite.c | 24 +++++++++++++++++++++++- > drivers/usb/gadget/udc/core.c | 27 +++++++++++++++++++++++++++ > drivers/usb/gadget/udc/trace.h | 5 +++++ > include/linux/usb/gadget.h | 8 ++++++++ > 4 files changed, 63 insertions(+), 1 deletion(-) > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > index dc3092c..05d1449 100644 > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -309,6 +309,7 @@ struct usb_udc; > struct usb_gadget_ops { > int (*get_frame)(struct usb_gadget *); > int (*wakeup)(struct usb_gadget *); > + int (*set_remotewakeup)(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); > @@ -383,6 +384,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. > + * @rw_capable: True if gadget is capable of sending remote wakeup. > + * @rw_armed: True if gadget is armed by the host for remote wakeup. Minor stylistic request: Could you choose something other than "rw" to start these field names? For too many people, that abbreviation is firmly associated with "read/write". Maybe just "wakeup"? Alan Stern
On 2/6/2023 12:14 PM, Alan Stern wrote: > On Mon, Feb 06, 2023 at 11:13:22AM -0800, Elson Roy Serrao wrote: >> 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. >> >> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> >> --- >> drivers/usb/gadget/composite.c | 24 +++++++++++++++++++++++- >> drivers/usb/gadget/udc/core.c | 27 +++++++++++++++++++++++++++ >> drivers/usb/gadget/udc/trace.h | 5 +++++ >> include/linux/usb/gadget.h | 8 ++++++++ >> 4 files changed, 63 insertions(+), 1 deletion(-) > >> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h >> index dc3092c..05d1449 100644 >> --- a/include/linux/usb/gadget.h >> +++ b/include/linux/usb/gadget.h >> @@ -309,6 +309,7 @@ struct usb_udc; >> struct usb_gadget_ops { >> int (*get_frame)(struct usb_gadget *); >> int (*wakeup)(struct usb_gadget *); >> + int (*set_remotewakeup)(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); >> @@ -383,6 +384,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. >> + * @rw_capable: True if gadget is capable of sending remote wakeup. >> + * @rw_armed: True if gadget is armed by the host for remote wakeup. > > Minor stylistic request: Could you choose something other than "rw" to > start these field names? For too many people, that abbreviation is > firmly associated with "read/write". Maybe just "wakeup"? > > Alan Stern Sure. Agree that "rw" is firmly associated with "read/write". Will just rename it to "wakeup" Thanks Elson
Hi Elson, On Mon, Feb 06, 2023, Elson Roy Serrao wrote: > 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. > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> > --- > drivers/usb/gadget/composite.c | 24 +++++++++++++++++++++++- > drivers/usb/gadget/udc/core.c | 27 +++++++++++++++++++++++++++ > drivers/usb/gadget/udc/trace.h | 5 +++++ > include/linux/usb/gadget.h | 8 ++++++++ > 4 files changed, 63 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > index fa7dd6c..e459fb0 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; > } > > +static void check_remote_wakeup_config(struct usb_gadget *gadget, > + struct usb_configuration *c) > +{ > + if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes) { > + /* Reset the rw bit if gadget is not capable of it */ > + if (!gadget->rw_capable) { Probably it's better to make sure we don't break the configuration for other UDCs until this is implemented in their drivers. Let's add another condition: if (!gadget->rw_capable && gadget->ops->set_remotewakeup) { ... } > + INFO(c->cdev, "Clearing rw bit for config c.%d\n", > + c->bConfigurationValue); Perhaps a warning is better since we're overwriting the user's configuration. > + c->bmAttributes &= ~USB_CONFIG_ATT_WAKEUP; > + } > + } > +} > + > static int config_buf(struct usb_configuration *config, > enum usb_device_speed speed, void *buf, u8 type) > { > @@ -620,8 +633,12 @@ static int config_desc(struct usb_composite_dev *cdev, unsigned w_value) > continue; > } > > - if (w_value == 0) > + if (w_value == 0) { > + /* Correctly configure the bmAttributes wakeup bit */ > + check_remote_wakeup_config(gadget, c); Checking here is too late. We should check in configfs_composite_bind(). > + > return config_buf(c, speed, cdev->req->buf, type); > + } > w_value--; > } > return -EINVAL; > @@ -1000,6 +1017,11 @@ static int set_config(struct usb_composite_dev *cdev, > else > usb_gadget_clear_selfpowered(gadget); > > + if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes) > + usb_gadget_set_remotewakeup(gadget, 1); > + else > + usb_gadget_set_remotewakeup(gadget, 0); > + > usb_gadget_vbus_draw(gadget, power); > if (result >= 0 && cdev->delayed_status) > result = USB_GADGET_DELAYED_STATUS; > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index 23b0629..5874d4f 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_remotewakeup - 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_remotewakeup(struct usb_gadget *gadget, int set) > +{ > + int ret = 0; > + > + if (!gadget->ops->set_remotewakeup) { > + ret = -EOPNOTSUPP; > + goto out; > + } > + > + ret = gadget->ops->set_remotewakeup(gadget, set); > + > +out: > + trace_usb_gadget_set_remotewakeup(gadget, ret); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(usb_gadget_set_remotewakeup); > + > +/** > * 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..a3314ce 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_remotewakeup, > + 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/gadget.h b/include/linux/usb/gadget.h > index dc3092c..05d1449 100644 > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -309,6 +309,7 @@ struct usb_udc; > struct usb_gadget_ops { > int (*get_frame)(struct usb_gadget *); > int (*wakeup)(struct usb_gadget *); > + int (*set_remotewakeup)(struct usb_gadget *, int set); minor nit: can we change this name to set_remote_wakeup. It's a bit easier to read IMO. > 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); > @@ -383,6 +384,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. > + * @rw_capable: True if gadget is capable of sending remote wakeup. > + * @rw_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 > * > @@ -444,6 +447,8 @@ struct usb_gadget { > unsigned deactivated:1; > unsigned connected:1; > unsigned lpm_capable:1; > + unsigned rw_capable:1; > + unsigned rw_armed:1; > int irq; > int id_number; > }; > @@ -600,6 +605,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_remotewakeup(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); > @@ -615,6 +621,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_remotewakeup(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 > Thanks, Thinh
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index fa7dd6c..e459fb0 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; } +static void check_remote_wakeup_config(struct usb_gadget *gadget, + struct usb_configuration *c) +{ + if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes) { + /* Reset the rw bit if gadget is not capable of it */ + if (!gadget->rw_capable) { + INFO(c->cdev, "Clearing rw 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) { @@ -620,8 +633,12 @@ static int config_desc(struct usb_composite_dev *cdev, unsigned w_value) continue; } - if (w_value == 0) + if (w_value == 0) { + /* Correctly configure the bmAttributes wakeup bit */ + check_remote_wakeup_config(gadget, c); + return config_buf(c, speed, cdev->req->buf, type); + } w_value--; } return -EINVAL; @@ -1000,6 +1017,11 @@ static int set_config(struct usb_composite_dev *cdev, else usb_gadget_clear_selfpowered(gadget); + if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes) + usb_gadget_set_remotewakeup(gadget, 1); + else + usb_gadget_set_remotewakeup(gadget, 0); + usb_gadget_vbus_draw(gadget, power); if (result >= 0 && cdev->delayed_status) result = USB_GADGET_DELAYED_STATUS; diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index 23b0629..5874d4f 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_remotewakeup - 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_remotewakeup(struct usb_gadget *gadget, int set) +{ + int ret = 0; + + if (!gadget->ops->set_remotewakeup) { + ret = -EOPNOTSUPP; + goto out; + } + + ret = gadget->ops->set_remotewakeup(gadget, set); + +out: + trace_usb_gadget_set_remotewakeup(gadget, ret); + + return ret; +} +EXPORT_SYMBOL_GPL(usb_gadget_set_remotewakeup); + +/** * 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..a3314ce 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_remotewakeup, + 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/gadget.h b/include/linux/usb/gadget.h index dc3092c..05d1449 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -309,6 +309,7 @@ struct usb_udc; struct usb_gadget_ops { int (*get_frame)(struct usb_gadget *); int (*wakeup)(struct usb_gadget *); + int (*set_remotewakeup)(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); @@ -383,6 +384,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. + * @rw_capable: True if gadget is capable of sending remote wakeup. + * @rw_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 * @@ -444,6 +447,8 @@ struct usb_gadget { unsigned deactivated:1; unsigned connected:1; unsigned lpm_capable:1; + unsigned rw_capable:1; + unsigned rw_armed:1; int irq; int id_number; }; @@ -600,6 +605,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_remotewakeup(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); @@ -615,6 +621,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_remotewakeup(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)
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. Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> --- drivers/usb/gadget/composite.c | 24 +++++++++++++++++++++++- drivers/usb/gadget/udc/core.c | 27 +++++++++++++++++++++++++++ drivers/usb/gadget/udc/trace.h | 5 +++++ include/linux/usb/gadget.h | 8 ++++++++ 4 files changed, 63 insertions(+), 1 deletion(-)