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

[John Keeping <john@metanate.com>]

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