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@xxxxxxxxxxx> > --- > 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