Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] usb: gadget: f_ncm: fix wrong usb_ep
@ 2019-11-21  4:18 Henry Lin
  2019-11-25  4:20 ` Peter Chen
  0 siblings, 1 reply; 8+ messages in thread
From: Henry Lin @ 2019-11-21  4:18 UTC (permalink / raw)
  To: unlisted-recipients:; (no To-header on input)
  Cc: Henry Lin, Felipe Balbi, Greg Kroah-Hartman, Romain Izard,
	linux-usb, linux-kernel

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(-)

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: gadget: f_ncm: fix wrong usb_ep
  2019-11-21  4:18 [PATCH] usb: gadget: f_ncm: fix wrong usb_ep Henry Lin
@ 2019-11-25  4:20 ` Peter Chen
  2019-11-26  7:26   ` Henry Lin
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Chen @ 2019-11-25  4:20 UTC (permalink / raw)
  To: Henry Lin
  Cc: Felipe Balbi, Greg Kroah-Hartman, Romain Izard, linux-usb, linux-kernel

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
> 

-- 

Thanks,
Peter Chen

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: gadget: f_ncm: fix wrong usb_ep
  2019-11-25  4:20 ` Peter Chen
@ 2019-11-26  7:26   ` Henry Lin
  2019-11-26  7:55     ` Peter Chen
  0 siblings, 1 reply; 8+ messages in thread
From: Henry Lin @ 2019-11-26  7:26 UTC (permalink / raw)
  To: Peter Chen
  Cc: Felipe Balbi, Greg Kroah-Hartman, Romain Izard, linux-usb, linux-kernel

>> 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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] usb: gadget: f_ncm: fix wrong usb_ep
  2019-11-26  7:26   ` Henry Lin
@ 2019-11-26  7:55     ` Peter Chen
  2019-11-26 10:03       ` Henry Lin
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Chen @ 2019-11-26  7:55 UTC (permalink / raw)
  To: Henry Lin
  Cc: Felipe Balbi, Greg Kroah-Hartman, Romain Izard, linux-usb, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: gadget: f_ncm: fix wrong usb_ep
  2019-11-26  7:55     ` Peter Chen
@ 2019-11-26 10:03       ` Henry Lin
  2019-11-27  1:54         ` Peter Chen
  0 siblings, 1 reply; 8+ messages in thread
From: Henry Lin @ 2019-11-26 10:03 UTC (permalink / raw)
  To: Peter Chen
  Cc: Felipe Balbi, Greg Kroah-Hartman, Romain Izard, linux-usb, linux-kernel

>> >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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] usb: gadget: f_ncm: fix wrong usb_ep
  2019-11-26 10:03       ` Henry Lin
@ 2019-11-27  1:54         ` Peter Chen
  2019-11-28  9:21           ` Henry Lin
  0 siblings, 1 reply; 8+ messages in thread
From: Peter Chen @ 2019-11-27  1:54 UTC (permalink / raw)
  To: Henry Lin
  Cc: Felipe Balbi, Greg Kroah-Hartman, Romain Izard, linux-usb, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] usb: gadget: f_ncm: fix wrong usb_ep
  2019-11-27  1:54         ` Peter Chen
@ 2019-11-28  9:21           ` Henry Lin
  2019-11-28  9:28             ` Peter Chen
  0 siblings, 1 reply; 8+ messages in thread
From: Henry Lin @ 2019-11-28  9:21 UTC (permalink / raw)
  To: Peter Chen
  Cc: Felipe Balbi, Greg Kroah-Hartman, Romain Izard, linux-usb,
	linux-kernel, EJ Hsu

>> >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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH] usb: gadget: f_ncm: fix wrong usb_ep
  2019-11-28  9:21           ` Henry Lin
@ 2019-11-28  9:28             ` Peter Chen
  0 siblings, 0 replies; 8+ messages in thread
From: Peter Chen @ 2019-11-28  9:28 UTC (permalink / raw)
  To: Henry Lin
  Cc: Felipe Balbi, Greg Kroah-Hartman, Romain Izard, linux-usb,
	linux-kernel, EJ Hsu

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-21  4:18 [PATCH] usb: gadget: f_ncm: fix wrong usb_ep Henry Lin
2019-11-25  4:20 ` Peter Chen
2019-11-26  7:26   ` Henry Lin
2019-11-26  7:55     ` Peter Chen
2019-11-26 10:03       ` Henry Lin
2019-11-27  1:54         ` Peter Chen
2019-11-28  9:21           ` Henry Lin
2019-11-28  9:28             ` Peter Chen

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git