| Message ID | 20191121041858.15746-1-henryl@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: f_ncm: fix wrong usb_ep | expand |
On 19-11-21 12:18:57, Henry Lin wrote: > Gadget driver should always use config_ep_by_speed() to initialize > usb_ep struct according to usb device's operating speed. Otherwise, > usb_ep struct may be wrong if usb devcie's operating speed is changed. > Would you please share the use case how to reproduce it? Peter > Signed-off-by: Henry Lin <henryl@nvidia.com> > --- > drivers/usb/gadget/function/f_ncm.c | 28 ++++++++++++---------------- > 1 file changed, 12 insertions(+), 16 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c > index 2d6e76e4cffa..420b9c96f2fe 100644 > --- a/drivers/usb/gadget/function/f_ncm.c > +++ b/drivers/usb/gadget/function/f_ncm.c > @@ -870,11 +870,10 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt) > DBG(cdev, "reset ncm control %d\n", intf); > usb_ep_disable(ncm->notify); > > - if (!(ncm->notify->desc)) { > - DBG(cdev, "init ncm ctrl %d\n", intf); > - if (config_ep_by_speed(cdev->gadget, f, ncm->notify)) > - goto fail; > - } > + DBG(cdev, "init ncm ctrl %d\n", intf); > + if (config_ep_by_speed(cdev->gadget, f, ncm->notify)) > + goto fail; > + > usb_ep_enable(ncm->notify); > > /* Data interface has two altsettings, 0 and 1 */ > @@ -897,17 +896,14 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt) > if (alt == 1) { > struct net_device *net; > > - if (!ncm->port.in_ep->desc || > - !ncm->port.out_ep->desc) { > - DBG(cdev, "init ncm\n"); > - if (config_ep_by_speed(cdev->gadget, f, > - ncm->port.in_ep) || > - config_ep_by_speed(cdev->gadget, f, > - ncm->port.out_ep)) { > - ncm->port.in_ep->desc = NULL; > - ncm->port.out_ep->desc = NULL; > - goto fail; > - } > + DBG(cdev, "init ncm\n"); > + if (config_ep_by_speed(cdev->gadget, f, > + ncm->port.in_ep) || > + config_ep_by_speed(cdev->gadget, f, > + ncm->port.out_ep)) { > + ncm->port.in_ep->desc = NULL; > + ncm->port.out_ep->desc = NULL; > + goto fail; > } > > /* TODO */ > -- > 2.17.1 >
>> Gadget driver should always use config_ep_by_speed() to initialize >> usb_ep struct according to usb device's operating speed. Otherwise, >> usb_ep struct may be wrong if usb devcie's operating speed is changed. >> >Would you please share the use case how to reproduce it? We checked again and found NCM gadget driver should not have this issue. Please ignore this patch. However, we still observe similar issue in ECM and RNDIS gadget driver. Take ECM as example, if we plug ECM gadget in high-speed host port first. And, the host only set interface with alternate setting != 1 (alt != 1 in ecm_set_alt()) for ECM gadget. Then, if we unplug gadget from high-speed host port to another super-speed host port afterwards, we will see both in_ep->desc and out_ep->desc in ecm->port keep values for high-speed endpoints. Although NCM's implementation is similar to ECM in this part, NCM doesn't have same issue as it only does config_ep_by_speed() under if alt is 1.
> >Would you please share the use case how to reproduce it? > We checked again and found NCM gadget driver should not have this issue. > Please ignore this patch. > > However, we still observe similar issue in ECM and RNDIS gadget driver. Take > ECM as example, if we plug ECM gadget in high-speed host port first. And, the host > only set interface with alternate setting != 1 (alt != 1 in ecm_set_alt()) for ECM > gadget. Then, if we unplug gadget from high-speed host port to another super- > speed host port afterwards, we will see both in_ep->desc and out_ep->desc in ecm- > >port keep values for high-speed endpoints. Although NCM's implementation is > similar to ECM in this part, NCM doesn't have same issue as it only does > config_ep_by_speed() under if alt is 1. Does your UDC driver call composite_disconnect when disconnect from the host? It should set all desc to NULL at related function ->disable. Peter
>> >Would you please share the use case how to reproduce it? >> We checked again and found NCM gadget driver should not have this issue. >> Please ignore this patch. >> >> However, we still observe similar issue in ECM and RNDIS gadget driver. Take >> ECM as example, if we plug ECM gadget in high-speed host port first. And, the host >> only set interface with alternate setting != 1 (alt != 1 in ecm_set_alt()) for ECM >> gadget. Then, if we unplug gadget from high-speed host port to another super- >> speed host port afterwards, we will see both in_ep->desc and out_ep->desc in ecm- > >port keep values for high-speed endpoints. Although NCM's implementation is >> similar to ECM in this part, NCM doesn't have same issue as it only does >> config_ep_by_speed() under if alt is 1. >Does your UDC driver call composite_disconnect when disconnect from the host? >It should set all desc to NULL at related function ->disable. For ECM case, if ecm_set_alt() gets called with alt == 0, in_ep->desc and out_ep->desc will be set up. But these two ep will not be enabled as gether_connect() is not executed. During disconnect from the host, ecm_disable() gets called with ep disabled. In this case, gether_disconnect() will not get called to set desc to NULL.
> >Does your UDC driver call composite_disconnect when disconnect from the host? > >It should set all desc to NULL at related function ->disable. > > For ECM case, if ecm_set_alt() gets called with alt == 0, in_ep->desc and out_ep- > >desc will be set up. But these two ep will not be enabled as gether_connect() is not > executed. During disconnect from the host, ecm_disable() gets called with ep > disabled. In this case, gether_disconnect() will not get called to set desc to NULL. Would you please share your test case? I use Linux host, and the host will always set alt for 1, and doesn't have this issue. Peter
>> >Does your UDC driver call composite_disconnect when disconnect from the host? >> >It should set all desc to NULL at related function ->disable. >> >> For ECM case, if ecm_set_alt() gets called with alt == 0, in_ep->desc and out_ep- >> >desc will be set up. But these two ep will not be enabled as gether_connect() is not >> executed. During disconnect from the host, ecm_disable() gets called with ep >> disabled. In this case, gether_disconnect() will not get called to set desc to NULL. >Would you please share your test case? I use Linux host, and the host will always set alt for 1, >and doesn't have this issue. Using Windows host (without any proprietary NCM/ECM driver installed) can reproduce set alt to 0. We just used Win10 to confirm this.
> >> >It should set all desc to NULL at related function ->disable. > >> > >> For ECM case, if ecm_set_alt() gets called with alt == 0, in_ep->desc > >> and out_ep- > >> >desc will be set up. But these two ep will not be enabled as > >> >gether_connect() is not > >> executed. During disconnect from the host, ecm_disable() gets called > >> with ep disabled. In this case, gether_disconnect() will not get called to set desc > to NULL. > > >Would you please share your test case? I use Linux host, and the host > >will always set alt for 1, and doesn't have this issue. > Using Windows host (without any proprietary NCM/ECM driver installed) can > reproduce set alt to 0. We just used Win10 to confirm this. Ok, feel free to submit a patch to fix it. Peter
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c index 2d6e76e4cffa..420b9c96f2fe 100644 --- a/drivers/usb/gadget/function/f_ncm.c +++ b/drivers/usb/gadget/function/f_ncm.c @@ -870,11 +870,10 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt) DBG(cdev, "reset ncm control %d\n", intf); usb_ep_disable(ncm->notify); - if (!(ncm->notify->desc)) { - DBG(cdev, "init ncm ctrl %d\n", intf); - if (config_ep_by_speed(cdev->gadget, f, ncm->notify)) - goto fail; - } + DBG(cdev, "init ncm ctrl %d\n", intf); + if (config_ep_by_speed(cdev->gadget, f, ncm->notify)) + goto fail; + usb_ep_enable(ncm->notify); /* Data interface has two altsettings, 0 and 1 */ @@ -897,17 +896,14 @@ static int ncm_set_alt(struct usb_function *f, unsigned intf, unsigned alt) if (alt == 1) { struct net_device *net; - if (!ncm->port.in_ep->desc || - !ncm->port.out_ep->desc) { - DBG(cdev, "init ncm\n"); - if (config_ep_by_speed(cdev->gadget, f, - ncm->port.in_ep) || - config_ep_by_speed(cdev->gadget, f, - ncm->port.out_ep)) { - ncm->port.in_ep->desc = NULL; - ncm->port.out_ep->desc = NULL; - goto fail; - } + DBG(cdev, "init ncm\n"); + if (config_ep_by_speed(cdev->gadget, f, + ncm->port.in_ep) || + config_ep_by_speed(cdev->gadget, f, + ncm->port.out_ep)) { + ncm->port.in_ep->desc = NULL; + ncm->port.out_ep->desc = NULL; + goto fail; } /* TODO */
Gadget driver should always use config_ep_by_speed() to initialize usb_ep struct according to usb device's operating speed. Otherwise, usb_ep struct may be wrong if usb devcie's operating speed is changed. Signed-off-by: Henry Lin <henryl@nvidia.com> --- drivers/usb/gadget/function/f_ncm.c | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-)