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

[Prashanth K <prashanth.k@oss.qualcomm.com>]

On 10-04-25 03:21 am, Thinh Nguyen wrote:
> On Wed, Apr 09, 2025, Prashanth K wrote:
>>
>>
>> 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?
> 
> It looks great!
> 
>>
>> /* 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));
> 
> 
> Some minor revision. How does this look? (not tested)
> 
> while (dwc->func_wakeup_pending && (next == DWC3_LINK_STATE_U0)) {
> 	intf_id = ffs(dwc->func_wakeup_pending) - 1;
> 	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, "Failed to send DN wake for intf %d\n", intf_id);
> 
> 	dwc->func_wakeup_pending &= ~BIT(intf_id);
> }
> 
> Thanks,
> Thinh

Good suggestion, ideally it should work, I'll test and add it in v2.

Thanks,
Prashanth K