Re: [PATCH v1 2/3] usb: gadget: Use get_status callback to set remote wakeup capability

[Prashanth K]

On 09-04-25 06:25 am, Thinh Nguyen wrote:
> On Tue, Apr 08, 2025, Prashanth K wrote:
>>
>>
>> On 08-04-25 06:48 am, Thinh Nguyen wrote:
>>> On Thu, Apr 03, 2025, Prashanth K wrote:
>>>> Currently when the host sends GET_STATUS request for an interface,
>>>> we use get_status callbacks to set/clear remote wakeup capability
>>>> of that interface. And if get_status callback isn't present for
>>>> that interface, then we assume its remote wakeup capability based
>>>> on bmAttributes.
>>>>
>>>> Now consider a scenario, where we have a USB configuration with
>>>> multiple interfaces (say ECM + ADB), here ECM is remote wakeup
>>>> capable and as of now ADB isn't. And bmAttributes will indicate
>>>> the device as wakeup capable. With the current implementation,
>>>> when host sends GET_STATUS request for both interfaces, we will
>>>> set FUNC_RW_CAP for both. This results in USB3 CV Chapter 9.15
>>>> (Function Remote Wakeup Test) failures as host expects remote
>>>> wakeup from both interfaces.
>>>>
>>>> The above scenario is just an example, and the failure can be
>>>> observed if we use configuration with any interface except ECM.
>>>> Hence avoid configuring remote wakeup capability from composite
>>>> driver based on bmAttributes, instead use get_status callbacks
>>>> and let the function drivers decide this.
>>>>
>>>> Cc: stable@xxxxxxxxxx
>>>> Fixes: 481c225c4802 ("usb: gadget: Handle function suspend feature selector")
>>>> Signed-off-by: Prashanth K <prashanth.k@xxxxxxxxxxxxxxxx>
>>>> ---
>>>>  drivers/usb/gadget/composite.c | 12 ++++--------
>>>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>>>> index 869ad99afb48..5c6da360e95b 100644
>>>> --- a/drivers/usb/gadget/composite.c
>>>> +++ b/drivers/usb/gadget/composite.c
>>>> @@ -2010,16 +2010,12 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>>>>  			break;
>>>>  
>>>>  		if (f->get_status) {
>>>> -			status = f->get_status(f);
>>>> +			/* if D5 is not set, then device is not wakeup capable */
>>>> +			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP)
>>>
>>> We should allow function to execute get_status regardless of
>>> USB_CONFIG_ATT_WAKEUP. There are other status beside wakeup.
>>>
>> Agree with the first part, I also wanted to remove the explicit check
>> for USB_CONFIG_ATT_WAKEUP. But anyways kept it since only 2 bits (RW_CAP
>> and RW) are defined in the spec as the status of GetStatus for an Interface.
>>
>> Lets do one thing, I'll rearrange it as follows
>>
>> if (f->get_status) {
>> 	status = f->get_status(f);
>> 	
>> /* if D5 is not set, then device is not wakeup capable */
>> if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP)
>> 	status &= ~(USB_INTRF_STAT_FUNC_RW_CAP | USB_INTRF_STAT_FUNC_RW);
> 
> Yes, something like this works, but I think you mean this:
> 
> 	if (!(f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP))
> 		...
> 
Yes right, will update it in next version.
>>
>>>> +				status = f->get_status(f);
>>>> +
>>>>  			if (status < 0)
>>>>  				break;
>>>> -		} else {
>>>> -			/* Set D0 and D1 bits based on func wakeup capability */
>>>> -			if (f->config->bmAttributes & USB_CONFIG_ATT_WAKEUP) {
>>>> -				status |= USB_INTRF_STAT_FUNC_RW_CAP;
>>>
>>>
>>> So right now we're not able to configure the function to enable RW
>>> capable? Perhaps we need to update the composite configfs for this?
>>>
>>
>> The removed code used to set USB_INTRF_STAT_FUNC_RW_CAP even for
>> interfaces which doesn't have RW capability. Its better to handle this
>> from function level than from composite.
>>
> 
> Not at the gadget level, I mean to create a configfs attribute common
> across different functions to allow the user to enable/disable the
> function wakeup capability via the configfs when you setup the function.
> 
> What do you think?
> 
> Thanks,
> Thinh

Thats a good idea, in fact I had the same thought. But thought of doing
it later since its beyond the scope of this patch/series.

We can add a configfs attribute to enable/disable FUNC_RW_CAP, and
functions can return get_status() based on the attribute.

Regards,
Prashanth K