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

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

[Gustavo A. R. Silva]

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

[PATCH 2/2] drivers: usb: gadget: udc: remove logically dead code

[Gustavo A. R. Silva]

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

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

[Felipe Balbi]

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

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

[Gustavo A. R. Silva]

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

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

[Felipe Balbi]

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

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

[Greg KH]

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

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

[Felipe Balbi]

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

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

[Michal Nazarewicz]

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»

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

[Greg KH]

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

[PATCH] usb: gadget: mv_udc: clarify a switch with an implicit fall-through

[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