Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* USB:UAC2: Incorrect req->length > maxpacket*mc
@ 2020-01-10  7:29 Pavel Hofman
  2020-01-10 11:28 ` [PATCH] usb: gadget: f_uac2: fix packet size calculation John Keeping
  2020-01-11  9:31 ` USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found Pavel Hofman
  0 siblings, 2 replies; 13+ messages in thread
From: Pavel Hofman @ 2020-01-10  7:29 UTC (permalink / raw)
  To: linux-usb, Felipe Balbi

Hi,

Together with dwc2 maintainer Minas Harutyunyan we have been
troubleshooting various issues of dwc2 on RPi4. We hit a problem where
the g_audio in capture (EP OUT, playback from USB host) requests req->
length larger than maxpacket*mc.

As a workaround we removed the check in dwc2/gadget.c, however that is
not a proper solution. Minas with his team decided to add a patch where 
dwc2 will rejecting this type of wrong requests in dwc2_hsotg_ep_queue()
function, to not process these request at all. The f_uac2 + g_audio
gadget should restrain from sending such requests.

Steps to reproduce:

* Changing fs_epout_desc.bInterval in f_uac2.c from 4 (1ms) to 1 (125us) 
- the goal is to maximize available throughput of the audio gadget

* Loading the g_audio module with c_srate=48000, c_ssize=2, c_chmask=2 - 
i.e. standard 48kHz/16bit/2ch USB playback -> alsa capture

This combination produces mps=24 and mc=1 for EP OUT. Yet the audio 
function driver sometimes queues request with req->length 192.


Please may I ask for fixing the audio function  in this respect? 
Unfortunately that is way above my knowledge of that code/requirements. 
However, I can send debug dump for u_audio.c, f_uac2.c, gadget.c if needed.

Thanks a lot in advance.

Best regards,

Pavel.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] usb: gadget: f_uac2: fix packet size calculation
  2020-01-10  7:29 USB:UAC2: Incorrect req->length > maxpacket*mc Pavel Hofman
@ 2020-01-10 11:28 ` John Keeping
  2020-01-11  9:12   ` Pavel Hofman
  2020-01-11  9:31 ` USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found Pavel Hofman
  1 sibling, 1 reply; 13+ messages in thread
From: John Keeping @ 2020-01-10 11:28 UTC (permalink / raw)
  To: Pavel Hofman; +Cc: linux-usb, Felipe Balbi

On Fri, 10 Jan 2020 08:29:06 +0100
Pavel Hofman <pavel.hofman@ivitera.com> wrote:

> Together with dwc2 maintainer Minas Harutyunyan we have been
> troubleshooting various issues of dwc2 on RPi4. We hit a problem where
> the g_audio in capture (EP OUT, playback from USB host) requests req->
> length larger than maxpacket*mc.
> 
> As a workaround we removed the check in dwc2/gadget.c, however that is
> not a proper solution. Minas with his team decided to add a patch where 
> dwc2 will rejecting this type of wrong requests in dwc2_hsotg_ep_queue()
> function, to not process these request at all. The f_uac2 + g_audio
> gadget should restrain from sending such requests.
> 
> Steps to reproduce:
> 
> * Changing fs_epout_desc.bInterval in f_uac2.c from 4 (1ms) to 1 (125us) 
> - the goal is to maximize available throughput of the audio gadget
> 
> * Loading the g_audio module with c_srate=48000, c_ssize=2, c_chmask=2 - 
> i.e. standard 48kHz/16bit/2ch USB playback -> alsa capture
> 
> This combination produces mps=24 and mc=1 for EP OUT. Yet the audio 
> function driver sometimes queues request with req->length 192.

This sounded familiar to me, and I realised that I've been running with
a patch for this case that I never submitted upstream.

Does the patch below fix the issue you're seeing?

-- >8 --
The packet size for USB audio must always be a multiple of the frame
size, otherwise we are transmitting a partial frame which omits some
channels (and these end up at the wrong offset in the next packet).
Furthermore, it breaks the residue handling such that we end up trying
to send a packet exceeding the maximum packet size for the endpoint.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/usb/gadget/function/u_audio.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index cf4f2358889b..6d956f190f5a 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -407,7 +407,7 @@ int u_audio_start_playback(struct g_audio *audio_dev)
 	struct usb_ep *ep;
 	struct uac_rtd_params *prm;
 	struct uac_params *params = &audio_dev->params;
-	unsigned int factor, rate;
+	unsigned int factor;
 	const struct usb_endpoint_descriptor *ep_desc;
 	int req_len, i;
 
@@ -426,13 +426,15 @@ int u_audio_start_playback(struct g_audio *audio_dev)
 	/* pre-compute some values for iso_complete() */
 	uac->p_framesize = params->p_ssize *
 			    num_channels(params->p_chmask);
-	rate = params->p_srate * uac->p_framesize;
 	uac->p_interval = factor / (1 << (ep_desc->bInterval - 1));
-	uac->p_pktsize = min_t(unsigned int, rate / uac->p_interval,
+	uac->p_pktsize = min_t(unsigned int,
+				uac->p_framesize *
+					(params->p_srate / uac->p_interval),
 				prm->max_psize);
 
 	if (uac->p_pktsize < prm->max_psize)
-		uac->p_pktsize_residue = rate % uac->p_interval;
+		uac->p_pktsize_residue = uac->p_framesize *
+			(params->p_srate % uac->p_interval);
 	else
 		uac->p_pktsize_residue = 0;
 
-- 
2.24.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] usb: gadget: f_uac2: fix packet size calculation
  2020-01-10 11:28 ` [PATCH] usb: gadget: f_uac2: fix packet size calculation John Keeping
@ 2020-01-11  9:12   ` Pavel Hofman
  0 siblings, 0 replies; 13+ messages in thread
From: Pavel Hofman @ 2020-01-11  9:12 UTC (permalink / raw)
  To: John Keeping; +Cc: linux-usb, Felipe Balbi

Hi,

Dne 10. 01. 20 v 12:28 John Keeping napsal(a):
> On Fri, 10 Jan 2020 08:29:06 +0100
> Pavel Hofman <pavel.hofman@ivitera.com> wrote:
> 
>> Together with dwc2 maintainer Minas Harutyunyan we have been
>> troubleshooting various issues of dwc2 on RPi4. We hit a problem where
>> the g_audio in capture (EP OUT, playback from USB host) requests req->
>> length larger than maxpacket*mc.
>>
>> As a workaround we removed the check in dwc2/gadget.c, however that is
>> not a proper solution. Minas with his team decided to add a patch where
>> dwc2 will rejecting this type of wrong requests in dwc2_hsotg_ep_queue()
>> function, to not process these request at all. The f_uac2 + g_audio
>> gadget should restrain from sending such requests.
>>
>> Steps to reproduce:
>>
>> * Changing fs_epout_desc.bInterval in f_uac2.c from 4 (1ms) to 1 (125us)
>> - the goal is to maximize available throughput of the audio gadget
>>
>> * Loading the g_audio module with c_srate=48000, c_ssize=2, c_chmask=2 -
>> i.e. standard 48kHz/16bit/2ch USB playback -> alsa capture
>>
>> This combination produces mps=24 and mc=1 for EP OUT. Yet the audio
>> function driver sometimes queues request with req->length 192.
> 
> This sounded familiar to me, and I realised that I've been running with
> a patch for this case that I never submitted upstream.
> 
> Does the patch below fix the issue you're seeing?
> @@ -407,7 +407,7 @@ int u_audio_start_playback(struct g_audio *audio_dev)

Thanks for your hint. Your patch handles u_audio playback which is the 
other direction than I am trying to fix (usb host playback -> u_audio 
capture).

With regards,

Pavel.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found
  2020-01-10  7:29 USB:UAC2: Incorrect req->length > maxpacket*mc Pavel Hofman
  2020-01-10 11:28 ` [PATCH] usb: gadget: f_uac2: fix packet size calculation John Keeping
@ 2020-01-11  9:31 ` Pavel Hofman
  2020-01-11  9:49   ` Pavel Hofman
  2020-01-14 18:39   ` Pavel Hofman
  1 sibling, 2 replies; 13+ messages in thread
From: Pavel Hofman @ 2020-01-11  9:31 UTC (permalink / raw)
  To: linux-usb, Felipe Balbi

Hi,

Dne 10. 01. 20 v 8:29 Pavel Hofman napsal(a):
> Hi,
> 
> Together with dwc2 maintainer Minas Harutyunyan we have been
> troubleshooting various issues of dwc2 on RPi4. We hit a problem where
> the g_audio in capture (EP OUT, playback from USB host) requests req->
> length larger than maxpacket*mc.
> 
> As a workaround we removed the check in dwc2/gadget.c, however that is
> not a proper solution. Minas with his team decided to add a patch where 
> dwc2 will rejecting this type of wrong requests in dwc2_hsotg_ep_queue()
> function, to not process these request at all. The f_uac2 + g_audio
> gadget should restrain from sending such requests.
> 
> Steps to reproduce:
> 
> * Changing fs_epout_desc.bInterval in f_uac2.c from 4 (1ms) to 1 (125us) 
> - the goal is to maximize available throughput of the audio gadget
> 
> * Loading the g_audio module with c_srate=48000, c_ssize=2, c_chmask=2 - 
> i.e. standard 48kHz/16bit/2ch USB playback -> alsa capture
> 
> This combination produces mps=24 and mc=1 for EP OUT. Yet the audio 
> function driver sometimes queues request with req->length 192.
> 

IMO the problem is here 
https://github.com/torvalds/linux/blob/master/drivers/usb/gadget/function/f_uac2.c#L675 
:

First wMaxPacketSize is correctly calculated for HS and FS. In my setup 
it yields (confirmed by an added debug in set_ep_max_packet_size):

FS: ep_desc->wMaxPacketSize 192
HS: ep_desc->wMaxPacketSize 24

However, a few lines later the agdev->out_ep_maxpsize is set as maximum 
from these two values 
https://github.com/torvalds/linux/blob/master/drivers/usb/gadget/function/f_uac2.c#L700 
:

agdev->out_ep_maxpsize = max_t(u16, 
le16_to_cpu(fs_epout_desc.wMaxPacketSize),			 
le16_to_cpu(hs_epout_desc.wMaxPacketSize));


In g_audio_setup this value is used for max_psize 
https://github.com/torvalds/linux/blob/master/drivers/usb/gadget/function/u_audio.c#L519 
:

prm->max_psize = g_audio->out_ep_maxpsize;

And max_psize is used as req->length in u_audio_start_capture 
https://github.com/torvalds/linux/blob/master/drivers/usb/gadget/function/u_audio.c#L379 
:

req_len = prm->max_psize;
...
req->length = req_len;


192bytes (the value for FS) will be assigned here, instead of the 
correct 24 bytes for HS. The reason it has worked so far is the original 
bInterval=4 for HS which yields the same 192bytes as the original for 
FS. But lowering the fs_epout_desc.bInterval reduces the packets, yet 
the maximum of the two values is being used.

IMO the very same problem will be on u_audio playback side, the 
preceeding line:

agdev->in_ep_maxpsize = max_t(u16,			 
le16_to_cpu(fs_epin_desc.wMaxPacketSize),			 
le16_to_cpu(hs_epin_desc.wMaxPacketSize));

Unfortunately I do not know the reason for selection of the maximum 
value from FS and HS, I cannot create a patch. Very likely there is more 
hidden know-how which I do not know.

Thanks a lot for looking at the issue.

With regards,

Pavel.






^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found
  2020-01-11  9:31 ` USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found Pavel Hofman
@ 2020-01-11  9:49   ` Pavel Hofman
  2020-01-14 18:39   ` Pavel Hofman
  1 sibling, 0 replies; 13+ messages in thread
From: Pavel Hofman @ 2020-01-11  9:49 UTC (permalink / raw)
  To: linux-usb, Felipe Balbi


Dne 11. 01. 20 v 10:31 Pavel Hofman napsal(a):
> Unfortunately I do not know the reason for selection of the maximum 
> value from FS and HS, I cannot create a patch. Very likely there is more 
> hidden know-how which I do not know.
> 

The change was introduced by 
https://github.com/torvalds/linux/commit/eb9fecb9e69b0be8c267c55b0bb52a08e8fb6bee#diff-db92e74adcaaf2659ecf1a2135fd5901R572

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found
  2020-01-11  9:31 ` USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found Pavel Hofman
  2020-01-11  9:49   ` Pavel Hofman
@ 2020-01-14 18:39   ` Pavel Hofman
  2020-01-14 20:04     ` John Keeping
  1 sibling, 1 reply; 13+ messages in thread
From: Pavel Hofman @ 2020-01-14 18:39 UTC (permalink / raw)
  To: linux-usb, Felipe Balbi

Hi,

Dne 11. 01. 20 v 10:31 Pavel Hofman napsal(a):
> Hi,
> 
> Dne 10. 01. 20 v 8:29 Pavel Hofman napsal(a):
>> Hi,
>>
>> Together with dwc2 maintainer Minas Harutyunyan we have been
>> troubleshooting various issues of dwc2 on RPi4. We hit a problem where
>> the g_audio in capture (EP OUT, playback from USB host) requests req->
>> length larger than maxpacket*mc.

My question relates to the recent patch 
https://marc.info/?l=linux-usb&m=157901102706577&w=2
> 
> IMO the problem is here 
> https://github.com/torvalds/linux/blob/master/drivers/usb/gadget/function/f_uac2.c#L675 
> :
> 
> However, a few lines later the agdev->out_ep_maxpsize is set as maximum 
> from these two values 
> https://github.com/torvalds/linux/blob/master/drivers/usb/gadget/function/f_uac2.c#L700 
> :
> 
> agdev->out_ep_maxpsize = max_t(u16, 
> le16_to_cpu(fs_epout_desc.wMaxPacketSize), 
> le16_to_cpu(hs_epout_desc.wMaxPacketSize));
> 
> 
> Unfortunately I do not know the reason for selection of the maximum 
> value from FS and HS, I cannot create a patch. Very likely there is more 
> hidden know-how which I do not know.
> 

Please can we solve this issue so that the gadget can work for any 
bInterval value?

Thanks a lot,

Pavel.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found
  2020-01-14 18:39   ` Pavel Hofman
@ 2020-01-14 20:04     ` John Keeping
  2020-01-14 20:21       ` Pavel Hofman
  2020-01-16 15:39       ` Pavel Hofman
  0 siblings, 2 replies; 13+ messages in thread
From: John Keeping @ 2020-01-14 20:04 UTC (permalink / raw)
  To: Pavel Hofman; +Cc: linux-usb, Felipe Balbi

Hi Pavel,

On Tue, 14 Jan 2020 19:39:05 +0100
Pavel Hofman <pavel.hofman@ivitera.com> wrote:

> Dne 11. 01. 20 v 10:31 Pavel Hofman napsal(a):
> > Hi,
> > 
> > Dne 10. 01. 20 v 8:29 Pavel Hofman napsal(a):  
> >> Hi,
> >>
> >> Together with dwc2 maintainer Minas Harutyunyan we have been
> >> troubleshooting various issues of dwc2 on RPi4. We hit a problem where
> >> the g_audio in capture (EP OUT, playback from USB host) requests req->
> >> length larger than maxpacket*mc.  
> 
> My question relates to the recent patch 
> https://marc.info/?l=linux-usb&m=157901102706577&w=2
> > 
> > IMO the problem is here 
> > https://github.com/torvalds/linux/blob/master/drivers/usb/gadget/function/f_uac2.c#L675 
> > :
> > 
> > However, a few lines later the agdev->out_ep_maxpsize is set as maximum 
> > from these two values 
> > https://github.com/torvalds/linux/blob/master/drivers/usb/gadget/function/f_uac2.c#L700 
> > :
> > 
> > agdev->out_ep_maxpsize = max_t(u16, 
> > le16_to_cpu(fs_epout_desc.wMaxPacketSize), 
> > le16_to_cpu(hs_epout_desc.wMaxPacketSize));
> > 
> > 
> > Unfortunately I do not know the reason for selection of the maximum 
> > value from FS and HS, I cannot create a patch. Very likely there is more 
> > hidden know-how which I do not know.
> >   
> 
> Please can we solve this issue so that the gadget can work for any 
> bInterval value?

I've taken a look at this and the patch below fixes it in my simple
testing.  But note that this doesn't adjust the PCM's min_period_bytes
which will be necessary if you want to minimize latency with an adjusted
high-speed bInterval setting.

I'm not sure what the right answer is for that; we could update
min_period_bytes if the PCM is opened after the gadget attaches, but
then if it is re-attached at a slower speed the PCM configuration will
be wrong.

-- >8 --
From 1809582b8acfa4127fb507a97206e598fa47f55a Mon Sep 17 00:00:00 2001
From: John Keeping <john@metanate.com>
Date: Tue, 14 Jan 2020 19:16:10 +0000
Subject: [PATCH] usb: gadget: u_audio: Fix high-speed max packet size

Prior to commit eb9fecb9e69b ("usb: gadget: f_uac2: split out audio
core") the maximum packet size was calculated only from the high-speed
descriptor but now we use the largest of the full-speed and high-speed
descriptors.

This is correct, but the full-speed value is likely to be higher than
that for high-speed and this leads to submitting requests for OUT
transfers (received by the gadget) which are larger than the endpoint's
maximum packet size.  These are rightly rejected by the gadget core.

config_ep_by_speed() already sets up the correct maximum packet size for
the enumerated speed in the usb_ep structure, so we can simply use this
instead of the overall value that has been used to allocate buffers for
requests.

Note that the minimum period for ALSA is still set from the largest
value, and this is unavoidable because it's possible to open the audio
device before the gadget has been enumerated.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/usb/gadget/function/u_audio.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index 6d956f190f5a..e6d32c536781 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -361,7 +361,7 @@ int u_audio_start_capture(struct g_audio *audio_dev)
 	ep = audio_dev->out_ep;
 	prm = &uac->c_prm;
 	config_ep_by_speed(gadget, &audio_dev->func, ep);
-	req_len = prm->max_psize;
+	req_len = ep->maxpacket;
 
 	prm->ep_enabled = true;
 	usb_ep_enable(ep);
@@ -379,7 +379,7 @@ int u_audio_start_capture(struct g_audio *audio_dev)
 			req->context = &prm->ureq[i];
 			req->length = req_len;
 			req->complete = u_audio_iso_complete;
-			req->buf = prm->rbuf + i * prm->max_psize;
+			req->buf = prm->rbuf + i * ep->maxpacket;
 		}
 
 		if (usb_ep_queue(ep, prm->ureq[i].req, GFP_ATOMIC))
@@ -430,9 +430,9 @@ int u_audio_start_playback(struct g_audio *audio_dev)
 	uac->p_pktsize = min_t(unsigned int,
 				uac->p_framesize *
 					(params->p_srate / uac->p_interval),
-				prm->max_psize);
+				ep->maxpacket);
 
-	if (uac->p_pktsize < prm->max_psize)
+	if (uac->p_pktsize < ep->maxpacket)
 		uac->p_pktsize_residue = uac->p_framesize *
 			(params->p_srate % uac->p_interval);
 	else
@@ -457,7 +457,7 @@ int u_audio_start_playback(struct g_audio *audio_dev)
 			req->context = &prm->ureq[i];
 			req->length = req_len;
 			req->complete = u_audio_iso_complete;
-			req->buf = prm->rbuf + i * prm->max_psize;
+			req->buf = prm->rbuf + i * ep->maxpacket;
 		}
 
 		if (usb_ep_queue(ep, prm->ureq[i].req, GFP_ATOMIC))
-- 
2.24.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found
  2020-01-14 20:04     ` John Keeping
@ 2020-01-14 20:21       ` Pavel Hofman
  2020-01-16 15:39       ` Pavel Hofman
  1 sibling, 0 replies; 13+ messages in thread
From: Pavel Hofman @ 2020-01-14 20:21 UTC (permalink / raw)
  To: John Keeping; +Cc: linux-usb, Felipe Balbi

Hi John,

Dne 14. 01. 20 v 21:04 John Keeping napsal(a):
> 
> I've taken a look at this and the patch below fixes it in my simple
> testing.  

I like your solution, simple and understandable.

> But note that this doesn't adjust the PCM's min_period_bytes
> which will be necessary if you want to minimize latency with an adjusted
> high-speed bInterval setting.

My motivation for the smaller bInterval is higher attainable throughput. 
In fact to reach stable operation (avoiding random xruns) I have to use 
larger period on RPi4 - hence larger latency anyway.
> 
> I'm not sure what the right answer is for that; we could update
> min_period_bytes if the PCM is opened after the gadget attaches, but
> then if it is re-attached at a slower speed the PCM configuration will
> be wrong.

I would suggest to keep the minimum period setting as is.

Thanks a lot for your help.

Pavel.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found
  2020-01-14 20:04     ` John Keeping
  2020-01-14 20:21       ` Pavel Hofman
@ 2020-01-16 15:39       ` Pavel Hofman
  2020-01-17 10:40         ` [PATCH] usb: gadget: u_audio: Fix high-speed max packet size John Keeping
  1 sibling, 1 reply; 13+ messages in thread
From: Pavel Hofman @ 2020-01-16 15:39 UTC (permalink / raw)
  To: John Keeping; +Cc: linux-usb, Felipe Balbi

Hi John,

> I've taken a look at this and the patch below fixes it in my simple
> testing.  But note that this doesn't adjust the PCM's min_period_bytes
> which will be necessary if you want to minimize latency with an adjusted
> high-speed bInterval setting.
> 

Please can I ask you to submit your patch? IMO your perhaps slightly 
suboptimal solution is much better than the current broken version.

Thanks a lot,

Pavel.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] usb: gadget: u_audio: Fix high-speed max packet size
  2020-01-16 15:39       ` Pavel Hofman
@ 2020-01-17 10:40         ` John Keeping
  2020-01-19 14:53           ` Pavel Hofman
  2020-01-24 12:52           ` Felipe Balbi
  0 siblings, 2 replies; 13+ messages in thread
From: John Keeping @ 2020-01-17 10:40 UTC (permalink / raw)
  To: Pavel Hofman; +Cc: linux-usb, Felipe Balbi

On Thu, 16 Jan 2020 16:39:50 +0100
Pavel Hofman <pavel.hofman@ivitera.com> wrote:

> > I've taken a look at this and the patch below fixes it in my simple
> > testing.  But note that this doesn't adjust the PCM's min_period_bytes
> > which will be necessary if you want to minimize latency with an adjusted
> > high-speed bInterval setting.
> >   
> 
> Please can I ask you to submit your patch? IMO your perhaps slightly 
> suboptimal solution is much better than the current broken version.

Yes, the patch is definitely an improvement.  I thought it would be
picked up from the earlier mail, but I think Patchwork requires the
subject to match, so I'm including it again here.

Are you able to provide a Tested-by for this change?

-- >8 --
Prior to commit eb9fecb9e69b ("usb: gadget: f_uac2: split out audio
core") the maximum packet size was calculated only from the high-speed
descriptor but now we use the largest of the full-speed and high-speed
descriptors.

This is correct, but the full-speed value is likely to be higher than
that for high-speed and this leads to submitting requests for OUT
transfers (received by the gadget) which are larger than the endpoint's
maximum packet size.  These are rightly rejected by the gadget core.

config_ep_by_speed() already sets up the correct maximum packet size for
the enumerated speed in the usb_ep structure, so we can simply use this
instead of the overall value that has been used to allocate buffers for
requests.

Note that the minimum period for ALSA is still set from the largest
value, and this is unavoidable because it's possible to open the audio
device before the gadget has been enumerated.

Signed-off-by: John Keeping <john@metanate.com>
---
 drivers/usb/gadget/function/u_audio.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/function/u_audio.c b/drivers/usb/gadget/function/u_audio.c
index 6d956f190f5a..e6d32c536781 100644
--- a/drivers/usb/gadget/function/u_audio.c
+++ b/drivers/usb/gadget/function/u_audio.c
@@ -361,7 +361,7 @@ int u_audio_start_capture(struct g_audio *audio_dev)
 	ep = audio_dev->out_ep;
 	prm = &uac->c_prm;
 	config_ep_by_speed(gadget, &audio_dev->func, ep);
-	req_len = prm->max_psize;
+	req_len = ep->maxpacket;
 
 	prm->ep_enabled = true;
 	usb_ep_enable(ep);
@@ -379,7 +379,7 @@ int u_audio_start_capture(struct g_audio *audio_dev)
 			req->context = &prm->ureq[i];
 			req->length = req_len;
 			req->complete = u_audio_iso_complete;
-			req->buf = prm->rbuf + i * prm->max_psize;
+			req->buf = prm->rbuf + i * ep->maxpacket;
 		}
 
 		if (usb_ep_queue(ep, prm->ureq[i].req, GFP_ATOMIC))
@@ -430,9 +430,9 @@ int u_audio_start_playback(struct g_audio *audio_dev)
 	uac->p_pktsize = min_t(unsigned int,
 				uac->p_framesize *
 					(params->p_srate / uac->p_interval),
-				prm->max_psize);
+				ep->maxpacket);
 
-	if (uac->p_pktsize < prm->max_psize)
+	if (uac->p_pktsize < ep->maxpacket)
 		uac->p_pktsize_residue = uac->p_framesize *
 			(params->p_srate % uac->p_interval);
 	else
@@ -457,7 +457,7 @@ int u_audio_start_playback(struct g_audio *audio_dev)
 			req->context = &prm->ureq[i];
 			req->length = req_len;
 			req->complete = u_audio_iso_complete;
-			req->buf = prm->rbuf + i * prm->max_psize;
+			req->buf = prm->rbuf + i * ep->maxpacket;
 		}
 
 		if (usb_ep_queue(ep, prm->ureq[i].req, GFP_ATOMIC))
-- 
2.24.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] usb: gadget: u_audio: Fix high-speed max packet size
  2020-01-17 10:40         ` [PATCH] usb: gadget: u_audio: Fix high-speed max packet size John Keeping
@ 2020-01-19 14:53           ` Pavel Hofman
  2020-01-24 12:16             ` Pavel Hofman
  2020-01-24 12:52           ` Felipe Balbi
  1 sibling, 1 reply; 13+ messages in thread
From: Pavel Hofman @ 2020-01-19 14:53 UTC (permalink / raw)
  To: John Keeping; +Cc: linux-usb, Felipe Balbi


Dne 17. 01. 20 v 11:40 John Keeping napsal(a):
> On Thu, 16 Jan 2020 16:39:50 +0100
> Pavel Hofman <pavel.hofman@ivitera.com> wrote:
> 
>>> I've taken a look at this and the patch below fixes it in my simple
>>> testing.  But note that this doesn't adjust the PCM's min_period_bytes
>>> which will be necessary if you want to minimize latency with an adjusted
>>> high-speed bInterval setting.
>>>    
>>
>> Please can I ask you to submit your patch? IMO your perhaps slightly
>> suboptimal solution is much better than the current broken version.
> 
> Yes, the patch is definitely an improvement.  I thought it would be
> picked up from the earlier mail, but I think Patchwork requires the
> subject to match, so I'm including it again here.
> 
> Are you able to provide a Tested-by for this change?


Testing looks OK, thanks a lot!

Tested-by: Pavel Hofman  <pavel.hofman@ivitera.com>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] usb: gadget: u_audio: Fix high-speed max packet size
  2020-01-19 14:53           ` Pavel Hofman
@ 2020-01-24 12:16             ` Pavel Hofman
  0 siblings, 0 replies; 13+ messages in thread
From: Pavel Hofman @ 2020-01-24 12:16 UTC (permalink / raw)
  To: John Keeping; +Cc: linux-usb, Felipe Balbi


Dne 19. 01. 20 v 15:53 Pavel Hofman napsal(a):
> 
> Dne 17. 01. 20 v 11:40 John Keeping napsal(a):
>> On Thu, 16 Jan 2020 16:39:50 +0100
>> Pavel Hofman <pavel.hofman@ivitera.com> wrote:
>>
>>>> I've taken a look at this and the patch below fixes it in my simple
>>>> testing.  But note that this doesn't adjust the PCM's min_period_bytes
>>>> which will be necessary if you want to minimize latency with an 
>>>> adjusted
>>>> high-speed bInterval setting.
>>>
>>> Please can I ask you to submit your patch? IMO your perhaps slightly
>>> suboptimal solution is much better than the current broken version.
>>
>> Yes, the patch is definitely an improvement.  I thought it would be
>> picked up from the earlier mail, but I think Patchwork requires the
>> subject to match, so I'm including it again here.
>>
>> Are you able to provide a Tested-by for this change?
> 
> 
> Testing looks OK, thanks a lot!
> 
> Tested-by: Pavel Hofman  <pavel.hofman@ivitera.com>
> 

Please may I ask for finishing the patch submittal procedure, when it is 
already prepared and useful?

Thanks a lot,

Pavel.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] usb: gadget: u_audio: Fix high-speed max packet size
  2020-01-17 10:40         ` [PATCH] usb: gadget: u_audio: Fix high-speed max packet size John Keeping
  2020-01-19 14:53           ` Pavel Hofman
@ 2020-01-24 12:52           ` Felipe Balbi
  1 sibling, 0 replies; 13+ messages in thread
From: Felipe Balbi @ 2020-01-24 12:52 UTC (permalink / raw)
  To: John Keeping, Pavel Hofman, Greg Kroah-Hartman; +Cc: linux-usb

[-- Attachment #1: Type: text/plain, Size: 1921 bytes --]


Hi,

John Keeping <john@metanate.com> writes:

> On Thu, 16 Jan 2020 16:39:50 +0100
> Pavel Hofman <pavel.hofman@ivitera.com> wrote:
>
>> > I've taken a look at this and the patch below fixes it in my simple
>> > testing.  But note that this doesn't adjust the PCM's min_period_bytes
>> > which will be necessary if you want to minimize latency with an adjusted
>> > high-speed bInterval setting.
>> >   
>> 
>> Please can I ask you to submit your patch? IMO your perhaps slightly 
>> suboptimal solution is much better than the current broken version.
>
> Yes, the patch is definitely an improvement.  I thought it would be
> picked up from the earlier mail, but I think Patchwork requires the
> subject to match, so I'm including it again here.
>
> Are you able to provide a Tested-by for this change?
>
> -- >8 --
> Prior to commit eb9fecb9e69b ("usb: gadget: f_uac2: split out audio
> core") the maximum packet size was calculated only from the high-speed
> descriptor but now we use the largest of the full-speed and high-speed
> descriptors.
>
> This is correct, but the full-speed value is likely to be higher than
> that for high-speed and this leads to submitting requests for OUT
> transfers (received by the gadget) which are larger than the endpoint's
> maximum packet size.  These are rightly rejected by the gadget core.
>
> config_ep_by_speed() already sets up the correct maximum packet size for
> the enumerated speed in the usb_ep structure, so we can simply use this
> instead of the overall value that has been used to allocate buffers for
> requests.
>
> Note that the minimum period for ALSA is still set from the largest
> value, and this is unavoidable because it's possible to open the audio
> device before the gadget has been enumerated.
>
> Signed-off-by: John Keeping <john@metanate.com>

Acked-by: Felipe Balbi <balbi@kernel.org>

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-10  7:29 USB:UAC2: Incorrect req->length > maxpacket*mc Pavel Hofman
2020-01-10 11:28 ` [PATCH] usb: gadget: f_uac2: fix packet size calculation John Keeping
2020-01-11  9:12   ` Pavel Hofman
2020-01-11  9:31 ` USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found Pavel Hofman
2020-01-11  9:49   ` Pavel Hofman
2020-01-14 18:39   ` Pavel Hofman
2020-01-14 20:04     ` John Keeping
2020-01-14 20:21       ` Pavel Hofman
2020-01-16 15:39       ` Pavel Hofman
2020-01-17 10:40         ` [PATCH] usb: gadget: u_audio: Fix high-speed max packet size John Keeping
2020-01-19 14:53           ` Pavel Hofman
2020-01-24 12:16             ` Pavel Hofman
2020-01-24 12:52           ` Felipe Balbi

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git