Message-ID: <510cd700-7d2e-2bb6-7b76-4ce8305618ae@quicinc.com>
Date:   Wed, 15 Mar 2023 15:41:05 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v10 5/5] usb: gadget: f_ecm: Add suspend/resume and remote
 wakeup support
Content-Language: en-US
To:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>
CC:     "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "balbi@kernel.org" <balbi@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "quic_wcheng@quicinc.com" <quic_wcheng@quicinc.com>,
        "quic_jackp@quicinc.com" <quic_jackp@quicinc.com>
References: <1678908551-27666-1-git-send-email-quic_eserrao@quicinc.com>
 <1678908551-27666-6-git-send-email-quic_eserrao@quicinc.com>
 <20230315194733.yjp5ddymehxm6abl@synopsys.com>
 <20230315211542.z6oc2zfgpnrixgi5@synopsys.com>
From:   Elson Serrao <quic_eserrao@quicinc.com>
In-Reply-To: <20230315211542.z6oc2zfgpnrixgi5@synopsys.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 3/15/2023 2:15 PM, Thinh Nguyen wrote:
> On Wed, Mar 15, 2023, Thinh Nguyen wrote:
>> On Wed, Mar 15, 2023, Elson Roy Serrao wrote:
>>> When host sends a suspend notification to the device, handle
>>> the suspend callbacks in the function driver. Enhanced super
>>> speed devices can support function suspend feature to put the
>>> function in suspend state. Handle function suspend callback.
>>>
>>> Depending on the remote wakeup capability the device can either
>>> trigger a remote wakeup or wait for the host initiated resume to
>>> start data transfer again.
>>>
>>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>>> ---
>>>   drivers/usb/gadget/function/f_ecm.c   | 77 +++++++++++++++++++++++++++++++++++
>>>   drivers/usb/gadget/function/u_ether.c | 63 ++++++++++++++++++++++++++++
>>>   drivers/usb/gadget/function/u_ether.h |  4 ++
>>>   3 files changed, 144 insertions(+)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
>>> index a7ab30e..fea07ab 100644
>>> --- a/drivers/usb/gadget/function/f_ecm.c
>>> +++ b/drivers/usb/gadget/function/f_ecm.c
>>> @@ -633,6 +633,8 @@ static void ecm_disable(struct usb_function *f)
>>>   
>>>   	usb_ep_disable(ecm->notify);
>>>   	ecm->notify->desc = NULL;
>>> +	f->func_suspended = false;
>>> +	f->func_wakeup_armed = false;
>>>   }
>>>   
>>>   /*-------------------------------------------------------------------------*/
>>> @@ -885,6 +887,77 @@ static struct usb_function_instance *ecm_alloc_inst(void)
>>>   	return &opts->func_inst;
>>>   }
>>>   
>>> +static void ecm_suspend(struct usb_function *f)
>>> +{
>>> +	struct f_ecm *ecm = func_to_ecm(f);
>>> +	struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
>>> +
>>> +	if (f->func_suspended) {
>>> +		DBG(cdev, "Function already suspended\n");
>>> +		return;
>>> +	}
>>> +
>>> +	DBG(cdev, "ECM Suspend\n");
>>> +
>>> +	gether_suspend(&ecm->port);
>>> +}
>>> +
>>> +static void ecm_resume(struct usb_function *f)
>>> +{
>>> +	struct f_ecm *ecm = func_to_ecm(f);
>>> +	struct usb_composite_dev *cdev = ecm->port.func.config->cdev;
>>> +
>>> +	/*
>>> +	 * If the function is in USB3 Function Suspend state, resume is
>>> +	 * canceled. In this case resume is done by a Function Resume request.
>>> +	 */
>>> +	if (f->func_suspended)
>>> +		return;
>>> +
>>> +	DBG(cdev, "ECM Resume\n");
>>> +
>>> +	gether_resume(&ecm->port);
>>> +}
>>> +
>>> +static int ecm_get_status(struct usb_function *f)
>>> +{
>>> +	struct usb_configuration *c = f->config;
>>> +
>>> +	/* D0 and D1 bit set to 0 if device is not wakeup capable */
>>> +	if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
>>> +		return 0;
>>> +
>>> +	return (f->func_wakeup_armed ? USB_INTRF_STAT_FUNC_RW : 0) |
>>> +		USB_INTRF_STAT_FUNC_RW_CAP;
>>> +}
>>> +
>>> +static int ecm_func_suspend(struct usb_function *f, u8 options)
>>> +{
>>> +	struct usb_configuration *c = f->config;
>>> +
>>> +	DBG(c->cdev, "func susp %u cmd\n", options);
>>> +
>>> +	/* Respond with negative errno if request is not supported */
>>> +	if (!(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
>>> +		return -EINVAL;
>>
>> We only need to return early if the host tries to enable remote wake
>> while the device isn't capable of it:
>>
>> 	wakeup_sel = !!(options & (USB_INTRF_FUNC_SUSPEND_RW >> 8));
>> 	if (wakeup_sel && !(USB_CONFIG_ATT_WAKEUP & c->bmAttributes))
>> 		return -EINVAL;
>>
>> 	f->func_wakeup_armed = wakeup_sel;
>>
> 
> Also, I notice that we can't differentiate between ClearFeature() and
> SetFeature() in f->func_suspend(). Perhaps we should handle arming the
> remote wakeup in the composite layer so we can set/clear the remote
> wake. It's common across multiple devices. It's probably better to be
> there.
> 
> Thanks,
> Thinh

Yeah agree with that. We check for function_wakeup_armed flag before 
sending remote wakeup in composite.c. So it makes more sense
to set/reset this flag in composite.c itself. I will make that change
and upload v12 along with fixing 'value = 0' removal that we discussed
earlier. Please let me know if that is fine

Thanks
Elson