Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

On 29/08/16 08:55, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>>> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>>>>> "Felipe F. Tonello" <e...@felipetonello.com> writes:
>>>>>> The default_length parameter of alloc_ep_req was not really necessary
>>>>>> and gadget drivers would almost always create an inline function to pass
>>>>>> the same value to len and default_len.
>>>>>>
>>>>>> So this patch also removes duplicate code from few drivers.
>>>>>>
>>>>>> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
>>>>>> ---
>>>>>>  drivers/usb/gadget/function/f_hid.c| 10 ++
>>>>>>  drivers/usb/gadget/function/f_loopback.c   |  9 +
>>>>>>  drivers/usb/gadget/function/f_midi.c   | 10 ++
>>>>>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
>>>>>>  drivers/usb/gadget/u_f.c   |  7 +++
>>>>>>  drivers/usb/gadget/u_f.h   |  3 +--
>>>>>>  6 files changed, 11 insertions(+), 39 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>>>>>> b/drivers/usb/gadget/function/f_hid.c
>>>>>> index 51980c50546d..e82a7468252e 100644
>>>>>> --- a/drivers/usb/gadget/function/f_hid.c
>>>>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>>>>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct 
>>>>>> file *fd)
>>>>>>  
>>>>>> /*-*/
>>>>>>  /*usb_function  
>>>>>>*/
>>>>>>  
>>>>>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>>>>>> -unsigned length)
>>>>>> -{
>>>>>> -return alloc_ep_req(ep, length, length);
>>>>>> -}
>>>>>
>>>>> actually, I prefer to keep these little helpers. I was recently playing
>>>>> with adding SG list support to g_zero (I should have patches soon) and
>>>>> it was actually very nice to have the sourcesink helper as I could just
>>>>> ditch alloc_ep_req(). The change to the driver was local to
>>>>> ss_alloc_ep_req() and nothing else changed :-)
>>>>>
>>>>
>>>> Right, but then it is worth to have the helper function. In this
>>>> particular case, I am removing a useless helper functions, especially
>>>> that the previous patch removes the need for the optional parameter in
>>>> alloc_ep_req.
>>>
>>> it's a static inline :-) It won't do any bad to keep it. And, as I said,
>>> if we want to ditch aloc_ep_req() eventually, then we have just one
>>> place to patch ;-)
>>
>> Yes, sure. But why drop alloc_ep_req()?
> 
> because we can't find a generic way of allocating sglist with enough
> entries :-) Some drivers (like f_fs.c) could actually have zero-copy
> sglist by just pinning user pages with get_user_pages_fast() and
> following with sg_alloc_from_pages().
> 
> g_zero, however, would just "emulate" sglist by just allocating a
> statically sized sg table and initializing chunks of the linear req->buf
> to each sg entry.

I see. :)

> 
>> So should I keep all these helper functions? If so, I actually still
>> need to fix them to use the newer alloc_ep_req() API.
> 
> yeah, keeping the helper functions would be nice. IMHO, alloc_ep_req()
> doesn't have a long life, but it's pretty good for the time being.

Ok, I did it on my last patchset I sent, I think you already applied to
your tree.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

On 29/08/16 08:55, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>>> Felipe Ferreri Tonello  writes:
>>>>> "Felipe F. Tonello"  writes:
>>>>>> The default_length parameter of alloc_ep_req was not really necessary
>>>>>> and gadget drivers would almost always create an inline function to pass
>>>>>> the same value to len and default_len.
>>>>>>
>>>>>> So this patch also removes duplicate code from few drivers.
>>>>>>
>>>>>> Signed-off-by: Felipe F. Tonello 
>>>>>> ---
>>>>>>  drivers/usb/gadget/function/f_hid.c| 10 ++
>>>>>>  drivers/usb/gadget/function/f_loopback.c   |  9 +
>>>>>>  drivers/usb/gadget/function/f_midi.c   | 10 ++
>>>>>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
>>>>>>  drivers/usb/gadget/u_f.c   |  7 +++
>>>>>>  drivers/usb/gadget/u_f.h   |  3 +--
>>>>>>  6 files changed, 11 insertions(+), 39 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>>>>>> b/drivers/usb/gadget/function/f_hid.c
>>>>>> index 51980c50546d..e82a7468252e 100644
>>>>>> --- a/drivers/usb/gadget/function/f_hid.c
>>>>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>>>>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct 
>>>>>> file *fd)
>>>>>>  
>>>>>> /*-*/
>>>>>>  /*usb_function  
>>>>>>*/
>>>>>>  
>>>>>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>>>>>> -unsigned length)
>>>>>> -{
>>>>>> -return alloc_ep_req(ep, length, length);
>>>>>> -}
>>>>>
>>>>> actually, I prefer to keep these little helpers. I was recently playing
>>>>> with adding SG list support to g_zero (I should have patches soon) and
>>>>> it was actually very nice to have the sourcesink helper as I could just
>>>>> ditch alloc_ep_req(). The change to the driver was local to
>>>>> ss_alloc_ep_req() and nothing else changed :-)
>>>>>
>>>>
>>>> Right, but then it is worth to have the helper function. In this
>>>> particular case, I am removing a useless helper functions, especially
>>>> that the previous patch removes the need for the optional parameter in
>>>> alloc_ep_req.
>>>
>>> it's a static inline :-) It won't do any bad to keep it. And, as I said,
>>> if we want to ditch aloc_ep_req() eventually, then we have just one
>>> place to patch ;-)
>>
>> Yes, sure. But why drop alloc_ep_req()?
> 
> because we can't find a generic way of allocating sglist with enough
> entries :-) Some drivers (like f_fs.c) could actually have zero-copy
> sglist by just pinning user pages with get_user_pages_fast() and
> following with sg_alloc_from_pages().
> 
> g_zero, however, would just "emulate" sglist by just allocating a
> statically sized sg table and initializing chunks of the linear req->buf
> to each sg entry.

I see. :)

> 
>> So should I keep all these helper functions? If so, I actually still
>> need to fix them to use the newer alloc_ep_req() API.
> 
> yeah, keeping the helper functions would be nice. IMHO, alloc_ep_req()
> doesn't have a long life, but it's pretty good for the time being.

Ok, I did it on my last patchset I sent, I think you already applied to
your tree.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v4 10/10] usb: gadget: f_hid: use alloc_ep_req()

[Felipe Ferreri Tonello]

Hi Balbi,

On 23/08/16 12:03, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>>> John Youn <john.y...@synopsys.com> writes:
>>>> On 8/8/2016 1:30 PM, Felipe F. Tonello wrote:
>>>>> Use gadget's framework allocation function instead of directly calling
>>>>> usb_ep_alloc_request().
>>>>>
>>>>> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
>>>>> ---
>>>>>  drivers/usb/gadget/function/f_hid.c | 6 +-
>>>>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>>>>> b/drivers/usb/gadget/function/f_hid.c
>>>>> index a010496e4e05..89d2e9a5a04f 100644
>>>>> --- a/drivers/usb/gadget/function/f_hid.c
>>>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>>>> @@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, 
>>>>> struct usb_function *f)
>>>>>  
>>>>>   /* preallocate request and buffer */
>>>>>   status = -ENOMEM;
>>>>> - hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
>>>>> + hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>>>>   if (!hidg->req)
>>>>>   goto fail;
>>>>>  
>>>>> - hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
>>>>> - if (!hidg->req->buf)
>>>>> - goto fail;
>>>>> -
>>>>>   /* set descriptor dynamic values */
>>>>>   hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
>>>>>   hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
>>>>>
>>>>
>>>> Hi Felipe,
>>>>
>>>> This commit on your testing/next breaks compilation.
>>>>
>>>> ../drivers/usb/gadget/function/f_hid.c: In function ‘hidg_bind’:
>>>> ../drivers/usb/gadget/function/f_hid.c:620:14: error: too few arguments to 
>>>> function ‘alloc_ep_req’
>>>>   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>>>   ^
>>>> In file included from ../drivers/usb/gadget/function/f_hid.c:24:0:
>>>> ../drivers/usb/gadget/u_f.h:63:21: note: declared here
>>>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int 
>>>> default_len);
>>>
>>> true that :-) Dropping from my queue.
>>>
>>
>> Are you applying the previous patches? Specially that this is the last
>> patch in the series, how can it break with you if it doesn't break here?
>> What should I do then?
> 
> Can you rebase your series on top of my testing/next? My HEAD is
> at commit 95bbb3474f1e87c9ec7ebe2acf25006e5e94a824.

It was at that time. I will rebease it then on v5. Just waiting for your
comments on the other thread.

Thanks

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

Hi Balbi,

On 23/08/16 12:01, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>>> "Felipe F. Tonello" <e...@felipetonello.com> writes:
>>>> The default_length parameter of alloc_ep_req was not really necessary
>>>> and gadget drivers would almost always create an inline function to pass
>>>> the same value to len and default_len.
>>>>
>>>> So this patch also removes duplicate code from few drivers.
>>>>
>>>> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
>>>> ---
>>>>  drivers/usb/gadget/function/f_hid.c| 10 ++
>>>>  drivers/usb/gadget/function/f_loopback.c   |  9 +
>>>>  drivers/usb/gadget/function/f_midi.c   | 10 ++
>>>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
>>>>  drivers/usb/gadget/u_f.c   |  7 +++
>>>>  drivers/usb/gadget/u_f.h   |  3 +--
>>>>  6 files changed, 11 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>>>> b/drivers/usb/gadget/function/f_hid.c
>>>> index 51980c50546d..e82a7468252e 100644
>>>> --- a/drivers/usb/gadget/function/f_hid.c
>>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct 
>>>> file *fd)
>>>>  
>>>> /*-*/
>>>>  /*usb_function
>>>>  */
>>>>  
>>>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>>>> -  unsigned length)
>>>> -{
>>>> -  return alloc_ep_req(ep, length, length);
>>>> -}
>>>
>>> actually, I prefer to keep these little helpers. I was recently playing
>>> with adding SG list support to g_zero (I should have patches soon) and
>>> it was actually very nice to have the sourcesink helper as I could just
>>> ditch alloc_ep_req(). The change to the driver was local to
>>> ss_alloc_ep_req() and nothing else changed :-)
>>>
>>
>> Right, but then it is worth to have the helper function. In this
>> particular case, I am removing a useless helper functions, especially
>> that the previous patch removes the need for the optional parameter in
>> alloc_ep_req.
> 
> it's a static inline :-) It won't do any bad to keep it. And, as I said,
> if we want to ditch aloc_ep_req() eventually, then we have just one
> place to patch ;-)

Yes, sure. But why drop alloc_ep_req()?

So should I keep all these helper functions? If so, I actually still
need to fix them to use the newer alloc_ep_req() API.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v4 10/10] usb: gadget: f_hid: use alloc_ep_req()

[Felipe Ferreri Tonello]

Hi Balbi,

On 23/08/16 12:03, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>>> John Youn  writes:
>>>> On 8/8/2016 1:30 PM, Felipe F. Tonello wrote:
>>>>> Use gadget's framework allocation function instead of directly calling
>>>>> usb_ep_alloc_request().
>>>>>
>>>>> Signed-off-by: Felipe F. Tonello 
>>>>> ---
>>>>>  drivers/usb/gadget/function/f_hid.c | 6 +-
>>>>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>>>>> b/drivers/usb/gadget/function/f_hid.c
>>>>> index a010496e4e05..89d2e9a5a04f 100644
>>>>> --- a/drivers/usb/gadget/function/f_hid.c
>>>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>>>> @@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, 
>>>>> struct usb_function *f)
>>>>>  
>>>>>   /* preallocate request and buffer */
>>>>>   status = -ENOMEM;
>>>>> - hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
>>>>> + hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>>>>   if (!hidg->req)
>>>>>   goto fail;
>>>>>  
>>>>> - hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
>>>>> - if (!hidg->req->buf)
>>>>> - goto fail;
>>>>> -
>>>>>   /* set descriptor dynamic values */
>>>>>   hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
>>>>>   hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
>>>>>
>>>>
>>>> Hi Felipe,
>>>>
>>>> This commit on your testing/next breaks compilation.
>>>>
>>>> ../drivers/usb/gadget/function/f_hid.c: In function ‘hidg_bind’:
>>>> ../drivers/usb/gadget/function/f_hid.c:620:14: error: too few arguments to 
>>>> function ‘alloc_ep_req’
>>>>   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>>>   ^
>>>> In file included from ../drivers/usb/gadget/function/f_hid.c:24:0:
>>>> ../drivers/usb/gadget/u_f.h:63:21: note: declared here
>>>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int 
>>>> default_len);
>>>
>>> true that :-) Dropping from my queue.
>>>
>>
>> Are you applying the previous patches? Specially that this is the last
>> patch in the series, how can it break with you if it doesn't break here?
>> What should I do then?
> 
> Can you rebase your series on top of my testing/next? My HEAD is
> at commit 95bbb3474f1e87c9ec7ebe2acf25006e5e94a824.

It was at that time. I will rebease it then on v5. Just waiting for your
comments on the other thread.

Thanks

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

Hi Balbi,

On 23/08/16 12:01, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>>> "Felipe F. Tonello"  writes:
>>>> The default_length parameter of alloc_ep_req was not really necessary
>>>> and gadget drivers would almost always create an inline function to pass
>>>> the same value to len and default_len.
>>>>
>>>> So this patch also removes duplicate code from few drivers.
>>>>
>>>> Signed-off-by: Felipe F. Tonello 
>>>> ---
>>>>  drivers/usb/gadget/function/f_hid.c| 10 ++
>>>>  drivers/usb/gadget/function/f_loopback.c   |  9 +
>>>>  drivers/usb/gadget/function/f_midi.c   | 10 ++
>>>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
>>>>  drivers/usb/gadget/u_f.c   |  7 +++
>>>>  drivers/usb/gadget/u_f.h   |  3 +--
>>>>  6 files changed, 11 insertions(+), 39 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>>>> b/drivers/usb/gadget/function/f_hid.c
>>>> index 51980c50546d..e82a7468252e 100644
>>>> --- a/drivers/usb/gadget/function/f_hid.c
>>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct 
>>>> file *fd)
>>>>  
>>>> /*-*/
>>>>  /*usb_function
>>>>  */
>>>>  
>>>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>>>> -  unsigned length)
>>>> -{
>>>> -  return alloc_ep_req(ep, length, length);
>>>> -}
>>>
>>> actually, I prefer to keep these little helpers. I was recently playing
>>> with adding SG list support to g_zero (I should have patches soon) and
>>> it was actually very nice to have the sourcesink helper as I could just
>>> ditch alloc_ep_req(). The change to the driver was local to
>>> ss_alloc_ep_req() and nothing else changed :-)
>>>
>>
>> Right, but then it is worth to have the helper function. In this
>> particular case, I am removing a useless helper functions, especially
>> that the previous patch removes the need for the optional parameter in
>> alloc_ep_req.
> 
> it's a static inline :-) It won't do any bad to keep it. And, as I said,
> if we want to ditch aloc_ep_req() eventually, then we have just one
> place to patch ;-)

Yes, sure. But why drop alloc_ep_req()?

So should I keep all these helper functions? If so, I actually still
need to fix them to use the newer alloc_ep_req() API.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v4 10/10] usb: gadget: f_hid: use alloc_ep_req()

[Felipe Ferreri Tonello]

Hi,

On 22/08/16 08:45, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
>> On 8/8/2016 1:30 PM, Felipe F. Tonello wrote:
>>> Use gadget's framework allocation function instead of directly calling
>>> usb_ep_alloc_request().
>>>
>>> Signed-off-by: Felipe F. Tonello 
>>> ---
>>>  drivers/usb/gadget/function/f_hid.c | 6 +-
>>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>>> b/drivers/usb/gadget/function/f_hid.c
>>> index a010496e4e05..89d2e9a5a04f 100644
>>> --- a/drivers/usb/gadget/function/f_hid.c
>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>> @@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, 
>>> struct usb_function *f)
>>>  
>>> /* preallocate request and buffer */
>>> status = -ENOMEM;
>>> -   hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
>>> +   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>> if (!hidg->req)
>>> goto fail;
>>>  
>>> -   hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
>>> -   if (!hidg->req->buf)
>>> -   goto fail;
>>> -
>>> /* set descriptor dynamic values */
>>> hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
>>> hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
>>>
>>
>> Hi Felipe,
>>
>> This commit on your testing/next breaks compilation.
>>
>> ../drivers/usb/gadget/function/f_hid.c: In function ‘hidg_bind’:
>> ../drivers/usb/gadget/function/f_hid.c:620:14: error: too few arguments to 
>> function ‘alloc_ep_req’
>>   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>   ^
>> In file included from ../drivers/usb/gadget/function/f_hid.c:24:0:
>> ../drivers/usb/gadget/u_f.h:63:21: note: declared here
>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int 
>> default_len);
> 
> true that :-) Dropping from my queue.
> 

Are you applying the previous patches? Specially that this is the last
patch in the series, how can it break with you if it doesn't break here?
What should I do then?

Thanks

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v4 10/10] usb: gadget: f_hid: use alloc_ep_req()

[Felipe Ferreri Tonello]

Hi,

On 22/08/16 08:45, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
>> On 8/8/2016 1:30 PM, Felipe F. Tonello wrote:
>>> Use gadget's framework allocation function instead of directly calling
>>> usb_ep_alloc_request().
>>>
>>> Signed-off-by: Felipe F. Tonello 
>>> ---
>>>  drivers/usb/gadget/function/f_hid.c | 6 +-
>>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>>> b/drivers/usb/gadget/function/f_hid.c
>>> index a010496e4e05..89d2e9a5a04f 100644
>>> --- a/drivers/usb/gadget/function/f_hid.c
>>> +++ b/drivers/usb/gadget/function/f_hid.c
>>> @@ -611,14 +611,10 @@ static int hidg_bind(struct usb_configuration *c, 
>>> struct usb_function *f)
>>>  
>>> /* preallocate request and buffer */
>>> status = -ENOMEM;
>>> -   hidg->req = usb_ep_alloc_request(hidg->in_ep, GFP_KERNEL);
>>> +   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>> if (!hidg->req)
>>> goto fail;
>>>  
>>> -   hidg->req->buf = kmalloc(hidg->report_length, GFP_KERNEL);
>>> -   if (!hidg->req->buf)
>>> -   goto fail;
>>> -
>>> /* set descriptor dynamic values */
>>> hidg_interface_desc.bInterfaceSubClass = hidg->bInterfaceSubClass;
>>> hidg_interface_desc.bInterfaceProtocol = hidg->bInterfaceProtocol;
>>>
>>
>> Hi Felipe,
>>
>> This commit on your testing/next breaks compilation.
>>
>> ../drivers/usb/gadget/function/f_hid.c: In function ‘hidg_bind’:
>> ../drivers/usb/gadget/function/f_hid.c:620:14: error: too few arguments to 
>> function ‘alloc_ep_req’
>>   hidg->req = alloc_ep_req(hidg->in_ep, hidg->report_length);
>>   ^
>> In file included from ../drivers/usb/gadget/function/f_hid.c:24:0:
>> ../drivers/usb/gadget/u_f.h:63:21: note: declared here
>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int 
>> default_len);
> 
> true that :-) Dropping from my queue.
> 

Are you applying the previous patches? Specially that this is the last
patch in the series, how can it break with you if it doesn't break here?
What should I do then?

Thanks

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

Hi Blabi,

On 18/08/16 08:12, Felipe Balbi wrote:
> 
> Hi,
> 
> "Felipe F. Tonello"  writes:
>> The default_length parameter of alloc_ep_req was not really necessary
>> and gadget drivers would almost always create an inline function to pass
>> the same value to len and default_len.
>>
>> So this patch also removes duplicate code from few drivers.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/function/f_hid.c| 10 ++
>>  drivers/usb/gadget/function/f_loopback.c   |  9 +
>>  drivers/usb/gadget/function/f_midi.c   | 10 ++
>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
>>  drivers/usb/gadget/u_f.c   |  7 +++
>>  drivers/usb/gadget/u_f.h   |  3 +--
>>  6 files changed, 11 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>> b/drivers/usb/gadget/function/f_hid.c
>> index 51980c50546d..e82a7468252e 100644
>> --- a/drivers/usb/gadget/function/f_hid.c
>> +++ b/drivers/usb/gadget/function/f_hid.c
>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file 
>> *fd)
>>  
>> /*-*/
>>  /*usb_function 
>> */
>>  
>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>> -unsigned length)
>> -{
>> -return alloc_ep_req(ep, length, length);
>> -}
> 
> actually, I prefer to keep these little helpers. I was recently playing
> with adding SG list support to g_zero (I should have patches soon) and
> it was actually very nice to have the sourcesink helper as I could just
> ditch alloc_ep_req(). The change to the driver was local to
> ss_alloc_ep_req() and nothing else changed :-)
> 

Right, but then it is worth to have the helper function. In this
particular case, I am removing a useless helper functions, especially
that the previous patch removes the need for the optional parameter in
alloc_ep_req.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

Hi Blabi,

On 18/08/16 08:12, Felipe Balbi wrote:
> 
> Hi,
> 
> "Felipe F. Tonello"  writes:
>> The default_length parameter of alloc_ep_req was not really necessary
>> and gadget drivers would almost always create an inline function to pass
>> the same value to len and default_len.
>>
>> So this patch also removes duplicate code from few drivers.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/function/f_hid.c| 10 ++
>>  drivers/usb/gadget/function/f_loopback.c   |  9 +
>>  drivers/usb/gadget/function/f_midi.c   | 10 ++
>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
>>  drivers/usb/gadget/u_f.c   |  7 +++
>>  drivers/usb/gadget/u_f.h   |  3 +--
>>  6 files changed, 11 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>> b/drivers/usb/gadget/function/f_hid.c
>> index 51980c50546d..e82a7468252e 100644
>> --- a/drivers/usb/gadget/function/f_hid.c
>> +++ b/drivers/usb/gadget/function/f_hid.c
>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file 
>> *fd)
>>  
>> /*-*/
>>  /*usb_function 
>> */
>>  
>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>> -unsigned length)
>> -{
>> -return alloc_ep_req(ep, length, length);
>> -}
> 
> actually, I prefer to keep these little helpers. I was recently playing
> with adding SG list support to g_zero (I should have patches soon) and
> it was actually very nice to have the sourcesink helper as I could just
> ditch alloc_ep_req(). The change to the driver was local to
> ss_alloc_ep_req() and nothing else changed :-)
> 

Right, but then it is worth to have the helper function. In this
particular case, I am removing a useless helper functions, especially
that the previous patch removes the need for the optional parameter in
alloc_ep_req.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/4] usb: gadget: f_midi: fixed endianness when using wMaxPacketSize

[Felipe Ferreri Tonello]

Hi Balbi,

On 10/08/16 12:24, Felipe Balbi wrote:
> 
> Hi,
> 
> Baolin Wang  writes:
>> On 26 July 2016 at 07:15, Felipe F. Tonello  wrote:
>>> USB spec specifies wMaxPacketSize to be little endian (as other properties),
>>> so when using this variable in the driver we should convert to the current
>>> CPU endianness if necessary.
>>>
>>> Signed-off-by: Felipe F. Tonello 
>>> ---
>>>  drivers/usb/gadget/function/f_midi.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>>> b/drivers/usb/gadget/function/f_midi.c
>>> index 58fc199a18ec..a83d852b1da5 100644
>>> --- a/drivers/usb/gadget/function/f_midi.c
>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>> @@ -362,7 +362,7 @@ static int f_midi_set_alt(struct usb_function *f, 
>>> unsigned intf, unsigned alt)
>>> struct usb_request *req =
>>> midi_alloc_ep_req(midi->out_ep,
>>> max_t(unsigned, midi->buflen,
>>> -   bulk_out_desc.wMaxPacketSize));
>>> +   
>>> le16_to_cpu(bulk_out_desc.wMaxPacketSize)));
>>
>> I think here we should use usb_ep_align_maybe() function instead of
>> max_t() to handle 'quirk_ep_out_aligned_size' quirk, please see the
>> patch I've send out: https://lkml.org/lkml/2016/7/12/106
> 
> agree, if usb_ep_align_maybe() has a bug with endianness, let's fix it
> since there are other gadgets using usb_ep_align_maybe()
> 

Can you take a look at v4 of this patchset, please? It's the latest
stuff I have sent.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys


signature.asc
Description: OpenPGP digital signature

Re: [PATCH 1/4] usb: gadget: f_midi: fixed endianness when using wMaxPacketSize

[Felipe Ferreri Tonello]

Hi Balbi,

On 10/08/16 12:24, Felipe Balbi wrote:
> 
> Hi,
> 
> Baolin Wang  writes:
>> On 26 July 2016 at 07:15, Felipe F. Tonello  wrote:
>>> USB spec specifies wMaxPacketSize to be little endian (as other properties),
>>> so when using this variable in the driver we should convert to the current
>>> CPU endianness if necessary.
>>>
>>> Signed-off-by: Felipe F. Tonello 
>>> ---
>>>  drivers/usb/gadget/function/f_midi.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>>> b/drivers/usb/gadget/function/f_midi.c
>>> index 58fc199a18ec..a83d852b1da5 100644
>>> --- a/drivers/usb/gadget/function/f_midi.c
>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>> @@ -362,7 +362,7 @@ static int f_midi_set_alt(struct usb_function *f, 
>>> unsigned intf, unsigned alt)
>>> struct usb_request *req =
>>> midi_alloc_ep_req(midi->out_ep,
>>> max_t(unsigned, midi->buflen,
>>> -   bulk_out_desc.wMaxPacketSize));
>>> +   
>>> le16_to_cpu(bulk_out_desc.wMaxPacketSize)));
>>
>> I think here we should use usb_ep_align_maybe() function instead of
>> max_t() to handle 'quirk_ep_out_aligned_size' quirk, please see the
>> patch I've send out: https://lkml.org/lkml/2016/7/12/106
> 
> agree, if usb_ep_align_maybe() has a bug with endianness, let's fix it
> since there are other gadgets using usb_ep_align_maybe()
> 

Can you take a look at v4 of this patchset, please? It's the latest
stuff I have sent.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys


signature.asc
Description: OpenPGP digital signature

Re: [PATCH 2/4] usb: gadget: f_midi: defaults buflen sizes to 512

[Felipe Ferreri Tonello]

Hi Balbi,

On 10/08/16 12:25, Felipe Balbi wrote:
> 
> Hi,
> 
> "Felipe F. Tonello"  writes:
>> 512 is the value used by wMaxPacketSize, as specified by the USB Spec. This
> 
> this is only true for HS :-) FS and SS use different sizes. Do you want
> to use 1024 (SS maxp) by default instead? Then all speeds will have this
> working out just fine.

That's true, altough I've never seen a FS or SS device with MIDI gadget,
they are all 1.1 or 2.0 max.

Nevertheless, your suggestion really makes sense since it will work in
any situation.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys


signature.asc
Description: OpenPGP digital signature

Re: [PATCH 2/4] usb: gadget: f_midi: defaults buflen sizes to 512

[Felipe Ferreri Tonello]

Hi Balbi,

On 10/08/16 12:25, Felipe Balbi wrote:
> 
> Hi,
> 
> "Felipe F. Tonello"  writes:
>> 512 is the value used by wMaxPacketSize, as specified by the USB Spec. This
> 
> this is only true for HS :-) FS and SS use different sizes. Do you want
> to use 1024 (SS maxp) by default instead? Then all speeds will have this
> working out just fine.

That's true, altough I've never seen a FS or SS device with MIDI gadget,
they are all 1.1 or 2.0 max.

Nevertheless, your suggestion really makes sense since it will work in
any situation.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys


signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 2/9] usb: gadget: align buffer size when allocating for OUT endpoint

[Felipe Ferreri Tonello]

Hi,

On 05-08-2016 20:15, kbuild test robot wrote:
> Hi Felipe,
> 
> [auto build test ERROR on balbi-usb/next]
> [also build test ERROR on v4.7 next-20160805]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Felipe-F-Tonello/Gadget-endpoint-request-allocation-and-MIDI/20160806-024520
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
> config: x86_64-randconfig-x013-201631 (attached as .config)
> compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64 
> 
> Note: the 
> linux-review/Felipe-F-Tonello/Gadget-endpoint-request-allocation-and-MIDI/20160806-024520
>  HEAD 5064a41cd5f89103e3b75c1a2ab9f6e98851503b builds fine.
>   It only hurts bisectibility.
> 
> All errors (new ones prefixed by >>):
> 
>>> drivers/usb/gadget/u_f.c:17:21: error: conflicting types for 'alloc_ep_req'
> struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int 
> default_len)
> ^~~~
>In file included from drivers/usb/gadget/u_f.c:14:0:
>drivers/usb/gadget/u_f.h:63:21: note: previous declaration of 
> 'alloc_ep_req' was here
> struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int 
> default_len);

Ok, I made the mistake to change to size_t the len type just on the
function declaration. I'll fix this on a v4.

> ^~~~
> 
> vim +/alloc_ep_req +17 drivers/usb/gadget/u_f.c
> 
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  11   * published by the Free 
> Software Foundation.
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  12   */
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  13  
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  14  #include "u_f.h"
> a7ffc68f Felipe F. Tonello 2016-08-05  15  #include 
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  16  
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07 @17  struct usb_request 
> *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  18  {
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  19 struct usb_request  
> *req;
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  20  
> 
> :: The code at line 17 was first introduced by commit
> :: 1efd54eab2b60c68c2ce75ea635306cef847d751 usb: gadget: factor out 
> alloc_ep_req
> 
> :: TO: Andrzej Pietrasiewicz 
> :: CC: Felipe Balbi 
> 
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation
> 

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v3 2/9] usb: gadget: align buffer size when allocating for OUT endpoint

[Felipe Ferreri Tonello]

Hi,

On 05-08-2016 20:15, kbuild test robot wrote:
> Hi Felipe,
> 
> [auto build test ERROR on balbi-usb/next]
> [also build test ERROR on v4.7 next-20160805]
> [if your patch is applied to the wrong git tree, please drop us a note to 
> help improve the system]
> 
> url:
> https://github.com/0day-ci/linux/commits/Felipe-F-Tonello/Gadget-endpoint-request-allocation-and-MIDI/20160806-024520
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
> config: x86_64-randconfig-x013-201631 (attached as .config)
> compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
> # save the attached .config to linux build tree
> make ARCH=x86_64 
> 
> Note: the 
> linux-review/Felipe-F-Tonello/Gadget-endpoint-request-allocation-and-MIDI/20160806-024520
>  HEAD 5064a41cd5f89103e3b75c1a2ab9f6e98851503b builds fine.
>   It only hurts bisectibility.
> 
> All errors (new ones prefixed by >>):
> 
>>> drivers/usb/gadget/u_f.c:17:21: error: conflicting types for 'alloc_ep_req'
> struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int 
> default_len)
> ^~~~
>In file included from drivers/usb/gadget/u_f.c:14:0:
>drivers/usb/gadget/u_f.h:63:21: note: previous declaration of 
> 'alloc_ep_req' was here
> struct usb_request *alloc_ep_req(struct usb_ep *ep, size_t len, int 
> default_len);

Ok, I made the mistake to change to size_t the len type just on the
function declaration. I'll fix this on a v4.

> ^~~~
> 
> vim +/alloc_ep_req +17 drivers/usb/gadget/u_f.c
> 
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  11   * published by the Free 
> Software Foundation.
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  12   */
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  13  
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  14  #include "u_f.h"
> a7ffc68f Felipe F. Tonello 2016-08-05  15  #include 
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  16  
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07 @17  struct usb_request 
> *alloc_ep_req(struct usb_ep *ep, int len, int default_len)
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  18  {
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  19 struct usb_request  
> *req;
> 1efd54ea Andrzej Pietrasiewicz 2013-11-07  20  
> 
> :: The code at line 17 was first introduced by commit
> :: 1efd54eab2b60c68c2ce75ea635306cef847d751 usb: gadget: factor out 
> alloc_ep_req
> 
> :: TO: Andrzej Pietrasiewicz 
> :: CC: Felipe Balbi 
> 
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all   Intel Corporation
> 

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 2/9] usb: gadget: align buffer size when allocating for OUT endpoint

[Felipe Ferreri Tonello]

Hi Michal,

On 27/07/16 20:59, Michal Nazarewicz wrote:
> On Tue, Jul 26 2016, Felipe F. Tonello wrote:
>> Using usb_ep_align() makes sure that the buffer size for OUT endpoints is
>> always aligned with wMaxPacketSize (512 usually). This makes sure
>> that no buffer has the wrong size, which can cause nasty bugs.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/u_f.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
>> index 4bc7eea8bfc8..d1933b0b76c3 100644
>> --- a/drivers/usb/gadget/u_f.c
>> +++ b/drivers/usb/gadget/u_f.c
>> @@ -12,6 +12,7 @@
>>   */
>>  
>>  #include "u_f.h"
>> +#include 
>>  
>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int 
>> default_len)
>>  {
>> @@ -20,6 +21,8 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int 
>> len, int default_len)
>>  req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>>  if (req) {
>>  req->length = len ?: default_len;
>> +if (usb_endpoint_dir_out(ep->desc))
>> +req->length = usb_ep_align(ep, req->length);
>>  req->buf = kmalloc(req->length, GFP_ATOMIC);
>>  if (!req->buf) {
>>  usb_ep_free_request(ep, req);
> 
> I’m a bit scared of this change.

I agree, it's scary. :D

> 
> Drivers which call alloc_ep_req and then ignore req->length using the
> same length they passed to the function will silently drop data.
> 
> Drivers which do not ignore req->length may end up overwriting some
> other buffer, e.g.:
> 
>   some_buffer = kmalloc(length, GFP_KERNEL);
> req = alloc_ep_req(ep, length, 0);
> … later …
> memcpy(some_buffer, req->buf, req->length);

True. The same happens if the data associated with an OUT endpoint is
smaller than wMaxPacketSize.

This patch doesn't fix all problems associated with that, but it allows
better practice to take place. It returns to the driver the actual
allocated size, like several POSIX functions.

I haven't seen any problems on all gadgets that rely on alloc_ep_req().
Maybe as we port other gadgets to this use this function instead of
usb_ep_alloc_request() we might find some issues.

Perhaps we should add better documentation to alloc_ep_req()?

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 2/9] usb: gadget: align buffer size when allocating for OUT endpoint

[Felipe Ferreri Tonello]

Hi Michal,

On 27/07/16 20:59, Michal Nazarewicz wrote:
> On Tue, Jul 26 2016, Felipe F. Tonello wrote:
>> Using usb_ep_align() makes sure that the buffer size for OUT endpoints is
>> always aligned with wMaxPacketSize (512 usually). This makes sure
>> that no buffer has the wrong size, which can cause nasty bugs.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/u_f.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
>> index 4bc7eea8bfc8..d1933b0b76c3 100644
>> --- a/drivers/usb/gadget/u_f.c
>> +++ b/drivers/usb/gadget/u_f.c
>> @@ -12,6 +12,7 @@
>>   */
>>  
>>  #include "u_f.h"
>> +#include 
>>  
>>  struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int 
>> default_len)
>>  {
>> @@ -20,6 +21,8 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int 
>> len, int default_len)
>>  req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>>  if (req) {
>>  req->length = len ?: default_len;
>> +if (usb_endpoint_dir_out(ep->desc))
>> +req->length = usb_ep_align(ep, req->length);
>>  req->buf = kmalloc(req->length, GFP_ATOMIC);
>>  if (!req->buf) {
>>  usb_ep_free_request(ep, req);
> 
> I’m a bit scared of this change.

I agree, it's scary. :D

> 
> Drivers which call alloc_ep_req and then ignore req->length using the
> same length they passed to the function will silently drop data.
> 
> Drivers which do not ignore req->length may end up overwriting some
> other buffer, e.g.:
> 
>   some_buffer = kmalloc(length, GFP_KERNEL);
> req = alloc_ep_req(ep, length, 0);
> … later …
> memcpy(some_buffer, req->buf, req->length);

True. The same happens if the data associated with an OUT endpoint is
smaller than wMaxPacketSize.

This patch doesn't fix all problems associated with that, but it allows
better practice to take place. It returns to the driver the actual
allocated size, like several POSIX functions.

I haven't seen any problems on all gadgets that rely on alloc_ep_req().
Maybe as we port other gadgets to this use this function instead of
usb_ep_alloc_request() we might find some issues.

Perhaps we should add better documentation to alloc_ep_req()?

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 7/9] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

Hi Michal,

On 27/07/16 21:02, Michal Nazarewicz wrote:
> On Tue, Jul 26 2016, Felipe F. Tonello wrote:
>> This parameter was not really necessary and gadget drivers would almost 
>> always
> 
> I personally like when commit messages can be read without subject, so
> perhaps:
> 
>   The default_length parameter of alloc_ep_req was not …
> 
> But that’s just me.

Good judgment.

> 
>> create an inline function to pass the same value to len and default_len.
>>
>> So this patch also removes duplicate code from few drivers.
>>
>> Signed-off-by: Felipe F. Tonello 
> 
> Acked-by: Michal Nazarewicz 
> 
>> ---
>>  drivers/usb/gadget/function/f_hid.c| 10 ++
>>  drivers/usb/gadget/function/f_loopback.c   |  9 +
>>  drivers/usb/gadget/function/f_midi.c   | 10 ++
>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
>>  drivers/usb/gadget/u_f.c   |  7 +++
>>  drivers/usb/gadget/u_f.h   |  2 +-
>>  6 files changed, 11 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>> b/drivers/usb/gadget/function/f_hid.c
>> index 51980c50546d..e82a7468252e 100644
>> --- a/drivers/usb/gadget/function/f_hid.c
>> +++ b/drivers/usb/gadget/function/f_hid.c
>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file 
>> *fd)
>>  
>> /*-*/
>>  /*usb_function 
>> */
>>  
>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>> -unsigned length)
>> -{
>> -return alloc_ep_req(ep, length, length);
>> -}
>> -
>>  static void hidg_set_report_complete(struct usb_ep *ep, struct usb_request 
>> *req)
>>  {
>>  struct f_hidg *hidg = (struct f_hidg *) req->context;
>> @@ -549,8 +543,8 @@ static int hidg_set_alt(struct usb_function *f, unsigned 
>> intf, unsigned alt)
>>   */
>>  for (i = 0; i < hidg->qlen && status == 0; i++) {
>>  struct usb_request *req =
>> -hidg_alloc_ep_req(hidg->out_ep,
>> -  hidg->report_length);
>> +alloc_ep_req(hidg->out_ep,
>> +hidg->report_length);
>>  if (req) {
>>  req->complete = hidg_set_report_complete;
>>  req->context  = hidg;
>> diff --git a/drivers/usb/gadget/function/f_loopback.c 
>> b/drivers/usb/gadget/function/f_loopback.c
>> index 3a9f8f9c77bd..701ee0f11c33 100644
>> --- a/drivers/usb/gadget/function/f_loopback.c
>> +++ b/drivers/usb/gadget/function/f_loopback.c
>> @@ -306,13 +306,6 @@ static void disable_loopback(struct f_loopback *loop)
>>  VDBG(cdev, "%s disabled\n", loop->function.name);
>>  }
>>  
>> -static inline struct usb_request *lb_alloc_ep_req(struct usb_ep *ep, int 
>> len)
>> -{
>> -struct f_loopback   *loop = ep->driver_data;
>> -
>> -return alloc_ep_req(ep, len, loop->buflen);
>> -}
>> -
>>  static int alloc_requests(struct usb_composite_dev *cdev,
>>struct f_loopback *loop)
>>  {
>> @@ -333,7 +326,7 @@ static int alloc_requests(struct usb_composite_dev *cdev,
>>  if (!in_req)
>>  goto fail;
>>  
>> -out_req = lb_alloc_ep_req(loop->out_ep, 0);
>> +out_req = alloc_ep_req(loop->out_ep, loop->buflen);
>>  if (!out_req)
>>  goto fail_in;
>>  
>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> b/drivers/usb/gadget/function/f_midi.c
>> index 3a47596afcab..abf26364b46f 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -208,12 +208,6 @@ static struct usb_gadget_strings *midi_strings[] = {
>>  NULL,
>>  };
>>  
>> -static inline struct usb_request *midi_alloc_ep_req(struct usb_ep *ep,
>> -unsigned length)
>> -{
>> -return alloc_ep_req(ep, length, length);
>> -}
>> -
>>  static const uint8_t f_midi_cin_length[] = {
>>  0, 0, 2, 3, 3, 1, 2, 3, 3, 3, 3, 3, 2, 2, 3, 1
>>  };
>> @@ -365,7 +359,7 @@ static int f_midi_set_alt(struct usb_function *f, 
>> unsigned intf, unsigned alt)
>>  /* pre-allocate write usb requests to use on f_midi_transmit. */
>>  while (kfifo_avail(>in_req_fifo)) {
>>  struct usb_request *req =
>> -midi_alloc_ep_req(midi->in_ep, midi->buflen);
>> +alloc_ep_req(midi->in_ep, midi->buflen);
>>  
>>  if (req == NULL)
>>  return -ENOMEM;
>> @@ -379,7 +373,7 @@ static int f_midi_set_alt(struct usb_function *f, 
>> unsigned intf, unsigned alt)
>>  /* allocate a bunch of read buffers and queue them 

Re: [PATCH 7/9] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

Hi Michal,

On 27/07/16 21:02, Michal Nazarewicz wrote:
> On Tue, Jul 26 2016, Felipe F. Tonello wrote:
>> This parameter was not really necessary and gadget drivers would almost 
>> always
> 
> I personally like when commit messages can be read without subject, so
> perhaps:
> 
>   The default_length parameter of alloc_ep_req was not …
> 
> But that’s just me.

Good judgment.

> 
>> create an inline function to pass the same value to len and default_len.
>>
>> So this patch also removes duplicate code from few drivers.
>>
>> Signed-off-by: Felipe F. Tonello 
> 
> Acked-by: Michal Nazarewicz 
> 
>> ---
>>  drivers/usb/gadget/function/f_hid.c| 10 ++
>>  drivers/usb/gadget/function/f_loopback.c   |  9 +
>>  drivers/usb/gadget/function/f_midi.c   | 10 ++
>>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
>>  drivers/usb/gadget/u_f.c   |  7 +++
>>  drivers/usb/gadget/u_f.h   |  2 +-
>>  6 files changed, 11 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_hid.c 
>> b/drivers/usb/gadget/function/f_hid.c
>> index 51980c50546d..e82a7468252e 100644
>> --- a/drivers/usb/gadget/function/f_hid.c
>> +++ b/drivers/usb/gadget/function/f_hid.c
>> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file 
>> *fd)
>>  
>> /*-*/
>>  /*usb_function 
>> */
>>  
>> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
>> -unsigned length)
>> -{
>> -return alloc_ep_req(ep, length, length);
>> -}
>> -
>>  static void hidg_set_report_complete(struct usb_ep *ep, struct usb_request 
>> *req)
>>  {
>>  struct f_hidg *hidg = (struct f_hidg *) req->context;
>> @@ -549,8 +543,8 @@ static int hidg_set_alt(struct usb_function *f, unsigned 
>> intf, unsigned alt)
>>   */
>>  for (i = 0; i < hidg->qlen && status == 0; i++) {
>>  struct usb_request *req =
>> -hidg_alloc_ep_req(hidg->out_ep,
>> -  hidg->report_length);
>> +alloc_ep_req(hidg->out_ep,
>> +hidg->report_length);
>>  if (req) {
>>  req->complete = hidg_set_report_complete;
>>  req->context  = hidg;
>> diff --git a/drivers/usb/gadget/function/f_loopback.c 
>> b/drivers/usb/gadget/function/f_loopback.c
>> index 3a9f8f9c77bd..701ee0f11c33 100644
>> --- a/drivers/usb/gadget/function/f_loopback.c
>> +++ b/drivers/usb/gadget/function/f_loopback.c
>> @@ -306,13 +306,6 @@ static void disable_loopback(struct f_loopback *loop)
>>  VDBG(cdev, "%s disabled\n", loop->function.name);
>>  }
>>  
>> -static inline struct usb_request *lb_alloc_ep_req(struct usb_ep *ep, int 
>> len)
>> -{
>> -struct f_loopback   *loop = ep->driver_data;
>> -
>> -return alloc_ep_req(ep, len, loop->buflen);
>> -}
>> -
>>  static int alloc_requests(struct usb_composite_dev *cdev,
>>struct f_loopback *loop)
>>  {
>> @@ -333,7 +326,7 @@ static int alloc_requests(struct usb_composite_dev *cdev,
>>  if (!in_req)
>>  goto fail;
>>  
>> -out_req = lb_alloc_ep_req(loop->out_ep, 0);
>> +out_req = alloc_ep_req(loop->out_ep, loop->buflen);
>>  if (!out_req)
>>  goto fail_in;
>>  
>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> b/drivers/usb/gadget/function/f_midi.c
>> index 3a47596afcab..abf26364b46f 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -208,12 +208,6 @@ static struct usb_gadget_strings *midi_strings[] = {
>>  NULL,
>>  };
>>  
>> -static inline struct usb_request *midi_alloc_ep_req(struct usb_ep *ep,
>> -unsigned length)
>> -{
>> -return alloc_ep_req(ep, length, length);
>> -}
>> -
>>  static const uint8_t f_midi_cin_length[] = {
>>  0, 0, 2, 3, 3, 1, 2, 3, 3, 3, 3, 3, 2, 2, 3, 1
>>  };
>> @@ -365,7 +359,7 @@ static int f_midi_set_alt(struct usb_function *f, 
>> unsigned intf, unsigned alt)
>>  /* pre-allocate write usb requests to use on f_midi_transmit. */
>>  while (kfifo_avail(>in_req_fifo)) {
>>  struct usb_request *req =
>> -midi_alloc_ep_req(midi->in_ep, midi->buflen);
>> +alloc_ep_req(midi->in_ep, midi->buflen);
>>  
>>  if (req == NULL)
>>  return -ENOMEM;
>> @@ -379,7 +373,7 @@ static int f_midi_set_alt(struct usb_function *f, 
>> unsigned intf, unsigned alt)
>>  /* allocate a bunch of read buffers and queue them all at once. */
>>  for (i = 0; i < 

Re: [PATCH 1/9] usb: gadget: fix usb_ep_align_maybe endianness and new usb_ep_align

[Felipe Ferreri Tonello]

Hi Michal,

On 27/07/16 20:37, Michal Nazarewicz wrote:
> On Tue, Jul 26 2016, Felipe F. Tonello wrote:
>> USB spec specifies wMaxPacketSize to be little endian (as other properties),
>> so when using this variable in the driver we should convert to the current
>> CPU endianness if necessary.
>>
>> This patch also introduces usb_ep_align() which does always returns the
>> aligned buffer size for an endpoint. This is useful to be used by USB 
>> requests
>> allocator functions.
>>
>> Signed-off-by: Felipe F. Tonello 
> 
> Acked-by: Michal Nazarewicz 
> 
>> ---
>>  include/linux/usb/gadget.h | 17 ++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 612dbdfa388e..b8fa6901b881 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -418,8 +418,20 @@ static inline struct usb_gadget 
>> *dev_to_usb_gadget(struct device *dev)
>>  list_for_each_entry(tmp, &(gadget)->ep_list, ep_list)
>>  
>>  /**
>> + * usb_ep_align - returns @len aligned to ep's maxpacketsize.
>> + * @ep: the endpoint whose maxpacketsize is used to align @len
>> + * @len: buffer size's length to align to @ep's maxpacketsize
>> + *
>> + * This helper is used to align buffer's size to an ep's maxpacketsize.
>> + */
>> +static inline size_t usb_ep_align(struct usb_ep *ep, size_t len)
>> +{
>> +return round_up(len, (size_t)le16_to_cpu(ep->desc->wMaxPacketSize));
>> +}
>> +
>> +/**
>>   * usb_ep_align_maybe - returns @len aligned to ep's maxpacketsize if gadget
>> - *  requires quirk_ep_out_aligned_size, otherwise reguens len.
>> + *  requires quirk_ep_out_aligned_size, otherwise returns len.
>>   * @g: controller to check for quirk
>>   * @ep: the endpoint whose maxpacketsize is used to align @len
>>   * @len: buffer size's length to align to @ep's maxpacketsize
>> @@ -430,8 +442,7 @@ static inline struct usb_gadget 
>> *dev_to_usb_gadget(struct device *dev)
>>  static inline size_t
>>  usb_ep_align_maybe(struct usb_gadget *g, struct usb_ep *ep, size_t len)
>>  {
>> -return !g->quirk_ep_out_aligned_size ? len :
>> -round_up(len, (size_t)ep->desc->wMaxPacketSize);
>> +return !g->quirk_ep_out_aligned_size ? len : usb_ep_align(ep, len);
> 
> I’d drop the negation:
> 
> + return g->quirk_ep_out_aligned_size ? usb_ep_align(ep, len) : len;

Agreed.

> 
>>  }
>>  
>>  /**
>> -- 
>> 2.9.0
>>
> 

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/9] usb: gadget: fix usb_ep_align_maybe endianness and new usb_ep_align

[Felipe Ferreri Tonello]

Hi Michal,

On 27/07/16 20:37, Michal Nazarewicz wrote:
> On Tue, Jul 26 2016, Felipe F. Tonello wrote:
>> USB spec specifies wMaxPacketSize to be little endian (as other properties),
>> so when using this variable in the driver we should convert to the current
>> CPU endianness if necessary.
>>
>> This patch also introduces usb_ep_align() which does always returns the
>> aligned buffer size for an endpoint. This is useful to be used by USB 
>> requests
>> allocator functions.
>>
>> Signed-off-by: Felipe F. Tonello 
> 
> Acked-by: Michal Nazarewicz 
> 
>> ---
>>  include/linux/usb/gadget.h | 17 ++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 612dbdfa388e..b8fa6901b881 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -418,8 +418,20 @@ static inline struct usb_gadget 
>> *dev_to_usb_gadget(struct device *dev)
>>  list_for_each_entry(tmp, &(gadget)->ep_list, ep_list)
>>  
>>  /**
>> + * usb_ep_align - returns @len aligned to ep's maxpacketsize.
>> + * @ep: the endpoint whose maxpacketsize is used to align @len
>> + * @len: buffer size's length to align to @ep's maxpacketsize
>> + *
>> + * This helper is used to align buffer's size to an ep's maxpacketsize.
>> + */
>> +static inline size_t usb_ep_align(struct usb_ep *ep, size_t len)
>> +{
>> +return round_up(len, (size_t)le16_to_cpu(ep->desc->wMaxPacketSize));
>> +}
>> +
>> +/**
>>   * usb_ep_align_maybe - returns @len aligned to ep's maxpacketsize if gadget
>> - *  requires quirk_ep_out_aligned_size, otherwise reguens len.
>> + *  requires quirk_ep_out_aligned_size, otherwise returns len.
>>   * @g: controller to check for quirk
>>   * @ep: the endpoint whose maxpacketsize is used to align @len
>>   * @len: buffer size's length to align to @ep's maxpacketsize
>> @@ -430,8 +442,7 @@ static inline struct usb_gadget 
>> *dev_to_usb_gadget(struct device *dev)
>>  static inline size_t
>>  usb_ep_align_maybe(struct usb_gadget *g, struct usb_ep *ep, size_t len)
>>  {
>> -return !g->quirk_ep_out_aligned_size ? len :
>> -round_up(len, (size_t)ep->desc->wMaxPacketSize);
>> +return !g->quirk_ep_out_aligned_size ? len : usb_ep_align(ep, len);
> 
> I’d drop the negation:
> 
> + return g->quirk_ep_out_aligned_size ? usb_ep_align(ep, len) : len;

Agreed.

> 
>>  }
>>  
>>  /**
>> -- 
>> 2.9.0
>>
> 

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v2 7/9] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

Forgot to mention, but changes from v1 is a typo alloc_ep_req().

On 26/07/16 20:18, Felipe F. Tonello wrote:
> This parameter was not really necessary and gadget drivers would almost always
> create an inline function to pass the same value to len and default_len.
> 
> So this patch also removes duplicate code from few drivers.
> 
> Signed-off-by: Felipe F. Tonello 
> ---
>  drivers/usb/gadget/function/f_hid.c| 10 ++
>  drivers/usb/gadget/function/f_loopback.c   |  9 +
>  drivers/usb/gadget/function/f_midi.c   | 10 ++
>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
>  drivers/usb/gadget/u_f.c   |  7 +++
>  drivers/usb/gadget/u_f.h   |  2 +-
>  6 files changed, 11 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_hid.c 
> b/drivers/usb/gadget/function/f_hid.c
> index 51980c50546d..e82a7468252e 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file 
> *fd)
>  /*-*/
>  /*usb_function */
>  
> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
> - unsigned length)
> -{
> - return alloc_ep_req(ep, length, length);
> -}
> -
>  static void hidg_set_report_complete(struct usb_ep *ep, struct usb_request 
> *req)
>  {
>   struct f_hidg *hidg = (struct f_hidg *) req->context;
> @@ -549,8 +543,8 @@ static int hidg_set_alt(struct usb_function *f, unsigned 
> intf, unsigned alt)
>*/
>   for (i = 0; i < hidg->qlen && status == 0; i++) {
>   struct usb_request *req =
> - hidg_alloc_ep_req(hidg->out_ep,
> -   hidg->report_length);
> + alloc_ep_req(hidg->out_ep,
> + hidg->report_length);
>   if (req) {
>   req->complete = hidg_set_report_complete;
>   req->context  = hidg;
> diff --git a/drivers/usb/gadget/function/f_loopback.c 
> b/drivers/usb/gadget/function/f_loopback.c
> index 3a9f8f9c77bd..701ee0f11c33 100644
> --- a/drivers/usb/gadget/function/f_loopback.c
> +++ b/drivers/usb/gadget/function/f_loopback.c
> @@ -306,13 +306,6 @@ static void disable_loopback(struct f_loopback *loop)
>   VDBG(cdev, "%s disabled\n", loop->function.name);
>  }
>  
> -static inline struct usb_request *lb_alloc_ep_req(struct usb_ep *ep, int len)
> -{
> - struct f_loopback   *loop = ep->driver_data;
> -
> - return alloc_ep_req(ep, len, loop->buflen);
> -}
> -
>  static int alloc_requests(struct usb_composite_dev *cdev,
> struct f_loopback *loop)
>  {
> @@ -333,7 +326,7 @@ static int alloc_requests(struct usb_composite_dev *cdev,
>   if (!in_req)
>   goto fail;
>  
> - out_req = lb_alloc_ep_req(loop->out_ep, 0);
> + out_req = alloc_ep_req(loop->out_ep, loop->buflen);
>   if (!out_req)
>   goto fail_in;
>  
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index 3a47596afcab..abf26364b46f 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -208,12 +208,6 @@ static struct usb_gadget_strings *midi_strings[] = {
>   NULL,
>  };
>  
> -static inline struct usb_request *midi_alloc_ep_req(struct usb_ep *ep,
> - unsigned length)
> -{
> - return alloc_ep_req(ep, length, length);
> -}
> -
>  static const uint8_t f_midi_cin_length[] = {
>   0, 0, 2, 3, 3, 1, 2, 3, 3, 3, 3, 3, 2, 2, 3, 1
>  };
> @@ -365,7 +359,7 @@ static int f_midi_set_alt(struct usb_function *f, 
> unsigned intf, unsigned alt)
>   /* pre-allocate write usb requests to use on f_midi_transmit. */
>   while (kfifo_avail(>in_req_fifo)) {
>   struct usb_request *req =
> - midi_alloc_ep_req(midi->in_ep, midi->buflen);
> + alloc_ep_req(midi->in_ep, midi->buflen);
>  
>   if (req == NULL)
>   return -ENOMEM;
> @@ -379,7 +373,7 @@ static int f_midi_set_alt(struct usb_function *f, 
> unsigned intf, unsigned alt)
>   /* allocate a bunch of read buffers and queue them all at once. */
>   for (i = 0; i < midi->qlen && err == 0; i++) {
>   struct usb_request *req =
> - midi_alloc_ep_req(midi->out_ep, midi->buflen);
> + alloc_ep_req(midi->out_ep, midi->buflen);
>  
>   if (req == NULL)
>   return -ENOMEM;
> diff 

Re: [PATCH v2 7/9] usb: gadget: remove useless parameter in alloc_ep_req()

[Felipe Ferreri Tonello]

Forgot to mention, but changes from v1 is a typo alloc_ep_req().

On 26/07/16 20:18, Felipe F. Tonello wrote:
> This parameter was not really necessary and gadget drivers would almost always
> create an inline function to pass the same value to len and default_len.
> 
> So this patch also removes duplicate code from few drivers.
> 
> Signed-off-by: Felipe F. Tonello 
> ---
>  drivers/usb/gadget/function/f_hid.c| 10 ++
>  drivers/usb/gadget/function/f_loopback.c   |  9 +
>  drivers/usb/gadget/function/f_midi.c   | 10 ++
>  drivers/usb/gadget/function/f_sourcesink.c | 11 ++-
>  drivers/usb/gadget/u_f.c   |  7 +++
>  drivers/usb/gadget/u_f.h   |  2 +-
>  6 files changed, 11 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_hid.c 
> b/drivers/usb/gadget/function/f_hid.c
> index 51980c50546d..e82a7468252e 100644
> --- a/drivers/usb/gadget/function/f_hid.c
> +++ b/drivers/usb/gadget/function/f_hid.c
> @@ -362,12 +362,6 @@ static int f_hidg_open(struct inode *inode, struct file 
> *fd)
>  /*-*/
>  /*usb_function */
>  
> -static inline struct usb_request *hidg_alloc_ep_req(struct usb_ep *ep,
> - unsigned length)
> -{
> - return alloc_ep_req(ep, length, length);
> -}
> -
>  static void hidg_set_report_complete(struct usb_ep *ep, struct usb_request 
> *req)
>  {
>   struct f_hidg *hidg = (struct f_hidg *) req->context;
> @@ -549,8 +543,8 @@ static int hidg_set_alt(struct usb_function *f, unsigned 
> intf, unsigned alt)
>*/
>   for (i = 0; i < hidg->qlen && status == 0; i++) {
>   struct usb_request *req =
> - hidg_alloc_ep_req(hidg->out_ep,
> -   hidg->report_length);
> + alloc_ep_req(hidg->out_ep,
> + hidg->report_length);
>   if (req) {
>   req->complete = hidg_set_report_complete;
>   req->context  = hidg;
> diff --git a/drivers/usb/gadget/function/f_loopback.c 
> b/drivers/usb/gadget/function/f_loopback.c
> index 3a9f8f9c77bd..701ee0f11c33 100644
> --- a/drivers/usb/gadget/function/f_loopback.c
> +++ b/drivers/usb/gadget/function/f_loopback.c
> @@ -306,13 +306,6 @@ static void disable_loopback(struct f_loopback *loop)
>   VDBG(cdev, "%s disabled\n", loop->function.name);
>  }
>  
> -static inline struct usb_request *lb_alloc_ep_req(struct usb_ep *ep, int len)
> -{
> - struct f_loopback   *loop = ep->driver_data;
> -
> - return alloc_ep_req(ep, len, loop->buflen);
> -}
> -
>  static int alloc_requests(struct usb_composite_dev *cdev,
> struct f_loopback *loop)
>  {
> @@ -333,7 +326,7 @@ static int alloc_requests(struct usb_composite_dev *cdev,
>   if (!in_req)
>   goto fail;
>  
> - out_req = lb_alloc_ep_req(loop->out_ep, 0);
> + out_req = alloc_ep_req(loop->out_ep, loop->buflen);
>   if (!out_req)
>   goto fail_in;
>  
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index 3a47596afcab..abf26364b46f 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -208,12 +208,6 @@ static struct usb_gadget_strings *midi_strings[] = {
>   NULL,
>  };
>  
> -static inline struct usb_request *midi_alloc_ep_req(struct usb_ep *ep,
> - unsigned length)
> -{
> - return alloc_ep_req(ep, length, length);
> -}
> -
>  static const uint8_t f_midi_cin_length[] = {
>   0, 0, 2, 3, 3, 1, 2, 3, 3, 3, 3, 3, 2, 2, 3, 1
>  };
> @@ -365,7 +359,7 @@ static int f_midi_set_alt(struct usb_function *f, 
> unsigned intf, unsigned alt)
>   /* pre-allocate write usb requests to use on f_midi_transmit. */
>   while (kfifo_avail(>in_req_fifo)) {
>   struct usb_request *req =
> - midi_alloc_ep_req(midi->in_ep, midi->buflen);
> + alloc_ep_req(midi->in_ep, midi->buflen);
>  
>   if (req == NULL)
>   return -ENOMEM;
> @@ -379,7 +373,7 @@ static int f_midi_set_alt(struct usb_function *f, 
> unsigned intf, unsigned alt)
>   /* allocate a bunch of read buffers and queue them all at once. */
>   for (i = 0; i < midi->qlen && err == 0; i++) {
>   struct usb_request *req =
> - midi_alloc_ep_req(midi->out_ep, midi->buflen);
> + alloc_ep_req(midi->out_ep, midi->buflen);
>  
>   if (req == NULL)
>   return -ENOMEM;
> diff --git 

Re: [PATCH v3] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize

[Felipe Ferreri Tonello]

Hi Baolin,

Sorry for not replying for previous emails because for some reason these
emails were archived. :(

On 12/07/16 10:01, Baolin Wang wrote:
> Some gadget device (such as dwc3 gadget) requires quirk_ep_out_aligned_size
> attribute, which means it need to align the request buffer's size to an ep's
> maxpacketsize.
> 
> Thus we add usb_ep_align_maybe() function to check if it is need to align
> the request buffer's size to an ep's maxpacketsize.
> 
> Signed-off-by: Baolin Wang 
> Acked-by: Michal Nazarewicz 
> ---
> Changelog:
> v2:
>  - Simplify the method to get buffer length.
> v1:
>  - Remove the in_ep modification.
>  - Remove max_t() function.
> 
>  drivers/usb/gadget/function/f_midi.c |   11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index 58fc199..3486941 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -359,10 +359,13 @@ static int f_midi_set_alt(struct usb_function *f, 
> unsigned intf, unsigned alt)
>  
>   /* allocate a bunch of read buffers and queue them all at once. */
>   for (i = 0; i < midi->qlen && err == 0; i++) {
> - struct usb_request *req =
> - midi_alloc_ep_req(midi->out_ep,
> - max_t(unsigned, midi->buflen,
> - bulk_out_desc.wMaxPacketSize));
> + struct usb_request *req;
> + unsigned length;
> +
> + length = usb_ep_align_maybe(midi->gadget, midi->out_ep,
> + midi->buflen);
> +
> + req = midi_alloc_ep_req(midi->out_ep, length);
>   if (req == NULL)
>   return -ENOMEM;
>  
> 

Yes, as mentioned before, my intent was to align the size otherwise an
actual nasty bug happens.

I have two problems with this approach:
* usb_ep_align_maybe() has a bug that it doesn't convert wMaxPacketSize
endianness to the CPU. See my patch on that[1] subject. Also this
function uses round_up which expects x and y to be a power of 2, is that
a feasible assumption?
* If the gadget driver doesn't support quirk_ep_out_aligned_size,
usb_ep_align_maybe() can potentially return a len < wMaxPacketSize.
Basically causing a regression.

I believe we should protect this bad behavior on alloc_ep_req() in
drivers/usb/gadget/u_f.c by forcing align the size if the endpoint in
subject is OUT.

[1] [PATCH 1/4] usb: gadget: f_midi: fixed endianness when using
wMaxPacketSize: https://lkml.org/lkml/2016/7/25/1028

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v3] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize

[Felipe Ferreri Tonello]

Hi Baolin,

Sorry for not replying for previous emails because for some reason these
emails were archived. :(

On 12/07/16 10:01, Baolin Wang wrote:
> Some gadget device (such as dwc3 gadget) requires quirk_ep_out_aligned_size
> attribute, which means it need to align the request buffer's size to an ep's
> maxpacketsize.
> 
> Thus we add usb_ep_align_maybe() function to check if it is need to align
> the request buffer's size to an ep's maxpacketsize.
> 
> Signed-off-by: Baolin Wang 
> Acked-by: Michal Nazarewicz 
> ---
> Changelog:
> v2:
>  - Simplify the method to get buffer length.
> v1:
>  - Remove the in_ep modification.
>  - Remove max_t() function.
> 
>  drivers/usb/gadget/function/f_midi.c |   11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index 58fc199..3486941 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -359,10 +359,13 @@ static int f_midi_set_alt(struct usb_function *f, 
> unsigned intf, unsigned alt)
>  
>   /* allocate a bunch of read buffers and queue them all at once. */
>   for (i = 0; i < midi->qlen && err == 0; i++) {
> - struct usb_request *req =
> - midi_alloc_ep_req(midi->out_ep,
> - max_t(unsigned, midi->buflen,
> - bulk_out_desc.wMaxPacketSize));
> + struct usb_request *req;
> + unsigned length;
> +
> + length = usb_ep_align_maybe(midi->gadget, midi->out_ep,
> + midi->buflen);
> +
> + req = midi_alloc_ep_req(midi->out_ep, length);
>   if (req == NULL)
>   return -ENOMEM;
>  
> 

Yes, as mentioned before, my intent was to align the size otherwise an
actual nasty bug happens.

I have two problems with this approach:
* usb_ep_align_maybe() has a bug that it doesn't convert wMaxPacketSize
endianness to the CPU. See my patch on that[1] subject. Also this
function uses round_up which expects x and y to be a power of 2, is that
a feasible assumption?
* If the gadget driver doesn't support quirk_ep_out_aligned_size,
usb_ep_align_maybe() can potentially return a len < wMaxPacketSize.
Basically causing a regression.

I believe we should protect this bad behavior on alloc_ep_req() in
drivers/usb/gadget/u_f.c by forcing align the size if the endpoint in
subject is OUT.

[1] [PATCH 1/4] usb: gadget: f_midi: fixed endianness when using
wMaxPacketSize: https://lkml.org/lkml/2016/7/25/1028

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

[Felipe Ferreri Tonello]

Hi Balbi,

On 04/04/16 11:46, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>>>> On 30/03/16 13:33, Michal Nazarewicz wrote:
>>>>> On Wed, Mar 30 2016, Felipe Balbi wrote:
>>>>>> a USB packet, right. that's correct. But a struct usb_request can
>>>>>> point to whatever size buffer it wants and UDC is required to split
>>>>>> that into wMaxPacketSize transfers.
>>>>>
>>>>> D’oh.  Of course.  Disregard all my comments on the patch (except for
>>>>> Ack).
>>>>>
>>>>
>>>> I didn't really get it. Does that mean that if buflen is multiple of
>>>> wMaxPacketSize, the UDC driver should fit as many [DATA] packets into
>>>> one usb_request and call complete() or it will always call complete() on
>>>> each [DATA] packet, thus not requiring buflen at all?
>>>>
>>>> Does that mean that we can still use buflen and this patch is still
>>>> valid? (besides the endianess issue that was addressed on v2)
>>>
>>> if you have e.g. 2048 bytes of data to transfer and wMaxPacketSize is
>>> e.g. 256 bytes, the UDC controller is required to do whatever it needs
>>> to do to transfer 2048 bytes (or less if there's a short packet).
>>>
>>> You don't need to break these 2048 bytes into several requests yourself,
>>> the UDC is required to do that for the gadget.
>>
>> Right, what about OUT endpoints?
> 
> also applicable
> 

Ok, so I will make few tests here and resend a v3 probably with buflen
still.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

[Felipe Ferreri Tonello]

Hi Balbi,

On 04/04/16 11:46, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>>>> On 30/03/16 13:33, Michal Nazarewicz wrote:
>>>>> On Wed, Mar 30 2016, Felipe Balbi wrote:
>>>>>> a USB packet, right. that's correct. But a struct usb_request can
>>>>>> point to whatever size buffer it wants and UDC is required to split
>>>>>> that into wMaxPacketSize transfers.
>>>>>
>>>>> D’oh.  Of course.  Disregard all my comments on the patch (except for
>>>>> Ack).
>>>>>
>>>>
>>>> I didn't really get it. Does that mean that if buflen is multiple of
>>>> wMaxPacketSize, the UDC driver should fit as many [DATA] packets into
>>>> one usb_request and call complete() or it will always call complete() on
>>>> each [DATA] packet, thus not requiring buflen at all?
>>>>
>>>> Does that mean that we can still use buflen and this patch is still
>>>> valid? (besides the endianess issue that was addressed on v2)
>>>
>>> if you have e.g. 2048 bytes of data to transfer and wMaxPacketSize is
>>> e.g. 256 bytes, the UDC controller is required to do whatever it needs
>>> to do to transfer 2048 bytes (or less if there's a short packet).
>>>
>>> You don't need to break these 2048 bytes into several requests yourself,
>>> the UDC is required to do that for the gadget.
>>
>> Right, what about OUT endpoints?
> 
> also applicable
> 

Ok, so I will make few tests here and resend a v3 probably with buflen
still.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

[Felipe Ferreri Tonello]

Hi Balbi,

On 01/04/16 11:22, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>> Hi Balbi and Mina,
>>
>> On 30/03/16 13:33, Michal Nazarewicz wrote:
>>> On Wed, Mar 30 2016, Felipe Balbi wrote:
>>>> a USB packet, right. that's correct. But a struct usb_request can
>>>> point to whatever size buffer it wants and UDC is required to split
>>>> that into wMaxPacketSize transfers.
>>>
>>> D’oh.  Of course.  Disregard all my comments on the patch (except for
>>> Ack).
>>>
>>
>> I didn't really get it. Does that mean that if buflen is multiple of
>> wMaxPacketSize, the UDC driver should fit as many [DATA] packets into
>> one usb_request and call complete() or it will always call complete() on
>> each [DATA] packet, thus not requiring buflen at all?
>>
>> Does that mean that we can still use buflen and this patch is still
>> valid? (besides the endianess issue that was addressed on v2)
> 
> if you have e.g. 2048 bytes of data to transfer and wMaxPacketSize is
> e.g. 256 bytes, the UDC controller is required to do whatever it needs
> to do to transfer 2048 bytes (or less if there's a short packet).
> 
> You don't need to break these 2048 bytes into several requests yourself,
> the UDC is required to do that for the gadget.

Right, what about OUT endpoints?

So that means that buflen is still usable, at least on IN endpoints.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

[Felipe Ferreri Tonello]

Hi Balbi,

On 01/04/16 11:22, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>> Hi Balbi and Mina,
>>
>> On 30/03/16 13:33, Michal Nazarewicz wrote:
>>> On Wed, Mar 30 2016, Felipe Balbi wrote:
>>>> a USB packet, right. that's correct. But a struct usb_request can
>>>> point to whatever size buffer it wants and UDC is required to split
>>>> that into wMaxPacketSize transfers.
>>>
>>> D’oh.  Of course.  Disregard all my comments on the patch (except for
>>> Ack).
>>>
>>
>> I didn't really get it. Does that mean that if buflen is multiple of
>> wMaxPacketSize, the UDC driver should fit as many [DATA] packets into
>> one usb_request and call complete() or it will always call complete() on
>> each [DATA] packet, thus not requiring buflen at all?
>>
>> Does that mean that we can still use buflen and this patch is still
>> valid? (besides the endianess issue that was addressed on v2)
> 
> if you have e.g. 2048 bytes of data to transfer and wMaxPacketSize is
> e.g. 256 bytes, the UDC controller is required to do whatever it needs
> to do to transfer 2048 bytes (or less if there's a short packet).
> 
> You don't need to break these 2048 bytes into several requests yourself,
> the UDC is required to do that for the gadget.

Right, what about OUT endpoints?

So that means that buflen is still usable, at least on IN endpoints.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

[Felipe Ferreri Tonello]

Hi Balbi and Mina,

On 30/03/16 13:33, Michal Nazarewicz wrote:
> On Wed, Mar 30 2016, Felipe Balbi wrote:
>> a USB packet, right. that's correct. But a struct usb_request can
>> point to whatever size buffer it wants and UDC is required to split
>> that into wMaxPacketSize transfers.
> 
> D’oh.  Of course.  Disregard all my comments on the patch (except for
> Ack).
> 

I didn't really get it. Does that mean that if buflen is multiple of
wMaxPacketSize, the UDC driver should fit as many [DATA] packets into
one usb_request and call complete() or it will always call complete() on
each [DATA] packet, thus not requiring buflen at all?

Does that mean that we can still use buflen and this patch is still
valid? (besides the endianess issue that was addressed on v2)

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

[Felipe Ferreri Tonello]

Hi Balbi and Mina,

On 30/03/16 13:33, Michal Nazarewicz wrote:
> On Wed, Mar 30 2016, Felipe Balbi wrote:
>> a USB packet, right. that's correct. But a struct usb_request can
>> point to whatever size buffer it wants and UDC is required to split
>> that into wMaxPacketSize transfers.
> 
> D’oh.  Of course.  Disregard all my comments on the patch (except for
> Ack).
> 

I didn't really get it. Does that mean that if buflen is multiple of
wMaxPacketSize, the UDC driver should fit as many [DATA] packets into
one usb_request and call complete() or it will always call complete() on
each [DATA] packet, thus not requiring buflen at all?

Does that mean that we can still use buflen and this patch is still
valid? (besides the endianess issue that was addressed on v2)

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

[Felipe Ferreri Tonello]

Hi Michal,

On 11/03/16 23:07, Michal Nazarewicz wrote:
> On Wed, Mar 09 2016, Felipe F. Tonello wrote:
>> buflen by default (256) is smaller than wMaxPacketSize (512) in high-speed
>> devices.
>>
>> That caused the OUT endpoint to freeze if the host send any data packet of
>> length greater than 256 bytes.
>>
>> This is an example dump of what happended on that enpoint:
>> HOST:   [DATA][Length=260][...]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> ...
>> HOST:   [PING]
>> DEVICE: [NAK]
>>
>> This patch fixes this problem by setting the minimum usb_request's buffer 
>> size
>> for the OUT endpoint as its wMaxPacketSize.
>>
>> Signed-off-by: Felipe F. Tonello 
> 
> Acked-by: Michal Nazarewicz 
> 
> But see comment below:
> 
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> b/drivers/usb/gadget/function/f_midi.c
>> index 8475e3dc82d4..826ba641f29d 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -366,7 +366,9 @@ static int f_midi_set_alt(struct usb_function *f, 
>> unsigned intf, unsigned alt)
>>  /* allocate a bunch of read buffers and queue them all at once. */
>>  for (i = 0; i < midi->qlen && err == 0; i++) {
>>  struct usb_request *req =
>> -midi_alloc_ep_req(midi->out_ep, midi->buflen);
>> +midi_alloc_ep_req(midi->out_ep,
>> +max_t(unsigned, midi->buflen,
>> +bulk_out_desc.wMaxPacketSize));
> 
> Or, just:
> 
> + midi_alloc_ep_req(midi->out_ep,
> +   bulk_out_desc.wMaxPacketSize);
> 
> Packet cannot be greater than wMaxPacketSize so there is no need to
> allocate more (if buflen > wMaxPacketSize).

I didn't know that was a constraint. If so, I agree with you.

> 
>>  if (req == NULL)
>>  return -ENOMEM;
>>  
> 
> I’m also wondering whether it would be beneficial to get rid of buflen
> all together and use wMaxPacketSie for in endpoints as well?  Is that
> feasible?

Yes, we could just remove the buflen parameter.

The only scenario where I can see buflen been "useful" is if the user
wants to have a smaller buffer size for the OUT endpoint. Should we
support this case or not?

I can rework this patch for any case we agree on.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

[Felipe Ferreri Tonello]

Hi Michal,

On 11/03/16 23:07, Michal Nazarewicz wrote:
> On Wed, Mar 09 2016, Felipe F. Tonello wrote:
>> buflen by default (256) is smaller than wMaxPacketSize (512) in high-speed
>> devices.
>>
>> That caused the OUT endpoint to freeze if the host send any data packet of
>> length greater than 256 bytes.
>>
>> This is an example dump of what happended on that enpoint:
>> HOST:   [DATA][Length=260][...]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> ...
>> HOST:   [PING]
>> DEVICE: [NAK]
>>
>> This patch fixes this problem by setting the minimum usb_request's buffer 
>> size
>> for the OUT endpoint as its wMaxPacketSize.
>>
>> Signed-off-by: Felipe F. Tonello 
> 
> Acked-by: Michal Nazarewicz 
> 
> But see comment below:
> 
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> b/drivers/usb/gadget/function/f_midi.c
>> index 8475e3dc82d4..826ba641f29d 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -366,7 +366,9 @@ static int f_midi_set_alt(struct usb_function *f, 
>> unsigned intf, unsigned alt)
>>  /* allocate a bunch of read buffers and queue them all at once. */
>>  for (i = 0; i < midi->qlen && err == 0; i++) {
>>  struct usb_request *req =
>> -midi_alloc_ep_req(midi->out_ep, midi->buflen);
>> +midi_alloc_ep_req(midi->out_ep,
>> +max_t(unsigned, midi->buflen,
>> +bulk_out_desc.wMaxPacketSize));
> 
> Or, just:
> 
> + midi_alloc_ep_req(midi->out_ep,
> +   bulk_out_desc.wMaxPacketSize);
> 
> Packet cannot be greater than wMaxPacketSize so there is no need to
> allocate more (if buflen > wMaxPacketSize).

I didn't know that was a constraint. If so, I agree with you.

> 
>>  if (req == NULL)
>>  return -ENOMEM;
>>  
> 
> I’m also wondering whether it would be beneficial to get rid of buflen
> all together and use wMaxPacketSie for in endpoints as well?  Is that
> feasible?

Yes, we could just remove the buflen parameter.

The only scenario where I can see buflen been "useful" is if the user
wants to have a smaller buffer size for the OUT endpoint. Should we
support this case or not?

I can rework this patch for any case we agree on.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

[Felipe Ferreri Tonello]

Hi Steve,

On 09/03/16 22:43, Steve Calfee wrote:
> On Wed, Mar 9, 2016 at 11:39 AM, Felipe F. Tonello  
> wrote:
>> buflen by default (256) is smaller than wMaxPacketSize (512) in high-speed
>> devices.
>>
>> That caused the OUT endpoint to freeze if the host send any data packet of
>> length greater than 256 bytes.
>>
>> This is an example dump of what happended on that enpoint:
>> HOST:   [DATA][Length=260][...]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> ...
>> HOST:   [PING]
>> DEVICE: [NAK]
>>
>> This patch fixes this problem by setting the minimum usb_request's buffer 
>> size
>> for the OUT endpoint as its wMaxPacketSize.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> b/drivers/usb/gadget/function/f_midi.c
>> index 8475e3dc82d4..826ba641f29d 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -366,7 +366,9 @@ static int f_midi_set_alt(struct usb_function *f, 
>> unsigned intf, unsigned alt)
>> /* allocate a bunch of read buffers and queue them all at once. */
>> for (i = 0; i < midi->qlen && err == 0; i++) {
>> struct usb_request *req =
>> -   midi_alloc_ep_req(midi->out_ep, midi->buflen);
>> +   midi_alloc_ep_req(midi->out_ep,
>> +   max_t(unsigned, midi->buflen,
>> +   bulk_out_desc.wMaxPacketSize));
>> if (req == NULL)
>> return -ENOMEM;
>>
> Won't you get a buffer overrun if midi->buflen is less than wMaxPacketSize?
> 

No, because of the *max_t(unsigned, midi->buflen,
bulk_out_desc.wMaxPacketSize)*.

Maybe that's not the most clear indentation but I had to break it in
order to avoid passing 80 columns.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize

[Felipe Ferreri Tonello]

Hi Steve,

On 09/03/16 22:43, Steve Calfee wrote:
> On Wed, Mar 9, 2016 at 11:39 AM, Felipe F. Tonello  
> wrote:
>> buflen by default (256) is smaller than wMaxPacketSize (512) in high-speed
>> devices.
>>
>> That caused the OUT endpoint to freeze if the host send any data packet of
>> length greater than 256 bytes.
>>
>> This is an example dump of what happended on that enpoint:
>> HOST:   [DATA][Length=260][...]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> HOST:   [PING]
>> DEVICE: [NAK]
>> ...
>> HOST:   [PING]
>> DEVICE: [NAK]
>>
>> This patch fixes this problem by setting the minimum usb_request's buffer 
>> size
>> for the OUT endpoint as its wMaxPacketSize.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> b/drivers/usb/gadget/function/f_midi.c
>> index 8475e3dc82d4..826ba641f29d 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -366,7 +366,9 @@ static int f_midi_set_alt(struct usb_function *f, 
>> unsigned intf, unsigned alt)
>> /* allocate a bunch of read buffers and queue them all at once. */
>> for (i = 0; i < midi->qlen && err == 0; i++) {
>> struct usb_request *req =
>> -   midi_alloc_ep_req(midi->out_ep, midi->buflen);
>> +   midi_alloc_ep_req(midi->out_ep,
>> +   max_t(unsigned, midi->buflen,
>> +   bulk_out_desc.wMaxPacketSize));
>> if (req == NULL)
>> return -ENOMEM;
>>
> Won't you get a buffer overrun if midi->buflen is less than wMaxPacketSize?
> 

No, because of the *max_t(unsigned, midi->buflen,
bulk_out_desc.wMaxPacketSize)*.

Maybe that's not the most clear indentation but I had to break it in
order to avoid passing 80 columns.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Ferreri Tonello]

Hi,

On 09/03/16 16:17, Felipe F. Tonello wrote:
> Since f_midi_transmit is called by both ALSA and USB sub-systems, it can
> potentially cause a race condition between both calls because f_midi_transmit
> is not reentrant nor thread-safe. This is due to an implementation detail that
> the transmit function looks for the next available usb request from the fifo
> and only enqueues it if there is data to send, otherwise just re-uses it. So,
> if both ALSA and USB frameworks calls this function at the same time,
> kfifo_seek() will return the same usb_request, which will cause a race
> condition.
> 
> To solve this problem a syncronization mechanism is necessary. In this case it
> is used a spinlock since f_midi_transmit is also called by 
> usb_request->complete
> callback in interrupt context.
> 
> Cc:  # v4.5+
> Fixes: e1e3d7ec5da3 ("usb: gadget: f_midi: pre-allocate IN requests")
> Signed-off-by: Felipe F. Tonello 

I'm sorry but I forgot to add v2 to the subject prefix.

Anyway, the changes from the previous patch is that this patch is on top
of Linus' v4.5-rc7 tag.

> ---
>  drivers/usb/gadget/function/f_midi.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index fb1fe96d3215..ecb46d6abf0e 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -24,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -92,6 +93,7 @@ struct f_midi {
>   /* This fifo is used as a buffer ring for pre-allocated IN usb_requests 
> */
>   DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
>   unsigned int in_last_port;
> + spinlock_t transmit_lock;
>  };
>  
>  static inline struct f_midi *func_to_midi(struct usb_function *f)
> @@ -535,12 +537,15 @@ static void f_midi_drop_out_substreams(struct f_midi 
> *midi)
>  static void f_midi_transmit(struct f_midi *midi)
>  {
>   struct usb_ep *ep = midi->in_ep;
> + unsigned long flags;
>   bool active;
>  
>   /* We only care about USB requests if IN endpoint is enabled */
>   if (!ep || !ep->enabled)
>   goto drop_out;
>  
> + spin_lock_irqsave(>transmit_lock, flags);
> +
>   do {
>   struct usb_request *req = NULL;
>   unsigned int len, i;
> @@ -552,14 +557,17 @@ static void f_midi_transmit(struct f_midi *midi)
>   len = kfifo_peek(>in_req_fifo, );
>   if (len != 1) {
>   ERROR(midi, "%s: Couldn't get usb request\n", __func__);
> + spin_unlock_irqrestore(>transmit_lock, flags);
>   goto drop_out;
>   }
>  
>   /* If buffer overrun, then we ignore this transmission.
>* IMPORTANT: This will cause the user-space rawmidi device to 
> block until a) usb
>* requests have been completed or b) snd_rawmidi_write() times 
> out. */
> - if (req->length > 0)
> + if (req->length > 0) {
> + spin_unlock_irqrestore(>transmit_lock, flags);
>   return;
> + }
>  
>   for (i = midi->in_last_port; i < MAX_PORTS; i++) {
>   struct gmidi_in_port *port = midi->in_port[i];
> @@ -611,6 +619,8 @@ static void f_midi_transmit(struct f_midi *midi)
>   }
>   } while (active);
>  
> + spin_unlock_irqrestore(>transmit_lock, flags);
> +
>   return;
>  
>  drop_out:
> @@ -1216,6 +1226,8 @@ static struct usb_function *f_midi_alloc(struct 
> usb_function_instance *fi)
>   if (status)
>   goto setup_fail;
>  
> + spin_lock_init(>transmit_lock);
> +
>   ++opts->refcnt;
>   mutex_unlock(>lock);
>  
> 

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Ferreri Tonello]

Hi,

On 09/03/16 16:17, Felipe F. Tonello wrote:
> Since f_midi_transmit is called by both ALSA and USB sub-systems, it can
> potentially cause a race condition between both calls because f_midi_transmit
> is not reentrant nor thread-safe. This is due to an implementation detail that
> the transmit function looks for the next available usb request from the fifo
> and only enqueues it if there is data to send, otherwise just re-uses it. So,
> if both ALSA and USB frameworks calls this function at the same time,
> kfifo_seek() will return the same usb_request, which will cause a race
> condition.
> 
> To solve this problem a syncronization mechanism is necessary. In this case it
> is used a spinlock since f_midi_transmit is also called by 
> usb_request->complete
> callback in interrupt context.
> 
> Cc:  # v4.5+
> Fixes: e1e3d7ec5da3 ("usb: gadget: f_midi: pre-allocate IN requests")
> Signed-off-by: Felipe F. Tonello 

I'm sorry but I forgot to add v2 to the subject prefix.

Anyway, the changes from the previous patch is that this patch is on top
of Linus' v4.5-rc7 tag.

> ---
>  drivers/usb/gadget/function/f_midi.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index fb1fe96d3215..ecb46d6abf0e 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -24,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -92,6 +93,7 @@ struct f_midi {
>   /* This fifo is used as a buffer ring for pre-allocated IN usb_requests 
> */
>   DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
>   unsigned int in_last_port;
> + spinlock_t transmit_lock;
>  };
>  
>  static inline struct f_midi *func_to_midi(struct usb_function *f)
> @@ -535,12 +537,15 @@ static void f_midi_drop_out_substreams(struct f_midi 
> *midi)
>  static void f_midi_transmit(struct f_midi *midi)
>  {
>   struct usb_ep *ep = midi->in_ep;
> + unsigned long flags;
>   bool active;
>  
>   /* We only care about USB requests if IN endpoint is enabled */
>   if (!ep || !ep->enabled)
>   goto drop_out;
>  
> + spin_lock_irqsave(>transmit_lock, flags);
> +
>   do {
>   struct usb_request *req = NULL;
>   unsigned int len, i;
> @@ -552,14 +557,17 @@ static void f_midi_transmit(struct f_midi *midi)
>   len = kfifo_peek(>in_req_fifo, );
>   if (len != 1) {
>   ERROR(midi, "%s: Couldn't get usb request\n", __func__);
> + spin_unlock_irqrestore(>transmit_lock, flags);
>   goto drop_out;
>   }
>  
>   /* If buffer overrun, then we ignore this transmission.
>* IMPORTANT: This will cause the user-space rawmidi device to 
> block until a) usb
>* requests have been completed or b) snd_rawmidi_write() times 
> out. */
> - if (req->length > 0)
> + if (req->length > 0) {
> + spin_unlock_irqrestore(>transmit_lock, flags);
>   return;
> + }
>  
>   for (i = midi->in_last_port; i < MAX_PORTS; i++) {
>   struct gmidi_in_port *port = midi->in_port[i];
> @@ -611,6 +619,8 @@ static void f_midi_transmit(struct f_midi *midi)
>   }
>   } while (active);
>  
> + spin_unlock_irqrestore(>transmit_lock, flags);
> +
>   return;
>  
>  drop_out:
> @@ -1216,6 +1226,8 @@ static struct usb_function *f_midi_alloc(struct 
> usb_function_instance *fi)
>   if (status)
>   goto setup_fail;
>  
> + spin_lock_init(>transmit_lock);
> +
>   ++opts->refcnt;
>   mutex_unlock(>lock);
>  
> 

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 2/5] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Ferreri Tonello]

Hi Balbi,

On 08/03/16 14:01, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>>>>>>>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>>>>>>> can
>>>>>>>> potentially cause a race condition between both calls. This is bad
>>>>>>> because the
>>>>>>>> way f_midi_transmit is implemented can't handle concurrent calls.
>>>>>>> This is due
>>>>>>>> to the fact that the usb request fifo looks for the next element and
>>>>>>> only if
>>>>>>>> it has data to process it enqueues the request, otherwise re-uses it.
>>>>>>> If both
>>>>>>>> (ALSA and USB) frameworks calls this function at the same time, the
>>>>>>>> kfifo_seek() will return the same usb_request, which will cause a
>>>>>>> race
>>>>>>>> condition.
>>>>>>>>
>>>>>>>> To solve this problem a syncronization mechanism is necessary. In
>>>>>>> this case it
>>>>>>>> is used a spinlock since f_midi_transmit is also called by
>>>>>>> usb_request->complete
>>>>>>>> callback in interrupt context.
>>>>>>>>
>>>>>>>> On benchmarks realized by me, spinlocks were more efficient then
>>>>>>> scheduling
>>>>>>>> the f_midi_transmit tasklet in process context and using a mutex
>>>>>>>> to synchronize. Also it performs better then previous
>>>>>>>> implementation
>>>>>>> that
>>>>>>>> allocated a usb_request for every new transmit made.
>>>>>>>
>>>>>>> behaves better in what way ? Also, previous implementation would not
>>>>>>> suffer from this concurrency problem, right ?
>>>>>>
>>>>>> The spin lock is faster than allocating usb requests all the time,
>>>>>> even if the udc uses da for it.
>>>>>
>>>>> did you measure ? Is the extra speed really necessary ? How did you
>>>>> benchmark this ?
>>>>
>>>> Yes I did measure and it was not that significant. This is not about
>>>> speed. There was a bug in that approach that I already explained on
>>>
>>> you have very confusing statements. When I mentioned that previous code
>>> wouldn't have the need for the spinlock you replied that spinlock was
>>> faster.
>>>
>>> When I asked you about benchmarks you reply saying it's not about the
>>> speed.
>>>
>>> Make up your mind dude. What are you trying to achieve ?
>>>
>>>> that patch, which was approved and applied BTW.
>>>
>>> patches can be reverted if we realise we're better off without
>>> them. Don't get cocky, please.
>>
>> Yes am I aware of that, but I honestly think that is the wrong way of
>> dealing with this.
>>
>> ?? I don't get why am I giving this impression.
> 
> re-read your emails. The gist goes like this:
> 
> . Send patch
> . Got comments
> . Well, whatever, you can just ignore if you don't agree

This is one of the problems with email. It can give the wrong impression
and feelings. :)

That was not what I meant at all. I mean that for real, not in a
childish manner. I'm sorry if I gave you that impression.

> 
>>>> Any way, this spinlock should've been there since that patch but I
>>>> couldn't really trigger this problem without a stress test.
>>>
>>> which tells me you sent me patches without properly testing. How much
>>> time did it take to trigger this ? How did you trigger this situation ?
>>
>> No, that is no true. The implementation I sent is working properly for
>> any real world usage.
>>
>> The stress test I made to break the current implementation is *not* a
>> real use-case. I made it in order to push as far as possible how fast
>> the driver can *reliably* handle while sending and reading data. Then I
>> noticed the bug.
>>
>> So, to answer your question. To trigger this bug is not a matter of
>> time. The following needs to happen:
>>  1. Device send MIDI message that is *bigger* than the usb request
>> length. (just this by itself is really unlikely to happen in real world
>> usage)
> 
> I wouldn't say it's unl

Re: [PATCH 2/5] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Ferreri Tonello]

Hi Balbi,

On 08/03/16 14:01, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>>>>>>>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>>>>>>> can
>>>>>>>> potentially cause a race condition between both calls. This is bad
>>>>>>> because the
>>>>>>>> way f_midi_transmit is implemented can't handle concurrent calls.
>>>>>>> This is due
>>>>>>>> to the fact that the usb request fifo looks for the next element and
>>>>>>> only if
>>>>>>>> it has data to process it enqueues the request, otherwise re-uses it.
>>>>>>> If both
>>>>>>>> (ALSA and USB) frameworks calls this function at the same time, the
>>>>>>>> kfifo_seek() will return the same usb_request, which will cause a
>>>>>>> race
>>>>>>>> condition.
>>>>>>>>
>>>>>>>> To solve this problem a syncronization mechanism is necessary. In
>>>>>>> this case it
>>>>>>>> is used a spinlock since f_midi_transmit is also called by
>>>>>>> usb_request->complete
>>>>>>>> callback in interrupt context.
>>>>>>>>
>>>>>>>> On benchmarks realized by me, spinlocks were more efficient then
>>>>>>> scheduling
>>>>>>>> the f_midi_transmit tasklet in process context and using a mutex
>>>>>>>> to synchronize. Also it performs better then previous
>>>>>>>> implementation
>>>>>>> that
>>>>>>>> allocated a usb_request for every new transmit made.
>>>>>>>
>>>>>>> behaves better in what way ? Also, previous implementation would not
>>>>>>> suffer from this concurrency problem, right ?
>>>>>>
>>>>>> The spin lock is faster than allocating usb requests all the time,
>>>>>> even if the udc uses da for it.
>>>>>
>>>>> did you measure ? Is the extra speed really necessary ? How did you
>>>>> benchmark this ?
>>>>
>>>> Yes I did measure and it was not that significant. This is not about
>>>> speed. There was a bug in that approach that I already explained on
>>>
>>> you have very confusing statements. When I mentioned that previous code
>>> wouldn't have the need for the spinlock you replied that spinlock was
>>> faster.
>>>
>>> When I asked you about benchmarks you reply saying it's not about the
>>> speed.
>>>
>>> Make up your mind dude. What are you trying to achieve ?
>>>
>>>> that patch, which was approved and applied BTW.
>>>
>>> patches can be reverted if we realise we're better off without
>>> them. Don't get cocky, please.
>>
>> Yes am I aware of that, but I honestly think that is the wrong way of
>> dealing with this.
>>
>> ?? I don't get why am I giving this impression.
> 
> re-read your emails. The gist goes like this:
> 
> . Send patch
> . Got comments
> . Well, whatever, you can just ignore if you don't agree

This is one of the problems with email. It can give the wrong impression
and feelings. :)

That was not what I meant at all. I mean that for real, not in a
childish manner. I'm sorry if I gave you that impression.

> 
>>>> Any way, this spinlock should've been there since that patch but I
>>>> couldn't really trigger this problem without a stress test.
>>>
>>> which tells me you sent me patches without properly testing. How much
>>> time did it take to trigger this ? How did you trigger this situation ?
>>
>> No, that is no true. The implementation I sent is working properly for
>> any real world usage.
>>
>> The stress test I made to break the current implementation is *not* a
>> real use-case. I made it in order to push as far as possible how fast
>> the driver can *reliably* handle while sending and reading data. Then I
>> noticed the bug.
>>
>> So, to answer your question. To trigger this bug is not a matter of
>> time. The following needs to happen:
>>  1. Device send MIDI message that is *bigger* than the usb request
>> length. (just this by itself is really unlikely to happen in real world
>> usage)
> 
> I wouldn't say it's unlikely. You just cannot trust the other

Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes

[Felipe Ferreri Tonello]

Hi,

On 08/03/16 14:20, Felipe Balbi wrote:
> 
> Hi,
> 
> Krzysztof Opasiak <k.opas...@samsung.com> writes:
>> [ text/plain ]
>>
>>
>> On 03/08/2016 02:54 PM, Felipe Ferreri Tonello wrote:
>>  (...)
>>
>>>>
>>>>> sort of preset library of configfs-based gadget drivers, we still need
>>>>> these modules.
>>>>
>>>> there is already a library called libusbg.
>>>
>>> By preset library I meant scripts or little programs that implement the
>>> legacy drivers we have.
>>>
>>
>> libusbgx implements an idea of gadget schemes[1]. That's simple
>> configuration files using libconfig syntax. I don't see any problems to
>> use it to create legacy gadget equivalents. Then you could simply load
>> it using usbg_import_gadget() in C code or gt[2] load from shell.
> 
> cool, bmAttributes and bMaxPower are already there. Nice.
> 

Yes! It is pretty awesome.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes

[Felipe Ferreri Tonello]

Hi,

On 08/03/16 14:20, Felipe Balbi wrote:
> 
> Hi,
> 
> Krzysztof Opasiak  writes:
>> [ text/plain ]
>>
>>
>> On 03/08/2016 02:54 PM, Felipe Ferreri Tonello wrote:
>>  (...)
>>
>>>>
>>>>> sort of preset library of configfs-based gadget drivers, we still need
>>>>> these modules.
>>>>
>>>> there is already a library called libusbg.
>>>
>>> By preset library I meant scripts or little programs that implement the
>>> legacy drivers we have.
>>>
>>
>> libusbgx implements an idea of gadget schemes[1]. That's simple
>> configuration files using libconfig syntax. I don't see any problems to
>> use it to create legacy gadget equivalents. Then you could simply load
>> it using usbg_import_gadget() in C code or gt[2] load from shell.
> 
> cool, bmAttributes and bMaxPower are already there. Nice.
> 

Yes! It is pretty awesome.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes

[Felipe Ferreri Tonello]

Hi Balbi,

On 08/03/16 07:43, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>>>>>>>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>>>>>>>  module_param(out_ports, uint, S_IRUGO);
>>>>>>>>  MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>>>>>>>  
>>>>>>>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>>>>>>>> +module_param(bmAttributes, uint, S_IRUGO);
>>>>>>>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>>>>>>> bmAttributes parameter");
>>>>>>>> +
>>>>>>>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>>>>>>>> +module_param(MaxPower, uint, S_IRUGO);
>>>>>>>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>>>>>>> Descriptor's bMaxPower parameter");
>>>>>>>
>>>>>>> you didn't run checkpatch, did you ? Also, are you sure you will need
>>>>>>> to
>>>>>>> change this by simply reloading the module ? I'm not convinced.
>>>>>>
>>>>>> Yes I always run checkpatch :)
>>>>>
>>>>> do you really ? Why do you have a 98-character line, then ?
> 
> btw, you didn't reply this ^^
> 
>>>>>>>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>>>>>>>.label  = "MIDI Gadget",
>>>>>>>>.bConfigurationValue = 1,
>>>>>>>>/* .iConfiguration = DYNAMIC */
>>>>>>>> -  .bmAttributes   = USB_CONFIG_ATT_ONE,
>>>>>>>
>>>>>>> nack, nackety nack nack nack :-)
>>>>>>>
>>>>>>> USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>>>>>>> give users the oportunity to violate USB spec.
>>>>>>
>>>>>> You are right. It needs to check the value before it assigns to
>>>>>> bmAttributes.
>>>>>
>>>>> why check ? You can just unconditionally or USB_CONFIG_ATT_ONE. In any
>>>>> case, I'm not convinced this is necessary at all.
>>>>
>>>> Right.
>>>>
>>>> This is necessary because this driver is actually wrong in which is
>>>> asking for the host to power itself. This is not specified on USB-MIDI
>>>> specification, neither makes any sense since this configuration is
>>>> device specific.
>>>>
>>>> What is your suggestion to make it configurable? Maybe at compile-time?
>>>> I really don't know what is the best solution if this is not something
>>>> you like it.
>>>
>>> well, you could use our configfs-based gadget interface. You don't
>>> really need to use gmidi.ko at all. In fact, we wanna do away with any
>>> static modules and rely only on configfs. If configfs doesn't let you
>>> change what you want/need, then we can talk about adding support for
>>> those.
>>>
>>> bMaxPower and bmAttributes sound like good things to have configurable
>>> over configfs but beware of what the USB specification says about them,
>>> we cannot let users violate the spec by passing bogus values on these
>>> fields.
>>
>> I agree that we should move to configfs, but the truth is that these
>> legacy devices are still useful. They just do one thing, mostly, but
> 
> yes, they are useful as they are. They don't need to be changed to be
> useful. Plus, you can have a gadget built with configfs that does only
> one thing. And you can do that with a simple shell script.
> 
>> its easy and simple to setup and use. So I think before we have some
> 
> so is configfs.
> 
>> sort of preset library of configfs-based gadget drivers, we still need
>> these modules.
> 
> there is already a library called libusbg.

By preset library I meant scripts or little programs that implement the
legacy drivers we have.

> 
>> Any suggestions on that?
>>
>> Do you want to support what I am proposing for gmidi.ko or just ignore
>> it and move on to configfs?
> 
> I prefer to not touch these gadget drivers if at all necessary. If you
> fixing a bug, then sure we must fix bugs. But you're not fixing a bug
> and, on top of that, you're adding regressions and violating the USB
> spec. This shows that you're writing these patches without knowing
> (and/or even caring about) the specification at all.

Yes, I see your point. My mistake was to not to enforce the first bit to
be set enabling the user to break the USB spec. I didn't think of that
scenario. And that's why it's always useful to have kernel maintainers
and others to provide such insights. :)

Anyway, I see that this patch is not useful even if corrected.

> 
> Here's some enlightening presentation you probably wanna watch:
> 
> https://www.youtube.com/watch?v=fMeH7wqOwXA
> 
> TL;DR : this project is large and you need to convince me we need your
> code/patch.
> 

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes

[Felipe Ferreri Tonello]

Hi Balbi,

On 08/03/16 07:43, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>>>>>>>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>>>>>>>  module_param(out_ports, uint, S_IRUGO);
>>>>>>>>  MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>>>>>>>  
>>>>>>>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>>>>>>>> +module_param(bmAttributes, uint, S_IRUGO);
>>>>>>>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>>>>>>> bmAttributes parameter");
>>>>>>>> +
>>>>>>>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>>>>>>>> +module_param(MaxPower, uint, S_IRUGO);
>>>>>>>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>>>>>>> Descriptor's bMaxPower parameter");
>>>>>>>
>>>>>>> you didn't run checkpatch, did you ? Also, are you sure you will need
>>>>>>> to
>>>>>>> change this by simply reloading the module ? I'm not convinced.
>>>>>>
>>>>>> Yes I always run checkpatch :)
>>>>>
>>>>> do you really ? Why do you have a 98-character line, then ?
> 
> btw, you didn't reply this ^^
> 
>>>>>>>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>>>>>>>.label  = "MIDI Gadget",
>>>>>>>>.bConfigurationValue = 1,
>>>>>>>>/* .iConfiguration = DYNAMIC */
>>>>>>>> -  .bmAttributes   = USB_CONFIG_ATT_ONE,
>>>>>>>
>>>>>>> nack, nackety nack nack nack :-)
>>>>>>>
>>>>>>> USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>>>>>>> give users the oportunity to violate USB spec.
>>>>>>
>>>>>> You are right. It needs to check the value before it assigns to
>>>>>> bmAttributes.
>>>>>
>>>>> why check ? You can just unconditionally or USB_CONFIG_ATT_ONE. In any
>>>>> case, I'm not convinced this is necessary at all.
>>>>
>>>> Right.
>>>>
>>>> This is necessary because this driver is actually wrong in which is
>>>> asking for the host to power itself. This is not specified on USB-MIDI
>>>> specification, neither makes any sense since this configuration is
>>>> device specific.
>>>>
>>>> What is your suggestion to make it configurable? Maybe at compile-time?
>>>> I really don't know what is the best solution if this is not something
>>>> you like it.
>>>
>>> well, you could use our configfs-based gadget interface. You don't
>>> really need to use gmidi.ko at all. In fact, we wanna do away with any
>>> static modules and rely only on configfs. If configfs doesn't let you
>>> change what you want/need, then we can talk about adding support for
>>> those.
>>>
>>> bMaxPower and bmAttributes sound like good things to have configurable
>>> over configfs but beware of what the USB specification says about them,
>>> we cannot let users violate the spec by passing bogus values on these
>>> fields.
>>
>> I agree that we should move to configfs, but the truth is that these
>> legacy devices are still useful. They just do one thing, mostly, but
> 
> yes, they are useful as they are. They don't need to be changed to be
> useful. Plus, you can have a gadget built with configfs that does only
> one thing. And you can do that with a simple shell script.
> 
>> its easy and simple to setup and use. So I think before we have some
> 
> so is configfs.
> 
>> sort of preset library of configfs-based gadget drivers, we still need
>> these modules.
> 
> there is already a library called libusbg.

By preset library I meant scripts or little programs that implement the
legacy drivers we have.

> 
>> Any suggestions on that?
>>
>> Do you want to support what I am proposing for gmidi.ko or just ignore
>> it and move on to configfs?
> 
> I prefer to not touch these gadget drivers if at all necessary. If you
> fixing a bug, then sure we must fix bugs. But you're not fixing a bug
> and, on top of that, you're adding regressions and violating the USB
> spec. This shows that you're writing these patches without knowing
> (and/or even caring about) the specification at all.

Yes, I see your point. My mistake was to not to enforce the first bit to
be set enabling the user to break the USB spec. I didn't think of that
scenario. And that's why it's always useful to have kernel maintainers
and others to provide such insights. :)

Anyway, I see that this patch is not useful even if corrected.

> 
> Here's some enlightening presentation you probably wanna watch:
> 
> https://www.youtube.com/watch?v=fMeH7wqOwXA
> 
> TL;DR : this project is large and you need to convince me we need your
> code/patch.
> 

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 2/5] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Ferreri Tonello]

Hi Balbi,

On 08/03/16 07:37, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>>>>>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>>>>> can
>>>>>> potentially cause a race condition between both calls. This is bad
>>>>> because the
>>>>>> way f_midi_transmit is implemented can't handle concurrent calls.
>>>>> This is due
>>>>>> to the fact that the usb request fifo looks for the next element and
>>>>> only if
>>>>>> it has data to process it enqueues the request, otherwise re-uses it.
>>>>> If both
>>>>>> (ALSA and USB) frameworks calls this function at the same time, the
>>>>>> kfifo_seek() will return the same usb_request, which will cause a
>>>>> race
>>>>>> condition.
>>>>>>
>>>>>> To solve this problem a syncronization mechanism is necessary. In
>>>>> this case it
>>>>>> is used a spinlock since f_midi_transmit is also called by
>>>>> usb_request->complete
>>>>>> callback in interrupt context.
>>>>>>
>>>>>> On benchmarks realized by me, spinlocks were more efficient then
>>>>> scheduling
>>>>>> the f_midi_transmit tasklet in process context and using a mutex
>>>>>> to synchronize. Also it performs better then previous
>>>>>> implementation
>>>>> that
>>>>>> allocated a usb_request for every new transmit made.
>>>>>
>>>>> behaves better in what way ? Also, previous implementation would not
>>>>> suffer from this concurrency problem, right ?
>>>>
>>>> The spin lock is faster than allocating usb requests all the time,
>>>> even if the udc uses da for it.
>>>
>>> did you measure ? Is the extra speed really necessary ? How did you
>>> benchmark this ?
>>
>> Yes I did measure and it was not that significant. This is not about
>> speed. There was a bug in that approach that I already explained on
> 
> you have very confusing statements. When I mentioned that previous code
> wouldn't have the need for the spinlock you replied that spinlock was
> faster.
> 
> When I asked you about benchmarks you reply saying it's not about the
> speed.
> 
> Make up your mind dude. What are you trying to achieve ?
> 
>> that patch, which was approved and applied BTW.
> 
> patches can be reverted if we realise we're better off without
> them. Don't get cocky, please.

Yes am I aware of that, but I honestly think that is the wrong way of
dealing with this.

?? I don't get why am I giving this impression.

> 
>> Any way, this spinlock should've been there since that patch but I
>> couldn't really trigger this problem without a stress test.
> 
> which tells me you sent me patches without properly testing. How much
> time did it take to trigger this ? How did you trigger this situation ?

No, that is no true. The implementation I sent is working properly for
any real world usage.

The stress test I made to break the current implementation is *not* a
real use-case. I made it in order to push as far as possible how fast
the driver can *reliably* handle while sending and reading data. Then I
noticed the bug.

So, to answer your question. To trigger this bug is not a matter of
time. The following needs to happen:
 1. Device send MIDI message that is *bigger* than the usb request
length. (just this by itself is really unlikely to happen in real world
usage)
 2. Host send a MIDI message back *exactly* at the same time as the
device is processing the second part of the usb request from the same
message.

I couldn't trigger this in all the tests we've made. I just triggered
when I was sending huge messages back and forth (device <-> host) as
mentioned.

In fact, we have thousands of devices out there without this patch (but
with my previous patch that introduced this bug).

I am not trying to say it wasn't a mistake. That patch unfortunately
introduces this bug, but it has real improvements over the previous
implementation. AFAIR the improvements are:
 * Fixes a bug that was causing the DMA buffer to fill it up causing a
kernel panic.
 * Pre allocate IN usb requests so there is no allocation overhead while
sending data (same behavior already existed for the OUT endpoint). This
ensure that the DMA memory is not misused affecting the rest of the system.
 * It doesn't crash if the host doesn't send an ACK after IN data
packets and we have reached the limit of available memory. Also, this is

Re: [PATCH 2/5] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Ferreri Tonello]

Hi Balbi,

On 08/03/16 07:37, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>>>>>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>>>>> can
>>>>>> potentially cause a race condition between both calls. This is bad
>>>>> because the
>>>>>> way f_midi_transmit is implemented can't handle concurrent calls.
>>>>> This is due
>>>>>> to the fact that the usb request fifo looks for the next element and
>>>>> only if
>>>>>> it has data to process it enqueues the request, otherwise re-uses it.
>>>>> If both
>>>>>> (ALSA and USB) frameworks calls this function at the same time, the
>>>>>> kfifo_seek() will return the same usb_request, which will cause a
>>>>> race
>>>>>> condition.
>>>>>>
>>>>>> To solve this problem a syncronization mechanism is necessary. In
>>>>> this case it
>>>>>> is used a spinlock since f_midi_transmit is also called by
>>>>> usb_request->complete
>>>>>> callback in interrupt context.
>>>>>>
>>>>>> On benchmarks realized by me, spinlocks were more efficient then
>>>>> scheduling
>>>>>> the f_midi_transmit tasklet in process context and using a mutex
>>>>>> to synchronize. Also it performs better then previous
>>>>>> implementation
>>>>> that
>>>>>> allocated a usb_request for every new transmit made.
>>>>>
>>>>> behaves better in what way ? Also, previous implementation would not
>>>>> suffer from this concurrency problem, right ?
>>>>
>>>> The spin lock is faster than allocating usb requests all the time,
>>>> even if the udc uses da for it.
>>>
>>> did you measure ? Is the extra speed really necessary ? How did you
>>> benchmark this ?
>>
>> Yes I did measure and it was not that significant. This is not about
>> speed. There was a bug in that approach that I already explained on
> 
> you have very confusing statements. When I mentioned that previous code
> wouldn't have the need for the spinlock you replied that spinlock was
> faster.
> 
> When I asked you about benchmarks you reply saying it's not about the
> speed.
> 
> Make up your mind dude. What are you trying to achieve ?
> 
>> that patch, which was approved and applied BTW.
> 
> patches can be reverted if we realise we're better off without
> them. Don't get cocky, please.

Yes am I aware of that, but I honestly think that is the wrong way of
dealing with this.

?? I don't get why am I giving this impression.

> 
>> Any way, this spinlock should've been there since that patch but I
>> couldn't really trigger this problem without a stress test.
> 
> which tells me you sent me patches without properly testing. How much
> time did it take to trigger this ? How did you trigger this situation ?

No, that is no true. The implementation I sent is working properly for
any real world usage.

The stress test I made to break the current implementation is *not* a
real use-case. I made it in order to push as far as possible how fast
the driver can *reliably* handle while sending and reading data. Then I
noticed the bug.

So, to answer your question. To trigger this bug is not a matter of
time. The following needs to happen:
 1. Device send MIDI message that is *bigger* than the usb request
length. (just this by itself is really unlikely to happen in real world
usage)
 2. Host send a MIDI message back *exactly* at the same time as the
device is processing the second part of the usb request from the same
message.

I couldn't trigger this in all the tests we've made. I just triggered
when I was sending huge messages back and forth (device <-> host) as
mentioned.

In fact, we have thousands of devices out there without this patch (but
with my previous patch that introduced this bug).

I am not trying to say it wasn't a mistake. That patch unfortunately
introduces this bug, but it has real improvements over the previous
implementation. AFAIR the improvements are:
 * Fixes a bug that was causing the DMA buffer to fill it up causing a
kernel panic.
 * Pre allocate IN usb requests so there is no allocation overhead while
sending data (same behavior already existed for the OUT endpoint). This
ensure that the DMA memory is not misused affecting the rest of the system.
 * It doesn't crash if the host doesn't send an ACK after IN data
packets and we have reached the limit of available memory. Also, this is
useful because it causes 

Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes

[Felipe Ferreri Tonello]

Hi Balbi, how are you?

On 07/03/16 10:59, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>>>>> "Felipe F. Tonello" <e...@felipetonello.com> writes:
>>>>>> [ text/plain ]
>>>>>> This gadget uses a bmAttributes and MaxPower that requires the USB
>>>>> bus to be
>>>>>> powered from the host, which is not correct because this
>>>>> configuration is device
>>>>>> specific, not a USB-MIDI requirement.
>>>>>>
>>>>>> This patch adds two modules parameters, bmAttributes and MaxPower,
>>>>> that allows
>>>>>> the user to set those flags. The default values are what the gadget
>>>>> used to use
>>>>>> for backward compatibility.
>>>>>>
>>>>>> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
>>>>>> ---
>>>>>>  drivers/usb/gadget/legacy/gmidi.c | 14 --
>>>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/gadget/legacy/gmidi.c
>>>>> b/drivers/usb/gadget/legacy/gmidi.c
>>>>>> index fc2ac150f5ff..0553435cc360 100644
>>>>>> --- a/drivers/usb/gadget/legacy/gmidi.c
>>>>>> +++ b/drivers/usb/gadget/legacy/gmidi.c
>>>>>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>>>>>  module_param(out_ports, uint, S_IRUGO);
>>>>>>  MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>>>>>  
>>>>>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>>>>>> +module_param(bmAttributes, uint, S_IRUGO);
>>>>>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>>>>> bmAttributes parameter");
>>>>>> +
>>>>>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>>>>>> +module_param(MaxPower, uint, S_IRUGO);
>>>>>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>>>>> Descriptor's bMaxPower parameter");
>>>>>
>>>>> you didn't run checkpatch, did you ? Also, are you sure you will need
>>>>> to
>>>>> change this by simply reloading the module ? I'm not convinced.
>>>>
>>>> Yes I always run checkpatch :)
>>>
>>> do you really ? Why do you have a 98-character line, then ?
>>>
>>>> What do you mean by reloading the module?
>>>
>>> modprobe g_midi MaxPower=4
>>> modprobe -r g_midi
>>> modprobe g_midi MaxPower=10
>>>
>>> I'm not convinced this is a valid use-case. Specially since you can just
>>> provide several configurations and let the host choose the one it suits
>>> it best.
>>
>> Ok, I will further test it.
>>
>>>
>>>>>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>>>>>  .label  = "MIDI Gadget",
>>>>>>  .bConfigurationValue = 1,
>>>>>>  /* .iConfiguration = DYNAMIC */
>>>>>> -.bmAttributes   = USB_CONFIG_ATT_ONE,
>>>>>
>>>>> nack, nackety nack nack nack :-)
>>>>>
>>>>> USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>>>>> give users the oportunity to violate USB spec.
>>>>
>>>> You are right. It needs to check the value before it assigns to
>>>> bmAttributes.
>>>
>>> why check ? You can just unconditionally or USB_CONFIG_ATT_ONE. In any
>>> case, I'm not convinced this is necessary at all.
>>
>> Right.
>>
>> This is necessary because this driver is actually wrong in which is
>> asking for the host to power itself. This is not specified on USB-MIDI
>> specification, neither makes any sense since this configuration is
>> device specific.
>>
>> What is your suggestion to make it configurable? Maybe at compile-time?
>> I really don't know what is the best solution if this is not something
>> you like it.
> 
> well, you could use our configfs-based gadget interface. You don't
> really need to use gmidi.ko at all. In fact, we wanna do away with any
> static modules and rely only on configfs. If configfs doesn't let you
> change what you want/need, then we can talk about adding support for
> those.
> 
> bMaxPower and bmAttributes sound like good things to have configurable
> over configfs but beware of what the USB specification says about them,
> we cannot let users violate the spec by passing bogus values on these
> fields.

I agree that we should move to configfs, but the truth is that these
legacy devices are still useful. They just do one thing, mostly, but its
easy and simple to setup and use. So I think before we have some sort of
preset library of configfs-based gadget drivers, we still need these
modules.

Any suggestions on that?

Do you want to support what I am proposing for gmidi.ko or just ignore
it and move on to configfs?

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes

[Felipe Ferreri Tonello]

Hi Balbi, how are you?

On 07/03/16 10:59, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>>>>> "Felipe F. Tonello"  writes:
>>>>>> [ text/plain ]
>>>>>> This gadget uses a bmAttributes and MaxPower that requires the USB
>>>>> bus to be
>>>>>> powered from the host, which is not correct because this
>>>>> configuration is device
>>>>>> specific, not a USB-MIDI requirement.
>>>>>>
>>>>>> This patch adds two modules parameters, bmAttributes and MaxPower,
>>>>> that allows
>>>>>> the user to set those flags. The default values are what the gadget
>>>>> used to use
>>>>>> for backward compatibility.
>>>>>>
>>>>>> Signed-off-by: Felipe F. Tonello 
>>>>>> ---
>>>>>>  drivers/usb/gadget/legacy/gmidi.c | 14 --
>>>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/gadget/legacy/gmidi.c
>>>>> b/drivers/usb/gadget/legacy/gmidi.c
>>>>>> index fc2ac150f5ff..0553435cc360 100644
>>>>>> --- a/drivers/usb/gadget/legacy/gmidi.c
>>>>>> +++ b/drivers/usb/gadget/legacy/gmidi.c
>>>>>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>>>>>  module_param(out_ports, uint, S_IRUGO);
>>>>>>  MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>>>>>  
>>>>>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>>>>>> +module_param(bmAttributes, uint, S_IRUGO);
>>>>>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>>>>> bmAttributes parameter");
>>>>>> +
>>>>>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>>>>>> +module_param(MaxPower, uint, S_IRUGO);
>>>>>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>>>>> Descriptor's bMaxPower parameter");
>>>>>
>>>>> you didn't run checkpatch, did you ? Also, are you sure you will need
>>>>> to
>>>>> change this by simply reloading the module ? I'm not convinced.
>>>>
>>>> Yes I always run checkpatch :)
>>>
>>> do you really ? Why do you have a 98-character line, then ?
>>>
>>>> What do you mean by reloading the module?
>>>
>>> modprobe g_midi MaxPower=4
>>> modprobe -r g_midi
>>> modprobe g_midi MaxPower=10
>>>
>>> I'm not convinced this is a valid use-case. Specially since you can just
>>> provide several configurations and let the host choose the one it suits
>>> it best.
>>
>> Ok, I will further test it.
>>
>>>
>>>>>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>>>>>  .label  = "MIDI Gadget",
>>>>>>  .bConfigurationValue = 1,
>>>>>>  /* .iConfiguration = DYNAMIC */
>>>>>> -.bmAttributes   = USB_CONFIG_ATT_ONE,
>>>>>
>>>>> nack, nackety nack nack nack :-)
>>>>>
>>>>> USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>>>>> give users the oportunity to violate USB spec.
>>>>
>>>> You are right. It needs to check the value before it assigns to
>>>> bmAttributes.
>>>
>>> why check ? You can just unconditionally or USB_CONFIG_ATT_ONE. In any
>>> case, I'm not convinced this is necessary at all.
>>
>> Right.
>>
>> This is necessary because this driver is actually wrong in which is
>> asking for the host to power itself. This is not specified on USB-MIDI
>> specification, neither makes any sense since this configuration is
>> device specific.
>>
>> What is your suggestion to make it configurable? Maybe at compile-time?
>> I really don't know what is the best solution if this is not something
>> you like it.
> 
> well, you could use our configfs-based gadget interface. You don't
> really need to use gmidi.ko at all. In fact, we wanna do away with any
> static modules and rely only on configfs. If configfs doesn't let you
> change what you want/need, then we can talk about adding support for
> those.
> 
> bMaxPower and bmAttributes sound like good things to have configurable
> over configfs but beware of what the USB specification says about them,
> we cannot let users violate the spec by passing bogus values on these
> fields.

I agree that we should move to configfs, but the truth is that these
legacy devices are still useful. They just do one thing, mostly, but its
easy and simple to setup and use. So I think before we have some sort of
preset library of configfs-based gadget drivers, we still need these
modules.

Any suggestions on that?

Do you want to support what I am proposing for gmidi.ko or just ignore
it and move on to configfs?

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes

[Felipe Ferreri Tonello]

Hi Balbi,

On 07/03/16 07:34, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>> [ text/plain ]
>> Hi Balbi, 
>>
>> On March 4, 2016 7:16:42 AM GMT+00:00, Felipe Balbi <ba...@kernel.org> wrote:
>>>
>>> Hi,
>>>
>>> "Felipe F. Tonello" <e...@felipetonello.com> writes:
>>>> [ text/plain ]
>>>> This gadget uses a bmAttributes and MaxPower that requires the USB
>>> bus to be
>>>> powered from the host, which is not correct because this
>>> configuration is device
>>>> specific, not a USB-MIDI requirement.
>>>>
>>>> This patch adds two modules parameters, bmAttributes and MaxPower,
>>> that allows
>>>> the user to set those flags. The default values are what the gadget
>>> used to use
>>>> for backward compatibility.
>>>>
>>>> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
>>>> ---
>>>>  drivers/usb/gadget/legacy/gmidi.c | 14 --
>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/legacy/gmidi.c
>>> b/drivers/usb/gadget/legacy/gmidi.c
>>>> index fc2ac150f5ff..0553435cc360 100644
>>>> --- a/drivers/usb/gadget/legacy/gmidi.c
>>>> +++ b/drivers/usb/gadget/legacy/gmidi.c
>>>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>>>  module_param(out_ports, uint, S_IRUGO);
>>>>  MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>>>  
>>>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>>>> +module_param(bmAttributes, uint, S_IRUGO);
>>>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>>> bmAttributes parameter");
>>>> +
>>>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>>>> +module_param(MaxPower, uint, S_IRUGO);
>>>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>>> Descriptor's bMaxPower parameter");
>>>
>>> you didn't run checkpatch, did you ? Also, are you sure you will need
>>> to
>>> change this by simply reloading the module ? I'm not convinced.
>>
>> Yes I always run checkpatch :)
> 
> do you really ? Why do you have a 98-character line, then ?
> 
>> What do you mean by reloading the module?
> 
> modprobe g_midi MaxPower=4
> modprobe -r g_midi
> modprobe g_midi MaxPower=10
> 
> I'm not convinced this is a valid use-case. Specially since you can just
> provide several configurations and let the host choose the one it suits
> it best.

Ok, I will further test it.

> 
>>>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>>>.label  = "MIDI Gadget",
>>>>.bConfigurationValue = 1,
>>>>/* .iConfiguration = DYNAMIC */
>>>> -  .bmAttributes   = USB_CONFIG_ATT_ONE,
>>>
>>> nack, nackety nack nack nack :-)
>>>
>>> USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>>> give users the oportunity to violate USB spec.
>>
>> You are right. It needs to check the value before it assigns to
>> bmAttributes.
> 
> why check ? You can just unconditionally or USB_CONFIG_ATT_ONE. In any
> case, I'm not convinced this is necessary at all.

Right.

This is necessary because this driver is actually wrong in which is
asking for the host to power itself. This is not specified on USB-MIDI
specification, neither makes any sense since this configuration is
device specific.

What is your suggestion to make it configurable? Maybe at compile-time?
I really don't know what is the best solution if this is not something
you like it.

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes

[Felipe Ferreri Tonello]

Hi Balbi,

On 07/03/16 07:34, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>> [ text/plain ]
>> Hi Balbi, 
>>
>> On March 4, 2016 7:16:42 AM GMT+00:00, Felipe Balbi  wrote:
>>>
>>> Hi,
>>>
>>> "Felipe F. Tonello"  writes:
>>>> [ text/plain ]
>>>> This gadget uses a bmAttributes and MaxPower that requires the USB
>>> bus to be
>>>> powered from the host, which is not correct because this
>>> configuration is device
>>>> specific, not a USB-MIDI requirement.
>>>>
>>>> This patch adds two modules parameters, bmAttributes and MaxPower,
>>> that allows
>>>> the user to set those flags. The default values are what the gadget
>>> used to use
>>>> for backward compatibility.
>>>>
>>>> Signed-off-by: Felipe F. Tonello 
>>>> ---
>>>>  drivers/usb/gadget/legacy/gmidi.c | 14 --
>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/legacy/gmidi.c
>>> b/drivers/usb/gadget/legacy/gmidi.c
>>>> index fc2ac150f5ff..0553435cc360 100644
>>>> --- a/drivers/usb/gadget/legacy/gmidi.c
>>>> +++ b/drivers/usb/gadget/legacy/gmidi.c
>>>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>>>  module_param(out_ports, uint, S_IRUGO);
>>>>  MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>>>  
>>>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>>>> +module_param(bmAttributes, uint, S_IRUGO);
>>>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>>> bmAttributes parameter");
>>>> +
>>>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>>>> +module_param(MaxPower, uint, S_IRUGO);
>>>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>>> Descriptor's bMaxPower parameter");
>>>
>>> you didn't run checkpatch, did you ? Also, are you sure you will need
>>> to
>>> change this by simply reloading the module ? I'm not convinced.
>>
>> Yes I always run checkpatch :)
> 
> do you really ? Why do you have a 98-character line, then ?
> 
>> What do you mean by reloading the module?
> 
> modprobe g_midi MaxPower=4
> modprobe -r g_midi
> modprobe g_midi MaxPower=10
> 
> I'm not convinced this is a valid use-case. Specially since you can just
> provide several configurations and let the host choose the one it suits
> it best.

Ok, I will further test it.

> 
>>>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>>>.label  = "MIDI Gadget",
>>>>.bConfigurationValue = 1,
>>>>/* .iConfiguration = DYNAMIC */
>>>> -  .bmAttributes   = USB_CONFIG_ATT_ONE,
>>>
>>> nack, nackety nack nack nack :-)
>>>
>>> USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>>> give users the oportunity to violate USB spec.
>>
>> You are right. It needs to check the value before it assigns to
>> bmAttributes.
> 
> why check ? You can just unconditionally or USB_CONFIG_ATT_ONE. In any
> case, I'm not convinced this is necessary at all.

Right.

This is necessary because this driver is actually wrong in which is
asking for the host to power itself. This is not specified on USB-MIDI
specification, neither makes any sense since this configuration is
device specific.

What is your suggestion to make it configurable? Maybe at compile-time?
I really don't know what is the best solution if this is not something
you like it.

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes

[Felipe Ferreri Tonello]

Hi Balbi,

On 07/03/16 07:35, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>> [ text/plain ]
>> Hi Michal, 
>>
>> On March 5, 2016 4:28:45 PM GMT+00:00, Michal Nazarewicz <min...@mina86.com> 
>> wrote:
>>>>> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>>>>>> @@ -16,7 +16,7 @@
>>>>>>   *   Copyright (C) 2006 Thumtronics Pty Ltd.
>>>>>>   *   Ben Williamson <ben.william...@greyinnovation.com>
>>>>>>   *
>>>>>> - * Licensed under the GPL-2 or later.
>>>>>> + * Licensed under the GPLv2.
>>>
>>>> On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz
>>> <min...@mina86.com> wrote:
>>>>> Any particular reason to do that?
>>>
>>> On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
>>>> Because the kernel is v2 only and not later. 
>>>
>>> Linux as a whole is GPLv2 only, but that doesn’t necessarily mean that
>>> parts of it cannot be dual licensed (or GPLv2+).  It’s safer to leave
>>> copyright noticed clear unless you explicitly want your contribution be
>>> GPLv2 only which brings the whole file GPLv2 only.
>>>
>>>> I just tried to make this driver more consistent with the coding
>>> style
>>>> used across the kernel. That's it.
>>>
>>> Column alignment of field names or RHS of assignment operators is quite
>>> inconsistent already within drivers/usb/gadget/ which is why I’m
>>> concerned whether this is really helping.
>>>
>>> Anyway, I actually don’t care much, just adding my two rappen.
>>
>> Right, I am ok with Balbi completely ignoring this patch. But I prefer
>> to have at least this driver consistent than nothing. Of course I'll
>> remove the license change I made.
> 
> consistent in what way ?

Source-code.

The goal of this patch is to update this driver coding style to promote
consistency, readability, and maintainability based on the Linux coding
style.

If this patch does not achieving that or if that is not necessary, than
just ignore this patch.

Thanks,
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes

[Felipe Ferreri Tonello]

Hi Balbi,

On 07/03/16 07:35, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>> [ text/plain ]
>> Hi Michal, 
>>
>> On March 5, 2016 4:28:45 PM GMT+00:00, Michal Nazarewicz  
>> wrote:
>>>>> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>>>>>> @@ -16,7 +16,7 @@
>>>>>>   *   Copyright (C) 2006 Thumtronics Pty Ltd.
>>>>>>   *   Ben Williamson 
>>>>>>   *
>>>>>> - * Licensed under the GPL-2 or later.
>>>>>> + * Licensed under the GPLv2.
>>>
>>>> On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz
>>>  wrote:
>>>>> Any particular reason to do that?
>>>
>>> On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
>>>> Because the kernel is v2 only and not later. 
>>>
>>> Linux as a whole is GPLv2 only, but that doesn’t necessarily mean that
>>> parts of it cannot be dual licensed (or GPLv2+).  It’s safer to leave
>>> copyright noticed clear unless you explicitly want your contribution be
>>> GPLv2 only which brings the whole file GPLv2 only.
>>>
>>>> I just tried to make this driver more consistent with the coding
>>> style
>>>> used across the kernel. That's it.
>>>
>>> Column alignment of field names or RHS of assignment operators is quite
>>> inconsistent already within drivers/usb/gadget/ which is why I’m
>>> concerned whether this is really helping.
>>>
>>> Anyway, I actually don’t care much, just adding my two rappen.
>>
>> Right, I am ok with Balbi completely ignoring this patch. But I prefer
>> to have at least this driver consistent than nothing. Of course I'll
>> remove the license change I made.
> 
> consistent in what way ?

Source-code.

The goal of this patch is to update this driver coding style to promote
consistency, readability, and maintainability based on the Linux coding
style.

If this patch does not achieving that or if that is not necessary, than
just ignore this patch.

Thanks,
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 2/5] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Ferreri Tonello]

Hi Balbi,

On 07/03/16 07:32, Felipe Balbi wrote:
> 
> Hi,
> 
> (please break your lines at 80-characters, have a look at
> Documentation/email-clients.txt if needed)
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>> [ text/plain ]
>> Hi Balbi, 
>>
>> On March 4, 2016 7:20:10 AM GMT+00:00, Felipe Balbi <ba...@kernel.org> wrote:
>>>
>>> Hi,
>>>
>>> "Felipe F. Tonello" <e...@felipetonello.com> writes:
>>>> [ text/plain ]
>>>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>>> can
>>>> potentially cause a race condition between both calls. This is bad
>>> because the
>>>> way f_midi_transmit is implemented can't handle concurrent calls.
>>> This is due
>>>> to the fact that the usb request fifo looks for the next element and
>>> only if
>>>> it has data to process it enqueues the request, otherwise re-uses it.
>>> If both
>>>> (ALSA and USB) frameworks calls this function at the same time, the
>>>> kfifo_seek() will return the same usb_request, which will cause a
>>> race
>>>> condition.
>>>>
>>>> To solve this problem a syncronization mechanism is necessary. In
>>> this case it
>>>> is used a spinlock since f_midi_transmit is also called by
>>> usb_request->complete
>>>> callback in interrupt context.
>>>>
>>>> On benchmarks realized by me, spinlocks were more efficient then
>>> scheduling
>>>> the f_midi_transmit tasklet in process context and using a mutex to
>>>> synchronize. Also it performs better then previous implementation
>>> that
>>>> allocated a usb_request for every new transmit made.
>>>
>>> behaves better in what way ? Also, previous implementation would not
>>> suffer from this concurrency problem, right ?
>>
>> The spin lock is faster than allocating usb requests all the time,
>> even if the udc uses da for it.
> 
> did you measure ? Is the extra speed really necessary ? How did you
> benchmark this ?

Yes I did measure and it was not that significant. This is not about
speed. There was a bug in that approach that I already explained on that
patch, which was approved and applied BTW.

Any way, this spinlock should've been there since that patch but I
couldn't really trigger this problem without a stress test.

So, this patch fixes a bug in the current implementation.

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 2/5] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Ferreri Tonello]

Hi Balbi,

On 07/03/16 07:32, Felipe Balbi wrote:
> 
> Hi,
> 
> (please break your lines at 80-characters, have a look at
> Documentation/email-clients.txt if needed)
> 
> Felipe Ferreri Tonello  writes:
>> [ text/plain ]
>> Hi Balbi, 
>>
>> On March 4, 2016 7:20:10 AM GMT+00:00, Felipe Balbi  wrote:
>>>
>>> Hi,
>>>
>>> "Felipe F. Tonello"  writes:
>>>> [ text/plain ]
>>>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>>> can
>>>> potentially cause a race condition between both calls. This is bad
>>> because the
>>>> way f_midi_transmit is implemented can't handle concurrent calls.
>>> This is due
>>>> to the fact that the usb request fifo looks for the next element and
>>> only if
>>>> it has data to process it enqueues the request, otherwise re-uses it.
>>> If both
>>>> (ALSA and USB) frameworks calls this function at the same time, the
>>>> kfifo_seek() will return the same usb_request, which will cause a
>>> race
>>>> condition.
>>>>
>>>> To solve this problem a syncronization mechanism is necessary. In
>>> this case it
>>>> is used a spinlock since f_midi_transmit is also called by
>>> usb_request->complete
>>>> callback in interrupt context.
>>>>
>>>> On benchmarks realized by me, spinlocks were more efficient then
>>> scheduling
>>>> the f_midi_transmit tasklet in process context and using a mutex to
>>>> synchronize. Also it performs better then previous implementation
>>> that
>>>> allocated a usb_request for every new transmit made.
>>>
>>> behaves better in what way ? Also, previous implementation would not
>>> suffer from this concurrency problem, right ?
>>
>> The spin lock is faster than allocating usb requests all the time,
>> even if the udc uses da for it.
> 
> did you measure ? Is the extra speed really necessary ? How did you
> benchmark this ?

Yes I did measure and it was not that significant. This is not about
speed. There was a bug in that approach that I already explained on that
patch, which was approved and applied BTW.

Any way, this spinlock should've been there since that patch but I
couldn't really trigger this problem without a stress test.

So, this patch fixes a bug in the current implementation.

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 5/5] usb: gadget: f_midi: updated copyright

[Felipe Ferreri Tonello]

Hi Balbi,

On 07/03/16 07:36, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>> [ text/plain ]
>> Hi Balbi, 
>>
>> On March 4, 2016 7:13:05 AM GMT+00:00, Felipe Balbi <ba...@kernel.org> wrote:
>>> "Felipe F. Tonello" <e...@felipetonello.com> writes:
>>>> [ text/plain ]
>>>> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
>>>
>>> no commit log == no commit
>>
>> Got it. 
>>
>>>
>>>> ---
>>>>  drivers/usb/gadget/function/f_midi.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_midi.c
>>> b/drivers/usb/gadget/function/f_midi.c
>>>> index 9a9e6112e224..5c7f5c780fda 100644
>>>> --- a/drivers/usb/gadget/function/f_midi.c
>>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>>> @@ -5,6 +5,9 @@
>>>>   * Developed for Thumtronics by Grey Innovation
>>>>   * Ben Williamson <ben.william...@greyinnovation.com>
>>>>   *
>>>> + * Copyright (C) 2015,2016 ROLI Ltd.
>>>> + * Felipe F. Tonello <felipe.tone...@roli.com>
>>>
>>> Did you check with your company's lawyer that your changes are enough
>>> to
>>> justify a copyright ?
>>
>> Yes. Specially if that state machine refractor gets approved. TBH I
>> can't see it won't.
> 
> okay, so did that same lawyer tell you to change the driver's license ?
> 

No. That was my bad call. TBH I really don't care about this copyright.
You can just ignore this patch and patch 4.

Thanks

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 5/5] usb: gadget: f_midi: updated copyright

[Felipe Ferreri Tonello]

Hi Balbi,

On 07/03/16 07:36, Felipe Balbi wrote:
> 
> Hi,
> 
> Felipe Ferreri Tonello  writes:
>> [ text/plain ]
>> Hi Balbi, 
>>
>> On March 4, 2016 7:13:05 AM GMT+00:00, Felipe Balbi  wrote:
>>> "Felipe F. Tonello"  writes:
>>>> [ text/plain ]
>>>> Signed-off-by: Felipe F. Tonello 
>>>
>>> no commit log == no commit
>>
>> Got it. 
>>
>>>
>>>> ---
>>>>  drivers/usb/gadget/function/f_midi.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_midi.c
>>> b/drivers/usb/gadget/function/f_midi.c
>>>> index 9a9e6112e224..5c7f5c780fda 100644
>>>> --- a/drivers/usb/gadget/function/f_midi.c
>>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>>> @@ -5,6 +5,9 @@
>>>>   * Developed for Thumtronics by Grey Innovation
>>>>   * Ben Williamson 
>>>>   *
>>>> + * Copyright (C) 2015,2016 ROLI Ltd.
>>>> + * Felipe F. Tonello 
>>>
>>> Did you check with your company's lawyer that your changes are enough
>>> to
>>> justify a copyright ?
>>
>> Yes. Specially if that state machine refractor gets approved. TBH I
>> can't see it won't.
> 
> okay, so did that same lawyer tell you to change the driver's license ?
> 

No. That was my bad call. TBH I really don't care about this copyright.
You can just ignore this patch and patch 4.

Thanks

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes

[Felipe Ferreri Tonello]

Hi Michal, 

On March 5, 2016 4:28:45 PM GMT+00:00, Michal Nazarewicz <min...@mina86.com> 
wrote:
>>> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>>>> @@ -16,7 +16,7 @@
>>>>   *   Copyright (C) 2006 Thumtronics Pty Ltd.
>>>>   *   Ben Williamson <ben.william...@greyinnovation.com>
>>>>   *
>>>> - * Licensed under the GPL-2 or later.
>>>> + * Licensed under the GPLv2.
>
>> On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz
><min...@mina86.com> wrote:
>>> Any particular reason to do that?
>
>On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
>> Because the kernel is v2 only and not later. 
>
>Linux as a whole is GPLv2 only, but that doesn’t necessarily mean that
>parts of it cannot be dual licensed (or GPLv2+).  It’s safer to leave
>copyright noticed clear unless you explicitly want your contribution be
>GPLv2 only which brings the whole file GPLv2 only.
>
>> I just tried to make this driver more consistent with the coding
>style
>> used across the kernel. That's it.
>
>Column alignment of field names or RHS of assignment operators is quite
>inconsistent already within drivers/usb/gadget/ which is why I’m
>concerned whether this is really helping.
>
>Anyway, I actually don’t care much, just adding my two rappen.

Right, I am ok with Balbi completely ignoring this patch. But I prefer to have 
at least this driver consistent than nothing. Of course I'll remove the license 
change I made. 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes

[Felipe Ferreri Tonello]

Hi Michal, 

On March 5, 2016 4:28:45 PM GMT+00:00, Michal Nazarewicz  
wrote:
>>> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>>>> @@ -16,7 +16,7 @@
>>>>   *   Copyright (C) 2006 Thumtronics Pty Ltd.
>>>>   *   Ben Williamson 
>>>>   *
>>>> - * Licensed under the GPL-2 or later.
>>>> + * Licensed under the GPLv2.
>
>> On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz
> wrote:
>>> Any particular reason to do that?
>
>On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
>> Because the kernel is v2 only and not later. 
>
>Linux as a whole is GPLv2 only, but that doesn’t necessarily mean that
>parts of it cannot be dual licensed (or GPLv2+).  It’s safer to leave
>copyright noticed clear unless you explicitly want your contribution be
>GPLv2 only which brings the whole file GPLv2 only.
>
>> I just tried to make this driver more consistent with the coding
>style
>> used across the kernel. That's it.
>
>Column alignment of field names or RHS of assignment operators is quite
>inconsistent already within drivers/usb/gadget/ which is why I’m
>concerned whether this is really helping.
>
>Anyway, I actually don’t care much, just adding my two rappen.

Right, I am ok with Balbi completely ignoring this patch. But I prefer to have 
at least this driver consistent than nothing. Of course I'll remove the license 
change I made. 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes

[Felipe Ferreri Tonello]

Hi Greg, 

On March 5, 2016 7:39:13 PM GMT+00:00, Greg KH <g...@kroah.com> wrote:
>On Sat, Mar 05, 2016 at 11:28:45AM -0500, Michal Nazarewicz wrote:
>> >> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>> >>> @@ -16,7 +16,7 @@
>> >>>   *   Copyright (C) 2006 Thumtronics Pty Ltd.
>> >>>   *   Ben Williamson <ben.william...@greyinnovation.com>
>> >>>   *
>> >>> - * Licensed under the GPL-2 or later.
>> >>> + * Licensed under the GPLv2.
>> 
>> > On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz
><min...@mina86.com> wrote:
>> >> Any particular reason to do that?
>> 
>> On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
>> > Because the kernel is v2 only and not later. 
>> 
>> Linux as a whole is GPLv2 only, but that doesn’t necessarily mean
>that
>> parts of it cannot be dual licensed (or GPLv2+).  It’s safer to leave
>> copyright noticed clear unless you explicitly want your contribution
>be
>> GPLv2 only which brings the whole file GPLv2 only.
>
>But you can't change the license of someone else's code, which is what
>is happening here.  Felipe T, you can't do that at all unless you want
>to get into big trouble, please consult a lawyer for all of the gory
>details.

Thanks for letting me know. TBH, I had no idea about it. 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes

[Felipe Ferreri Tonello]

Hi Greg, 

On March 5, 2016 7:39:13 PM GMT+00:00, Greg KH  wrote:
>On Sat, Mar 05, 2016 at 11:28:45AM -0500, Michal Nazarewicz wrote:
>> >> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>> >>> @@ -16,7 +16,7 @@
>> >>>   *   Copyright (C) 2006 Thumtronics Pty Ltd.
>> >>>   *   Ben Williamson 
>> >>>   *
>> >>> - * Licensed under the GPL-2 or later.
>> >>> + * Licensed under the GPLv2.
>> 
>> > On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz
> wrote:
>> >> Any particular reason to do that?
>> 
>> On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
>> > Because the kernel is v2 only and not later. 
>> 
>> Linux as a whole is GPLv2 only, but that doesn’t necessarily mean
>that
>> parts of it cannot be dual licensed (or GPLv2+).  It’s safer to leave
>> copyright noticed clear unless you explicitly want your contribution
>be
>> GPLv2 only which brings the whole file GPLv2 only.
>
>But you can't change the license of someone else's code, which is what
>is happening here.  Felipe T, you can't do that at all unless you want
>to get into big trouble, please consult a lawyer for all of the gory
>details.

Thanks for letting me know. TBH, I had no idea about it. 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes

[Felipe Ferreri Tonello]

Hi Michal, 

On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz  
wrote:
>On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 77
>+++-
>>  1 file changed, 40 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>b/drivers/usb/gadget/function/f_midi.c
>> index 8475e3dc82d4..9a9e6112e224 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -1,5 +1,5 @@
>>  /*
>> - * f_midi.c -- USB MIDI class function driver
>> + * f_midi.c -- USB-MIDI class function driver
>>   *
>>   * Copyright (C) 2006 Thumtronics Pty Ltd.
>>   * Developed for Thumtronics by Grey Innovation
>> @@ -16,7 +16,7 @@
>>   *   Copyright (C) 2006 Thumtronics Pty Ltd.
>>   *   Ben Williamson 
>>   *
>> - * Licensed under the GPL-2 or later.
>> + * Licensed under the GPLv2.
>
>Any particular reason to do that?

Because the kernel is v2 only and not later. 

>
>>   */
>>  
>>  #include 
>> @@ -41,8 +41,8 @@
>>  MODULE_AUTHOR("Ben Williamson");
>>  MODULE_LICENSE("GPL v2");
>>  
>> -static const char f_midi_shortname[] = "f_midi";
>> -static const char f_midi_longname[] = "MIDI Gadget";
>> +static const char f_midi_shortname[] =  "f_midi";
>> +static const char f_midi_longname[] =   "MIDI Gadget";
>>  
>>  /*
>>   * We can only handle 16 cables on one single endpoint, as cable
>numbers are
>> @@ -78,28 +78,31 @@ struct gmidi_in_port {
>>  };
>>  
>>  struct f_midi {
>> -struct usb_function func;
>> -struct usb_gadget   *gadget;
>> -struct usb_ep   *in_ep, *out_ep;
>> -struct snd_card *card;
>> -struct snd_rawmidi  *rmidi;
>> -u8  ms_id;
>> -
>> -struct snd_rawmidi_substream *out_substream[MAX_PORTS];
>> -
>> -unsigned long   out_triggered;
>> -struct tasklet_struct   tasklet;
>> +struct usb_function func;
>> +struct usb_gadget *gadget;
>> +struct usb_ep *in_ep, *out_ep;
>> +u8 ms_id;
>> +unsigned long out_triggered;
>>  unsigned int in_ports;
>>  unsigned int out_ports;
>> -int index;
>> -char *id;
>> -unsigned int buflen, qlen;
>> +unsigned int buflen;
>> +unsigned int qlen;
>> +unsigned int len;
>> +
>>  /* This fifo is used as a buffer ring for pre-allocated IN
>usb_requests */
>>  DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
>>  spinlock_t transmit_lock;
>> +
>> +/* ALSA stuff */
>> +struct snd_card *card;
>> +struct snd_rawmidi *rmidi;
>> +struct snd_rawmidi_substream *out_substream[MAX_PORTS];
>> +struct tasklet_struct tasklet;
>>  unsigned int in_last_port;
>> +int index;
>> +char *id;
>>  
>> -struct gmidi_in_portin_ports_array[/* in_ports */];
>> +struct gmidi_in_port in_ports_array[/* in_ports */];
>>  };
>>  
>>  static inline struct f_midi *func_to_midi(struct usb_function *f)
>> @@ -191,7 +194,7 @@ static struct usb_ms_endpoint_descriptor_16
>ms_in_desc = {
>>  
>>  /* string IDs are assigned dynamically */
>>  
>> -#define STRING_FUNC_IDX 0
>> +#define STRING_FUNC_IDX 0
>>  
>>  static struct usb_string midi_string_defs[] = {
>>  [STRING_FUNC_IDX].s = "MIDI function",
>> @@ -199,7 +202,7 @@ static struct usb_string midi_string_defs[] = {
>>  };
>>  
>>  static struct usb_gadget_strings midi_stringtab = {
>> -.language   = 0x0409,   /* en-us */
>> +.language   = 0x0409, /* en-us */
>>  .strings= midi_string_defs,
>>  };
>>  
>> @@ -409,7 +412,7 @@ static int f_midi_snd_free(struct snd_device
>*device)
>>  }
>>  
>>  /*
>> - * Converts MIDI commands to USB MIDI packets.
>> + * Converts MIDI commands to USB-MIDI packets.
>>   */
>>  static void f_midi_transmit_byte(struct usb_request *req,
>>   struct gmidi_in_port *port, uint8_t b)
>> @@ -956,15 +959,15 @@ static int f_midi_bind(struct usb_configuration
>*c, struct usb_function *f)
>>  in_emb->iJack   = 0;
>>  midi_function[i++] = (struct usb_descriptor_header *) in_emb;
>>  
>> -out_ext->bLength =  USB_DT_MIDI_OUT_SIZE(1);
>> -out_ext->bDescriptorType =  USB_DT_CS_INTERFACE;
>> -out_ext->bDescriptorSubtype =   USB_MS_MIDI_OUT_JACK;
>> -out_ext->bJackType =USB_MS_EXTERNAL;
>> -out_ext->bJackID =  jack++;
>> -out_ext->bNrInputPins = 1;
>> -out_ext->iJack =0;
>> -out_ext->pins[0].baSourceID =   in_emb->bJackID;
>> -out_ext->pins[0].baSourcePin =  1;
>> +out_ext->bLength= USB_DT_MIDI_OUT_SIZE(1);
>> +out_ext->bDescriptorType= USB_DT_CS_INTERFACE;
>> +

Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes

[Felipe Ferreri Tonello]

Hi Michal, 

On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz  
wrote:
>On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 77
>+++-
>>  1 file changed, 40 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>b/drivers/usb/gadget/function/f_midi.c
>> index 8475e3dc82d4..9a9e6112e224 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -1,5 +1,5 @@
>>  /*
>> - * f_midi.c -- USB MIDI class function driver
>> + * f_midi.c -- USB-MIDI class function driver
>>   *
>>   * Copyright (C) 2006 Thumtronics Pty Ltd.
>>   * Developed for Thumtronics by Grey Innovation
>> @@ -16,7 +16,7 @@
>>   *   Copyright (C) 2006 Thumtronics Pty Ltd.
>>   *   Ben Williamson 
>>   *
>> - * Licensed under the GPL-2 or later.
>> + * Licensed under the GPLv2.
>
>Any particular reason to do that?

Because the kernel is v2 only and not later. 

>
>>   */
>>  
>>  #include 
>> @@ -41,8 +41,8 @@
>>  MODULE_AUTHOR("Ben Williamson");
>>  MODULE_LICENSE("GPL v2");
>>  
>> -static const char f_midi_shortname[] = "f_midi";
>> -static const char f_midi_longname[] = "MIDI Gadget";
>> +static const char f_midi_shortname[] =  "f_midi";
>> +static const char f_midi_longname[] =   "MIDI Gadget";
>>  
>>  /*
>>   * We can only handle 16 cables on one single endpoint, as cable
>numbers are
>> @@ -78,28 +78,31 @@ struct gmidi_in_port {
>>  };
>>  
>>  struct f_midi {
>> -struct usb_function func;
>> -struct usb_gadget   *gadget;
>> -struct usb_ep   *in_ep, *out_ep;
>> -struct snd_card *card;
>> -struct snd_rawmidi  *rmidi;
>> -u8  ms_id;
>> -
>> -struct snd_rawmidi_substream *out_substream[MAX_PORTS];
>> -
>> -unsigned long   out_triggered;
>> -struct tasklet_struct   tasklet;
>> +struct usb_function func;
>> +struct usb_gadget *gadget;
>> +struct usb_ep *in_ep, *out_ep;
>> +u8 ms_id;
>> +unsigned long out_triggered;
>>  unsigned int in_ports;
>>  unsigned int out_ports;
>> -int index;
>> -char *id;
>> -unsigned int buflen, qlen;
>> +unsigned int buflen;
>> +unsigned int qlen;
>> +unsigned int len;
>> +
>>  /* This fifo is used as a buffer ring for pre-allocated IN
>usb_requests */
>>  DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
>>  spinlock_t transmit_lock;
>> +
>> +/* ALSA stuff */
>> +struct snd_card *card;
>> +struct snd_rawmidi *rmidi;
>> +struct snd_rawmidi_substream *out_substream[MAX_PORTS];
>> +struct tasklet_struct tasklet;
>>  unsigned int in_last_port;
>> +int index;
>> +char *id;
>>  
>> -struct gmidi_in_portin_ports_array[/* in_ports */];
>> +struct gmidi_in_port in_ports_array[/* in_ports */];
>>  };
>>  
>>  static inline struct f_midi *func_to_midi(struct usb_function *f)
>> @@ -191,7 +194,7 @@ static struct usb_ms_endpoint_descriptor_16
>ms_in_desc = {
>>  
>>  /* string IDs are assigned dynamically */
>>  
>> -#define STRING_FUNC_IDX 0
>> +#define STRING_FUNC_IDX 0
>>  
>>  static struct usb_string midi_string_defs[] = {
>>  [STRING_FUNC_IDX].s = "MIDI function",
>> @@ -199,7 +202,7 @@ static struct usb_string midi_string_defs[] = {
>>  };
>>  
>>  static struct usb_gadget_strings midi_stringtab = {
>> -.language   = 0x0409,   /* en-us */
>> +.language   = 0x0409, /* en-us */
>>  .strings= midi_string_defs,
>>  };
>>  
>> @@ -409,7 +412,7 @@ static int f_midi_snd_free(struct snd_device
>*device)
>>  }
>>  
>>  /*
>> - * Converts MIDI commands to USB MIDI packets.
>> + * Converts MIDI commands to USB-MIDI packets.
>>   */
>>  static void f_midi_transmit_byte(struct usb_request *req,
>>   struct gmidi_in_port *port, uint8_t b)
>> @@ -956,15 +959,15 @@ static int f_midi_bind(struct usb_configuration
>*c, struct usb_function *f)
>>  in_emb->iJack   = 0;
>>  midi_function[i++] = (struct usb_descriptor_header *) in_emb;
>>  
>> -out_ext->bLength =  USB_DT_MIDI_OUT_SIZE(1);
>> -out_ext->bDescriptorType =  USB_DT_CS_INTERFACE;
>> -out_ext->bDescriptorSubtype =   USB_MS_MIDI_OUT_JACK;
>> -out_ext->bJackType =USB_MS_EXTERNAL;
>> -out_ext->bJackID =  jack++;
>> -out_ext->bNrInputPins = 1;
>> -out_ext->iJack =0;
>> -out_ext->pins[0].baSourceID =   in_emb->bJackID;
>> -out_ext->pins[0].baSourcePin =  1;
>> +out_ext->bLength= USB_DT_MIDI_OUT_SIZE(1);
>> +out_ext->bDescriptorType= USB_DT_CS_INTERFACE;
>> +out_ext->bDescriptorSubtype = USB_MS_MIDI_OUT_JACK;
>> +

Re: [PATCH 2/5] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Ferreri Tonello]

Hi Balbi, 

On March 4, 2016 7:20:10 AM GMT+00:00, Felipe Balbi  wrote:
>
>Hi,
>
>"Felipe F. Tonello"  writes:
>> [ text/plain ]
>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>can
>> potentially cause a race condition between both calls. This is bad
>because the
>> way f_midi_transmit is implemented can't handle concurrent calls.
>This is due
>> to the fact that the usb request fifo looks for the next element and
>only if
>> it has data to process it enqueues the request, otherwise re-uses it.
>If both
>> (ALSA and USB) frameworks calls this function at the same time, the
>> kfifo_seek() will return the same usb_request, which will cause a
>race
>> condition.
>>
>> To solve this problem a syncronization mechanism is necessary. In
>this case it
>> is used a spinlock since f_midi_transmit is also called by
>usb_request->complete
>> callback in interrupt context.
>>
>> On benchmarks realized by me, spinlocks were more efficient then
>scheduling
>> the f_midi_transmit tasklet in process context and using a mutex to
>> synchronize. Also it performs better then previous implementation
>that
>> allocated a usb_request for every new transmit made.
>
>behaves better in what way ? Also, previous implementation would not
>suffer from this concurrency problem, right ?

The spin lock is faster than allocating usb requests all the time, even if the 
udc uses da for it. 

That's true it wasn't necessary to put a spin lock in the gadget driver because 
the udc driver does it when allocating a new request. 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 2/5] usb: gadget: f_midi: added spinlock on transmit function

[Felipe Ferreri Tonello]

Hi Balbi, 

On March 4, 2016 7:20:10 AM GMT+00:00, Felipe Balbi  wrote:
>
>Hi,
>
>"Felipe F. Tonello"  writes:
>> [ text/plain ]
>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>can
>> potentially cause a race condition between both calls. This is bad
>because the
>> way f_midi_transmit is implemented can't handle concurrent calls.
>This is due
>> to the fact that the usb request fifo looks for the next element and
>only if
>> it has data to process it enqueues the request, otherwise re-uses it.
>If both
>> (ALSA and USB) frameworks calls this function at the same time, the
>> kfifo_seek() will return the same usb_request, which will cause a
>race
>> condition.
>>
>> To solve this problem a syncronization mechanism is necessary. In
>this case it
>> is used a spinlock since f_midi_transmit is also called by
>usb_request->complete
>> callback in interrupt context.
>>
>> On benchmarks realized by me, spinlocks were more efficient then
>scheduling
>> the f_midi_transmit tasklet in process context and using a mutex to
>> synchronize. Also it performs better then previous implementation
>that
>> allocated a usb_request for every new transmit made.
>
>behaves better in what way ? Also, previous implementation would not
>suffer from this concurrency problem, right ?

The spin lock is faster than allocating usb requests all the time, even if the 
udc uses da for it. 

That's true it wasn't necessary to put a spin lock in the gadget driver because 
the udc driver does it when allocating a new request. 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes

[Felipe Ferreri Tonello]

Hi Balbi, 

On March 4, 2016 7:16:42 AM GMT+00:00, Felipe Balbi  wrote:
>
>Hi,
>
>"Felipe F. Tonello"  writes:
>> [ text/plain ]
>> This gadget uses a bmAttributes and MaxPower that requires the USB
>bus to be
>> powered from the host, which is not correct because this
>configuration is device
>> specific, not a USB-MIDI requirement.
>>
>> This patch adds two modules parameters, bmAttributes and MaxPower,
>that allows
>> the user to set those flags. The default values are what the gadget
>used to use
>> for backward compatibility.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/legacy/gmidi.c | 14 --
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/legacy/gmidi.c
>b/drivers/usb/gadget/legacy/gmidi.c
>> index fc2ac150f5ff..0553435cc360 100644
>> --- a/drivers/usb/gadget/legacy/gmidi.c
>> +++ b/drivers/usb/gadget/legacy/gmidi.c
>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>  module_param(out_ports, uint, S_IRUGO);
>>  MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>  
>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>> +module_param(bmAttributes, uint, S_IRUGO);
>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>bmAttributes parameter");
>> +
>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>> +module_param(MaxPower, uint, S_IRUGO);
>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>Descriptor's bMaxPower parameter");
>
>you didn't run checkpatch, did you ? Also, are you sure you will need
>to
>change this by simply reloading the module ? I'm not convinced.

Yes I always run checkpatch :) 

What do you mean by reloading the module? 

>
>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>  .label  = "MIDI Gadget",
>>  .bConfigurationValue = 1,
>>  /* .iConfiguration = DYNAMIC */
>> -.bmAttributes   = USB_CONFIG_ATT_ONE,
>
>nack, nackety nack nack nack :-)
>
>USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>give users the oportunity to violate USB spec.

You are right. It needs to check the value before it assigns to bmAttributes. 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes

[Felipe Ferreri Tonello]

Hi Balbi, 

On March 4, 2016 7:16:42 AM GMT+00:00, Felipe Balbi  wrote:
>
>Hi,
>
>"Felipe F. Tonello"  writes:
>> [ text/plain ]
>> This gadget uses a bmAttributes and MaxPower that requires the USB
>bus to be
>> powered from the host, which is not correct because this
>configuration is device
>> specific, not a USB-MIDI requirement.
>>
>> This patch adds two modules parameters, bmAttributes and MaxPower,
>that allows
>> the user to set those flags. The default values are what the gadget
>used to use
>> for backward compatibility.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/legacy/gmidi.c | 14 --
>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/legacy/gmidi.c
>b/drivers/usb/gadget/legacy/gmidi.c
>> index fc2ac150f5ff..0553435cc360 100644
>> --- a/drivers/usb/gadget/legacy/gmidi.c
>> +++ b/drivers/usb/gadget/legacy/gmidi.c
>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>  module_param(out_ports, uint, S_IRUGO);
>>  MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>  
>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>> +module_param(bmAttributes, uint, S_IRUGO);
>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>bmAttributes parameter");
>> +
>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>> +module_param(MaxPower, uint, S_IRUGO);
>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>Descriptor's bMaxPower parameter");
>
>you didn't run checkpatch, did you ? Also, are you sure you will need
>to
>change this by simply reloading the module ? I'm not convinced.

Yes I always run checkpatch :) 

What do you mean by reloading the module? 

>
>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>  .label  = "MIDI Gadget",
>>  .bConfigurationValue = 1,
>>  /* .iConfiguration = DYNAMIC */
>> -.bmAttributes   = USB_CONFIG_ATT_ONE,
>
>nack, nackety nack nack nack :-)
>
>USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>give users the oportunity to violate USB spec.

You are right. It needs to check the value before it assigns to bmAttributes. 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 0/5] MIDI USB Gadget improvements

[Felipe Ferreri Tonello]

Hi Balbi, 

On March 4, 2016 7:11:30 AM GMT+00:00, Felipe Balbi  wrote:
>
>Hi,
>
>"Felipe F. Tonello"  writes:
>> [ text/plain ]
>> Patches are pretty much self-described.
>>
>> Patch 1 is revised from comments.
>
>you really need to describe what you changed. This also should have v2
>on subject line.

Right. I didn't in this case because I sent this patch previously a while ago 
right before you changed employer. 

>
>I guess it's too late to get this in v4.6 merge window as I'm already
>applying the last few patches and plan to send a pull request in a few
>minutes.

That's fine I won't be able to rework the comments before Monday anyway. 

Thanks, 
Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 0/5] MIDI USB Gadget improvements

[Felipe Ferreri Tonello]

Hi Balbi, 

On March 4, 2016 7:11:30 AM GMT+00:00, Felipe Balbi  wrote:
>
>Hi,
>
>"Felipe F. Tonello"  writes:
>> [ text/plain ]
>> Patches are pretty much self-described.
>>
>> Patch 1 is revised from comments.
>
>you really need to describe what you changed. This also should have v2
>on subject line.

Right. I didn't in this case because I sent this patch previously a while ago 
right before you changed employer. 

>
>I guess it's too late to get this in v4.6 merge window as I'm already
>applying the last few patches and plan to send a pull request in a few
>minutes.

That's fine I won't be able to rework the comments before Monday anyway. 

Thanks, 
Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 5/5] usb: gadget: f_midi: updated copyright

[Felipe Ferreri Tonello]

Hi Balbi, 

On March 4, 2016 7:13:05 AM GMT+00:00, Felipe Balbi  wrote:
>"Felipe F. Tonello"  writes:
>> [ text/plain ]
>> Signed-off-by: Felipe F. Tonello 
>
>no commit log == no commit

Got it. 

>
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>b/drivers/usb/gadget/function/f_midi.c
>> index 9a9e6112e224..5c7f5c780fda 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -5,6 +5,9 @@
>>   * Developed for Thumtronics by Grey Innovation
>>   * Ben Williamson 
>>   *
>> + * Copyright (C) 2015,2016 ROLI Ltd.
>> + * Felipe F. Tonello 
>
>Did you check with your company's lawyer that your changes are enough
>to
>justify a copyright ?

Yes. Specially if that state machine refractor gets approved. TBH I can't see 
it won't. 

Thanks, 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 5/5] usb: gadget: f_midi: updated copyright

[Felipe Ferreri Tonello]

Hi Balbi, 

On March 4, 2016 7:13:05 AM GMT+00:00, Felipe Balbi  wrote:
>"Felipe F. Tonello"  writes:
>> [ text/plain ]
>> Signed-off-by: Felipe F. Tonello 
>
>no commit log == no commit

Got it. 

>
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>b/drivers/usb/gadget/function/f_midi.c
>> index 9a9e6112e224..5c7f5c780fda 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -5,6 +5,9 @@
>>   * Developed for Thumtronics by Grey Innovation
>>   * Ben Williamson 
>>   *
>> + * Copyright (C) 2015,2016 ROLI Ltd.
>> + * Felipe F. Tonello 
>
>Did you check with your company's lawyer that your changes are enough
>to
>justify a copyright ?

Yes. Specially if that state machine refractor gets approved. TBH I can't see 
it won't. 

Thanks, 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens, 

On March 4, 2016 8:07:40 AM GMT+00:00, Clemens Ladisch <clem...@ladisch.de> 
wrote:
>Felipe Ferreri Tonello wrote:
>> On 03/03/16 11:38, Clemens Ladisch wrote:
>>> But in what way was the old state machine not "proper"?
>>
>> Because it didn't reflect all the correct and possible MIDI states
>
>The whole point of the one-byte real-time messages is that they do not
>affect the parsing of the surrounding MIDI stream.  So not making them
>part of the state machine is the proper way of handling them.  (Also
>see the flowchart in appendix A of the spec.)

I really don't get your point. So why do we have a state machine at all? 

>
>> This patch doesn't change any functionality. But the important thing
>> here is that it improves the driver maintainability [...]
>
>Then I won't get in the way of this driver's maintainer.


Clemens, I really value your feedback. I just want to understand what's the 
problem of this patch. 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens, 

On March 4, 2016 8:07:40 AM GMT+00:00, Clemens Ladisch  
wrote:
>Felipe Ferreri Tonello wrote:
>> On 03/03/16 11:38, Clemens Ladisch wrote:
>>> But in what way was the old state machine not "proper"?
>>
>> Because it didn't reflect all the correct and possible MIDI states
>
>The whole point of the one-byte real-time messages is that they do not
>affect the parsing of the surrounding MIDI stream.  So not making them
>part of the state machine is the proper way of handling them.  (Also
>see the flowchart in appendix A of the spec.)

I really don't get your point. So why do we have a state machine at all? 

>
>> This patch doesn't change any functionality. But the important thing
>> here is that it improves the driver maintainability [...]
>
>Then I won't get in the way of this driver's maintainer.


Clemens, I really value your feedback. I just want to understand what's the 
problem of this patch. 

Felipe 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens,

On 03/03/16 11:38, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 02/03/16 21:09, Clemens Ladisch wrote:
>>> Felipe F. Tonello wrote:
>>>> This refactor results in a cleaner state machine code
>>>
>>> It increases the number of states, and now juggles two state variables.
>>> I cannot agree to it being cleaner.
>>
>> Yes, it increases the number of states. That was done in order to
>> actually implement a proper finite state machine with one state at a
>> time and a transition state.
> 
> I know, "clean" is subjective.

Clean is subjective, yes. However, based on our common sense and
experience we can discern on what is clean and what is not. There is
also good literature about the subject that we can always consider.

> But in what way was the old state
> machine not "proper"?

Because it didn't reflect all the correct and possible MIDI states and
there was no transitional state.

> 
> And how is handling two states (port->state and next_state) cleaner?
> As far as I can tell, the requirement for a separate variable comes not
> from any inherent complexity of the state machine itself, but only
> because the transmit_packet function was inlined.

next_state is a transitional state, thus the temporal nature.

This patch doesn't change any functionality. But the important thing
here is that it improves the driver maintainability by making the state
machine cleaner (which is one of the most important pieces of code of
the driver). I call it clean because on each circumstance of each state
it's clear on what is about to happen to the USB request and to the
port's buffers.

I confess I would not spend the time on it just for puritanisms, but I
found myself a hard time while debugging it.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens,

On 03/03/16 11:38, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 02/03/16 21:09, Clemens Ladisch wrote:
>>> Felipe F. Tonello wrote:
>>>> This refactor results in a cleaner state machine code
>>>
>>> It increases the number of states, and now juggles two state variables.
>>> I cannot agree to it being cleaner.
>>
>> Yes, it increases the number of states. That was done in order to
>> actually implement a proper finite state machine with one state at a
>> time and a transition state.
> 
> I know, "clean" is subjective.

Clean is subjective, yes. However, based on our common sense and
experience we can discern on what is clean and what is not. There is
also good literature about the subject that we can always consider.

> But in what way was the old state
> machine not "proper"?

Because it didn't reflect all the correct and possible MIDI states and
there was no transitional state.

> 
> And how is handling two states (port->state and next_state) cleaner?
> As far as I can tell, the requirement for a separate variable comes not
> from any inherent complexity of the state machine itself, but only
> because the transmit_packet function was inlined.

next_state is a transitional state, thus the temporal nature.

This patch doesn't change any functionality. But the important thing
here is that it improves the driver maintainability by making the state
machine cleaner (which is one of the most important pieces of code of
the driver). I call it clean because on each circumstance of each state
it's clear on what is about to happen to the USB request and to the
port's buffers.

I confess I would not spend the time on it just for puritanisms, but I
found myself a hard time while debugging it.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens,

On 02/03/16 21:09, Clemens Ladisch wrote:
> Felipe F. Tonello wrote:
>> This refactor results in a cleaner state machine code
> 
> It increases the number of states, and now juggles two state variables.
> I cannot agree to it being cleaner.

Yes, it increases the number of states. That was done in order to
actually implement a proper finite state machine with one state at a
time and a transition state. The result is a much cleaner MIDI parser
that is easy to maintain and read.

I recommend you to apply the patch yourself (it's on top of Balbi's next
branch) because the patch can be confusing to understand the end result.

> 
>> and as a result fixed a bug when packaging a USB-MIDI packet right after
>> a non-conformant MIDI byte stream.
> 
> I have been unable to determine where exactly the new code behaves
> differently.  Can you show an example?

Sorry, I forgot to remove this comment since your last revision. There
is no bug I could reproduce with the previous parser.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens,

On 02/03/16 21:09, Clemens Ladisch wrote:
> Felipe F. Tonello wrote:
>> This refactor results in a cleaner state machine code
> 
> It increases the number of states, and now juggles two state variables.
> I cannot agree to it being cleaner.

Yes, it increases the number of states. That was done in order to
actually implement a proper finite state machine with one state at a
time and a transition state. The result is a much cleaner MIDI parser
that is easy to maintain and read.

I recommend you to apply the patch yourself (it's on top of Balbi's next
branch) because the patch can be confusing to understand the end result.

> 
>> and as a result fixed a bug when packaging a USB-MIDI packet right after
>> a non-conformant MIDI byte stream.
> 
> I have been unable to determine where exactly the new code behaves
> differently.  Can you show an example?

Sorry, I forgot to remove this comment since your last revision. There
is no bug I could reproduce with the previous parser.

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCHv3 00/11] Fixes and improvements to f_fs and f_midi

[Felipe Ferreri Tonello]

Hi Michal,

On 26/01/16 14:53, Michal Nazarewicz wrote:
> Resending my previous two sets for f_fs and f_midi.  This time rebased
> on top of Felipe’s next branch.
> 
> Dan Carpenter (1):
>   usb: gadget: f_midi: missing unlock on error path
> 
> Du, Changbin (1):
>   usb: f_fs: avoid race condition with ffs_epfile_io_complete
> 
> Felipe F. Tonello (1):
>   usb: gadget: f_midi: remove useless midi reference from port struct
> 
> Michal Nazarewicz (8):
>   usb: f_fs: fix memory leak when ep changes during transfer
>   usb: f_fs: fix ffs_epfile_io returning success on req alloc failure
>   usb: f_fs: replace unnecessary goto with a return
>   usb: f_fs: refactor ffs_epfile_io
>   usb: gadget: f_midi: move some of f_midi_transmit to separate func
>   usb: gadget: f_midi: fix in_last_port looping logic
>   usb: gadget: f_midi: use flexible array member for gmidi_in_port
> elements
>   usb: gadget: f_midi: stash substream in gmidi_in_port structure

Sorry for the delay, but I will look into your patches related to f_midi
this week.

> 
>  drivers/usb/gadget/function/f_fs.c   | 155 +--
>  drivers/usb/gadget/function/f_midi.c | 200 
> ---
>  2 files changed, 164 insertions(+), 191 deletions(-)
> 

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCHv3 00/11] Fixes and improvements to f_fs and f_midi

[Felipe Ferreri Tonello]

Hi Michal,

On 26/01/16 14:53, Michal Nazarewicz wrote:
> Resending my previous two sets for f_fs and f_midi.  This time rebased
> on top of Felipe’s next branch.
> 
> Dan Carpenter (1):
>   usb: gadget: f_midi: missing unlock on error path
> 
> Du, Changbin (1):
>   usb: f_fs: avoid race condition with ffs_epfile_io_complete
> 
> Felipe F. Tonello (1):
>   usb: gadget: f_midi: remove useless midi reference from port struct
> 
> Michal Nazarewicz (8):
>   usb: f_fs: fix memory leak when ep changes during transfer
>   usb: f_fs: fix ffs_epfile_io returning success on req alloc failure
>   usb: f_fs: replace unnecessary goto with a return
>   usb: f_fs: refactor ffs_epfile_io
>   usb: gadget: f_midi: move some of f_midi_transmit to separate func
>   usb: gadget: f_midi: fix in_last_port looping logic
>   usb: gadget: f_midi: use flexible array member for gmidi_in_port
> elements
>   usb: gadget: f_midi: stash substream in gmidi_in_port structure

Sorry for the delay, but I will look into your patches related to f_midi
this week.

> 
>  drivers/usb/gadget/function/f_fs.c   | 155 +--
>  drivers/usb/gadget/function/f_midi.c | 200 
> ---
>  2 files changed, 164 insertions(+), 191 deletions(-)
> 

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v2 2/2] devicetree: sound: Fix fsl-asoc-card identation

[Felipe Ferreri Tonello]

Hi Nicolin,

On 29/01/16 18:02, Nicolin Chen wrote:
> On Fri, Jan 29, 2016 at 11:01:01AM +, Felipe F. Tonello wrote:
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  Documentation/devicetree/bindings/sound/fsl-asoc-card.txt | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt 
>> b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
>> index ceaef5126989..16b254b04f8c 100644
>> --- a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
>> +++ b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
>> @@ -43,7 +43,7 @@ Required properties:
>>  
>>- audio-cpu   : The phandle of an CPU DAI controller
>>  
>> -  - audio-codec : The phandle of an audio codec
>> +  - audio-codec : The phandle of an audio codec
> 
> The indentation was fine actually.how did you see it misaligned?

It doesn't look right on tab width as 8 spaces. I use `less -x8' for
core.pager git config for example.

But if it is correct, just ignore this patch then.

Thanks

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v2 2/2] devicetree: sound: Fix fsl-asoc-card identation

[Felipe Ferreri Tonello]

Hi Nicolin,

On 29/01/16 18:02, Nicolin Chen wrote:
> On Fri, Jan 29, 2016 at 11:01:01AM +, Felipe F. Tonello wrote:
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  Documentation/devicetree/bindings/sound/fsl-asoc-card.txt | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt 
>> b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
>> index ceaef5126989..16b254b04f8c 100644
>> --- a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
>> +++ b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
>> @@ -43,7 +43,7 @@ Required properties:
>>  
>>- audio-cpu   : The phandle of an CPU DAI controller
>>  
>> -  - audio-codec : The phandle of an audio codec
>> +  - audio-codec : The phandle of an audio codec
> 
> The indentation was fine actually.how did you see it misaligned?

It doesn't look right on tab width as 8 spaces. I use `less -x8' for
core.pager git config for example.

But if it is correct, just ignore this patch then.

Thanks

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] ASoC: fsl-asoc-card: add cs4271 and cs4272 support

[Felipe Ferreri Tonello]

Hi Nicolin,

On 28/01/16 18:24, Nicolin Chen wrote:
> On Thu, Jan 28, 2016 at 10:52:35AM +, Felipe F. Tonello wrote:
>> add cs4271 and cs42727 support for fsl-asoc-card
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  .../devicetree/bindings/sound/imx-audio-cs427x.txt | 47 
>> ++
>>  sound/soc/fsl/Kconfig  |  4 +-
>>  sound/soc/fsl/fsl-asoc-card.c  |  7 
>>  3 files changed, 56 insertions(+), 2 deletions(-)
>>  create mode 100644 
>> Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt 
>> b/Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt
>> new file mode 100644
>> index ..295f60b19418
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt
>> @@ -0,0 +1,47 @@
>> +Freescale i.MX audio complex with CS4271 or CS4272 codec
> 
> I think it could be better to insert these into the fsl-asoc-card.txt
> without those redundant property descriptions. At least it should be
> added to the compatible list. And you may add a comment in (...) to
> describe that it supports both 4271 and 4272.

I agree.

> 
>> +  - mux-int-port: The internal port of the i.MX audio muxer (AUDMUX)
>> +
>> +  - mux-ext-port: The external port of the i.MX audio muxer
>> +
>> +Note: The AUDMUX port numbering should start at 1, which is consistent with
>> +hardware manual.
> 
> These two properties are missing in the fsl-asoc-card binding docs.
> So it could be nice to have them as well. But they should be optional
> unless SSI is selected as a CPU DAI.

Ok.

Thanks

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] ASoC: fsl-asoc-card: add cs4271 and cs4272 support

[Felipe Ferreri Tonello]

Hi Nicolin,

On 28/01/16 18:24, Nicolin Chen wrote:
> On Thu, Jan 28, 2016 at 10:52:35AM +, Felipe F. Tonello wrote:
>> add cs4271 and cs42727 support for fsl-asoc-card
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  .../devicetree/bindings/sound/imx-audio-cs427x.txt | 47 
>> ++
>>  sound/soc/fsl/Kconfig  |  4 +-
>>  sound/soc/fsl/fsl-asoc-card.c  |  7 
>>  3 files changed, 56 insertions(+), 2 deletions(-)
>>  create mode 100644 
>> Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt 
>> b/Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt
>> new file mode 100644
>> index ..295f60b19418
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt
>> @@ -0,0 +1,47 @@
>> +Freescale i.MX audio complex with CS4271 or CS4272 codec
> 
> I think it could be better to insert these into the fsl-asoc-card.txt
> without those redundant property descriptions. At least it should be
> added to the compatible list. And you may add a comment in (...) to
> describe that it supports both 4271 and 4272.

I agree.

> 
>> +  - mux-int-port: The internal port of the i.MX audio muxer (AUDMUX)
>> +
>> +  - mux-ext-port: The external port of the i.MX audio muxer
>> +
>> +Note: The AUDMUX port numbering should start at 1, which is consistent with
>> +hardware manual.
> 
> These two properties are missing in the fsl-asoc-card binding docs.
> So it could be nice to have them as well. But they should be optional
> unless SSI is selected as a CPU DAI.

Ok.

Thanks

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] ASoC: fsl: add imx-cs427x machine driver

[Felipe Ferreri Tonello]

Hi Fabio,

On 26/01/16 09:47, Fabio Estevam wrote:
> Hi Felipe,
> 
> On Tue, Jan 26, 2016 at 7:43 AM, Felipe Ferreri Tonello
>  wrote:
> 
>> I agree, but that how it is today. These platform drivers for imx are
>> similar but not identical. Looking at them I would guess that they have
>> 50 to 60% of duplicated code.
> 
> Would simple-audio-card or fsl-asoc-card help in this case?
> 

Actually yes, thanks! I didn't know about the existence of fsl-asoc-card.

I get some errors but I don't think they actually matter:
[   19.734494] fsl-asrc 2034000.asrc: driver registered
[   19.738707] fsl-asoc-card sound: ASoC: CPU DAI (null) not registered
[   19.738717] fsl-asoc-card sound: snd_soc_register_card failed (-517)
[   19.741556] fsl-asoc-card sound: ASoC: CPU DAI (null) not registered
[   19.741564] fsl-asoc-card sound: snd_soc_register_card failed (-517)
[   19.774591] fsl-asoc-card sound: cs4271-hifi <-> 2028000.ssi mapping ok
[   19.781507] fsl-asoc-card sound: ASoC: no source widget found for
ASRC-Playback
[   19.790065] fsl-asoc-card sound: ASoC: Failed to add route
ASRC-Playback -> direct -> CPU-Playback
[   19.805349] fsl-asoc-card sound: ASoC: no sink widget found for
ASRC-Capture
[   19.817222] fsl-asoc-card sound: ASoC: Failed to add route
CPU-Capture -> direct -> ASRC-Capture

The codec is producing sound, which is good. Any idea on why these
errors are been triggered?

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] ASoC: fsl: add imx-cs427x machine driver

[Felipe Ferreri Tonello]

Hi Rob,

On 26/01/16 02:29, Rob Herring wrote:
> On Mon, Jan 25, 2016 at 05:53:23PM +, Felipe F. Tonello wrote:
>> This is the initial imx-cs427x device-tree-only machine driver working with
>> fsl_ssi driver. More features can be added on top of it later.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  .../devicetree/bindings/sound/imx-audio-cs427x.txt |  47 +
>>  sound/soc/fsl/Kconfig  |  12 ++
>>  sound/soc/fsl/Makefile |   2 +
>>  sound/soc/fsl/imx-cs427x.c | 218 
>> +
>>  4 files changed, 279 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt
>>  create mode 100644 sound/soc/fsl/imx-cs427x.c
>>
>> diff --git a/Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt 
>> b/Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt
>> new file mode 100644
>> index ..295f60b19418
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt
>> @@ -0,0 +1,47 @@
>> +Freescale i.MX audio complex with CS4271 or CS4272 codec
> 
> Do all the i.MX audio bindings really vary more that the codec? Seems 
> like a lot of duplication.

I agree, but that how it is today. These platform drivers for imx are
similar but not identical. Looking at them I would guess that they have
50 to 60% of duplicated code.

I believe we can add this driver and work on re-using the code on a
future series of patches.

> 
>> +
>> +Required properties:
>> +
>> +  - compatible  : "fsl,imx-audio-cs427x"
>> +
>> +  - model   : The user-visible name of this sound complex
>> +
>> +  - ssi-controller  : The phandle of the i.MX SSI controller
>> +
>> +  - audio-codec : The phandle of the CS4271 audio codec
>> +
>> +  - audio-routing   : A list of the connections between audio components.
>> +  Each entry is a pair of strings, the first being the
>> +  connection's sink, the second being the connection's
>> +  source. Valid names could be power supplies, CS427x
>> +  pins, and the jacks on the board:
>> +
>> +  Board connectors:
>> +   * Mic Jack
>> +   * Headphone Jack
> 
> This should be an exact list of possible strings and valid combinations.

This is the exact list for this version of this driver. At least that is
what I can verify on my hardware (custom build).

> 
>> +  - mux-int-port: The internal port of the i.MX audio muxer (AUDMUX)
>> +
>> +  - mux-ext-port: The external port of the i.MX audio muxer
>> +
>> +Note: The AUDMUX port numbering should start at 1, which is consistent with
>> +hardware manual.
>> +
>> +Example:
>> +
>> +sound {
>> +compatible = "fsl,imx6-rex-cs427x",
>> +"fsl,imx-audio-cs427x";
>> +model = "audio-cs427x";
>> +ssi-controller = <>;
>> +audio-codec = <>;
>> +audio-routing =
>> +"Mic Jack", "AINA",
>> +"Mic Jack", "AINB",
>> +"Headphone Jack", "AOUTA+",
>> +"Headphone Jack", "AOUTA-",
>> +"Headphone Jack", "AOUTB+",
>> +"Headphone Jack", "AOUTB-";
>> +mux-int-port = <1>;
>> +mux-ext-port = <3>;
>> +};

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] ASoC: fsl: add imx-cs427x machine driver

[Felipe Ferreri Tonello]

Hi Rob,

On 26/01/16 02:29, Rob Herring wrote:
> On Mon, Jan 25, 2016 at 05:53:23PM +, Felipe F. Tonello wrote:
>> This is the initial imx-cs427x device-tree-only machine driver working with
>> fsl_ssi driver. More features can be added on top of it later.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  .../devicetree/bindings/sound/imx-audio-cs427x.txt |  47 +
>>  sound/soc/fsl/Kconfig  |  12 ++
>>  sound/soc/fsl/Makefile |   2 +
>>  sound/soc/fsl/imx-cs427x.c | 218 
>> +
>>  4 files changed, 279 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt
>>  create mode 100644 sound/soc/fsl/imx-cs427x.c
>>
>> diff --git a/Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt 
>> b/Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt
>> new file mode 100644
>> index ..295f60b19418
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/imx-audio-cs427x.txt
>> @@ -0,0 +1,47 @@
>> +Freescale i.MX audio complex with CS4271 or CS4272 codec
> 
> Do all the i.MX audio bindings really vary more that the codec? Seems 
> like a lot of duplication.

I agree, but that how it is today. These platform drivers for imx are
similar but not identical. Looking at them I would guess that they have
50 to 60% of duplicated code.

I believe we can add this driver and work on re-using the code on a
future series of patches.

> 
>> +
>> +Required properties:
>> +
>> +  - compatible  : "fsl,imx-audio-cs427x"
>> +
>> +  - model   : The user-visible name of this sound complex
>> +
>> +  - ssi-controller  : The phandle of the i.MX SSI controller
>> +
>> +  - audio-codec : The phandle of the CS4271 audio codec
>> +
>> +  - audio-routing   : A list of the connections between audio components.
>> +  Each entry is a pair of strings, the first being the
>> +  connection's sink, the second being the connection's
>> +  source. Valid names could be power supplies, CS427x
>> +  pins, and the jacks on the board:
>> +
>> +  Board connectors:
>> +   * Mic Jack
>> +   * Headphone Jack
> 
> This should be an exact list of possible strings and valid combinations.

This is the exact list for this version of this driver. At least that is
what I can verify on my hardware (custom build).

> 
>> +  - mux-int-port: The internal port of the i.MX audio muxer (AUDMUX)
>> +
>> +  - mux-ext-port: The external port of the i.MX audio muxer
>> +
>> +Note: The AUDMUX port numbering should start at 1, which is consistent with
>> +hardware manual.
>> +
>> +Example:
>> +
>> +sound {
>> +compatible = "fsl,imx6-rex-cs427x",
>> +"fsl,imx-audio-cs427x";
>> +model = "audio-cs427x";
>> +ssi-controller = <>;
>> +audio-codec = <>;
>> +audio-routing =
>> +"Mic Jack", "AINA",
>> +"Mic Jack", "AINB",
>> +"Headphone Jack", "AOUTA+",
>> +"Headphone Jack", "AOUTA-",
>> +"Headphone Jack", "AOUTB+",
>> +"Headphone Jack", "AOUTB-";
>> +mux-int-port = <1>;
>> +mux-ext-port = <3>;
>> +};

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH] ASoC: fsl: add imx-cs427x machine driver

[Felipe Ferreri Tonello]

Hi Fabio,

On 26/01/16 09:47, Fabio Estevam wrote:
> Hi Felipe,
> 
> On Tue, Jan 26, 2016 at 7:43 AM, Felipe Ferreri Tonello
> <e...@felipetonello.com> wrote:
> 
>> I agree, but that how it is today. These platform drivers for imx are
>> similar but not identical. Looking at them I would guess that they have
>> 50 to 60% of duplicated code.
> 
> Would simple-audio-card or fsl-asoc-card help in this case?
> 

Actually yes, thanks! I didn't know about the existence of fsl-asoc-card.

I get some errors but I don't think they actually matter:
[   19.734494] fsl-asrc 2034000.asrc: driver registered
[   19.738707] fsl-asoc-card sound: ASoC: CPU DAI (null) not registered
[   19.738717] fsl-asoc-card sound: snd_soc_register_card failed (-517)
[   19.741556] fsl-asoc-card sound: ASoC: CPU DAI (null) not registered
[   19.741564] fsl-asoc-card sound: snd_soc_register_card failed (-517)
[   19.774591] fsl-asoc-card sound: cs4271-hifi <-> 2028000.ssi mapping ok
[   19.781507] fsl-asoc-card sound: ASoC: no source widget found for
ASRC-Playback
[   19.790065] fsl-asoc-card sound: ASoC: Failed to add route
ASRC-Playback -> direct -> CPU-Playback
[   19.805349] fsl-asoc-card sound: ASoC: no sink widget found for
ASRC-Capture
[   19.817222] fsl-asoc-card sound: ASoC: Failed to add route
CPU-Capture -> direct -> ASRC-Capture

The codec is producing sound, which is good. Any idea on why these
errors are been triggered?

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens,

On 22/12/15 17:10, Clemens Ladisch wrote:
> Felipe F. Tonello wrote:
>> This refactor includes the following:
>>  * Cleaner state machine code;
> 
> It does not correctly handle system real time messages inserted between
> the status and data bytes of other messages.

True, thanks for pointing that out. I fixed that on next revision of
this patch.

> 
>>  * Reset state if MIDI message parsed is non-conformant;
> 
> Why?  In a byte stream like "C1 C3 44", where the data byte of the first
> message was lost, the reset would also drop the second message.

True. That was fixed as well.

> 
>>  * Fixed bug when a conformant MIDI message was followed by a non-conformant
>>causing the MIDI-USB message to use old temporary data (port->data[0..1]),
>>thus packing a wrong MIDI-USB request.
> 
> Running status is feature.

What do you mean by that? I don't qualify writing a *wrong* MIDI-USB
packet because of a previous MIDI message as a feature.

For instance, try this MIDI message:
"8A 54 24 00 40"

It will be converted to MIDI-USB as "08 8A 54 24 08 8A 00 40" which is
wrong. It should only be "08 8A 54 24" and ignore the "00 40" MIDI bytes.

On every state byte the message should basically reset data[0..1] to
zero overwriting previous data. This should also be true when a MIDI-USB
packet is complete.

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Hi Balbi,

On 22/12/15 17:49, Felipe Balbi wrote:
> 
> Hi,
> 
> "Felipe F. Tonello"  writes:
>> This refactor includes the following: * Cleaner state machine
>> code; * Reset state if MIDI message parsed is non-conformant; *
>> Fixed bug when a conformant MIDI message was followed by a
>> non-conformant causing the MIDI-USB message to use old temporary
>> data (port->data[0..1]), thus packing a wrong MIDI-USB request.
> 
> we don't do more than one logical thing per patch. Please split
> this up.

Actually this patch has only one logical change, the state machine
refactoring. But by doing it, those three items were a consequence.

Felipe
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCAAGBQJWemc+AAoJEMxEtNCSaY5qpXwP/1B/sVsfapRtP4dw93YF6En5
w/ej9JatIyJIaXNxauCVzgRl9uuiXGyEqErRjJdodyvHar3yKvD9HEXE6MhowEp4
JMD/phN2v9Sdj1VxRf9Z0XDSWsg6huVhfQU47HBMRGiY8ezvEgir2bvg7dYbZMsv
ACgIx8oh6N/AEHPq9GoLbEpXfJ58Pl564Sq/2o6wWsJQS06A+jp+JmqK4Y3eB5M5
18rmLW7lQLcZPO08Pf/c6BExWQLbzY/mT8KofwycvC9hWxYp9LPwJY4oMzOWROe8
AZS1ayRqlabG3qx3dPRcV4j6c6uROEfQY+HejCn+Zbi0CfVYtsI+xO5LkwnZMUyc
0qvy90h0PUQ2e37/wo5YnnVLK0ce1Gm6gY50iFXwqE69m55KHTufsIVX4eTUBfcj
PtugPtTnKEsW171r/gHPYO9A+WKxycCvjPs9Wogsljly+NrRzgPMAI7Alx//4lwq
QhwRWF1BkNOoCPEpHnVlLkcIhgdcAbbnvWxlcAVlLAQ/oYl6ShbQFv96y/2IWCVO
Mwr7Ab8a/dnyJ/GWEQdpJUmfKGQbKNpM93H5yD4AQlmz4Hj620gzm3y2XNJY2bUt
8H37Q2VWtlcRy2UHjgBSeF0YKCvdDuDmZwRZbPpajkmbcYSGtJFDve1/L6bOtZSj
7nVfYIqvtIBWrDZ3PF9G
=3uC5
-END PGP SIGNATURE-


0x92698E6A.asc
Description: application/pgp-keys


0x92698E6A.asc.sig
Description: PGP signature

Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens,

On 22/12/15 17:10, Clemens Ladisch wrote:
> Felipe F. Tonello wrote:
>> This refactor includes the following:
>>  * Cleaner state machine code;
> 
> It does not correctly handle system real time messages inserted between
> the status and data bytes of other messages.

True, thanks for pointing that out. I fixed that on next revision of
this patch.

> 
>>  * Reset state if MIDI message parsed is non-conformant;
> 
> Why?  In a byte stream like "C1 C3 44", where the data byte of the first
> message was lost, the reset would also drop the second message.

True. That was fixed as well.

> 
>>  * Fixed bug when a conformant MIDI message was followed by a non-conformant
>>causing the MIDI-USB message to use old temporary data (port->data[0..1]),
>>thus packing a wrong MIDI-USB request.
> 
> Running status is feature.

What do you mean by that? I don't qualify writing a *wrong* MIDI-USB
packet because of a previous MIDI message as a feature.

For instance, try this MIDI message:
"8A 54 24 00 40"

It will be converted to MIDI-USB as "08 8A 54 24 08 8A 00 40" which is
wrong. It should only be "08 8A 54 24" and ignore the "00 40" MIDI bytes.

On every state byte the message should basically reset data[0..1] to
zero overwriting previous data. This should also be true when a MIDI-USB
packet is complete.

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Hi Balbi,

On 22/12/15 17:49, Felipe Balbi wrote:
> 
> Hi,
> 
> "Felipe F. Tonello"  writes:
>> This refactor includes the following: * Cleaner state machine
>> code; * Reset state if MIDI message parsed is non-conformant; *
>> Fixed bug when a conformant MIDI message was followed by a
>> non-conformant causing the MIDI-USB message to use old temporary
>> data (port->data[0..1]), thus packing a wrong MIDI-USB request.
> 
> we don't do more than one logical thing per patch. Please split
> this up.

Actually this patch has only one logical change, the state machine
refactoring. But by doing it, those three items were a consequence.

Felipe
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCAAGBQJWemc+AAoJEMxEtNCSaY5qpXwP/1B/sVsfapRtP4dw93YF6En5
w/ej9JatIyJIaXNxauCVzgRl9uuiXGyEqErRjJdodyvHar3yKvD9HEXE6MhowEp4
JMD/phN2v9Sdj1VxRf9Z0XDSWsg6huVhfQU47HBMRGiY8ezvEgir2bvg7dYbZMsv
ACgIx8oh6N/AEHPq9GoLbEpXfJ58Pl564Sq/2o6wWsJQS06A+jp+JmqK4Y3eB5M5
18rmLW7lQLcZPO08Pf/c6BExWQLbzY/mT8KofwycvC9hWxYp9LPwJY4oMzOWROe8
AZS1ayRqlabG3qx3dPRcV4j6c6uROEfQY+HejCn+Zbi0CfVYtsI+xO5LkwnZMUyc
0qvy90h0PUQ2e37/wo5YnnVLK0ce1Gm6gY50iFXwqE69m55KHTufsIVX4eTUBfcj
PtugPtTnKEsW171r/gHPYO9A+WKxycCvjPs9Wogsljly+NrRzgPMAI7Alx//4lwq
QhwRWF1BkNOoCPEpHnVlLkcIhgdcAbbnvWxlcAVlLAQ/oYl6ShbQFv96y/2IWCVO
Mwr7Ab8a/dnyJ/GWEQdpJUmfKGQbKNpM93H5yD4AQlmz4Hj620gzm3y2XNJY2bUt
8H37Q2VWtlcRy2UHjgBSeF0YKCvdDuDmZwRZbPpajkmbcYSGtJFDve1/L6bOtZSj
7nVfYIqvtIBWrDZ3PF9G
=3uC5
-END PGP SIGNATURE-


0x92698E6A.asc
Description: application/pgp-keys


0x92698E6A.asc.sig
Description: PGP signature

Re: [PATCH v3 01/36] Documentation: usb: update usb-tools repository address

[Felipe Ferreri Tonello]

Hi Robert,

On 11/12/15 11:24, Robert Baldyga wrote:
> It seems that gitotious repository is no longer accessible, so we replace
> it with address to active repository.
> 
> Signed-off-by: Robert Baldyga 
> ---
>  Documentation/usb/gadget-testing.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/usb/gadget-testing.txt 
> b/Documentation/usb/gadget-testing.txt
> index 84b3d10..5819605 100644
> --- a/Documentation/usb/gadget-testing.txt
> +++ b/Documentation/usb/gadget-testing.txt
> @@ -434,7 +434,7 @@ On host: serialc -v  -p  
> -i -a1 -s1024 \
>  
>  where seriald and serialc are Felipe's utilities found here:
>  
> -https://git.gitorious.org/usb/usb-tools.git master
> +https://github.com/felipebalbi/usb-tools.git master
>  
>  12. PHONET function
>  ===
> 

This patch was already applied to Balbi's testing/next branch.

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v6 0/3] USB MIDI Gadget improvements and bug fixes

[Felipe Ferreri Tonello]

Hi Balbi,

On 16/12/15 16:03, Felipe Balbi wrote:
> 
> Hi
> 
> Felipe Ferreri Tonello  writes:
>> Hi all,
>>
>> On 01/12/15 18:30, Felipe F. Tonello wrote:
>>> Fixed all comments suggested by the linux-usb list.
>>>
>>> changes in v6:
>>>  - Removed patches already applied in Balbi's tree
>>>  - Cleanups on pre-allocation usb requrests patch
>>>  - Fixed indentention on patch 1
>>>  - Added patch which fails set_alt if a failure happened while
>>>allocating usb requests
>>>
>>> changes in v5:
>>>  - Use ep->enabled insetad of creating driver specific flag
>>>  - Save MIDIStreaming interface id in driver data
>>>  - define free_ep_req as static inline in header
>>>
>>> changes in v4:
>>>  - pre-alocation of in requests.
>>>  - more code clean up
>>>  - fix memory leak on out requests
>>>  - configure endpoints only when setting up MIDIStreaming interface
>>> 
>>> Felipe F. Tonello (3):
>>>   usb: gadget: f_midi: set altsettings only for MIDIStreaming interface
>>>   usb: gadget: f_midi: fail if set_alt fails to allocate requests
>>>   usb: gadget: f_midi: pre-allocate IN requests
>>>
>>>  drivers/usb/gadget/function/f_midi.c | 175 
>>> +++
>>>  drivers/usb/gadget/legacy/gmidi.c|   2 +-
>>>  2 files changed, 135 insertions(+), 42 deletions(-)
>>>
>>
>> Ping?
> 
> It should be in my testing/next now, I had to wait until Greg merged
> fixes to linus before applying.
> 

Thanks, I still have two more patches that I will be sending as soon as
these get to your next branch.

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v6 0/3] USB MIDI Gadget improvements and bug fixes

[Felipe Ferreri Tonello]

Hi Balbi,

On 16/12/15 16:03, Felipe Balbi wrote:
> 
> Hi
> 
> Felipe Ferreri Tonello <e...@felipetonello.com> writes:
>> Hi all,
>>
>> On 01/12/15 18:30, Felipe F. Tonello wrote:
>>> Fixed all comments suggested by the linux-usb list.
>>>
>>> changes in v6:
>>>  - Removed patches already applied in Balbi's tree
>>>  - Cleanups on pre-allocation usb requrests patch
>>>  - Fixed indentention on patch 1
>>>  - Added patch which fails set_alt if a failure happened while
>>>allocating usb requests
>>>
>>> changes in v5:
>>>  - Use ep->enabled insetad of creating driver specific flag
>>>  - Save MIDIStreaming interface id in driver data
>>>  - define free_ep_req as static inline in header
>>>
>>> changes in v4:
>>>  - pre-alocation of in requests.
>>>  - more code clean up
>>>  - fix memory leak on out requests
>>>  - configure endpoints only when setting up MIDIStreaming interface
>>> 
>>> Felipe F. Tonello (3):
>>>   usb: gadget: f_midi: set altsettings only for MIDIStreaming interface
>>>   usb: gadget: f_midi: fail if set_alt fails to allocate requests
>>>   usb: gadget: f_midi: pre-allocate IN requests
>>>
>>>  drivers/usb/gadget/function/f_midi.c | 175 
>>> +++
>>>  drivers/usb/gadget/legacy/gmidi.c|   2 +-
>>>  2 files changed, 135 insertions(+), 42 deletions(-)
>>>
>>
>> Ping?
> 
> It should be in my testing/next now, I had to wait until Greg merged
> fixes to linus before applying.
> 

Thanks, I still have two more patches that I will be sending as soon as
these get to your next branch.

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v3 01/36] Documentation: usb: update usb-tools repository address

[Felipe Ferreri Tonello]

Hi Robert,

On 11/12/15 11:24, Robert Baldyga wrote:
> It seems that gitotious repository is no longer accessible, so we replace
> it with address to active repository.
> 
> Signed-off-by: Robert Baldyga 
> ---
>  Documentation/usb/gadget-testing.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/usb/gadget-testing.txt 
> b/Documentation/usb/gadget-testing.txt
> index 84b3d10..5819605 100644
> --- a/Documentation/usb/gadget-testing.txt
> +++ b/Documentation/usb/gadget-testing.txt
> @@ -434,7 +434,7 @@ On host: serialc -v  -p  
> -i -a1 -s1024 \
>  
>  where seriald and serialc are Felipe's utilities found here:
>  
> -https://git.gitorious.org/usb/usb-tools.git master
> +https://github.com/felipebalbi/usb-tools.git master
>  
>  12. PHONET function
>  ===
> 

This patch was already applied to Balbi's testing/next branch.

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v6 0/3] USB MIDI Gadget improvements and bug fixes

[Felipe Ferreri Tonello]

Hi all,

On 01/12/15 18:30, Felipe F. Tonello wrote:
> Fixed all comments suggested by the linux-usb list.
> 
> changes in v6:
>  - Removed patches already applied in Balbi's tree
>  - Cleanups on pre-allocation usb requrests patch
>  - Fixed indentention on patch 1
>  - Added patch which fails set_alt if a failure happened while
>allocating usb requests
> 
> changes in v5:
>  - Use ep->enabled insetad of creating driver specific flag
>  - Save MIDIStreaming interface id in driver data
>  - define free_ep_req as static inline in header
> 
> changes in v4:
>  - pre-alocation of in requests.
>  - more code clean up
>  - fix memory leak on out requests
>  - configure endpoints only when setting up MIDIStreaming interface
>   
> Felipe F. Tonello (3):
>   usb: gadget: f_midi: set altsettings only for MIDIStreaming interface
>   usb: gadget: f_midi: fail if set_alt fails to allocate requests
>   usb: gadget: f_midi: pre-allocate IN requests
> 
>  drivers/usb/gadget/function/f_midi.c | 175 
> +++
>  drivers/usb/gadget/legacy/gmidi.c|   2 +-
>  2 files changed, 135 insertions(+), 42 deletions(-)
> 

Ping?

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v6 0/3] USB MIDI Gadget improvements and bug fixes

[Felipe Ferreri Tonello]

Hi all,

On 01/12/15 18:30, Felipe F. Tonello wrote:
> Fixed all comments suggested by the linux-usb list.
> 
> changes in v6:
>  - Removed patches already applied in Balbi's tree
>  - Cleanups on pre-allocation usb requrests patch
>  - Fixed indentention on patch 1
>  - Added patch which fails set_alt if a failure happened while
>allocating usb requests
> 
> changes in v5:
>  - Use ep->enabled insetad of creating driver specific flag
>  - Save MIDIStreaming interface id in driver data
>  - define free_ep_req as static inline in header
> 
> changes in v4:
>  - pre-alocation of in requests.
>  - more code clean up
>  - fix memory leak on out requests
>  - configure endpoints only when setting up MIDIStreaming interface
>   
> Felipe F. Tonello (3):
>   usb: gadget: f_midi: set altsettings only for MIDIStreaming interface
>   usb: gadget: f_midi: fail if set_alt fails to allocate requests
>   usb: gadget: f_midi: pre-allocate IN requests
> 
>  drivers/usb/gadget/function/f_midi.c | 175 
> +++
>  drivers/usb/gadget/legacy/gmidi.c|   2 +-
>  2 files changed, 135 insertions(+), 42 deletions(-)
> 

Ping?

-- 
Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v6 3/3] usb: gadget: f_midi: pre-allocate IN requests

[Felipe Ferreri Tonello]

On 01/12/15 18:31, Felipe F. Tonello wrote:
> This patch introduces pre-allocation of IN endpoint USB requests. This
> improves on latency (requires no usb request allocation on transmit) and avoid
> several potential probles on allocating too many usb requests (which involves
> DMA pool allocation problems).
> 
> This implementation also handles better multiple MIDI Gadget ports, always
> processing the last processed MIDI substream if the last USB request wasn't
> enought to handle the whole stream.
> 
> Signed-off-by: Felipe F. Tonello 
> ---
>  drivers/usb/gadget/function/f_midi.c | 166 
> +++
>  drivers/usb/gadget/legacy/gmidi.c|   2 +-
>  2 files changed, 129 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index 79dc611..fb1fe96d 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -88,6 +89,9 @@ struct f_midi {
>   int index;
>   char *id;
>   unsigned int buflen, qlen;
> + /* This fifo is used as a buffer ring for pre-allocated IN usb_requests 
> */
> + DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
> + unsigned int in_last_port;
>  };
>  
>  static inline struct f_midi *func_to_midi(struct usb_function *f)
> @@ -95,7 +99,7 @@ static inline struct f_midi *func_to_midi(struct 
> usb_function *f)
>   return container_of(f, struct f_midi, func);
>  }
>  
> -static void f_midi_transmit(struct f_midi *midi, struct usb_request *req);
> +static void f_midi_transmit(struct f_midi *midi);
>  
>  DECLARE_UAC_AC_HEADER_DESCRIPTOR(1);
>  DECLARE_USB_MIDI_OUT_JACK_DESCRIPTOR(1);
> @@ -253,7 +257,8 @@ f_midi_complete(struct usb_ep *ep, struct usb_request 
> *req)
>   } else if (ep == midi->in_ep) {
>   /* Our transmit completed. See if there's more to go.
>* f_midi_transmit eats req, don't queue it again. */
> - f_midi_transmit(midi, req);
> + req->length = 0;
> + f_midi_transmit(midi);
>   return;
>   }
>   break;
> @@ -264,10 +269,12 @@ f_midi_complete(struct usb_ep *ep, struct usb_request 
> *req)
>   case -ESHUTDOWN:/* disconnect from host */
>   VDBG(cdev, "%s gone (%d), %d/%d\n", ep->name, status,
>   req->actual, req->length);
> - if (ep == midi->out_ep)
> + if (ep == midi->out_ep) {
>   f_midi_handle_out_data(ep, req);
> -
> - free_ep_req(ep, req);
> + /* We don't need to free IN requests because it's 
> handled
> +  * by the midi->in_req_fifo. */
> + free_ep_req(ep, req);
> + }
>   return;
>  
>   case -EOVERFLOW:/* buffer overrun on read means that
> @@ -334,6 +341,20 @@ static int f_midi_set_alt(struct usb_function *f, 
> unsigned intf, unsigned alt)
>   if (err)
>   return err;
>  
> + /* pre-allocate write usb requests to use on f_midi_transmit. */
> + while (kfifo_avail(>in_req_fifo)) {
> + struct usb_request *req =
> + midi_alloc_ep_req(midi->in_ep, midi->buflen);
> +
> + if (req == NULL)
> + return -ENOMEM;
> +
> + req->length = 0;
> + req->complete = f_midi_complete;
> +
> + kfifo_put(>in_req_fifo, req);
> + }
> +
>   /* allocate a bunch of read buffers and queue them all at once. */
>   for (i = 0; i < midi->qlen && err == 0; i++) {
>   struct usb_request *req =
> @@ -358,6 +379,7 @@ static void f_midi_disable(struct usb_function *f)
>  {
>   struct f_midi *midi = func_to_midi(f);
>   struct usb_composite_dev *cdev = f->config->cdev;
> + struct usb_request *req = NULL;
>  
>   DBG(cdev, "disable\n");
>  
> @@ -367,6 +389,10 @@ static void f_midi_disable(struct usb_function *f)
>*/
>   usb_ep_disable(midi->in_ep);
>   usb_ep_disable(midi->out_ep);
> +
> + /* release IN requests */
> + while (kfifo_get(>in_req_fifo, ))
> + free_ep_req(midi->in_ep, req);
>  }
>  
>  static int f_midi_snd_free(struct snd_device *device)
> @@ -488,57 +514,113 @@ static void f_midi_transmit_byte(struct usb_request 
> *req,
>   }
>  }
>  
> -static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
> +static void f_midi_drop_out_substreams(struct f_midi *midi)
>  {
> - struct usb_ep *ep = midi->in_ep;
> - int i;
> -
> - if (!ep)
> - return;
> -
> - if (!req)
> - req = midi_alloc_ep_req(ep, midi->buflen);
> -
> - if (!req) {
> - ERROR(midi, "%s: alloc_ep_request failed\n", __func__);
> -  

Re: [PATCH v6 3/3] usb: gadget: f_midi: pre-allocate IN requests

[Felipe Ferreri Tonello]

On 01/12/15 18:31, Felipe F. Tonello wrote:
> This patch introduces pre-allocation of IN endpoint USB requests. This
> improves on latency (requires no usb request allocation on transmit) and avoid
> several potential probles on allocating too many usb requests (which involves
> DMA pool allocation problems).
> 
> This implementation also handles better multiple MIDI Gadget ports, always
> processing the last processed MIDI substream if the last USB request wasn't
> enought to handle the whole stream.
> 
> Signed-off-by: Felipe F. Tonello 
> ---
>  drivers/usb/gadget/function/f_midi.c | 166 
> +++
>  drivers/usb/gadget/legacy/gmidi.c|   2 +-
>  2 files changed, 129 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index 79dc611..fb1fe96d 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -88,6 +89,9 @@ struct f_midi {
>   int index;
>   char *id;
>   unsigned int buflen, qlen;
> + /* This fifo is used as a buffer ring for pre-allocated IN usb_requests 
> */
> + DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
> + unsigned int in_last_port;
>  };
>  
>  static inline struct f_midi *func_to_midi(struct usb_function *f)
> @@ -95,7 +99,7 @@ static inline struct f_midi *func_to_midi(struct 
> usb_function *f)
>   return container_of(f, struct f_midi, func);
>  }
>  
> -static void f_midi_transmit(struct f_midi *midi, struct usb_request *req);
> +static void f_midi_transmit(struct f_midi *midi);
>  
>  DECLARE_UAC_AC_HEADER_DESCRIPTOR(1);
>  DECLARE_USB_MIDI_OUT_JACK_DESCRIPTOR(1);
> @@ -253,7 +257,8 @@ f_midi_complete(struct usb_ep *ep, struct usb_request 
> *req)
>   } else if (ep == midi->in_ep) {
>   /* Our transmit completed. See if there's more to go.
>* f_midi_transmit eats req, don't queue it again. */
> - f_midi_transmit(midi, req);
> + req->length = 0;
> + f_midi_transmit(midi);
>   return;
>   }
>   break;
> @@ -264,10 +269,12 @@ f_midi_complete(struct usb_ep *ep, struct usb_request 
> *req)
>   case -ESHUTDOWN:/* disconnect from host */
>   VDBG(cdev, "%s gone (%d), %d/%d\n", ep->name, status,
>   req->actual, req->length);
> - if (ep == midi->out_ep)
> + if (ep == midi->out_ep) {
>   f_midi_handle_out_data(ep, req);
> -
> - free_ep_req(ep, req);
> + /* We don't need to free IN requests because it's 
> handled
> +  * by the midi->in_req_fifo. */
> + free_ep_req(ep, req);
> + }
>   return;
>  
>   case -EOVERFLOW:/* buffer overrun on read means that
> @@ -334,6 +341,20 @@ static int f_midi_set_alt(struct usb_function *f, 
> unsigned intf, unsigned alt)
>   if (err)
>   return err;
>  
> + /* pre-allocate write usb requests to use on f_midi_transmit. */
> + while (kfifo_avail(>in_req_fifo)) {
> + struct usb_request *req =
> + midi_alloc_ep_req(midi->in_ep, midi->buflen);
> +
> + if (req == NULL)
> + return -ENOMEM;
> +
> + req->length = 0;
> + req->complete = f_midi_complete;
> +
> + kfifo_put(>in_req_fifo, req);
> + }
> +
>   /* allocate a bunch of read buffers and queue them all at once. */
>   for (i = 0; i < midi->qlen && err == 0; i++) {
>   struct usb_request *req =
> @@ -358,6 +379,7 @@ static void f_midi_disable(struct usb_function *f)
>  {
>   struct f_midi *midi = func_to_midi(f);
>   struct usb_composite_dev *cdev = f->config->cdev;
> + struct usb_request *req = NULL;
>  
>   DBG(cdev, "disable\n");
>  
> @@ -367,6 +389,10 @@ static void f_midi_disable(struct usb_function *f)
>*/
>   usb_ep_disable(midi->in_ep);
>   usb_ep_disable(midi->out_ep);
> +
> + /* release IN requests */
> + while (kfifo_get(>in_req_fifo, ))
> + free_ep_req(midi->in_ep, req);
>  }
>  
>  static int f_midi_snd_free(struct snd_device *device)
> @@ -488,57 +514,113 @@ static void f_midi_transmit_byte(struct usb_request 
> *req,
>   }
>  }
>  
> -static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
> +static void f_midi_drop_out_substreams(struct f_midi *midi)
>  {
> - struct usb_ep *ep = midi->in_ep;
> - int i;
> -
> - if (!ep)
> - return;
> -
> - if (!req)
> - req = midi_alloc_ep_req(ep, midi->buflen);
> -
> - if (!req) {
> - ERROR(midi, "%s: alloc_ep_request 

Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests

[Felipe Ferreri Tonello]

Hi Clemens

On 27/11/15 09:05, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 13/11/15 08:55, Clemens Ladisch wrote:
>>> Felipe F. Tonello wrote:
>>>> +static void f_midi_transmit(struct f_midi *midi)
>>>> +{
>>>> +...
>>>> +  len = kfifo_peek(>in_req_fifo, );
>>>> +  ...
>>>> +  if (req->length > 0) {
>>>> +  WARNING(midi, "%s: All USB requests have been used. 
>>>> Current queue size "
>>>> +  "is %u, consider increasing it.\n", __func__, 
>>>> midi->in_req_num);
>>>> +  goto drop_out;
>>>> +  }
>>>
>>> There are two cases where the in_req FIFO might overflow:
>>> 1) the gadget is trying to send too much data at once; or
>>> 2) the host does not bother to read any of the data.
>>>
>>> In case 1), the appropriate action would be to do nothing, so that the
>>> remaining data is sent after some currently queued packets have been
>>> transmitted.  In case 2), the appropriate action would be to drop the
>>> data (even better, the _oldest_ data), and spamming the log with error
>>> messages would not help.
>>
>> True. In this case the log will be spammed.
>>
>> How would you suggest to drop the oldest data? That doesn't really seem
>> to be feasible.
> 
> There is usb_ep_dequeue().  Its documentation warns about some hardware,
> but it would be possible to at least try it.
> 
>>> I'm not quite sure if trying to detect which of these cases we have is
>>> possible, or worthwhile.  Anyway, with a packet size of 64, the queue
>>> size would be 32*64 = 2KB, which should be enough for everyone.  So I
>>> propose to ignore case 1), and to drop the error message.
> 
> After some thought, I'm not so sure anymore -- the ability to buffer
> more than 2 KB of data is part of the snd_rawmidi_write() API, so this
> could introduce a regression.  And I can imagine cases where one would
> actually want to transmit large amounts data.

One thing to consider is that the ALSA rawmidi device buffer is
sequential and our USB request buffer is not. This means that our 32
(qlen) * 256 (buflen) = 8KB of data is non-linear. Some requests might
have 3 or 4 bytes (average size of a normal MIDI message) of data and
some others might contain the full 256 bytes (for SysEx messages).

I am considering this especially for MPE (Multidimensional Polyphonic
Expression) MIDI protocol. On few benchmarks I did, a device that
implements this protocol generates around 500-2000 b/s of *raw* MIDI
data. And in practice only 4 (average MIDI message) * 32 (USB requests
defined by qlen) bytes will be used. Which means that the 8KB USB
request buffer will be under used.

So I think we have to treat the ALSA buffers and the USB request buffers
differently.

That's why I think this approach is fine by allowing the user to
increase that number of requests and its size if it needs to deal with a
higher throughput devices.

> 
> I think the safest approach would be to behave similar to the old driver,
> i.e., when the queue overflows, do nothing (not even dropping data), and
> rely on the transmit completion handler to continue.  (This implies that
> ALSA's buffer can fill up, and that snd_rawmidi_write() can block.)
> 

The previous implementation would not block, even though
snd_rawmidi_write() can block, because it was been created a new USB
request for each write call and data was been consumed even if this
request would not be enqueued to the endpoint.

But, anyway, I agree with your suggestion.

> 
> It you want to dequeue outdated data, I think this should be done with
> a timeout, i.e., when the host did not read anything for some tens of
> milliseconds or so.  This would be independent of the fill level of the
> queue, and could be done either for individual packets, or just on the
> entire endpoint queue.

That can be done. But I believe in another patch since it is not
required to work for this patch.

== Conclusion ==

Based on our conversation and your suggestions, I think that to just
ignore if an overrun occurs to the USB requests is fine. Upon completion
the request will be reused.
Important to note that if the overrun occurs, it will cause user-space
to block until a) the completion function is called successfully or b)
snd_rawmidi_write() times out. Which I think this is expected by ALSA users.

Does that make sense?

If yes then I will send the v6 of this patch.

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests

[Felipe Ferreri Tonello]

Hi Clemens

On 27/11/15 09:05, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 13/11/15 08:55, Clemens Ladisch wrote:
>>> Felipe F. Tonello wrote:
>>>> +static void f_midi_transmit(struct f_midi *midi)
>>>> +{
>>>> +...
>>>> +  len = kfifo_peek(>in_req_fifo, );
>>>> +  ...
>>>> +  if (req->length > 0) {
>>>> +  WARNING(midi, "%s: All USB requests have been used. 
>>>> Current queue size "
>>>> +  "is %u, consider increasing it.\n", __func__, 
>>>> midi->in_req_num);
>>>> +  goto drop_out;
>>>> +  }
>>>
>>> There are two cases where the in_req FIFO might overflow:
>>> 1) the gadget is trying to send too much data at once; or
>>> 2) the host does not bother to read any of the data.
>>>
>>> In case 1), the appropriate action would be to do nothing, so that the
>>> remaining data is sent after some currently queued packets have been
>>> transmitted.  In case 2), the appropriate action would be to drop the
>>> data (even better, the _oldest_ data), and spamming the log with error
>>> messages would not help.
>>
>> True. In this case the log will be spammed.
>>
>> How would you suggest to drop the oldest data? That doesn't really seem
>> to be feasible.
> 
> There is usb_ep_dequeue().  Its documentation warns about some hardware,
> but it would be possible to at least try it.
> 
>>> I'm not quite sure if trying to detect which of these cases we have is
>>> possible, or worthwhile.  Anyway, with a packet size of 64, the queue
>>> size would be 32*64 = 2KB, which should be enough for everyone.  So I
>>> propose to ignore case 1), and to drop the error message.
> 
> After some thought, I'm not so sure anymore -- the ability to buffer
> more than 2 KB of data is part of the snd_rawmidi_write() API, so this
> could introduce a regression.  And I can imagine cases where one would
> actually want to transmit large amounts data.

One thing to consider is that the ALSA rawmidi device buffer is
sequential and our USB request buffer is not. This means that our 32
(qlen) * 256 (buflen) = 8KB of data is non-linear. Some requests might
have 3 or 4 bytes (average size of a normal MIDI message) of data and
some others might contain the full 256 bytes (for SysEx messages).

I am considering this especially for MPE (Multidimensional Polyphonic
Expression) MIDI protocol. On few benchmarks I did, a device that
implements this protocol generates around 500-2000 b/s of *raw* MIDI
data. And in practice only 4 (average MIDI message) * 32 (USB requests
defined by qlen) bytes will be used. Which means that the 8KB USB
request buffer will be under used.

So I think we have to treat the ALSA buffers and the USB request buffers
differently.

That's why I think this approach is fine by allowing the user to
increase that number of requests and its size if it needs to deal with a
higher throughput devices.

> 
> I think the safest approach would be to behave similar to the old driver,
> i.e., when the queue overflows, do nothing (not even dropping data), and
> rely on the transmit completion handler to continue.  (This implies that
> ALSA's buffer can fill up, and that snd_rawmidi_write() can block.)
> 

The previous implementation would not block, even though
snd_rawmidi_write() can block, because it was been created a new USB
request for each write call and data was been consumed even if this
request would not be enqueued to the endpoint.

But, anyway, I agree with your suggestion.

> 
> It you want to dequeue outdated data, I think this should be done with
> a timeout, i.e., when the host did not read anything for some tens of
> milliseconds or so.  This would be independent of the fill level of the
> queue, and could be done either for individual packets, or just on the
> entire endpoint queue.

That can be done. But I believe in another patch since it is not
required to work for this patch.

== Conclusion ==

Based on our conversation and your suggestions, I think that to just
ignore if an overrun occurs to the USB requests is fine. Upon completion
the request will be reused.
Important to note that if the overrun occurs, it will cause user-space
to block until a) the completion function is called successfully or b)
snd_rawmidi_write() times out. Which I think this is expected by ALSA users.

Does that make sense?

If yes then I will send the v6 of this patch.

Felipe


0x92698E6A.asc
Description: application/pgp-keys