Re: [PATCH] v4l2-async: add subnotifier registration for subdevices

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

 



Hejssan!

On Fri, Apr 28, 2017 at 01:47:48PM +0200, Niklas Söderlund wrote:
> On 2017-04-28 13:28:17 +0300, Sakari Ailus wrote:
> > Hi Niklas,
> > 
> > Thank you for the patch.
> > 
> > Do you happen to have a driver that would use this, to see some example of
> > how the code is to be used?
> 
> Yes, the latest R-Car CSI-2 series make use of this, see:
> 
> https://www.spinics.net/lists/linux-renesas-soc/msg13693.html

Ah, thanks. I'll take a look at that --- which should do for other reasons
as well...

...

> > > +
> > > +	/*
> > > +	 * This function can be called recursively so the list
> > > +	 * might be modified in a recursive call. Start from the
> > > +	 * top of the list each iteration.
> > > +	 */
> > > +	found = 1;
> > > +	while (found) {
> > > +		found = 0;
> > >  
> > > -	list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> > > -		int ret;
> > > +		list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> > > +			int ret;
> > >  
> > > -		asd = v4l2_async_belongs(notifier, sd);
> > > -		if (!asd)
> > > -			continue;
> > > +			asd = v4l2_async_belongs(notifier, sd);
> > > +			if (!asd)
> > > +				continue;
> > >  
> > > -		ret = v4l2_async_test_notify(notifier, sd, asd);
> > > -		if (ret < 0) {
> > > -			mutex_unlock(&list_lock);
> > > -			return ret;
> > > +			ret = v4l2_async_test_notify(notifier, sd, asd);
> > > +			if (ret < 0) {
> > > +				if (!subnotifier)
> > > +					mutex_unlock(&list_lock);
> > > +				return ret;
> > > +			}
> > > +
> > > +			found = 1;
> > > +			break;
> > >  		}
> > >  	}
> > >  
> > >  	/* Keep also completed notifiers on the list */
> > >  	list_add(&notifier->list, &notifier_list);
> > >  
> > > -	mutex_unlock(&list_lock);
> > > +	if (!subnotifier)
> > > +		mutex_unlock(&list_lock);
> > >  
> > >  	return 0;
> > >  }
> > > +
> > > +int v4l2_async_subnotifier_register(struct v4l2_subdev *sd,
> > > +				    struct v4l2_async_notifier *notifier)
> > > +{
> > > +	if (!sd->v4l2_dev) {
> > > +		dev_err(sd->dev ? sd->dev : NULL,

sd->dev is enough.

> > > +			"Can't register subnotifier for without v4l2_dev\n");
> > > +		return -EINVAL;
> > 
> > When did this start happening? :-)
> 
> What do you mean? I'm not sure I understand this comment.

Uh, right. So the caller simply needs to specify v4l2_dev? The same applies
to v4l2_async_notifier_register() which does not test that --- but it
should.

How about adding this change in a separate patch to what will be called
v4l2_async_do_notifier_register()?

> 
> > 
> > > +	}
> > > +
> > > +	return v4l2_async_do_notifier_register(sd->v4l2_dev, notifier, true);
> > > +}
> > > +EXPORT_SYMBOL(v4l2_async_subnotifier_register);
> > > +
> > > +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> > > +				 struct v4l2_async_notifier *notifier)
> > > +{
> > > +	return v4l2_async_do_notifier_register(v4l2_dev, notifier, false);
> > > +}
> > >  EXPORT_SYMBOL(v4l2_async_notifier_register);
> > >  
> > > -void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> > > +static void
> > > +v4l2_async_do_notifier_unregister(struct v4l2_async_notifier *notifier,
> > > +				  bool subnotifier)
> > >  {
> > >  	struct v4l2_subdev *sd, *tmp;
> > >  	unsigned int notif_n_subdev = notifier->num_subdevs;
> > > @@ -210,7 +248,8 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> > >  			"Failed to allocate device cache!\n");
> > >  	}
> > >  
> > > -	mutex_lock(&list_lock);
> > > +	if (!subnotifier)
> > > +		mutex_lock(&list_lock);
> > >  
> > >  	list_del(&notifier->list);
> > >  
> > > @@ -237,15 +276,20 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> > >  			put_device(d);
> > >  	}
> > >  
> > > -	mutex_unlock(&list_lock);
> > > +	if (!subnotifier)
> > > +		mutex_unlock(&list_lock);
> > >  
> > >  	/*
> > >  	 * Call device_attach() to reprobe devices
> > >  	 *
> > >  	 * NOTE: If dev allocation fails, i is 0, and the whole loop won't be
> > >  	 * executed.
> > > +	 * TODO: If we are unregistering a subdevice notifier we can't reprobe
> > > +	 * since the lock_list is held by the master device and attaching that
> > > +	 * device would call v4l2_async_register_subdev() and end in a deadlock
> > > +	 * on list_lock.
> > >  	 */
> > > -	while (i--) {
> > > +	while (i-- && !subnotifier) {
> > 
> > Why is this not done for sub-notifiers?
> > 
> > That said, the code here looks really dubious. But that's out of scope of
> > the patchset.
> 
> I try to explain this in the comment above :-)
> 
> If this is called for sub-notifiers it will result in the probe function 
> of the subdevices it contained to be called. And as most drivers call 
> v4l2_async_register_subdev() in there probe functions this will result 
> in a dead lock since v4l2_async_register_subdev() will try to lock the 
> list_lock (which for sub-notifiers already is held).
> 
> This is not optimal of course and I agree with you that this code is 
> dubious. It calls remove and then probe on all subdevices of the 
> notifier that is unregistered.

Ack. Let's address this one later.

-- 
Trevliga hälsningar,

Sakari Ailus
e-mail: sakari.ailus@xxxxxx	XMPP: sailus@xxxxxxxxxxxxxx



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux