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

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

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))
		...

> 
> >> +				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