The snps,reserved-endpoints property lists the reserved endpoints
that shouldn't be used for normal transfers. Add support for that
to the driver.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/usb/dwc3/gadget.c | 38 +++++++++++++++++++++++++++++++++++++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 31a654c6f15b..3f806fb8b61c 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3349,14 +3349,50 @@ static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8 epnum)
return 0;
}
+static int dwc3_gadget_parse_reserved_endpoints(struct dwc3 *dwc, u8 *eps, size_t count)
+{
+ const char *propname = "snps,reserved-endpoints";
+ int ret;
+
+ ret = device_property_count_u8(dwc->dev, propname);
+ if (ret < 0)
+ return 0;
+ if (ret == 0)
+ return 0;
+ if (ret > count) {
+ dev_err(dwc->dev, "too many entries in %s\n", propname);
+ return -EINVAL;
+ }
+
+ count = ret;
+ ret = device_property_read_u8_array(dwc->dev, propname, eps, count);
+ if (ret)
+ dev_err(dwc->dev, "failed to read %s\n", propname);
+
+ return ret;
+}
+
static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 total)
{
u8 epnum;
+ u8 eps[DWC3_ENDPOINTS_NUM];
+ u8 count, num;
+ int ret;
INIT_LIST_HEAD(&dwc->gadget->ep_list);
+ ret = dwc3_gadget_parse_reserved_endpoints(dwc, eps, ARRAY_SIZE(eps));
+ if (ret < 0)
+ 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)
--
2.43.0.rc1.1336.g36b5255a03acOn Thu, Jan 16, 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.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/usb/dwc3/gadget.c | 38 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 31a654c6f15b..3f806fb8b61c 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3349,14 +3349,50 @@ static int dwc3_gadget_init_endpoint(struct dwc3 *dwc, u8 epnum)
> return 0;
> }
>
> +static int dwc3_gadget_parse_reserved_endpoints(struct dwc3 *dwc, u8 *eps, size_t count)
> +{
> + const char *propname = "snps,reserved-endpoints";
> + int ret;
> +
> + ret = device_property_count_u8(dwc->dev, propname);
> + if (ret < 0)
> + return 0;
> + if (ret == 0)
> + return 0;
Just use if (ret <= 0) return 0.
> + if (ret > count) {
> + dev_err(dwc->dev, "too many entries in %s\n", propname);
> + return -EINVAL;
> + }
> +
> + count = ret;
> + ret = device_property_read_u8_array(dwc->dev, propname, eps, count);
> + if (ret)
> + dev_err(dwc->dev, "failed to read %s\n", propname);
> +
> + return ret;
> +}
> +
> static int dwc3_gadget_init_endpoints(struct dwc3 *dwc, u8 total)
> {
> u8 epnum;
> + u8 eps[DWC3_ENDPOINTS_NUM];
Please keep consistent alignment.
> + u8 count, num;
Please keep declaration in separate lines here.
> + int ret;
>
> INIT_LIST_HEAD(&dwc->gadget->ep_list);
>
> + ret = dwc3_gadget_parse_reserved_endpoints(dwc, eps, ARRAY_SIZE(eps));
> + if (ret < 0)
> + 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;
You can probably rewrite this logic better.
>
> ret = dwc3_gadget_init_endpoint(dwc, epnum);
> if (ret)
> --
> 2.43.0.rc1.1336.g36b5255a03ac
>
BR,
Thinh
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!
--
With Best Regards,
Andy Shevchenko
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
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?
--
With Best Regards,
Andy Shevchenko
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
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.
--
With Best Regards,
Andy Shevchenko
On Tue, Jan 28, 2025, Andy Shevchenko wrote: > 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: > > > > 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. > Yes, help double check that. Thanks, Thinh
© 2016 - 2025 Red Hat, Inc.