Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 3 May 2022 14:02:58 -0500
From:   Dan Vacura <w36195@motorola.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-usb@vger.kernel.org,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Felipe Balbi <balbi@kernel.org>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] usb: gadget: uvc: allow for application to cleanly
 shutdown
Message-ID: <YnF8YmKqFm5oONkB@p1g3>
References: <20220429192001.385636-1-w36195@motorola.com>
 <YmzrwgiEO2hoKM4U@kroah.com>
 <Ym9Z+BfHcwDKlwjy@p1g3>
 <Ym/En8EjfkpIVm+a@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Ym/En8EjfkpIVm+a@kroah.com>
Precedence: bulk

Hi Greg,

Thanks for the feedback.

On Mon, May 02, 2022 at 01:46:39PM +0200, Greg Kroah-Hartman wrote:
> On Sun, May 01, 2022 at 11:11:36PM -0500, Dan Vacura wrote:
> > On Sat, Apr 30, 2022 at 09:56:50AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Apr 29, 2022 at 02:20:01PM -0500, Dan Vacura wrote:
> > > > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c
> > > > index 50e6e7a58b41..3cc8cf24a7c7 100644
> > > > --- a/drivers/usb/gadget/function/f_uvc.c
> > > > +++ b/drivers/usb/gadget/function/f_uvc.c
> > > > @@ -892,13 +892,36 @@ static void uvc_function_unbind(struct usb_configuration *c,
> > > >  {
> > > >  	struct usb_composite_dev *cdev = c->cdev;
> > > >  	struct uvc_device *uvc = to_uvc(f);
> > > > +	int wait_ret = 1;
> > > >  
> > > >  	uvcg_info(f, "%s()\n", __func__);
> > > 
> > > Ick, wait, is that in the kernel?  That needs to be removed, ftrace can
> > > do that for you.
> > 
> > Yes, part of the kernel, and tbh, I find it to be quite helpful in
> > debugging field issues from customers, where enabling ftrace isn't
> > practical.
> 
> Why isn't ftrace ok to enable in a running kernel?

ftrace is totally fine to enable. I should've said convenient instead of
practical, from my experience there's a bit more offline/developer
overhead for setup and integrating the trace logs with a panic or kmsg,
then bundling that with the bug reports we get.

> 
> Worst case, this should be dev_dbg(), right?
> 
> > If you still want to remove, there are other locations in
> > this gadget driver that log function entry. Perhaps it'd be better to
> > do a separate change that cleans up logging a bit or do you prefer to
> > just refactor this one now?
> 
> This commit is fine, it's a separate issue, I just noticed it as it was
> in the context of this change.
> 
> > > > +	/* If we know we're connected via v4l2, then there should be a cleanup
> > > > +	 * of the device from userspace either via UVC_EVENT_DISCONNECT or
> > > > +	 * though the video device removal uevent. Allow some time for the
> > > > +	 * application to close out before things get deleted.
> > > > +	 */
> > > > +	if (uvc->func_connected) {
> > > > +		uvcg_info(f, "%s waiting for clean disconnect\n", __func__);
> > > > +		wait_ret = wait_event_interruptible_timeout(uvc->func_connected_queue,
> > > > +				uvc->func_connected == false, msecs_to_jiffies(500));
> > > > +		uvcg_info(f, "%s done waiting with ret: %u\n", __func__, wait_ret);
> > > 
> > > Please remove debugging code before submitting patches.
> > 
> > Will do.
> 
> But this should be removed :)
> 
> Feel free to change it to dev_dbg(), which gives you the __func__
> automatically without anything extra needed.

Yes, I'll go with this approach, as it doesn't make sense to always
print, but is good to have when developing or debugging. The timeouts
seem reasonable to me and work for our setup, but I was expecting some
comments about them or a suggestion for a different approach.

Thanks,

Dan