Re: [PATCH v1 3/3] usb: dwc3: gadget: Make gadget_wakeup asynchronous

[Prashanth K]

On 09-04-25 06:13 am, Thinh Nguyen wrote:
> On Tue, Apr 08, 2025, Prashanth K wrote:
>>
>>
>> On 08-04-25 05:08 am, Thinh Nguyen wrote:
>>> On Thu, Apr 03, 2025, Prashanth K wrote:
> 
>>>> @@ -4398,6 +4371,21 @@ static void dwc3_gadget_linksts_change_interrupt(struct dwc3 *dwc,
>>>>  	}
>>>>  
>>>>  	dwc->link_state = next;
>>>> +
>>>> +	/* Proceed with func wakeup if any interfaces that has requested */
>>>> +	while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
>>>> +		if (dwc->func_wakeup_pending & BIT(0)) {
>>>> +			ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
>>>> +							       DWC3_DGCMDPAR_DN_FUNC_WAKE |
>>>> +							       DWC3_DGCMDPAR_INTF_SEL(intf_id));
>>>> +			if (ret)
>>>> +				dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
>>>> +					intf_id, ret);
>>>> +		}
>>>> +		dwc->func_wakeup_pending >>= 1;
>>>
>>> This would break the bitmap of dwc->func_wakeup_pending. Perhaps we can
>>> use ffs(x) to properly find and clear the interface ID from the bitmap
>>> one at a time.
>>>
>> Yes, we can use ffs(x). But I didn't understand how this would break
>> bitmap of dwc->func_wakeup_pending.
>>
> 
> Since you're sending device notification to all the wakeup armed
> interfaces and not reusing the func_wakeup_pending afterward, the result
> of what you're doing here will be the same.
> 
> IMHO, for clarity, what I was suggesting is to revise the logic to
> preserve the dwc->func_wakeup_pending bitmap instead of shifting and
> overwriting it, causing the bitmap to no longer match the intf_id. We
> get the intf_id from the bitmap and clear the intf_id bit from the
> bitmap as we go.
> 
The above logic works, but as you suggested I'll refactor it using
ffs(x) and clear the intf_id directly (instead of shifting).

Does this look good?

/* Proceed with func wakeup if any interfaces that has requested */
while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
	intf_id = ffs(dwc->func_wakeup_pending);
	if (intf_id)
		ret = dwc3_send_gadget_generic_command(dwc, DWC3_DGCMD_DEV_NOTIFICATION,
							   DWC3_DGCMDPAR_DN_FUNC_WAKE |
							   DWC3_DGCMDPAR_INTF_SEL(intf_id - 1));
		if (ret)
			dev_err(dwc->dev, "function remote wakeup failed for %d, ret:%d\n",
				intf_id, ret);
	}
	dwc->func_wakeup_pending &= ~(1 << (intf_id - 1));
}

Regards,
Prashanth K