Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1576332 > unrolled thread

[PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch

Started by"Gustavo A. R. Silva" <garsilva@embeddedor.com>
First post2017-02-08 09:20 +0100
Last post2017-02-08 18:40 +0100
Articles 8 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-02-08 09:20 +0100
    Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch Felipe Balbi <balbi@kernel.org> - 2017-02-08 10:30 +0100
      Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in  switch "Gustavo A. R. Silva" <garsilva@embeddedor.com> - 2017-02-08 12:00 +0100
        Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch Felipe Balbi <balbi@kernel.org> - 2017-02-08 13:10 +0100
          Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in  switch Greg KH <gregkh@linuxfoundation.org> - 2017-02-08 14:20 +0100
            Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch Felipe Balbi <balbi@kernel.org> - 2017-02-08 14:20 +0100
      Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch Michal Nazarewicz <mina86@mina86.com> - 2017-02-08 14:30 +0100
        Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in  switch Greg KH <gregkh@linuxfoundation.org> - 2017-02-08 18:40 +0100

#1576332 — [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch

From"Gustavo A. R. Silva" <garsilva@embeddedor.com>
Date2017-02-08 09:20 +0100
Subject[PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
Message-ID<t8vaO-5So-5@gated-at.bofh.it>
Add missing break in switch.

Addresses-Coverity-ID: 201385
Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
---
 drivers/usb/gadget/udc/mv_udc_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/udc/mv_udc_core.c b/drivers/usb/gadget/udc/mv_udc_core.c
index 27ebb0d..56b3574 100644
--- a/drivers/usb/gadget/udc/mv_udc_core.c
+++ b/drivers/usb/gadget/udc/mv_udc_core.c
@@ -489,6 +489,7 @@ static int mv_ep_enable(struct usb_ep *_ep,
 		break;
 	case USB_ENDPOINT_XFER_CONTROL:
 		ios = 1;
+		break;
 	case USB_ENDPOINT_XFER_INT:
 		mult = 0;
 		break;
-- 
2.5.0

[toc] | [next] | [standalone]

#1576385

FromFelipe Balbi <balbi@kernel.org>
Date2017-02-08 10:30 +0100
Message-ID<t8wJA-6VL-3@gated-at.bofh.it>
In reply to#1576332

[Multipart message — attachments visible in raw view] — view raw

Hi,

"Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
> Add missing break in switch.
>
> Addresses-Coverity-ID: 201385
> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> ---
>  drivers/usb/gadget/udc/mv_udc_core.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/udc/mv_udc_core.c b/drivers/usb/gadget/udc/mv_udc_core.c
> index 27ebb0d..56b3574 100644
> --- a/drivers/usb/gadget/udc/mv_udc_core.c
> +++ b/drivers/usb/gadget/udc/mv_udc_core.c
> @@ -489,6 +489,7 @@ static int mv_ep_enable(struct usb_ep *_ep,
>  		break;
>  	case USB_ENDPOINT_XFER_CONTROL:
>  		ios = 1;
> +		break;

are you SURE this is supposed to have this break statement? What if we
want to initialize mult to 0 *also* for control endpoints? How did you
test this? Do you have access to Marvel's documentation for this
controller?

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1576442 — Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch

From"Gustavo A. R. Silva" <garsilva@embeddedor.com>
Date2017-02-08 12:00 +0100
SubjectRe: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
Message-ID<t8y8H-7Ir-45@gated-at.bofh.it>
In reply to#1576385
Hello Felipe,

Quoting Felipe Balbi <balbi@kernel.org>:

> Hi,
>
> "Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
>> Add missing break in switch.
>>
>> Addresses-Coverity-ID: 201385
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> ---
>>  drivers/usb/gadget/udc/mv_udc_core.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/gadget/udc/mv_udc_core.c  
>> b/drivers/usb/gadget/udc/mv_udc_core.c
>> index 27ebb0d..56b3574 100644
>> --- a/drivers/usb/gadget/udc/mv_udc_core.c
>> +++ b/drivers/usb/gadget/udc/mv_udc_core.c
>> @@ -489,6 +489,7 @@ static int mv_ep_enable(struct usb_ep *_ep,
>>  		break;
>>  	case USB_ENDPOINT_XFER_CONTROL:
>>  		ios = 1;
>> +		break;
>
> are you SURE this is supposed to have this break statement? What if we
> want to initialize mult to 0 *also* for control endpoints? How did you
> test this? Do you have access to Marvel's documentation for this
> controller?
>

Certainly I wasn't sure, but I also think this is kind of obscure  
code. If that is the case that we also want to initialize mult to 0,  
wouldn't it be clearer (for maintenance purposes) to add mult = 0 and  
the break statement after ios = 1?

What do you think if I modify that piece of code as follows:

case USB_ENDPOINT_XFER_CONTROL:
  	ios = 1;
         mult = 0;
	break;


Thank you
--
Gustavo A. R. Silva

[toc] | [prev] | [next] | [standalone]

#1576494

FromFelipe Balbi <balbi@kernel.org>
Date2017-02-08 13:10 +0100
Message-ID<t8zeq-a8-27@gated-at.bofh.it>
In reply to#1576442

[Multipart message — attachments visible in raw view] — view raw

Hi,

"Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
>> "Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
>>> Add missing break in switch.
>>>
>>> Addresses-Coverity-ID: 201385
>>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>>> ---
>>>  drivers/usb/gadget/udc/mv_udc_core.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/usb/gadget/udc/mv_udc_core.c  
>>> b/drivers/usb/gadget/udc/mv_udc_core.c
>>> index 27ebb0d..56b3574 100644
>>> --- a/drivers/usb/gadget/udc/mv_udc_core.c
>>> +++ b/drivers/usb/gadget/udc/mv_udc_core.c
>>> @@ -489,6 +489,7 @@ static int mv_ep_enable(struct usb_ep *_ep,
>>>  		break;
>>>  	case USB_ENDPOINT_XFER_CONTROL:
>>>  		ios = 1;
>>> +		break;
>>
>> are you SURE this is supposed to have this break statement? What if we
>> want to initialize mult to 0 *also* for control endpoints? How did you
>> test this? Do you have access to Marvel's documentation for this
>> controller?
>>
>
> Certainly I wasn't sure, but I also think this is kind of obscure  
> code. If that is the case that we also want to initialize mult to 0,  
> wouldn't it be clearer (for maintenance purposes) to add mult = 0 and  
> the break statement after ios = 1?
>
> What do you think if I modify that piece of code as follows:

I think you need to test it, or get someone to test it for you :-)

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1576556 — Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch

FromGreg KH <gregkh@linuxfoundation.org>
Date2017-02-08 14:20 +0100
SubjectRe: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
Message-ID<t8Aka-O0-13@gated-at.bofh.it>
In reply to#1576494
On Wed, Feb 08, 2017 at 02:05:35PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> "Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
> >> "Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
> >>> Add missing break in switch.
> >>>
> >>> Addresses-Coverity-ID: 201385
> >>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> >>> ---
> >>>  drivers/usb/gadget/udc/mv_udc_core.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/usb/gadget/udc/mv_udc_core.c  
> >>> b/drivers/usb/gadget/udc/mv_udc_core.c
> >>> index 27ebb0d..56b3574 100644
> >>> --- a/drivers/usb/gadget/udc/mv_udc_core.c
> >>> +++ b/drivers/usb/gadget/udc/mv_udc_core.c
> >>> @@ -489,6 +489,7 @@ static int mv_ep_enable(struct usb_ep *_ep,
> >>>  		break;
> >>>  	case USB_ENDPOINT_XFER_CONTROL:
> >>>  		ios = 1;
> >>> +		break;
> >>
> >> are you SURE this is supposed to have this break statement? What if we
> >> want to initialize mult to 0 *also* for control endpoints? How did you
> >> test this? Do you have access to Marvel's documentation for this
> >> controller?
> >>
> >
> > Certainly I wasn't sure, but I also think this is kind of obscure  
> > code. If that is the case that we also want to initialize mult to 0,  
> > wouldn't it be clearer (for maintenance purposes) to add mult = 0 and  
> > the break statement after ios = 1?
> >
> > What do you think if I modify that piece of code as follows:
> 
> I think you need to test it, or get someone to test it for you :-)

For crap code like this where it's "obvious" that something is wrong?
That's really hard.

How about a nice comment instead:
	/* Code path falls through, is it correct or not, who knows??? */
which will make the static code checkers stop complaining about it, and
if someone actually has the hardware, then they can test it.

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1576570

FromFelipe Balbi <balbi@kernel.org>
Date2017-02-08 14:20 +0100
Message-ID<t8Akb-O0-51@gated-at.bofh.it>
In reply to#1576556

[Multipart message — attachments visible in raw view] — view raw

Hi,

Greg KH <gregkh@linuxfoundation.org> writes:
> On Wed, Feb 08, 2017 at 02:05:35PM +0200, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> "Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
>> >> "Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
>> >>> Add missing break in switch.
>> >>>
>> >>> Addresses-Coverity-ID: 201385
>> >>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> >>> ---
>> >>>  drivers/usb/gadget/udc/mv_udc_core.c | 1 +
>> >>>  1 file changed, 1 insertion(+)
>> >>>
>> >>> diff --git a/drivers/usb/gadget/udc/mv_udc_core.c  
>> >>> b/drivers/usb/gadget/udc/mv_udc_core.c
>> >>> index 27ebb0d..56b3574 100644
>> >>> --- a/drivers/usb/gadget/udc/mv_udc_core.c
>> >>> +++ b/drivers/usb/gadget/udc/mv_udc_core.c
>> >>> @@ -489,6 +489,7 @@ static int mv_ep_enable(struct usb_ep *_ep,
>> >>>  		break;
>> >>>  	case USB_ENDPOINT_XFER_CONTROL:
>> >>>  		ios = 1;
>> >>> +		break;
>> >>
>> >> are you SURE this is supposed to have this break statement? What if we
>> >> want to initialize mult to 0 *also* for control endpoints? How did you
>> >> test this? Do you have access to Marvel's documentation for this
>> >> controller?
>> >>
>> >
>> > Certainly I wasn't sure, but I also think this is kind of obscure  
>> > code. If that is the case that we also want to initialize mult to 0,  
>> > wouldn't it be clearer (for maintenance purposes) to add mult = 0 and  
>> > the break statement after ios = 1?
>> >
>> > What do you think if I modify that piece of code as follows:
>> 
>> I think you need to test it, or get someone to test it for you :-)
>
> For crap code like this where it's "obvious" that something is wrong?
> That's really hard.

heh :-)

> How about a nice comment instead:
> 	/* Code path falls through, is it correct or not, who knows??? */
> which will make the static code checkers stop complaining about it, and
> if someone actually has the hardware, then they can test it.

works for me

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1576576

FromMichal Nazarewicz <mina86@mina86.com>
Date2017-02-08 14:30 +0100
Message-ID<t8AtQ-Rq-13@gated-at.bofh.it>
In reply to#1576385
On Wed, Feb 08 2017, Felipe Balbi wrote:
> Hi,
>
> "Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
>> Add missing break in switch.
>>
>> Addresses-Coverity-ID: 201385
>> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
>> ---
>>  drivers/usb/gadget/udc/mv_udc_core.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/gadget/udc/mv_udc_core.c b/drivers/usb/gadget/udc/mv_udc_core.c
>> index 27ebb0d..56b3574 100644
>> --- a/drivers/usb/gadget/udc/mv_udc_core.c
>> +++ b/drivers/usb/gadget/udc/mv_udc_core.c
>> @@ -489,6 +489,7 @@ static int mv_ep_enable(struct usb_ep *_ep,
>>  		break;
>>  	case USB_ENDPOINT_XFER_CONTROL:
>>  		ios = 1;
>> +		break;
>
> are you SURE this is supposed to have this break statement? What if we
> want to initialize mult to 0 *also* for control endpoints? How did you
> test this? Do you have access to Marvel's documentation for this
> controller?

Actually it doesn’t matter.  mult is initialised to zero when it’s
declared and then not touched before the switch.

To improve readability, maybe something like:

---- >8 ------------------------------------------------------------
diff --git a/drivers/usb/gadget/udc/mv_udc_core.c b/drivers/usb/gadget/udc/mv_udc_core.c
index d82a91bddbd9..7440c9f92ec1 100644
--- a/drivers/usb/gadget/udc/mv_udc_core.c
+++ b/drivers/usb/gadget/udc/mv_udc_core.c
@@ -445,7 +445,8 @@ static int mv_ep_enable(struct usb_ep *_ep,
        struct mv_dqh *dqh;
        u16 max = 0;
        u32 bit_pos, epctrlx, direction;
-       unsigned char zlt = 0, ios = 0, mult = 0;
+       const unsigned char zlt = 1;
+       unsigned char ios, mult;
        unsigned long flags;
 
        ep = container_of(_ep, struct mv_ep, ep);
@@ -465,8 +466,6 @@ static int mv_ep_enable(struct usb_ep *_ep,
         * disable HW zero length termination select
         * driver handles zero length packet through req->req.zero
         */
-       zlt = 1;
-
        bit_pos = 1 << ((direction == EP_DIR_OUT ? 0 : 16) + ep->ep_num);
 
        /* Check if the Endpoint is Primed */
@@ -481,16 +480,16 @@ static int mv_ep_enable(struct usb_ep *_ep,
                        (unsigned)bit_pos);
                goto en_done;
        }
+
        /* Set the max packet length, interrupt on Setup and Mult fields */
+       ios = 0;
+       mult = 0;
        switch (desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) {
        case USB_ENDPOINT_XFER_BULK:
-               zlt = 1;
-               mult = 0;
+       case USB_ENDPOINT_XFER_INT:
                break;
        case USB_ENDPOINT_XFER_CONTROL:
                ios = 1;
-       case USB_ENDPOINT_XFER_INT:
-               mult = 0;
                break;
        case USB_ENDPOINT_XFER_ISOC:
                /* Calculate transactions needed for high bandwidth iso */
---- >8 ------------------------------------------------------------


-- 
Best regards
ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»

[toc] | [prev] | [next] | [standalone]

#1576776 — Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch

FromGreg KH <gregkh@linuxfoundation.org>
Date2017-02-08 18:40 +0100
SubjectRe: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
Message-ID<t8BJg-1xy-13@gated-at.bofh.it>
In reply to#1576576
On Wed, Feb 08, 2017 at 02:16:27PM +0100, Michal Nazarewicz wrote:
> On Wed, Feb 08 2017, Felipe Balbi wrote:
> > Hi,
> >
> > "Gustavo A. R. Silva" <garsilva@embeddedor.com> writes:
> >> Add missing break in switch.
> >>
> >> Addresses-Coverity-ID: 201385
> >> Signed-off-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
> >> ---
> >>  drivers/usb/gadget/udc/mv_udc_core.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/usb/gadget/udc/mv_udc_core.c b/drivers/usb/gadget/udc/mv_udc_core.c
> >> index 27ebb0d..56b3574 100644
> >> --- a/drivers/usb/gadget/udc/mv_udc_core.c
> >> +++ b/drivers/usb/gadget/udc/mv_udc_core.c
> >> @@ -489,6 +489,7 @@ static int mv_ep_enable(struct usb_ep *_ep,
> >>  		break;
> >>  	case USB_ENDPOINT_XFER_CONTROL:
> >>  		ios = 1;
> >> +		break;
> >
> > are you SURE this is supposed to have this break statement? What if we
> > want to initialize mult to 0 *also* for control endpoints? How did you
> > test this? Do you have access to Marvel's documentation for this
> > controller?
> 
> Actually it doesn’t matter.  mult is initialised to zero when it’s
> declared and then not touched before the switch.
> 
> To improve readability, maybe something like:
> 
> ---- >8 ------------------------------------------------------------
> diff --git a/drivers/usb/gadget/udc/mv_udc_core.c b/drivers/usb/gadget/udc/mv_udc_core.c
> index d82a91bddbd9..7440c9f92ec1 100644
> --- a/drivers/usb/gadget/udc/mv_udc_core.c
> +++ b/drivers/usb/gadget/udc/mv_udc_core.c
> @@ -445,7 +445,8 @@ static int mv_ep_enable(struct usb_ep *_ep,
>         struct mv_dqh *dqh;
>         u16 max = 0;
>         u32 bit_pos, epctrlx, direction;
> -       unsigned char zlt = 0, ios = 0, mult = 0;
> +       const unsigned char zlt = 1;
> +       unsigned char ios, mult;
>         unsigned long flags;
>  
>         ep = container_of(_ep, struct mv_ep, ep);
> @@ -465,8 +466,6 @@ static int mv_ep_enable(struct usb_ep *_ep,
>          * disable HW zero length termination select
>          * driver handles zero length packet through req->req.zero
>          */
> -       zlt = 1;
> -
>         bit_pos = 1 << ((direction == EP_DIR_OUT ? 0 : 16) + ep->ep_num);
>  
>         /* Check if the Endpoint is Primed */
> @@ -481,16 +480,16 @@ static int mv_ep_enable(struct usb_ep *_ep,
>                         (unsigned)bit_pos);
>                 goto en_done;
>         }
> +
>         /* Set the max packet length, interrupt on Setup and Mult fields */
> +       ios = 0;
> +       mult = 0;
>         switch (desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) {
>         case USB_ENDPOINT_XFER_BULK:
> -               zlt = 1;
> -               mult = 0;
> +       case USB_ENDPOINT_XFER_INT:
>                 break;
>         case USB_ENDPOINT_XFER_CONTROL:
>                 ios = 1;
> -       case USB_ENDPOINT_XFER_INT:
> -               mult = 0;
>                 break;
>         case USB_ENDPOINT_XFER_ISOC:
>                 /* Calculate transactions needed for high bandwidth iso */
> ---- >8 ------------------------------------------------------------

Ah, looks even better.  Want to resend this in a "proper" format that we
can apply it in?

thanks,

greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web