* [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize @ 2016-07-07 9:11 Baolin Wang 2016-07-07 12:51 ` Michal Nazarewicz 2016-07-08 13:21 ` Felipe Balbi 0 siblings, 2 replies; 12+ messages in thread From: Baolin Wang @ 2016-07-07 9:11 UTC (permalink / raw) To: balbi Cc: gregkh, eu, mina86, r.baldyga, dan.carpenter, linux-usb, linux-kernel, broonie, baolin.wang 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.wang@linaro.org> --- 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)); if (req == NULL) return -ENOMEM; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize 2016-07-07 9:11 [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize Baolin Wang @ 2016-07-07 12:51 ` Michal Nazarewicz 2016-07-08 2:25 ` Baolin Wang 2016-07-08 13:21 ` Felipe Balbi 1 sibling, 1 reply; 12+ messages in thread From: Michal Nazarewicz @ 2016-07-07 12:51 UTC (permalink / raw) To: Baolin Wang, balbi Cc: gregkh, eu, r.baldyga, dan.carpenter, linux-usb, linux-kernel, broonie, baolin.wang 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.wang@linaro.org> Acked-by: Michal Nazarewicz <mina86@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. > if (req == NULL) > return -ENOMEM; > -- Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving» ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize 2016-07-07 12:51 ` Michal Nazarewicz @ 2016-07-08 2:25 ` Baolin Wang 2016-07-08 13:04 ` Michal Nazarewicz 0 siblings, 1 reply; 12+ messages in thread From: Baolin Wang @ 2016-07-08 2:25 UTC (permalink / raw) To: Michal Nazarewicz Cc: Felipe Balbi, Greg KH, eu, r.baldyga, dan.carpenter, USB, LKML, Mark Brown On 7 July 2016 at 20:51, Michal Nazarewicz <mina86@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.wang@linaro.org> > > Acked-by: Michal Nazarewicz <mina86@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. > >> if (req == NULL) >> return -ENOMEM; >> > > -- > Best regards > ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ > «If at first you don’t succeed, give up skydiving» -- Baolin.wang Best Regards ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize 2016-07-08 2:25 ` Baolin Wang @ 2016-07-08 13:04 ` Michal Nazarewicz 2016-07-08 13:25 ` Felipe Balbi 2016-07-11 2:26 ` Baolin Wang 0 siblings, 2 replies; 12+ messages in thread From: Michal Nazarewicz @ 2016-07-08 13:04 UTC (permalink / raw) To: Baolin Wang Cc: Felipe Balbi, Greg KH, eu, dan.carpenter, USB, LKML, Mark Brown On Fri, Jul 08 2016, Baolin Wang wrote: > On 7 July 2016 at 20:51, Michal Nazarewicz <mina86@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.wang@linaro.org> >> >> Acked-by: Michal Nazarewicz <mina86@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.> >>> if (req == NULL) >>> return -ENOMEM; >>> >> >> -- >> Best regards >> ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ >> «If at first you don’t succeed, give up skydiving» > > > > -- > Baolin.wang > Best Regards -- Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving» ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize 2016-07-08 13:04 ` Michal Nazarewicz @ 2016-07-08 13:25 ` Felipe Balbi 2016-07-08 13:27 ` Felipe Balbi 2016-07-08 14:05 ` Michal Nazarewicz 2016-07-11 2:26 ` Baolin Wang 1 sibling, 2 replies; 12+ messages in thread From: Felipe Balbi @ 2016-07-08 13:25 UTC (permalink / raw) To: Michal Nazarewicz, Baolin Wang, Felipe Ferreri Tonello Cc: Greg KH, eu, dan.carpenter, USB, LKML, Mark Brown [-- Attachment #1: Type: text/plain, Size: 5778 bytes --] Hi, Michal Nazarewicz <mina86@mina86.com> writes: > On Fri, Jul 08 2016, Baolin Wang wrote: >> On 7 July 2016 at 20:51, Michal Nazarewicz <mina86@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.wang@linaro.org> >>> >>> Acked-by: Michal Nazarewicz <mina86@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 <eu@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 <mina86@mina86.com> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com> Signed-off-by: Felipe Balbi <felipe.balbi@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? -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize 2016-07-08 13:25 ` Felipe Balbi @ 2016-07-08 13:27 ` Felipe Balbi 2016-07-11 2:25 ` Baolin Wang 2016-07-08 14:05 ` Michal Nazarewicz 1 sibling, 1 reply; 12+ messages in thread From: Felipe Balbi @ 2016-07-08 13:27 UTC (permalink / raw) To: Michal Nazarewicz, Baolin Wang, Felipe Ferreri Tonello Cc: Greg KH, eu, dan.carpenter, USB, LKML, Mark Brown [-- Attachment #1: Type: text/plain, Size: 6189 bytes --] Hi again, Felipe Balbi <balbi@kernel.org> writes: > Michal Nazarewicz <mina86@mina86.com> writes: >> On Fri, Jul 08 2016, Baolin Wang wrote: >>> On 7 July 2016 at 20:51, Michal Nazarewicz <mina86@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.wang@linaro.org> >>>> >>>> Acked-by: Michal Nazarewicz <mina86@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 <eu@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 <mina86@mina86.com> > Signed-off-by: Felipe F. Tonello <eu@felipetonello.com> > Signed-off-by: Felipe Balbi <felipe.balbi@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. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize 2016-07-08 13:27 ` Felipe Balbi @ 2016-07-11 2:25 ` Baolin Wang 0 siblings, 0 replies; 12+ messages in thread From: Baolin Wang @ 2016-07-11 2:25 UTC (permalink / raw) To: Felipe Balbi Cc: Michal Nazarewicz, Felipe Ferreri Tonello, Greg KH, dan.carpenter, USB, LKML, Mark Brown On 8 July 2016 at 21:27, Felipe Balbi <balbi@kernel.org> wrote: > > Hi again, > > Felipe Balbi <balbi@kernel.org> writes: >> Michal Nazarewicz <mina86@mina86.com> writes: >>> On Fri, Jul 08 2016, Baolin Wang wrote: >>>> On 7 July 2016 at 20:51, Michal Nazarewicz <mina86@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.wang@linaro.org> >>>>> >>>>> Acked-by: Michal Nazarewicz <mina86@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 <eu@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 <mina86@mina86.com> >> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com> >> Signed-off-by: Felipe Balbi <felipe.balbi@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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize 2016-07-08 13:25 ` Felipe Balbi 2016-07-08 13:27 ` Felipe Balbi @ 2016-07-08 14:05 ` Michal Nazarewicz 1 sibling, 0 replies; 12+ messages in thread From: Michal Nazarewicz @ 2016-07-08 14:05 UTC (permalink / raw) To: Felipe Balbi, Baolin Wang, Felipe Ferreri Tonello Cc: Greg KH, eu, dan.carpenter, USB, LKML, Mark Brown On Fri, Jul 08 2016, Felipe Balbi wrote: > 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 <eu@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 <mina86@mina86.com> > Signed-off-by: Felipe F. Tonello <eu@felipetonello.com> > Signed-off-by: Felipe Balbi <felipe.balbi@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? In that case, I agree that max is likely unnecessary. -- Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving» ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize 2016-07-08 13:04 ` Michal Nazarewicz 2016-07-08 13:25 ` Felipe Balbi @ 2016-07-11 2:26 ` Baolin Wang 1 sibling, 0 replies; 12+ messages in thread From: Baolin Wang @ 2016-07-11 2:26 UTC (permalink / raw) To: Michal Nazarewicz Cc: Felipe Balbi, Greg KH, Felipe Ferreri Tonello, dan.carpenter, USB, LKML, Mark Brown On 8 July 2016 at 21:04, Michal Nazarewicz <mina86@mina86.com> wrote: > On Fri, Jul 08 2016, Baolin Wang wrote: >> On 7 July 2016 at 20:51, Michal Nazarewicz <mina86@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.wang@linaro.org> >>> >>> Acked-by: Michal Nazarewicz <mina86@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)); OK. > > for some reason.> > >>>> if (req == NULL) >>>> return -ENOMEM; >>>> >>> >>> -- >>> Best regards >>> ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ >>> «If at first you don’t succeed, give up skydiving» >> >> >> >> -- >> Baolin.wang >> Best Regards > > -- > Best regards > ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ > «If at first you don’t succeed, give up skydiving» -- Baolin.wang Best Regards ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize 2016-07-07 9:11 [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize Baolin Wang 2016-07-07 12:51 ` Michal Nazarewicz @ 2016-07-08 13:21 ` Felipe Balbi 2016-07-08 14:04 ` Michal Nazarewicz 2016-07-11 2:22 ` Baolin Wang 1 sibling, 2 replies; 12+ messages in thread From: Felipe Balbi @ 2016-07-08 13:21 UTC (permalink / raw) To: Baolin Wang Cc: gregkh, eu, mina86, r.baldyga, dan.carpenter, linux-usb, linux-kernel, broonie, baolin.wang [-- Attachment #1: Type: text/plain, Size: 2427 bytes --] Hi, Baolin Wang <baolin.wang@linaro.org> writes: > 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.wang@linaro.org> > --- > 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); this is not needed for in_ep, only out_ep, right? > + 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); after calling usb_ep_align_maybe()... > + req = midi_alloc_ep_req(midi->out_ep, > + max_t(unsigned, length, bulk_out_desc.wMaxPacketSize)); ... max_t() is pointless. length will *always* >= wMaxPacketSize. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize 2016-07-08 13:21 ` Felipe Balbi @ 2016-07-08 14:04 ` Michal Nazarewicz 2016-07-11 2:22 ` Baolin Wang 1 sibling, 0 replies; 12+ messages in thread From: Michal Nazarewicz @ 2016-07-08 14:04 UTC (permalink / raw) To: Felipe Balbi, Baolin Wang Cc: gregkh, eu, r.baldyga, dan.carpenter, linux-usb, linux-kernel, broonie, baolin.wang > Baolin Wang <baolin.wang@linaro.org> writes: >> @@ -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); On Fri, Jul 08 2016, Felipe Balbi wrote: > after calling usb_ep_align_maybe()... > >> + req = midi_alloc_ep_req(midi->out_ep, >> + max_t(unsigned, length, bulk_out_desc.wMaxPacketSize)); > > ... max_t() is pointless. length will *always* >= wMaxPacketSize. That is only true for gadgets with the quirk. usb_ep_align_maybe is a noöp for gadgets without the quirk. -- Best regards ミハウ “𝓶𝓲𝓷𝓪86” ナザレヴイツ «If at first you don’t succeed, give up skydiving» ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize 2016-07-08 13:21 ` Felipe Balbi 2016-07-08 14:04 ` Michal Nazarewicz @ 2016-07-11 2:22 ` Baolin Wang 1 sibling, 0 replies; 12+ messages in thread From: Baolin Wang @ 2016-07-11 2:22 UTC (permalink / raw) To: Felipe Balbi Cc: Greg KH, eu, Michal Nazarewicz, r.baldyga, dan.carpenter, USB, LKML, Mark Brown On 8 July 2016 at 21:21, Felipe Balbi <balbi@kernel.org> wrote: > > Hi, > > Baolin Wang <baolin.wang@linaro.org> writes: >> 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.wang@linaro.org> >> --- >> 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); > > this is not needed for in_ep, only out_ep, right? Yes. You are right. > >> + 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); > > after calling usb_ep_align_maybe()... > >> + req = midi_alloc_ep_req(midi->out_ep, >> + max_t(unsigned, length, bulk_out_desc.wMaxPacketSize)); > > ... max_t() is pointless. length will *always* >= wMaxPacketSize. It is not. If the gadget device dose not require quirk_ep_out_aligned_size, then the lengh may be <= wMaxPacketSize. -- Baolin.wang Best Regards ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-07-11 2:26 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-07-07 9:11 [PATCH] usb: gadget: f_midi: Add checking if it need align buffer's size to an ep's maxpacketsize Baolin Wang 2016-07-07 12:51 ` Michal Nazarewicz 2016-07-08 2:25 ` Baolin Wang 2016-07-08 13:04 ` Michal Nazarewicz 2016-07-08 13:25 ` Felipe Balbi 2016-07-08 13:27 ` Felipe Balbi 2016-07-11 2:25 ` Baolin Wang 2016-07-08 14:05 ` Michal Nazarewicz 2016-07-11 2:26 ` Baolin Wang 2016-07-08 13:21 ` Felipe Balbi 2016-07-08 14:04 ` Michal Nazarewicz 2016-07-11 2:22 ` Baolin Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox