diff mbox series

dummy_hcd: add isoc support

Message ID 20220126132249.2133168-1-m.grzeschik@pengutronix.de
State New
Headers show
Series dummy_hcd: add isoc support | expand

Commit Message

Michael Grzeschik Jan. 26, 2022, 1:22 p.m. UTC
With this patch, the dummy_hcd gains first support for isoc transfers.
It will complete the whole urb with all packages. Even if the gadget
side did not enqueue any request, the urb will be handled.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---

With this patch it is possible to test the series [1] on any device
using the uvc-gadget code [2].

I added some patches on top of uvc-gadget to prove that it is now
possible to completely remove the configfs parsing and DATA/SETUP event
handling from userspace [3].

To test the gadget, just fill the uvc configfs setup with some script
[4] or even use the modern (but optional) way with gt (gadget-tool) [5]
including libusbgx (uvc/configfs) [6] support and a scheme file
describing the gadget.

[1] https://lore.kernel.org/linux-usb/20220105115527.3592860-1-m.grzeschik@pengutronix.de/

[2] https://git.ideasonboard.org/uvc-gadget.git

[3] https://git.pengutronix.de/cgit/mgr/uvc-gadget/log/?h=ml

[4] https://git.ideasonboard.org/uvc-gadget.git/blob/HEAD:/scripts/uvc-gadget.sh

[5] https://github.com/linux-usb-gadgets/libusbgx

[6] https://github.com/linux-usb-gadgets/gt

 drivers/usb/gadget/udc/dummy_hcd.c | 34 +++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 10 deletions(-)

Comments

Michael Grzeschik Jan. 26, 2022, 6:31 p.m. UTC | #1
On Wed, Jan 26, 2022 at 11:09:23AM -0500, Alan Stern wrote:
>On Wed, Jan 26, 2022 at 02:22:49PM +0100, Michael Grzeschik wrote:
>> With this patch, the dummy_hcd gains first support for isoc transfers.
>> It will complete the whole urb with all packages.
>
>"packets", not "packages".

Right.

>>  Even if the gadget
>> side did not enqueue any request, the urb will be handled.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>
>I don't like this idea.  If support for isochronous transfers is added,
>it should be done correctly.  That is, the implementation should support
>scheduling of transfers, periodic bandwidth reservation, high-bandwidth
>transfers, and so on.
>
>The whole point of dummy-hcd is to emulate a real host controller as
>closely as possible.  Real isochronous transfers do not complete all at
>once.

I agree, that whole isoc support needs proper improvement.

I could/should have added RFC to the patch. As the whole intention of
this code, for now, is to validate the gadget/uvc configfs patchseries
mentioned in the the comments.

With this patch, it is at least possible to get the gadget running on
dummy_hcd and try out the "non-isoc dependent" parts, that are actually
changed, in the mentioned series.

The validation of the payload path, actually using the mentioned isoc
endpoints, is left to the developer who implements the missing parts
you mentioned above.

Michael
Kieran Bingham Jan. 31, 2022, 12:08 p.m. UTC | #2
Hi Michael, Alan,

Quoting Michael Grzeschik (2022-01-26 18:31:38)
> On Wed, Jan 26, 2022 at 11:09:23AM -0500, Alan Stern wrote:
> >On Wed, Jan 26, 2022 at 02:22:49PM +0100, Michael Grzeschik wrote:
> >> With this patch, the dummy_hcd gains first support for isoc transfers.
> >> It will complete the whole urb with all packages.
> >
> >"packets", not "packages".
> 
> Right.
> 
> >>  Even if the gadget
> >> side did not enqueue any request, the urb will be handled.
> >>
> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> >
> >I don't like this idea.  If support for isochronous transfers is added,
> >it should be done correctly.  That is, the implementation should support
> >scheduling of transfers, periodic bandwidth reservation, high-bandwidth
> >transfers, and so on.
> >
> >The whole point of dummy-hcd is to emulate a real host controller as
> >closely as possible.  Real isochronous transfers do not complete all at
> >once.

Being able to at least test uvc-gadget in a virtual environment would
already be a big benefit. As this is emulation, not simulation is it
essential that an exact mapping of the hardware is in place?

Is there anything we can do to support the progression of this
development?

I.e. could we support this method first with a 
  WARN_ONCE("This does not fully emulate Isochronous support");


That would allow infrastructure to be built up that uses this
functionality, which would then in turn feed back into providing a means
to actually test the improvements to the isochronous support on top.


> I agree, that whole isoc support needs proper improvement.
> 
> I could/should have added RFC to the patch. As the whole intention of
> this code, for now, is to validate the gadget/uvc configfs patchseries
> mentioned in the the comments.
> 
> With this patch, it is at least possible to get the gadget running on
> dummy_hcd and try out the "non-isoc dependent" parts, that are actually
> changed, in the mentioned series.
> 
> The validation of the payload path, actually using the mentioned isoc
> endpoints, is left to the developer who implements the missing parts
> you mentioned above.
> 
> Michael
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
Greg KH Jan. 31, 2022, 12:43 p.m. UTC | #3
On Mon, Jan 31, 2022 at 12:08:20PM +0000, Kieran Bingham wrote:
> Hi Michael, Alan,
> 
> Quoting Michael Grzeschik (2022-01-26 18:31:38)
> > On Wed, Jan 26, 2022 at 11:09:23AM -0500, Alan Stern wrote:
> > >On Wed, Jan 26, 2022 at 02:22:49PM +0100, Michael Grzeschik wrote:
> > >> With this patch, the dummy_hcd gains first support for isoc transfers.
> > >> It will complete the whole urb with all packages.
> > >
> > >"packets", not "packages".
> > 
> > Right.
> > 
> > >>  Even if the gadget
> > >> side did not enqueue any request, the urb will be handled.
> > >>
> > >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > >
> > >I don't like this idea.  If support for isochronous transfers is added,
> > >it should be done correctly.  That is, the implementation should support
> > >scheduling of transfers, periodic bandwidth reservation, high-bandwidth
> > >transfers, and so on.
> > >
> > >The whole point of dummy-hcd is to emulate a real host controller as
> > >closely as possible.  Real isochronous transfers do not complete all at
> > >once.
> 
> Being able to at least test uvc-gadget in a virtual environment would
> already be a big benefit. As this is emulation, not simulation is it
> essential that an exact mapping of the hardware is in place?

Bindly being a sink for all data is not emulation for drivers that
require some sort of feedback loop.

> Is there anything we can do to support the progression of this
> development?
> 
> I.e. could we support this method first with a 
>   WARN_ONCE("This does not fully emulate Isochronous support");

That would instantly trigger syzbot to send us reports for no good
reason.  Please don't do that :(

thanks,

greg k-h
Alan Stern Jan. 31, 2022, 3:32 p.m. UTC | #4
On Mon, Jan 31, 2022 at 12:08:20PM +0000, Kieran Bingham wrote:
> Hi Michael, Alan,
> 
> Quoting Michael Grzeschik (2022-01-26 18:31:38)
> > On Wed, Jan 26, 2022 at 11:09:23AM -0500, Alan Stern wrote:
> > >On Wed, Jan 26, 2022 at 02:22:49PM +0100, Michael Grzeschik wrote:
> > >> With this patch, the dummy_hcd gains first support for isoc transfers.
> > >> It will complete the whole urb with all packages.
> > >
> > >"packets", not "packages".
> > 
> > Right.
> > 
> > >>  Even if the gadget
> > >> side did not enqueue any request, the urb will be handled.
> > >>
> > >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> > >
> > >I don't like this idea.  If support for isochronous transfers is added,
> > >it should be done correctly.  That is, the implementation should support
> > >scheduling of transfers, periodic bandwidth reservation, high-bandwidth
> > >transfers, and so on.
> > >
> > >The whole point of dummy-hcd is to emulate a real host controller as
> > >closely as possible.  Real isochronous transfers do not complete all at
> > >once.
> 
> Being able to at least test uvc-gadget in a virtual environment would
> already be a big benefit. As this is emulation, not simulation is it
> essential that an exact mapping of the hardware is in place?

It doesn't have to be exact, but it should be close enough to test the 
essential aspects of isochronous transfers.  In particular, people will 
need to be able to test the timing aspects -- they are a big part of the 
isochronous mechanism.

> Is there anything we can do to support the progression of this
> development?
> 
> I.e. could we support this method first with a 
>   WARN_ONCE("This does not fully emulate Isochronous support");

No, as Greg said, don't do that.

Improve the emulation in the patch so that it does do proper scheduling 
for isochronous transfers.  You don't have to worry about all the 
aspects of this -- for example, since dummy-hcd supports only one device 
at a time, it should be okay to leave out checks for overcommitting 
periodic transfer times during a frame or microframe.  (dummy-hcd 
doesn't currently do that sort of check for interrupt transfers.)  But 
the emulation should be sufficiently realistic to return -EXDEV errors 
for URBs or packets that were submitted too late.

Alternatively, you can simply keep the patch that Michael submitted as 
an out-of-tree resource for people who want to test uvc-gadget.

> That would allow infrastructure to be built up that uses this
> functionality, which would then in turn feed back into providing a means
> to actually test the improvements to the isochronous support on top.

Without realistic timing emulation, the tests would be only minimally 
useful.

Alan Stern
diff mbox series

Patch

diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c
index a2d956af42a23c..aff5f1fa4feef9 100644
--- a/drivers/usb/gadget/udc/dummy_hcd.c
+++ b/drivers/usb/gadget/udc/dummy_hcd.c
@@ -147,36 +147,30 @@  static const struct {
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_BULK, USB_EP_CAPS_DIR_IN)),
 	EP_INFO("ep2out-bulk",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_BULK, USB_EP_CAPS_DIR_OUT)),
-/*
 	EP_INFO("ep3in-iso",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_ISO, USB_EP_CAPS_DIR_IN)),
 	EP_INFO("ep4out-iso",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_ISO, USB_EP_CAPS_DIR_OUT)),
-*/
 	EP_INFO("ep5in-int",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_INT, USB_EP_CAPS_DIR_IN)),
 	EP_INFO("ep6in-bulk",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_BULK, USB_EP_CAPS_DIR_IN)),
 	EP_INFO("ep7out-bulk",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_BULK, USB_EP_CAPS_DIR_OUT)),
-/*
 	EP_INFO("ep8in-iso",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_ISO, USB_EP_CAPS_DIR_IN)),
 	EP_INFO("ep9out-iso",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_ISO, USB_EP_CAPS_DIR_OUT)),
-*/
 	EP_INFO("ep10in-int",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_INT, USB_EP_CAPS_DIR_IN)),
 	EP_INFO("ep11in-bulk",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_BULK, USB_EP_CAPS_DIR_IN)),
 	EP_INFO("ep12out-bulk",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_BULK, USB_EP_CAPS_DIR_OUT)),
-/*
 	EP_INFO("ep13in-iso",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_ISO, USB_EP_CAPS_DIR_IN)),
 	EP_INFO("ep14out-iso",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_ISO, USB_EP_CAPS_DIR_OUT)),
-*/
 	EP_INFO("ep15in-int",
 		USB_EP_CAPS(USB_EP_CAPS_TYPE_INT, USB_EP_CAPS_DIR_IN)),
 
@@ -1402,6 +1396,7 @@  static int transfer(struct dummy_hcd *dum_hcd, struct urb *urb,
 	struct dummy		*dum = dum_hcd->dum;
 	struct dummy_request	*req;
 	int			sent = 0;
+	int			count = 0;
 
 top:
 	/* if there's no request queued, the device is NAKing; return */
@@ -1459,6 +1454,13 @@  static int transfer(struct dummy_hcd *dum_hcd, struct urb *urb,
 				sent += len;
 				urb->actual_length += len;
 				req->req.actual += len;
+				if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) {
+					if (count <= urb->number_of_packets) {
+						urb->iso_frame_desc[count].actual_length = len;
+						urb->iso_frame_desc[count].status = 0;
+						count++;
+					}
+				}
 			}
 		}
 
@@ -1527,6 +1529,17 @@  static int transfer(struct dummy_hcd *dum_hcd, struct urb *urb,
 		if (rescan)
 			goto top;
 	}
+
+	if (usb_pipetype(urb->pipe) == PIPE_ISOCHRONOUS) {
+		int i;
+
+		for (i = count; i < urb->number_of_packets; ++i) {
+			urb->iso_frame_desc[i].actual_length = 0;
+			urb->iso_frame_desc[i].status = 0;
+		}
+		*status = 0;
+	}
+
 	return sent;
 }
 
@@ -1950,13 +1963,14 @@  static void dummy_timer(struct timer_list *t)
 			 * here are some of the issues we'd have to face:
 			 *
 			 * 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.
 			 */
 			limit = max(limit, periodic_bytes(dum, ep));
-			status = -EINVAL;	/* fail all xfers */
-			break;
+			ep->last_io = jiffies;
+			total -= transfer(dum_hcd, urb, ep, limit, &status);
+			if (status == -EINPROGRESS)
+				continue;
+			goto return_urb;
 
 		case PIPE_INTERRUPT:
 			/* FIXME is it urb->interval since the last xfer?