Re: [PATCH v5 3/5] usb: gadget: Add function wakeup support

From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To: Elson Serrao <quic_eserrao@quicinc.com>
Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	"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>
Subject: Re: [PATCH v5 3/5] usb: gadget: Add function wakeup support
Date: Fri, 17 Feb 2023 02:02:42 +0000	[thread overview]
Message-ID: <20230217020233.ptta6otn6f77chn4@synopsys.com> (raw)
In-Reply-To: <96bbacbc-35d5-b8a4-938b-53259dcc03a7@quicinc.com>

On Thu, Feb 16, 2023, Elson Serrao wrote:
> 
> 
> 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

We may be able to say that for usb_func_wakeup() and document more on
the func_wakeup_armed flag. However, the driver can still call
usb_gadget_func_wakeup() directly right? Can you document the expected
behavior of usb_gadget_func_wakeup(). Would it fall back to behave like
usb_gadget_wakeup()? Or would it become no-op? Whichever you choose,
please document it.

Thanks,
Thinh

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