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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux