Message-ID: <0bf5bddd-515b-76b6-f22f-9da10a140d83@quicinc.com>
Date:   Fri, 17 Mar 2023 10:59:53 -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 v12 6/6] 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>,
        "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: <1679009888-8239-1-git-send-email-quic_eserrao@quicinc.com>
 <1679009888-8239-7-git-send-email-quic_eserrao@quicinc.com>
 <20230317001149.nlvcj2y3fuvq32qt@synopsys.com>
From:   Elson Serrao <quic_eserrao@quicinc.com>
In-Reply-To: <20230317001149.nlvcj2y3fuvq32qt@synopsys.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 3/16/2023 5:11 PM, Thinh Nguyen wrote:
> On Thu, Mar 16, 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   | 71 +++++++++++++++++++++++++++++++++++
>>   drivers/usb/gadget/function/u_ether.c | 63 +++++++++++++++++++++++++++++++
>>   drivers/usb/gadget/function/u_ether.h |  4 ++
>>   3 files changed, 138 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_ecm.c b/drivers/usb/gadget/function/f_ecm.c
>> index a7ab30e..c43cd557 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,71 @@ 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;
>> +}
> 
> Why do we need to implement ecm_get_status if it's already handled in
> composite.c now?
> 

Yes this can be removed now. Will modify accordingly.
>> +
>> +static int ecm_func_suspend(struct usb_function *f, u8 options)
>> +{
>> +	struct usb_composite_dev *cdev = f->config->cdev;
>> +
>> +	DBG(cdev, "func susp %u cmd\n", options);
>> +
>> +	if (options & (USB_INTRF_FUNC_SUSPEND_LP >> 8)) {
> 
> This feature selector doesn't indicate whether it's SetFeature or
> ClearFeature request. ecm_func_suspend is supposed to be for
> SetFeature(suspend) only. Perhaps we may have to define func_resume()
> for ClearFeature(suspend)?
> 
> Thanks,
> Thinh
> 
Host uses the same feature selector FUNCTION_SUSPEND for function 
suspend and function resume and func_suspend() callback can be used to
handle both the cases ? The distinction comes whether it is a 
SetFeature(FUNCTION_SUSPEND) or ClearFeature(FUNCTION_SUSPEND) which can 
be easily done in the func_suspend callback itself. We can add another 
callback func_resume specific to ClearFeature(FUNCTION_SUSPEND) but wont 
that be redundant and more callback handling on function 
driver/composite side as well? Please let me know your opinion.

Thanks
Elson

>> +		if (!f->func_suspended) {
>> +			ecm_suspend(f);
>> +			f->func_suspended = true;
>> +		}
>> +	} else {
>> +		if (f->func_suspended) {
>> +			f->func_suspended = false;
>> +			ecm_resume(f);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +