| Message ID | 20170730223158.14405-2-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Sakari Ailus |
| Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1dbwlF-0003pD-Aq; Sun, 30 Jul 2017 22:32:33 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751727AbdG3WcX (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Sun, 30 Jul 2017 18:32:23 -0400 Received: from smtp-4.sys.kth.se ([130.237.48.193]:54263 "EHLO smtp-4.sys.kth.se" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751126AbdG3WcV (ORCPT <rfc822;linux-media@vger.kernel.org>); Sun, 30 Jul 2017 18:32:21 -0400 Received: from smtp-4.sys.kth.se (localhost.localdomain [127.0.0.1]) by smtp-4.sys.kth.se (Postfix) with ESMTP id AA30E2561; Mon, 31 Jul 2017 00:32:19 +0200 (CEST) X-Virus-Scanned: by amavisd-new at kth.se Received: from smtp-4.sys.kth.se ([127.0.0.1]) by smtp-4.sys.kth.se (smtp-4.sys.kth.se [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 68lLKbpA66kU; Mon, 31 Jul 2017 00:32:19 +0200 (CEST) X-KTH-Auth: niso [89.233.230.99] X-KTH-mail-from: niklas.soderlund+renesas@ragnatech.se Received: from bismarck.berto.se (unknown [89.233.230.99]) by smtp-4.sys.kth.se (Postfix) with ESMTPSA id AEDD52454; Mon, 31 Jul 2017 00:32:18 +0200 (CEST) From: =?UTF-8?q?Niklas=20S=C3=B6derlund?= <niklas.soderlund+renesas@ragnatech.se> To: Sakari Ailus <sakari.ailus@linux.intel.com>, Hans Verkuil <hverkuil@xs4all.nl>, Laurent Pinchart <laurent.pinchart@ideasonboard.com>, linux-media@vger.kernel.org Cc: Kieran Bingham <kieran.bingham@ideasonboard.com>, linux-renesas-soc@vger.kernel.org, Maxime Ripard <maxime.ripard@free-electrons.com>, Sylwester Nawrocki <snawrocki@kernel.org>, =?UTF-8?q?Niklas=20S=C3=B6derlund?= <niklas.soderlund+renesas@ragnatech.se> Subject: [PATCH 1/4] v4l: async: fix unbind error in v4l2_async_notifier_unregister() Date: Mon, 31 Jul 2017 00:31:55 +0200 Message-Id: <20170730223158.14405-2-niklas.soderlund+renesas@ragnatech.se> X-Mailer: git-send-email 2.13.3 In-Reply-To: <20170730223158.14405-1-niklas.soderlund+renesas@ragnatech.se> References: <20170730223158.14405-1-niklas.soderlund+renesas@ragnatech.se> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Niklas Söderlund
July 30, 2017, 10:31 p.m. UTC
The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it
to notifier->unbind() have no effect and leaves the notifier confused.
Call the unbind() callback prior to cleaning up the subdevice to avoid
this.
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
drivers/media/v4l2-core/v4l2-async.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Comments
On Mon, Jul 31, 2017 at 12:31:55AM +0200, Niklas Söderlund wrote: > The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it > to notifier->unbind() have no effect and leaves the notifier confused. > Call the unbind() callback prior to cleaning up the subdevice to avoid > this. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> This is a bugfix and worthy without any other patches and so should be applied separately. I think it'd be safer to store sd->asd locally and call the notifier unbind with that. Now you're making changes to the order in which things work, and that's not necessary to achieve the objective of passing the async subdev pointer to the notifier. With that changed, Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 851f128eba2219ad..0acf288d7227ba97 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -226,14 +226,14 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > > d = get_device(sd->dev); > > + if (notifier->unbind) > + notifier->unbind(notifier, sd, sd->asd); > + > v4l2_async_cleanup(sd); > > /* If we handled USB devices, we'd have to lock the parent too */ > device_release_driver(d); > > - if (notifier->unbind) > - notifier->unbind(notifier, sd, sd->asd); > - > /* > * Store device at the device cache, in order to call > * put_device() on the final step > -- > 2.13.3 >
Hi Niklas, Thank you for the patch. On Monday 31 Jul 2017 00:31:55 Niklas Söderlund wrote: > The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it > to notifier->unbind() have no effect and leaves the notifier confused. > Call the unbind() callback prior to cleaning up the subdevice to avoid > this. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c > b/drivers/media/v4l2-core/v4l2-async.c index > 851f128eba2219ad..0acf288d7227ba97 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -226,14 +226,14 @@ void v4l2_async_notifier_unregister(struct > v4l2_async_notifier *notifier) > > d = get_device(sd->dev); > > + if (notifier->unbind) > + notifier->unbind(notifier, sd, sd->asd); > + > v4l2_async_cleanup(sd); > > /* If we handled USB devices, we'd have to lock the parent too */ > device_release_driver(d); > > - if (notifier->unbind) > - notifier->unbind(notifier, sd, sd->asd); > - > /* > * Store device at the device cache, in order to call > * put_device() on the final step
Hi Sakari, On Tuesday 15 Aug 2017 19:16:14 Sakari Ailus wrote: > On Mon, Jul 31, 2017 at 12:31:55AM +0200, Niklas Söderlund wrote: > > The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it > > to notifier->unbind() have no effect and leaves the notifier confused. > > Call the unbind() callback prior to cleaning up the subdevice to avoid > > this. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > This is a bugfix and worthy without any other patches and so should be > applied separately. > > I think it'd be safer to store sd->asd locally and call the notifier unbind > with that. Now you're making changes to the order in which things work, and > that's not necessary to achieve the objective of passing the async subdev > pointer to the notifier. But on the other hand I think the unbind notification should be called before the subdevice gets unbound, the same way the bound notification is called after it gets bound. One of the purposes of the unbind notification is to allow drivers to prepare for subdev about to be unbound, and they can't prepare if the unbind happened already. > With that changed, > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > > drivers/media/v4l2-core/v4l2-async.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c > > b/drivers/media/v4l2-core/v4l2-async.c index > > 851f128eba2219ad..0acf288d7227ba97 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -226,14 +226,14 @@ void v4l2_async_notifier_unregister(struct > > v4l2_async_notifier *notifier)> > > d = get_device(sd->dev); > > > > + if (notifier->unbind) > > + notifier->unbind(notifier, sd, sd->asd); > > + > > > > v4l2_async_cleanup(sd); > > > > /* If we handled USB devices, we'd have to lock the parent too */ > > device_release_driver(d); > > > > - if (notifier->unbind) > > - notifier->unbind(notifier, sd, sd->asd); > > - > > > > /* > > > > * Store device at the device cache, in order to call > > * put_device() on the final step
Hi, On 2017-08-18 14:15:26 +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Tuesday 15 Aug 2017 19:16:14 Sakari Ailus wrote: > > On Mon, Jul 31, 2017 at 12:31:55AM +0200, Niklas Söderlund wrote: > > > The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it > > > to notifier->unbind() have no effect and leaves the notifier confused. > > > Call the unbind() callback prior to cleaning up the subdevice to avoid > > > this. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > This is a bugfix and worthy without any other patches and so should be > > applied separately. > > > > I think it'd be safer to store sd->asd locally and call the notifier unbind > > with that. Now you're making changes to the order in which things work, and > > that's not necessary to achieve the objective of passing the async subdev > > pointer to the notifier. > > But on the other hand I think the unbind notification should be called before > the subdevice gets unbound, the same way the bound notification is called > after it gets bound. One of the purposes of the unbind notification is to > allow drivers to prepare for subdev about to be unbound, and they can't > prepare if the unbind happened already. I'm not opposed to move in the direction suggested by Sakari but I agree with Laurent here. It makes more sens that the unbind callback is called before the actual unbind happens. At the same time I agree that it dose change the behavior, but I think it's for the better. > > > With that changed, > > > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > > --- > > > > > > drivers/media/v4l2-core/v4l2-async.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c > > > b/drivers/media/v4l2-core/v4l2-async.c index > > > 851f128eba2219ad..0acf288d7227ba97 100644 > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > @@ -226,14 +226,14 @@ void v4l2_async_notifier_unregister(struct > > > v4l2_async_notifier *notifier)> > > > d = get_device(sd->dev); > > > > > > + if (notifier->unbind) > > > + notifier->unbind(notifier, sd, sd->asd); > > > + > > > > > > v4l2_async_cleanup(sd); > > > > > > /* If we handled USB devices, we'd have to lock the parent too > */ > > > device_release_driver(d); > > > > > > - if (notifier->unbind) > > > - notifier->unbind(notifier, sd, sd->asd); > > > - > > > > > > /* > > > > > > * Store device at the device cache, in order to call > > > * put_device() on the final step > > -- > Regards, > > Laurent Pinchart >
On Fri, Aug 18, 2017 at 03:42:07PM +0200, Niklas Söderlund wrote: > Hi, > > On 2017-08-18 14:15:26 +0300, Laurent Pinchart wrote: > > Hi Sakari, > > > > On Tuesday 15 Aug 2017 19:16:14 Sakari Ailus wrote: > > > On Mon, Jul 31, 2017 at 12:31:55AM +0200, Niklas Söderlund wrote: > > > > The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it > > > > to notifier->unbind() have no effect and leaves the notifier confused. > > > > Call the unbind() callback prior to cleaning up the subdevice to avoid > > > > this. > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > > > This is a bugfix and worthy without any other patches and so should be > > > applied separately. > > > > > > I think it'd be safer to store sd->asd locally and call the notifier unbind > > > with that. Now you're making changes to the order in which things work, and > > > that's not necessary to achieve the objective of passing the async subdev > > > pointer to the notifier. > > > > But on the other hand I think the unbind notification should be called before > > the subdevice gets unbound, the same way the bound notification is called > > after it gets bound. One of the purposes of the unbind notification is to > > allow drivers to prepare for subdev about to be unbound, and they can't > > prepare if the unbind happened already. > > I'm not opposed to move in the direction suggested by Sakari but I agree > with Laurent here. It makes more sens that the unbind callback is called > before the actual unbind happens. At the same time I agree that it dose > change the behavior, but I think it's for the better. I agree with Laurent's reasoning. Feel free to add my ack. > > > > > > With that changed, > > > > > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > > > > --- > > > > > > > > drivers/media/v4l2-core/v4l2-async.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c > > > > b/drivers/media/v4l2-core/v4l2-async.c index > > > > 851f128eba2219ad..0acf288d7227ba97 100644 > > > > --- a/drivers/media/v4l2-core/v4l2-async.c > > > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > > > @@ -226,14 +226,14 @@ void v4l2_async_notifier_unregister(struct > > > > v4l2_async_notifier *notifier)> > > > > d = get_device(sd->dev); > > > > > > > > + if (notifier->unbind) > > > > + notifier->unbind(notifier, sd, sd->asd); > > > > + > > > > > > > > v4l2_async_cleanup(sd); > > > > > > > > /* If we handled USB devices, we'd have to lock the parent too > > */ > > > > device_release_driver(d); > > > > > > > > - if (notifier->unbind) > > > > - notifier->unbind(notifier, sd, sd->asd); > > > > - > > > > > > > > /* > > > > > > > > * Store device at the device cache, in order to call > > > > * put_device() on the final step > > > > -- > > Regards, > > > > Laurent Pinchart > > > > -- > Regards, > Niklas Söderlund
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index 851f128eba2219ad..0acf288d7227ba97 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -226,14 +226,14 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) d = get_device(sd->dev); + if (notifier->unbind) + notifier->unbind(notifier, sd, sd->asd); + v4l2_async_cleanup(sd); /* If we handled USB devices, we'd have to lock the parent too */ device_release_driver(d); - if (notifier->unbind) - notifier->unbind(notifier, sd, sd->asd); - /* * Store device at the device cache, in order to call * put_device() on the final step