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@xxxxxxxxxxxxxx> writes:
> >> Add missing break in switch.
> >>
> >> Addresses-Coverity-ID: 201385
> >> Signed-off-by: Gustavo A. R. Silva <garsilva@xxxxxxxxxxxxxx>
> >> ---
> >> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
