Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS

Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS

[Andrey Konovalov]

Hi Alan,

I'm working on implementing a USB proxy that uses libusb on one side
and Raw Gadget on the other. The idea is to pass all requests received
from the host via Raw Gadget to the proxied device via libusb and
report back the responses.

However, I've stumbled upon an issue with non-0-length control
requests that get stalled by the proxied device.

To pass an OUT control request to the device via libusb I need to
first retrieve the data for the request from the host. And with Raw
Gadget I can do that via USB_RAW_IOCTL_EP0_READ, which internally
calls usb_ep_queue, waits for its completion, and copies the data to
userspace.

But the problem is that once I retrieve the data, the request is
automatically acked. Thus, if the proxied device stalls the request,
it's already too late to stall it via Raw Gadget.

AFAIU, GadgetFS works the same way.

Is there a way to work around this? If this requires a change is Raw
Gadget, that is fine. But I'm wondering if this is possible to do at
all with the USB Gadget API: AFAIU, we have to either stall or
retrieve the data; we cannot do both.

If this is indeed impossible, perhaps you know if there's a way to
directly use usbfs to separately submit the control request header to
the proxied device to figure out if it wants to stall? And only then
retrieve the data from the host via Raw Gadget if the device doesn't
stall.

Thank you!

Re: Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS

[Alan Stern]

On Sat, Jun 15, 2024 at 12:31:12AM +0200, Andrey Konovalov wrote:
> Hi Alan,
> 
> I'm working on implementing a USB proxy that uses libusb on one side
> and Raw Gadget on the other. The idea is to pass all requests received
> from the host via Raw Gadget to the proxied device via libusb and
> report back the responses.
> 
> However, I've stumbled upon an issue with non-0-length control
> requests that get stalled by the proxied device.
> 
> To pass an OUT control request to the device via libusb I need to
> first retrieve the data for the request from the host. And with Raw
> Gadget I can do that via USB_RAW_IOCTL_EP0_READ, which internally
> calls usb_ep_queue, waits for its completion, and copies the data to
> userspace.
> 
> But the problem is that once I retrieve the data, the request is
> automatically acked. Thus, if the proxied device stalls the request,
> it's already too late to stall it via Raw Gadget.
> 
> AFAIU, GadgetFS works the same way.
> 
> Is there a way to work around this? If this requires a change is Raw
> Gadget, that is fine. But I'm wondering if this is possible to do at
> all with the USB Gadget API: AFAIU, we have to either stall or
> retrieve the data; we cannot do both.

Yeah, this is a known weakness of the Gadget API.  There's no way to do 
it at present.

> If this is indeed impossible, perhaps you know if there's a way to
> directly use usbfs to separately submit the control request header to
> the proxied device to figure out if it wants to stall? And only then
> retrieve the data from the host via Raw Gadget if the device doesn't
> stall.

usbfs allows the user to send a complete transfer, not a partial one 
(i.e., just the SETUP transaction).  Besides, it's not possible for a 
device to stall a SETUP packet -- the USB-2.0 spec requires devices to 
respond to SETUP with ACK every time (section 8.5.3) -- so this approach 
won't solve the problem anyway. And even if it did, you'd still have to 
handle the situation where the proxy device accepts the SETUP packet and 
the data but then stalls during the Status stage of the transfer.

There has been a patch posted to support UDC drivers that don't 
automatically acknowledge non-zero-length control-OUT transfers.  But 
the patch hasn't been merged, and even if it were, all the existing UDC 
drivers would still need to be updated.

Sorry, but the kernel just doesn't provide any way to do this.

Alan Stern

Re: Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS

[Andrey Konovalov]

On Sat, Jun 15, 2024 at 4:12 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> Yeah, this is a known weakness of the Gadget API.  There's no way to do
> it at present.

Ack.

> usbfs allows the user to send a complete transfer, not a partial one
> (i.e., just the SETUP transaction).  Besides, it's not possible for a
> device to stall a SETUP packet -- the USB-2.0 spec requires devices to
> respond to SETUP with ACK every time (section 8.5.3) -- so this approach
> won't solve the problem anyway. And even if it did, you'd still have to
> handle the situation where the proxy device accepts the SETUP packet and
> the data but then stalls during the Status stage of the transfer.

Ah, so dealing with this on the usbfs side is impossible.

> There has been a patch posted to support UDC drivers that don't
> automatically acknowledge non-zero-length control-OUT transfers.  But
> the patch hasn't been merged, and even if it were, all the existing UDC
> drivers would still need to be updated.

This series below is the one you're referring to, right?

https://lore.kernel.org/linux-kernel/20190124030228.19840-1-paul.elder@ideasonboard.com/

Do you know why it wasn't merged? (CC Paul). There are no comments on
the latest version I managed to find.

Also, just to check my understanding: with that series in place and
assuming the UDC drivers are updated, a gadget driver would need to
first do usb_ep_queue with the proper length and explicit_status ==
true to get the data for the control OUT request, and then either do
usb_ep_queue again with length 0 to ack or do usb_ep_set_halt to
stall?

Thank you!

Re: Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS

[Alan Stern]

On Sat, Jun 15, 2024 at 10:49:46PM +0200, Andrey Konovalov wrote:
> On Sat, Jun 15, 2024 at 4:12 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > There has been a patch posted to support UDC drivers that don't
> > automatically acknowledge non-zero-length control-OUT transfers.  But
> > the patch hasn't been merged, and even if it were, all the existing UDC
> > drivers would still need to be updated.
> 
> This series below is the one you're referring to, right?
> 
> https://lore.kernel.org/linux-kernel/20190124030228.19840-1-paul.elder@ideasonboard.com/

Yes, that's it.  I'm impressed that you were able to find it; I had lost 
track of it.

> Do you know why it wasn't merged? (CC Paul). There are no comments on
> the latest version I managed to find.

I guess Felipe Balbi (the USB Gadget maintainer at the time) just wasn't 
very interested in fixing the problem.

> Also, just to check my understanding: with that series in place and
> assuming the UDC drivers are updated, a gadget driver would need to
> first do usb_ep_queue with the proper length and explicit_status ==
> true to get the data for the control OUT request, and then either do
> usb_ep_queue again with length 0 to ack or do usb_ep_set_halt to
> stall?

Yes, that's how it worked.  Alternatively, if the gadget driver didn't 
set explicit_status in the control-OUT request then the UDC core would 
automatically call usb_ep_queue again with a 0-length transfer to send 
the status.  That way existing gadget drivers would continue to work 
after the UDC drivers were updated, and updated UDC drivers wouldn't 
have to worry about doing an automatic acknowledge only some of the 
time.

Note that in order to avoid breaking things during the transition 
period, it would also be necessary to add a flag to the usb_gadget 
structure, indicating that the UDC driver has been updated to support 
explicit_status.

Alan Stern

PS: There's another weakness in the Gadget API which you might possibly 
run across in your project.  It's less likely to arise because it 
involves lengthy delays.

Say there's a control transfer with delayed status, and the gadget 
driver delays for so long that the host times out the transfer.  Then 
the host starts a new control transfer before the gadget driver queues 
its status reply.  Since the Gadget API doesn't have any way to indicate 
which control transfer a usb_request was meant for, the reply that was 
meant for the old transfer would get sent to the host, and the host 
would think it was a reply to the new transfer.

This problem could be solved by adding a unique ID tag to each 
usb_request, and passing the transfer ID as an extra argument to the 
gadget driver's setup() callback.  That would explicitly indicate which 
transfer a request was meant for.  But doing this would also require 
updating every function driver and every UDC driver.  Probably not worth 
the effort, considering how unlikely it is that the situation will ever 
arise.

Re: Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS

[Andrey Konovalov]

On Sun, Jun 16, 2024 at 4:33 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> > This series below is the one you're referring to, right?
> >
> > https://lore.kernel.org/linux-kernel/20190124030228.19840-1-paul.elder@ideasonboard.com/
>
> Yes, that's it.  I'm impressed that you were able to find it; I had lost
> track of it.

Luckily, it was one of the first google results on the topic :)

> > Do you know why it wasn't merged? (CC Paul). There are no comments on
> > the latest version I managed to find.
>
> I guess Felipe Balbi (the USB Gadget maintainer at the time) just wasn't
> very interested in fixing the problem.

Ok, I see.

> > Also, just to check my understanding: with that series in place and
> > assuming the UDC drivers are updated, a gadget driver would need to
> > first do usb_ep_queue with the proper length and explicit_status ==
> > true to get the data for the control OUT request, and then either do
> > usb_ep_queue again with length 0 to ack or do usb_ep_set_halt to
> > stall?
>
> Yes, that's how it worked.  Alternatively, if the gadget driver didn't
> set explicit_status in the control-OUT request then the UDC core would
> automatically call usb_ep_queue again with a 0-length transfer to send
> the status.  That way existing gadget drivers would continue to work
> after the UDC drivers were updated, and updated UDC drivers wouldn't
> have to worry about doing an automatic acknowledge only some of the
> time.
>
> Note that in order to avoid breaking things during the transition
> period, it would also be necessary to add a flag to the usb_gadget
> structure, indicating that the UDC driver has been updated to support
> explicit_status.

Ack, thank you for explaining!

I've been collecting different kinds of non-critical issues and
inconsistencies within the Gadget subsystem I hit while testing Raw
Gadget. It's unlikely I'll get to working on them in the foreseeable
future, but it's good to know what needs fixing should the need arise.

So far, I have:

- Allow stalling non-0-length control OUT transfers
(https://github.com/xairy/raw-gadget/issues/71);
- Contain USB_GADGET_DELAYED_STATUS within composite framework
(https://github.com/xairy/raw-gadget/issues/70);
- Support isochronous transfers in Dummy HCD/UDC
(https://github.com/xairy/raw-gadget/issues/72);
- Fix dwc2 issuing disconnect instead of reset
(https://github.com/xairy/raw-gadget/issues/48);
- dwc3 doesn't support low speed, even through a comment says every
UDC must (https://github.com/xairy/raw-gadget/issues/41#issuecomment-1783022764).

> PS: There's another weakness in the Gadget API which you might possibly
> run across in your project.  It's less likely to arise because it
> involves lengthy delays.
>
> Say there's a control transfer with delayed status, and the gadget
> driver delays for so long that the host times out the transfer.  Then
> the host starts a new control transfer before the gadget driver queues
> its status reply.  Since the Gadget API doesn't have any way to indicate
> which control transfer a usb_request was meant for, the reply that was
> meant for the old transfer would get sent to the host, and the host
> would think it was a reply to the new transfer.
>
> This problem could be solved by adding a unique ID tag to each
> usb_request, and passing the transfer ID as an extra argument to the
> gadget driver's setup() callback.  That would explicitly indicate which
> transfer a request was meant for.  But doing this would also require
> updating every function driver and every UDC driver.  Probably not worth
> the effort, considering how unlikely it is that the situation will ever
> arise.

Ah, good to know! My experience confirms that this is unlikely: I
haven't hit this issue in practice yet.

P.S. By the way, I've been lately using the dwc3-based UDC built into
one of my ThinkPad laptops for testing gadget drivers. I had to switch
a hidden BIOS option to enable it (which was not straightforward), but
it's nice to avoid dealing with external boards. I've written an
article about this, in case you're interested:
https://xairy.io/articles/thinkpad-xdci.

Thank you!

Re: Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS

[Alan Stern]

On Mon, Jun 17, 2024 at 12:42:56AM +0200, Andrey Konovalov wrote:
> I've been collecting different kinds of non-critical issues and
> inconsistencies within the Gadget subsystem I hit while testing Raw
> Gadget. It's unlikely I'll get to working on them in the foreseeable
> future, but it's good to know what needs fixing should the need arise.
> 
> So far, I have:
> 
> - Allow stalling non-0-length control OUT transfers
> (https://github.com/xairy/raw-gadget/issues/71);
> - Contain USB_GADGET_DELAYED_STATUS within composite framework
> (https://github.com/xairy/raw-gadget/issues/70);
> - Support isochronous transfers in Dummy HCD/UDC
> (https://github.com/xairy/raw-gadget/issues/72);

As it happens, I wrote a patch (meant mainly for testing, not production 
use) that implemented a start at isochronous support in dummy-hcd.  It 
was far from complete, but it could be expanded.

However, it's another thing I have lost track of.  It may have been 
posted to linux-usb at one point, but that would have been quite some 
time ago.  Probably before well lore.kernel.org existed.  (I vaguely 
remember working on it before moving to my current home, which means 
in 2008 or earlier.)

Alan Stern

Re: Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS

[Greg KH]

On Sun, Jun 16, 2024 at 10:10:33PM -0400, Alan Stern wrote:
> However, it's another thing I have lost track of.  It may have been 
> posted to linux-usb at one point, but that would have been quite some 
> time ago.  Probably before well lore.kernel.org existed.  (I vaguely 
> remember working on it before moving to my current home, which means 
> in 2008 or earlier.)

I think lore is populated with all of the linux-usb mailing list
archives, but we might have missed from before when there was -user and
-devel versions of the list.

If needed, I can try to dig up my older archives from cold storage and
get Konstantin to import them.

thanks,

greg k-h

Partial isochronous support for dummy-hcd [was: Re: Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS]

[Alan Stern]

On Mon, Jun 17, 2024 at 08:29:12AM +0200, Greg KH wrote:
> On Sun, Jun 16, 2024 at 10:10:33PM -0400, Alan Stern wrote:
> > However, it's another thing I have lost track of.  It may have been 
> > posted to linux-usb at one point, but that would have been quite some 
> > time ago.  Probably before well lore.kernel.org existed.  (I vaguely 
> > remember working on it before moving to my current home, which means 
> > in 2008 or earlier.)
> 
> I think lore is populated with all of the linux-usb mailing list
> archives, but we might have missed from before when there was -user and
> -devel versions of the list.
> 
> If needed, I can try to dig up my older archives from cold storage and
> get Konstantin to import them.

No need -- I found a copy of it on an old backup drive.  It probably got 
left behind when I switched my system over from using 32-bit userspace 
to 64 bits.

For the record, the patch is appended below.  The file on the backup 
drive was dated May 2006 (before the 2.6.17 kernel was released!); the 
original work may have been even earlier.

Alan Stern



Index: usb/drivers/usb/gadget/dummy_hcd.c
===================================================================
--- usb.orig/drivers/usb/gadget/dummy_hcd.c
+++ usb/drivers/usb/gadget/dummy_hcd.c
@@ -84,6 +84,9 @@ struct dummy_ep {
 	struct usb_gadget		*gadget;
 	const struct usb_endpoint_descriptor *desc;
 	struct usb_ep			ep;
+	int				iso_desc_num;
+	int				iso_status;
+	int				interval_left;
 	unsigned			halted : 1;
 	unsigned			already_seen : 1;
 	unsigned			setup_stage : 1;
@@ -808,6 +811,7 @@ usb_gadget_register_driver (struct usb_g
 
 	dum->gadget.ep0 = &dum->ep [0].ep;
 	dum->ep [0].ep.maxpacket = 64;
+	dum->ep [0].setup_stage = 1;
 	list_del_init (&dum->ep [0].ep.ep_list);
 	INIT_LIST_HEAD(&dum->fifo_req.queue);
 
@@ -1017,8 +1021,7 @@ static int dummy_urb_enqueue (
 
 	list_add_tail (&urbp->urbp_list, &dum->urbp_list);
 	urb->hcpriv = urbp;
-	if (usb_pipetype (urb->pipe) == PIPE_CONTROL)
-		urb->error_count = 1;		/* mark as a new urb */
+	urb->error_count = 0;
 
 	/* kick the scheduler, it'll do the rest */
 	if (!timer_pending (&dum->timer))
@@ -1056,13 +1059,44 @@ static int
 transfer (struct dummy *dum, struct urb *urb, struct dummy_ep *ep, int limit)
 {
 	struct dummy_request	*req;
+	struct usb_iso_packet_descriptor *iso;
+	int			to_host;
+
+	to_host = usb_pipein (urb->pipe);
+	iso = NULL;
+	if (usb_pipetype (urb->pipe) == PIPE_ISOCHRONOUS)
+		iso = &urb->iso_frame_desc [ep->iso_desc_num];
 
 top:
+	/* isochronous transfers go through even without a responder */
+	if (iso && list_empty (&ep->queue)) {
+		int		limit2 = limit;
+		int		maxlen = ep->ep.maxpacket;
+
+		while (maxlen <= limit2) {
+			limit2 -= maxlen;
+			if (to_host)
+				ep->iso_status = iso->status = -EPROTO;
+			else {
+				iso->status = 0;
+				iso->actual_length = iso->length;
+				urb->actual_length += iso->length;
+			}
+			++iso;
+			if (++ep->iso_desc_num >= urb->number_of_packets) {
+				maybe_set_status (urb, ep->iso_status);
+				break;
+			}
+		}
+		return limit;
+	}
+
 	/* if there's no request queued, the device is NAKing; return */
 	list_for_each_entry (req, &ep->queue, queue) {
 		unsigned	host_len, dev_len, len;
-		int		is_short, to_host;
+		int		is_short;
 		int		rescan = 0;
+		int		ustatus;
 
 		/* 1..N packets of ep->ep.maxpacket each ... the last one
 		 * may be short (including zero length).
@@ -1071,24 +1105,22 @@ top:
 		 * (length mod maxpacket zero, and 'zero' flag); they always
 		 * terminate reads.
 		 */
-		host_len = urb->transfer_buffer_length - urb->actual_length;
+		host_len = (iso ? iso->length : urb->transfer_buffer_length -
+				urb->actual_length);
 		dev_len = req->req.length - req->req.actual;
 		len = min (host_len, dev_len);
 
 		/* FIXME update emulated data toggle too */
 
-		to_host = usb_pipein (urb->pipe);
 		if (unlikely (len == 0))
 			is_short = 1;
 		else {
 			char		*ubuf, *rbuf;
 
 			/* not enough bandwidth left? */
-			if (limit < ep->ep.maxpacket && limit < len)
+			if (limit < ep->ep.maxpacket && limit < host_len)
 				break;
 			len = min (len, (unsigned) limit);
-			if (len == 0)
-				break;
 
 			/* use an extra pass for the final short packet */
 			if (len > ep->ep.maxpacket) {
@@ -1098,15 +1130,15 @@ top:
 			is_short = (len % ep->ep.maxpacket) != 0;
 
 			/* else transfer packet(s) */
-			ubuf = urb->transfer_buffer + urb->actual_length;
+			ubuf = urb->transfer_buffer + (iso ?
+					iso->offset : urb->actual_length);
 			rbuf = req->req.buf + req->req.actual;
 			if (to_host)
 				memcpy (ubuf, rbuf, len);
 			else
 				memcpy (rbuf, ubuf, len);
-			ep->last_io = jiffies;
 
-			limit -= len;
+			limit -= (iso ? ep->ep.maxpacket : len);
 			urb->actual_length += len;
 			req->req.actual += len;
 		}
@@ -1119,21 +1151,18 @@ top:
 		 * need to emulate such data-in-flight.  so we only show part
 		 * of the URB_SHORT_NOT_OK effect: completion status.
 		 */
+		ustatus = 0;
 		if (is_short) {
 			if (host_len == dev_len) {
 				req->req.status = 0;
-				maybe_set_status (urb, 0);
 			} else if (to_host) {
 				req->req.status = 0;
 				if (dev_len > host_len)
-					maybe_set_status (urb, -EOVERFLOW);
-				else
-					maybe_set_status (urb,
-						(urb->transfer_flags
-							& URB_SHORT_NOT_OK)
-						? -EREMOTEIO : 0);
-			} else if (!to_host) {
-				maybe_set_status (urb, 0);
+					ustatus = -EOVERFLOW;
+				else if (urb->transfer_flags &
+						URB_SHORT_NOT_OK)
+					ustatus = -EREMOTEIO;
+			} else {
 				if (host_len > dev_len)
 					req->req.status = -EOVERFLOW;
 				else
@@ -1147,10 +1176,25 @@ top:
 				req->req.status = 0;
 			if (urb->transfer_buffer_length == urb->actual_length
 					&& !(urb->transfer_flags
-						& URB_ZERO_PACKET)) {
-				maybe_set_status (urb, 0);
+						& URB_ZERO_PACKET))
+				;		/* URB is done */
+			else if (!iso)
+				ustatus = 1;	/* URB continues */
+		}
+
+		if (iso) {
+			iso->actual_length = len;
+			iso->status = ustatus;
+			if (ustatus) {
+				ep->iso_status = ustatus;
+				++urb->error_count;
 			}
+			++iso;
+			if (++ep->iso_desc_num >= urb->number_of_packets)
+				maybe_set_status (urb, ep->iso_status);
 		}
+		else if (ustatus <= 0)
+			maybe_set_status (urb, ustatus);
 
 		/* device side completion --> continuable */
 		if (req->req.status != -EINPROGRESS) {
@@ -1309,10 +1353,6 @@ restart:
 		if (ep->already_seen)
 			continue;
 		ep->already_seen = 1;
-		if (ep == &dum->ep [0] && urb->error_count) {
-			ep->setup_stage = 1;	/* a new urb */
-			urb->error_count = 0;
-		}
 		if (ep->halted && !ep->setup_stage) {
 			/* NOTE: must not be iso! */
 			dev_dbg (dummy_dev(dum), "ep %s halted, urb %p\n",
@@ -1323,7 +1363,7 @@ restart:
 		/* FIXME make sure both ends agree on maxpacket */
 
 		/* handle control requests */
-		if (ep == &dum->ep [0] && ep->setup_stage) {
+		if (ep->setup_stage) {
 			struct usb_ctrlrequest		setup;
 			int				value = 1;
 			struct dummy_ep			*ep2;
@@ -1483,7 +1523,7 @@ restart:
 				if (value >= 0) {
 					/* no delays (max 64KB data stage) */
 					limit = 64*1024;
-					goto treat_control_like_bulk;
+					goto treat_like_bulk;
 				}
 				/* error, see below */
 			}
@@ -1504,25 +1544,24 @@ restart:
 		limit = total;
 		switch (usb_pipetype (urb->pipe)) {
 		case PIPE_ISOCHRONOUS:
-			/* FIXME is it urb->interval since the last xfer?
-			 * use urb->iso_frame_desc[i].
-			 * complete whether or not ep has requests queued.
-			 * report random errors, to debug drivers.
+			/* FIXME: report random errors, to debug drivers.
 			 */
+			if (--ep->interval_left > 0)
+				break;
+			ep->interval_left = urb->interval;
 			limit = max (limit, periodic_bytes (dum, ep));
-			maybe_set_status (urb, -ENOSYS);
-			break;
+			goto treat_like_bulk;
 
 		case PIPE_INTERRUPT:
-			/* FIXME is it urb->interval since the last xfer?
-			 * this almost certainly polls too fast.
-			 */
+			if (--ep->interval_left > 0)
+				break;
+			ep->interval_left = urb->interval;
 			limit = max (limit, periodic_bytes (dum, ep));
 			/* FALLTHROUGH */
 
 		// case PIPE_BULK:  case PIPE_CONTROL:
 		default:
-		treat_control_like_bulk:
+		treat_like_bulk:
 			ep->last_io = jiffies;
 			total = transfer (dum, urb, ep, limit);
 			break;
@@ -1536,8 +1575,13 @@ return_urb:
 		urb->hcpriv = NULL;
 		list_del (&urbp->urbp_list);
 		kfree (urbp);
-		if (ep)
-			ep->already_seen = ep->setup_stage = 0;
+		if (ep) {
+			ep->already_seen = 0;
+			ep->iso_desc_num = 0;
+			ep->iso_status = 0;
+			if (ep == &dum->ep [0])
+				ep->setup_stage = 1;
+		}
 
 		spin_unlock (&dum->lock);
 		usb_hcd_giveback_urb (dummy_to_hcd(dum), urb, NULL);

Re: Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS

[Paul Elder]

On Sat, Jun 15, 2024 at 10:33:28PM -0400, Alan Stern wrote:
> On Sat, Jun 15, 2024 at 10:49:46PM +0200, Andrey Konovalov wrote:
> > On Sat, Jun 15, 2024 at 4:12 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > There has been a patch posted to support UDC drivers that don't
> > > automatically acknowledge non-zero-length control-OUT transfers.  But
> > > the patch hasn't been merged, and even if it were, all the existing UDC
> > > drivers would still need to be updated.
> > 
> > This series below is the one you're referring to, right?
> > 
> > https://lore.kernel.org/linux-kernel/20190124030228.19840-1-paul.elder@ideasonboard.com/
> 
> Yes, that's it.  I'm impressed that you were able to find it; I had lost 
> track of it.
> 
> > Do you know why it wasn't merged? (CC Paul). There are no comments on
> > the latest version I managed to find.
> 
> I guess Felipe Balbi (the USB Gadget maintainer at the time) just wasn't 
> very interested in fixing the problem.

So that's why we never continued with merging it...

Is it time to dust it off and try to upstream it again? :)


Paul

> 
> > Also, just to check my understanding: with that series in place and
> > assuming the UDC drivers are updated, a gadget driver would need to
> > first do usb_ep_queue with the proper length and explicit_status ==
> > true to get the data for the control OUT request, and then either do
> > usb_ep_queue again with length 0 to ack or do usb_ep_set_halt to
> > stall?
> 
> Yes, that's how it worked.  Alternatively, if the gadget driver didn't 
> set explicit_status in the control-OUT request then the UDC core would 
> automatically call usb_ep_queue again with a 0-length transfer to send 
> the status.  That way existing gadget drivers would continue to work 
> after the UDC drivers were updated, and updated UDC drivers wouldn't 
> have to worry about doing an automatic acknowledge only some of the 
> time.
> 
> Note that in order to avoid breaking things during the transition 
> period, it would also be necessary to add a flag to the usb_gadget 
> structure, indicating that the UDC driver has been updated to support 
> explicit_status.
> 
> Alan Stern
> 
> PS: There's another weakness in the Gadget API which you might possibly 
> run across in your project.  It's less likely to arise because it 
> involves lengthy delays.
> 
> Say there's a control transfer with delayed status, and the gadget 
> driver delays for so long that the host times out the transfer.  Then 
> the host starts a new control transfer before the gadget driver queues 
> its status reply.  Since the Gadget API doesn't have any way to indicate 
> which control transfer a usb_request was meant for, the reply that was 
> meant for the old transfer would get sent to the host, and the host 
> would think it was a reply to the new transfer.
> 
> This problem could be solved by adding a unique ID tag to each 
> usb_request, and passing the transfer ID as an extra argument to the 
> gadget driver's setup() callback.  That would explicitly indicate which 
> transfer a request was meant for.  But doing this would also require 
> updating every function driver and every UDC driver.  Probably not worth 
> the effort, considering how unlikely it is that the situation will ever 
> arise.

Re: Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS

[Alan Stern]

On Wed, Jun 19, 2024 at 10:44:15AM +0900, Paul Elder wrote:
> On Sat, Jun 15, 2024 at 10:33:28PM -0400, Alan Stern wrote:
> > On Sat, Jun 15, 2024 at 10:49:46PM +0200, Andrey Konovalov wrote:
> > > On Sat, Jun 15, 2024 at 4:12 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > There has been a patch posted to support UDC drivers that don't
> > > > automatically acknowledge non-zero-length control-OUT transfers.  But
> > > > the patch hasn't been merged, and even if it were, all the existing UDC
> > > > drivers would still need to be updated.
> > > 
> > > This series below is the one you're referring to, right?
> > > 
> > > https://lore.kernel.org/linux-kernel/20190124030228.19840-1-paul.elder@ideasonboard.com/
> > 
> > Yes, that's it.  I'm impressed that you were able to find it; I had lost 
> > track of it.
> > 
> > > Do you know why it wasn't merged? (CC Paul). There are no comments on
> > > the latest version I managed to find.
> > 
> > I guess Felipe Balbi (the USB Gadget maintainer at the time) just wasn't 
> > very interested in fixing the problem.
> 
> So that's why we never continued with merging it...
> 
> Is it time to dust it off and try to upstream it again? :)

You certainly could try.  But be sure to add a supports_explicit_status 
flag to the usb_gadget structure.

Alan Stern

Re: Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS

[Andrey Konovalov]

On Wed, Jun 19, 2024 at 3:44 AM Paul Elder <paul.elder@ideasonboard.com> wrote:
>
> > > This series below is the one you're referring to, right?
> > >
> > > https://lore.kernel.org/linux-kernel/20190124030228.19840-1-paul.elder@ideasonboard.com/
> >
> > Yes, that's it.  I'm impressed that you were able to find it; I had lost
> > track of it.
> >
> > > Do you know why it wasn't merged? (CC Paul). There are no comments on
> > > the latest version I managed to find.
> >
> > I guess Felipe Balbi (the USB Gadget maintainer at the time) just wasn't
> > very interested in fixing the problem.
>
> So that's why we never continued with merging it...
>
> Is it time to dust it off and try to upstream it again? :)

Thank would be awesome! :)

If you get around to it, I can add Raw Gadget integration on top and test it.

Do you know, what would be the better board to get with an MUSB UDC
for testing? Ideally, something that supports the mainline kernel.

Thanks!

Re: Stalling non-0-length OUT control requests in Raw Gadget/GadgetFS

[Paul Elder]

On Thu, Jun 20, 2024 at 02:57:34PM +0200, Andrey Konovalov wrote:
> On Wed, Jun 19, 2024 at 3:44 AM Paul Elder <paul.elder@ideasonboard.com> wrote:
> >
> > > > This series below is the one you're referring to, right?
> > > >
> > > > https://lore.kernel.org/linux-kernel/20190124030228.19840-1-paul.elder@ideasonboard.com/
> > >
> > > Yes, that's it.  I'm impressed that you were able to find it; I had lost
> > > track of it.
> > >
> > > > Do you know why it wasn't merged? (CC Paul). There are no comments on
> > > > the latest version I managed to find.
> > >
> > > I guess Felipe Balbi (the USB Gadget maintainer at the time) just wasn't
> > > very interested in fixing the problem.
> >
> > So that's why we never continued with merging it...
> >
> > Is it time to dust it off and try to upstream it again? :)
> 
> Thank would be awesome! :)
> 
> If you get around to it, I can add Raw Gadget integration on top and test it.
> 
> Do you know, what would be the better board to get with an MUSB UDC
> for testing? Ideally, something that supports the mainline kernel.

The one that I used last time was a Pandaboard ES, but also it was 4.18
back then so it's got a lot of dust...


Paul