Re: [PATCH] usbip: vudc: Refactor init_vudc_hw() to be more obvious

[Krzysztof Opasiak]

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 = _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 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

[Shuah Khan]

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 = _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, ~0);
> + ep->ep.max_streams = 16;
> + ep->gadget = >gadget;
> + ep->desc = NULL;
> + INIT_LIST_HEAD(>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;
>   } 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 = _ops;
> - list_add_tail(>ep.ep_list, >gadget.ep_list);
> - ep->halted = ep->wedged = ep->already_seen =
> - ep->setup_stage = 0;
> - usb_ep_set_maxpacket_limit(>ep, ~0);
> - ep->ep.max_streams = 16;
> - ep->gadget = >gadget;
> - ep->desc = NULL;
> - INIT_LIST_HEAD(>req_queue);
> + list_add_tail(>ep.ep_list, >gadget.ep_list);
> + }
>   }
>  
>   spin_lock_init(>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 = >ep[0].ep;
> - list_del_init(>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

[Shuah Khan]

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 = _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, ~0);
>>> +   ep->ep.max_streams = 16;
>>> +   ep->gadget = >gadget;
>>> +   ep->desc = NULL;
> 
> Probably the same here.
> 
>>> +   INIT_LIST_HEAD(>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;
>>> } 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 = _ops;
>>> -   list_add_tail(>ep.ep_list, >gadget.ep_list);
>>> -   ep->halted = ep->wedged = ep->already_seen =
>>> -   ep->setup_stage = 0;
>>> -   usb_ep_set_maxpacket_limit(>ep, ~0);
>>> -   ep->ep.max_streams = 16;
>>> -   ep->gadget = >gadget;
>>> -   ep->desc = NULL;
>>> -   INIT_LIST_HEAD(>req_queue);
>>> +   list_add_tail(>ep.ep_list, >gadget.ep_list);
>>> +   }
>>> }
>>>  
>>> spin_lock_init(>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

[Krzysztof Opasiak]

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 = _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, ~0);
>> +ep->ep.max_streams = 16;
>> +ep->gadget = >gadget;
>> +ep->desc = NULL;

Probably the same here.

>> +INIT_LIST_HEAD(>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;
>>  } 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 = _ops;
>> -list_add_tail(>ep.ep_list, >gadget.ep_list);
>> -ep->halted = ep->wedged = ep->already_seen =
>> -ep->setup_stage = 0;
>> -usb_ep_set_maxpacket_limit(>ep, ~0);
>> -ep->ep.max_streams = 16;
>> -ep->gadget = >gadget;
>> -ep->desc = NULL;
>> -INIT_LIST_HEAD(>req_queue);
>> +list_add_tail(>ep.ep_list, >gadget.ep_list);
>> +}
>>  }
>>  
>>  spin_lock_init(>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 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

[Krzysztof Opasiak]

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 = _ops;
+
+   ep->halted = ep->wedged = ep->already_seen =
+   ep->setup_stage = 0;
+   usb_ep_set_maxpacket_limit(>ep, ~0);
+   ep->ep.max_streams = 16;
+   ep->gadget = >gadget;
+   ep->desc = NULL;
+   INIT_LIST_HEAD(>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;
} 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 = _ops;
-   list_add_tail(>ep.ep_list, >gadget.ep_list);
-   ep->halted = ep->wedged = ep->already_seen =
-   ep->setup_stage = 0;
-   usb_ep_set_maxpacket_limit(>ep, ~0);
-   ep->ep.max_streams = 16;
-   ep->gadget = >gadget;
-   ep->desc = NULL;
-   INIT_LIST_HEAD(>req_queue);
+   list_add_tail(>ep.ep_list, >gadget.ep_list);
+   }
}
 
spin_lock_init(>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 = >ep[0].ep;
-   list_del_init(>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