Re: [PATCH] usbip: vudc: Refactor init_vudc_hw() to be more obvious
On 12/02/2016 04:37 PM, Shuah Khan wrote: > On 12/02/2016 08:27 AM, Krzysztof Opasiak wrote: >> >> >> On 12/02/2016 04:15 PM, Shuah Khan wrote: >>> Hi Krzysztof, >>> >>> Thanks for the patch. >>> >>> On 12/01/2016 10:02 AM, Krzysztof Opasiak wrote: Current implementation of init_vudc_hw() adds ep0 to ep_list and then after looping through all endpoints removes it from that list. As this may be misleading let's refactor this function and avoid adding and removing ep0 to eplist and place it immediately in correct place. >>> Signed-off-by: Krzysztof Opasiak --- drivers/usb/usbip/vudc_dev.c | 38 +- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c index 7091848..a5ca29b 100644 --- a/drivers/usb/usbip/vudc_dev.c +++ b/drivers/usb/usbip/vudc_dev.c @@ -549,30 +549,37 @@ static int init_vudc_hw(struct vudc *udc) sprintf(ep->name, "ep%d%s", num, i ? (is_out ? "out" : "in") : ""); ep->ep.name = ep->name; + + ep->ep.ops = &vep_ops; + + ep->halted = ep->wedged = ep->already_seen = + ep->setup_stage = 0; >>> >>> Do you need to clear these explicitly. kcalloc() should do it for you. >> >> Well, that's true. I may remove this if you like. > > Please do. It is redundant. > I will do this and send v2 shortly. Thanks, -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usbip: vudc: Refactor init_vudc_hw() to be more obvious
Hi Krzysztof, Thanks for the patch. On 12/01/2016 10:02 AM, Krzysztof Opasiak wrote: > Current implementation of init_vudc_hw() adds ep0 to ep_list > and then after looping through all endpoints removes it from > that list. > > As this may be misleading let's refactor this function > and avoid adding and removing ep0 to eplist and place it > immediately in correct place. > > Signed-off-by: Krzysztof Opasiak > --- > drivers/usb/usbip/vudc_dev.c | 38 +- > 1 file changed, 21 insertions(+), 17 deletions(-) > > diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c > index 7091848..a5ca29b 100644 > --- a/drivers/usb/usbip/vudc_dev.c > +++ b/drivers/usb/usbip/vudc_dev.c > @@ -549,30 +549,37 @@ static int init_vudc_hw(struct vudc *udc) > sprintf(ep->name, "ep%d%s", num, > i ? (is_out ? "out" : "in") : ""); > ep->ep.name = ep->name; > + > + ep->ep.ops = &vep_ops; > + > + ep->halted = ep->wedged = ep->already_seen = > + ep->setup_stage = 0; Do you need to clear these explicitly. kcalloc() should do it for you. > + usb_ep_set_maxpacket_limit(&ep->ep, ~0); > + ep->ep.max_streams = 16; > + ep->gadget = &udc->gadget; > + ep->desc = NULL; > + INIT_LIST_HEAD(&ep->req_queue); > + > if (i == 0) { > + /* ep0 */ > ep->ep.caps.type_control = true; > ep->ep.caps.dir_out = true; > ep->ep.caps.dir_in = true; > + > + udc->gadget.ep0 = &ep->ep; > } else { > + /* All other eps */ > ep->ep.caps.type_iso = true; > ep->ep.caps.type_int = true; > ep->ep.caps.type_bulk = true; > - } > > - if (is_out) > - ep->ep.caps.dir_out = true; > - else > - ep->ep.caps.dir_in = true; > + if (is_out) > + ep->ep.caps.dir_out = true; > + else > + ep->ep.caps.dir_in = true; You are moving the is_out handling which was common for all eps including ep0 under non-eop0 - is that right? > > - ep->ep.ops = &vep_ops; > - list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list); > - ep->halted = ep->wedged = ep->already_seen = > - ep->setup_stage = 0; > - usb_ep_set_maxpacket_limit(&ep->ep, ~0); > - ep->ep.max_streams = 16; > - ep->gadget = &udc->gadget; > - ep->desc = NULL; > - INIT_LIST_HEAD(&ep->req_queue); > + list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list); > + } > } > > spin_lock_init(&udc->lock); > @@ -589,9 +596,6 @@ static int init_vudc_hw(struct vudc *udc) > ud->eh_ops.reset= vudc_device_reset; > ud->eh_ops.unusable = vudc_device_unusable; > > - udc->gadget.ep0 = &udc->ep[0].ep; > - list_del_init(&udc->ep[0].ep.ep_list); > - > v_init_timer(udc); > return 0; > > thanks, -- Shuah -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usbip: vudc: Refactor init_vudc_hw() to be more obvious
On 12/02/2016 08:27 AM, Krzysztof Opasiak wrote: > > > On 12/02/2016 04:15 PM, Shuah Khan wrote: >> Hi Krzysztof, >> >> Thanks for the patch. >> >> On 12/01/2016 10:02 AM, Krzysztof Opasiak wrote: >>> Current implementation of init_vudc_hw() adds ep0 to ep_list >>> and then after looping through all endpoints removes it from >>> that list. >>> >>> As this may be misleading let's refactor this function >>> and avoid adding and removing ep0 to eplist and place it >>> immediately in correct place. >> >>> >>> Signed-off-by: Krzysztof Opasiak >>> --- >>> drivers/usb/usbip/vudc_dev.c | 38 +- >>> 1 file changed, 21 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c >>> index 7091848..a5ca29b 100644 >>> --- a/drivers/usb/usbip/vudc_dev.c >>> +++ b/drivers/usb/usbip/vudc_dev.c >>> @@ -549,30 +549,37 @@ static int init_vudc_hw(struct vudc *udc) >>> sprintf(ep->name, "ep%d%s", num, >>> i ? (is_out ? "out" : "in") : ""); >>> ep->ep.name = ep->name; >>> + >>> + ep->ep.ops = &vep_ops; >>> + >>> + ep->halted = ep->wedged = ep->already_seen = >>> + ep->setup_stage = 0; >> >> Do you need to clear these explicitly. kcalloc() should do it for you. > > Well, that's true. I may remove this if you like. Please do. It is redundant. > >> >>> + usb_ep_set_maxpacket_limit(&ep->ep, ~0); >>> + ep->ep.max_streams = 16; >>> + ep->gadget = &udc->gadget; >>> + ep->desc = NULL; > > Probably the same here. > >>> + INIT_LIST_HEAD(&ep->req_queue); >>> + >>> if (i == 0) { >>> + /* ep0 */ >>> ep->ep.caps.type_control = true; >>> ep->ep.caps.dir_out = true; >>> ep->ep.caps.dir_in = true; >>> + >>> + udc->gadget.ep0 = &ep->ep; >>> } else { >>> + /* All other eps */ >>> ep->ep.caps.type_iso = true; >>> ep->ep.caps.type_int = true; >>> ep->ep.caps.type_bulk = true; >>> - } >>> >>> - if (is_out) >>> - ep->ep.caps.dir_out = true; >>> - else >>> - ep->ep.caps.dir_in = true; >>> + if (is_out) >>> + ep->ep.caps.dir_out = true; >>> + else >>> + ep->ep.caps.dir_in = true; >> >> You are moving the is_out handling which was common for all eps >> including ep0 under non-eop0 - is that right? > > Yes it's right. take a look at ep0 inside if(). We set there both > directions to true so setting one of them once again to true doesn't > make any sense. > Yeah. I missed that. thanks, -- Shuah >> >>> >>> - ep->ep.ops = &vep_ops; >>> - list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list); >>> - ep->halted = ep->wedged = ep->already_seen = >>> - ep->setup_stage = 0; >>> - usb_ep_set_maxpacket_limit(&ep->ep, ~0); >>> - ep->ep.max_streams = 16; >>> - ep->gadget = &udc->gadget; >>> - ep->desc = NULL; >>> - INIT_LIST_HEAD(&ep->req_queue); >>> + list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list); >>> + } >>> } >>> >>> spin_lock_init(&udc->lock); >>> @@ -589,9 +596,6 @@ static int init_vudc_hw(struct vudc *udc) >>> ud->eh_ops.reset= vudc_device_reset; >>> ud->eh_ops.unusable = vudc_device_unusable; > > Best regards, > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usbip: vudc: Refactor init_vudc_hw() to be more obvious
On 12/02/2016 04:15 PM, Shuah Khan wrote: > Hi Krzysztof, > > Thanks for the patch. > > On 12/01/2016 10:02 AM, Krzysztof Opasiak wrote: >> Current implementation of init_vudc_hw() adds ep0 to ep_list >> and then after looping through all endpoints removes it from >> that list. >> >> As this may be misleading let's refactor this function >> and avoid adding and removing ep0 to eplist and place it >> immediately in correct place. > >> >> Signed-off-by: Krzysztof Opasiak >> --- >> drivers/usb/usbip/vudc_dev.c | 38 +- >> 1 file changed, 21 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c >> index 7091848..a5ca29b 100644 >> --- a/drivers/usb/usbip/vudc_dev.c >> +++ b/drivers/usb/usbip/vudc_dev.c >> @@ -549,30 +549,37 @@ static int init_vudc_hw(struct vudc *udc) >> sprintf(ep->name, "ep%d%s", num, >> i ? (is_out ? "out" : "in") : ""); >> ep->ep.name = ep->name; >> + >> +ep->ep.ops = &vep_ops; >> + >> +ep->halted = ep->wedged = ep->already_seen = >> +ep->setup_stage = 0; > > Do you need to clear these explicitly. kcalloc() should do it for you. Well, that's true. I may remove this if you like. > >> +usb_ep_set_maxpacket_limit(&ep->ep, ~0); >> +ep->ep.max_streams = 16; >> +ep->gadget = &udc->gadget; >> +ep->desc = NULL; Probably the same here. >> +INIT_LIST_HEAD(&ep->req_queue); >> + >> if (i == 0) { >> +/* ep0 */ >> ep->ep.caps.type_control = true; >> ep->ep.caps.dir_out = true; >> ep->ep.caps.dir_in = true; >> + >> +udc->gadget.ep0 = &ep->ep; >> } else { >> +/* All other eps */ >> ep->ep.caps.type_iso = true; >> ep->ep.caps.type_int = true; >> ep->ep.caps.type_bulk = true; >> -} >> >> -if (is_out) >> -ep->ep.caps.dir_out = true; >> -else >> -ep->ep.caps.dir_in = true; >> +if (is_out) >> +ep->ep.caps.dir_out = true; >> +else >> +ep->ep.caps.dir_in = true; > > You are moving the is_out handling which was common for all eps > including ep0 under non-eop0 - is that right? Yes it's right. take a look at ep0 inside if(). We set there both directions to true so setting one of them once again to true doesn't make any sense. > >> >> -ep->ep.ops = &vep_ops; >> -list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list); >> -ep->halted = ep->wedged = ep->already_seen = >> -ep->setup_stage = 0; >> -usb_ep_set_maxpacket_limit(&ep->ep, ~0); >> -ep->ep.max_streams = 16; >> -ep->gadget = &udc->gadget; >> -ep->desc = NULL; >> -INIT_LIST_HEAD(&ep->req_queue); >> +list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list); >> +} >> } >> >> spin_lock_init(&udc->lock); >> @@ -589,9 +596,6 @@ static int init_vudc_hw(struct vudc *udc) >> ud->eh_ops.reset= vudc_device_reset; >> ud->eh_ops.unusable = vudc_device_unusable; Best regards, -- Krzysztof Opasiak Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usbip: vudc: Refactor init_vudc_hw() to be more obvious
Current implementation of init_vudc_hw() adds ep0 to ep_list and then after looping through all endpoints removes it from that list. As this may be misleading let's refactor this function and avoid adding and removing ep0 to eplist and place it immediately in correct place. Signed-off-by: Krzysztof Opasiak --- drivers/usb/usbip/vudc_dev.c | 38 +- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c index 7091848..a5ca29b 100644 --- a/drivers/usb/usbip/vudc_dev.c +++ b/drivers/usb/usbip/vudc_dev.c @@ -549,30 +549,37 @@ static int init_vudc_hw(struct vudc *udc) sprintf(ep->name, "ep%d%s", num, i ? (is_out ? "out" : "in") : ""); ep->ep.name = ep->name; + + ep->ep.ops = &vep_ops; + + ep->halted = ep->wedged = ep->already_seen = + ep->setup_stage = 0; + usb_ep_set_maxpacket_limit(&ep->ep, ~0); + ep->ep.max_streams = 16; + ep->gadget = &udc->gadget; + ep->desc = NULL; + INIT_LIST_HEAD(&ep->req_queue); + if (i == 0) { + /* ep0 */ ep->ep.caps.type_control = true; ep->ep.caps.dir_out = true; ep->ep.caps.dir_in = true; + + udc->gadget.ep0 = &ep->ep; } else { + /* All other eps */ ep->ep.caps.type_iso = true; ep->ep.caps.type_int = true; ep->ep.caps.type_bulk = true; - } - if (is_out) - ep->ep.caps.dir_out = true; - else - ep->ep.caps.dir_in = true; + if (is_out) + ep->ep.caps.dir_out = true; + else + ep->ep.caps.dir_in = true; - ep->ep.ops = &vep_ops; - list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list); - ep->halted = ep->wedged = ep->already_seen = - ep->setup_stage = 0; - usb_ep_set_maxpacket_limit(&ep->ep, ~0); - ep->ep.max_streams = 16; - ep->gadget = &udc->gadget; - ep->desc = NULL; - INIT_LIST_HEAD(&ep->req_queue); + list_add_tail(&ep->ep.ep_list, &udc->gadget.ep_list); + } } spin_lock_init(&udc->lock); @@ -589,9 +596,6 @@ static int init_vudc_hw(struct vudc *udc) ud->eh_ops.reset= vudc_device_reset; ud->eh_ops.unusable = vudc_device_unusable; - udc->gadget.ep0 = &udc->ep[0].ep; - list_del_init(&udc->ep[0].ep.ep_list); - v_init_timer(udc); return 0; -- 2.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html