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 <[email protected]>
>> ---
>> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
