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

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

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
> 

-- 
Regards,
Niklas Söderlund