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

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

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