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(¬ifier->list, ¬ifier_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(¬ifier->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: [email protected] XMPP: [email protected]
