Message-ID: <96bbacbc-35d5-b8a4-938b-53259dcc03a7@quicinc.com>
Date:   Thu, 16 Feb 2023 16:38:24 -0800
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 v5 3/5] usb: gadget: Add function 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: <1676586588-25989-1-git-send-email-quic_eserrao@quicinc.com>
 <1676586588-25989-4-git-send-email-quic_eserrao@quicinc.com>
 <20230216235759.ylqsrsyp3tpppmef@synopsys.com>
From:   Elson Serrao <quic_eserrao@quicinc.com>
In-Reply-To: <20230216235759.ylqsrsyp3tpppmef@synopsys.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2/16/2023 3:58 PM, Thinh Nguyen wrote:
> On Thu, Feb 16, 2023, Elson Roy Serrao wrote:
>> A function which is in function suspend state has to send a
>> function wake notification to the host in case it needs to
>> exit from this state and resume data transfer. Add support to
>> handle such requests by exposing a new gadget op.
>>
>> Signed-off-by: Elson Roy Serrao <quic_eserrao@quicinc.com>
>> ---
>>   drivers/usb/gadget/composite.c | 24 ++++++++++++++++++++++++
>>   drivers/usb/gadget/udc/core.c  | 21 +++++++++++++++++++++
>>   include/linux/usb/composite.h  |  6 ++++++
>>   include/linux/usb/gadget.h     |  4 ++++
>>   4 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index a37a8f4..f649f997 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -492,6 +492,30 @@ int usb_interface_id(struct usb_configuration *config,
>>   }
>>   EXPORT_SYMBOL_GPL(usb_interface_id);
>>   
>> +int usb_func_wakeup(struct usb_function *func)
>> +{
>> +	int ret, id;
>> +
>> +	if (!func->func_wakeup_armed) {
>> +		ERROR(func->config->cdev, "not armed for func remote wakeup\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	for (id = 0; id < MAX_CONFIG_INTERFACES; id++)
>> +		if (func->config->interface[id] == func)
>> +			break;
>> +
>> +	if (id == MAX_CONFIG_INTERFACES) {
>> +		ERROR(func->config->cdev, "Invalid function\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = usb_gadget_func_wakeup(func->config->cdev->gadget, id);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_func_wakeup);
>> +
>>   static u8 encode_bMaxPower(enum usb_device_speed speed,
>>   		struct usb_configuration *c)
>>   {
>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> index 3dcbba7..59e7a7e 100644
>> --- a/drivers/usb/gadget/udc/core.c
>> +++ b/drivers/usb/gadget/udc/core.c
>> @@ -846,6 +846,27 @@ int usb_gadget_activate(struct usb_gadget *gadget)
>>   }
>>   EXPORT_SYMBOL_GPL(usb_gadget_activate);
>>   
>> +/**
>> + * usb_gadget_func_wakeup - sends function wake notification to the host.
>> + * @gadget: controller used to wake up the host
>> + * @interface_id: interface on which function wake notification is sent.
> 
> Device notification is only applicable for eSS devices. What will happen
> if the device is operating in lower speed and the driver calls this
> function?
> 
> Thanks,
> Thinh
> 

Since the non-eSS devices dont support function suspend, the function 
suspend feature selector is not sent by the host and the function is not 
armed for sending function remote wakeup. So the usb_func_wakeup() API 
that is called from the function drivers fails the attempt

int usb_func_wakeup(struct usb_function *func)
{
	int ret, id;

	if (!func->func_wakeup_armed) {
		ERROR(func->config->cdev, "not armed for func remote wakeup\n");
		return -EINVAL;
	}

Let me know if its better to add an explicit speed check as well here.

Thanks
Elson

>> + *
>> + * On completion, function wake notification is sent. If the device is in
>> + * low power state it tries to bring the device to active state before sending
>> + * the wake notification. Since it is a synchronous call, caller must take care
>> + * of not calling it in interrupt context.
>> + *
>> + * Returns zero on success, else negative errno.
>> + */
>> +int usb_gadget_func_wakeup(struct usb_gadget *gadget, int intf_id)
>> +{
>> +	if (!gadget->ops->func_wakeup)
>> +		return -EOPNOTSUPP;
>> +
>> +	return gadget->ops->func_wakeup(gadget, intf_id);
>> +}
>> +EXPORT_SYMBOL_GPL(usb_gadget_func_wakeup);