* f_mass_storage vs drivers/target
@ 2019-08-21 3:38 Benjamin Herrenschmidt
2019-08-21 9:32 ` Greg KH
2019-08-21 14:25 ` Alan Stern
0 siblings, 2 replies; 11+ messages in thread
From: Benjamin Herrenschmidt @ 2019-08-21 3:38 UTC (permalink / raw)
To: USB list; +Cc: Alan Stern
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: f_mass_storage vs drivers/target
2019-08-21 3:38 f_mass_storage vs drivers/target Benjamin Herrenschmidt
@ 2019-08-21 9:32 ` Greg KH
2019-08-21 14:25 ` Alan Stern
1 sibling, 0 replies; 11+ messages in thread
From: Greg KH @ 2019-08-21 9:32 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: USB list, Alan Stern
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: f_mass_storage vs drivers/target
2019-08-21 3:38 f_mass_storage vs drivers/target Benjamin Herrenschmidt
2019-08-21 9:32 ` Greg KH
@ 2019-08-21 14:25 ` Alan Stern
2019-08-22 0:10 ` Benjamin Herrenschmidt
1 sibling, 1 reply; 11+ messages in thread
From: Alan Stern @ 2019-08-21 14:25 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: USB list
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: f_mass_storage vs drivers/target
2019-08-21 14:25 ` Alan Stern
@ 2019-08-22 0:10 ` Benjamin Herrenschmidt
2019-08-22 4:58 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 11+ messages in thread
From: Benjamin Herrenschmidt @ 2019-08-22 0:10 UTC (permalink / raw)
To: Alan Stern; +Cc: USB list
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: f_mass_storage vs drivers/target
2019-08-22 0:10 ` Benjamin Herrenschmidt
@ 2019-08-22 4:58 ` Benjamin Herrenschmidt
2019-08-22 5:14 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 11+ messages in thread
From: Benjamin Herrenschmidt @ 2019-08-22 4:58 UTC (permalink / raw)
To: Alan Stern; +Cc: USB list
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: f_mass_storage vs drivers/target
2019-08-22 4:58 ` Benjamin Herrenschmidt
@ 2019-08-22 5:14 ` Benjamin Herrenschmidt
2019-08-22 5:48 ` Benjamin Herrenschmidt
2019-08-22 17:30 ` Alan Stern
0 siblings, 2 replies; 11+ messages in thread
From: Benjamin Herrenschmidt @ 2019-08-22 5:14 UTC (permalink / raw)
To: Alan Stern; +Cc: USB list, Sebastian Andrzej Siewior
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: f_mass_storage vs drivers/target
2019-08-22 5:14 ` Benjamin Herrenschmidt
@ 2019-08-22 5:48 ` Benjamin Herrenschmidt
2019-08-22 17:30 ` Alan Stern
1 sibling, 0 replies; 11+ messages in thread
From: Benjamin Herrenschmidt @ 2019-08-22 5:48 UTC (permalink / raw)
To: Alan Stern; +Cc: USB list, Sebastian Andrzej Siewior
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: f_mass_storage vs drivers/target
2019-08-22 5:14 ` Benjamin Herrenschmidt
2019-08-22 5:48 ` Benjamin Herrenschmidt
@ 2019-08-22 17:30 ` Alan Stern
2019-08-23 0:40 ` Benjamin Herrenschmidt
1 sibling, 1 reply; 11+ messages in thread
From: Alan Stern @ 2019-08-22 17:30 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: USB list, Sebastian Andrzej Siewior
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: f_mass_storage vs drivers/target
2019-08-22 17:30 ` Alan Stern
@ 2019-08-23 0:40 ` Benjamin Herrenschmidt
2019-08-23 15:21 ` Alan Stern
0 siblings, 1 reply; 11+ messages in thread
From: Benjamin Herrenschmidt @ 2019-08-23 0:40 UTC (permalink / raw)
To: Alan Stern; +Cc: USB list, Sebastian Andrzej Siewior
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: f_mass_storage vs drivers/target
2019-08-23 0:40 ` Benjamin Herrenschmidt
@ 2019-08-23 15:21 ` Alan Stern
2019-08-26 2:19 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 11+ messages in thread
From: Alan Stern @ 2019-08-23 15:21 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: USB list, Sebastian Andrzej Siewior
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: f_mass_storage vs drivers/target
2019-08-23 15:21 ` Alan Stern
@ 2019-08-26 2:19 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 11+ messages in thread
From: Benjamin Herrenschmidt @ 2019-08-26 2:19 UTC (permalink / raw)
To: Alan Stern; +Cc: USB list, Sebastian Andrzej Siewior
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-08-26 2:20 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-21 3:38 f_mass_storage vs drivers/target Benjamin Herrenschmidt
2019-08-21 9:32 ` Greg KH
2019-08-21 14:25 ` Alan Stern
2019-08-22 0:10 ` Benjamin Herrenschmidt
2019-08-22 4:58 ` Benjamin Herrenschmidt
2019-08-22 5:14 ` Benjamin Herrenschmidt
2019-08-22 5:48 ` Benjamin Herrenschmidt
2019-08-22 17:30 ` Alan Stern
2019-08-23 0:40 ` Benjamin Herrenschmidt
2019-08-23 15:21 ` Alan Stern
2019-08-26 2:19 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).