f_mass_storage vs drivers/target

f_mass_storage vs drivers/target

[Benjamin Herrenschmidt]

Hi folks !

It seems that f_mass_storage duplicates (well maybe predates too..) a
lot of what's in drivers/target.

Anybody working on implementing a new version of f_mass_storage that
is layered upon drivers/target instead ? That would bring quite a lot
of additional functionality.

If not, I might look into it.

Cheers,
Ben.

Re: f_mass_storage vs drivers/target

[Greg KH]

On Wed, Aug 21, 2019 at 01:38:49PM +1000, Benjamin Herrenschmidt wrote:
> Hi folks !
> 
> It seems that f_mass_storage duplicates (well maybe predates too..) a
> lot of what's in drivers/target.

It predates it by a long time.

> Anybody working on implementing a new version of f_mass_storage that
> is layered upon drivers/target instead ? That would bring quite a lot
> of additional functionality.

Why is that needed?  What functionality is missing that it will provide?
Will it make the code simpler?

> If not, I might look into it.

Hey, we don't refuse patches, for cleaning stuff up, you know that :)

thanks,

greg k-h

Re: f_mass_storage vs drivers/target

[Alan Stern]

On Wed, 21 Aug 2019, Benjamin Herrenschmidt wrote:

> Hi folks !
> 
> It seems that f_mass_storage duplicates (well maybe predates too..) a
> lot of what's in drivers/target.
> 
> Anybody working on implementing a new version of f_mass_storage that
> is layered upon drivers/target instead ? That would bring quite a lot
> of additional functionality.

That's what f_tcm does.

Alan Stern

> If not, I might look into it.
> 
> Cheers,
> Ben.

Re: f_mass_storage vs drivers/target

[Benjamin Herrenschmidt]

On Wed, 2019-08-21 at 10:25 -0400, Alan Stern wrote:
> On Wed, 21 Aug 2019, Benjamin Herrenschmidt wrote:
> 
> > Hi folks !
> > 
> > It seems that f_mass_storage duplicates (well maybe predates too..)
> a
> > lot of what's in drivers/target.
> > 
> > Anybody working on implementing a new version of f_mass_storage
> that
> > is layered upon drivers/target instead ? That would bring quite a
> lot
> > of additional functionality.
> 
> That's what f_tcm does.
> 
> Alan Stern

Haha nice, I never noticed it :-)

Cheers,
Ben.

Re: f_mass_storage vs drivers/target

[Benjamin Herrenschmidt]

On Thu, 2019-08-22 at 10:10 +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2019-08-21 at 10:25 -0400, Alan Stern wrote:
> > On Wed, 21 Aug 2019, Benjamin Herrenschmidt wrote:
> > 
> > > Hi folks !
> > > 
> > > It seems that f_mass_storage duplicates (well maybe predates too..)
> > 
> > a
> > > lot of what's in drivers/target.
> > > 
> > > Anybody working on implementing a new version of f_mass_storage
> > 
> > that
> > > is layered upon drivers/target instead ? That would bring quite a
> > 
> > lot
> > > of additional functionality.
> > 
> > That's what f_tcm does.
> > 
> > Alan Stern
> 
> Haha nice, I never noticed it :-)

Ah lovely ... the 338x fails in EP autoconf with f_tcm, digging...

While digging I found this gem:

	/* USB3380: use same address for usb and hardware endpoints */
	snprintf(name, sizeof(name), "ep%d%s", usb_endpoint_num(desc),
			usb_endpoint_dir_in(desc) ? "in" : "out");
	ep = gadget_find_ep_by_name(_gadget, name);
	if (ep && usb_gadget_ep_match_desc(_gadget, ep, desc, ep_comp))
		return ep;

Any idea what's that supposed to achieve ?

When ep_match is called, usb_endpoint_num() hasn't been set yet so
it's always 0 and always fails... or am I missing something ?

Cheers,
Ben.

Re: f_mass_storage vs drivers/target

[Benjamin Herrenschmidt]

On Thu, 2019-08-22 at 14:58 +1000, Benjamin Herrenschmidt wrote:
> 
> Ah lovely ... the 338x fails in EP autoconf with f_tcm, digging...
> 
> While digging I found this gem:
> 
> 	/* USB3380: use same address for usb and hardware endpoints */
> 	snprintf(name, sizeof(name), "ep%d%s", usb_endpoint_num(desc),
> 			usb_endpoint_dir_in(desc) ? "in" : "out");
> 	ep = gadget_find_ep_by_name(_gadget, name);
> 	if (ep && usb_gadget_ep_match_desc(_gadget, ep, desc, ep_comp))
> 		return ep;
> 
> Any idea what's that supposed to achieve ?
> 
> When ep_match is called, usb_endpoint_num() hasn't been set yet so
> it's always 0 and always fails... or am I missing something ?

Two problems:

 - net2280.c doesn't set a max EP size, so autoconfig fails since
f_tcm specifies one. What about this ?

--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -940,12 +940,14 @@ int usb_gadget_ep_match_desc(struct usb_gadget *gadget,
        if (usb_endpoint_dir_out(desc) && !ep->caps.dir_out)
                return 0;
 
-       if (max > ep->maxpacket_limit)
+       if (ep->maxpacket_limit && max > ep->maxpacket_limit)
                return 0;

(ie assume that ep->maxpacket_limit 0 means the UDC supports any
legal size)

 - No UDC driver other than dummy sets max_streams, and f_tcm requires 4,
so f_tcm will fail with *any* superspeed UDC driver as far as I can tell.

Was it ever tested with USB 3 ?

I'm not sure what the right fix here yet is as I yet have to learn about
what those USB3 streams are :-) For now I've commented things out.

It's still not working yet as configuring f_tcm seems to be a black art
with no useful documentation or examples anywhere (the device shows up on
the host but doesn't bind to any mass storage driver ... yet).

Cheers,
Ben.

Re: f_mass_storage vs drivers/target

[Benjamin Herrenschmidt]

On Thu, 2019-08-22 at 15:14 +1000, Benjamin Herrenschmidt wrote:
> - No UDC driver other than dummy sets max_streams, and f_tcm requires 4,
> so f_tcm will fail with *any* superspeed UDC driver as far as I can tell.
> 
> Was it ever tested with USB 3 ?

Ok so I spoke too soon... dwc3 does, I didn't notice bcs it doesn't
live in drivers/usb/gadget...

So Sebastian, what would be the best way to "fallback" to non-stream
use rather than just fail probing ?

Do we really want the autoconf EP matching to enforce the stream
numbers ? If we allow it to not match, then f_tcm could check after
matching if enough streams were found and disable stream support if
not.

Another option would be to make this 2-pass, though that complicates
even more: a first pass where we enforce the stream count, and if
we fail, a second pass where we ignore them.

Suggestions ?

Cheers,
Ben.

Re: f_mass_storage vs drivers/target

[Alan Stern]

On Thu, 22 Aug 2019, Benjamin Herrenschmidt wrote:

> On Thu, 2019-08-22 at 14:58 +1000, Benjamin Herrenschmidt wrote:
> > 
> > Ah lovely ... the 338x fails in EP autoconf with f_tcm, digging...
> > 
> > While digging I found this gem:
> > 
> > 	/* USB3380: use same address for usb and hardware endpoints */
> > 	snprintf(name, sizeof(name), "ep%d%s", usb_endpoint_num(desc),
> > 			usb_endpoint_dir_in(desc) ? "in" : "out");
> > 	ep = gadget_find_ep_by_name(_gadget, name);
> > 	if (ep && usb_gadget_ep_match_desc(_gadget, ep, desc, ep_comp))
> > 		return ep;
> > 
> > Any idea what's that supposed to achieve ?

It looks like in one mode, the endpoint number has to be the value 
predetermined by the hardware.  In the other mode, any hardware 
endpoint can be assigned any endpoint number.

> > When ep_match is called, usb_endpoint_num() hasn't been set yet so
> > it's always 0 and always fails... or am I missing something ?
> 
> Two problems:
> 
>  - net2280.c doesn't set a max EP size, so autoconfig fails since
> f_tcm specifies one. What about this ?
> 
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -940,12 +940,14 @@ int usb_gadget_ep_match_desc(struct usb_gadget *gadget,
>         if (usb_endpoint_dir_out(desc) && !ep->caps.dir_out)
>                 return 0;
>  
> -       if (max > ep->maxpacket_limit)
> +       if (ep->maxpacket_limit && max > ep->maxpacket_limit)
>                 return 0;
> 
> (ie assume that ep->maxpacket_limit 0 means the UDC supports any
> legal size)

That looks reasonable.

>  - No UDC driver other than dummy sets max_streams, and f_tcm requires 4,
> so f_tcm will fail with *any* superspeed UDC driver as far as I can tell.
> 
> Was it ever tested with USB 3 ?

Note that USB 2 does not support streams at all.

> I'm not sure what the right fix here yet is as I yet have to learn about
> what those USB3 streams are :-) For now I've commented things out.

They are for multiplexing multiple data streams over a single USB 
endpoint.  As far as I know, the only use case for such a thing is USB 
Mass Storage.

Alan Stern

> It's still not working yet as configuring f_tcm seems to be a black art
> with no useful documentation or examples anywhere (the device shows up on
> the host but doesn't bind to any mass storage driver ... yet).
> 
> Cheers,
> Ben.

Re: f_mass_storage vs drivers/target

[Benjamin Herrenschmidt]

On Thu, 2019-08-22 at 13:30 -0400, Alan Stern wrote:
> On Thu, 22 Aug 2019, Benjamin Herrenschmidt wrote:
> 
> > On Thu, 2019-08-22 at 14:58 +1000, Benjamin Herrenschmidt wrote:
> > > 
> > > Ah lovely ... the 338x fails in EP autoconf with f_tcm, digging...
> > > 
> > > While digging I found this gem:
> > > 
> > >     /* USB3380: use same address for usb and hardware endpoints */
> > >     snprintf(name, sizeof(name), "ep%d%s", usb_endpoint_num(desc),
> > >                     usb_endpoint_dir_in(desc) ? "in" : "out");
> > >     ep = gadget_find_ep_by_name(_gadget, name);
> > >     if (ep && usb_gadget_ep_match_desc(_gadget, ep, desc, ep_comp))
> > >             return ep;
> > > 
> > > Any idea what's that supposed to achieve ?
> 
> It looks like in one mode, the endpoint number has to be the value 
> predetermined by the hardware.  In the other mode, any hardware 
> endpoint can be assigned any endpoint number.

Sure but as I wrote, this is ep_match, which when called, always has
usb_endpoint_num() set to 0... this function is supposed to chose the
EP number afaik. So I don't think the above ever works, it just returns
NULL. Or do we ever call this again with already predetermined EP nums,
for example when doing multifunction ?

> > > When ep_match is called, usb_endpoint_num() hasn't been set yet so
> > > it's always 0 and always fails... or am I missing something ?
> > 
> > Two problems:
> > 
> >  - net2280.c doesn't set a max EP size, so autoconfig fails since
> > f_tcm specifies one. What about this ?
> > 
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -940,12 +940,14 @@ int usb_gadget_ep_match_desc(struct usb_gadget *gadget,
> >         if (usb_endpoint_dir_out(desc) && !ep->caps.dir_out)
> >                 return 0;
> >  
> > -       if (max > ep->maxpacket_limit)
> > +       if (ep->maxpacket_limit && max > ep->maxpacket_limit)
> >                 return 0;
> > 
> > (ie assume that ep->maxpacket_limit 0 means the UDC supports any
> > legal size)
> 
> That looks reasonable.

I'll send a patch.

> >  - No UDC driver other than dummy sets max_streams, and f_tcm requires 4,
> > so f_tcm will fail with *any* superspeed UDC driver as far as I can tell.
> > 
> > Was it ever tested with USB 3 ?
> 
> Note that USB 2 does not support streams at all.

Yes, f_tcm only requires them for superspeed, but it does *require*
them in that case.

> > I'm not sure what the right fix here yet is as I yet have to learn about
> > what those USB3 streams are :-) For now I've commented things out.
> 
> They are for multiplexing multiple data streams over a single USB 
> endpoint.  As far as I know, the only use case for such a thing is USB 
> Mass Storage.

So f_tcm could operate in a degraded mode in the absence of streams
easily, the problem is the mechanics of EP matching in epautoconf. It
will just fail.

I wonder since f_tcm is also the only user, whether we could change the
matching logic to either:

  - Don't try to match, return streams is available. This could be
problematic if the UDC supports streams on some EPs and not others
however.

  - Do two passes: one pass trying to match the streams, and one patch
without matching them if the first one fails.

Then f_tcm could check whether it got EPs with streams and enable
stream usage accordingly.

Opinions ? Other option ?

Cheers,
Ben.

Re: f_mass_storage vs drivers/target

[Alan Stern]

On Fri, 23 Aug 2019, Benjamin Herrenschmidt wrote:

> On Thu, 2019-08-22 at 13:30 -0400, Alan Stern wrote:
> > On Thu, 22 Aug 2019, Benjamin Herrenschmidt wrote:
> > 
> > > On Thu, 2019-08-22 at 14:58 +1000, Benjamin Herrenschmidt wrote:
> > > > 
> > > > Ah lovely ... the 338x fails in EP autoconf with f_tcm, digging...
> > > > 
> > > > While digging I found this gem:
> > > > 
> > > >     /* USB3380: use same address for usb and hardware endpoints */
> > > >     snprintf(name, sizeof(name), "ep%d%s", usb_endpoint_num(desc),
> > > >                     usb_endpoint_dir_in(desc) ? "in" : "out");
> > > >     ep = gadget_find_ep_by_name(_gadget, name);
> > > >     if (ep && usb_gadget_ep_match_desc(_gadget, ep, desc, ep_comp))
> > > >             return ep;
> > > > 
> > > > Any idea what's that supposed to achieve ?
> > 
> > It looks like in one mode, the endpoint number has to be the value 
> > predetermined by the hardware.  In the other mode, any hardware 
> > endpoint can be assigned any endpoint number.
> 
> Sure but as I wrote, this is ep_match, which when called, always has
> usb_endpoint_num() set to 0... this function is supposed to chose the
> EP number afaik. So I don't think the above ever works, it just returns
> NULL. Or do we ever call this again with already predetermined EP nums,
> for example when doing multifunction ?

I don't know.  You might try asking the person who wrote that code.

> > > > When ep_match is called, usb_endpoint_num() hasn't been set yet so
> > > > it's always 0 and always fails... or am I missing something ?
> > > 
> > > Two problems:
> > > 
> > >  - net2280.c doesn't set a max EP size, so autoconfig fails since
> > > f_tcm specifies one. What about this ?
> > > 
> > > --- a/drivers/usb/gadget/udc/core.c
> > > +++ b/drivers/usb/gadget/udc/core.c
> > > @@ -940,12 +940,14 @@ int usb_gadget_ep_match_desc(struct usb_gadget *gadget,
> > >         if (usb_endpoint_dir_out(desc) && !ep->caps.dir_out)
> > >                 return 0;
> > >  
> > > -       if (max > ep->maxpacket_limit)
> > > +       if (ep->maxpacket_limit && max > ep->maxpacket_limit)
> > >                 return 0;
> > > 
> > > (ie assume that ep->maxpacket_limit 0 means the UDC supports any
> > > legal size)
> > 
> > That looks reasonable.
> 
> I'll send a patch.
> 
> > >  - No UDC driver other than dummy sets max_streams, and f_tcm requires 4,
> > > so f_tcm will fail with *any* superspeed UDC driver as far as I can tell.
> > > 
> > > Was it ever tested with USB 3 ?
> > 
> > Note that USB 2 does not support streams at all.
> 
> Yes, f_tcm only requires them for superspeed, but it does *require*
> them in that case.

I don't see any reason why f_tcm shouldn't fall back on Bulk-Only 
Transport when streams aren't available, even on a SuperSpeed 
connection.

> > > I'm not sure what the right fix here yet is as I yet have to learn about
> > > what those USB3 streams are :-) For now I've commented things out.
> > 
> > They are for multiplexing multiple data streams over a single USB 
> > endpoint.  As far as I know, the only use case for such a thing is USB 
> > Mass Storage.
> 
> So f_tcm could operate in a degraded mode in the absence of streams
> easily, the problem is the mechanics of EP matching in epautoconf. It
> will just fail.
> 
> I wonder since f_tcm is also the only user, whether we could change the
> matching logic to either:
> 
>   - Don't try to match, return streams is available. This could be
> problematic if the UDC supports streams on some EPs and not others
> however.
> 
>   - Do two passes: one pass trying to match the streams, and one patch
> without matching them if the first one fails.
> 
> Then f_tcm could check whether it got EPs with streams and enable
> stream usage accordingly.
> 
> Opinions ? Other option ?

I like the two-pass option better.  Maybe Felipe will weigh in when
he's back from vacation.

Alan Stern

Re: f_mass_storage vs drivers/target

[Benjamin Herrenschmidt]

On Fri, 2019-08-23 at 11:21 -0400, Alan Stern wrote:
> > I wonder since f_tcm is also the only user, whether we could change
> > the
> > matching logic to either:
> > 
> >    - Don't try to match, return streams is available. This could be
> > problematic if the UDC supports streams on some EPs and not others
> > however.
> > 
> >    - Do two passes: one pass trying to match the streams, and one
> > patch
> > without matching them if the first one fails.
> > 
> > Then f_tcm could check whether it got EPs with streams and enable
> > stream usage accordingly.
> > 
> > Opinions ? Other option ?
> 
> I like the two-pass option better.  Maybe Felipe will weigh in when
> he's back from vacation.

Yup, I prefer it too. I'll cook up something in the meantime so I have
a patch to show.

Cheers,
Ben.