| Message ID | 1678731892-20503-6-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, Mar 13, 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 | 68 +++++++++++++++++++++++++++++++++++ > drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++++++ > drivers/usb/gadget/function/u_ether.h | 4 +++ > 3 files changed, 135 insertions(+) > > diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c > index a7ab30e..d50c1a4 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,68 @@ 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) > +{ > + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) | > + USB_INTRF_STAT_FUNC_RW_CAP; Need to check the usb configuration is if it's wakeup_capable. > +} > + > +static int ecm_func_suspend(struct usb_function *f, u8 options) > +{ > + struct f_ecm *ecm = func_to_ecm(f); > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev; > + > + DBG(cdev, "func susp %u cmd\n", options); > + > + f->func_wakeup_armed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8)); Same here. Check config's bmAttributes if it's remote wakeup capable before arming for remote wakeup. > + > + 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; Need to return negative error if SetFeature fails. We should fix the composite layer to allow for protocal STALL here. Host needs to know if it should proceed to put the function in suspend. Thanks, Thinh > +} > + > static void ecm_free(struct usb_function *f) > { > struct f_ecm *ecm; > @@ -952,6 +1016,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 >
On 3/13/2023 1:27 PM, Thinh Nguyen wrote: > On Mon, Mar 13, 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 | 68 +++++++++++++++++++++++++++++++++++ >> drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++++++ >> drivers/usb/gadget/function/u_ether.h | 4 +++ >> 3 files changed, 135 insertions(+) >> >> diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c >> index a7ab30e..d50c1a4 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,68 @@ 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) >> +{ >> + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) | >> + USB_INTRF_STAT_FUNC_RW_CAP; > > Need to check the usb configuration is if it's wakeup_capable. > >> +} >> + >> +static int ecm_func_suspend(struct usb_function *f, u8 options) >> +{ >> + struct f_ecm *ecm = func_to_ecm(f); >> + struct usb_composite_dev *cdev = ecm->port.func.config->cdev; >> + >> + DBG(cdev, "func susp %u cmd\n", options); >> + >> + f->func_wakeup_armed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8)); > > Same here. Check config's bmAttributes if it's remote wakeup capable > before arming for remote wakeup. > Done. I will add that check for above two cases. >> + >> + 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; > > Need to return negative error if SetFeature fails. We should fix the > composite layer to allow for protocal STALL here. Host needs to know if > it should proceed to put the function in suspend. > > Thanks, > Thinh > Could you please clarify what SetFeature fail here means? The host puts the function in function suspend state through this SetFeature request. If the device is not configured for remote wakeup (bmAtrributes wakeup bit), like you mentioned above we should not arm the function for remote wakeup. But the host is free to put the function in function suspend state and wake it up through host initiated function resume right? Thanks Elson >> +} >> + >> static void ecm_free(struct usb_function *f) >> { >> struct f_ecm *ecm; >> @@ -952,6 +1016,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
On Tue, Mar 14, 2023, Elson Serrao wrote: > > > On 3/13/2023 1:27 PM, Thinh Nguyen wrote: > > On Mon, Mar 13, 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 | 68 +++++++++++++++++++++++++++++++++++ > > > drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++++++ > > > drivers/usb/gadget/function/u_ether.h | 4 +++ > > > 3 files changed, 135 insertions(+) > > > > > > diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c > > > index a7ab30e..d50c1a4 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,68 @@ 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) > > > +{ > > > + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) | > > > + USB_INTRF_STAT_FUNC_RW_CAP; > > > > Need to check the usb configuration is if it's wakeup_capable. > > > > > +} > > > + > > > +static int ecm_func_suspend(struct usb_function *f, u8 options) > > > +{ > > > + struct f_ecm *ecm = func_to_ecm(f); > > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev; > > > + > > > + DBG(cdev, "func susp %u cmd\n", options); > > > + > > > + f->func_wakeup_armed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8)); > > > > Same here. Check config's bmAttributes if it's remote wakeup capable > > before arming for remote wakeup. > > > Done. I will add that check for above two cases. > > > + > > > + 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; > > > > Need to return negative error if SetFeature fails. We should fix the > > composite layer to allow for protocal STALL here. Host needs to know if > > it should proceed to put the function in suspend. > > > > Thanks, > > Thinh > > > > Could you please clarify what SetFeature fail here means? The host puts the > function in function suspend state through this SetFeature request. > If the device is not configured for remote wakeup (bmAtrributes wakeup bit), > like you mentioned above we should not arm the function for remote wakeup. > But the host is free to put the function in function suspend state and wake > it up through host initiated function resume right? > I mean if we want to tell the host that a feature cannot be set or that it doesn't exist, we should respond with a protocol STALL. How the host respond to the rejected SetFeature request is up to the host. But we should at least let the host know that. I'm suggesting to remove the setting of value = 0 in composite.c: -- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -2000,7 +2000,6 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) ERROR(cdev, "func_suspend() returned error %d\n", value); - value = 0; } break; } i.e. we should allow the return value to go through. Thanks, Thinh
On Tue, Mar 14, 2023, Thinh Nguyen wrote: > On Tue, Mar 14, 2023, Elson Serrao wrote: > > > > > > On 3/13/2023 1:27 PM, Thinh Nguyen wrote: > > > On Mon, Mar 13, 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 | 68 +++++++++++++++++++++++++++++++++++ > > > > drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++++++ > > > > drivers/usb/gadget/function/u_ether.h | 4 +++ > > > > 3 files changed, 135 insertions(+) > > > > > > > > diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c > > > > index a7ab30e..d50c1a4 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,68 @@ 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) > > > > +{ > > > > + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) | > > > > + USB_INTRF_STAT_FUNC_RW_CAP; > > > > > > Need to check the usb configuration is if it's wakeup_capable. > > > > > > > +} > > > > + > > > > +static int ecm_func_suspend(struct usb_function *f, u8 options) > > > > +{ > > > > + struct f_ecm *ecm = func_to_ecm(f); > > > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev; > > > > + > > > > + DBG(cdev, "func susp %u cmd\n", options); > > > > + > > > > + f->func_wakeup_armed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8)); > > > > > > Same here. Check config's bmAttributes if it's remote wakeup capable > > > before arming for remote wakeup. > > > > > Done. I will add that check for above two cases. > > > > + > > > > + 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; > > > > > > Need to return negative error if SetFeature fails. We should fix the > > > composite layer to allow for protocal STALL here. Host needs to know if > > > it should proceed to put the function in suspend. > > > > > > Thanks, > > > Thinh > > > > > > > Could you please clarify what SetFeature fail here means? The host puts the > > function in function suspend state through this SetFeature request. > > If the device is not configured for remote wakeup (bmAtrributes wakeup bit), > > like you mentioned above we should not arm the function for remote wakeup. > > But the host is free to put the function in function suspend state and wake > > it up through host initiated function resume right? > > > > I mean if we want to tell the host that a feature cannot be set or that > it doesn't exist, we should respond with a protocol STALL. How the host > respond to the rejected SetFeature request is up to the host. But we > should at least let the host know that. > > I'm suggesting to remove the setting of value = 0 in composite.c: > > -- a/drivers/usb/gadget/composite.c > +++ b/drivers/usb/gadget/composite.c > @@ -2000,7 +2000,6 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) > ERROR(cdev, > "func_suspend() returned error %d\n", > value); > - value = 0; > } > break; > } > > > i.e. we should allow the return value to go through. > Also, I imagine there are cases where we don't want the host to put the device in suspend because it lacks remote wakeup. e.g. a HID device such as a keyboard (though it's a bit odd to see one without remote wake capability) Thanks, Thinh
On 3/14/2023 1:45 PM, Thinh Nguyen wrote: > On Tue, Mar 14, 2023, Thinh Nguyen wrote: >> On Tue, Mar 14, 2023, Elson Serrao wrote: >>> >>> >>> On 3/13/2023 1:27 PM, Thinh Nguyen wrote: >>>> On Mon, Mar 13, 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 | 68 +++++++++++++++++++++++++++++++++++ >>>>> drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++++++ >>>>> drivers/usb/gadget/function/u_ether.h | 4 +++ >>>>> 3 files changed, 135 insertions(+) >>>>> >>>>> diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c >>>>> index a7ab30e..d50c1a4 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,68 @@ 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) >>>>> +{ >>>>> + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) | >>>>> + USB_INTRF_STAT_FUNC_RW_CAP; >>>> >>>> Need to check the usb configuration is if it's wakeup_capable. >>>> >>>>> +} >>>>> + >>>>> +static int ecm_func_suspend(struct usb_function *f, u8 options) >>>>> +{ >>>>> + struct f_ecm *ecm = func_to_ecm(f); >>>>> + struct usb_composite_dev *cdev = ecm->port.func.config->cdev; >>>>> + >>>>> + DBG(cdev, "func susp %u cmd\n", options); >>>>> + >>>>> + f->func_wakeup_armed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8)); >>>> >>>> Same here. Check config's bmAttributes if it's remote wakeup capable >>>> before arming for remote wakeup. >>>> >>> Done. I will add that check for above two cases. >>>>> + >>>>> + 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; >>>> >>>> Need to return negative error if SetFeature fails. We should fix the >>>> composite layer to allow for protocal STALL here. Host needs to know if >>>> it should proceed to put the function in suspend. >>>> >>>> Thanks, >>>> Thinh >>>> >>> >>> Could you please clarify what SetFeature fail here means? The host puts the >>> function in function suspend state through this SetFeature request. >>> If the device is not configured for remote wakeup (bmAtrributes wakeup bit), >>> like you mentioned above we should not arm the function for remote wakeup. >>> But the host is free to put the function in function suspend state and wake >>> it up through host initiated function resume right? >>> >> >> I mean if we want to tell the host that a feature cannot be set or that >> it doesn't exist, we should respond with a protocol STALL. How the host >> respond to the rejected SetFeature request is up to the host. But we >> should at least let the host know that. >> >> I'm suggesting to remove the setting of value = 0 in composite.c: >> >> -- a/drivers/usb/gadget/composite.c >> +++ b/drivers/usb/gadget/composite.c >> @@ -2000,7 +2000,6 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) >> ERROR(cdev, >> "func_suspend() returned error %d\n", >> value); >> - value = 0; >> } >> break; >> } >> >> >> i.e. we should allow the return value to go through. >> > > Also, I imagine there are cases where we don't want the host to put the > device in suspend because it lacks remote wakeup. e.g. a HID device such > as a keyboard (though it's a bit odd to see one without remote wake > capability) > > Thanks, > Thinh Sound good. I will make that change. Would you prefer this change (i.e removing value = 0 in composite.c) to be part of this series OR should I upload a separate change for this? Thanks Elson
On Tue, Mar 14, 2023, Elson Serrao wrote: > > > On 3/14/2023 1:45 PM, Thinh Nguyen wrote: > > On Tue, Mar 14, 2023, Thinh Nguyen wrote: > > > On Tue, Mar 14, 2023, Elson Serrao wrote: > > > > > > > > > > > > On 3/13/2023 1:27 PM, Thinh Nguyen wrote: > > > > > On Mon, Mar 13, 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 | 68 +++++++++++++++++++++++++++++++++++ > > > > > > drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++++++ > > > > > > drivers/usb/gadget/function/u_ether.h | 4 +++ > > > > > > 3 files changed, 135 insertions(+) > > > > > > > > > > > > diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c > > > > > > index a7ab30e..d50c1a4 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,68 @@ 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) > > > > > > +{ > > > > > > + return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) | > > > > > > + USB_INTRF_STAT_FUNC_RW_CAP; > > > > > > > > > > Need to check the usb configuration is if it's wakeup_capable. > > > > > > > > > > > +} > > > > > > + > > > > > > +static int ecm_func_suspend(struct usb_function *f, u8 options) > > > > > > +{ > > > > > > + struct f_ecm *ecm = func_to_ecm(f); > > > > > > + struct usb_composite_dev *cdev = ecm->port.func.config->cdev; > > > > > > + > > > > > > + DBG(cdev, "func susp %u cmd\n", options); > > > > > > + > > > > > > + f->func_wakeup_armed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8)); > > > > > > > > > > Same here. Check config's bmAttributes if it's remote wakeup capable > > > > > before arming for remote wakeup. > > > > > > > > > Done. I will add that check for above two cases. > > > > > > + > > > > > > + 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; > > > > > > > > > > Need to return negative error if SetFeature fails. We should fix the > > > > > composite layer to allow for protocal STALL here. Host needs to know if > > > > > it should proceed to put the function in suspend. > > > > > > > > > > Thanks, > > > > > Thinh > > > > > > > > > > > > > Could you please clarify what SetFeature fail here means? The host puts the > > > > function in function suspend state through this SetFeature request. > > > > If the device is not configured for remote wakeup (bmAtrributes wakeup bit), > > > > like you mentioned above we should not arm the function for remote wakeup. > > > > But the host is free to put the function in function suspend state and wake > > > > it up through host initiated function resume right? > > > > > > > > > > I mean if we want to tell the host that a feature cannot be set or that > > > it doesn't exist, we should respond with a protocol STALL. How the host > > > respond to the rejected SetFeature request is up to the host. But we > > > should at least let the host know that. > > > > > > I'm suggesting to remove the setting of value = 0 in composite.c: > > > > > > -- a/drivers/usb/gadget/composite.c > > > +++ b/drivers/usb/gadget/composite.c > > > @@ -2000,7 +2000,6 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl) > > > ERROR(cdev, > > > "func_suspend() returned error %d\n", > > > value); > > > - value = 0; > > > } > > > break; > > > } > > > > > > > > > i.e. we should allow the return value to go through. > > > > > > > Also, I imagine there are cases where we don't want the host to put the > > device in suspend because it lacks remote wakeup. e.g. a HID device such > > as a keyboard (though it's a bit odd to see one without remote wake > > capability) > > > > Thanks, > > Thinh > > Sound good. I will make that change. Would you prefer this change (i.e > removing value = 0 in composite.c) to be part of this series OR should I > upload a separate change for this? > That change can be separated from this series. Let's try to get this series in merged. Thanks, Thinh
diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c index a7ab30e..d50c1a4 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,68 @@ 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) +{ + 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 f_ecm *ecm = func_to_ecm(f); + struct usb_composite_dev *cdev = ecm->port.func.config->cdev; + + DBG(cdev, "func susp %u cmd\n", options); + + f->func_wakeup_armed = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8)); + + 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 +1016,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 *);
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 | 68 +++++++++++++++++++++++++++++++++++ drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++++++ drivers/usb/gadget/function/u_ether.h | 4 +++ 3 files changed, 135 insertions(+)