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@xxxxxxxxxxxx> > > > > > > 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@xxxxxxxxxxxxxxx> > > > > > > > --- > > > > > > > > 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 -- Sakari Ailus sakari.ailus@xxxxxxxxxxxxxxx
