usb: gadget: f_acm: don't disable disabled EP
diff mbox series

Message ID 237e4bc8c63680f9ce0388d35b4c34a856ed8595.1590690518.git.mirq-linux@rere.qmqm.pl
State Accepted
Commit e7a0ed3fa31be13a4bf5f81426cb4db560e031ee
Headers show
Series
  • usb: gadget: f_acm: don't disable disabled EP
Related show

Commit Message

Michał Mirosław May 28, 2020, 6:30 p.m. UTC
Make debugging real problems easier by not trying to disable an EP that
was not yet enabled.

Fixes: 4aab757ca44a ("usb: gadget: f_acm: eliminate abuse of ep->driver data")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/usb/gadget/function/f_acm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Peter Chen May 29, 2020, 8:10 a.m. UTC | #1
On 20-05-28 20:30:28, Michał Mirosław wrote:
> Make debugging real problems easier by not trying to disable an EP that
> was not yet enabled.
> 
> Fixes: 4aab757ca44a ("usb: gadget: f_acm: eliminate abuse of ep->driver data")
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/usb/gadget/function/f_acm.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_acm.c b/drivers/usb/gadget/function/f_acm.c
> index 200596ea9557..46647bfac2ef 100644
> --- a/drivers/usb/gadget/function/f_acm.c
> +++ b/drivers/usb/gadget/function/f_acm.c
> @@ -425,9 +425,11 @@ static int acm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>  	/* we know alt == 0, so this is an activation or a reset */
>  
>  	if (intf == acm->ctrl_id) {
> -		dev_vdbg(&cdev->gadget->dev,
> -				"reset acm control interface %d\n", intf);
> -		usb_ep_disable(acm->notify);
> +		if (acm->notify->enabled) {
> +			dev_vdbg(&cdev->gadget->dev,
> +					"reset acm control interface %d\n", intf);
> +			usb_ep_disable(acm->notify);
> +		}

But it does not fix any issues, the usb_ep_disable checks 'enabled' flag.
Michał Mirosław May 29, 2020, 1:55 p.m. UTC | #2
On Fri, May 29, 2020 at 08:10:40AM +0000, Peter Chen wrote:
> On 20-05-28 20:30:28, Michał Mirosław wrote:
> > Make debugging real problems easier by not trying to disable an EP that
> > was not yet enabled.
> > 
> > Fixes: 4aab757ca44a ("usb: gadget: f_acm: eliminate abuse of ep->driver data")
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> >  drivers/usb/gadget/function/f_acm.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/function/f_acm.c b/drivers/usb/gadget/function/f_acm.c
> > index 200596ea9557..46647bfac2ef 100644
> > --- a/drivers/usb/gadget/function/f_acm.c
> > +++ b/drivers/usb/gadget/function/f_acm.c
> > @@ -425,9 +425,11 @@ static int acm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
> >  	/* we know alt == 0, so this is an activation or a reset */
> >  
> >  	if (intf == acm->ctrl_id) {
> > -		dev_vdbg(&cdev->gadget->dev,
> > -				"reset acm control interface %d\n", intf);
> > -		usb_ep_disable(acm->notify);
> > +		if (acm->notify->enabled) {
> > +			dev_vdbg(&cdev->gadget->dev,
> > +					"reset acm control interface %d\n", intf);
> > +			usb_ep_disable(acm->notify);
> > +		}
> 
> But it does not fix any issues, the usb_ep_disable checks 'enabled' flag.

It generates spurious trace events if you enable them.

Best Regards,
Michał Mirosław
Peter Chen May 30, 2020, 1:03 a.m. UTC | #3
> > > @@ -425,9 +425,11 @@ static int acm_set_alt(struct usb_function *f, unsigned
> intf, unsigned alt)
> > >  	/* we know alt == 0, so this is an activation or a reset */
> > >
> > >  	if (intf == acm->ctrl_id) {
> > > -		dev_vdbg(&cdev->gadget->dev,
> > > -				"reset acm control interface %d\n", intf);
> > > -		usb_ep_disable(acm->notify);
> > > +		if (acm->notify->enabled) {
> > > +			dev_vdbg(&cdev->gadget->dev,
> > > +					"reset acm control interface %d\n", intf);
> > > +			usb_ep_disable(acm->notify);
> > > +		}
> >
> > But it does not fix any issues, the usb_ep_disable checks 'enabled' flag.
> 
> It generates spurious trace events if you enable them.
> 

You mean the trace events from core.c? If it is, we could try to improve it
and indicate it is already enabled or disabled.

Peter
Michał Mirosław May 30, 2020, 5:15 p.m. UTC | #4
On Sat, May 30, 2020 at 01:03:17AM +0000, Peter Chen wrote:
>  
> > > > @@ -425,9 +425,11 @@ static int acm_set_alt(struct usb_function *f, unsigned
> > intf, unsigned alt)
> > > >  	/* we know alt == 0, so this is an activation or a reset */
> > > >
> > > >  	if (intf == acm->ctrl_id) {
> > > > -		dev_vdbg(&cdev->gadget->dev,
> > > > -				"reset acm control interface %d\n", intf);
> > > > -		usb_ep_disable(acm->notify);
> > > > +		if (acm->notify->enabled) {
> > > > +			dev_vdbg(&cdev->gadget->dev,
> > > > +					"reset acm control interface %d\n", intf);
> > > > +			usb_ep_disable(acm->notify);
> > > > +		}
> > >
> > > But it does not fix any issues, the usb_ep_disable checks 'enabled' flag.
> > 
> > It generates spurious trace events if you enable them.
> You mean the trace events from core.c? If it is, we could try to improve it
> and indicate it is already enabled or disabled.

It is indicated in return code, but the problem is that this generates
noise and wastes debugging time. The problem I was seeing manifested
itself as disabling disabled EPs and desync of EP state between core
and UDC driver. The patch avoids the noise and makes the code obvious.
(This check was there at some point in time, BTW.)

Best Regards,
Michał Mirosław
Peter Chen June 1, 2020, 3:53 a.m. UTC | #5
On 20-05-30 19:15:52, Michał Mirosław wrote:
> On Sat, May 30, 2020 at 01:03:17AM +0000, Peter Chen wrote:
> >  
> > > > > @@ -425,9 +425,11 @@ static int acm_set_alt(struct usb_function *f, unsigned
> > > intf, unsigned alt)
> > > > >  	/* we know alt == 0, so this is an activation or a reset */
> > > > >
> > > > >  	if (intf == acm->ctrl_id) {
> > > > > -		dev_vdbg(&cdev->gadget->dev,
> > > > > -				"reset acm control interface %d\n", intf);
> > > > > -		usb_ep_disable(acm->notify);
> > > > > +		if (acm->notify->enabled) {
> > > > > +			dev_vdbg(&cdev->gadget->dev,
> > > > > +					"reset acm control interface %d\n", intf);
> > > > > +			usb_ep_disable(acm->notify);
> > > > > +		}
> > > >
> > > > But it does not fix any issues, the usb_ep_disable checks 'enabled' flag.
> > > 
> > > It generates spurious trace events if you enable them.
> > You mean the trace events from core.c? If it is, we could try to improve it
> > and indicate it is already enabled or disabled.
> 
> It is indicated in return code, but the problem is that this generates
> noise and wastes debugging time. The problem I was seeing manifested
> itself as disabling disabled EPs and desync of EP state between core
> and UDC driver. The patch avoids the noise and makes the code obvious.
> (This check was there at some point in time, BTW.)
> 

Reviewed-by: Peter Chen <peter.chen@nxp.com>
Felipe Balbi July 1, 2020, 6:53 a.m. UTC | #6
Peter Chen <peter.chen@nxp.com> writes:

>  
>> > > @@ -425,9 +425,11 @@ static int acm_set_alt(struct usb_function *f, unsigned
>> intf, unsigned alt)
>> > >  	/* we know alt == 0, so this is an activation or a reset */
>> > >
>> > >  	if (intf == acm->ctrl_id) {
>> > > -		dev_vdbg(&cdev->gadget->dev,
>> > > -				"reset acm control interface %d\n", intf);
>> > > -		usb_ep_disable(acm->notify);
>> > > +		if (acm->notify->enabled) {
>> > > +			dev_vdbg(&cdev->gadget->dev,
>> > > +					"reset acm control interface %d\n", intf);
>> > > +			usb_ep_disable(acm->notify);
>> > > +		}
>> >
>> > But it does not fix any issues, the usb_ep_disable checks 'enabled' flag.
>> 
>> It generates spurious trace events if you enable them.
>> 
>
> You mean the trace events from core.c? If it is, we could try to improve it
> and indicate it is already enabled or disabled.

I agree :-)
Felipe Balbi July 1, 2020, 6:54 a.m. UTC | #7
Michał Mirosław <mirq-linux@rere.qmqm.pl> writes:

> On Sat, May 30, 2020 at 01:03:17AM +0000, Peter Chen wrote:
>>  
>> > > > @@ -425,9 +425,11 @@ static int acm_set_alt(struct usb_function *f, unsigned
>> > intf, unsigned alt)
>> > > >  	/* we know alt == 0, so this is an activation or a reset */
>> > > >
>> > > >  	if (intf == acm->ctrl_id) {
>> > > > -		dev_vdbg(&cdev->gadget->dev,
>> > > > -				"reset acm control interface %d\n", intf);
>> > > > -		usb_ep_disable(acm->notify);
>> > > > +		if (acm->notify->enabled) {
>> > > > +			dev_vdbg(&cdev->gadget->dev,
>> > > > +					"reset acm control interface %d\n", intf);
>> > > > +			usb_ep_disable(acm->notify);
>> > > > +		}
>> > >
>> > > But it does not fix any issues, the usb_ep_disable checks 'enabled' flag.
>> > 
>> > It generates spurious trace events if you enable them.
>> You mean the trace events from core.c? If it is, we could try to improve it
>> and indicate it is already enabled or disabled.
>
> It is indicated in return code, but the problem is that this generates
> noise and wastes debugging time. The problem I was seeing manifested
> itself as disabling disabled EPs and desync of EP state between core
> and UDC driver. The patch avoids the noise and makes the code obvious.
> (This check was there at some point in time, BTW.)

I agree with this as well. But still, $subject doesn't look like a
candidate for the -rc :-) I'll apply it for the next merge window.

Patch
diff mbox series

diff --git a/drivers/usb/gadget/function/f_acm.c b/drivers/usb/gadget/function/f_acm.c
index 200596ea9557..46647bfac2ef 100644
--- a/drivers/usb/gadget/function/f_acm.c
+++ b/drivers/usb/gadget/function/f_acm.c
@@ -425,9 +425,11 @@  static int acm_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 	/* we know alt == 0, so this is an activation or a reset */
 
 	if (intf == acm->ctrl_id) {
-		dev_vdbg(&cdev->gadget->dev,
-				"reset acm control interface %d\n", intf);
-		usb_ep_disable(acm->notify);
+		if (acm->notify->enabled) {
+			dev_vdbg(&cdev->gadget->dev,
+					"reset acm control interface %d\n", intf);
+			usb_ep_disable(acm->notify);
+		}
 
 		if (!acm->notify->desc)
 			if (config_ep_by_speed(cdev->gadget, f, acm->notify))