| Message ID | 1673992507-7823-2-git-send-email-quic_eserrao@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add function suspend/resume and remote wakeup support | expand |
On Tue, Jan 17, 2023 at 01:55:03PM -0800, Elson Roy Serrao wrote: > Add a flag to indicate whether the gadget is capable > of sending remote wakeup to the host. > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> > --- > drivers/usb/gadget/composite.c | 3 +++ > include/linux/usb/gadget.h | 2 ++ > 2 files changed, 5 insertions(+) > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > index 403563c..b83963a 100644 > --- a/drivers/usb/gadget/composite.c > +++ b/drivers/usb/gadget/composite.c > @@ -965,6 +965,9 @@ static int set_config(struct usb_composite_dev *cdev, > else > usb_gadget_clear_selfpowered(gadget); > > + if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes) > + gadget->rw_capable = 1; > + > usb_gadget_vbus_draw(gadget, power); > if (result >= 0 && cdev->delayed_status) > result = USB_GADGET_DELAYED_STATUS; > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > index dc3092c..15785f8 100644 > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -385,6 +385,7 @@ struct usb_gadget_ops { > * indicates that it supports LPM as per the LPM ECN & errata. > * @irq: the interrupt number for device controller. > * @id_number: a unique ID number for ensuring that gadget names are distinct > + * @rw_capable: True if the gadget is capable of sending remote wakeup. > * > * Gadgets have a mostly-portable "gadget driver" implementing device > * functions, handling all usb configurations and interfaces. Gadget > @@ -446,6 +447,7 @@ struct usb_gadget { > unsigned lpm_capable:1; > int irq; > int id_number; > + unsigned rw_capable:1; Why not put this by the other bitfields in this structure? thanks, greg k-h
On 1/19/2023 5:15 PM, Thinh Nguyen wrote: > On Thu, Jan 19, 2023, Elson Serrao wrote: >> >> >> On 1/18/2023 5:44 PM, Thinh Nguyen wrote: >>> On Tue, Jan 17, 2023, Elson Roy Serrao wrote: >>>> Add a flag to indicate whether the gadget is capable >>>> of sending remote wakeup to the host. >>>> >>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> >>>> --- >>>> drivers/usb/gadget/composite.c | 3 +++ >>>> include/linux/usb/gadget.h | 2 ++ >>>> 2 files changed, 5 insertions(+) >>>> >>>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c >>>> index 403563c..b83963a 100644 >>>> --- a/drivers/usb/gadget/composite.c >>>> +++ b/drivers/usb/gadget/composite.c >>>> @@ -965,6 +965,9 @@ static int set_config(struct usb_composite_dev *cdev, >>>> else >>>> usb_gadget_clear_selfpowered(gadget); >>>> + if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes) >>>> + gadget->rw_capable = 1; >>> >>> Some device may not support remote wakeup. gadget->rw_capable should be >>> set and reported by the UDC. May need a gadget ops to enable remote >>> wakeup here. >>> >>> Thanks, >>> Thinh >>> >> Not exactly clear on which parameter in UDC decides whether a device >> supports remote wakeup. Here I have this flag just to indicate whether the >> connected device is rw capable based on the bmAttributes populated in the >> config descriptor. If the UDC doesnt have a callback for remote wakeup we >> have that check when calling the gadget op in udc/core.c (have added a >> similar check in usb_func_wakeup() also ). > > That flag describes the gadget's capability, not the device > configuration. However, it's being used as a configuration flag. > > Thanks, > Thinh > Thank you for the clarification. Please let me know if below approach where we consider both gadget's capability and device configuration fine? if (gadget->ops->wakeup || gadget->ops->func_wakeup) gadget->rw_capable = USB_CONFIG_ATT_WAKEUP & c->bmAttributes ? 1: 0; Thanks Elson >> >> int usb_gadget_wakeup(struct usb_gadget *gadget) >> { >> int ret = 0; >> >> if (!gadget->ops->wakeup) { >> ret = -EOPNOTSUPP; >> goto out; >> >> Thanks >> Elson >> >>>> + >>>> usb_gadget_vbus_draw(gadget, power); >>>> if (result >= 0 && cdev->delayed_status) >>>> result = USB_GADGET_DELAYED_STATUS; >>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h >>>> index dc3092c..15785f8 100644 >>>> --- a/include/linux/usb/gadget.h >>>> +++ b/include/linux/usb/gadget.h >>>> @@ -385,6 +385,7 @@ struct usb_gadget_ops { >>>> * indicates that it supports LPM as per the LPM ECN & errata. >>>> * @irq: the interrupt number for device controller. >>>> * @id_number: a unique ID number for ensuring that gadget names are distinct >>>> + * @rw_capable: True if the gadget is capable of sending remote wakeup. >>>> *
On Fri, Jan 20, 2023, Elson Serrao wrote: > > > On 1/19/2023 5:15 PM, Thinh Nguyen wrote: > > On Thu, Jan 19, 2023, Elson Serrao wrote: > > > > > > > > > On 1/18/2023 5:44 PM, Thinh Nguyen wrote: > > > > On Tue, Jan 17, 2023, Elson Roy Serrao wrote: > > > > > Add a flag to indicate whether the gadget is capable > > > > > of sending remote wakeup to the host. > > > > > > > > > > Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> > > > > > --- > > > > > drivers/usb/gadget/composite.c | 3 +++ > > > > > include/linux/usb/gadget.h | 2 ++ > > > > > 2 files changed, 5 insertions(+) > > > > > > > > > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > > > > > index 403563c..b83963a 100644 > > > > > --- a/drivers/usb/gadget/composite.c > > > > > +++ b/drivers/usb/gadget/composite.c > > > > > @@ -965,6 +965,9 @@ static int set_config(struct usb_composite_dev *cdev, > > > > > else > > > > > usb_gadget_clear_selfpowered(gadget); > > > > > + if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes) > > > > > + gadget->rw_capable = 1; > > > > > > > > Some device may not support remote wakeup. gadget->rw_capable should be > > > > set and reported by the UDC. May need a gadget ops to enable remote > > > > wakeup here. > > > > > > > > Thanks, > > > > Thinh > > > > > > > Not exactly clear on which parameter in UDC decides whether a device > > > supports remote wakeup. Here I have this flag just to indicate whether the > > > connected device is rw capable based on the bmAttributes populated in the > > > config descriptor. If the UDC doesnt have a callback for remote wakeup we > > > have that check when calling the gadget op in udc/core.c (have added a > > > similar check in usb_func_wakeup() also ). > > > > That flag describes the gadget's capability, not the device > > configuration. However, it's being used as a configuration flag. > > > > Thanks, > > Thinh > > > > Thank you for the clarification. Please let me know if below approach where > we consider both gadget's capability and device configuration fine? > > if (gadget->ops->wakeup || gadget->ops->func_wakeup) > gadget->rw_capable = USB_CONFIG_ATT_WAKEUP & c->bmAttributes ? 1: 0; > The way gadget->rw_capable is named and described, it's a capability flag. That is, its value shouldn't change from the user config. Perhaps we don't need that in the usb_gadget, and we can have something that looks like this: if (gadget->ops->wakeup && (c->bmAttributes & USB_CONFIG_ATT_WAKEUP)) usb_gadget_enable_remote_wakeup(g); else usb_gadget_disable_remote_wakeup(g); The setting of the remote wakeup configuration can be tracked internally by the dwc3 driver based on the usb_gadget_enable_remote_wakeup call. Thanks, Thinh
On Sat, Jan 21, 2023 at 12:21:10AM +0000, Thinh Nguyen wrote: > The way gadget->rw_capable is named and described, it's a capability > flag. That is, its value shouldn't change from the user config. Perhaps > we don't need that in the usb_gadget, and we can have something that > looks like this: > > if (gadget->ops->wakeup && (c->bmAttributes & USB_CONFIG_ATT_WAKEUP)) > usb_gadget_enable_remote_wakeup(g); > else > usb_gadget_disable_remote_wakeup(g); > > The setting of the remote wakeup configuration can be tracked internally > by the dwc3 driver based on the usb_gadget_enable_remote_wakeup call. A UDC design might have multiple versions, some supporting remote wakeup and others not. But drivers generally use a single static usb_gadget_ops structure, and they don't modify it at runtime to account for hardware differences. So if a single driver controls those multiple versions, you can't rely on the presence of gadget->ops->wakeup to indicate whether there actually is hardware remote wakeup support. Ideally, the usb_gadget structure should have a wakeup_capable flag which the UDC driver would set appropriately (probably during its probe routine). Alan Stern
On Fri, Jan 20, 2023, Alan Stern wrote: > On Sat, Jan 21, 2023 at 12:21:10AM +0000, Thinh Nguyen wrote: > > The way gadget->rw_capable is named and described, it's a capability > > flag. That is, its value shouldn't change from the user config. Perhaps > > we don't need that in the usb_gadget, and we can have something that > > looks like this: > > > > if (gadget->ops->wakeup && (c->bmAttributes & USB_CONFIG_ATT_WAKEUP)) > > usb_gadget_enable_remote_wakeup(g); > > else > > usb_gadget_disable_remote_wakeup(g); > > > > The setting of the remote wakeup configuration can be tracked internally > > by the dwc3 driver based on the usb_gadget_enable_remote_wakeup call. > > A UDC design might have multiple versions, some supporting remote wakeup > and others not. But drivers generally use a single static > usb_gadget_ops structure, and they don't modify it at runtime to account > for hardware differences. So if a single driver controls those multiple > versions, you can't rely on the presence of gadget->ops->wakeup to > indicate whether there actually is hardware remote wakeup support. > > Ideally, the usb_gadget structure should have a wakeup_capable flag > which the UDC driver would set appropriately (probably during its probe > routine). > I was thinking that it can be handled by the usb_gadget_enable_remote_wakeup() so we can do away with the wakeup_capable flag. BR, Thinh
On Sat, Jan 21, 2023 at 02:02:36AM +0000, Thinh Nguyen wrote: > On Fri, Jan 20, 2023, Alan Stern wrote: > > A UDC design might have multiple versions, some supporting remote wakeup > > and others not. But drivers generally use a single static > > usb_gadget_ops structure, and they don't modify it at runtime to account > > for hardware differences. So if a single driver controls those multiple > > versions, you can't rely on the presence of gadget->ops->wakeup to > > indicate whether there actually is hardware remote wakeup support. > > > > Ideally, the usb_gadget structure should have a wakeup_capable flag > > which the UDC driver would set appropriately (probably during its probe > > routine). > > > > I was thinking that it can be handled by the > usb_gadget_enable_remote_wakeup() so we can do away with the > wakeup_capable flag. usb_gadget_enable_remote_wakeup() gets called when the gadget or function is suspended, right? But a gadget driver may want to know long before that whether the UDC supports remote wakeup, in order to set up its config descriptor correctly. Alan Stern
On Fri, Jan 20, 2023, Alan Stern wrote: > On Sat, Jan 21, 2023 at 02:02:36AM +0000, Thinh Nguyen wrote: > > On Fri, Jan 20, 2023, Alan Stern wrote: > > > A UDC design might have multiple versions, some supporting remote wakeup > > > and others not. But drivers generally use a single static > > > usb_gadget_ops structure, and they don't modify it at runtime to account > > > for hardware differences. So if a single driver controls those multiple > > > versions, you can't rely on the presence of gadget->ops->wakeup to > > > indicate whether there actually is hardware remote wakeup support. > > > > > > Ideally, the usb_gadget structure should have a wakeup_capable flag > > > which the UDC driver would set appropriately (probably during its probe > > > routine). > > > > > > > I was thinking that it can be handled by the > > usb_gadget_enable_remote_wakeup() so we can do away with the > > wakeup_capable flag. > > usb_gadget_enable_remote_wakeup() gets called when the gadget or > function is suspended, right? But a gadget driver may want to know long > before that whether the UDC supports remote wakeup, in order to set up > its config descriptor correctly. > No, this is to be called during set configuration. If the configuration doesn't support remote wakeup, the device should not be able to send remote wakeup. BR, Thinh
On Mon, Jan 23, 2023, Elson Serrao wrote: > > > On 1/23/2023 11:33 AM, Thinh Nguyen wrote: > > On Sat, Jan 21, 2023, Thinh Nguyen wrote: > > > On Fri, Jan 20, 2023, Alan Stern wrote: > > > > On Sat, Jan 21, 2023 at 02:02:36AM +0000, Thinh Nguyen wrote: > > > > > On Fri, Jan 20, 2023, Alan Stern wrote: > > > > > > A UDC design might have multiple versions, some supporting remote wakeup > > > > > > and others not. But drivers generally use a single static > > > > > > usb_gadget_ops structure, and they don't modify it at runtime to account > > > > > > for hardware differences. So if a single driver controls those multiple > > > > > > versions, you can't rely on the presence of gadget->ops->wakeup to > > > > > > indicate whether there actually is hardware remote wakeup support. > > > > > > > > > > > > Ideally, the usb_gadget structure should have a wakeup_capable flag > > > > > > which the UDC driver would set appropriately (probably during its probe > > > > > > routine). > > > > > > > > > > > > > > > > I was thinking that it can be handled by the > > > > > usb_gadget_enable_remote_wakeup() so we can do away with the > > > > > wakeup_capable flag. > > > > > > > > usb_gadget_enable_remote_wakeup() gets called when the gadget or > > > > function is suspended, right? But a gadget driver may want to know long > > > > before that whether the UDC supports remote wakeup, in order to set up > > > > its config descriptor correctly. > > > > > > > > > > No, this is to be called during set configuration. If the configuration > > > doesn't support remote wakeup, the device should not be able to send > > > remote wakeup. > > > > > > > On second thought, you're right about the descriptor. It's better to > > warn and prevent the remote wakeup bit from being set in the descriptor > > if the UDC doesn't support remote wakeup. Warning the user at set > > configuration is too late. > > > > So, we need both rw_capable flag and usb_gadget_enable_remote_wakeup(). > > > > Thanks, > > Thinh > > Do we need usb_gadget_enable_remote_wakeup() gadget-op ? > As per the discussion, we can have rw_capable flag in usb_gadget struct and > set it during gagdet init/probe if the UDC supports resume signalling OR > wants the remote wakeup feature to be enabled. > This flag now represents UDC capability to initiate resume signalling. > > During enumeration phase, when preparing the config descriptor we can use > gadget->rw_capable flag to rightly modify the remote wakeup > bit. Based on this, host will decide whether to arm the device for remote > wakeup or not. > If the configuration doesn't allow remote wakeup, the device should not be able to send signal to wake up the host regardless whether the host armed the device for remote wake or not. The rw_capable flag will inform the composite layer whether the UDC is capable of remote wakeup. The composite layer needs to tell the UDC whether the configuration allows for remote wakeup. Whether it's usb_gadget_enable_remote_wakeup() or the remote wakeup bit in the bmAttribute, the composite layer needs to communicate that to the controller driver. But we should not expect the UDC driver to parse for that bit. The prepared control transfer from the the composite layer should be abstracted from the UDC driver. > For gadget->ops->wakeup callback support we already have explicit checks > when invoking this gadget op and device wont be able to send remote wakeup > if callback support doesn't exist. > Please let me know if I am missing something. As mentioned previously, we should also warn the user if the UDC doesn't support remote wakeup and prevent the remote wakeup bit being set in the descriptor. Let me know if it makes sense. Thanks, Thinh
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index 403563c..b83963a 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -965,6 +965,9 @@ static int set_config(struct usb_composite_dev *cdev, else usb_gadget_clear_selfpowered(gadget); + if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes) + gadget->rw_capable = 1; + usb_gadget_vbus_draw(gadget, power); if (result >= 0 && cdev->delayed_status) result = USB_GADGET_DELAYED_STATUS; diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index dc3092c..15785f8 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -385,6 +385,7 @@ struct usb_gadget_ops { * indicates that it supports LPM as per the LPM ECN & errata. * @irq: the interrupt number for device controller. * @id_number: a unique ID number for ensuring that gadget names are distinct + * @rw_capable: True if the gadget is capable of sending remote wakeup. * * Gadgets have a mostly-portable "gadget driver" implementing device * functions, handling all usb configurations and interfaces. Gadget @@ -446,6 +447,7 @@ struct usb_gadget { unsigned lpm_capable:1; int irq; int id_number; + unsigned rw_capable:1; }; #define work_to_gadget(w) (container_of((w), struct usb_gadget, work))
Add a flag to indicate whether the gadget is capable of sending remote wakeup to the host. Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com> --- drivers/usb/gadget/composite.c | 3 +++ include/linux/usb/gadget.h | 2 ++ 2 files changed, 5 insertions(+)