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.g36b5255a03acOn 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
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
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
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
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
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
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
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
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
© 2016 - 2026 Red Hat, Inc.