All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
@ 2017-02-08  7:22 Gustavo A. R. Silva
  2017-02-08  7:25 ` [PATCH 2/2] drivers: usb: gadget: udc: remove logically dead code Gustavo A. R. Silva
  2017-02-08  9:23 ` [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch Felipe Balbi
  0 siblings, 2 replies; 11+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-08  7:22 UTC (permalink / raw)
  To: gregkh, balbi, bhumirks, mina86; +Cc: linux-usb, linux-kernel

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

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

* [PATCH 2/2] drivers: usb: gadget: udc: remove logically dead code
  2017-02-08  7:22 [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch Gustavo A. R. Silva
@ 2017-02-08  7:25 ` Gustavo A. R. Silva
  2017-02-08  9:23 ` [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch Felipe Balbi
  1 sibling, 0 replies; 11+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-08  7:25 UTC (permalink / raw)
  To: gregkh, balbi, bhumirks, mina86; +Cc: linux-usb, linux-kernel

Remove unnecesary code because zlt never evaluates to zero.

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

diff --git a/drivers/usb/gadget/udc/mv_udc_core.c b/drivers/usb/gadget/udc/mv_udc_core.c
index 56b3574..9ca6d92 100644
--- a/drivers/usb/gadget/udc/mv_udc_core.c
+++ b/drivers/usb/gadget/udc/mv_udc_core.c
@@ -509,7 +509,7 @@ static int mv_ep_enable(struct usb_ep *_ep,
 	dqh = ep->dqh;
 	dqh->max_packet_length = (max << EP_QUEUE_HEAD_MAX_PKT_LEN_POS)
 		| (mult << EP_QUEUE_HEAD_MULT_POS)
-		| (zlt ? EP_QUEUE_HEAD_ZLT_SEL : 0)
+		| EP_QUEUE_HEAD_ZLT_SEL
 		| (ios ? EP_QUEUE_HEAD_IOS : 0);
 	dqh->next_dtd_ptr = 1;
 	dqh->size_ioc_int_sts = 0;
-- 
2.5.0

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

* Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
  2017-02-08  7:22 [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch Gustavo A. R. Silva
  2017-02-08  7:25 ` [PATCH 2/2] drivers: usb: gadget: udc: remove logically dead code Gustavo A. R. Silva
@ 2017-02-08  9:23 ` Felipe Balbi
  2017-02-08 10:02   ` Gustavo A. R. Silva
  2017-02-08 13:16   ` Michal Nazarewicz
  1 sibling, 2 replies; 11+ messages in thread
From: Felipe Balbi @ 2017-02-08  9:23 UTC (permalink / raw)
  To: Gustavo A. R. Silva, gregkh, bhumirks, mina86; +Cc: linux-usb, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 891 bytes --]


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
  2017-02-08  9:23 ` [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch Felipe Balbi
@ 2017-02-08 10:02   ` Gustavo A. R. Silva
  2017-02-08 12:05     ` Felipe Balbi
  2017-02-08 13:16   ` Michal Nazarewicz
  1 sibling, 1 reply; 11+ messages in thread
From: Gustavo A. R. Silva @ 2017-02-08 10:02 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: gregkh, bhumirks, mina86, linux-usb, linux-kernel

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

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

* Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
  2017-02-08 10:02   ` Gustavo A. R. Silva
@ 2017-02-08 12:05     ` Felipe Balbi
  2017-02-08 13:03       ` Greg KH
  0 siblings, 1 reply; 11+ messages in thread
From: Felipe Balbi @ 2017-02-08 12:05 UTC (permalink / raw)
  To: Gustavo A. R. Silva; +Cc: gregkh, bhumirks, mina86, linux-usb, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
  2017-02-08 12:05     ` Felipe Balbi
@ 2017-02-08 13:03       ` Greg KH
  2017-02-08 13:13         ` Felipe Balbi
  0 siblings, 1 reply; 11+ messages in thread
From: Greg KH @ 2017-02-08 13:03 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Gustavo A. R. Silva, bhumirks, mina86, linux-usb, linux-kernel

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

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

* Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
  2017-02-08 13:03       ` Greg KH
@ 2017-02-08 13:13         ` Felipe Balbi
  0 siblings, 0 replies; 11+ messages in thread
From: Felipe Balbi @ 2017-02-08 13:13 UTC (permalink / raw)
  To: Greg KH; +Cc: Gustavo A. R. Silva, bhumirks, mina86, linux-usb, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2016 bytes --]


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
  2017-02-08  9:23 ` [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch Felipe Balbi
  2017-02-08 10:02   ` Gustavo A. R. Silva
@ 2017-02-08 13:16   ` Michal Nazarewicz
  2017-02-08 13:21     ` Greg KH
  1 sibling, 1 reply; 11+ messages in thread
From: Michal Nazarewicz @ 2017-02-08 13:16 UTC (permalink / raw)
  To: Felipe Balbi, Gustavo A. R. Silva, gregkh, bhumirks
  Cc: linux-usb, linux-kernel

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»

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

* Re: [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch
  2017-02-08 13:16   ` Michal Nazarewicz
@ 2017-02-08 13:21     ` Greg KH
  2017-02-08 14:49       ` [PATCH] usb: gadget: mv_udc: clarify a switch with an implicit fall-through Michal Nazarewicz
  0 siblings, 1 reply; 11+ messages in thread
From: Greg KH @ 2017-02-08 13:21 UTC (permalink / raw)
  To: Michal Nazarewicz
  Cc: Felipe Balbi, Gustavo A. R. Silva, bhumirks, linux-usb, linux-kernel

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

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

* [PATCH] usb: gadget: mv_udc: clarify a switch with an implicit fall-through
  2017-02-08 13:21     ` Greg KH
@ 2017-02-08 14:49       ` Michal Nazarewicz
  0 siblings, 0 replies; 11+ messages in thread
From: Michal Nazarewicz @ 2017-02-08 14:49 UTC (permalink / raw)
  To: Greg KH; +Cc: Felipe Balbi, linux-kernel, linux-usb, Michal Nazarewicz

Rearrange statements in mv_ep_enable function so that it’s obvious
what the switch does and how zlt, ios and mult variables are
initialised.  Most notably, this gets rid of an implicit fall-through
so people don’t have to wonder whether it was intenional or not.

Addresses-Coverity-ID: 201385
Reported-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/udc/mv_udc_core.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

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 */
-- 
2.11.0.483.g087da7b7c-goog

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

* [PATCH] usb: gadget: mv_udc: clarify a switch with an implicit fall-through
@ 2017-03-01 14:34 Michal Nazarewicz
  0 siblings, 0 replies; 11+ messages in thread
From: Michal Nazarewicz @ 2017-03-01 14:34 UTC (permalink / raw)
  To: Felipe Balbi; +Cc: linux-kernel, linux-usb, Michal Nazarewicz

Rearrange statements in mv_ep_enable function so that it’s obvious
what the switch does and how zlt, ios and mult variables are
initialised.  Most notably, this gets rid of an implicit fall-through
so people don’t have to wonder whether it was intenional or not.

Addresses-Coverity-ID: 201385
Reported-by: Gustavo A. R. Silva <garsilva@embeddedor.com>
Signed-off-by: Michal Nazarewicz <mina86@mina86.com>
---
 drivers/usb/gadget/udc/mv_udc_core.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/udc/mv_udc_core.c b/drivers/usb/gadget/udc/mv_udc_core.c
index 27ebb0d5449d..76f56c5762f9 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 */
-- 
2.12.0.rc1.440.g5b76565f74-goog

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

end of thread, other threads:[~2017-03-01 14:46 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-08  7:22 [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch Gustavo A. R. Silva
2017-02-08  7:25 ` [PATCH 2/2] drivers: usb: gadget: udc: remove logically dead code Gustavo A. R. Silva
2017-02-08  9:23 ` [PATCH 1/2] drivers: usb: gadget: udc: add missing break in switch Felipe Balbi
2017-02-08 10:02   ` Gustavo A. R. Silva
2017-02-08 12:05     ` Felipe Balbi
2017-02-08 13:03       ` Greg KH
2017-02-08 13:13         ` Felipe Balbi
2017-02-08 13:16   ` Michal Nazarewicz
2017-02-08 13:21     ` Greg KH
2017-02-08 14:49       ` [PATCH] usb: gadget: mv_udc: clarify a switch with an implicit fall-through Michal Nazarewicz
2017-03-01 14:34 Michal Nazarewicz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.