Re: [PATCH 1/3] usb: dwc2: gadget: fix below build warning

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tuesday, August 19, 2014 8:08 AM, Paul Zimmerman wrote:
> 
> > From: Paul Zimmerman
> > Sent: Monday, August 18, 2014 11:03 AM
> >
> > > From: Jingoo Han [mailto:jg1.han@xxxxxxxxxxx]
> > > Sent: Monday, August 18, 2014 2:17 AM
> > >
> > > On Monday, August 18, 2014 5:32 PM, Peter Chen wrote:
> > > >
> > > > linux-2.6/drivers/usb/dwc2/gadget.c: In function 's3c_hsotg_irq_enumdone':
> > > > linux-2.6/drivers/usb/dwc2/gadget.c:1904: warning: 'ep_mps' may be used uninitialized in this
> function
> > >
> > > (+cc  Dinh Nguyen)
> > >
> > > I think that this warning is false.
> > > Because, 'ep_mps' cannot be used uninitialized in s3c_hsotg_irq_enumdone().
> > >
> > > static void s3c_hsotg_irq_enumdone(struct s3c_hsotg *hsotg)
> > > {
> > > 	int ep0_mps = 0, ep_mps;
> > >
> > > 	case DSTS_ENUMSPD_FS:
> > > 	case DSTS_ENUMSPD_FS48:
> > > 		ep0_mps = EP0_MPS_LIMIT;
> > > 		ep_mps = 1023;
> > > 		break;
> > >
> > > 	case DSTS_ENUMSPD_HS:
> > > 		ep0_mps = EP0_MPS_LIMIT;
> > > 		ep_mps = 1024;
> > > 		break;
> > >
> > > In the case of DSTS_ENUMSPD_FS, DSTS_ENUMSPD_FS48, and DSTS_ENUMSPD_HS,
> > > both 'ep0_mps' and 'ep_mps' are initialized.
> > >
> > > 	case DSTS_ENUMSPD_LS:
> > > 		hsotg->gadget.speed = USB_SPEED_LOW;
> > > 		break;
> > >
> > > In the case of DSTS_ENUMSPD_LS, 'ep_mps' is uninitialized.
> > > But, 'ep0_mps' is also '0'.
> > >
> > > 	.....
> > >
> > > 	if (ep0_mps) {
> > > 		int i;
> > > 		s3c_hsotg_set_ep_maxpacket(hsotg, 0, ep0_mps);
> > > 		for (i = 1; i < hsotg->num_of_eps; i++)
> > > 			s3c_hsotg_set_ep_maxpacket(hsotg, i, ep_mps);
> > > 	}
> > >
> > > If 'ep0_mps' is '0', 'ep_mps' will not be used.
> > > So, uninitialized 'ep_mps' cannot be used.
> > >
> > > However, in the future, it is not sure that the current scheme
> > > will not be modified. Thus, in order to make sure the code safety,
> > > this patch may be necessary.
> >
> > But 0 is not a valid default value for an endpoint mps. So I think the
> > patch serves no purpose other than to shut up the compiler. That's OK
> > with me, but I think the usual kernel practice is not to add unnecessary
> > initializations just to silence a compiler warning.
> 
> Sorry, I lost the context of the original patch because Jingoo didn't
> quote it.

Please don't blame me. It is unrelated with me.
In addition, I did not said that 'ep_mps' is 0.

> I see the patch sets the default mps to 1024, not 0.
> 
> However, 1024 is not a valid mps in the case of full-speed. If you change
> the value to 1023, you can add my acked-by.

No, low-speed should be considered.
In the case of full-speed, 'ep_mps' is set as 1023.
However, in the case of full-speed, 'ep_mps' is not set.

The maximum data payload size allowed for low-speed devices
is 8 bytes. So, this patch should set the default mps as '8'
for the low-speed.

Best regards,
Jingoo Han

> 
> --
> Paul

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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux