Re: [PATCH v16 19/32] v4l: async: Ensure only unique fwnodes are registered to notifiers

Niklas Söderlund
2017-10-27T09:52:27Z

Hi Sakari,

Thanks for your patch.

On 2017-10-26 10:53:29 +0300, Sakari Ailus wrote:
> While registering a notifier, check that each newly added fwnode is
> unique, and return an error if it is not. Also check that a newly added
> notifier does not have the same fwnodes twice.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> drivers/media/v4l2-core/v4l2-async.c | 82 +++++++++++++++++++++++++++++++++---
> 1 file changed, 77 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index ed539c4fd5dc..b4e88eef195f 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -308,8 +308,71 @@ static void v4l2_async_notifier_unbind_all_subdevs(
> notifier->parent = NULL;
> }
>
> +/* See if an fwnode can be found in a notifier's lists. */
> +static bool __v4l2_async_notifier_fwnode_has_async_subdev(
> + struct v4l2_async_notifier *notifier, struct fwnode_handle *fwnode)
> +{
> + struct v4l2_async_subdev *asd;
> + struct v4l2_subdev *sd;
> +
> + list_for_each_entry(asd, &notifier->waiting, list) {
> + if (asd->match_type != V4L2_ASYNC_MATCH_FWNODE)
> + continue;
> +
> + if (asd->match.fwnode.fwnode == fwnode)
> + return true;
> + }
> +
> + list_for_each_entry(sd, &notifier->done, async_list) {
> + if (WARN_ON(!sd->asd))
> + continue;
> +
> + if (sd->asd->match_type != V4L2_ASYNC_MATCH_FWNODE)
> + continue;
> +
> + if (sd->asd->match.fwnode.fwnode == fwnode)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/*
> + * Find out whether an async sub-device was set up for an fwnode already or
> + * whether it exists in a given notifier before @this_index.
> + */
> +static bool v4l2_async_notifier_fwnode_has_async_subdev(
> + struct v4l2_async_notifier *notifier, struct fwnode_handle *fwnode,
> + unsigned int this_index)
> +{
> + unsigned int j;
> +
> + lockdep_assert_held(&list_lock);
> +
> + /* Check that an fwnode is not being added more than once. */
> + for (j = 0; j < this_index; j++) {
> + struct v4l2_async_subdev *asd = notifier->subdevs[this_index];
> + struct v4l2_async_subdev *other_asd = notifier->subdevs[j];
> +
> + if (other_asd->match_type == V4L2_ASYNC_MATCH_FWNODE &&
> + asd->match.fwnode.fwnode ==
> + other_asd->match.fwnode.fwnode)
> + return true;
> + }
> +
> + /* Check than an fwnode did not exist in other notifiers. */
> + list_for_each_entry(notifier, &notifier_list, list)
> + if (__v4l2_async_notifier_fwnode_has_async_subdev(
> + notifier, fwnode))
> + return true;
> +
> + return false;
> +}
> +
> static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> {
> + struct device *dev =
> + notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
> struct v4l2_async_subdev *asd;
> int ret;
> int i;
> @@ -320,6 +383,8 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> INIT_LIST_HEAD(&notifier->waiting);
> INIT_LIST_HEAD(&notifier->done);
>
> + mutex_lock(&list_lock);
> +
> for (i = 0; i < notifier->num_subdevs; i++) {
> asd = notifier->subdevs[i];
>
> @@ -327,19 +392,25 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> case V4L2_ASYNC_MATCH_CUSTOM:
> case V4L2_ASYNC_MATCH_DEVNAME:
> case V4L2_ASYNC_MATCH_I2C:
> + break;
> case V4L2_ASYNC_MATCH_FWNODE:
> + if (v4l2_async_notifier_fwnode_has_async_subdev(
> + notifier, asd->match.fwnode.fwnode, i)) {
> + dev_err(dev,
> + "fwnode has already been registered or in notifier's subdev list\n");
> + ret = -EEXIST;
> + goto out_unlock;

You store the error code in ret before the jump, but in the out_unlock
path ret is not considered and 0 is always returned.

> + }
> break;
> default:
> - dev_err(notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL,
> - "Invalid match type %u on %p\n",
> + dev_err(dev, "Invalid match type %u on %p\n",
> asd->match_type, asd);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out_unlock;

Same here.

> }
> list_add_tail(&asd->list, &notifier->waiting);
> }
>
> - mutex_lock(&list_lock);
> -
> ret = v4l2_async_notifier_try_all_subdevs(notifier);
> if (ret)
> goto err_unbind;
> @@ -351,6 +422,7 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> /* Keep also completed notifiers on the list */
> list_add(&notifier->list, &notifier_list);
>
> +out_unlock:
> mutex_unlock(&list_lock);
>
> return 0;
> --
> 2.11.0
>

--
Regards,
Niklas Söderlund