Re: [PATCH v13 14/25] v4l: async: Allow binding notifiers to sub-devices

[Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>]

Hi Sakari,

Thank you for the patch.

On Friday, 15 September 2017 17:17:13 EEST Sakari Ailus wrote:
> Registering a notifier has required the knowledge of struct v4l2_device
> for the reason that sub-devices generally are registered to the
> v4l2_device (as well as the media device, also available through
> v4l2_device).
> 
> This information is not available for sub-device drivers at probe time.
> 
> What this patch does is that it allows registering notifiers without
> having v4l2_device around. Instead the sub-device pointer is stored in the
> notifier. Once the sub-device of the driver that registered the notifier
> is registered, the notifier will gain the knowledge of the v4l2_device,
> and the binding of async sub-devices from the sub-device driver's notifier
> may proceed.
> 
> The root notifier's complete callback is only called when all sub-device
> notifiers are completed.

This is a bit hard to review, shouldn't it be split in two patches, one that 
refactors the functions, and another one that allows binding notifiers to 
subdevs ?

> Signed-off-by: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 218 +++++++++++++++++++++++++++-----
>  include/media/v4l2-async.h           |  16 ++-
>  2 files changed, 203 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index 4be2f16af051..52fe22b9b6b4
> 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -53,6 +53,10 @@ static int v4l2_async_notifier_call_complete(struct
> v4l2_async_notifier *n) return n->ops->complete(n);
>  }
> 
> +static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> +				   struct v4l2_subdev *sd,
> +				   struct v4l2_async_subdev *asd);

Forward declarations are often a sign that something is wrong :-/ If you 
really need to keep this I'd move it right before the function that needs it.

>  static bool match_i2c(struct v4l2_subdev *sd, struct v4l2_async_subdev
> *asd) {
>  #if IS_ENABLED(CONFIG_I2C)
> @@ -124,14 +128,127 @@ static struct v4l2_async_subdev
> *v4l2_async_find_match( return NULL;
>  }
> 
> +/* Find the sub-device notifier registered by a sub-device driver. */
> +static struct v4l2_async_notifier *v4l2_async_find_subdev_notifier(
> +	struct v4l2_subdev *sd)
> +{
> +	struct v4l2_async_notifier *n;
> +
> +	list_for_each_entry(n, &notifier_list, list)
> +		if (n->sd == sd)
> +			return n;
> +
> +	return NULL;
> +}
> +
> +/* Return true if all sub-device notifiers are complete, false otherwise.
> */ 
> +static bool v4l2_async_subdev_notifiers_complete(
> +	struct v4l2_async_notifier *notifier)
> +{
> +	struct v4l2_subdev *sd;
> +
> +	if (!list_empty(&notifier->waiting))
> +		return false;
> +
> +	list_for_each_entry(sd, &notifier->done, async_list) {
> +		struct v4l2_async_notifier *subdev_notifier =
> +			v4l2_async_find_subdev_notifier(sd);
> +
> +		if (subdev_notifier &&
> +		    !v4l2_async_subdev_notifiers_complete(subdev_notifier))
> +			return false;

This will loop forever if two subdevs add each other to their respective 
notifiers. We might not have any use case for that right now, but it's bound 
to happen, at least as a bug during development, and an infinite loop (with an 
additional stack overflow bonus) isn't very nice to debug.

> +	}
> +
> +	return true;
> +}
> +
> +/* Get v4l2_device related to the notifier if one can be found. */
> +static struct v4l2_device *v4l2_async_notifier_find_v4l2_dev(
> +	struct v4l2_async_notifier *notifier)
> +{
> +	while (notifier->parent)
> +		notifier = notifier->parent;
> +
> +	return notifier->v4l2_dev;
> +}
> +
> +/* Test all async sub-devices in a notifier for a match. */
> +static int v4l2_async_notifier_try_all_subdevs(
> +	struct v4l2_async_notifier *notifier)
> +{
> +	struct v4l2_subdev *sd;
> +
> +	if (!v4l2_async_notifier_find_v4l2_dev(notifier))
> +		return 0;
> +
> +again:
> +	list_for_each_entry(sd, &subdev_list, async_list) {
> +		struct v4l2_async_subdev *asd;
> +		int ret;
> +
> +		asd = v4l2_async_find_match(notifier, sd);
> +		if (!asd)
> +			continue;
> +
> +		ret = v4l2_async_match_notify(notifier, sd, asd);
> +		if (ret < 0)
> +			return ret;
> +
> +		/*
> +		 * v4l2_async_match_notify() may lead to registering a
> +		 * new notifier and thus changing the async subdevs
> +		 * list. In order to proceed safely from here, restart
> +		 * parsing the list from the beginning.
> +		 */
> +		goto again;
> +	}
> +
> +	return 0;
> +}
> +
> +/* Try completing a notifier. */
> +static int v4l2_async_notifier_try_complete(
> +	struct v4l2_async_notifier *notifier)
> +{
> +	do {
> +		int ret;
> +
> +		/* Any local async sub-devices left? */
> +		if (!list_empty(&notifier->waiting))
> +			return 0;
> +
> +		/*
> +		 * Any sub-device notifiers waiting for async subdevs
> +		 * to be bound?
> +		 */
> +		if (!v4l2_async_subdev_notifiers_complete(notifier))
> +			return 0;
> +
> +		/* Proceed completing the notifier */
> +		ret = v4l2_async_notifier_call_complete(notifier);
> +		if (ret < 0)
> +			return ret;
> +
> +		/*
> +		 * Obtain notifier's parent. If there is one, repeat
> +		 * the process, otherwise we're done here.
> +		 */
> +		notifier = notifier->parent;
> +	} while (notifier);
> +
> +	return 0;
> +}
> +
>  static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_subdev *sd,
>  				   struct v4l2_async_subdev *asd)
>  {
> +	struct v4l2_async_notifier *subdev_notifier;
>  	int ret;
> 
> -	ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> -	if (ret < 0)
> +	ret = v4l2_device_register_subdev(
> +		v4l2_async_notifier_find_v4l2_dev(notifier), sd);
> +	if (ret)
>  		return ret;
> 
>  	ret = v4l2_async_notifier_call_bound(notifier, sd, asd);
> @@ -148,10 +265,20 @@ static int v4l2_async_match_notify(struct
> v4l2_async_notifier *notifier, /* Move from the global subdevice list to
> notifier's done */
>  	list_move(&sd->async_list, &notifier->done);
> 
> -	if (list_empty(&notifier->waiting))
> -		return v4l2_async_notifier_call_complete(notifier);
> +	/*
> +	 * See if the sub-device has a notifier. If it does, proceed
> +	 * with checking for its async sub-devices.
> +	 */
> +	subdev_notifier = v4l2_async_find_subdev_notifier(sd);
> +	if (subdev_notifier && !subdev_notifier->parent) {
> +		subdev_notifier->parent = notifier;
> +		ret = v4l2_async_notifier_try_all_subdevs(subdev_notifier);
> +		if (ret)
> +			return ret;
> +	}
> 
> -	return 0;
> +	/* Try completing the notifier and its parent(s). */
> +	return v4l2_async_notifier_try_complete(notifier);
>  }
> 
>  static void v4l2_async_cleanup(struct v4l2_subdev *sd)
> @@ -163,17 +290,15 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd)
> sd->dev = NULL;
>  }
> 
> -int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> -				 struct v4l2_async_notifier *notifier)
> +static int __v4l2_async_notifier_register(struct v4l2_async_notifier
> *notifier) {
> -	struct v4l2_subdev *sd, *tmp;
>  	struct v4l2_async_subdev *asd;
> +	int ret;
>  	int i;
> 
> -	if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS)
> +	if (notifier->num_subdevs > V4L2_MAX_SUBDEVS)
>  		return -EINVAL;
> 
> -	notifier->v4l2_dev = v4l2_dev;
>  	INIT_LIST_HEAD(&notifier->waiting);
>  	INIT_LIST_HEAD(&notifier->done);
> 
> @@ -200,18 +325,10 @@ int v4l2_async_notifier_register(struct v4l2_device
> *v4l2_dev,
> 
>  	mutex_lock(&list_lock);
> 
> -	list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) {
> -		int ret;
> -
> -		asd = v4l2_async_find_match(notifier, sd);
> -		if (!asd)
> -			continue;
> -
> -		ret = v4l2_async_match_notify(notifier, sd, asd);
> -		if (ret < 0) {
> -			mutex_unlock(&list_lock);
> -			return ret;
> -		}
> +	ret = v4l2_async_notifier_try_all_subdevs(notifier);
> +	if (ret) {
> +		mutex_unlock(&list_lock);
> +		return ret;
>  	}
> 
>  	/* Keep also completed notifiers on the list */
> @@ -221,29 +338,70 @@ int v4l2_async_notifier_register(struct v4l2_device
> *v4l2_dev,
> 
>  	return 0;
>  }
> +
> +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> +				 struct v4l2_async_notifier *notifier)
> +{
> +	if (WARN_ON(!v4l2_dev || notifier->sd))
> +		return -EINVAL;
> +
> +	notifier->v4l2_dev = v4l2_dev;
> +
> +	return __v4l2_async_notifier_register(notifier);
> +}
>  EXPORT_SYMBOL(v4l2_async_notifier_register);
> 
> -void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> +int v4l2_async_subdev_notifier_register(struct v4l2_subdev *sd,
> +					struct v4l2_async_notifier *notifier)
>  {
> -	struct v4l2_subdev *sd, *tmp;
> +	if (WARN_ON(!sd || notifier->v4l2_dev))
> +		return -EINVAL;
> 
> -	if (!notifier->v4l2_dev)
> -		return;
> +	notifier->sd = sd;
> 
> -	mutex_lock(&list_lock);
> +	return __v4l2_async_notifier_register(notifier);
> +}
> +EXPORT_SYMBOL(v4l2_async_subdev_notifier_register);
> 
> -	list_del(&notifier->list);
> +/* Unbind all sub-devices in the notifier tree. */
> +static void v4l2_async_notifier_unbind_all_subdevs(
> +	struct v4l2_async_notifier *notifier)
> +{
> +	struct v4l2_subdev *sd, *tmp;
> 
>  	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
> +		struct v4l2_async_notifier *subdev_notifier =
> +			v4l2_async_find_subdev_notifier(sd);
> +
> +		if (subdev_notifier)
> +			v4l2_async_notifier_unbind_all_subdevs(subdev_notifier);
> +
>  		v4l2_async_cleanup(sd);
> 
>  		v4l2_async_notifier_call_unbind(notifier, sd, sd->asd);
> -	}
> 
> -	mutex_unlock(&list_lock);
> +		list_del(&sd->async_list);
> +		list_add(&sd->async_list, &subdev_list);

How about list_move() ?

This seems to be new code, and by the look of it, I wonder whether it doesn't 
belong in the reprobing removal patch.

> +	}
> 
> +	notifier->parent = NULL;
> +	notifier->sd = NULL;
>  	notifier->v4l2_dev = NULL;
>  }
> +
> +void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> +{
> +	if (!notifier->v4l2_dev && !notifier->sd)
> +		return;
> +
> +	mutex_lock(&list_lock);
> +
> +	v4l2_async_notifier_unbind_all_subdevs(notifier);
> +
> +	list_del(&notifier->list);
> +
> +	mutex_unlock(&list_lock);
> +}
>  EXPORT_SYMBOL(v4l2_async_notifier_unregister);
> 
>  void v4l2_async_notifier_release(struct v4l2_async_notifier *notifier)

[snip]

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html