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
