USB:UAC2: Incorrect req->length > maxpacket*mc

USB:UAC2: Incorrect req->length > maxpacket*mc

[Pavel Hofman]

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.

[PATCH] usb: gadget: f_uac2: fix packet size calculation

[John Keeping]

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

Re: [PATCH] usb: gadget: f_uac2: fix packet size calculation

[Pavel Hofman]

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.

Re: USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found

[Pavel Hofman]

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.

Re: USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found

[Pavel Hofman]

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

Re: USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found

[Pavel Hofman]

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.

Re: USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found

[John Keeping]

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

Re: USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found

[Pavel Hofman]

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.

Re: USB:UAC2: Incorrect req->length > maxpacket*mc - cause likely found

[Pavel Hofman]

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.

[PATCH] usb: gadget: u_audio: Fix high-speed max packet size

[John Keeping]

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

Re: [PATCH] usb: gadget: u_audio: Fix high-speed max packet size

[Pavel Hofman]

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>

Re: [PATCH] usb: gadget: u_audio: Fix high-speed max packet size

[Pavel Hofman]

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.

Re: [PATCH] usb: gadget: u_audio: Fix high-speed max packet size

[Felipe Balbi]

[-- 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 --]

Re: [PATCH] usb: gadget: u_audio: Fix high-speed max packet size

[Pavel Hofman]

Dne 24. 01. 20 v 13:16 Pavel Hofman napsal(a):
> 
> 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>
>>
> 

I apologize for a basic question - please which official repository to 
check status of a gadget patch after being accepted? Thanks a lot for 
the information.

Best regards,

Pavel.

Re: [PATCH] usb: gadget: u_audio: Fix high-speed max packet size

[John Keeping]

On Fri, 31 Jan 2020 11:30:11 +0100
Pavel Hofman <pavel.hofman@ivitera.com> wrote:

> Dne 24. 01. 20 v 13:16 Pavel Hofman napsal(a):
> > 
> > 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>
> >>  
> >   
> 
> I apologize for a basic question - please which official repository to 
> check status of a gadget patch after being accepted? Thanks a lot for 
> the information.

If you have a kernel tree, you can ask the MAINTAINERS file:

	./scripts/get_maintainer.pl --scm -f drivers/usb/gadget/function/u_audio.c

I'd expect this to appear in Felipe's tree first at:

	https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git

but I don't see it yet.  I guess it won't be picked up until after the
merge window.


Regards,
John

Re: [PATCH] usb: gadget: u_audio: Fix high-speed max packet size

[Pavel Hofman]

Hi John,

>> I apologize for a basic question - please which official repository to
>> check status of a gadget patch after being accepted? Thanks a lot for
>> the information.
> 
> If you have a kernel tree, you can ask the MAINTAINERS file:
> 
> 	./scripts/get_maintainer.pl --scm -f drivers/usb/gadget/function/u_audio.c
> 
> I'd expect this to appear in Felipe's tree first at:
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
> 
> but I don't see it yet.  I guess it won't be picked up until after the
> merge window.
> 

Thanks a lot for you info. How does the maintainer pick a patch from the 
flood of messages? Some extra headers (Tested:by, Acked-by:) are sent 
separately by different people, does the maintainer have to keep track 
of all of that manually?

Thanks for your enlightenment.

Pavel.

Re: [PATCH] usb: gadget: u_audio: Fix high-speed max packet size

[Felipe Balbi]

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

Pavel Hofman <pavel.hofman@ivitera.com> writes:

> Hi John,
>
>>> I apologize for a basic question - please which official repository to
>>> check status of a gadget patch after being accepted? Thanks a lot for
>>> the information.
>> 
>> If you have a kernel tree, you can ask the MAINTAINERS file:
>> 
>> 	./scripts/get_maintainer.pl --scm -f drivers/usb/gadget/function/u_audio.c
>> 
>> I'd expect this to appear in Felipe's tree first at:
>> 
>> 	https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
>> 
>> but I don't see it yet.  I guess it won't be picked up until after the
>> merge window.
>> 
>
> Thanks a lot for you info. How does the maintainer pick a patch from the 
> flood of messages? Some extra headers (Tested:by, Acked-by:) are sent 
> separately by different people, does the maintainer have to keep track 
> of all of that manually?

I had acked it, so I was under the expectation that Greg would pick it
up. Unfortunately, it seems like it has slipped through the cracks. I
have now queued it in my testing/fixes branch. It'll be sent forward
after -rc1 is tagged.

-- 
balbi

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