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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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