Testing endpoint halt support for raw-gadget

Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

Hi Alan and Greg,

I've been thinking about what kind of features raw-gadget might be
missing, that would allow more flexibility in emulating USB devices.
One of the things that is currently missing is halting endpoints.
Adding this functionality seems to be fairly easy, but it's unclear to
me how to test it. Any suggestions?

Also, are there some other features that might make sense to add? I
see that e.g. GadgetFS has GADGETFS_FIFO_STATUS/FLUSH ioctls. Are
those useful?

Thanks!

Re: Testing endpoint halt support for raw-gadget

[Alan Stern]

On Thu, 9 Apr 2020, Andrey Konovalov wrote:

> Hi Alan and Greg,
> 
> I've been thinking about what kind of features raw-gadget might be
> missing, that would allow more flexibility in emulating USB devices.
> One of the things that is currently missing is halting endpoints.
> Adding this functionality seems to be fairly easy, but it's unclear to
> me how to test it. Any suggestions?

You should use the usbtest driver along with the testusb program in
tools/usb.  Of course, to do it you will need a userspace driver for
raw-gadget.  usbtest works best with gadget-zero, but it can be used
(in degraded form) with any USB device.

> Also, are there some other features that might make sense to add? I
> see that e.g. GadgetFS has GADGETFS_FIFO_STATUS/FLUSH ioctls. Are
> those useful?

I believe that was included just as an emulation of some existing UDC 
hardware.  It's rather controller-specific, not of general interest.
(The general idea is that the UDC says that an IN request completes 
when its data has been loaded into the controller's FIFO, rather than 
when the data has actually been sent to the host.)

Have you implemented wedge as well as halt?  Wedge is needed for the
mass-storage protocol; as far as I know it isn't used anywhere else.  

And have you given any thought to suspend/resume support?  It's a bit 
tricky because you have to consider both gadget suspend and USB bus 
suspend.

Nothing else springs to mind.

Alan Stern

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Fri, Apr 10, 2020 at 2:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, 9 Apr 2020, Andrey Konovalov wrote:
>
> > Hi Alan and Greg,
> >
> > I've been thinking about what kind of features raw-gadget might be
> > missing, that would allow more flexibility in emulating USB devices.
> > One of the things that is currently missing is halting endpoints.
> > Adding this functionality seems to be fairly easy, but it's unclear to
> > me how to test it. Any suggestions?
>
> You should use the usbtest driver along with the testusb program in
> tools/usb.  Of course, to do it you will need a userspace driver for
> raw-gadget.  usbtest works best with gadget-zero, but it can be used
> (in degraded form) with any USB device.

Oh, right, I'll try that!

> > Also, are there some other features that might make sense to add? I
> > see that e.g. GadgetFS has GADGETFS_FIFO_STATUS/FLUSH ioctls. Are
> > those useful?
>
> I believe that was included just as an emulation of some existing UDC
> hardware.  It's rather controller-specific, not of general interest.
> (The general idea is that the UDC says that an IN request completes
> when its data has been loaded into the controller's FIFO, rather than
> when the data has actually been sent to the host.)

OK, then I guess raw-gadget won't need those at least just yet.

> Have you implemented wedge as well as halt?  Wedge is needed for the
> mass-storage protocol; as far as I know it isn't used anywhere else.

No, I didn't know about "wedge" at all :) Looks like the API for it is
really simple, just usb_ep_set_wedge(). I'll need to figure out what
it is and how it works, and I'll send a patch that adds halt/wedge
support then.

> And have you given any thought to suspend/resume support?  It's a bit
> tricky because you have to consider both gadget suspend and USB bus
> suspend.

Hm, no. Is there something specific I need to consider to support it?
I guess I'll need to read about how it works as well, before I can
understand what it would require and ask meaningful questions.

> Nothing else springs to mind.

Something else: I've been testing raw-gadget with various UDCs that I
have [1] and everything seems to work, except for emulating SuperSpeed
devices with net2280. I've just found it out yesterday night, and
haven't had a chance to debug that yet, but if you know about some
potential issues I could encounter with SuperSpeed/USB3+, please let
me know.

Thank you, Alan!

[1] https://github.com/xairy/raw-gadget#usb-device-controllers

Re: Testing endpoint halt support for raw-gadget

[Alan Stern]

On Fri, 10 Apr 2020, Andrey Konovalov wrote:

> On Fri, Apr 10, 2020 at 2:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:

> > Have you implemented wedge as well as halt?  Wedge is needed for the
> > mass-storage protocol; as far as I know it isn't used anywhere else.
> 
> No, I didn't know about "wedge" at all :) Looks like the API for it is
> really simple, just usb_ep_set_wedge(). I'll need to figure out what
> it is and how it works, and I'll send a patch that adds halt/wedge
> support then.

usb_ep_set_wedge(ep) does almost the same thing as 
usb_ep_set_halt(ep).  The difference is that a Clear-Feature(halt) 
request from the host will un-halt an endpoint if it is merely halted, 
but it won't un-halt a wedged endpoint.  (I don't think this is 
documented anywhere, unfortunately.)

> > And have you given any thought to suspend/resume support?  It's a bit
> > tricky because you have to consider both gadget suspend and USB bus
> > suspend.
> 
> Hm, no. Is there something specific I need to consider to support it?
> I guess I'll need to read about how it works as well, before I can
> understand what it would require and ask meaningful questions.

The really tricky part involves a gadget that is in system suspend.  If 
the USB bus isn't also suspended, the gadget won't work properly -- it 
won't be able to respond to requests from the host.  Basically, when a 
UDC driver sees that the system is going into suspend, it has no choice 
but to disconnect from the USB bus.  This probably isn't implemented 
very well in a lot of UDC drivers.

USB bus suspend, on the other hand, _should_ be implemented.

> > Nothing else springs to mind.
> 
> Something else: I've been testing raw-gadget with various UDCs that I
> have [1] and everything seems to work, except for emulating SuperSpeed
> devices with net2280. I've just found it out yesterday night, and
> haven't had a chance to debug that yet, but if you know about some
> potential issues I could encounter with SuperSpeed/USB3+, please let
> me know.

Well, USB-3 has streams, unlike USB-2.  You may want to think about
supporting them.

Also, bear in mind that dummy-hcd doesn't support isochronous transfers 
(although all real UDCs do support them).  So perhaps you haven't given 
them as much testing.  usbtest can help a little with that.

Alan Stern

Re: Testing endpoint halt support for raw-gadget

[Peter Chen]

On 20-04-10 11:53:20, Alan Stern wrote:
> On Fri, 10 Apr 2020, Andrey Konovalov wrote:
> 
> > On Fri, Apr 10, 2020 at 2:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> 
> > > Have you implemented wedge as well as halt?  Wedge is needed for the
> > > mass-storage protocol; as far as I know it isn't used anywhere else.
> > 
> > No, I didn't know about "wedge" at all :) Looks like the API for it is
> > really simple, just usb_ep_set_wedge(). I'll need to figure out what
> > it is and how it works, and I'll send a patch that adds halt/wedge
> > support then.
> 
> usb_ep_set_wedge(ep) does almost the same thing as 
> usb_ep_set_halt(ep).  The difference is that a Clear-Feature(halt) 
> request from the host will un-halt an endpoint if it is merely halted, 
> but it won't un-halt a wedged endpoint.  (I don't think this is 
> documented anywhere, unfortunately.)

Hi Alan,

It is documented at drivers/usb/gadget/udc/core.c, function
usb_ep_set_wedge.

-- 

Thanks,
Peter Chen

Re: Testing endpoint halt support for raw-gadget

[Alan Stern]

On Mon, 27 Apr 2020, Peter Chen wrote:

> On 20-04-10 11:53:20, Alan Stern wrote:
> > On Fri, 10 Apr 2020, Andrey Konovalov wrote:
> > 
> > > On Fri, Apr 10, 2020 at 2:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > 
> > > > Have you implemented wedge as well as halt?  Wedge is needed for the
> > > > mass-storage protocol; as far as I know it isn't used anywhere else.
> > > 
> > > No, I didn't know about "wedge" at all :) Looks like the API for it is
> > > really simple, just usb_ep_set_wedge(). I'll need to figure out what
> > > it is and how it works, and I'll send a patch that adds halt/wedge
> > > support then.
> > 
> > usb_ep_set_wedge(ep) does almost the same thing as 
> > usb_ep_set_halt(ep).  The difference is that a Clear-Feature(halt) 
> > request from the host will un-halt an endpoint if it is merely halted, 
> > but it won't un-halt a wedged endpoint.  (I don't think this is 
> > documented anywhere, unfortunately.)
> 
> Hi Alan,
> 
> It is documented at drivers/usb/gadget/udc/core.c, function
> usb_ep_set_wedge.

Ah, that's where it ended up.  Thanks.

Maybe at some point we should move these documentation comments out of 
various random .c files and into the apropriate header files...

Alan Stern

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Fri, Apr 10, 2020 at 5:53 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, 10 Apr 2020, Andrey Konovalov wrote:
>
> > On Fri, Apr 10, 2020 at 2:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> > Something else: I've been testing raw-gadget with various UDCs that I
> > have [1] and everything seems to work, except for emulating SuperSpeed
> > devices with net2280. I've just found it out yesterday night, and
> > haven't had a chance to debug that yet, but if you know about some
> > potential issues I could encounter with SuperSpeed/USB3+, please let
> > me know.
>
> Well, USB-3 has streams, unlike USB-2.  You may want to think about
> supporting them.

Do I understand correctly, that to support streams I basically need to
allow the user to set stream_id on the request being submitted?

Re: Testing endpoint halt support for raw-gadget

[Alan Stern]

On Wed, 29 Apr 2020, Andrey Konovalov wrote:

> On Fri, Apr 10, 2020 at 5:53 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Fri, 10 Apr 2020, Andrey Konovalov wrote:
> >
> > > On Fri, Apr 10, 2020 at 2:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > > Something else: I've been testing raw-gadget with various UDCs that I
> > > have [1] and everything seems to work, except for emulating SuperSpeed
> > > devices with net2280. I've just found it out yesterday night, and
> > > haven't had a chance to debug that yet, but if you know about some
> > > potential issues I could encounter with SuperSpeed/USB3+, please let
> > > me know.
> >
> > Well, USB-3 has streams, unlike USB-2.  You may want to think about
> > supporting them.
> 
> Do I understand correctly, that to support streams I basically need to
> allow the user to set stream_id on the request being submitted?

Yes, for bulk endpoints.

Alan Stern

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Wed, Apr 29, 2020 at 4:06 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, 29 Apr 2020, Andrey Konovalov wrote:
>
> > On Fri, Apr 10, 2020 at 5:53 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Fri, 10 Apr 2020, Andrey Konovalov wrote:
> > >
> > > > On Fri, Apr 10, 2020 at 2:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > > Something else: I've been testing raw-gadget with various UDCs that I
> > > > have [1] and everything seems to work, except for emulating SuperSpeed
> > > > devices with net2280. I've just found it out yesterday night, and
> > > > haven't had a chance to debug that yet, but if you know about some
> > > > potential issues I could encounter with SuperSpeed/USB3+, please let
> > > > me know.
> > >
> > > Well, USB-3 has streams, unlike USB-2.  You may want to think about
> > > supporting them.
> >
> > Do I understand correctly, that to support streams I basically need to
> > allow the user to set stream_id on the request being submitted?
>
> Yes, for bulk endpoints.

One more question (sorry for so many :).

Looking at other fields of usb_request struct I see frame_number.
AFAIU it's filled in by the UDC driver for ISO transfers. Does it make
sense to expose it to userspace? I don't see any composite/legacy
gadgets use that field at all.

Re: Testing endpoint halt support for raw-gadget

[Alan Stern]

On Mon, 4 May 2020, Andrey Konovalov wrote:

> One more question (sorry for so many :).
> 
> Looking at other fields of usb_request struct I see frame_number.
> AFAIU it's filled in by the UDC driver for ISO transfers. Does it make
> sense to expose it to userspace? I don't see any composite/legacy
> gadgets use that field at all.

Do any of those gadget drivers use isochronous endpoints?

In fact, it also looks like none of the drivers in gadget/udc/ touch
the frame_number field.  Maybe we should just get rid of it, since it
isn't being used.

Felipe, any preference?

Alan Stern

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Mon, May 4, 2020 at 4:24 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, 4 May 2020, Andrey Konovalov wrote:
>
> > One more question (sorry for so many :).
> >
> > Looking at other fields of usb_request struct I see frame_number.
> > AFAIU it's filled in by the UDC driver for ISO transfers. Does it make
> > sense to expose it to userspace? I don't see any composite/legacy
> > gadgets use that field at all.
>
> Do any of those gadget drivers use isochronous endpoints?

Yes, there are audio/uvc function/legacy drivers that use those.

> In fact, it also looks like none of the drivers in gadget/udc/ touch
> the frame_number field.  Maybe we should just get rid of it, since it
> isn't being used.

It is used by dwc2/3 gadget drivers (which are not in gadget/udc/).

Re: Testing endpoint halt support for raw-gadget

[Alan Stern]

On Mon, 4 May 2020, Andrey Konovalov wrote:

> On Mon, May 4, 2020 at 4:24 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Mon, 4 May 2020, Andrey Konovalov wrote:
> >
> > > One more question (sorry for so many :).
> > >
> > > Looking at other fields of usb_request struct I see frame_number.
> > > AFAIU it's filled in by the UDC driver for ISO transfers. Does it make
> > > sense to expose it to userspace? I don't see any composite/legacy
> > > gadgets use that field at all.
> >
> > Do any of those gadget drivers use isochronous endpoints?
> 
> Yes, there are audio/uvc function/legacy drivers that use those.
> 
> > In fact, it also looks like none of the drivers in gadget/udc/ touch
> > the frame_number field.  Maybe we should just get rid of it, since it
> > isn't being used.
> 
> It is used by dwc2/3 gadget drivers (which are not in gadget/udc/).

Well, if Felipe thinks we ought to keep the field then you might as 
well export it to userspace.  Drivers are free to ignore it.  :-)

Alan Stern

Re: Testing endpoint halt support for raw-gadget

[Felipe Balbi]

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


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Mon, 4 May 2020, Andrey Konovalov wrote:
>
>> On Mon, May 4, 2020 at 4:24 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>> >
>> > On Mon, 4 May 2020, Andrey Konovalov wrote:
>> >
>> > > One more question (sorry for so many :).
>> > >
>> > > Looking at other fields of usb_request struct I see frame_number.
>> > > AFAIU it's filled in by the UDC driver for ISO transfers. Does it make
>> > > sense to expose it to userspace? I don't see any composite/legacy
>> > > gadgets use that field at all.
>> >
>> > Do any of those gadget drivers use isochronous endpoints?
>> 
>> Yes, there are audio/uvc function/legacy drivers that use those.
>> 
>> > In fact, it also looks like none of the drivers in gadget/udc/ touch
>> > the frame_number field.  Maybe we should just get rid of it, since it
>> > isn't being used.
>> 
>> It is used by dwc2/3 gadget drivers (which are not in gadget/udc/).
>
> Well, if Felipe thinks we ought to keep the field then you might as 
> well export it to userspace.  Drivers are free to ignore it.  :-)

dwc3 has its own private frame_number as part of its own endpoint
structure. We simply copy that to the request. That's is currently
telling the gadget driver which frame the request completed. It could be
used by the function to decide when to queue more requests. It can also
be used to predict if we're in sync with the frames or will we diverge
and miss frames in the future.

If nobody has implemented any of that so far, I don't mind removing
it. We need strong evidence that this will never be used, though :-)

-- 
balbi

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

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Tue, May 5, 2020 at 8:34 AM Felipe Balbi <balbi@kernel.org> wrote:
>
>
> Hi,
>
> Alan Stern <stern@rowland.harvard.edu> writes:
> > On Mon, 4 May 2020, Andrey Konovalov wrote:
> >
> >> On Mon, May 4, 2020 at 4:24 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >> >
> >> > On Mon, 4 May 2020, Andrey Konovalov wrote:
> >> >
> >> > > One more question (sorry for so many :).
> >> > >
> >> > > Looking at other fields of usb_request struct I see frame_number.
> >> > > AFAIU it's filled in by the UDC driver for ISO transfers. Does it make
> >> > > sense to expose it to userspace? I don't see any composite/legacy
> >> > > gadgets use that field at all.
> >> >
> >> > Do any of those gadget drivers use isochronous endpoints?
> >>
> >> Yes, there are audio/uvc function/legacy drivers that use those.
> >>
> >> > In fact, it also looks like none of the drivers in gadget/udc/ touch
> >> > the frame_number field.  Maybe we should just get rid of it, since it
> >> > isn't being used.
> >>
> >> It is used by dwc2/3 gadget drivers (which are not in gadget/udc/).
> >
> > Well, if Felipe thinks we ought to keep the field then you might as
> > well export it to userspace.  Drivers are free to ignore it.  :-)
>
> dwc3 has its own private frame_number as part of its own endpoint
> structure. We simply copy that to the request. That's is currently
> telling the gadget driver which frame the request completed. It could be
> used by the function to decide when to queue more requests. It can also
> be used to predict if we're in sync with the frames or will we diverge
> and miss frames in the future.
>
> If nobody has implemented any of that so far, I don't mind removing
> it. We need strong evidence that this will never be used, though :-)

OK, I see, If this field is a potential candidate for removal, I won't
expose it through raw-gadget just yet, but I'll try to make the
interface extensible so that it can be easily added when needed.

Re: Testing endpoint halt support for raw-gadget

[Thinh Nguyen]

Andrey Konovalov wrote:
> On Tue, May 5, 2020 at 8:34 AM Felipe Balbi <balbi@kernel.org> wrote:
>>
>> Hi,
>>
>> Alan Stern <stern@rowland.harvard.edu> writes:
>>> On Mon, 4 May 2020, Andrey Konovalov wrote:
>>>
>>>> On Mon, May 4, 2020 at 4:24 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>>>>> On Mon, 4 May 2020, Andrey Konovalov wrote:
>>>>>
>>>>>> One more question (sorry for so many :).
>>>>>>
>>>>>> Looking at other fields of usb_request struct I see frame_number.
>>>>>> AFAIU it's filled in by the UDC driver for ISO transfers. Does it make
>>>>>> sense to expose it to userspace? I don't see any composite/legacy
>>>>>> gadgets use that field at all.
>>>>> Do any of those gadget drivers use isochronous endpoints?
>>>> Yes, there are audio/uvc function/legacy drivers that use those.
>>>>
>>>>> In fact, it also looks like none of the drivers in gadget/udc/ touch
>>>>> the frame_number field.  Maybe we should just get rid of it, since it
>>>>> isn't being used.
>>>> It is used by dwc2/3 gadget drivers (which are not in gadget/udc/).
>>> Well, if Felipe thinks we ought to keep the field then you might as
>>> well export it to userspace.  Drivers are free to ignore it.  :-)
>> dwc3 has its own private frame_number as part of its own endpoint
>> structure. We simply copy that to the request. That's is currently
>> telling the gadget driver which frame the request completed. It could be
>> used by the function to decide when to queue more requests. It can also
>> be used to predict if we're in sync with the frames or will we diverge
>> and miss frames in the future.
>>
>> If nobody has implemented any of that so far, I don't mind removing
>> it. We need strong evidence that this will never be used, though :-)
> OK, I see, If this field is a potential candidate for removal, I won't
> expose it through raw-gadget just yet, but I'll try to make the
> interface extensible so that it can be easily added when needed.

Please don't remove this. We also use it for one of our HW validation tools.

BR,
Thinh

Re: Testing endpoint halt support for raw-gadget

[Felipe Balbi]

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


Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Mon, 4 May 2020, Andrey Konovalov wrote:
>
>> One more question (sorry for so many :).
>> 
>> Looking at other fields of usb_request struct I see frame_number.
>> AFAIU it's filled in by the UDC driver for ISO transfers. Does it make
>> sense to expose it to userspace? I don't see any composite/legacy
>> gadgets use that field at all.
>
> Do any of those gadget drivers use isochronous endpoints?
>
> In fact, it also looks like none of the drivers in gadget/udc/ touch
> the frame_number field.  Maybe we should just get rid of it, since it
> isn't being used.
>
> Felipe, any preference?

It was added for functions to use to tell the UDC which frame they want
to start the request, but it was never used. I don't mind removing it.

-- 
balbi

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

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Fri, Apr 10, 2020 at 2:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Thu, 9 Apr 2020, Andrey Konovalov wrote:
>
> > Hi Alan and Greg,
> >
> > I've been thinking about what kind of features raw-gadget might be
> > missing, that would allow more flexibility in emulating USB devices.
> > One of the things that is currently missing is halting endpoints.
> > Adding this functionality seems to be fairly easy, but it's unclear to
> > me how to test it. Any suggestions?
>
> You should use the usbtest driver along with the testusb program in
> tools/usb.  Of course, to do it you will need a userspace driver for
> raw-gadget.  usbtest works best with gadget-zero, but it can be used
> (in degraded form) with any USB device.

Hi Alan,

I've started working on a test suite for raw-gadget based on the
usbtest module as you suggested and have a few questions:

1. (Re test #10:) Currently there's no way to stall USB (control)
requests with raw-gadget (which is what happens when you return -EPIPE
from gadget's setup() callback AFAIU). Is stalling an important part
of the protocol? Should we somehow support it? AFAIU gadgetfs also has
no ability to stall requests that are passed to userspace.

2. Re test #4: the test fails with length that is not divisible by
endpoint's max packet value when using dummy_hcd (assuming that gadget
keeps queueing URBs with max packet length), as dummy_hcd's transfer()
function sets status to -EOVERFLOW when this happens. Is this
expected?

3. Re test #7: the test fails when e.g. vary parameter is set to some
odd value when using dummy_hcd. AFAIU this happens since dummy_hcd
doesn't have no_sg_constraint flag set and therefore the sanity check
in usb_submit_urb() fails. Is this expected?

Thanks!

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Fri, Apr 24, 2020 at 9:36 PM Andrey Konovalov <andreyknvl@google.com> wrote:
>
> On Fri, Apr 10, 2020 at 2:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Thu, 9 Apr 2020, Andrey Konovalov wrote:
> >
> > > Hi Alan and Greg,
> > >
> > > I've been thinking about what kind of features raw-gadget might be
> > > missing, that would allow more flexibility in emulating USB devices.
> > > One of the things that is currently missing is halting endpoints.
> > > Adding this functionality seems to be fairly easy, but it's unclear to
> > > me how to test it. Any suggestions?
> >
> > You should use the usbtest driver along with the testusb program in
> > tools/usb.  Of course, to do it you will need a userspace driver for
> > raw-gadget.  usbtest works best with gadget-zero, but it can be used
> > (in degraded form) with any USB device.
>
> Hi Alan,
>
> I've started working on a test suite for raw-gadget based on the
> usbtest module as you suggested and have a few questions:
>
> 1. (Re test #10:) Currently there's no way to stall USB (control)
> requests with raw-gadget (which is what happens when you return -EPIPE
> from gadget's setup() callback AFAIU). Is stalling an important part
> of the protocol? Should we somehow support it? AFAIU gadgetfs also has
> no ability to stall requests that are passed to userspace.
>
> 2. Re test #4: the test fails with length that is not divisible by
> endpoint's max packet value when using dummy_hcd (assuming that gadget
> keeps queueing URBs with max packet length), as dummy_hcd's transfer()
> function sets status to -EOVERFLOW when this happens. Is this
> expected?
>
> 3. Re test #7: the test fails when e.g. vary parameter is set to some
> odd value when using dummy_hcd. AFAIU this happens since dummy_hcd
> doesn't have no_sg_constraint flag set and therefore the sanity check
> in usb_submit_urb() fails. Is this expected?

4. Re test #13: it seems that both dummy_hcd and the UDC on Raspberry
Pi Zero handle host driven endpoint halts themselves without any need
to support them on the gadget side. Thus this test can't really be
used to test the halt implementation I have for raw-gadget. Are there
other ways to test it?

Re: Testing endpoint halt support for raw-gadget

[Alan Stern]

On Fri, 24 Apr 2020, Andrey Konovalov wrote:

> On Fri, Apr 24, 2020 at 9:36 PM Andrey Konovalov <andreyknvl@google.com> wrote:

> > Hi Alan,
> >
> > I've started working on a test suite for raw-gadget based on the
> > usbtest module as you suggested and have a few questions:
> >
> > 1. (Re test #10:) Currently there's no way to stall USB (control)
> > requests with raw-gadget (which is what happens when you return -EPIPE
> > from gadget's setup() callback AFAIU). Is stalling an important part
> > of the protocol? Should we somehow support it? AFAIU gadgetfs also has
> > no ability to stall requests that are passed to userspace.

Yes, stalling is important, and you do need to support it.  gadgetfs
does have a way to stall requests on ep0 from userspace: just perform
I/O in the "wrong" direction.  If the host sends a control-IN request
and the user does a read of the ep0 file, or if the host sends a
control-OUT request and the user does a write, gadgetfs will call
usb_ep_set_halt.  (However I do not remember how the setup_can_stall
flag is meant to work.)

> > 2. Re test #4: the test fails with length that is not divisible by
> > endpoint's max packet value when using dummy_hcd (assuming that gadget
> > keeps queueing URBs with max packet length), as dummy_hcd's transfer()
> > function sets status to -EOVERFLOW when this happens. Is this
> > expected?

Yes, it is.  If you want to avoid overflow errors, you have to set the
"vary" parameter to a multiple of the bulk-IN endpoint's maxpacket
value and the "length" parameter to a multiple of that.

> > 3. Re test #7: the test fails when e.g. vary parameter is set to some
> > odd value when using dummy_hcd. AFAIU this happens since dummy_hcd
> > doesn't have no_sg_constraint flag set and therefore the sanity check
> > in usb_submit_urb() fails. Is this expected?

No, I don't think so.  Have you tried setting no_sg_constraint?  
Probably we just forgot to do it.

> 4. Re test #13: it seems that both dummy_hcd and the UDC on Raspberry
> Pi Zero handle host driven endpoint halts themselves without any need
> to support them on the gadget side. Thus this test can't really be
> used to test the halt implementation I have for raw-gadget. Are there
> other ways to test it?

Have you tried running the USBCV tests, available from usb.org?  They
need a Windows host to run on but are otherwise pretty straightforward.
If a mass-storage gadget (like g-mass-storage) can pass the USBCV
Mass-Storage test, for example, that's a pretty stringent verification.  

Try them on any userspace gadget drivers you have lying around.

Alan Stern

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Sat, Apr 25, 2020 at 3:53 AM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Fri, 24 Apr 2020, Andrey Konovalov wrote:
>
> > On Fri, Apr 24, 2020 at 9:36 PM Andrey Konovalov <andreyknvl@google.com> wrote:
>
> > > Hi Alan,
> > >
> > > I've started working on a test suite for raw-gadget based on the
> > > usbtest module as you suggested and have a few questions:
> > >
> > > 1. (Re test #10:) Currently there's no way to stall USB (control)
> > > requests with raw-gadget (which is what happens when you return -EPIPE
> > > from gadget's setup() callback AFAIU). Is stalling an important part
> > > of the protocol? Should we somehow support it? AFAIU gadgetfs also has
> > > no ability to stall requests that are passed to userspace.
>
> Yes, stalling is important, and you do need to support it.  gadgetfs
> does have a way to stall requests on ep0 from userspace: just perform
> I/O in the "wrong" direction.  If the host sends a control-IN request
> and the user does a read of the ep0 file, or if the host sends a
> control-OUT request and the user does a write, gadgetfs will call
> usb_ep_set_halt.  (However I do not remember how the setup_can_stall
> flag is meant to work.)

Ah, so halting ep0 after having successfully received a setup stage
request (setup() callback returns 0) will result in a stall during the
data stage (I hope I'm using those "stage" terms right) without the
gadget needing to queue an URB as it happens during a normal response?
Shouldn't this halt the endpoint until the user (or the gadget)
unhalts it? Does this work when we want to just stall a single
request? What happens with the requests that come after?

>
> > > 2. Re test #4: the test fails with length that is not divisible by
> > > endpoint's max packet value when using dummy_hcd (assuming that gadget
> > > keeps queueing URBs with max packet length), as dummy_hcd's transfer()
> > > function sets status to -EOVERFLOW when this happens. Is this
> > > expected?
>
> Yes, it is.  If you want to avoid overflow errors, you have to set the
> "vary" parameter to a multiple of the bulk-IN endpoint's maxpacket
> value and the "length" parameter to a multiple of that.

OK, got it. Those tests actually also fail with g_zero. I guess the
way to use usbtest in a particular setup (with particular host and
device controllers) is to first run the tests with e.g. g_zero, find
the ones that fail, and don't run those with raw-gadget or whatever
you're trying to test.

> > > 3. Re test #7: the test fails when e.g. vary parameter is set to some
> > > odd value when using dummy_hcd. AFAIU this happens since dummy_hcd
> > > doesn't have no_sg_constraint flag set and therefore the sanity check
> > > in usb_submit_urb() fails. Is this expected?
>
> No, I don't think so.  Have you tried setting no_sg_constraint?
> Probably we just forgot to do it.

Will try.

> > 4. Re test #13: it seems that both dummy_hcd and the UDC on Raspberry
> > Pi Zero handle host driven endpoint halts themselves without any need
> > to support them on the gadget side. Thus this test can't really be
> > used to test the halt implementation I have for raw-gadget. Are there
> > other ways to test it?
>
> Have you tried running the USBCV tests, available from usb.org?  They
> need a Windows host to run on but are otherwise pretty straightforward.
> If a mass-storage gadget (like g-mass-storage) can pass the USBCV
> Mass-Storage test, for example, that's a pretty stringent verification.
>
> Try them on any userspace gadget drivers you have lying around.

Didn't yet get to those, I'll try them too.

Thanks!

Re: Testing endpoint halt support for raw-gadget

[Alan Stern]

On Sat, 25 Apr 2020, Andrey Konovalov wrote:

> On Sat, Apr 25, 2020 at 3:53 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Fri, 24 Apr 2020, Andrey Konovalov wrote:
> >
> > > On Fri, Apr 24, 2020 at 9:36 PM Andrey Konovalov <andreyknvl@google.com> wrote:
> >
> > > > Hi Alan,
> > > >
> > > > I've started working on a test suite for raw-gadget based on the
> > > > usbtest module as you suggested and have a few questions:
> > > >
> > > > 1. (Re test #10:) Currently there's no way to stall USB (control)
> > > > requests with raw-gadget (which is what happens when you return -EPIPE
> > > > from gadget's setup() callback AFAIU). Is stalling an important part
> > > > of the protocol? Should we somehow support it? AFAIU gadgetfs also has
> > > > no ability to stall requests that are passed to userspace.
> >
> > Yes, stalling is important, and you do need to support it.  gadgetfs
> > does have a way to stall requests on ep0 from userspace: just perform
> > I/O in the "wrong" direction.  If the host sends a control-IN request
> > and the user does a read of the ep0 file, or if the host sends a
> > control-OUT request and the user does a write, gadgetfs will call
> > usb_ep_set_halt.  (However I do not remember how the setup_can_stall
> > flag is meant to work.)
> 
> Ah, so halting ep0 after having successfully received a setup stage
> request (setup() callback returns 0) will result in a stall during the
> data stage

Or during the status stage, if there is no data stage (a 0-length 
transfer).

>  (I hope I'm using those "stage" terms right) without the
> gadget needing to queue an URB as it happens during a normal response?

Yes.

> Shouldn't this halt the endpoint until the user (or the gadget)
> unhalts it? Does this work when we want to just stall a single
> request? What happens with the requests that come after?

Ep0 is special.  See the description of protocol stalls in sections 
8.4.5 and 8.5.3 (especially 8.5.3.4) in the USB 2.0 spec.

Think about the problem for a moment.  Suppose a halt of ep0 persisted 
until it was cleared by the host.  Then it would never get cleared -- 
the only way the host can clear a halt condition is by sending a 
Clear-Halt request on ep0!

Alan Stern

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Fri, Apr 24, 2020 at 9:36 PM Andrey Konovalov <andreyknvl@google.com> wrote:
>
> On Fri, Apr 10, 2020 at 2:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Thu, 9 Apr 2020, Andrey Konovalov wrote:
> >
> > > Hi Alan and Greg,
> > >
> > > I've been thinking about what kind of features raw-gadget might be
> > > missing, that would allow more flexibility in emulating USB devices.
> > > One of the things that is currently missing is halting endpoints.
> > > Adding this functionality seems to be fairly easy, but it's unclear to
> > > me how to test it. Any suggestions?
> >
> > You should use the usbtest driver along with the testusb program in
> > tools/usb.  Of course, to do it you will need a userspace driver for
> > raw-gadget.  usbtest works best with gadget-zero, but it can be used
> > (in degraded form) with any USB device.
>
> Hi Alan,
>
> I've started working on a test suite for raw-gadget based on the
> usbtest module as you suggested and have a few questions:

Another question, which actually seems to be a major problem.

It looks like automatic endpoint selection based on required features
doesn't work in raw-gadget. The way it tries to do that is iterating
over the list of available endpoints and finding one that has the
right direction and transfer type. But it seems that the right way to
do that (like it's done in usb_ep_autoconfig()) is to also patch the
bEndpointAddress field of the endpoint descriptor.

Currently with raw-gadget the endpoints are supposed to be enabled
after a set_configuration/set_interface request from the host, so it's
too late to patch the endpoint descriptor (the one that was sent to
the host during enumeration). I'm guessing that sending one endpoint
descriptor during enumeration and then using a different one (with
patched bEndpointAddress) to set ep->desc before doing usb_ep_enable()
won't work (as there's going to be mismatch in endpoint addresses
between the host and the UDC), right?

I wonder what we can do about that. It seems that we actually need to
parse the descriptors and figure out the right endpoints before the
enumeration starts.

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Mon, Apr 27, 2020 at 9:51 PM Andrey Konovalov <andreyknvl@google.com> wrote:
>
> On Fri, Apr 24, 2020 at 9:36 PM Andrey Konovalov <andreyknvl@google.com> wrote:
> >
> > On Fri, Apr 10, 2020 at 2:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Thu, 9 Apr 2020, Andrey Konovalov wrote:
> > >
> > > > Hi Alan and Greg,
> > > >
> > > > I've been thinking about what kind of features raw-gadget might be
> > > > missing, that would allow more flexibility in emulating USB devices.
> > > > One of the things that is currently missing is halting endpoints.
> > > > Adding this functionality seems to be fairly easy, but it's unclear to
> > > > me how to test it. Any suggestions?
> > >
> > > You should use the usbtest driver along with the testusb program in
> > > tools/usb.  Of course, to do it you will need a userspace driver for
> > > raw-gadget.  usbtest works best with gadget-zero, but it can be used
> > > (in degraded form) with any USB device.
> >
> > Hi Alan,
> >
> > I've started working on a test suite for raw-gadget based on the
> > usbtest module as you suggested and have a few questions:
>
> Another question, which actually seems to be a major problem.
>
> It looks like automatic endpoint selection based on required features
> doesn't work in raw-gadget. The way it tries to do that is iterating
> over the list of available endpoints and finding one that has the
> right direction and transfer type. But it seems that the right way to
> do that (like it's done in usb_ep_autoconfig()) is to also patch the
> bEndpointAddress field of the endpoint descriptor.
>
> Currently with raw-gadget the endpoints are supposed to be enabled
> after a set_configuration/set_interface request from the host, so it's
> too late to patch the endpoint descriptor (the one that was sent to
> the host during enumeration). I'm guessing that sending one endpoint
> descriptor during enumeration and then using a different one (with
> patched bEndpointAddress) to set ep->desc before doing usb_ep_enable()
> won't work (as there's going to be mismatch in endpoint addresses
> between the host and the UDC), right?
>
> I wonder what we can do about that. It seems that we actually need to
> parse the descriptors and figure out the right endpoints before the
> enumeration starts.

Or maybe somehow expose all endpoints and their features (like
GadgetFS) and let userspace put proper addresses into endpoint
descriptors before enumeration starts.

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Mon, Apr 27, 2020 at 10:47 PM Andrey Konovalov <andreyknvl@google.com> wrote:
>
> On Mon, Apr 27, 2020 at 9:51 PM Andrey Konovalov <andreyknvl@google.com> wrote:
> >
> > On Fri, Apr 24, 2020 at 9:36 PM Andrey Konovalov <andreyknvl@google.com> wrote:
> > >
> > > On Fri, Apr 10, 2020 at 2:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > >
> > > > On Thu, 9 Apr 2020, Andrey Konovalov wrote:
> > > >
> > > > > Hi Alan and Greg,
> > > > >
> > > > > I've been thinking about what kind of features raw-gadget might be
> > > > > missing, that would allow more flexibility in emulating USB devices.
> > > > > One of the things that is currently missing is halting endpoints.
> > > > > Adding this functionality seems to be fairly easy, but it's unclear to
> > > > > me how to test it. Any suggestions?
> > > >
> > > > You should use the usbtest driver along with the testusb program in
> > > > tools/usb.  Of course, to do it you will need a userspace driver for
> > > > raw-gadget.  usbtest works best with gadget-zero, but it can be used
> > > > (in degraded form) with any USB device.
> > >
> > > Hi Alan,
> > >
> > > I've started working on a test suite for raw-gadget based on the
> > > usbtest module as you suggested and have a few questions:
> >
> > Another question, which actually seems to be a major problem.
> >
> > It looks like automatic endpoint selection based on required features
> > doesn't work in raw-gadget. The way it tries to do that is iterating
> > over the list of available endpoints and finding one that has the
> > right direction and transfer type. But it seems that the right way to
> > do that (like it's done in usb_ep_autoconfig()) is to also patch the
> > bEndpointAddress field of the endpoint descriptor.
> >
> > Currently with raw-gadget the endpoints are supposed to be enabled
> > after a set_configuration/set_interface request from the host, so it's
> > too late to patch the endpoint descriptor (the one that was sent to
> > the host during enumeration). I'm guessing that sending one endpoint
> > descriptor during enumeration and then using a different one (with
> > patched bEndpointAddress) to set ep->desc before doing usb_ep_enable()
> > won't work (as there's going to be mismatch in endpoint addresses
> > between the host and the UDC), right?
> >
> > I wonder what we can do about that. It seems that we actually need to
> > parse the descriptors and figure out the right endpoints before the
> > enumeration starts.
>
> Or maybe somehow expose all endpoints and their features (like
> GadgetFS) and let userspace put proper addresses into endpoint
> descriptors before enumeration starts.

I think this is the way to go, giving the userspace maximum control
over what's going on. Seems not too hard to implement too. The only
part that is not clear to me is how to figure out the endpoint address
that a particular UDC endpoint expects to have.

Most of the time endpoints are named as something like "ep1in" or
"ep2out", and AFAIU the number in the endpoint name is the address.
However net2280 seems to have weird endpoint names "ep-a", "ep-b",
etc. Looking at usb_ep_autoconfig_ss(), those endpoints supposedly
have sequential addresses starting from 1 (for each in/out type). But
then in the GadgetFS example [1] e.g. "ep-a" corresponds to address 7,
which I don't understand. My guess is that this is done to handle both
dummy_udc and net2280 in a single if case, but I'm not sure.

[1] http://www.linux-usb.org/gadget/usb.c

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Tue, Apr 28, 2020 at 2:50 AM Andrey Konovalov <andreyknvl@google.com> wrote:
>
> On Mon, Apr 27, 2020 at 10:47 PM Andrey Konovalov <andreyknvl@google.com> wrote:
> >
> > On Mon, Apr 27, 2020 at 9:51 PM Andrey Konovalov <andreyknvl@google.com> wrote:
> > >
> > > On Fri, Apr 24, 2020 at 9:36 PM Andrey Konovalov <andreyknvl@google.com> wrote:
> > > >
> > > > On Fri, Apr 10, 2020 at 2:29 AM Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > >
> > > > > On Thu, 9 Apr 2020, Andrey Konovalov wrote:
> > > > >
> > > > > > Hi Alan and Greg,
> > > > > >
> > > > > > I've been thinking about what kind of features raw-gadget might be
> > > > > > missing, that would allow more flexibility in emulating USB devices.
> > > > > > One of the things that is currently missing is halting endpoints.
> > > > > > Adding this functionality seems to be fairly easy, but it's unclear to
> > > > > > me how to test it. Any suggestions?
> > > > >
> > > > > You should use the usbtest driver along with the testusb program in
> > > > > tools/usb.  Of course, to do it you will need a userspace driver for
> > > > > raw-gadget.  usbtest works best with gadget-zero, but it can be used
> > > > > (in degraded form) with any USB device.
> > > >
> > > > Hi Alan,
> > > >
> > > > I've started working on a test suite for raw-gadget based on the
> > > > usbtest module as you suggested and have a few questions:
> > >
> > > Another question, which actually seems to be a major problem.
> > >
> > > It looks like automatic endpoint selection based on required features
> > > doesn't work in raw-gadget. The way it tries to do that is iterating
> > > over the list of available endpoints and finding one that has the
> > > right direction and transfer type. But it seems that the right way to
> > > do that (like it's done in usb_ep_autoconfig()) is to also patch the
> > > bEndpointAddress field of the endpoint descriptor.
> > >
> > > Currently with raw-gadget the endpoints are supposed to be enabled
> > > after a set_configuration/set_interface request from the host, so it's
> > > too late to patch the endpoint descriptor (the one that was sent to
> > > the host during enumeration). I'm guessing that sending one endpoint
> > > descriptor during enumeration and then using a different one (with
> > > patched bEndpointAddress) to set ep->desc before doing usb_ep_enable()
> > > won't work (as there's going to be mismatch in endpoint addresses
> > > between the host and the UDC), right?
> > >
> > > I wonder what we can do about that. It seems that we actually need to
> > > parse the descriptors and figure out the right endpoints before the
> > > enumeration starts.
> >
> > Or maybe somehow expose all endpoints and their features (like
> > GadgetFS) and let userspace put proper addresses into endpoint
> > descriptors before enumeration starts.
>
> I think this is the way to go, giving the userspace maximum control
> over what's going on. Seems not too hard to implement too. The only
> part that is not clear to me is how to figure out the endpoint address
> that a particular UDC endpoint expects to have.
>
> Most of the time endpoints are named as something like "ep1in" or
> "ep2out", and AFAIU the number in the endpoint name is the address.
> However net2280 seems to have weird endpoint names "ep-a", "ep-b",
> etc. Looking at usb_ep_autoconfig_ss(), those endpoints supposedly
> have sequential addresses starting from 1 (for each in/out type). But
> then in the GadgetFS example [1] e.g. "ep-a" corresponds to address 7,
> which I don't understand. My guess is that this is done to handle both
> dummy_udc and net2280 in a single if case, but I'm not sure.
>
> [1] http://www.linux-usb.org/gadget/usb.c

Oh, there's actually a comment that explains this [1] in dummy_hcd.c
(exactly the place where one would look for it :). So "ep-a" and
others are fully configurable and accept any endpoint address. OK, I
think I've figured out what to do here, will send a patch.

[1] https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/usb/gadget/udc/dummy_hcd.c#L113

Re: Testing endpoint halt support for raw-gadget

[Alan Stern]

Better late than never...

On Tue, 28 Apr 2020, Andrey Konovalov wrote:

> On Tue, Apr 28, 2020 at 2:50 AM Andrey Konovalov <andreyknvl@google.com> wrote:
> >
> > On Mon, Apr 27, 2020 at 10:47 PM Andrey Konovalov <andreyknvl@google.com> wrote:
> > >
> > > On Mon, Apr 27, 2020 at 9:51 PM Andrey Konovalov <andreyknvl@google.com> wrote:

> > > > Another question, which actually seems to be a major problem.
> > > >
> > > > It looks like automatic endpoint selection based on required features
> > > > doesn't work in raw-gadget. The way it tries to do that is iterating
> > > > over the list of available endpoints and finding one that has the
> > > > right direction and transfer type. But it seems that the right way to
> > > > do that (like it's done in usb_ep_autoconfig()) is to also patch the
> > > > bEndpointAddress field of the endpoint descriptor.
> > > >
> > > > Currently with raw-gadget the endpoints are supposed to be enabled
> > > > after a set_configuration/set_interface request from the host, so it's
> > > > too late to patch the endpoint descriptor (the one that was sent to
> > > > the host during enumeration). I'm guessing that sending one endpoint
> > > > descriptor during enumeration and then using a different one (with
> > > > patched bEndpointAddress) to set ep->desc before doing usb_ep_enable()
> > > > won't work (as there's going to be mismatch in endpoint addresses
> > > > between the host and the UDC), right?

It certainly won't work.

> > > > I wonder what we can do about that. It seems that we actually need to
> > > > parse the descriptors and figure out the right endpoints before the
> > > > enumeration starts.

That's right.  In the case of raw-gadget, this would be done around the 
time the user tells the driver what the config descriptors are.

> > > Or maybe somehow expose all endpoints and their features (like
> > > GadgetFS) and let userspace put proper addresses into endpoint
> > > descriptors before enumeration starts.

And if the user messes up the assignment, his driver won't work.

> > I think this is the way to go, giving the userspace maximum control
> > over what's going on. Seems not too hard to implement too. The only
> > part that is not clear to me is how to figure out the endpoint address
> > that a particular UDC endpoint expects to have.
> >
> > Most of the time endpoints are named as something like "ep1in" or
> > "ep2out", and AFAIU the number in the endpoint name is the address.
> > However net2280 seems to have weird endpoint names "ep-a", "ep-b",
> > etc. Looking at usb_ep_autoconfig_ss(), those endpoints supposedly
> > have sequential addresses starting from 1 (for each in/out type). But
> > then in the GadgetFS example [1] e.g. "ep-a" corresponds to address 7,
> > which I don't understand. My guess is that this is done to handle both
> > dummy_udc and net2280 in a single if case, but I'm not sure.
> >
> > [1] http://www.linux-usb.org/gadget/usb.c
> 
> Oh, there's actually a comment that explains this [1] in dummy_hcd.c
> (exactly the place where one would look for it :). So "ep-a" and
> others are fully configurable and accept any endpoint address. OK, I
> think I've figured out what to do here, will send a patch.
> 
> [1] https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/usb/gadget/udc/dummy_hcd.c#L113

I know you want to provide maximum flexibility in terms of what the
user can do.  Still, there has to be some way for the kernel driver to
know what endpoints the user wants to expose and what their
characteristics should be.  You can't get this information just by
snooping Get-Descriptor responses because in theory the host doesn't
have to send any such requests.

Alan Stern

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Tue, Apr 28, 2020 at 3:27 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> Better late than never...
>
> On Tue, 28 Apr 2020, Andrey Konovalov wrote:
>
> > On Tue, Apr 28, 2020 at 2:50 AM Andrey Konovalov <andreyknvl@google.com> wrote:
> > >
> > > On Mon, Apr 27, 2020 at 10:47 PM Andrey Konovalov <andreyknvl@google.com> wrote:
> > > >
> > > > On Mon, Apr 27, 2020 at 9:51 PM Andrey Konovalov <andreyknvl@google.com> wrote:
>
> > > > > Another question, which actually seems to be a major problem.
> > > > >
> > > > > It looks like automatic endpoint selection based on required features
> > > > > doesn't work in raw-gadget. The way it tries to do that is iterating
> > > > > over the list of available endpoints and finding one that has the
> > > > > right direction and transfer type. But it seems that the right way to
> > > > > do that (like it's done in usb_ep_autoconfig()) is to also patch the
> > > > > bEndpointAddress field of the endpoint descriptor.
> > > > >
> > > > > Currently with raw-gadget the endpoints are supposed to be enabled
> > > > > after a set_configuration/set_interface request from the host, so it's
> > > > > too late to patch the endpoint descriptor (the one that was sent to
> > > > > the host during enumeration). I'm guessing that sending one endpoint
> > > > > descriptor during enumeration and then using a different one (with
> > > > > patched bEndpointAddress) to set ep->desc before doing usb_ep_enable()
> > > > > won't work (as there's going to be mismatch in endpoint addresses
> > > > > between the host and the UDC), right?
>
> It certainly won't work.
>
> > > > > I wonder what we can do about that. It seems that we actually need to
> > > > > parse the descriptors and figure out the right endpoints before the
> > > > > enumeration starts.
>
> That's right.  In the case of raw-gadget, this would be done around the
> time the user tells the driver what the config descriptors are.
>
> > > > Or maybe somehow expose all endpoints and their features (like
> > > > GadgetFS) and let userspace put proper addresses into endpoint
> > > > descriptors before enumeration starts.
>
> And if the user messes up the assignment, his driver won't work.
>
> > > I think this is the way to go, giving the userspace maximum control
> > > over what's going on. Seems not too hard to implement too. The only
> > > part that is not clear to me is how to figure out the endpoint address
> > > that a particular UDC endpoint expects to have.
> > >
> > > Most of the time endpoints are named as something like "ep1in" or
> > > "ep2out", and AFAIU the number in the endpoint name is the address.
> > > However net2280 seems to have weird endpoint names "ep-a", "ep-b",
> > > etc. Looking at usb_ep_autoconfig_ss(), those endpoints supposedly
> > > have sequential addresses starting from 1 (for each in/out type). But
> > > then in the GadgetFS example [1] e.g. "ep-a" corresponds to address 7,
> > > which I don't understand. My guess is that this is done to handle both
> > > dummy_udc and net2280 in a single if case, but I'm not sure.
> > >
> > > [1] http://www.linux-usb.org/gadget/usb.c
> >
> > Oh, there's actually a comment that explains this [1] in dummy_hcd.c
> > (exactly the place where one would look for it :). So "ep-a" and
> > others are fully configurable and accept any endpoint address. OK, I
> > think I've figured out what to do here, will send a patch.
> >
> > [1] https://elixir.bootlin.com/linux/v5.7-rc3/source/drivers/usb/gadget/udc/dummy_hcd.c#L113

Hi Alan,

I've been looking at this a little more. Do I understand correctly
that even though Dummy UDC names endpoints as "ep1in", etc. it
actually allows to assign endpoints addresses different from what is
specified in the endpoint names (it uses find_endpoint() to find the
right endpoint based on ep->desc)? E.g. you can technically assign
endpoint with address 2 (| USB_DIR_IN) to "ep1in".

If this is correct, this kind of limits Dummy UDC usage with Raw
Gadget the way it is currently implemented, as Raw Gadget assumes that
the endpoint address must be fixed when the endpoint is named as
ep1in.

Would it be acceptable to add another mode to Dummy UDC that names the
endpoints as "ep-a"? Perhaps enabled with a module parameter. I'm not
sure if this kind of naming would be technically correct, as "ep-a"
name assumes that we can assign arbitrary transfer type to the
endpoint as well, which isn't possible with Dummy UDC, as it doesn't
support ISO transfers.

Or do you think there can be another way to expose the fact that Dummy
UDC allows arbitrary addresses? I could hardcode this into Raw Gadget,
but it doesn't feel like the right approach.

Thanks!

Re: Testing endpoint halt support for raw-gadget

[Alan Stern]

On Wed, May 13, 2020 at 07:07:20PM +0200, Andrey Konovalov wrote:
> Hi Alan,
> 
> I've been looking at this a little more. Do I understand correctly
> that even though Dummy UDC names endpoints as "ep1in", etc. it
> actually allows to assign endpoints addresses different from what is
> specified in the endpoint names (it uses find_endpoint() to find the
> right endpoint based on ep->desc)? E.g. you can technically assign
> endpoint with address 2 (| USB_DIR_IN) to "ep1in".

Yes, that's right.  In fact, you can do this with any UDC.  (But with 
other UDCs it won't work, whereas with dummy-hcd it will.)

> If this is correct, this kind of limits Dummy UDC usage with Raw
> Gadget the way it is currently implemented, as Raw Gadget assumes that
> the endpoint address must be fixed when the endpoint is named as
> ep1in.

Okay.  That makes sense, since it is true for most UDCs.

> Would it be acceptable to add another mode to Dummy UDC that names the
> endpoints as "ep-a"? Perhaps enabled with a module parameter. I'm not
> sure if this kind of naming would be technically correct, as "ep-a"
> name assumes that we can assign arbitrary transfer type to the
> endpoint as well, which isn't possible with Dummy UDC, as it doesn't
> support ISO transfers.
> 
> Or do you think there can be another way to expose the fact that Dummy
> UDC allows arbitrary addresses? I could hardcode this into Raw Gadget,
> but it doesn't feel like the right approach.

Why do you want to do this?  Does anything go wrong if you just continue 
to assume the endpoint numbers are fixed?

I suppose, if you thought this was really necessary, we could change 
find_endpoint() so that it looks for a match against the endpoint's name 
instead of against the address stored in the descriptor.  Or we could 
change the last thirteen "generic" endpoints in ep_info[] to be 
configurable: "ep-a", ..., "ep-m", or "ep-aout", ..., "ep-min".  (The 
fact that the endpoints don't support isochronous is exposed through the 
usb_ep_caps structures.)

Alan Stern

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Wed, May 13, 2020 at 8:14 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, May 13, 2020 at 07:07:20PM +0200, Andrey Konovalov wrote:
> > Hi Alan,
> >
> > I've been looking at this a little more. Do I understand correctly
> > that even though Dummy UDC names endpoints as "ep1in", etc. it
> > actually allows to assign endpoints addresses different from what is
> > specified in the endpoint names (it uses find_endpoint() to find the
> > right endpoint based on ep->desc)? E.g. you can technically assign
> > endpoint with address 2 (| USB_DIR_IN) to "ep1in".
>
> Yes, that's right.  In fact, you can do this with any UDC.  (But with
> other UDCs it won't work, whereas with dummy-hcd it will.)
>
> > If this is correct, this kind of limits Dummy UDC usage with Raw
> > Gadget the way it is currently implemented, as Raw Gadget assumes that
> > the endpoint address must be fixed when the endpoint is named as
> > ep1in.
>
> Okay.  That makes sense, since it is true for most UDCs.
>
> > Would it be acceptable to add another mode to Dummy UDC that names the
> > endpoints as "ep-a"? Perhaps enabled with a module parameter. I'm not
> > sure if this kind of naming would be technically correct, as "ep-a"
> > name assumes that we can assign arbitrary transfer type to the
> > endpoint as well, which isn't possible with Dummy UDC, as it doesn't
> > support ISO transfers.
> >
> > Or do you think there can be another way to expose the fact that Dummy
> > UDC allows arbitrary addresses? I could hardcode this into Raw Gadget,
> > but it doesn't feel like the right approach.
>
> Why do you want to do this?  Does anything go wrong if you just continue
> to assume the endpoint numbers are fixed?

Yes. Some USB drivers require endpoints with certain logical functions
to have certain addresses (e.g. for ath9k: [1]). This limits the
ability to use Raw Gadget + Dummy UDC for fuzzing, as sometimes we
can't emulate such devices unless Dummy UDC endpoint with a particular
address has required capabilities to implement those logical functions
(e.g. for ath9k: we can't emulate USB_REG_IN_PIPE endpoint, as Dummy
UDC only has an OUT endpoint with address 3).

It's acceptable to be unable to emulate such devices with real UDCs
with fixed address endpoints, but it would be highly desirable to be
able to do that with Dummy UDC, as it technically supports
configurable endpoint addresses.

[1] https://elixir.bootlin.com/linux/v5.6.12/source/drivers/net/wireless/ath/ath9k/hif_usb.h#L68

> I suppose, if you thought this was really necessary, we could change
> find_endpoint() so that it looks for a match against the endpoint's name
> instead of against the address stored in the descriptor.

That would make the behaviour of Dummy UDC match what is expected from
it based on the endpoint names, but won't help with the problem
described above.

> Or we could
> change the last thirteen "generic" endpoints in ep_info[] to be
> configurable: "ep-a", ..., "ep-m", or "ep-aout", ..., "ep-min".  (The
> fact that the endpoints don't support isochronous is exposed through the
> usb_ep_caps structures.)

I think this should work. If we put this under a module parameter,
then we can make all endpoints have configurable names.

Re: Testing endpoint halt support for raw-gadget

[Alan Stern]

On Wed, May 13, 2020 at 08:31:35PM +0200, Andrey Konovalov wrote:
> On Wed, May 13, 2020 at 8:14 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Wed, May 13, 2020 at 07:07:20PM +0200, Andrey Konovalov wrote:
> > > Hi Alan,
> > >
> > > I've been looking at this a little more. Do I understand correctly
> > > that even though Dummy UDC names endpoints as "ep1in", etc. it
> > > actually allows to assign endpoints addresses different from what is
> > > specified in the endpoint names (it uses find_endpoint() to find the
> > > right endpoint based on ep->desc)? E.g. you can technically assign
> > > endpoint with address 2 (| USB_DIR_IN) to "ep1in".
> >
> > Yes, that's right.  In fact, you can do this with any UDC.  (But with
> > other UDCs it won't work, whereas with dummy-hcd it will.)
> >
> > > If this is correct, this kind of limits Dummy UDC usage with Raw
> > > Gadget the way it is currently implemented, as Raw Gadget assumes that
> > > the endpoint address must be fixed when the endpoint is named as
> > > ep1in.
> >
> > Okay.  That makes sense, since it is true for most UDCs.
> >
> > > Would it be acceptable to add another mode to Dummy UDC that names the
> > > endpoints as "ep-a"? Perhaps enabled with a module parameter. I'm not
> > > sure if this kind of naming would be technically correct, as "ep-a"
> > > name assumes that we can assign arbitrary transfer type to the
> > > endpoint as well, which isn't possible with Dummy UDC, as it doesn't
> > > support ISO transfers.
> > >
> > > Or do you think there can be another way to expose the fact that Dummy
> > > UDC allows arbitrary addresses? I could hardcode this into Raw Gadget,
> > > but it doesn't feel like the right approach.
> >
> > Why do you want to do this?  Does anything go wrong if you just continue
> > to assume the endpoint numbers are fixed?
> 
> Yes. Some USB drivers require endpoints with certain logical functions
> to have certain addresses (e.g. for ath9k: [1]). This limits the
> ability to use Raw Gadget + Dummy UDC for fuzzing, as sometimes we
> can't emulate such devices unless Dummy UDC endpoint with a particular
> address has required capabilities to implement those logical functions
> (e.g. for ath9k: we can't emulate USB_REG_IN_PIPE endpoint, as Dummy
> UDC only has an OUT endpoint with address 3).
> 
> It's acceptable to be unable to emulate such devices with real UDCs
> with fixed address endpoints, but it would be highly desirable to be
> able to do that with Dummy UDC, as it technically supports
> configurable endpoint addresses.

Okay, that's reasonable.

> [1] https://elixir.bootlin.com/linux/v5.6.12/source/drivers/net/wireless/ath/ath9k/hif_usb.h#L68
> 
> > I suppose, if you thought this was really necessary, we could change
> > find_endpoint() so that it looks for a match against the endpoint's name
> > instead of against the address stored in the descriptor.
> 
> That would make the behaviour of Dummy UDC match what is expected from
> it based on the endpoint names, but won't help with the problem
> described above.

Would you want to do this anyway?  It doesn't seem necessary to me.

> > Or we could
> > change the last thirteen "generic" endpoints in ep_info[] to be
> > configurable: "ep-a", ..., "ep-m", or "ep-aout", ..., "ep-min".  (The
> > fact that the endpoints don't support isochronous is exposed through the
> > usb_ep_caps structures.)
> 
> I think this should work. If we put this under a module parameter,
> then we can make all endpoints have configurable names.

No need for a module parameter; just make it a permanent change to the 
driver.  Would you like to submit a patch?

Alan Stern

Re: Testing endpoint halt support for raw-gadget

[Andrey Konovalov]

On Wed, May 13, 2020 at 9:09 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, May 13, 2020 at 08:31:35PM +0200, Andrey Konovalov wrote:
> > On Wed, May 13, 2020 at 8:14 PM Alan Stern <stern@rowland.harvard.edu> wrote:
> > >
> > > On Wed, May 13, 2020 at 07:07:20PM +0200, Andrey Konovalov wrote:
> > > > Hi Alan,
> > > >
> > > > I've been looking at this a little more. Do I understand correctly
> > > > that even though Dummy UDC names endpoints as "ep1in", etc. it
> > > > actually allows to assign endpoints addresses different from what is
> > > > specified in the endpoint names (it uses find_endpoint() to find the
> > > > right endpoint based on ep->desc)? E.g. you can technically assign
> > > > endpoint with address 2 (| USB_DIR_IN) to "ep1in".
> > >
> > > Yes, that's right.  In fact, you can do this with any UDC.  (But with
> > > other UDCs it won't work, whereas with dummy-hcd it will.)
> > >
> > > > If this is correct, this kind of limits Dummy UDC usage with Raw
> > > > Gadget the way it is currently implemented, as Raw Gadget assumes that
> > > > the endpoint address must be fixed when the endpoint is named as
> > > > ep1in.
> > >
> > > Okay.  That makes sense, since it is true for most UDCs.
> > >
> > > > Would it be acceptable to add another mode to Dummy UDC that names the
> > > > endpoints as "ep-a"? Perhaps enabled with a module parameter. I'm not
> > > > sure if this kind of naming would be technically correct, as "ep-a"
> > > > name assumes that we can assign arbitrary transfer type to the
> > > > endpoint as well, which isn't possible with Dummy UDC, as it doesn't
> > > > support ISO transfers.
> > > >
> > > > Or do you think there can be another way to expose the fact that Dummy
> > > > UDC allows arbitrary addresses? I could hardcode this into Raw Gadget,
> > > > but it doesn't feel like the right approach.
> > >
> > > Why do you want to do this?  Does anything go wrong if you just continue
> > > to assume the endpoint numbers are fixed?
> >
> > Yes. Some USB drivers require endpoints with certain logical functions
> > to have certain addresses (e.g. for ath9k: [1]). This limits the
> > ability to use Raw Gadget + Dummy UDC for fuzzing, as sometimes we
> > can't emulate such devices unless Dummy UDC endpoint with a particular
> > address has required capabilities to implement those logical functions
> > (e.g. for ath9k: we can't emulate USB_REG_IN_PIPE endpoint, as Dummy
> > UDC only has an OUT endpoint with address 3).
> >
> > It's acceptable to be unable to emulate such devices with real UDCs
> > with fixed address endpoints, but it would be highly desirable to be
> > able to do that with Dummy UDC, as it technically supports
> > configurable endpoint addresses.
>
> Okay, that's reasonable.
>
> > [1] https://elixir.bootlin.com/linux/v5.6.12/source/drivers/net/wireless/ath/ath9k/hif_usb.h#L68
> >
> > > I suppose, if you thought this was really necessary, we could change
> > > find_endpoint() so that it looks for a match against the endpoint's name
> > > instead of against the address stored in the descriptor.
> >
> > That would make the behaviour of Dummy UDC match what is expected from
> > it based on the endpoint names, but won't help with the problem
> > described above.
>
> Would you want to do this anyway?  It doesn't seem necessary to me.

No, no need for this :)

>
> > > Or we could
> > > change the last thirteen "generic" endpoints in ep_info[] to be
> > > configurable: "ep-a", ..., "ep-m", or "ep-aout", ..., "ep-min".  (The
> > > fact that the endpoints don't support isochronous is exposed through the
> > > usb_ep_caps structures.)
> >
> > I think this should work. If we put this under a module parameter,
> > then we can make all endpoints have configurable names.
>
> No need for a module parameter; just make it a permanent change to the
> driver.

I was thinking that it could break some existing users, but I don't
know for sure.

> Would you like to submit a patch?

Sure, will do. Thanks!