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)
+				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;
-				if (f->func_wakeup_armed)
-					status |= USB_INTRF_STAT_FUNC_RW;
-			}
 		}
 
 		put_unaligned_le16(status & 0x0000ffff, req->buf);
-- 
2.25.1

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.

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

BR,
Thinh

> -				if (f->func_wakeup_armed)
> -					status |= USB_INTRF_STAT_FUNC_RW;
> -			}
>  		}
>  
>  		put_unaligned_le16(status & 0x0000ffff, req->buf);
> -- 
> 2.25.1
> 

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

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

Regards,
Prashanth K

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

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.

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

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

On Wed, Apr 09, 2025, Thinh Nguyen wrote:
> On Wed, Apr 09, 2025, Prashanth K wrote:
> > 
> > 
> > On 09-04-25 06:25 am, Thinh Nguyen wrote:
> > > 
> > > 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.

Sorry typos: I mean arguably, this can cause a regression for remote
wake capable devices to perform remote wakeup in usb3 speed.

BR,
Thinh

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

On 10-04-25 03:42 am, Thinh Nguyen wrote:
> On Wed, Apr 09, 2025, Thinh Nguyen wrote:
>> On Wed, Apr 09, 2025, Prashanth K wrote:
>>>
>>>
>>> On 09-04-25 06:25 am, Thinh Nguyen wrote:
>>>>
>>>> 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.

Currently function wakeup is implemented only on f_ecm and others don't
have the capability, so the expectation is functions should add add the
get_status callbacks while implementing remote/func wakeup and mark
itself and RW/FW capable. And if get_status callback is not there, then
func is not FW capable.

Current implementation sets RW/FW capability to all interfaces if
USB_CONFIG_ATT_WAKEUP is set (which is not right). I have provided an
example in the commit text where we incorrectly set FW capability.
>>
>> Arguably, this can cause a regression for remote capable devices to
>> operate in usb3 speeds.
> 
> Sorry typos: I mean arguably, this can cause a regression for remote
> wake capable devices to perform remote wakeup in usb3 speed.
> 
> BR,
> Thinh
> 
>>
>>>
>>> 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.
>>
I seriously think we can take it out of this series and do that
separately. The intention of this series was to fix the wakeup
operations. And enable/disable func_wakeup from function driver would be
a new implementation. Ill take it up after this.

Regards,
Prashanth K