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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Wed, Apr 09, 2025, Prashanth K wrote:
> 
> 
> 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@kernel.org
> >>>> Fixes: 481c225c4802 ("usb: gadget: Handle function suspend feature selector")
> >>>> Signed-off-by: Prashanth K <prashanth.k@oss.qualcomm.com>
> >>>> ---
> >>>>  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.

The way you have it done now forces a usb3x function driver to implement
f->get_status to be able to respond with function wakeup capable.
Previously, we automatically enable function wakeup capability for all
functions if the USB_CONFIG_ATT_WAKEUP is set.

Arguably, this can cause a regression for remote capable devices to
operate in usb3 speeds.

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

That would be great! This would fit this series perfectly. Let me know
if there's an issue.

Thanks,
Thinh