mbox series

[v1,0/3] usb: dwc3: Avoid using reserved EPs

Message ID 20250116154117.148915-1-andriy.shevchenko@linux.intel.com
Headers show
Series usb: dwc3: Avoid using reserved EPs | expand

Message

Andy Shevchenko Jan. 16, 2025, 3:40 p.m. UTC
On some platforms (Intel-based and AFAIK ARM-based) the EPs in the gadget
(USB Device Controller mode) may be reserved for some special means, such as
tracing. This series extends DT schema and driver to avoid using those.
Without this the USB gadget mode won't work properly (those devices that
"luckily" allocated the reserved EPs).

Ferry already tested the privately sent PoC of this, but I ask him again to
re-test this version which is slightly different.

Andy Shevchenko (3):
  dt-bindings: usb: dwc3: Add a property to reserve endpoints
  usb: dwc3: gadget: Add support for snps,reserved-endpoints property
  usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield

 .../devicetree/bindings/usb/snps,dwc3.yaml    | 10 +++++
 drivers/usb/dwc3/dwc3-pci.c                   |  9 +++++
 drivers/usb/dwc3/gadget.c                     | 38 ++++++++++++++++++-
 3 files changed, 56 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Jan. 17, 2025, 1:32 p.m. UTC | #1
On Thu, Jan 16, 2025 at 11:35:19PM +0000, Thinh Nguyen wrote:
> On Thu, Jan 16, 2025, Andy Shevchenko wrote:

...

> >  	for (epnum = 0; epnum < total; epnum++) {
> > -		int			ret;
> > +		for (num = 0; num < count; num++) {
> > +			if (epnum == eps[num])
> > +				break;
> > +		}
> > +		if (num < count)
> > +			continue;
> 
> You can probably rewrite this logic better.

Any suggestions?

Thanks for the review!
Andy Shevchenko Jan. 17, 2025, 1:38 p.m. UTC | #2
On Thu, Jan 16, 2025 at 11:18:45PM +0000, Thinh Nguyen wrote:
> On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> > On some platforms (Intel-based and AFAIK ARM-based) the EPs in the gadget
> > (USB Device Controller mode) may be reserved for some special means, such as
> > tracing. This series extends DT schema and driver to avoid using those.
> > Without this the USB gadget mode won't work properly (those devices that
> > "luckily" allocated the reserved EPs).
> > 
> > Ferry already tested the privately sent PoC of this, but I ask him again to
> > re-test this version which is slightly different.

...

> I'm not entirely clear on the reason for this change yet.
> 
> How would this even work without dwc3 managing these endpoints (all the
> init/teardown/fifo allocation/start/stop flow).

You perhaps know much better how it can be done, I have access to a limited
documentation and in practice if those endpoints are not skipped any gadget
that allocates them simply won't work, and IIRC the entire USB transfers are
stuck.

> Can you provide more info on this hardware?

I am afraid I can't provide more, sorry. I can look for some specifics,
but I'm not that guy who know anything about in-SoC tracing.
Ferry Toth Jan. 17, 2025, 10 p.m. UTC | #3
Op 16-01-2025 om 16:40 schreef Andy Shevchenko:
> On some platforms (Intel-based and AFAIK ARM-based) the EPs in the gadget
> (USB Device Controller mode) may be reserved for some special means, such as
> tracing. This series extends DT schema and driver to avoid using those.
> Without this the USB gadget mode won't work properly (those devices that
> "luckily" allocated the reserved EPs).
> 
> Ferry already tested the privately sent PoC of this, but I ask him again to
> re-test this version which is slightly different.
> 
> Andy Shevchenko (3):
>    dt-bindings: usb: dwc3: Add a property to reserve endpoints
>    usb: dwc3: gadget: Add support for snps,reserved-endpoints property
>    usb: dwc3: gadget: Skip endpoints ep[18]{in,out} on Intel Merrifield
> 
>   .../devicetree/bindings/usb/snps,dwc3.yaml    | 10 +++++
>   drivers/usb/dwc3/dwc3-pci.c                   |  9 +++++
>   drivers/usb/dwc3/gadget.c                     | 38 ++++++++++++++++++-
>   3 files changed, 56 insertions(+), 1 deletion(-)
> 
Yes I retested this now on v6.13.0-rc7 Intel Merrifield and found no 
problems. Skipping the tracing end point has definitely always been 
needed on this platform. Thanks!

Tested-by: Ferry Toth <fntoth@gmail.com>
Thinh Nguyen Jan. 21, 2025, 11:43 p.m. UTC | #4
On Mon, Jan 20, 2025, Andy Shevchenko wrote:
> On Fri, Jan 17, 2025 at 03:38:46PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 16, 2025 at 11:18:45PM +0000, Thinh Nguyen wrote:
> > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> > > > On some platforms (Intel-based and AFAIK ARM-based) the EPs in the gadget
> > > > (USB Device Controller mode) may be reserved for some special means, such as
> > > > tracing. This series extends DT schema and driver to avoid using those.
> > > > Without this the USB gadget mode won't work properly (those devices that
> > > > "luckily" allocated the reserved EPs).
> > > > 
> > > > Ferry already tested the privately sent PoC of this, but I ask him again to
> > > > re-test this version which is slightly different.
> 
> ...
> 
> > > I'm not entirely clear on the reason for this change yet.
> > > 
> > > How would this even work without dwc3 managing these endpoints (all the
> > > init/teardown/fifo allocation/start/stop flow).
> > 
> > You perhaps know much better how it can be done, I have access to a limited
> > documentation and in practice if those endpoints are not skipped any gadget
> > that allocates them simply won't work, and IIRC the entire USB transfers are
> > stuck.
> > 
> > > Can you provide more info on this hardware?
> > 
> > I am afraid I can't provide more, sorry. I can look for some specifics,
> > but I'm not that guy who know anything about in-SoC tracing.
> 
> So, here is what I found:
> 
> ---8<---
> 
> However the endpoints allocated for STM and EXI debug traffic cannot be re-allocated
> if being used because the sideband flow control signals are hard wired to certain
> endpoints:
> • 1 High BW Bulk IN (IN#1) (RTIT)
> • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> 
> In device mode, since RTIT (EP#1) and EXI/RunControl (EP#8) uses External Buffer
> Control (EBC) mode, these endpoints are to be mapped to EBC mode (to be done by
> EXI target driver). Additionally TRB for RTIT and EXI are maintained in STM (System
> Trace Module) unit and the EXI target driver will as well configure the TRB location for
> EP #1 IN and EP#8 (IN and OUT). Since STM/PTI and EXI hardware blocks manage
> these endpoints and interface to OTG3 controller through EBC interface, there is no
> need to enable any events (such as XferComplete etc) for these end points.
> 
> Does it help you to understand the required quirk better?
> 

Thanks for looking up the info. This makes more sense now. So these
endpoints use EBC. Can you also provide this info to the commit?

BR,
Thinh
Thinh Nguyen Jan. 21, 2025, 11:51 p.m. UTC | #5
On Tue, Jan 21, 2025, Thinh Nguyen wrote:
> On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> > On Thu, Jan 16, 2025 at 11:39:42PM +0000, Thinh Nguyen wrote:
> > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> > 
> > ...
> > 
> > > > + * Intel Merrifield uses these endpoints for tracing and they shouldn't be used
> > > > + * for normal transfers, we need to skip them.
> > > > + * • 1 High BW Bulk IN (IN#1) (RTIT)
> > > > + * • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> > > 
> > > Please use regular bullet character and list the endpoint per line.
> > 
> > Which is...?
> > 
> > To my curiosity, what's wrong with the above?
> > 
> 
> Please use a character that we can find on the keyboard (- or * for
> example).
> 
> And why would you want to list them like this:
> 
> 	* Endpoint A
> 	* Endpoint B + Endpoint C
> 
> As oppose to:
> 
> 	* Endpoint A
> 	* Endpoint B
> 	* Endpoint C
> 

Also, please fix the $subject and replace "endpoints ep[18]{in,out}" to
just "reserved endpoints".

BR,
Thinh
Thinh Nguyen Jan. 22, 2025, 1:44 a.m. UTC | #6
On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> On Thu, Jan 16, 2025 at 11:35:19PM +0000, Thinh Nguyen wrote:
> > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> 
> ...
> 
> > >  	for (epnum = 0; epnum < total; epnum++) {
> > > -		int			ret;
> > > +		for (num = 0; num < count; num++) {
> > > +			if (epnum == eps[num])
> > > +				break;
> > > +		}
> > > +		if (num < count)
> > > +			continue;
> > 
> > You can probably rewrite this logic better.
> 
> Any suggestions?
> 
> Thanks for the review!
> 

From the first look, is the list sorted? If so, you don't need another
for-loop.

Also, we loop over the number of endpoints throughout the driver, but
you only skip it here during init. Please double check for invalid
accessing of endpoints in other places.

Perhaps set the dwc->eps[reserved_ep] to ERR_PTR(-EINVAL) or something
when you parse the reserved endpoints so you can skip them in your loop.

BR,
Thinh
Andy Shevchenko Jan. 22, 2025, 4:24 p.m. UTC | #7
On Tue, Jan 21, 2025 at 11:51:51PM +0000, Thinh Nguyen wrote:
> On Tue, Jan 21, 2025, Thinh Nguyen wrote:

...

> Also, please fix the $subject and replace "endpoints ep[18]{in,out}" to
> just "reserved endpoints".

Sure.

Thank you for the review.
Andy Shevchenko Jan. 22, 2025, 5:12 p.m. UTC | #8
On Wed, Jan 22, 2025 at 01:44:02AM +0000, Thinh Nguyen wrote:
> On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> > On Thu, Jan 16, 2025 at 11:35:19PM +0000, Thinh Nguyen wrote:
> > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:

...

> > > >  	for (epnum = 0; epnum < total; epnum++) {
> > > > -		int			ret;
> > > > +		for (num = 0; num < count; num++) {
> > > > +			if (epnum == eps[num])
> > > > +				break;
> > > > +		}
> > > > +		if (num < count)
> > > > +			continue;
> > > 
> > > You can probably rewrite this logic better.
> > 
> > Any suggestions?
> > 
> > Thanks for the review!
> 
> From the first look, is the list sorted? If so, you don't need another
> for-loop.

Even if it's sorted it's not 1:1 mapped by indices. I do not understand how we
can avoid the second loop. The only possibility is indeed to sort the list and
sparse it in accordance to the endpoint numbers, but if we are going this way,
it's much easier to switch to bitmap and the respective bitops.

> Also, we loop over the number of endpoints throughout the driver, but
> you only skip it here during init. Please double check for invalid
> accessing of endpoints in other places.
> 
> Perhaps set the dwc->eps[reserved_ep] to ERR_PTR(-EINVAL) or something
> when you parse the reserved endpoints so you can skip them in your loop.

Note, this is only for UDC, in USB host these are okay to be used.
Does your suggestion imply that?
Thinh Nguyen Jan. 28, 2025, 2:21 a.m. UTC | #9
On Wed, Jan 22, 2025, Andy Shevchenko wrote:
> On Tue, Jan 21, 2025 at 11:46:17PM +0000, Thinh Nguyen wrote:
> > On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> > > On Thu, Jan 16, 2025 at 11:39:42PM +0000, Thinh Nguyen wrote:
> > > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > + * Intel Merrifield uses these endpoints for tracing and they shouldn't be used
> > > > > + * for normal transfers, we need to skip them.
> > > > > + * • 1 High BW Bulk IN (IN#1) (RTIT)
> > > > > + * • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> > > > 
> > > > Please use regular bullet character and list the endpoint per line.
> > > 
> > > Which is...?
> > > 
> > > To my curiosity, what's wrong with the above?
> > 
> > Please use a character that we can find on the keyboard (- or * for
> > example).
> 
> Hmm... We can find all characters on keyboard by using standard approach of
> typing Unicode ones. I'm not sure why this is a problem. Linux kernel is UTF-8
> ready project (from source tree point of view), at least I haven't found any
> limitations in the documentation.
> 
> Note, this is _not_ a kernel-doc style to which you may refer when pointing out

I'm not requesting this out of any kernel-doc style. It's just a
personal preference and consistency in dwc3. If it's not too difficult,
please use "-". But if you must insist, future lists would need to be
consistent to this new unicode style. Then I would need to ask others to
use the new Unicode one. Typically typing * doesn't automatically
convert to • unless you edit using Word, and so I prefer something I and
others can easily find on the keyboard.

> to the how lists should be represented.
> 
> But it's not big deal for me to change the • character.
> 
> > And why would you want to list them like this:
> > 
> > 	* Endpoint A
> > 	* Endpoint B + Endpoint C
> 
> Because:
> 1) they are logically connected;
> 2) the above is the exact citation from the specification and I would like to
> keep it that way.
> 
> > As oppose to:
> > 
> > 	* Endpoint A
> > 	* Endpoint B
> > 	* Endpoint C
> 

If you prefer to keep the snippet of your vendor specification intact,
we can instead document this fully in the commit message and note the
EBC feature. Remove these comments here.

Thanks,
Thinh
Thinh Nguyen Jan. 28, 2025, 2:25 a.m. UTC | #10
On Wed, Jan 22, 2025, Andy Shevchenko wrote:
> On Tue, Jan 21, 2025 at 11:43:21PM +0000, Thinh Nguyen wrote:
> > On Mon, Jan 20, 2025, Andy Shevchenko wrote:
> > > On Fri, Jan 17, 2025 at 03:38:46PM +0200, Andy Shevchenko wrote:
> > > > On Thu, Jan 16, 2025 at 11:18:45PM +0000, Thinh Nguyen wrote:
> > > > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > I'm not entirely clear on the reason for this change yet.
> > > > > 
> > > > > How would this even work without dwc3 managing these endpoints (all the
> > > > > init/teardown/fifo allocation/start/stop flow).
> > > > 
> > > > You perhaps know much better how it can be done, I have access to a limited
> > > > documentation and in practice if those endpoints are not skipped any gadget
> > > > that allocates them simply won't work, and IIRC the entire USB transfers are
> > > > stuck.
> > > > 
> > > > > Can you provide more info on this hardware?
> > > > 
> > > > I am afraid I can't provide more, sorry. I can look for some specifics,
> > > > but I'm not that guy who know anything about in-SoC tracing.
> > > 
> > > So, here is what I found:
> > > 
> > > ---8<---
> > > 
> > > However the endpoints allocated for STM and EXI debug traffic cannot be re-allocated
> > > if being used because the sideband flow control signals are hard wired to certain
> > > endpoints:
> > > • 1 High BW Bulk IN (IN#1) (RTIT)
> > > • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> > > 
> > > In device mode, since RTIT (EP#1) and EXI/RunControl (EP#8) uses External Buffer
> > > Control (EBC) mode, these endpoints are to be mapped to EBC mode (to be done by
> > > EXI target driver). Additionally TRB for RTIT and EXI are maintained in STM (System
> > > Trace Module) unit and the EXI target driver will as well configure the TRB location for
> > > EP #1 IN and EP#8 (IN and OUT). Since STM/PTI and EXI hardware blocks manage
> > > these endpoints and interface to OTG3 controller through EBC interface, there is no
> > > need to enable any events (such as XferComplete etc) for these end points.
> > > 
> > > ---8<---
> > > 
> > > Does it help you to understand the required quirk better?
> > 
> > Thanks for looking up the info. This makes more sense now. So these
> > endpoints use EBC. Can you also provide this info to the commit?
> 
> Sure, since I published it already it makes no difference if it appears in the
> Git log (from the publicity point of view).
> 

It's more difficult to find this outside of git log, especially to a
link version of a git change that's not applied.

BR,
Thinh
Thinh Nguyen Jan. 28, 2025, 2:39 a.m. UTC | #11
On Wed, Jan 22, 2025, Andy Shevchenko wrote:
> On Wed, Jan 22, 2025 at 01:44:02AM +0000, Thinh Nguyen wrote:
> > On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> > > On Thu, Jan 16, 2025 at 11:35:19PM +0000, Thinh Nguyen wrote:
> > > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:
> 
> ...
> 
> > > > >  	for (epnum = 0; epnum < total; epnum++) {
> > > > > -		int			ret;
> > > > > +		for (num = 0; num < count; num++) {
> > > > > +			if (epnum == eps[num])
> > > > > +				break;
> > > > > +		}
> > > > > +		if (num < count)
> > > > > +			continue;
> > > > 
> > > > You can probably rewrite this logic better.
> > > 
> > > Any suggestions?
> > > 
> > > Thanks for the review!
> > 
> > From the first look, is the list sorted? If so, you don't need another
> > for-loop.
> 
> Even if it's sorted it's not 1:1 mapped by indices. I do not understand how we
> can avoid the second loop. The only possibility is indeed to sort the list and
> sparse it in accordance to the endpoint numbers, but if we are going this way,
> it's much easier to switch to bitmap and the respective bitops.

If it's sorted, it can be something like this. Just a quick logic and not tested:

num = 0
for (epnum = 0; epnum < total; epnum++) {
	if (num < count && epnum == eps[num]) {
		num++;
		continue;
	}

	...
}

> 
> > Also, we loop over the number of endpoints throughout the driver, but
> > you only skip it here during init. Please double check for invalid
> > accessing of endpoints in other places.
> > 
> > Perhaps set the dwc->eps[reserved_ep] to ERR_PTR(-EINVAL) or something
> > when you parse the reserved endpoints so you can skip them in your loop.
> 
> Note, this is only for UDC, in USB host these are okay to be used.
> Does your suggestion imply that?
> 

No. We track the total num_eps in dwc->num_eps. Then we do for-loop to
dwc->eps[i] and access the endpoint. Often we check if the endpoint is
NULL before accessing dwc->eps[i]. However, we don't do it everywhere.
So I ask for you to review to make sure that this change doesn't break
elsewhere where we may try to access dwc->eps[i] to an uninit endpoint
(Note I see at least 1 place e.g. dwc3_gadget_clear_tx_fifos that may
break)

BR,
Thinh
Andy Shevchenko Jan. 28, 2025, 4:50 p.m. UTC | #12
On Tue, Jan 28, 2025 at 02:21:40AM +0000, Thinh Nguyen wrote:
> On Wed, Jan 22, 2025, Andy Shevchenko wrote:
> > On Tue, Jan 21, 2025 at 11:46:17PM +0000, Thinh Nguyen wrote:
> > > On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> > > > On Thu, Jan 16, 2025 at 11:39:42PM +0000, Thinh Nguyen wrote:
> > > > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:

...

> > > > > > + * Intel Merrifield uses these endpoints for tracing and they shouldn't be used
> > > > > > + * for normal transfers, we need to skip them.
> > > > > > + * • 1 High BW Bulk IN (IN#1) (RTIT)
> > > > > > + * • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> > > > > 
> > > > > Please use regular bullet character and list the endpoint per line.
> > > > 
> > > > Which is...?
> > > > 
> > > > To my curiosity, what's wrong with the above?
> > > 
> > > Please use a character that we can find on the keyboard (- or * for
> > > example).
> > 
> > Hmm... We can find all characters on keyboard by using standard approach of
> > typing Unicode ones. I'm not sure why this is a problem. Linux kernel is UTF-8
> > ready project (from source tree point of view), at least I haven't found any
> > limitations in the documentation.
> > 
> > Note, this is _not_ a kernel-doc style to which you may refer when pointing out
> 
> I'm not requesting this out of any kernel-doc style. It's just a
> personal preference and consistency in dwc3. If it's not too difficult,
> please use "-".

As I said...

> But if you must insist, future lists would need to be
> consistent to this new unicode style. Then I would need to ask others to
> use the new Unicode one. Typically typing * doesn't automatically
> convert to • unless you edit using Word, and so I prefer something I and
> others can easily find on the keyboard.
> 
> > to the how lists should be represented.
> > 
> > But it's not big deal for me to change the • character.

...not a big deal to me, I will change as requested.

> > > And why would you want to list them like this:
> > > 
> > > 	* Endpoint A
> > > 	* Endpoint B + Endpoint C
> > 
> > Because:
> > 1) they are logically connected;
> > 2) the above is the exact citation from the specification and I would like to
> > keep it that way.
> > 
> > > As oppose to:
> > > 
> > > 	* Endpoint A
> > > 	* Endpoint B
> > > 	* Endpoint C
> 
> If you prefer to keep the snippet of your vendor specification intact,
> we can instead document this fully in the commit message and note the
> EBC feature. Remove these comments here.

I prefer to have a comment to explain magic numbers. I just want it to be
as closer as possible to the specification wording.
Andy Shevchenko Jan. 28, 2025, 4:55 p.m. UTC | #13
On Tue, Jan 28, 2025 at 02:39:50AM +0000, Thinh Nguyen wrote:
> On Wed, Jan 22, 2025, Andy Shevchenko wrote:
> > On Wed, Jan 22, 2025 at 01:44:02AM +0000, Thinh Nguyen wrote:
> > > On Fri, Jan 17, 2025, Andy Shevchenko wrote:
> > > > On Thu, Jan 16, 2025 at 11:35:19PM +0000, Thinh Nguyen wrote:
> > > > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:

...

> > > > > >  	for (epnum = 0; epnum < total; epnum++) {
> > > > > > -		int			ret;
> > > > > > +		for (num = 0; num < count; num++) {
> > > > > > +			if (epnum == eps[num])
> > > > > > +				break;
> > > > > > +		}
> > > > > > +		if (num < count)
> > > > > > +			continue;
> > > > > 
> > > > > You can probably rewrite this logic better.
> > > > 
> > > > Any suggestions?
> > > > 
> > > > Thanks for the review!
> > > 
> > > From the first look, is the list sorted? If so, you don't need another
> > > for-loop.
> > 
> > Even if it's sorted it's not 1:1 mapped by indices. I do not understand how we
> > can avoid the second loop. The only possibility is indeed to sort the list and
> > sparse it in accordance to the endpoint numbers, but if we are going this way,
> > it's much easier to switch to bitmap and the respective bitops.
> 
> If it's sorted, it can be something like this. Just a quick logic and not tested:
> 
> num = 0
> for (epnum = 0; epnum < total; epnum++) {
> 	if (num < count && epnum == eps[num]) {
> 		num++;
> 		continue;
> 	}
> 
> 	...
> }

Ah, okay, I have got the idea. Let me try to mock up something working
out of it.

> > > Also, we loop over the number of endpoints throughout the driver, but
> > > you only skip it here during init. Please double check for invalid
> > > accessing of endpoints in other places.
> > > 
> > > Perhaps set the dwc->eps[reserved_ep] to ERR_PTR(-EINVAL) or something
> > > when you parse the reserved endpoints so you can skip them in your loop.
> > 
> > Note, this is only for UDC, in USB host these are okay to be used.
> > Does your suggestion imply that?
> 
> No. We track the total num_eps in dwc->num_eps. Then we do for-loop to
> dwc->eps[i] and access the endpoint. Often we check if the endpoint is
> NULL before accessing dwc->eps[i]. However, we don't do it everywhere.
> So I ask for you to review to make sure that this change doesn't break
> elsewhere where we may try to access dwc->eps[i] to an uninit endpoint
> (Note I see at least 1 place e.g. dwc3_gadget_clear_tx_fifos that may
> break)

I see, so having my code as is also requiring to check all users of
the eps array in the _gadget part_ of the driver to see if they won't
crash due to NULL pointer dereference. Is it what you want?
If so, definitely I will revisit that.
Andy Shevchenko Jan. 28, 2025, 4:57 p.m. UTC | #14
On Tue, Jan 28, 2025 at 02:25:21AM +0000, Thinh Nguyen wrote:
> On Wed, Jan 22, 2025, Andy Shevchenko wrote:
> > On Tue, Jan 21, 2025 at 11:43:21PM +0000, Thinh Nguyen wrote:
> > > On Mon, Jan 20, 2025, Andy Shevchenko wrote:
> > > > On Fri, Jan 17, 2025 at 03:38:46PM +0200, Andy Shevchenko wrote:
> > > > > On Thu, Jan 16, 2025 at 11:18:45PM +0000, Thinh Nguyen wrote:
> > > > > > On Thu, Jan 16, 2025, Andy Shevchenko wrote:

...

> > > > > > I'm not entirely clear on the reason for this change yet.
> > > > > > 
> > > > > > How would this even work without dwc3 managing these endpoints (all the
> > > > > > init/teardown/fifo allocation/start/stop flow).
> > > > > 
> > > > > You perhaps know much better how it can be done, I have access to a limited
> > > > > documentation and in practice if those endpoints are not skipped any gadget
> > > > > that allocates them simply won't work, and IIRC the entire USB transfers are
> > > > > stuck.
> > > > > 
> > > > > > Can you provide more info on this hardware?
> > > > > 
> > > > > I am afraid I can't provide more, sorry. I can look for some specifics,
> > > > > but I'm not that guy who know anything about in-SoC tracing.
> > > > 
> > > > So, here is what I found:
> > > > 
> > > > ---8<---
> > > > 
> > > > However the endpoints allocated for STM and EXI debug traffic cannot be re-allocated
> > > > if being used because the sideband flow control signals are hard wired to certain
> > > > endpoints:
> > > > • 1 High BW Bulk IN (IN#1) (RTIT)
> > > > • 1 1KB BW Bulk IN (IN#8) + 1 1KB BW Bulk OUT (Run Control) (OUT#8)
> > > > 
> > > > In device mode, since RTIT (EP#1) and EXI/RunControl (EP#8) uses External Buffer
> > > > Control (EBC) mode, these endpoints are to be mapped to EBC mode (to be done by
> > > > EXI target driver). Additionally TRB for RTIT and EXI are maintained in STM (System
> > > > Trace Module) unit and the EXI target driver will as well configure the TRB location for
> > > > EP #1 IN and EP#8 (IN and OUT). Since STM/PTI and EXI hardware blocks manage
> > > > these endpoints and interface to OTG3 controller through EBC interface, there is no
> > > > need to enable any events (such as XferComplete etc) for these end points.
> > > > 
> > > > ---8<---
> > > > 
> > > > Does it help you to understand the required quirk better?
> > > 
> > > Thanks for looking up the info. This makes more sense now. So these
> > > endpoints use EBC. Can you also provide this info to the commit?
> > 
> > Sure, since I published it already it makes no difference if it appears in the
> > Git log (from the publicity point of view).
> 
> It's more difficult to find this outside of git log, especially to a
> link version of a git change that's not applied.

I'm not objecting this, what I am telling is that information went public in
this thread, so expanding a commit message in the next version of the series
to include additional information is fine with me. I'll do that.
Thinh Nguyen Jan. 30, 2025, 1:44 a.m. UTC | #15
On Tue, Jan 28, 2025, Andy Shevchenko wrote:
> On Tue, Jan 28, 2025 at 02:21:40AM +0000, Thinh Nguyen wrote:
> > 
> > If you prefer to keep the snippet of your vendor specification intact,
> > we can instead document this fully in the commit message and note the
> > EBC feature. Remove these comments here.
> 
> I prefer to have a comment to explain magic numbers. I just want it to be
> as closer as possible to the specification wording.
> 

Ok. That's fine.

BR,
Thinh