Re: [RFC 06/13] v4l2-async: per notifier locking

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 7507 bytes --]

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(&notifier->waiting);
>  	INIT_LIST_HEAD(&notifier->done);
> +	mutex_init(&notifier->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, &notifier->waiting);
>  	}
>  
> -	mutex_lock(&list_lock);
> +	/* Keep also completed notifiers on the list */
> +	list_add(&notifier->list, &notifier_list);
> +	mutex_lock(&notifier->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(&notifier->lock);
>  
> -	/* Keep also completed notifiers on the list */
> -	list_add(&notifier->list, &notifier_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(&notifier->lock);
>  
>  	list_del(&notifier->list);
>  
> @@ -237,7 +234,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  			put_device(d);
>  	}
>  
> -	mutex_unlock(&list_lock);
> +	mutex_unlock(&notifier->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(&notifier->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, &notifier_list, list) {
> -		struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, sd);
> +	list_for_each_entry_safe(notifier, tmp, &notifier_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(&notifier->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(&notifier->lock);
>  			return ret;
>  		}
> +		mutex_unlock(&notifier->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(&notifier->lock, SINGLE_DEPTH_NESTING);
>  
>  	list_add(&sd->asd->list, &notifier->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(&notifier->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



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]