Re: MIDI keyboard doesn't work when USB 3.0 controller is enabled in BIOS
Hi Karmo, On 02/01/17 15:57, Greg KH wrote: > On Mon, Jan 02, 2017 at 03:38:40PM +0200, Karmo Rosental wrote: >> I have tried kernels 4.8.0 and 4.9.0 from Ubuntu repositories. >> Probably I don't have latest BIOS. > > Try updating it and see if that resolves this issue. > It is very unlikely to be Linux since there was no changes in the USB host driver for MIDI neither ALSA recently. Looks like a BIOS issue. -- Felipe 0x92698E6A.asc Description: application/pgp-keys
Re: [alsa-devel] Jack sensing in snd_usb_audio ?
Hi Takashi On 18/10/16 13:07, Takashi Iwai wrote: > On Wed, 12 Oct 2016 18:15:04 +0200, > Bastien Nocera wrote: >> >> On Wed, 2016-10-12 at 18:06 +0200, Clemens Ladisch wrote: >>> Bastien Nocera wrote: On Wed, 2016-10-12 at 14:43 +0200, Clemens Ladisch wrote: > Bastien Nocera wrote: >> " >> A change of state in the audio function is most often caused by >> a >> certain event that takes place. An event can either be user- >> initiated >> or device-initiated. User-initiated jack insertion or removal >> is a >> typical example of a user-initiated event. >> " > > > There are not many USB Audio 2.0 devices, and I'm not aware of > any > that actually implements this. I guess I would see whether there are events if I captured the USB traffic even without special handling/turning on a feature in the drivers, right? >>> >>> >>> Most devices do not even have the status endpoint (see "lsusb -v"). >>> To check what events arrive, you can add logging to the >>> snd_usb_mixer_interrupt() function. >> >> I'm guessing it doesn't support it then (see attached log) > > So this looks like a HID, not from the audio device class. > It's an oft-seen implementation. > >> I also checked the input device output when plugging in something, with >> evtest, and no feedback either. > > Then at first you need to hack a HID driver to support this device. > It'll create an input device, and then we'll need to find some way to > couple the given input device and the audio device. We can parse the > sysfs device path to figure out, but I'm not sure what's the best way > to tell it to applications. > Why not use similar API as a normal ALSA card? This will enable jack detection by default in applications using kcontrol interface. -- Felipe 0x92698E6A.asc Description: application/pgp-keys
Re: Jack sensing in snd_usb_audio ?
Hi Bastien, On 12/10/16 12:58, Bastien Nocera wrote: > On Wed, 2016-10-12 at 19:36 +0900, Takashi Sakamoto wrote: >> On Oct 12 2016 14:10, Bastien Nocera wrote: >>> My questions are: >>> - does the USB audio driver support jack sensing? >>> - is this something standard that's just not implemented yet? In >>> which >>> case, I'd be up for at least trying, given specs. >>> - or is it something that depends on the device, and in which case, >>> how >>> would I find out? >> >> >> In ALSA usb-related codes, there's no functions calls for >> snd_jack_*(), >> thus none of ALSA drivers for USB support Jack sense feature of ALSA >> control interface. >> >> The requirement of Jack sense feature is whether hardwares support >> it. >> For example, some hardware codecs such as HDA codecs generates >> signals >> when plugs are insert to jacks connected to the codecs. Corresponding >> ALSA drivers catch the signals, then tell it to user land. >> >> If your hardware performs like it, you have a probability to add >> support >> for jack sense feature to ALSA drivers for USB. But I don't know >> exactly >> that USB related specifications such as USB Audio Device Class >> 1.0/2.0/3.0 supports the feature. > > Looks like whether or not jack sensing works depends on the device > itself, but there is a mechanism to propagate the change in setup in > the USB Audio 2.0 spec, in the "Interrupts" section: > " > A change of state in the audio function is most often caused by a > certain event that takes place. An event can either be user-initiated > or device-initiated. User-initiated jack insertion or removal is a > typical example of a user-initiated event. > " snd_usb_audio doesn't support control interrupts yet. It's a nice feature to work on. -- Felipe 0x92698E6A.asc Description: application/pgp-keys
Re: Jack sensing in snd_usb_audio ?
Hi Bastien, On 12/10/16 07:10, Bastien Nocera wrote: > Hey, > > I recently bought some cheap USB soundcards for a computer that doesn't > have any audio output other than through the HDMI output, and the > screen I'm attaching doesn't have an audio output. > > So I'm looking to plug in 2 of those USB soundcards, and switch between > them depending on whether I'm using headphones, or want to use the > standalone speaker. > > Obviously, it would be so much nicer if I didn't have to switch between > the outputs by hand, and ignored the "headphones" sound card when not > plugged in. > > My questions are: > - does the USB audio driver support jack sensing? > - is this something standard that's just not implemented yet? In which > case, I'd be up for at least trying, given specs. > - or is it something that depends on the device, and in which case, how > would I find out? What you need is PulseAudio server instead. PulseAudio supports this via kcontrol for quite some time. Jack is supposed to be a low-latency audio server for audio applications, not for normal desktop usage. > > Some details about the device itself below. > > Cheers > > /proc/asound/cards: > 4 [Device ]: USB-Audio - USB Audio Device > GeneralPlus USB Audio Device at usb-:00:14.0-9, > full speed > > $ amixer -c 4 > Simple mixer control 'Speaker',0 > Capabilities: pvolume pswitch pswitch-joined > Playback channels: Front Left - Front Right > Limits: Playback 0 - 30 > Mono: > Front Left: Playback 16 [53%] [-21.00dB] [on] > Front Right: Playback 16 [53%] [-21.00dB] [on] > Simple mixer control 'Mic',0 > Capabilities: pvolume pvolume-joined cvolume cvolume-joined pswitch > pswitch-joined cswitch cswitch-joined > Playback channels: Mono > Capture channels: Mono > Limits: Playback 0 - 14 Capture 0 - 30 > Mono: Playback 1 [7%] [-10.50dB] [off] Capture 26 [87%] [27.00dB] > [on] > Simple mixer control 'Auto Gain Control',0 > Capabilities: pswitch pswitch-joined > Playback channels: Mono > Mono: Playback [off] > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- Felipe 0x92698E6A.asc Description: application/pgp-keys
Re: [PATCH v4 08/10] usb: gadget: remove useless parameter in alloc_ep_req()
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()
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()
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()
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()
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
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
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
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
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()
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(&midi->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
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()
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(&midi->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 a/drivers/us
Re: [PATCH v3] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize
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: unlock on error
Hi Michal,
On 04/04/16 13:11, Michal Nazarewicz wrote:
> On Sat, Apr 02 2016, Dan Carpenter wrote:
>> We added some new locking here, but missed an error path where we need
>> to unlock.
>>
>> Fixes: 9acdf4df2fc4 ('usb: gadget: f_midi: added spinlock on transmit
>> function')
>> Signed-off-by: Dan Carpenter
>>
>
> Acked-by: Michal Nazarewicz
>
> But perhaps this would be cleaner:
>
> diff --git a/drivers/usb/gadget/function/f_midi.c
> b/drivers/usb/gadget/function/f_midi.c
> index 56e2dde..c04436f 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -606,19 +606,14 @@ static void f_midi_transmit(struct f_midi *midi)
> goto drop_out;
>
> spin_lock_irqsave(&midi->transmit_lock, flags);
> -
> - do {
> - ret = f_midi_do_transmit(midi, ep);
> - if (ret < 0)
> - goto drop_out;
> - } while (ret);
> -
> + while ((ret = f_midi_do_transmit(midi, ep)) > 0) {
> + /* nop */
> + }
> spin_unlock_irqrestore(&midi->transmit_lock, flags);
>
> - return;
> -
> + if (ret < 0)
> drop_out:
> - f_midi_drop_out_substreams(midi);
> + f_midi_drop_out_substreams(midi);
> }
>
> static void f_midi_in_tasklet(unsigned long data)
>
> Or maybe even this which gets away with gotos all together:
>
> diff --git a/drivers/usb/gadget/function/f_midi.c
> b/drivers/usb/gadget/function/f_midi.c
> index 56e2dde..91cae60 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -598,27 +598,20 @@ done:
> static void f_midi_transmit(struct f_midi *midi)
> {
> struct usb_ep *ep = midi->in_ep;
> - int ret;
> unsigned long flags;
> + int ret = -1;
>
> /* We only care about USB requests if IN endpoint is enabled */
> - if (!ep || !ep->enabled)
> - goto drop_out;
> -
> - spin_lock_irqsave(&midi->transmit_lock, flags);
> -
> - do {
> - ret = f_midi_do_transmit(midi, ep);
> - if (ret < 0)
> - goto drop_out;
> - } while (ret);
> -
> - spin_unlock_irqrestore(&midi->transmit_lock, flags);
> -
> - return;
> + if (ep && ep->enabled) {
> + spin_lock_irqsave(&midi->transmit_lock, flags);
> + while ((ret = f_midi_do_transmit(midi, ep)) > 0) {
> + /* nop */
> + }
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
> + }
>
> -drop_out:
> - f_midi_drop_out_substreams(midi);
> + if (ret < 0)
> + f_midi_drop_out_substreams(midi);
> }
>
> static void f_midi_in_tasklet(unsigned long data)
I am fine with these options, probably the second, but I don't think
they are any clearer than before, so I don't see any real benefits with
the changes.
In fact, I think f_midi_do_transmit should be documented, since there
are 3 possible return condition:
<0 breaks the loop and drop out substreams
0 breaks the loop
>0 continues the loop
>
>
>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>> b/drivers/usb/gadget/function/f_midi.c
>> index 56e2dde..2c0616c 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -609,8 +609,10 @@ static void f_midi_transmit(struct f_midi *midi)
>>
>> do {
>> ret = f_midi_do_transmit(midi, ep);
>> -if (ret < 0)
>> +if (ret < 0) {
>> +spin_unlock_irqrestore(&midi->transmit_lock, flags);
>> goto drop_out;
>> +}
>> } while (ret);
>>
>> spin_unlock_irqrestore(&midi->transmit_lock, flags);
>
--
Felipe
0x92698E6A.asc
Description: application/pgp-keys
Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
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: unlock on error
Hi Dan,
On 02/04/16 05:51, Dan Carpenter wrote:
> We added some new locking here, but missed an error path where we need
> to unlock.
>
> Fixes: 9acdf4df2fc4 ('usb: gadget: f_midi: added spinlock on transmit
> function')
> Signed-off-by: Dan Carpenter
Acked-by: Felipe F. Tonello
>
> diff --git a/drivers/usb/gadget/function/f_midi.c
> b/drivers/usb/gadget/function/f_midi.c
> index 56e2dde..2c0616c 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -609,8 +609,10 @@ static void f_midi_transmit(struct f_midi *midi)
>
> do {
> ret = f_midi_do_transmit(midi, ep);
> - if (ret < 0)
> + if (ret < 0) {
> + spin_unlock_irqrestore(&midi->transmit_lock, flags);
> goto drop_out;
> + }
> } while (ret);
>
> spin_unlock_irqrestore(&midi->transmit_lock, flags);
>
--
Felipe
0x92698E6A.asc
Description: application/pgp-keys
Re: [PATCH] usb: gadget: f_midi: Fixed a bug when buflen was smaller than wMaxPacketSize
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
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
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
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: Bug ID: 114111
Hi, On 09/03/16 17:12, Greg KH wrote: > On Wed, Mar 09, 2016 at 05:47:00PM +0100, Peter Maciejko wrote: >> https://bugzilla.kernel.org/show_bug.cgi?id=114111 >> >> Bug ID: 114111 >> Summary: Reboot after shutdown, when USB 3.0 controller is enabled > > Please send us the full details so we don't have to dig them out on the > website... There is absolutely no description of the bug besides the Summary, which by itself is not saying much. Peter, can you describe exactly was is going on? -- Felipe 0x92698E6A.asc Description: application/pgp-keys
Re: [PATCH] usb: gadget: f_midi: added spinlock on transmit function
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(&midi->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(&midi->in_req_fifo, &req);
> if (len != 1) {
> ERROR(midi, "%s: Couldn't get usb request\n", __func__);
> + spin_unlock_irqrestore(&midi->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(&midi->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(&midi->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(&midi->transmit_lock);
> +
> ++opts->refcnt;
> mutex_unlock(&opts->lock);
>
>
--
Felipe
0x92698E6A.asc
Description: application/pgp-keys
Re: [PATCH] usb: gadget: f_midi: added spinlock on transmit function
Hi Balbi,
On 09/03/16 10:33, Felipe Balbi wrote:
>
> Hi,
>
> Felipe Ferreri Tonello writes:
>> [ text/plain ]
>> Hi Balbi,
>>
>> On 09/03/16 07:20, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> "Felipe F. Tonello" writes:
>>>> [ text/plain ]
>>>> 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.4+
>>>
>>> this should be v4.5+
>>>
>>> $ git describe e1e3d7ec5da3
>>> v4.4-rc5-59-ge1e3d7ec5da3
>>>
>>>> @@ -651,17 +653,22 @@ static void f_midi_transmit(struct f_midi *midi)
>>>> {
>>>>struct usb_ep *ep = midi->in_ep;
>>>>int ret;
>>>> + unsigned long flags;
>>>>
>>>>/* We only care about USB requests if IN endpoint is enabled */
>>>>if (!ep || !ep->enabled)
>>>>goto drop_out;
>>>>
>>>> + spin_lock_irqsave(&midi->transmit_lock, flags);
>>>> +
>>>>do {
>>>>ret = f_midi_do_transmit(midi, ep);
>>>
>>> you wrote this commit on top of 'next' right ? I know that because of
>>> this call to f_midi_do_transmit() here which is introduced by commit
>>> aac887442d5e ("usb: gadget: f_midi: move some of f_midi_transmit to
>>> separate func") which is not in Linus' tree yet.
>>>
>>> This prevents me from taking this patch during current -rc.
>>
>> Yes, but then what should I do? Because if I patch on Linus' tree, then
>> the patch won't apply to your tree.
>
> it should apply to my "fixes" branch where fixes for current -rc should
> go :-) Note that v4.5-rc7 doesn't have f_midi_do_transmit()
Right, but then you will have to fix the merge conflicts by hand. :(
--
Felipe
0x92698E6A.asc
Description: application/pgp-keys
Re: [PATCH] usb: gadget: f_midi: refactor state machine
Hi Balbi,
On 09/03/16 07:04, Felipe Balbi wrote:
>
> Hi,
>
> "Felipe F. Tonello" writes:
>> [ text/plain ]
>> This refactor results in a cleaner state machine code and promotes
>> consistency, readability, and maintanability of this driver.
>>
>> Signed-off-by: Felipe F. Tonello
>
> while working on this driver ...
>
>> ---
>> drivers/usb/gadget/function/f_midi.c | 204
>> ++-
>> 1 file changed, 129 insertions(+), 75 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>> b/drivers/usb/gadget/function/f_midi.c
>> index 84c0ee5ebd1e..3cdb0741f3f8 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -50,6 +50,19 @@ static const char f_midi_longname[] = "MIDI Gadget";
>> */
>> #define MAX_PORTS 16
>>
>> +/* MIDI message states */
>> +enum {
>> +STATE_INITIAL = 0, /* pseudo state */
>> +STATE_1PARAM,
>> +STATE_2PARAM_1,
>> +STATE_2PARAM_2,
>> +STATE_SYSEX_0,
>> +STATE_SYSEX_1,
>> +STATE_SYSEX_2,
>> +STATE_REAL_TIME,
>> +STATE_FINISHED, /* pseudo state */
>> +};
>> +
>> /*
>> * This is a gadget, and the IN/OUT naming is from the host's perspective.
>> * USB -> OUT endpoint -> rawmidi
>> @@ -60,13 +73,6 @@ struct gmidi_in_port {
>> int active;
>> uint8_t cable;
>> uint8_t state;
>> -#define STATE_UNKNOWN 0
>> -#define STATE_1PARAM1
>> -#define STATE_2PARAM_1 2
>> -#define STATE_2PARAM_2 3
>> -#define STATE_SYSEX_0 4
>> -#define STATE_SYSEX_1 5
>> -#define STATE_SYSEX_2 6
>> uint8_t data[2];
>> };
>>
>> @@ -400,118 +406,166 @@ static int f_midi_snd_free(struct snd_device *device)
>> return 0;
>> }
>>
>> -static void f_midi_transmit_packet(struct usb_request *req, uint8_t p0,
>> -uint8_t p1, uint8_t p2, uint8_t p3)
>> -{
>> -unsigned length = req->length;
>> -u8 *buf = (u8 *)req->buf + length;
>> -
>> -buf[0] = p0;
>> -buf[1] = p1;
>> -buf[2] = p2;
>> -buf[3] = p3;
>> -req->length = length + 4;
>> -}
>> -
>> /*
>> * 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)
>
> ... could you get rid of the userspace types (as a separate patch, of
> course) ?
userspace types?
>
>> {
>> -uint8_t p0 = port->cable << 4;
>> +uint8_t p[4] = { port->cable << 4, 0, 0, 0 };
>> +uint8_t next_state = STATE_INITIAL;
>
> and here too. I know you're going for consistency, but maybe it makes
> sense to clean up the types before you cleanup the state machine.
What do you mean by types? Do you mean to convert the state macros to an
enum?
>
> [...]
>
>> +/* States where we have to write into the USB request */
>> +if (next_state == STATE_FINISHED ||
>> +port->state == STATE_SYSEX_2 ||
>> +port->state == STATE_1PARAM ||
>> +port->state == STATE_2PARAM_2 ||
>> +port->state == STATE_REAL_TIME) {
>> +
>> +unsigned length = req->length;
>> +u8 *buf = (u8 *)req->buf + length;
>> +
>> +memcpy(buf, p, sizeof(p));
>> +req->length = length + sizeof(p);
>> +
>> +if (next_state == STATE_FINISHED) {
>> +next_state = STATE_INITIAL;
>> +port->data[0] = port->data[1] = 0;
>> +}
>> }
>> +
>> +port->state = next_state;
>
> okay, so this function will be called from ->complete() which is called
> without controller lock held but with IRQs disabled. I wonder if you're
> racing port->state here with this change. Could race on SMP, no ?
Yes it will race because this function is not thread-safe. BTW, it would
race anyway even without my patch.
That's why we need that spin lock patch.
It goes like this:
USB request complete() or ALSA transmit():
f_midi_transmit()
spin lock with irq
call this refactored function()
spin unlock with irq
return
return
--
Felipe
0x92698E6A.asc
Description: application/pgp-keys
Re: [PATCH] usb: gadget: f_midi: added spinlock on transmit function
Hi Balbi,
On 09/03/16 07:20, Felipe Balbi wrote:
>
> Hi,
>
> "Felipe F. Tonello" writes:
>> [ text/plain ]
>> 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.4+
>
> this should be v4.5+
>
> $ git describe e1e3d7ec5da3
> v4.4-rc5-59-ge1e3d7ec5da3
>
>> @@ -651,17 +653,22 @@ static void f_midi_transmit(struct f_midi *midi)
>> {
>> struct usb_ep *ep = midi->in_ep;
>> int ret;
>> +unsigned long flags;
>>
>> /* We only care about USB requests if IN endpoint is enabled */
>> if (!ep || !ep->enabled)
>> goto drop_out;
>>
>> +spin_lock_irqsave(&midi->transmit_lock, flags);
>> +
>> do {
>> ret = f_midi_do_transmit(midi, ep);
>
> you wrote this commit on top of 'next' right ? I know that because of
> this call to f_midi_do_transmit() here which is introduced by commit
> aac887442d5e ("usb: gadget: f_midi: move some of f_midi_transmit to
> separate func") which is not in Linus' tree yet.
>
> This prevents me from taking this patch during current -rc.
Yes, but then what should I do? Because if I patch on Linus' tree, then
the patch won't apply to your tree.
--
Felipe
0x92698E6A.asc
Description: application/pgp-keys
Re: [PATCH 2/5] usb: gadget: f_midi: added spinlock on transmit function
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'
Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes
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
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
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
Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes
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
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
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
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
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
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. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes
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. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] usb: gadget: f_midi: cleanups and typos fixes
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;
>> +out_ex
Re: [PATCH 2/5] usb: gadget: f_midi: added spinlock on transmit function
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. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] usb: gadget: gmidi: remove bus powered requirement on bmAttributes
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] MIDI USB Gadget improvements
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. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] usb: gadget: f_midi: updated copyright
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. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine
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. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] usb: gadget: f_midi: refactor state machine
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
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
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] usb: gadget: f_midi: missing unlock on error path
Hi Dan, On 05/01/16 12:44, Dan Carpenter wrote: > On Tue, Jan 05, 2016 at 01:28:11PM +0100, Julia Lawall wrote: >> >> >> On Tue, 5 Jan 2016, kbuild test robot wrote: >> >>> Hi Dan, >>> >>> [auto build test WARNING on balbi-usb/next] >>> [also build test WARNING on v4.4-rc8 next-20160105] >>> [if your patch is applied to the wrong git tree, please drop us a note to >>> help improving the system] >>> >>> url: >>> https://github.com/0day-ci/linux/commits/Dan-Carpenter/usb-gadget-f_midi-missing-unlock-on-error-path/20160105-183115 >>> base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next >>> >>> >>> coccinelle warnings: (new ones prefixed by >>) >>> > drivers/usb/gadget/function/f_midi.c:1233:14-21: ERROR: midi is NULL but > dereferenced. >> >> It's a false positive for coccinelle, but I wonder if avoiding duplicating >> the mutex_lock is really worth it? > > It's not the most beautiful code in the world. I considered a bunch of > different ways to write it... This is what Felipe Tonello wanted, > though. This case is not a matter of been pretty but a matter of been less error prone. What would you suggest? Thanks, Felipe 0x92698E6A.asc Description: application/pgp-keys
Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine
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
-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] usb: gadget: f_midi: missing unlock on error path
Hi Dan,
On 21/12/15 13:20, Dan Carpenter wrote:
> We added a new error path to this function and we forgot to drop the
> lock.
>
> Fixes: e1e3d7ec5da3 ('usb: gadget: f_midi: pre-allocate IN requests')
> Signed-off-by: Dan Carpenter
>
> diff --git a/drivers/usb/gadget/function/f_midi.c
> b/drivers/usb/gadget/function/f_midi.c
> index fb1fe96d..92b9ec8 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -1213,8 +1213,10 @@ static struct usb_function *f_midi_alloc(struct
> usb_function_instance *fi)
> midi->in_last_port = 0;
>
> status = kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL);
> - if (status)
> + if (status) {
> + mutex_unlock(&opts->lock);
This is fine. But looking into this further, I believe this mutex_unlock
should be under the setup_fail label, because even if the alloc() fails,
it will be need to unlock the mutex in any circumstance anyway.
> goto setup_fail;
> + }
>
> ++opts->refcnt;
> mutex_unlock(&opts->lock);
>
Felipe
0x92698E6A.asc
Description: application/pgp-keys
Re: [PATCH v3 01/36] Documentation: usb: update usb-tools repository address
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
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
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
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(&midi->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(&midi->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(&midi->in_req_fifo, &req))
> + 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"
Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests
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(&midi->in_req_fifo, &req);
>>>> + ...
>>>> + 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
Hi Clemens
On 13/11/15 08:55, Clemens Ladisch wrote:
> 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.
>
>> ...
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -88,6 +89,9 @@ struct f_midi {
>> int index;
>> char *id;
>> unsigned int buflen, qlen;
>> +DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
>> +unsigned int in_req_num;
>> +unsigned int in_last_port;
>
> As far as I can see, in_req_num must always have the same value as qlen.
Yes, I've removed in_req_num.
>
>> @@ -366,6 +388,20 @@ static void f_midi_disable(struct usb_function *f)
>> +while (!kfifo_is_empty(&midi->in_req_fifo)) {
>> +...
>> +len = kfifo_get(&midi->in_req_fifo, &req);
>> +if (len == 1)
>> +free_ep_req(midi->in_ep, req);
>> +else
>> +ERROR(midi, "%s couldn't free usb request: something
>> went wrong with kfifo\n",
>> + midi->in_ep->name);
>> +}
>
> kfifo_get() already checks for the FIFO being empty, so you can just
> drop kfifi_is_empty().
Ok. I've simplified this code. I wasn't really happy with it either.
>
>> @@ -487,57 +523,111 @@ static void f_midi_transmit_byte(struct usb_request
>> *req,
>> ...
>> +static void f_midi_transmit(struct f_midi *midi)
>> +{
>> +...
>> +len = kfifo_peek(&midi->in_req_fifo, &req);
>> +...
>> +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.
>
> This code shows the error message for case 1), but does the action for
> case 2).
>
> 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.
Agree. It would be useful for users to know about case 1), but like you
said it is probably not worthwhile to do to so.
>
>> @@ -1130,6 +1222,12 @@ static struct usb_function *f_midi_alloc(struct
>> usb_function_instance *fi)
>> +if (kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL))
>> +goto setup_fail;
>
> The setup_fail code expects an error code in the status variable.
Done.
Felipe
0x92698E6A.asc
Description: application/pgp-keys
Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests
Hi Robert,
On 13/11/15 12:38, Robert Baldyga wrote:
> Hi Felipe,
>
> On 11/10/2015 06:52 PM, 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 | 174
>> +++
>> drivers/usb/gadget/legacy/gmidi.c| 2 +-
>> 2 files changed, 137 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>> b/drivers/usb/gadget/function/f_midi.c
>> index 615d632..6ac39f6 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;
>> +DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
>> +unsigned int in_req_num;
>> +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,21 @@ 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(&midi->in_req_fifo)) {
>> +struct usb_request *req =
>> +midi_alloc_ep_req(midi->in_ep, midi->buflen);
>> +
>> +if (req == NULL)
>> +return -ENOMEM;
>
> We need to free previously allocated requests in case of failure.
>
>> +
>> +req->length = 0;
>> +req->complete = f_midi_complete;
>> +
>> +kfifo_put(&midi->in_req_fifo, req);
>> +midi->in_req_num++;
>> +}
>> +
>> /* 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 =
>> @@ -366,6 +388,20 @@ 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_is_empty(&midi->in_req_fifo)) {
>> +struct usb_request *req = NULL;
>> +unsigned int len;
>> +
>> +len = kfifo_get(&midi->in_req_fifo, &req);
>> +if (len == 1)
>> +free_ep_req(midi->in_ep, req);
>> +else
>> +ERROR(midi, "%s couldn't free usb request: something
>> went wrong with kfifo\n",
>> + midi->in_ep->name);
>> +}
>> +midi->in_req_num = 0;
>> }
>>
>> static int f_midi_snd_free(struct snd_device *device)
>> @@ -487,57 +523,111 @@ static void f_midi_transmit_byte(struct usb_request
>> *req,
>> }
>> }
>>
>> -static void f_midi_transmit(struct f_midi *midi, struct usb_request
Re: [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue out requests
Hi Robert,
On 16/11/15 11:43, Robert Baldyga wrote:
> On 11/16/2015 12:08 PM, Felipe Ferreri Tonello wrote:
>> Hi Robert,
>>
>> On 13/11/15 08:31, Robert Baldyga wrote:
>>> Hi Felipe,
>>>
>>> On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
>>>> This patch fixes a memory leak that occurs when an endpoint fails to
>>>> enqueue
>>>> the request. If that happens the complete function will never be called,
>>>> thus
>>>> never freeing the request.
>>>>
>>>> Signed-off-by: Felipe F. Tonello
>>>> ---
>>>> drivers/usb/gadget/function/f_midi.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_midi.c
>>>> b/drivers/usb/gadget/function/f_midi.c
>>>> index f36db2d..76ea53c 100644
>>>> --- a/drivers/usb/gadget/function/f_midi.c
>>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>>> @@ -345,6 +345,7 @@ static int f_midi_set_alt(struct usb_function *f,
>>>> unsigned intf, unsigned alt)
>>>>if (err) {
>>>>ERROR(midi, "%s queue req: %d\n",
>>>>midi->out_ep->name, err);
>>>> + free_ep_req(midi->out_ep, req);
>>>>}
>>>>}
>>>>
>>>>
>>>
>>> There is one more thing I haven't noticed before. We can have situation
>>> when all requests were allocated successfully, but their allocation
>>> failed. What we get then is set_alt() returning 0, while no request is
>>> allocated, hence the function is, in fact, inactive.
>>
>> Right. So in this case should we return some error? We can restrict the
>> function to work iff allocates the 'qlen' number of allocations,
>> otherwise returns an error and frees all other requests (IN and OUT).
>>
>
> Yes, IMO it's a proper solution. When user sets qlen to given value he
> expects that exact number of requests to be allocated and enqueued, and
> if we cannot do that we should consider this as an error.
>
Ok. I will do that in a new patch on v6 since this patch is correct
anyway and Blabi already applied to his test branch.
Felipe
0x92698E6A.asc
Description: application/pgp-keys
Re: [PATCH v5 6/7] usb: gadget: f_midi: set altsettings only for MIDIStreaming interface
Hi Clemens,
On 13/11/15 08:11, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 10/11/15 18:43, Sergei Shtylyov wrote:
>>> On 11/10/2015 08:52 PM, Felipe F. Tonello wrote:
>>>> @@ -75,6 +75,7 @@ struct f_midi {
>>>> struct usb_ep*in_ep, *out_ep;
>>>> struct snd_card*card;
>>>> struct snd_rawmidi*rmidi;
>>>> +u8ms_id;
>>>
>>> Why 'ms_id' is not aligned with the above field names?
>>
>> It is actually aligned.
>
> It's not in the original mail, which contains tab characters.
>
>> Here is from my local git diff:
>>
>> @@ -75,6 +75,7 @@ struct f_midi {
>> struct usb_ep *in_ep, *out_ep;
>> struct snd_card *card;
>> struct snd_rawmidi *rmidi;
>> + u8 ms_id;
>
> Apparently, you're using four spaces per tab. Linux uses eight.
Yes. I'll fix that.
--
Felipe
0x92698E6A.asc
Description: application/pgp-keys
Re: [PATCH v5 4/7] usb: gadget: f_midi: fix leak on failed to enqueue out requests
Hi Robert,
On 13/11/15 08:31, Robert Baldyga wrote:
> Hi Felipe,
>
> On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
>> This patch fixes a memory leak that occurs when an endpoint fails to enqueue
>> the request. If that happens the complete function will never be called, thus
>> never freeing the request.
>>
>> Signed-off-by: Felipe F. Tonello
>> ---
>> drivers/usb/gadget/function/f_midi.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>> b/drivers/usb/gadget/function/f_midi.c
>> index f36db2d..76ea53c 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -345,6 +345,7 @@ static int f_midi_set_alt(struct usb_function *f,
>> unsigned intf, unsigned alt)
>> if (err) {
>> ERROR(midi, "%s queue req: %d\n",
>> midi->out_ep->name, err);
>> +free_ep_req(midi->out_ep, req);
>> }
>> }
>>
>>
>
> There is one more thing I haven't noticed before. We can have situation
> when all requests were allocated successfully, but their allocation
> failed. What we get then is set_alt() returning 0, while no request is
> allocated, hence the function is, in fact, inactive.
Right. So in this case should we return some error? We can restrict the
function to work iff allocates the 'qlen' number of allocations,
otherwise returns an error and frees all other requests (IN and OUT).
--
Felipe
0x92698E6A.asc
Description: application/pgp-keys
Re: [PATCH v5 6/7] usb: gadget: f_midi: set altsettings only for MIDIStreaming interface
Hi Sergei,
On 10/11/15 18:43, Sergei Shtylyov wrote:
> Hello.
>
> On 11/10/2015 08:52 PM, Felipe F. Tonello wrote:
>
>> This avoids duplication of USB requests for OUT endpoint and
>> re-enabling endpoints.
>>
>> 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 76ea53c..615d632 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -75,6 +75,7 @@ struct f_midi {
>> struct usb_ep*in_ep, *out_ep;
>> struct snd_card*card;
>> struct snd_rawmidi*rmidi;
>> +u8ms_id;
>
> Why 'ms_id' is not aligned with the above field names?
It is actually aligned. Perhaps because of the +?
Here is from my local git diff:
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -75,6 +75,7 @@ struct f_midi {
struct usb_ep *in_ep, *out_ep;
struct snd_card *card;
struct snd_rawmidi *rmidi;
+ u8 ms_id;
struct snd_rawmidi_substream *in_substream[MAX_PORTS];
struct snd_rawmidi_substream *out_substream[MAX_PORTS];
--
Felipe
0x92698E6A.asc
Description: application/pgp-keys
Re: [PATCH 04/23] usb: gadget: f_loopback: free requests in loopback_disable()
Hi Robert,
On 03/11/15 12:53, Robert Baldyga wrote:
> USB requests in Loopback function are allocated in loopback_get_alt()
> function, so we prefer to free them rather in loopback_disable() than
> in loopback_complete() when request is completed with error. It provides
> better symetry in resource management and improves code readability.
Are we doing this for all functions? Because I see that several
functions does the same thing, they free IN/OUT ep requests on
complete() instead of disable().
I also prefer this, but then we need to refactor most function code.
>
> Signed-off-by: Robert Baldyga
> ---
> drivers/usb/gadget/function/f_loopback.c | 58
> +---
> 1 file changed, 23 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_loopback.c
> b/drivers/usb/gadget/function/f_loopback.c
> index 6b2102b..41464ae 100644
> --- a/drivers/usb/gadget/function/f_loopback.c
> +++ b/drivers/usb/gadget/function/f_loopback.c
> @@ -35,6 +35,9 @@ struct f_loopback {
> struct usb_ep *in_ep;
> struct usb_ep *out_ep;
>
> + struct usb_request *in_req;
> + struct usb_request *out_req;
> +
> unsignedqlen;
> unsignedbuflen;
> };
> @@ -249,30 +252,25 @@ static void loopback_complete(struct usb_ep *ep, struct
> usb_request *req)
>* We received some data from the host so let's
>* queue it so host can read the from our in ep
>*/
> - struct usb_request *in_req = req->context;
> -
> - in_req->zero = (req->actual < req->length);
> - in_req->length = req->actual;
> + loop->in_req->zero = (req->actual < req->length);
> + loop->in_req->length = req->actual;
> + req = loop->in_req;
> ep = loop->in_ep;
> - req = in_req;
> } else {
> /*
>* We have just looped back a bunch of data
>* to host. Now let's wait for some more data.
>*/
> - req = req->context;
> + req = loop->out_req;
> ep = loop->out_ep;
> }
>
> /* queue the buffer back to host or for next bunch of data */
> status = usb_ep_queue(ep, req, GFP_ATOMIC);
> - if (status == 0) {
> - return;
> - } else {
> + if (status < 0)
> ERROR(cdev, "Unable to loop back buffer to %s: %d\n",
> ep->name, status);
> - goto free_req;
> - }
> + break;
>
> /* "should never get here" */
> default:
> @@ -280,20 +278,10 @@ static void loopback_complete(struct usb_ep *ep, struct
> usb_request *req)
> status, req->actual, req->length);
> /* FALLTHROUGH */
>
> - /* NOTE: since this driver doesn't maintain an explicit record
> - * of requests it submitted (just maintains qlen count), we
> - * rely on the hardware driver to clean up on disconnect or
> - * endpoint disable.
> - */
> case -ECONNABORTED: /* hardware forced ep reset */
> case -ECONNRESET: /* request dequeued */
> case -ESHUTDOWN:/* disconnect from host */
> -free_req:
> - usb_ep_free_request(ep == loop->in_ep ?
> - loop->out_ep : loop->in_ep,
> - req->context);
> - free_ep_req(ep, req);
> - return;
> + break;
> }
> }
>
> @@ -316,7 +304,6 @@ static inline struct usb_request *lb_alloc_ep_req(struct
> usb_ep *ep, int len)
> static int alloc_requests(struct usb_composite_dev *cdev,
> struct f_loopback *loop)
> {
> - struct usb_request *in_req, *out_req;
> int i;
> int result = 0;
>
> @@ -329,23 +316,21 @@ static int alloc_requests(struct usb_composite_dev
> *cdev,
> for (i = 0; i < loop->qlen && result == 0; i++) {
> result = -ENOMEM;
>
> - in_req = usb_ep_alloc_request(loop->in_ep, GFP_KERNEL);
> - if (!in_req)
> + loop->in_req = usb_ep_alloc_request(loop->in_ep, GFP_KERNEL);
> + if (!loop->in_req)
> goto fail;
>
> - out_req = lb_alloc_ep_req(loop->out_ep, 0);
> - if (!out_req)
> + loop->out_req = lb_alloc_ep_req(loop->out_ep, 0);
> + if (!loop->out_req)
> goto fail_in;
>
> - in_req->complete = loopback_complete;
> - out_req->complete = loopback_complete;
> + loop->in_req->complete = loopback_
Re: [PATCH v4 0/7] USB MIDI Gadget improvements and bug fixes
Hi Balbi, On 26/10/15 16:55, Felipe F. Tonello wrote: > Patch 7 has changes on how to transmit IN USB requests. It implements a FIFO > of pre-allocated usb requests and uses then as needed, instead of allocating > then on demand. This is my initial implementation and is open for > suggestions and comments. > > Patches 1-6 is pretty much straight forward. > > 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 (7): > usb: gadget: f_midi: Transmit data only when IN ep is enabled > usb: gadget: f_midi: remove duplicated code > usb: gadget: define free_ep_req as universal function > usb: gadget: f_midi: fix leak on failed to enqueue out requests > usb: gadget: f_midi: set altsettings only for MIDIStreaming interface > usb: gadget: gmidi: Cleanup legacy code > usb: gadget: f_midi: pre-allocate IN requests > > drivers/usb/gadget/function/f_midi.c | 201 > - > drivers/usb/gadget/function/f_sourcesink.c | 6 - > drivers/usb/gadget/function/g_zero.h | 1 - > drivers/usb/gadget/legacy/gmidi.c | 12 +- > drivers/usb/gadget/u_f.c | 8 ++ > drivers/usb/gadget/u_f.h | 3 +- > 6 files changed, 151 insertions(+), 80 deletions(-) > I have rebased this patchset on top of your next branch. It removes the need for patch 1 and patch 5. I am waiting for more comments on other patches to fix things if needed before sending v5. -- Felipe -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/7] usb: gadget: define free_ep_req as universal function
Hi Robert,
On 27/10/15 09:47, Robert Baldyga wrote:
> On 10/27/2015 10:18 AM, Felipe Ferreri Tonello wrote:
>> Hi Robert,
>>
>> On 27/10/15 06:53, Robert Baldyga wrote:
>>> On 10/26/2015 05:55 PM, Felipe F. Tonello wrote:
>>>> This function is shared between gadget functions, so this avoid unnecessary
>>>> duplicated code and potentially avoid memory leaks.
>>>>
>>>> Signed-off-by: Felipe F. Tonello
>>>> ---
>>>> drivers/usb/gadget/function/f_midi.c | 6 --
>>>> drivers/usb/gadget/function/f_sourcesink.c | 6 --
>>>> drivers/usb/gadget/function/g_zero.h | 1 -
>>>> drivers/usb/gadget/u_f.c | 8
>>>> drivers/usb/gadget/u_f.h | 3 +--
>>>> 5 files changed, 9 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_midi.c
>>>> b/drivers/usb/gadget/function/f_midi.c
>>>> index c19f154..4c01c8a 100644
>>>> --- a/drivers/usb/gadget/function/f_midi.c
>>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>>> @@ -202,12 +202,6 @@ static inline struct usb_request
>>>> *midi_alloc_ep_req(struct usb_ep *ep,
>>>>return alloc_ep_req(ep, length, length);
>>>> }
>>>>
>>>> -static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
>>>> -{
>>>> - kfree(req->buf);
>>>> - usb_ep_free_request(ep, req);
>>>> -}
>>>> -
>>>> static const uint8_t f_midi_cin_length[] = {
>>>>0, 0, 2, 3, 3, 1, 2, 3, 3, 3, 3, 3, 2, 2, 3, 1
>>>> };
>>>> diff --git a/drivers/usb/gadget/function/f_sourcesink.c
>>>> b/drivers/usb/gadget/function/f_sourcesink.c
>>>> index 3a5ae99..eedea7f 100644
>>>> --- a/drivers/usb/gadget/function/f_sourcesink.c
>>>> +++ b/drivers/usb/gadget/function/f_sourcesink.c
>>>> @@ -307,12 +307,6 @@ static inline struct usb_request
>>>> *ss_alloc_ep_req(struct usb_ep *ep, int len)
>>>>return alloc_ep_req(ep, len, buflen);
>>>> }
>>>>
>>>> -void free_ep_req(struct usb_ep *ep, struct usb_request *req)
>>>> -{
>>>> - kfree(req->buf);
>>>> - usb_ep_free_request(ep, req);
>>>> -}
>>>> -
>>>> static void disable_ep(struct usb_composite_dev *cdev, struct usb_ep *ep)
>>>> {
>>>>int value;
>>>> diff --git a/drivers/usb/gadget/function/g_zero.h
>>>> b/drivers/usb/gadget/function/g_zero.h
>>>> index 15f1809..5ed90b4 100644
>>>> --- a/drivers/usb/gadget/function/g_zero.h
>>>> +++ b/drivers/usb/gadget/function/g_zero.h
>>>> @@ -59,7 +59,6 @@ void lb_modexit(void);
>>>> int lb_modinit(void);
>>>>
>>>> /* common utilities */
>>>> -void free_ep_req(struct usb_ep *ep, struct usb_request *req);
>>>> void disable_endpoints(struct usb_composite_dev *cdev,
>>>>struct usb_ep *in, struct usb_ep *out,
>>>>struct usb_ep *iso_in, struct usb_ep *iso_out);
>>>> diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
>>>> index c6276f0..f78bd1f 100644
>>>> --- a/drivers/usb/gadget/u_f.c
>>>> +++ b/drivers/usb/gadget/u_f.c
>>>> @@ -14,6 +14,7 @@
>>>> #include
>>>> #include "u_f.h"
>>>>
>>>> +/* Requests allocated via alloc_ep_req() must be freed by free_ep_req().
>>>> */
>>>> struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int
>>>> default_len)
>>>> {
>>>>struct usb_request *req;
>>>> @@ -30,3 +31,10 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int
>>>> len, int default_len)
>>>>return req;
>>>> }
>>>> EXPORT_SYMBOL_GPL(alloc_ep_req);
>>>> +
>>>> +void free_ep_req(struct usb_ep *ep, struct usb_request *req)
>>>> +{
>>>> + kfree(req->buf);
>>>> + usb_ep_free_request(ep, req);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(free_ep_req);
>>>> diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
>>>> index 1d5f0eb..2a1a6fb 100644
>>>> --- a/drivers/usb/gadget/u_f.h
>>>> +++ b/drivers/usb/gadget/u_f.h
>>>> @@ -46,7 +46,6 @@ struct usb_ep;
>>>> struct usb_request;
>>>>
>>>> struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int
>>>> default_len);
>>>> +void free_ep_req(struct usb_ep *ep, struct usb_request *req);
>>>>
>>>> #endif /* __U_F_H__ */
>>>> -
>>>> -
>>>>
>>>
>>> Isn't it simple enough to be static inline?
>>
>> inline yes. And the compiler will do it automatically. But I can add it
>> for clarity.
>
> No, compiler will never make function inline when you export its symbol.
> To make it inline you should place it in header.
Correct.
I will improve it on next revision.
--
Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] usb: chipidea: udc: improve error handling on ep_queue
Hi Peter,
Have you seen this patch? I saw that you didn't apply it to your tree,
so I wonder if it is good or do I have to change anything.
This patch is a bug fix for a memory leak, so it is quite important.
--
Felipe
On 18/09/15 18:30, e...@felipetonello.com wrote:
> From: "Felipe F. Tonello"
>
> _ep_queue() didn't check for errors when using add_td_to_list()
> which can fail if dma_pool_alloc fails, thus causing a kernel
> panic when lastnode->ptr is NULL.
>
> Signed-off-by: Felipe F. Tonello
> ---
>
> Changes for v2:
> - Use separate patch for cleanups.
>
> drivers/usb/chipidea/udc.c | 18 +-
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index c936c72..7169113e 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -435,19 +435,27 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep,
> struct ci_hw_req *hwreq)
> if (hwreq->req.dma % PAGE_SIZE)
> pages--;
>
> - if (rest == 0)
> - add_td_to_list(hwep, hwreq, 0);
> + if (rest == 0) {
> + ret = add_td_to_list(hwep, hwreq, 0);
> + if (ret < 0)
> + goto done;
> + }
>
> while (rest > 0) {
> unsigned count = min(hwreq->req.length - hwreq->req.actual,
> (unsigned)(pages * CI_HDRC_PAGE_SIZE));
> - add_td_to_list(hwep, hwreq, count);
> + ret = add_td_to_list(hwep, hwreq, count);
> + if (ret < 0)
> + goto done;
> rest -= count;
> }
>
> if (hwreq->req.zero && hwreq->req.length
> - && (hwreq->req.length % hwep->ep.maxpacket == 0))
> - add_td_to_list(hwep, hwreq, 0);
> + && (hwreq->req.length % hwep->ep.maxpacket == 0)) {
> + ret = add_td_to_list(hwep, hwreq, 0);
> + if (ret < 0)
> + goto done;
> + }
>
> firstnode = list_first_entry(&hwreq->tds, struct td_node, td);
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/7] usb: gadget: f_midi: Transmit data only when IN ep is enabled
Hi Robert,
On 27/10/15 06:41, Robert Baldyga wrote:
> On 10/26/2015 11:49 PM, Felipe Tonello wrote:
>> Hi Robert,
>>
>> On Mon, Oct 26, 2015 at 10:13 PM, Robert Baldyga
>> wrote:
>>> Hi Felipe,
>>>
>>> On 10/26/2015 05:55 PM, Felipe F. Tonello wrote:
This makes sure f_midi doesn't try to enqueue data when the IN endpoint is
disabled, ie, USB cable is disconnected.
Signed-off-by: Felipe F. Tonello
---
drivers/usb/gadget/function/f_midi.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_midi.c
b/drivers/usb/gadget/function/f_midi.c
index edb84ca..e08f365 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -87,6 +87,7 @@ struct f_midi {
int index;
char *id;
unsigned int buflen, qlen;
+ bool in_ep_enabled;
>>>
>>> It's not necessary, you can use ep->enabled flag instead.
>>
>> There is no such flag in usb_ep struct[1].
>>
>> [1] http://lxr.free-electrons.com/source/include/linux/usb/gadget.h#L170
>
> It's already in next branch of Felipe Balbi's tree.
>
> Look here:
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/
Cool. Thanks.
I though that this flag would be very useful, but didn't want to add to
the main struct as it seems no other driver cared about this flag. But
it is good to see that it is been merged now.
--
Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/7] usb: gadget: f_midi: set altsettings only for MIDIStreaming interface
Hi Robert, On 27/10/15 06:47, Robert Baldyga wrote: > On 10/26/2015 11:53 PM, Felipe Tonello wrote: >> Hi Robert, >> >> On Mon, Oct 26, 2015 at 10:30 PM, Robert Baldyga >> wrote: >>> On 10/26/2015 05:55 PM, Felipe F. Tonello wrote: This avoids duplication of USB requests for OUT endpoint and re-enabling endpoints. Signed-off-by: Felipe F. Tonello --- drivers/usb/gadget/function/f_midi.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c index 0e9cdeb..a617df3 100644 --- a/drivers/usb/gadget/function/f_midi.c +++ b/drivers/usb/gadget/function/f_midi.c @@ -323,6 +323,10 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt) unsigned i; int err; + /* We don't care if it is not MIDIStreaming interface */ + if (intf != ms_interface_desc.bInterfaceNumber) + return 0; + >>> >>> These global descriptors are overwritten in bind() of each instance of >>> f_midi, so you have no guarantee that your bInterfaceNumber is correct >>> for your current instance. Instead you should store value obtained from >>> usb_interface_id() during bind(). >> >> Ok. >> >> But then this interface descriptors shouldn't be global static, >> because they will always reflect the latest bind() only. Right? > > They are copied for each instance of USB function, so they are actually > template to fill and copy in bind(). I'm currently working on some > patches changing a bit behavior of set_alt() to make it clearer, but for > now the only way to distinguish between altsettings properly is to store > bInterfaceNumber value for each function instance in its bind(). Ok. Thanks. -- Felipe -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/7] usb: gadget: define free_ep_req as universal function
Hi Robert,
On 27/10/15 06:53, Robert Baldyga wrote:
> On 10/26/2015 05:55 PM, Felipe F. Tonello wrote:
>> This function is shared between gadget functions, so this avoid unnecessary
>> duplicated code and potentially avoid memory leaks.
>>
>> Signed-off-by: Felipe F. Tonello
>> ---
>> drivers/usb/gadget/function/f_midi.c | 6 --
>> drivers/usb/gadget/function/f_sourcesink.c | 6 --
>> drivers/usb/gadget/function/g_zero.h | 1 -
>> drivers/usb/gadget/u_f.c | 8
>> drivers/usb/gadget/u_f.h | 3 +--
>> 5 files changed, 9 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>> b/drivers/usb/gadget/function/f_midi.c
>> index c19f154..4c01c8a 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -202,12 +202,6 @@ static inline struct usb_request
>> *midi_alloc_ep_req(struct usb_ep *ep,
>> return alloc_ep_req(ep, length, length);
>> }
>>
>> -static void free_ep_req(struct usb_ep *ep, struct usb_request *req)
>> -{
>> -kfree(req->buf);
>> -usb_ep_free_request(ep, req);
>> -}
>> -
>> static const uint8_t f_midi_cin_length[] = {
>> 0, 0, 2, 3, 3, 1, 2, 3, 3, 3, 3, 3, 2, 2, 3, 1
>> };
>> diff --git a/drivers/usb/gadget/function/f_sourcesink.c
>> b/drivers/usb/gadget/function/f_sourcesink.c
>> index 3a5ae99..eedea7f 100644
>> --- a/drivers/usb/gadget/function/f_sourcesink.c
>> +++ b/drivers/usb/gadget/function/f_sourcesink.c
>> @@ -307,12 +307,6 @@ static inline struct usb_request
>> *ss_alloc_ep_req(struct usb_ep *ep, int len)
>> return alloc_ep_req(ep, len, buflen);
>> }
>>
>> -void free_ep_req(struct usb_ep *ep, struct usb_request *req)
>> -{
>> -kfree(req->buf);
>> -usb_ep_free_request(ep, req);
>> -}
>> -
>> static void disable_ep(struct usb_composite_dev *cdev, struct usb_ep *ep)
>> {
>> int value;
>> diff --git a/drivers/usb/gadget/function/g_zero.h
>> b/drivers/usb/gadget/function/g_zero.h
>> index 15f1809..5ed90b4 100644
>> --- a/drivers/usb/gadget/function/g_zero.h
>> +++ b/drivers/usb/gadget/function/g_zero.h
>> @@ -59,7 +59,6 @@ void lb_modexit(void);
>> int lb_modinit(void);
>>
>> /* common utilities */
>> -void free_ep_req(struct usb_ep *ep, struct usb_request *req);
>> void disable_endpoints(struct usb_composite_dev *cdev,
>> struct usb_ep *in, struct usb_ep *out,
>> struct usb_ep *iso_in, struct usb_ep *iso_out);
>> diff --git a/drivers/usb/gadget/u_f.c b/drivers/usb/gadget/u_f.c
>> index c6276f0..f78bd1f 100644
>> --- a/drivers/usb/gadget/u_f.c
>> +++ b/drivers/usb/gadget/u_f.c
>> @@ -14,6 +14,7 @@
>> #include
>> #include "u_f.h"
>>
>> +/* Requests allocated via alloc_ep_req() must be freed by free_ep_req(). */
>> struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int
>> default_len)
>> {
>> struct usb_request *req;
>> @@ -30,3 +31,10 @@ struct usb_request *alloc_ep_req(struct usb_ep *ep, int
>> len, int default_len)
>> return req;
>> }
>> EXPORT_SYMBOL_GPL(alloc_ep_req);
>> +
>> +void free_ep_req(struct usb_ep *ep, struct usb_request *req)
>> +{
>> +kfree(req->buf);
>> +usb_ep_free_request(ep, req);
>> +}
>> +EXPORT_SYMBOL_GPL(free_ep_req);
>> diff --git a/drivers/usb/gadget/u_f.h b/drivers/usb/gadget/u_f.h
>> index 1d5f0eb..2a1a6fb 100644
>> --- a/drivers/usb/gadget/u_f.h
>> +++ b/drivers/usb/gadget/u_f.h
>> @@ -46,7 +46,6 @@ struct usb_ep;
>> struct usb_request;
>>
>> struct usb_request *alloc_ep_req(struct usb_ep *ep, int len, int
>> default_len);
>> +void free_ep_req(struct usb_ep *ep, struct usb_request *req);
>>
>> #endif /* __U_F_H__ */
>> -
>> -
>>
>
> Isn't it simple enough to be static inline?
inline yes. And the compiler will do it automatically. But I can add it
for clarity.
Make it static it doesn't make sense. This function is exported in the
kernel.
--
Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
