Re: [PATCH 1/4] v4l: async: fix unbind error in v4l2_async_notifier_unregister()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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

-- 
Regards,

Laurent Pinchart