[PATCH v2 0/3] usb: dwc3: Avoid using reserved EPs

[PATCH v2 0/3] usb: dwc3: Avoid using reserved EPs

[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 previous versions, but I ask him again to
re-test this version which is significantly rewritten. I have not
applied given tag just to be sure we got it carefully tested again.

Changelog v2:
- added minItems to the schema (Rob)
- revisited code and add NULL check to avoid crashes (Thinh)
- rewrote helper function to return error to the user if parsing fails
- elaborated in the commit message why we need this quirk (Thinh)
- addressed miscellaneous style issues (Thinh)

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: Avoid using reserved endpoints on Intel Merrifield

 .../bindings/usb/snps,dwc3-common.yaml        | 11 ++++
 drivers/usb/dwc3/dwc3-pci.c                   | 10 ++++
 drivers/usb/dwc3/gadget.c                     | 60 +++++++++++++++++--
 3 files changed, 76 insertions(+), 5 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 1/3] dt-bindings: usb: dwc3: Add a property to reserve endpoints

[Andy Shevchenko]

Some of the endpoints may be reserved by hardware for different purposes,
e.g., tracing control and output. This is the case, for instance, on
Intel Merrifield and Moorefield platforms that reserve a few and USB driver
may not use them for the regular transfers. Add the respective bindings.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../devicetree/bindings/usb/snps,dwc3-common.yaml     | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
index c956053fd036..71249b6ba616 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
@@ -65,6 +65,17 @@ properties:
       mode.
     type: boolean
 
+  snps,reserved-endpoints:
+    description:
+      Reserve endpoints for other needs, e.g, for tracing control and output.
+      When set, the driver will avoid using them for the regular USB transfers.
+    $ref: /schemas/types.yaml#/definitions/uint8-array
+    minItems: 1
+    maxItems: 30
+    items:
+      minimum: 2
+      maximum: 31
+
   snps,dis-start-transfer-quirk:
     description:
       When set, disable isoc START TRANSFER command failure SW work-around
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 1/3] dt-bindings: usb: dwc3: Add a property to reserve endpoints

[Rob Herring (Arm)]

On Mon, 03 Feb 2025 21:10:53 +0200, Andy Shevchenko wrote:
> Some of the endpoints may be reserved by hardware for different purposes,
> e.g., tracing control and output. This is the case, for instance, on
> Intel Merrifield and Moorefield platforms that reserve a few and USB driver
> may not use them for the regular transfers. Add the respective bindings.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  .../devicetree/bindings/usb/snps,dwc3-common.yaml     | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

[PATCH v2 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Andy Shevchenko]

The snps,reserved-endpoints property lists the reserved endpoints
that shouldn't be used for normal transfers. Add support for that
to the driver.

While at it, make sure we don't crash by a sudden access to those
endpoints in the gadget driver.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/usb/dwc3/gadget.c | 60 +++++++++++++++++++++++++++++++++++----
 1 file changed, 55 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index d27af65eb08a..93b1e389a983 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -547,6 +547,7 @@ static int dwc3_gadget_set_xfer_resource(struct dwc3_ep *dep)
 int dwc3_gadget_start_config(struct dwc3 *dwc, unsigned int resource_index)
 {
 	struct dwc3_gadget_ep_cmd_params params;
+	struct dwc3_ep		*dep;
 	u32			cmd;
 	int			i;
 	int			ret;
@@ -563,8 +564,13 @@ int dwc3_gadget_start_config(struct dwc3 *dwc, unsigned int resource_index)
 		return ret;
 
 	/* Reset resource allocation flags */
-	for (i = resource_index; i < dwc->num_eps && dwc->eps[i]; i++)
-		dwc->eps[i]->flags &= ~DWC3_EP_RESOURCE_ALLOCATED;
+	for (i = resource_index; i < dwc->num_eps; i++) {
+		dep = dwc->eps[i];
+		if (!dep)
+			continue;
+
+		dep->flags &= ~DWC3_EP_RESOURCE_ALLOCATED;
+	}
 
 	return 0;
 }
@@ -751,9 +757,11 @@ void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
 
 	dwc->last_fifo_depth = fifo_depth;
 	/* Clear existing TXFIFO for all IN eps except ep0 */
-	for (num = 3; num < min_t(int, dwc->num_eps, DWC3_ENDPOINTS_NUM);
-	     num += 2) {
+	for (num = 3; num < min_t(int, dwc->num_eps, DWC3_ENDPOINTS_NUM); num += 2) {
 		dep = dwc->eps[num];
+		if (!dep)
+			continue;
+
 		/* Don't change TXFRAMNUM on usb31 version */
 		size = DWC3_IP_IS(DWC3) ? 0 :
 			dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1)) &
@@ -3395,14 +3403,52 @@ static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8 epnum)
 	return 0;
 }
 
+static int dwc3_gadget_parse_reserved_endpoints(struct dwc3 *dwc, const char *propname,
+						u8 *eps, u8 num)
+{
+	u8 count;
+	int ret;
+
+	if (!device_property_present(dwc->dev, propname))
+		return 0;
+
+	ret = device_property_count_u8(dwc->dev, propname);
+	if (ret < 0)
+		return ret;
+	count = ret;
+
+	ret = device_property_read_u8_array(dwc->dev, propname, eps, min(num, count));
+	if (ret)
+		return ret;
+
+	return count;
+}
+
 static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 total)
 {
+	const char			*propname = "snps,reserved-endpoints";
 	u8				epnum;
+	u8				eps[DWC3_ENDPOINTS_NUM];
+	u8				count;
+	u8				num;
+	int				ret;
 
 	INIT_LIST_HEAD(&dwc->gadget->ep_list);
 
+	ret = dwc3_gadget_parse_reserved_endpoints(dwc, propname, eps, ARRAY_SIZE(eps));
+	if (ret < 0) {
+		dev_err(dwc->dev, "failed to read %s\n", propname);
+		return ret;
+	}
+	count = ret;
+
 	for (epnum = 0; epnum < total; epnum++) {
-		int			ret;
+		for (num = 0; num < count; num++) {
+			if (epnum == eps[num])
+				break;
+		}
+		if (num < count)
+			continue;
 
 		ret = dwc3_gadget_init_endpoint(dwc, epnum);
 		if (ret)
@@ -3669,6 +3715,8 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
 
 		for (i = 0; i < DWC3_ENDPOINTS_NUM; i++) {
 			dep = dwc->eps[i];
+			if (!dep)
+				continue;
 
 			if (!(dep->flags & DWC3_EP_ENABLED))
 				continue;
@@ -3818,6 +3866,8 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
 	u8			epnum = event->endpoint_number;
 
 	dep = dwc->eps[epnum];
+	if (!dep)
+		return;
 
 	if (!(dep->flags & DWC3_EP_ENABLED)) {
 		if ((epnum > 1) && !(dep->flags & DWC3_EP_TRANSFER_STARTED))
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Thinh Nguyen]

On Mon, Feb 03, 2025, Andy Shevchenko wrote:
> The snps,reserved-endpoints property lists the reserved endpoints
> that shouldn't be used for normal transfers. Add support for that
> to the driver.
> 
> While at it, make sure we don't crash by a sudden access to those
> endpoints in the gadget driver.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/usb/dwc3/gadget.c | 60 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 55 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index d27af65eb08a..93b1e389a983 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -547,6 +547,7 @@ static int dwc3_gadget_set_xfer_resource(struct dwc3_ep *dep)
>  int dwc3_gadget_start_config(struct dwc3 *dwc, unsigned int resource_index)
>  {
>  	struct dwc3_gadget_ep_cmd_params params;
> +	struct dwc3_ep		*dep;
>  	u32			cmd;
>  	int			i;
>  	int			ret;
> @@ -563,8 +564,13 @@ int dwc3_gadget_start_config(struct dwc3 *dwc, unsigned int resource_index)
>  		return ret;
>  
>  	/* Reset resource allocation flags */
> -	for (i = resource_index; i < dwc->num_eps && dwc->eps[i]; i++)
> -		dwc->eps[i]->flags &= ~DWC3_EP_RESOURCE_ALLOCATED;
> +	for (i = resource_index; i < dwc->num_eps; i++) {
> +		dep = dwc->eps[i];
> +		if (!dep)
> +			continue;
> +
> +		dep->flags &= ~DWC3_EP_RESOURCE_ALLOCATED;
> +	}

Please keep code refactoring as a separate patch.

>  
>  	return 0;
>  }
> @@ -751,9 +757,11 @@ void dwc3_gadget_clear_tx_fifos(struct dwc3 *dwc)
>  
>  	dwc->last_fifo_depth = fifo_depth;
>  	/* Clear existing TXFIFO for all IN eps except ep0 */
> -	for (num = 3; num < min_t(int, dwc->num_eps, DWC3_ENDPOINTS_NUM);
> -	     num += 2) {
> +	for (num = 3; num < min_t(int, dwc->num_eps, DWC3_ENDPOINTS_NUM); num += 2) {
>  		dep = dwc->eps[num];
> +		if (!dep)
> +			continue;
> +
>  		/* Don't change TXFRAMNUM on usb31 version */
>  		size = DWC3_IP_IS(DWC3) ? 0 :
>  			dwc3_readl(dwc->regs, DWC3_GTXFIFOSIZ(num >> 1)) &
> @@ -3395,14 +3403,52 @@ static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8 epnum)
>  	return 0;
>  }
>  
> +static int dwc3_gadget_parse_reserved_endpoints(struct dwc3 *dwc, const char *propname,
> +						u8 *eps, u8 num)
> +{
> +	u8 count;
> +	int ret;
> +
> +	if (!device_property_present(dwc->dev, propname))
> +		return 0;
> +
> +	ret = device_property_count_u8(dwc->dev, propname);
> +	if (ret < 0)
> +		return ret;
> +	count = ret;
> +
> +	ret = device_property_read_u8_array(dwc->dev, propname, eps, min(num, count));

Why do min(num, count)? Can we just put the size of the eps array as
specified by the function doc.

> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +
>  static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 total)
>  {
> +	const char			*propname = "snps,reserved-endpoints";
>  	u8				epnum;
> +	u8				eps[DWC3_ENDPOINTS_NUM];

Can we rename eps to reserved_eps.

> +	u8				count;
> +	u8				num;
> +	int				ret;
>  
>  	INIT_LIST_HEAD(&dwc->gadget->ep_list);
>  
> +	ret = dwc3_gadget_parse_reserved_endpoints(dwc, propname, eps, ARRAY_SIZE(eps));

Base on the name of this function, I would expect the return value
to be a status and not a count. Since we are not really parsing but
getting the property array. Can we rename this to
dwc3_gadget_get_reserved_endpoints()?

> +	if (ret < 0) {
> +		dev_err(dwc->dev, "failed to read %s\n", propname);
> +		return ret;
> +	}
> +	count = ret;
> +
>  	for (epnum = 0; epnum < total; epnum++) {
> -		int			ret;
> +		for (num = 0; num < count; num++) {
> +			if (epnum == eps[num])
> +				break;
> +		}
> +		if (num < count)
> +			continue;
>  
>  		ret = dwc3_gadget_init_endpoint(dwc, epnum);
>  		if (ret)
> @@ -3669,6 +3715,8 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
>  
>  		for (i = 0; i < DWC3_ENDPOINTS_NUM; i++) {
>  			dep = dwc->eps[i];
> +			if (!dep)
> +				continue;

It should be fine to ignore this check here. Something must be really
wrong if there's an interrupt pointing to an endpoint that we shouldn't
be touching. If we do add a check, we should print a warn or something
here. But that should be a patch separate from this.

>  
>  			if (!(dep->flags & DWC3_EP_ENABLED))
>  				continue;
> @@ -3818,6 +3866,8 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>  	u8			epnum = event->endpoint_number;
>  
>  	dep = dwc->eps[epnum];
> +	if (!dep)
> +		return;

Same here.

Looks like the only NULL check needed is in
dwc3_gadget_clear_tx_fifos().

>  
>  	if (!(dep->flags & DWC3_EP_ENABLED)) {
>  		if ((epnum > 1) && !(dep->flags & DWC3_EP_TRANSFER_STARTED))
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac
> 


Thanks,
Thinh

Re: [PATCH v2 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Thinh Nguyen]

On Wed, Feb 12, 2025, Thinh Nguyen wrote:
> On Mon, Feb 03, 2025, Andy Shevchenko wrote:
> >  
> > +static int dwc3_gadget_parse_reserved_endpoints(struct dwc3 *dwc, const char *propname,
> > +						u8 *eps, u8 num)
> > +{
> > +	u8 count;
> > +	int ret;
> > +
> > +	if (!device_property_present(dwc->dev, propname))
> > +		return 0;
> > +
> > +	ret = device_property_count_u8(dwc->dev, propname);
> > +	if (ret < 0)
> > +		return ret;
> > +	count = ret;
> > +
> > +	ret = device_property_read_u8_array(dwc->dev, propname, eps, min(num, count));
> 
> Why do min(num, count)? Can we just put the size of the eps array as
> specified by the function doc.
> 

Actually ignore this. What you have here is fine.

Thanks,
Thinh

Re: [PATCH v2 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Andy Shevchenko]

On Wed, Feb 12, 2025 at 01:10:17AM +0000, Thinh Nguyen wrote:
> On Mon, Feb 03, 2025, Andy Shevchenko wrote:
> > The snps,reserved-endpoints property lists the reserved endpoints
> > that shouldn't be used for normal transfers. Add support for that
> > to the driver.

> > While at it, make sure we don't crash by a sudden access to those
> > endpoints in the gadget driver.

^^^ (1)

...

> >  	/* Reset resource allocation flags */
> > -	for (i = resource_index; i < dwc->num_eps && dwc->eps[i]; i++)
> > -		dwc->eps[i]->flags &= ~DWC3_EP_RESOURCE_ALLOCATED;
> > +	for (i = resource_index; i < dwc->num_eps; i++) {
> > +		dep = dwc->eps[i];
> > +		if (!dep)
> > +			continue;
> > +
> > +		dep->flags &= ~DWC3_EP_RESOURCE_ALLOCATED;
> > +	}
> 
> Please keep code refactoring as a separate patch.

It's induced by the change you asked for, it's not a simple refactoring.

Or do you want me to have the patch to check eps against NULL to be separated
from this one (see (1) above)?

> >  
> >  	return 0;

...

> > +static int dwc3_gadget_parse_reserved_endpoints(struct dwc3 *dwc, const char *propname,
> > +						u8 *eps, u8 num)
> > +{
> > +	u8 count;
> > +	int ret;
> > +
> > +	if (!device_property_present(dwc->dev, propname))
> > +		return 0;
> > +
> > +	ret = device_property_count_u8(dwc->dev, propname);
> > +	if (ret < 0)
> > +		return ret;
> > +	count = ret;
> > +
> > +	ret = device_property_read_u8_array(dwc->dev, propname, eps, min(num, count));
> 
> Why do min(num, count)? Can we just put the size of the eps array as
> specified by the function doc.

No, we can't ask more than there is. The call will fail in such a case.
In case you wonder, the similar OF call also behaves in the same way.

> > +	if (ret)
> > +		return ret;
> > +
> > +	return count;
> > +}

...

> >  static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 total)
> >  {
> > +	const char			*propname = "snps,reserved-endpoints";
> >  	u8				epnum;
> > +	u8				eps[DWC3_ENDPOINTS_NUM];
> 
> Can we rename eps to reserved_eps.

Sure.

> > +	u8				count;
> > +	u8				num;
> > +	int				ret;
> >  
> >  	INIT_LIST_HEAD(&dwc->gadget->ep_list);
> >  
> > +	ret = dwc3_gadget_parse_reserved_endpoints(dwc, propname, eps, ARRAY_SIZE(eps));
> 
> Base on the name of this function, I would expect the return value
> to be a status and not a count. Since we are not really parsing but
> getting the property array. Can we rename this to
> dwc3_gadget_get_reserved_endpoints()?

Sure.

> > +	if (ret < 0) {
> > +		dev_err(dwc->dev, "failed to read %s\n", propname);
> > +		return ret;
> > +	}
> > +	count = ret;

...

> > static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,

> >  		for (i = 0; i < DWC3_ENDPOINTS_NUM; i++) {
> >  			dep = dwc->eps[i];
> > +			if (!dep)
> > +				continue;
> 
> It should be fine to ignore this check here. Something must be really
> wrong if there's an interrupt pointing to an endpoint that we shouldn't
> be touching. If we do add a check, we should print a warn or something
> here. But that should be a patch separate from this.

Theoretically everything is possible as it may be HW integration bug,
for example. But are you asking about separate patch even from the rest
of the checks? Please, elaborate what do you want to see.

...

> > static void dwc3_endpoint_interrupt(struct dwc3 *dwc,

> >  	dep = dwc->eps[epnum];
> > +	if (!dep)
> > +		return;
> 
> Same here.
> 
> Looks like the only NULL check needed is in
> dwc3_gadget_clear_tx_fifos().

Seems more, see above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Andy Shevchenko]

On Wed, Feb 12, 2025 at 12:36:04PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 12, 2025 at 01:10:17AM +0000, Thinh Nguyen wrote:
> > On Mon, Feb 03, 2025, Andy Shevchenko wrote:

...

> > > static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
> 
> > >  		for (i = 0; i < DWC3_ENDPOINTS_NUM; i++) {
> > >  			dep = dwc->eps[i];
> > > +			if (!dep)
> > > +				continue;
> > 
> > It should be fine to ignore this check here. Something must be really
> > wrong if there's an interrupt pointing to an endpoint that we shouldn't
> > be touching. If we do add a check, we should print a warn or something
> > here. But that should be a patch separate from this.
> 
> Theoretically everything is possible as it may be HW integration bug,
> for example. But are you asking about separate patch even from the rest
> of the checks? Please, elaborate what do you want to see.

Re-reading the code again, I don't understand. If we get to this loop
ever (theoretically it might be an old IP with the reserved endpoints),
we crash the kernel on the first gap in the array. And since the function
is called on an endpoint, it doesn't mean that all endpoints are allocated,
so I do not see the justification to issue a warning here.
Or do you imply that DWC3_VER_IS_PRIOR(DWC3, 183A) may not have an HW
integration similar to what we have on Intel Merrifield?

For now I'm going to leave this check as is.

...

> > > static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
> 
> > >  	dep = dwc->eps[epnum];
> > > +	if (!dep)
> > > +		return;
> > 
> > Same here.

Here I agree and I will add a warning message. Indeed, if we get and interrupt
for undefined endpoint, something is not correct.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Thinh Nguyen]

On Wed, Feb 12, 2025, Andy Shevchenko wrote:
> On Wed, Feb 12, 2025 at 12:36:04PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 12, 2025 at 01:10:17AM +0000, Thinh Nguyen wrote:
> > > On Mon, Feb 03, 2025, Andy Shevchenko wrote:
> 
> ...
> 
> > > > static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
> > 
> > > >  		for (i = 0; i < DWC3_ENDPOINTS_NUM; i++) {
> > > >  			dep = dwc->eps[i];
> > > > +			if (!dep)
> > > > +				continue;
> > > 
> > > It should be fine to ignore this check here. Something must be really
> > > wrong if there's an interrupt pointing to an endpoint that we shouldn't
> > > be touching. If we do add a check, we should print a warn or something
> > > here. But that should be a patch separate from this.
> > 
> > Theoretically everything is possible as it may be HW integration bug,
> > for example. But are you asking about separate patch even from the rest
> > of the checks? Please, elaborate what do you want to see.
> 
> Re-reading the code again, I don't understand. If we get to this loop
> ever (theoretically it might be an old IP with the reserved endpoints),
> we crash the kernel on the first gap in the array. And since the function
> is called on an endpoint, it doesn't mean that all endpoints are allocated,
> so I do not see the justification to issue a warning here.
> Or do you imply that DWC3_VER_IS_PRIOR(DWC3, 183A) may not have an HW
> integration similar to what we have on Intel Merrifield?
> 
> For now I'm going to leave this check as is.

Oops, you are correct. I read this as the same logic as below.

> 
> ...
> 
> > > > static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
> > 
> > > >  	dep = dwc->eps[epnum];
> > > > +	if (!dep)
> > > > +		return;
> > > 
> > > Same here.
> 
> Here I agree and I will add a warning message. Indeed, if we get and interrupt
> for undefined endpoint, something is not correct.
> 

BR,
Thinh

Re: [PATCH v2 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Andy Shevchenko]

On Thu, Feb 13, 2025 at 01:17:41AM +0000, Thinh Nguyen wrote:
> On Wed, Feb 12, 2025, Andy Shevchenko wrote:
> > On Wed, Feb 12, 2025 at 12:36:04PM +0200, Andy Shevchenko wrote:
> > > On Wed, Feb 12, 2025 at 01:10:17AM +0000, Thinh Nguyen wrote:
> > > > On Mon, Feb 03, 2025, Andy Shevchenko wrote:

...

> > > > > static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
> > > 
> > > > >  		for (i = 0; i < DWC3_ENDPOINTS_NUM; i++) {
> > > > >  			dep = dwc->eps[i];
> > > > > +			if (!dep)
> > > > > +				continue;
> > > > 
> > > > It should be fine to ignore this check here. Something must be really
> > > > wrong if there's an interrupt pointing to an endpoint that we shouldn't
> > > > be touching. If we do add a check, we should print a warn or something
> > > > here. But that should be a patch separate from this.
> > > 
> > > Theoretically everything is possible as it may be HW integration bug,
> > > for example. But are you asking about separate patch even from the rest
> > > of the checks? Please, elaborate what do you want to see.
> > 
> > Re-reading the code again, I don't understand. If we get to this loop
> > ever (theoretically it might be an old IP with the reserved endpoints),
> > we crash the kernel on the first gap in the array. And since the function
> > is called on an endpoint, it doesn't mean that all endpoints are allocated,
> > so I do not see the justification to issue a warning here.
> > Or do you imply that DWC3_VER_IS_PRIOR(DWC3, 183A) may not have an HW
> > integration similar to what we have on Intel Merrifield?
> > 
> > For now I'm going to leave this check as is.
> 
> Oops, you are correct. I read this as the same logic as below.

NP. Thank you for the review, and thanks for acking the next version!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Thinh Nguyen]

On Wed, Feb 12, 2025, Andy Shevchenko wrote:
> On Wed, Feb 12, 2025 at 01:10:17AM +0000, Thinh Nguyen wrote:
> > On Mon, Feb 03, 2025, Andy Shevchenko wrote:
> > > The snps,reserved-endpoints property lists the reserved endpoints
> > > that shouldn't be used for normal transfers. Add support for that
> > > to the driver.
> 
> > > While at it, make sure we don't crash by a sudden access to those
> > > endpoints in the gadget driver.
> 
> ^^^ (1)
> 
> ...
> 
> > >  	/* Reset resource allocation flags */
> > > -	for (i = resource_index; i < dwc->num_eps && dwc->eps[i]; i++)
> > > -		dwc->eps[i]->flags &= ~DWC3_EP_RESOURCE_ALLOCATED;
> > > +	for (i = resource_index; i < dwc->num_eps; i++) {
> > > +		dep = dwc->eps[i];
> > > +		if (!dep)
> > > +			continue;
> > > +
> > > +		dep->flags &= ~DWC3_EP_RESOURCE_ALLOCATED;
> > > +	}
> > 
> > Please keep code refactoring as a separate patch.
> 
> It's induced by the change you asked for, it's not a simple refactoring.
> 
> Or do you want me to have the patch to check eps against NULL to be separated
> from this one (see (1) above)?


The condition "i < dwc->num && dwc->eps[i]" already does the NULL check.
The change here only move things around.

> 
> > >  
> > >  	return 0;
> 
> ...
> 
> > > +static int dwc3_gadget_parse_reserved_endpoints(struct dwc3 *dwc, const char *propname,
> > > +						u8 *eps, u8 num)
> > > +{
> > > +	u8 count;
> > > +	int ret;
> > > +
> > > +	if (!device_property_present(dwc->dev, propname))
> > > +		return 0;
> > > +
> > > +	ret = device_property_count_u8(dwc->dev, propname);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	count = ret;
> > > +
> > > +	ret = device_property_read_u8_array(dwc->dev, propname, eps, min(num, count));
> > 
> > Why do min(num, count)? Can we just put the size of the eps array as
> > specified by the function doc.
> 
> No, we can't ask more than there is. The call will fail in such a case.
> In case you wonder, the similar OF call also behaves in the same way.

Yeah, I realized that right after I wrote the comment and responded
after.

BR,
Thinh

Re: [PATCH v2 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Thinh Nguyen]

On Thu, Feb 13, 2025, Thinh Nguyen wrote:
> On Wed, Feb 12, 2025, Andy Shevchenko wrote:
> > On Wed, Feb 12, 2025 at 01:10:17AM +0000, Thinh Nguyen wrote:
> > > On Mon, Feb 03, 2025, Andy Shevchenko wrote:
> > > > The snps,reserved-endpoints property lists the reserved endpoints
> > > > that shouldn't be used for normal transfers. Add support for that
> > > > to the driver.
> > 
> > > > While at it, make sure we don't crash by a sudden access to those
> > > > endpoints in the gadget driver.
> > 
> > ^^^ (1)
> > 
> > ...
> > 
> > > >  	/* Reset resource allocation flags */
> > > > -	for (i = resource_index; i < dwc->num_eps && dwc->eps[i]; i++)
> > > > -		dwc->eps[i]->flags &= ~DWC3_EP_RESOURCE_ALLOCATED;
> > > > +	for (i = resource_index; i < dwc->num_eps; i++) {
> > > > +		dep = dwc->eps[i];
> > > > +		if (!dep)
> > > > +			continue;
> > > > +
> > > > +		dep->flags &= ~DWC3_EP_RESOURCE_ALLOCATED;
> > > > +	}
> > > 
> > > Please keep code refactoring as a separate patch.
> > 
> > It's induced by the change you asked for, it's not a simple refactoring.
> > 
> > Or do you want me to have the patch to check eps against NULL to be separated
> > from this one (see (1) above)?
> 
> 
> The condition "i < dwc->num && dwc->eps[i]" already does the NULL check.
> The change here only move things around.
> 

Ah... my brain is fried. You're right. This change is needed.

Thanks,
Thinh

Re: [PATCH v2 2/3] usb: dwc3: gadget: Add support for snps,reserved-endpoints property

[Andy Shevchenko]

On Thu, Feb 13, 2025 at 01:16:15AM +0000, Thinh Nguyen wrote:
> On Wed, Feb 12, 2025, Andy Shevchenko wrote:
> > On Wed, Feb 12, 2025 at 01:10:17AM +0000, Thinh Nguyen wrote:
> > > On Mon, Feb 03, 2025, Andy Shevchenko wrote:

...

> > > >  	/* Reset resource allocation flags */
> > > > -	for (i = resource_index; i < dwc->num_eps && dwc->eps[i]; i++)
> > > > -		dwc->eps[i]->flags &= ~DWC3_EP_RESOURCE_ALLOCATED;
> > > > +	for (i = resource_index; i < dwc->num_eps; i++) {
> > > > +		dep = dwc->eps[i];
> > > > +		if (!dep)
> > > > +			continue;
> > > > +
> > > > +		dep->flags &= ~DWC3_EP_RESOURCE_ALLOCATED;
> > > > +	}
> > > 
> > > Please keep code refactoring as a separate patch.
> > 
> > It's induced by the change you asked for, it's not a simple refactoring.
> > 
> > Or do you want me to have the patch to check eps against NULL to be separated
> > from this one (see (1) above)?
> 
> The condition "i < dwc->num && dwc->eps[i]" already does the NULL check.
> The change here only move things around.

Yes, but the problem is that the loop will stop on the first gap, but we would
like to continue.

> > > >  	return 0;

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/3] usb: dwc3: gadget: Avoid using reserved endpoints on Intel Merrifield

[Andy Shevchenko]

Intel Merrifield SoC uses these endpoints for tracing and they cannot
be re-allocated if being used because the side band 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.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/usb/dwc3/dwc3-pci.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 052852f80146..54a4ee2b90b7 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -148,11 +148,21 @@ static const struct property_entry dwc3_pci_intel_byt_properties[] = {
 	{}
 };
 
+/*
+ * Intel Merrifield SoC uses these endpoints for tracing and they cannot
+ * be re-allocated if being used because the side band 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)
+ */
+static const u8 dwc3_pci_mrfld_reserved_endpoints[] = { 3, 16, 17 };
+
 static const struct property_entry dwc3_pci_mrfld_properties[] = {
 	PROPERTY_ENTRY_STRING("dr_mode", "otg"),
 	PROPERTY_ENTRY_STRING("linux,extcon-name", "mrfld_bcove_pwrsrc"),
 	PROPERTY_ENTRY_BOOL("snps,dis_u3_susphy_quirk"),
 	PROPERTY_ENTRY_BOOL("snps,dis_u2_susphy_quirk"),
+	PROPERTY_ENTRY_U8_ARRAY("snps,reserved-endpoints", dwc3_pci_mrfld_reserved_endpoints),
 	PROPERTY_ENTRY_BOOL("snps,usb2-gadget-lpm-disable"),
 	PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
 	{}
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 3/3] usb: dwc3: gadget: Avoid using reserved endpoints on Intel Merrifield

[Thinh Nguyen]

On Mon, Feb 03, 2025, Andy Shevchenko wrote:
> Intel Merrifield SoC uses these endpoints for tracing and they cannot
> be re-allocated if being used because the side band 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.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/usb/dwc3/dwc3-pci.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> index 052852f80146..54a4ee2b90b7 100644
> --- a/drivers/usb/dwc3/dwc3-pci.c
> +++ b/drivers/usb/dwc3/dwc3-pci.c
> @@ -148,11 +148,21 @@ static const struct property_entry dwc3_pci_intel_byt_properties[] = {
>  	{}
>  };
>  
> +/*
> + * Intel Merrifield SoC uses these endpoints for tracing and they cannot
> + * be re-allocated if being used because the side band 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)
> + */
> +static const u8 dwc3_pci_mrfld_reserved_endpoints[] = { 3, 16, 17 };
> +
>  static const struct property_entry dwc3_pci_mrfld_properties[] = {
>  	PROPERTY_ENTRY_STRING("dr_mode", "otg"),
>  	PROPERTY_ENTRY_STRING("linux,extcon-name", "mrfld_bcove_pwrsrc"),
>  	PROPERTY_ENTRY_BOOL("snps,dis_u3_susphy_quirk"),
>  	PROPERTY_ENTRY_BOOL("snps,dis_u2_susphy_quirk"),
> +	PROPERTY_ENTRY_U8_ARRAY("snps,reserved-endpoints", dwc3_pci_mrfld_reserved_endpoints),
>  	PROPERTY_ENTRY_BOOL("snps,usb2-gadget-lpm-disable"),
>  	PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
>  	{}
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

Re: [PATCH v2 0/3] usb: dwc3: Avoid using reserved EPs

[Ferry Toth]

Op 03-02-2025 om 20:10 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 previous versions, but I ask him again to
> re-test this version which is significantly rewritten. I have not
> applied given tag just to be sure we got it carefully tested again.
> 
> Changelog v2:
> - added minItems to the schema (Rob)
> - revisited code and add NULL check to avoid crashes (Thinh)
> - rewrote helper function to return error to the user if parsing fails
> - elaborated in the commit message why we need this quirk (Thinh)
> - addressed miscellaneous style issues (Thinh)
> 
> 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: Avoid using reserved endpoints on Intel Merrifield
> 
>   .../bindings/usb/snps,dwc3-common.yaml        | 11 ++++
>   drivers/usb/dwc3/dwc3-pci.c                   | 10 ++++
>   drivers/usb/dwc3/gadget.c                     | 60 +++++++++++++++++--
>   3 files changed, 76 insertions(+), 5 deletions(-)
> 
Retested this on v6.13.0 Intel Merrifield and found no problems due to 
this patch. With simultaneous iperf3 on cdc eem, and a disk bench mark 
on mass storage, it is possible to overload the gadgets causing no or 
delayed responses (delayed until killing iperf3) on gser, eventually 
causing the host side to need a reboot. So, nothing new there :-)

Thanks!

Tested-by: Ferry Toth <fntoth@gmail.com>