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
