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 <k.opasiak@xxxxxxxxxxx>
>>> ---
>>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
