From: Greg KH <gregkh@linuxfoundation.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Michael Grzeschik <m.grzeschik@pengutronix.de>,
linux-usb@vger.kernel.org, linux-media@vger.kernel.org,
dan.scally@ideasonboard.com, nicolas@ndufresne.ca,
kernel@pengutronix.de
Subject: Re: [PATCH v2 1/3] usb: gadget: uvc: stop pump thread on video disable
Date: Thu, 5 Oct 2023 10:58:28 +0200 [thread overview]
Message-ID: <2023100507-sponsor-guise-d459@gregkh> (raw)
In-Reply-To: <20231005084805.GB31450@pendragon.ideasonboard.com>
On Thu, Oct 05, 2023 at 11:48:05AM +0300, Laurent Pinchart wrote:
> On Thu, Oct 05, 2023 at 10:40:10AM +0200, Greg KH wrote:
> > On Thu, Oct 05, 2023 at 11:17:16AM +0300, Laurent Pinchart wrote:
> > > Hi Michael,
> > >
> > > Thank you for the patch.
> > >
> > > On Mon, Sep 11, 2023 at 04:05:28PM +0200, Michael Grzeschik wrote:
> > > > Since the uvc-video gadget driver is using the v4l2 interface,
> > > > the streamon and streamoff can be triggered at any times. To ensure
> > > > that the pump worker will be closed as soon the userspace is
> > > > calling streamoff we synchronize the state of the gadget ensuring
> > > > the pump worker to bail out.
> > >
> > > I'm sorry but I really dislike this. Not only does the patch fail to
> > > ensure real synchronization, as the uvcg_video_pump() function still
> > > runs asynchronously, it messes up the usage of the state field that now
> > > tracks the state both from a host point of view (which it was doing so
> > > far, updating the state based on callbacks from the UDC), and from a
> > > gadget userspace point of view. This lacks clarity and is confusing.
> > > Furthermore, the commit message doesn't even explain what issue is being
> > > fixed here.
> > >
> > > Greg, I think this series has been merged too soon :-(
> >
> > Ok, I'll go revert them now, thanks for the review.
>
> Or we can wait a day for Michael to reply, in case this can quickly be
> fixed on top for v6.7. I'm now reading on the loooon discussion from v1,
> and reviewing the other pending patches that try to tackle the same
> issue.
I'd rather take a patchset that everyone agrees with, reverting was easy
and now done.
thanks,
greg k-h
next prev parent reply other threads:[~2023-10-05 8:58 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-11 14:05 [PATCH v2 0/3] usb: gadget: uvc: restart fixes Michael Grzeschik
2023-09-11 14:05 ` [PATCH v2 1/3] usb: gadget: uvc: stop pump thread on video disable Michael Grzeschik
2023-10-05 8:17 ` Laurent Pinchart
2023-10-05 8:40 ` Greg KH
2023-10-05 8:48 ` Laurent Pinchart
2023-10-05 8:58 ` Greg KH [this message]
2023-09-11 14:05 ` [PATCH v2 2/3] usb: gadget: uvc: cleanup request when not in correct state Michael Grzeschik
2023-10-05 8:21 ` Laurent Pinchart
2023-09-11 14:05 ` [PATCH v2 3/3] usb: gadget: uvc: rework pump worker to avoid while loop Michael Grzeschik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2023100507-sponsor-guise-d459@gregkh \
--to=gregkh@linuxfoundation.org \
--cc=dan.scally@ideasonboard.com \
--cc=kernel@pengutronix.de \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=m.grzeschik@pengutronix.de \
--cc=nicolas@ndufresne.ca \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.