diff mbox

[v16,17/32] v4l: async: Prepare for async sub-device notifiers

Message ID 20171026075342.5760-18-sakari.ailus@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sakari Ailus Oct. 26, 2017, 7:53 a.m. UTC 
Refactor the V4L2 async framework a little in preparation for async
sub-device notifiers. This avoids making some structural changes in the
patch actually implementing sub-device notifiers, making that patch easier
to review.

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 | 71 ++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 19 deletions(-)

Comments

Niklas Söderlund Oct. 26, 2017, 10:10 p.m. UTC | #1 
On 2017-10-26 10:53:27 +0300, Sakari Ailus wrote:
> Refactor the V4L2 async framework a little in preparation for async
> sub-device notifiers. This avoids making some structural changes in the
> patch actually implementing sub-device notifiers, making that patch easier
> to review.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/media/v4l2-core/v4l2-async.c | 71 ++++++++++++++++++++++++++----------
>  1 file changed, 52 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 1b536d68cedf..eb31d96254d1 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -125,12 +125,13 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
>  }
>  
>  static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> +				   struct v4l2_device *v4l2_dev,
>  				   struct v4l2_subdev *sd,
>  				   struct v4l2_async_subdev *asd)
>  {
>  	int ret;
>  
> -	ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> +	ret = v4l2_device_register_subdev(v4l2_dev, sd);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -151,6 +152,31 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>  	return 0;
>  }
>  
> +/* 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_device *v4l2_dev = notifier->v4l2_dev;
> +	struct v4l2_subdev *sd, *tmp;
> +
> +	list_for_each_entry_safe(sd, tmp, &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, v4l2_dev, sd, asd);
> +		if (ret < 0) {
> +			mutex_unlock(&list_lock);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static void v4l2_async_cleanup(struct v4l2_subdev *sd)
>  {
>  	v4l2_device_unregister_subdev(sd);
> @@ -172,18 +198,15 @@ static void v4l2_async_notifier_unbind_all_subdevs(
>  	}
>  }
>  
> -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);
>  
> @@ -216,18 +239,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;
>  	}
>  
>  	if (list_empty(&notifier->waiting)) {
> @@ -250,6 +265,23 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
>  
>  	return ret;
>  }
> +
> +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> +				 struct v4l2_async_notifier *notifier)
> +{
> +	int ret;
> +
> +	if (WARN_ON(!v4l2_dev))
> +		return -EINVAL;
> +
> +	notifier->v4l2_dev = v4l2_dev;
> +
> +	ret = __v4l2_async_notifier_register(notifier);
> +	if (ret)
> +		notifier->v4l2_dev = NULL;
> +
> +	return ret;
> +}
>  EXPORT_SYMBOL(v4l2_async_notifier_register);
>  
>  void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> @@ -324,7 +356,8 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  		if (!asd)
>  			continue;
>  
> -		ret = v4l2_async_match_notify(notifier, sd, asd);
> +		ret = v4l2_async_match_notify(notifier, notifier->v4l2_dev, sd,
> +					      asd);
>  		if (ret)
>  			goto err_unlock;
>  
> -- 
> 2.11.0
>
Niklas Söderlund Oct. 27, 2017, 8:10 a.m. UTC | #2 
Hi Sakari,

On 2017-10-27 00:10:51 +0200, Niklas Söderlund wrote:
> On 2017-10-26 10:53:27 +0300, Sakari Ailus wrote:
> > Refactor the V4L2 async framework a little in preparation for async
> > sub-device notifiers. This avoids making some structural changes in the
> > patch actually implementing sub-device notifiers, making that patch easier
> > to review.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Acked-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

I'm sorry I acted to soon, I think I found a small issue with this 
patch, see bellow. With that fixed feel free to attach my ack.

> 
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 71 ++++++++++++++++++++++++++----------
> >  1 file changed, 52 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index 1b536d68cedf..eb31d96254d1 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -125,12 +125,13 @@ static struct v4l2_async_subdev *v4l2_async_find_match(
> >  }
> >  
> >  static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> > +				   struct v4l2_device *v4l2_dev,
> >  				   struct v4l2_subdev *sd,
> >  				   struct v4l2_async_subdev *asd)
> >  {
> >  	int ret;
> >  
> > -	ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
> > +	ret = v4l2_device_register_subdev(v4l2_dev, sd);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -151,6 +152,31 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> >  	return 0;
> >  }
> >  
> > +/* 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_device *v4l2_dev = notifier->v4l2_dev;
> > +	struct v4l2_subdev *sd, *tmp;
> > +
> > +	list_for_each_entry_safe(sd, tmp, &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, v4l2_dev, sd, asd);
> > +		if (ret < 0) {
> > +			mutex_unlock(&list_lock);

The mutex should not be unlocked here, as the caller will also unlock it 
if ret is none zero. You fix this in 18/32 so the end result is OK but I 
think its better to fix this in this patch.

> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static void v4l2_async_cleanup(struct v4l2_subdev *sd)
> >  {
> >  	v4l2_device_unregister_subdev(sd);
> > @@ -172,18 +198,15 @@ static void v4l2_async_notifier_unbind_all_subdevs(
> >  	}
> >  }
> >  
> > -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);
> >  
> > @@ -216,18 +239,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;
> >  	}
> >  
> >  	if (list_empty(&notifier->waiting)) {
> > @@ -250,6 +265,23 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> >  
> >  	return ret;
> >  }
> > +
> > +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> > +				 struct v4l2_async_notifier *notifier)
> > +{
> > +	int ret;
> > +
> > +	if (WARN_ON(!v4l2_dev))
> > +		return -EINVAL;
> > +
> > +	notifier->v4l2_dev = v4l2_dev;
> > +
> > +	ret = __v4l2_async_notifier_register(notifier);
> > +	if (ret)
> > +		notifier->v4l2_dev = NULL;
> > +
> > +	return ret;
> > +}
> >  EXPORT_SYMBOL(v4l2_async_notifier_register);
> >  
> >  void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> > @@ -324,7 +356,8 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> >  		if (!asd)
> >  			continue;
> >  
> > -		ret = v4l2_async_match_notify(notifier, sd, asd);
> > +		ret = v4l2_async_match_notify(notifier, notifier->v4l2_dev, sd,
> > +					      asd);
> >  		if (ret)
> >  			goto err_unlock;
> >  
> > -- 
> > 2.11.0
> > 
> 
> -- 
> Regards,
> Niklas Söderlund
Sakari Ailus Oct. 27, 2017, 8:30 a.m. UTC | #3 
On Fri, Oct 27, 2017 at 10:10:02AM +0200, Niklas Söderlund wrote:
> > > @@ -151,6 +152,31 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
> > >  	return 0;
> > >  }
> > >  
> > > +/* 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_device *v4l2_dev = notifier->v4l2_dev;
> > > +	struct v4l2_subdev *sd, *tmp;
> > > +
> > > +	list_for_each_entry_safe(sd, tmp, &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, v4l2_dev, sd, asd);
> > > +		if (ret < 0) {
> > > +			mutex_unlock(&list_lock);
> 
> The mutex should not be unlocked here, as the caller will also unlock it 
> if ret is none zero. You fix this in 18/32 so the end result is OK but I 
> think its better to fix this in this patch.

Good catch. I've sent v16.1 of these and forgot the change log. There are
no other changes than fixing this, i.e. the end result is the same.
diff mbox

Patch

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 1b536d68cedf..eb31d96254d1 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -125,12 +125,13 @@  static struct v4l2_async_subdev *v4l2_async_find_match(
 }
 
 static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
+				   struct v4l2_device *v4l2_dev,
 				   struct v4l2_subdev *sd,
 				   struct v4l2_async_subdev *asd)
 {
 	int ret;
 
-	ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd);
+	ret = v4l2_device_register_subdev(v4l2_dev, sd);
 	if (ret < 0)
 		return ret;
 
@@ -151,6 +152,31 @@  static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
 	return 0;
 }
 
+/* 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_device *v4l2_dev = notifier->v4l2_dev;
+	struct v4l2_subdev *sd, *tmp;
+
+	list_for_each_entry_safe(sd, tmp, &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, v4l2_dev, sd, asd);
+		if (ret < 0) {
+			mutex_unlock(&list_lock);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static void v4l2_async_cleanup(struct v4l2_subdev *sd)
 {
 	v4l2_device_unregister_subdev(sd);
@@ -172,18 +198,15 @@  static void v4l2_async_notifier_unbind_all_subdevs(
 	}
 }
 
-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);
 
@@ -216,18 +239,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;
 	}
 
 	if (list_empty(&notifier->waiting)) {
@@ -250,6 +265,23 @@  int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
 
 	return ret;
 }
+
+int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
+				 struct v4l2_async_notifier *notifier)
+{
+	int ret;
+
+	if (WARN_ON(!v4l2_dev))
+		return -EINVAL;
+
+	notifier->v4l2_dev = v4l2_dev;
+
+	ret = __v4l2_async_notifier_register(notifier);
+	if (ret)
+		notifier->v4l2_dev = NULL;
+
+	return ret;
+}
 EXPORT_SYMBOL(v4l2_async_notifier_register);
 
 void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
@@ -324,7 +356,8 @@  int v4l2_async_register_subdev(struct v4l2_subdev *sd)
 		if (!asd)
 			continue;
 
-		ret = v4l2_async_match_notify(notifier, sd, asd);
+		ret = v4l2_async_match_notify(notifier, notifier->v4l2_dev, sd,
+					      asd);
 		if (ret)
 			goto err_unlock;