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

[Greg KH <gregkh@linuxfoundation.org>]

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