On 8 July 2016 at 21:27, Felipe Balbi <ba...@kernel.org> wrote: > > Hi again, > > Felipe Balbi <ba...@kernel.org> writes: >> Michal Nazarewicz <min...@mina86.com> writes: >>> On Fri, Jul 08 2016, Baolin Wang wrote: >>>> On 7 July 2016 at 20:51, Michal Nazarewicz <min...@mina86.com> wrote: >>>>> On Thu, Jul 07 2016, 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 <baolin.w...@linaro.org> >>>>> >>>>> Acked-by: Michal Nazarewicz <min...@mina86.com> >>>>> >>>>>> --- >>>>>> drivers/usb/gadget/function/f_midi.c | 18 +++++++++++------- >>>>>> 1 file changed, 11 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/drivers/usb/gadget/function/f_midi.c >>>>>> b/drivers/usb/gadget/function/f_midi.c >>>>>> index 58fc199..2e3f11e 100644 >>>>>> --- a/drivers/usb/gadget/function/f_midi.c >>>>>> +++ b/drivers/usb/gadget/function/f_midi.c >>>>>> @@ -328,7 +328,7 @@ static int f_midi_start_ep(struct f_midi *midi, >>>>>> static int f_midi_set_alt(struct usb_function *f, unsigned intf, >>>>>> unsigned alt) >>>>>> { >>>>>> struct f_midi *midi = func_to_midi(f); >>>>>> - unsigned i; >>>>>> + unsigned i, length; >>>>>> int err; >>>>>> >>>>>> /* we only set alt for MIDIStreaming interface */ >>>>>> @@ -345,9 +345,11 @@ 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); >>>>>> + struct usb_request *req; >>>>>> >>>>>> + length = usb_ep_align_maybe(midi->gadget, midi->in_ep, >>>>>> + midi->buflen); >>>>>> + req = midi_alloc_ep_req(midi->in_ep, length); >>>>>> if (req == NULL) >>>>>> return -ENOMEM; >>>>>> >>>>>> @@ -359,10 +361,12 @@ 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; >>>>>> + >>>>>> + length = usb_ep_align_maybe(midi->gadget, midi->out_ep, >>>>>> + midi->buflen); >>>>>> + req = midi_alloc_ep_req(midi->out_ep, >>>>>> + max_t(unsigned, length, >>>>>> bulk_out_desc.wMaxPacketSize)); >>>>> >>>>> Perhaps: >>>>> >>>>> + length = midi->buflen < bulk_out_desc.wMaxPacketSize >>>>> + ? bulk_out_desc.wMaxPacketSize >>>>> + : usb_ep_align_maybe(midi->gadget, midi->out_ep, >>>>> + midi->buflen); >>>>> + req = midi_alloc_ep_req(midi->out_ep, length); >>>>> >>>>> I find it somewhat cleaner. Up to you. >>>> >>>> But if the gadget does not requires 'quirk_ep_out_aligned_size', then >>>> we also can keep midi->buflen length although midi->buflen < >>>> bulk_out_desc.wMaxPacketSize, right? Thanks for your comment. >>> >>> I don’t know. That’s not what the original code was doing. The >>> original code was using: >>> >>> max_t(unsigned, midi->buflen, bulk_out_desc.wMaxPacketSize)); >>> >>> for some reason.> >> >> My take on this is that it's calling max_t() to try and align to >> wMaxPacketSize. We can see from original commit what was the intent: >> >> commit 03d27ade4941076b34c823d63d91dc895731a595 >> Author: Felipe F. Tonello <e...@felipetonello.com> >> Date: Wed Mar 9 19:39:30 2016 +0000 >> >> usb: gadget: f_midi: Fixed a bug when buflen was smaller than >> wMaxPacketSize >> >> 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. >> >> Acked-by: Michal Nazarewicz <min...@mina86.com> >> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com> >> Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com> >> >> diff --git a/drivers/usb/gadget/function/f_midi.c >> b/drivers/usb/gadget/function/f_midi.c >> index 56e2dde99b03..9ad51dcab982 100644 >> --- a/drivers/usb/gadget/function/f_midi.c >> +++ b/drivers/usb/gadget/function/f_midi.c >> @@ -360,7 +360,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; >> >> Seems to me usb_ep_align_maybe() would cover this case just as well. But >> then, Felipe's UDC driver seems to need quirk_ep_out_aligned_size. Felipe? > > another way to look at this is that buflen < wMaxPacketSize for bulk > endpoints is kinda weird, so we might just go ahead and allocate buflen > aligned to wMaxPacketSize for OUT eps.
OK. So I will accept Michal's suggestion as below: length = midi->buflen < bulk_out_desc.wMaxPacketSize + ? bulk_out_desc.wMaxPacketSize + : usb_ep_align_maybe(midi->gadget, midi->out_ep, + midi->buflen); + req = midi_alloc_ep_req(midi->out_ep, length); -- Baolin.wang Best Regards