> From: Jingoo Han [mailto:jg1.han@xxxxxxxxxxx] > Sent: Tuesday, August 19, 2014 6:16 PM > > 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. Sorry, I didn't mean to blame you. I was just stating a fact - since the original patch wasn't in your email that I was replying to, I mis-remembered what the change was. > > 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. I think the patch is fine as-is. As the comment in that function says, the driver doesn't support low-speed anyway. But if you want to change it to 8, please submit a patch for that, since Peter's patch has already been added to Greg's tree. -- 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
