| Message ID | 20170214133956.GA8530@amd (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Tue, Feb 14, 2017 at 02:39:56PM +0100, Pavel Machek wrote: > From: Sebastian Reichel <sre@kernel.org> > > Without this, camera support breaks boot on N900. That's kind of vague. I just checked my original patch and it looks like I did not bother to write a proper patch description. I suggest to make this "Fix v4l2-async locking, so that v4l2_async_notifiers can be used recursively. This is important for sensors, that are only reachable by the image signal processor through some intermediate device." You should probably move move this patch directly before the video-bus-switch patch, since its a preparation for it. Also this is missing my SoB: Signed-off-by: Sebastian Reichel <sre@kernel.org> > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 54 ++++++++++++++++++------------------ > include/media/v4l2-async.h | 2 ++ > 2 files changed, 29 insertions(+), 27 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 96cc733..26492a2 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -57,7 +57,6 @@ static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > > static LIST_HEAD(subdev_list); > static LIST_HEAD(notifier_list); > -static DEFINE_MUTEX(list_lock); > > static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd) > @@ -102,12 +101,15 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier, > > if (notifier->bound) { > ret = notifier->bound(notifier, sd, asd); > - if (ret < 0) > + if (ret < 0) { > + dev_warn(notifier->v4l2_dev->dev, "subdev bound failed\n"); > return ret; > + } > } > > ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd); > if (ret < 0) { > + dev_warn(notifier->v4l2_dev->dev, "subdev register failed\n"); > if (notifier->unbind) > notifier->unbind(notifier, sd, asd); > return ret; > @@ -141,7 +143,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > { > struct v4l2_subdev *sd, *tmp; > struct v4l2_async_subdev *asd; > - int i; > + int ret = 0, i; > > if (!notifier->num_subdevs || notifier->num_subdevs > V4L2_MAX_SUBDEVS) > return -EINVAL; > @@ -149,6 +151,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > notifier->v4l2_dev = v4l2_dev; > INIT_LIST_HEAD(¬ifier->waiting); > INIT_LIST_HEAD(¬ifier->done); > + mutex_init(¬ifier->lock); > > for (i = 0; i < notifier->num_subdevs; i++) { > asd = notifier->subdevs[i]; > @@ -168,28 +171,22 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > list_add_tail(&asd->list, ¬ifier->waiting); > } > > - mutex_lock(&list_lock); > + /* Keep also completed notifiers on the list */ > + list_add(¬ifier->list, ¬ifier_list); > + mutex_lock(¬ifier->lock); > > list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) { > - int ret; > - > 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; > - } > + if (ret < 0) > + break; > } > + mutex_unlock(¬ifier->lock); > > - /* Keep also completed notifiers on the list */ > - list_add(¬ifier->list, ¬ifier_list); > - > - mutex_unlock(&list_lock); > - > - return 0; > + return ret; > } > EXPORT_SYMBOL(v4l2_async_notifier_register); > > @@ -210,7 +207,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > "Failed to allocate device cache!\n"); > } > > - mutex_lock(&list_lock); > + mutex_lock(¬ifier->lock); > > list_del(¬ifier->list); > > @@ -237,7 +234,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > put_device(d); > } > > - mutex_unlock(&list_lock); > + mutex_unlock(¬ifier->lock); > > /* > * Call device_attach() to reprobe devices > @@ -262,6 +259,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > } > kfree(dev); > > + mutex_destroy(¬ifier->lock); > notifier->v4l2_dev = NULL; > > /* > @@ -274,6 +272,7 @@ EXPORT_SYMBOL(v4l2_async_notifier_unregister); > int v4l2_async_register_subdev(struct v4l2_subdev *sd) > { > struct v4l2_async_notifier *notifier; > + struct v4l2_async_notifier *tmp; > > /* > * No reference taken. The reference is held by the device > @@ -283,24 +282,25 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > if (!sd->of_node && sd->dev) > sd->of_node = sd->dev->of_node; > > - mutex_lock(&list_lock); > - > INIT_LIST_HEAD(&sd->async_list); > > - list_for_each_entry(notifier, ¬ifier_list, list) { > - struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, sd); > + list_for_each_entry_safe(notifier, tmp, ¬ifier_list, list) { > + struct v4l2_async_subdev *asd; > + > + /* TODO: FIXME: if this is called by ->bound() we will also iterate over the locked notifier */ > + mutex_lock_nested(¬ifier->lock, SINGLE_DEPTH_NESTING); > + asd = v4l2_async_belongs(notifier, sd); > if (asd) { > int ret = v4l2_async_test_notify(notifier, sd, asd); > - mutex_unlock(&list_lock); > + mutex_unlock(¬ifier->lock); > return ret; > } > + mutex_unlock(¬ifier->lock); > } > > /* None matched, wait for hot-plugging */ > list_add(&sd->async_list, &subdev_list); > > - mutex_unlock(&list_lock); > - > return 0; > } > EXPORT_SYMBOL(v4l2_async_register_subdev); > @@ -315,7 +315,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > return; > } > > - mutex_lock(&list_lock); > + mutex_lock_nested(¬ifier->lock, SINGLE_DEPTH_NESTING); > > list_add(&sd->asd->list, ¬ifier->waiting); > > @@ -324,6 +324,6 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > if (notifier->unbind) > notifier->unbind(notifier, sd, sd->asd); > > - mutex_unlock(&list_lock); > + mutex_unlock(¬ifier->lock); > } > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > index 8e2a236..690a81f 100644 > --- a/include/media/v4l2-async.h > +++ b/include/media/v4l2-async.h > @@ -84,6 +84,7 @@ struct v4l2_async_subdev { > * @waiting: list of struct v4l2_async_subdev, waiting for their drivers > * @done: list of struct v4l2_subdev, already probed > * @list: member in a global list of notifiers > + * @lock: lock hold when the notifier is being processed > * @bound: a subdevice driver has successfully probed one of subdevices > * @complete: all subdevices have been probed successfully > * @unbind: a subdevice is leaving > @@ -95,6 +96,7 @@ struct v4l2_async_notifier { > struct list_head waiting; > struct list_head done; > struct list_head list; > + struct mutex lock; > int (*bound)(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *subdev, > struct v4l2_async_subdev *asd); > -- > 2.1.4 > > > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Pavel and Sebastian, On Tue, Feb 14, 2017 at 02:39:56PM +0100, Pavel Machek wrote: > From: Sebastian Reichel <sre@kernel.org> > > Without this, camera support breaks boot on N900. > > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 54 ++++++++++++++++++------------------ > include/media/v4l2-async.h | 2 ++ > 2 files changed, 29 insertions(+), 27 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 96cc733..26492a2 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -57,7 +57,6 @@ static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) > > static LIST_HEAD(subdev_list); > static LIST_HEAD(notifier_list); > -static DEFINE_MUTEX(list_lock); > > static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *sd) > @@ -102,12 +101,15 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier, > > if (notifier->bound) { > ret = notifier->bound(notifier, sd, asd); > - if (ret < 0) > + if (ret < 0) { > + dev_warn(notifier->v4l2_dev->dev, "subdev bound failed\n"); > return ret; > + } > } > > ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd); > if (ret < 0) { > + dev_warn(notifier->v4l2_dev->dev, "subdev register failed\n"); > if (notifier->unbind) > notifier->unbind(notifier, sd, asd); > return ret; > @@ -141,7 +143,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > { > struct v4l2_subdev *sd, *tmp; > struct v4l2_async_subdev *asd; > - int i; > + int ret = 0, i; > > if (!notifier->num_subdevs || notifier->num_subdevs > V4L2_MAX_SUBDEVS) > return -EINVAL; > @@ -149,6 +151,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > notifier->v4l2_dev = v4l2_dev; > INIT_LIST_HEAD(¬ifier->waiting); > INIT_LIST_HEAD(¬ifier->done); > + mutex_init(¬ifier->lock); > > for (i = 0; i < notifier->num_subdevs; i++) { > asd = notifier->subdevs[i]; > @@ -168,28 +171,22 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > list_add_tail(&asd->list, ¬ifier->waiting); > } > > - mutex_lock(&list_lock); > + /* Keep also completed notifiers on the list */ > + list_add(¬ifier->list, ¬ifier_list); > + mutex_lock(¬ifier->lock); > > list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) { > - int ret; > - > 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; > - } > + if (ret < 0) > + break; > } > + mutex_unlock(¬ifier->lock); > > - /* Keep also completed notifiers on the list */ > - list_add(¬ifier->list, ¬ifier_list); > - > - mutex_unlock(&list_lock); > - > - return 0; > + return ret; > } > EXPORT_SYMBOL(v4l2_async_notifier_register); > > @@ -210,7 +207,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > "Failed to allocate device cache!\n"); > } > > - mutex_lock(&list_lock); > + mutex_lock(¬ifier->lock); > > list_del(¬ifier->list); > > @@ -237,7 +234,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > put_device(d); > } > > - mutex_unlock(&list_lock); > + mutex_unlock(¬ifier->lock); > > /* > * Call device_attach() to reprobe devices > @@ -262,6 +259,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > } > kfree(dev); > > + mutex_destroy(¬ifier->lock); > notifier->v4l2_dev = NULL; > > /* > @@ -274,6 +272,7 @@ EXPORT_SYMBOL(v4l2_async_notifier_unregister); > int v4l2_async_register_subdev(struct v4l2_subdev *sd) > { > struct v4l2_async_notifier *notifier; > + struct v4l2_async_notifier *tmp; > > /* > * No reference taken. The reference is held by the device > @@ -283,24 +282,25 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > if (!sd->of_node && sd->dev) > sd->of_node = sd->dev->of_node; > > - mutex_lock(&list_lock); > - > INIT_LIST_HEAD(&sd->async_list); > > - list_for_each_entry(notifier, ¬ifier_list, list) { > - struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, sd); > + list_for_each_entry_safe(notifier, tmp, ¬ifier_list, list) { You still need to serialise access to the global notifier list. The _safe iterator variants allow deleting the current entry from the list but they do not help more than that. One possible approach could be to gather the matching async sub-devices and call the v4l2_async_test_notify() while not holding the notifier list mutex anymore. I presume the same problem is present in notifier registration. > + struct v4l2_async_subdev *asd; > + > + /* TODO: FIXME: if this is called by ->bound() we will also iterate over the locked notifier */ > + mutex_lock_nested(¬ifier->lock, SINGLE_DEPTH_NESTING); > + asd = v4l2_async_belongs(notifier, sd); > if (asd) { > int ret = v4l2_async_test_notify(notifier, sd, asd); > - mutex_unlock(&list_lock); > + mutex_unlock(¬ifier->lock); > return ret; > } > + mutex_unlock(¬ifier->lock); > } > > /* None matched, wait for hot-plugging */ > list_add(&sd->async_list, &subdev_list); > > - mutex_unlock(&list_lock); > - > return 0; > } > EXPORT_SYMBOL(v4l2_async_register_subdev); > @@ -315,7 +315,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > return; > } > > - mutex_lock(&list_lock); > + mutex_lock_nested(¬ifier->lock, SINGLE_DEPTH_NESTING); > > list_add(&sd->asd->list, ¬ifier->waiting); > > @@ -324,6 +324,6 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) > if (notifier->unbind) > notifier->unbind(notifier, sd, sd->asd); > > - mutex_unlock(&list_lock); > + mutex_unlock(¬ifier->lock); > } > EXPORT_SYMBOL(v4l2_async_unregister_subdev); > diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h > index 8e2a236..690a81f 100644 > --- a/include/media/v4l2-async.h > +++ b/include/media/v4l2-async.h > @@ -84,6 +84,7 @@ struct v4l2_async_subdev { > * @waiting: list of struct v4l2_async_subdev, waiting for their drivers > * @done: list of struct v4l2_subdev, already probed > * @list: member in a global list of notifiers > + * @lock: lock hold when the notifier is being processed > * @bound: a subdevice driver has successfully probed one of subdevices > * @complete: all subdevices have been probed successfully > * @unbind: a subdevice is leaving > @@ -95,6 +96,7 @@ struct v4l2_async_notifier { > struct list_head waiting; > struct list_head done; > struct list_head list; > + struct mutex lock; > int (*bound)(struct v4l2_async_notifier *notifier, > struct v4l2_subdev *subdev, > struct v4l2_async_subdev *asd);
Hi! > > From: Sebastian Reichel <sre@kernel.org> > > > > Without this, camera support breaks boot on N900. > > That's kind of vague. I just checked my original patch and it looks > like I did not bother to write a proper patch description. I suggest > to make this > > "Fix v4l2-async locking, so that v4l2_async_notifiers can be used > recursively. This is important for sensors, that are only reachable > by the image signal processor through some intermediate device." > > You should probably move move this patch directly before the > video-bus-switch patch, since its a preparation for it. > > Also this is missing my SoB: > > Signed-off-by: Sebastian Reichel <sre@kernel.org> Thanks, done. Pavel
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index 96cc733..26492a2 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -57,7 +57,6 @@ static bool match_custom(struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) static LIST_HEAD(subdev_list); static LIST_HEAD(notifier_list); -static DEFINE_MUTEX(list_lock); static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *notifier, struct v4l2_subdev *sd) @@ -102,12 +101,15 @@ static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier, if (notifier->bound) { ret = notifier->bound(notifier, sd, asd); - if (ret < 0) + if (ret < 0) { + dev_warn(notifier->v4l2_dev->dev, "subdev bound failed\n"); return ret; + } } ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd); if (ret < 0) { + dev_warn(notifier->v4l2_dev->dev, "subdev register failed\n"); if (notifier->unbind) notifier->unbind(notifier, sd, asd); return ret; @@ -141,7 +143,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, { struct v4l2_subdev *sd, *tmp; struct v4l2_async_subdev *asd; - int i; + int ret = 0, i; if (!notifier->num_subdevs || notifier->num_subdevs > V4L2_MAX_SUBDEVS) return -EINVAL; @@ -149,6 +151,7 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, notifier->v4l2_dev = v4l2_dev; INIT_LIST_HEAD(¬ifier->waiting); INIT_LIST_HEAD(¬ifier->done); + mutex_init(¬ifier->lock); for (i = 0; i < notifier->num_subdevs; i++) { asd = notifier->subdevs[i]; @@ -168,28 +171,22 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, list_add_tail(&asd->list, ¬ifier->waiting); } - mutex_lock(&list_lock); + /* Keep also completed notifiers on the list */ + list_add(¬ifier->list, ¬ifier_list); + mutex_lock(¬ifier->lock); list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) { - int ret; - 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; - } + if (ret < 0) + break; } + mutex_unlock(¬ifier->lock); - /* Keep also completed notifiers on the list */ - list_add(¬ifier->list, ¬ifier_list); - - mutex_unlock(&list_lock); - - return 0; + return ret; } EXPORT_SYMBOL(v4l2_async_notifier_register); @@ -210,7 +207,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) "Failed to allocate device cache!\n"); } - mutex_lock(&list_lock); + mutex_lock(¬ifier->lock); list_del(¬ifier->list); @@ -237,7 +234,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) put_device(d); } - mutex_unlock(&list_lock); + mutex_unlock(¬ifier->lock); /* * Call device_attach() to reprobe devices @@ -262,6 +259,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) } kfree(dev); + mutex_destroy(¬ifier->lock); notifier->v4l2_dev = NULL; /* @@ -274,6 +272,7 @@ EXPORT_SYMBOL(v4l2_async_notifier_unregister); int v4l2_async_register_subdev(struct v4l2_subdev *sd) { struct v4l2_async_notifier *notifier; + struct v4l2_async_notifier *tmp; /* * No reference taken. The reference is held by the device @@ -283,24 +282,25 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) if (!sd->of_node && sd->dev) sd->of_node = sd->dev->of_node; - mutex_lock(&list_lock); - INIT_LIST_HEAD(&sd->async_list); - list_for_each_entry(notifier, ¬ifier_list, list) { - struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, sd); + list_for_each_entry_safe(notifier, tmp, ¬ifier_list, list) { + struct v4l2_async_subdev *asd; + + /* TODO: FIXME: if this is called by ->bound() we will also iterate over the locked notifier */ + mutex_lock_nested(¬ifier->lock, SINGLE_DEPTH_NESTING); + asd = v4l2_async_belongs(notifier, sd); if (asd) { int ret = v4l2_async_test_notify(notifier, sd, asd); - mutex_unlock(&list_lock); + mutex_unlock(¬ifier->lock); return ret; } + mutex_unlock(¬ifier->lock); } /* None matched, wait for hot-plugging */ list_add(&sd->async_list, &subdev_list); - mutex_unlock(&list_lock); - return 0; } EXPORT_SYMBOL(v4l2_async_register_subdev); @@ -315,7 +315,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) return; } - mutex_lock(&list_lock); + mutex_lock_nested(¬ifier->lock, SINGLE_DEPTH_NESTING); list_add(&sd->asd->list, ¬ifier->waiting); @@ -324,6 +324,6 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) if (notifier->unbind) notifier->unbind(notifier, sd, sd->asd); - mutex_unlock(&list_lock); + mutex_unlock(¬ifier->lock); } EXPORT_SYMBOL(v4l2_async_unregister_subdev); diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h index 8e2a236..690a81f 100644 --- a/include/media/v4l2-async.h +++ b/include/media/v4l2-async.h @@ -84,6 +84,7 @@ struct v4l2_async_subdev { * @waiting: list of struct v4l2_async_subdev, waiting for their drivers * @done: list of struct v4l2_subdev, already probed * @list: member in a global list of notifiers + * @lock: lock hold when the notifier is being processed * @bound: a subdevice driver has successfully probed one of subdevices * @complete: all subdevices have been probed successfully * @unbind: a subdevice is leaving @@ -95,6 +96,7 @@ struct v4l2_async_notifier { struct list_head waiting; struct list_head done; struct list_head list; + struct mutex lock; int (*bound)(struct v4l2_async_notifier *notifier, struct v4l2_subdev *subdev, struct v4l2_async_subdev *asd);