On Wed, Feb 12, 2025, Andy Shevchenko wrote:

On Wed, Feb 12, 2025 at 12:36:04PM +0200, Andy Shevchenko wrote:

quoted

On Wed, Feb 12, 2025 at 01:10:17AM +0000, Thinh Nguyen wrote:

quoted

On Mon, Feb 03, 2025, Andy Shevchenko wrote:

...

quoted

quoted

quoted

static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,

quoted

quoted

 		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.

...

quoted

quoted

quoted

static void dwc3_endpoint_interrupt(struct dwc3 *dwc,

quoted

quoted

 	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