diff mbox series

[05/18] media: v4l: async: Clean testing for duplicated async subdevs

Message ID 20230330115853.1628216-6-sakari.ailus@linux.intel.com (mailing list archive)
State Changes Requested
Delegated to: Sakari Ailus
Headers
Series Separate links and async sub-devices |

Commit Message

Sakari Ailus March 30, 2023, 11:58 a.m. UTC 
  There's a need to verify that a single async sub-device isn't being added
multiple times, this would be an error. This takes place at the time of
adding the async sub-device to the notifier's list as well as when the
notifier is added to the global notifier's list.

Use the pointer to the sub-device for testing this instead of an index to
an array that is long gone.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-async.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Jacopo Mondi April 13, 2023, 4:58 p.m. UTC | #1 
Hi Sakari

On Thu, Mar 30, 2023 at 02:58:40PM +0300, Sakari Ailus wrote:
> There's a need to verify that a single async sub-device isn't being added
> multiple times, this would be an error. This takes place at the time of
> adding the async sub-device to the notifier's list as well as when the
> notifier is added to the global notifier's list.
>
> Use the pointer to the sub-device for testing this instead of an index to
> an array that is long gone.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index bb78e3618ab5..fc9ae22e2b47 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -456,21 +456,19 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
>
>  /*
>   * Find out whether an async sub-device was set up already or
> - * whether it exists in a given notifier before @this_index.
> - * If @this_index < 0, search the notifier's entire @asd_list.
> + * whether it exists in a given notifier.
>   */
>  static bool
>  v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
> -			       struct v4l2_async_subdev *asd, int this_index)
> +			       struct v4l2_async_subdev *asd, bool skip_self)

is skip_self used ?

>  {
>  	struct v4l2_async_subdev *asd_y;
> -	int j = 0;
>
>  	lockdep_assert_held(&list_lock);
>
>  	/* Check that an asd is not being added more than once. */
>  	list_for_each_entry(asd_y, &notifier->asd_list, asd_list) {
> -		if (this_index >= 0 && j++ >= this_index)
> +		if (asd == asd_y)
>  			break;
>  		if (asd_equal(asd, asd_y))
>  			return true;
> @@ -486,7 +484,7 @@ v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
>
>  static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_async_subdev *asd,
> -				   int this_index)
> +				   bool skip_self)
>  {
>  	struct device *dev =
>  		notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
> @@ -497,7 +495,7 @@ static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier,
>  	switch (asd->match.type) {
>  	case V4L2_ASYNC_MATCH_I2C:
>  	case V4L2_ASYNC_MATCH_FWNODE:
> -		if (v4l2_async_nf_has_async_subdev(notifier, asd, this_index)) {
> +		if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) {
>  			dev_dbg(dev, "subdev descriptor already listed in this or other notifiers\n");
>  			return -EEXIST;
>  		}
> @@ -520,7 +518,7 @@ EXPORT_SYMBOL(v4l2_async_nf_init);
>  static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier)
>  {
>  	struct v4l2_async_subdev *asd;
> -	int ret, i = 0;
> +	int ret;
>
>  	INIT_LIST_HEAD(&notifier->waiting);
>  	INIT_LIST_HEAD(&notifier->done);
> @@ -528,7 +526,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier)
>  	mutex_lock(&list_lock);
>
>  	list_for_each_entry(asd, &notifier->asd_list, asd_list) {
> -		ret = v4l2_async_nf_asd_valid(notifier, asd, i++);
> +		ret = v4l2_async_nf_asd_valid(notifier, asd, true);
>  		if (ret)
>  			goto err_unlock;
>
> @@ -661,7 +659,7 @@ int __v4l2_async_nf_add_subdev(struct v4l2_async_notifier *notifier,
>
>  	mutex_lock(&list_lock);
>
> -	ret = v4l2_async_nf_asd_valid(notifier, asd, -1);
> +	ret = v4l2_async_nf_asd_valid(notifier, asd, false);
>  	if (ret)
>  		goto unlock;
>
> --
> 2.30.2
>
Sakari Ailus April 14, 2023, 11:16 a.m. UTC | #2 
Hi Jacopo,

On Thu, Apr 13, 2023 at 06:58:56PM +0200, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Thu, Mar 30, 2023 at 02:58:40PM +0300, Sakari Ailus wrote:
> > There's a need to verify that a single async sub-device isn't being added
> > multiple times, this would be an error. This takes place at the time of
> > adding the async sub-device to the notifier's list as well as when the
> > notifier is added to the global notifier's list.
> >
> > Use the pointer to the sub-device for testing this instead of an index to
> > an array that is long gone.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-async.c | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > index bb78e3618ab5..fc9ae22e2b47 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -456,21 +456,19 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
> >
> >  /*
> >   * Find out whether an async sub-device was set up already or
> > - * whether it exists in a given notifier before @this_index.
> > - * If @this_index < 0, search the notifier's entire @asd_list.
> > + * whether it exists in a given notifier.
> >   */
> >  static bool
> >  v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
> > -			       struct v4l2_async_subdev *asd, int this_index)
> > +			       struct v4l2_async_subdev *asd, bool skip_self)
> 
> is skip_self used ?

Yes, it should have been there. I'll add it for v2.
Laurent Pinchart April 25, 2023, 1:15 a.m. UTC | #3 
Hi Sakari,

Thank you for the patch.

On Thu, Mar 30, 2023 at 02:58:40PM +0300, Sakari Ailus wrote:
> There's a need to verify that a single async sub-device isn't being added
> multiple times, this would be an error. This takes place at the time of
> adding the async sub-device to the notifier's list as well as when the
> notifier is added to the global notifier's list.
> 
> Use the pointer to the sub-device for testing this instead of an index to
> an array that is long gone.

Reading the patch, I have no idea what the "long gone array" is. Could
you please expand the commit message to make this easier to review ?
v4l2-async is very difficult to follow in general, reviewing this series
is painful :-S Let's try to improve it with better commit messages.

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index bb78e3618ab5..fc9ae22e2b47 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -456,21 +456,19 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
>  
>  /*
>   * Find out whether an async sub-device was set up already or
> - * whether it exists in a given notifier before @this_index.
> - * If @this_index < 0, search the notifier's entire @asd_list.
> + * whether it exists in a given notifier.
>   */
>  static bool
>  v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
> -			       struct v4l2_async_subdev *asd, int this_index)
> +			       struct v4l2_async_subdev *asd, bool skip_self)
>  {
>  	struct v4l2_async_subdev *asd_y;
> -	int j = 0;
>  
>  	lockdep_assert_held(&list_lock);
>  
>  	/* Check that an asd is not being added more than once. */
>  	list_for_each_entry(asd_y, &notifier->asd_list, asd_list) {
> -		if (this_index >= 0 && j++ >= this_index)
> +		if (asd == asd_y)
>  			break;
>  		if (asd_equal(asd, asd_y))
>  			return true;
> @@ -486,7 +484,7 @@ v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
>  
>  static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_async_subdev *asd,
> -				   int this_index)
> +				   bool skip_self)
>  {
>  	struct device *dev =
>  		notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
> @@ -497,7 +495,7 @@ static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier,
>  	switch (asd->match.type) {
>  	case V4L2_ASYNC_MATCH_I2C:
>  	case V4L2_ASYNC_MATCH_FWNODE:
> -		if (v4l2_async_nf_has_async_subdev(notifier, asd, this_index)) {
> +		if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) {
>  			dev_dbg(dev, "subdev descriptor already listed in this or other notifiers\n");
>  			return -EEXIST;
>  		}
> @@ -520,7 +518,7 @@ EXPORT_SYMBOL(v4l2_async_nf_init);
>  static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier)
>  {
>  	struct v4l2_async_subdev *asd;
> -	int ret, i = 0;
> +	int ret;
>  
>  	INIT_LIST_HEAD(&notifier->waiting);
>  	INIT_LIST_HEAD(&notifier->done);
> @@ -528,7 +526,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier)
>  	mutex_lock(&list_lock);
>  
>  	list_for_each_entry(asd, &notifier->asd_list, asd_list) {
> -		ret = v4l2_async_nf_asd_valid(notifier, asd, i++);
> +		ret = v4l2_async_nf_asd_valid(notifier, asd, true);
>  		if (ret)
>  			goto err_unlock;
>  
> @@ -661,7 +659,7 @@ int __v4l2_async_nf_add_subdev(struct v4l2_async_notifier *notifier,
>  
>  	mutex_lock(&list_lock);
>  
> -	ret = v4l2_async_nf_asd_valid(notifier, asd, -1);
> +	ret = v4l2_async_nf_asd_valid(notifier, asd, false);
>  	if (ret)
>  		goto unlock;
>
Sakari Ailus April 27, 2023, 11:06 a.m. UTC | #4 
Hi Laurent,

On Tue, Apr 25, 2023 at 04:15:41AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Thu, Mar 30, 2023 at 02:58:40PM +0300, Sakari Ailus wrote:
> > There's a need to verify that a single async sub-device isn't being added
> > multiple times, this would be an error. This takes place at the time of
> > adding the async sub-device to the notifier's list as well as when the
> > notifier is added to the global notifier's list.
> > 
> > Use the pointer to the sub-device for testing this instead of an index to
> > an array that is long gone.
> 
> Reading the patch, I have no idea what the "long gone array" is. Could
> you please expand the commit message to make this easier to review ?

Yes... the async sub-devices were placed in an array earlier, that's what
the index was referring to. Although this could be an entry in a linked
list. Not how they are usually referred to though. This will go away
permanently later on in the set.

I'll add this to the commit message.

> v4l2-async is very difficult to follow in general, reviewing this series
> is painful :-S Let's try to improve it with better commit messages.
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index bb78e3618ab5..fc9ae22e2b47 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -456,21 +456,19 @@  __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
 
 /*
  * Find out whether an async sub-device was set up already or
- * whether it exists in a given notifier before @this_index.
- * If @this_index < 0, search the notifier's entire @asd_list.
+ * whether it exists in a given notifier.
  */
 static bool
 v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
-			       struct v4l2_async_subdev *asd, int this_index)
+			       struct v4l2_async_subdev *asd, bool skip_self)
 {
 	struct v4l2_async_subdev *asd_y;
-	int j = 0;
 
 	lockdep_assert_held(&list_lock);
 
 	/* Check that an asd is not being added more than once. */
 	list_for_each_entry(asd_y, &notifier->asd_list, asd_list) {
-		if (this_index >= 0 && j++ >= this_index)
+		if (asd == asd_y)
 			break;
 		if (asd_equal(asd, asd_y))
 			return true;
@@ -486,7 +484,7 @@  v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
 
 static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier,
 				   struct v4l2_async_subdev *asd,
-				   int this_index)
+				   bool skip_self)
 {
 	struct device *dev =
 		notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
@@ -497,7 +495,7 @@  static int v4l2_async_nf_asd_valid(struct v4l2_async_notifier *notifier,
 	switch (asd->match.type) {
 	case V4L2_ASYNC_MATCH_I2C:
 	case V4L2_ASYNC_MATCH_FWNODE:
-		if (v4l2_async_nf_has_async_subdev(notifier, asd, this_index)) {
+		if (v4l2_async_nf_has_async_subdev(notifier, asd, skip_self)) {
 			dev_dbg(dev, "subdev descriptor already listed in this or other notifiers\n");
 			return -EEXIST;
 		}
@@ -520,7 +518,7 @@  EXPORT_SYMBOL(v4l2_async_nf_init);
 static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier)
 {
 	struct v4l2_async_subdev *asd;
-	int ret, i = 0;
+	int ret;
 
 	INIT_LIST_HEAD(&notifier->waiting);
 	INIT_LIST_HEAD(&notifier->done);
@@ -528,7 +526,7 @@  static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier)
 	mutex_lock(&list_lock);
 
 	list_for_each_entry(asd, &notifier->asd_list, asd_list) {
-		ret = v4l2_async_nf_asd_valid(notifier, asd, i++);
+		ret = v4l2_async_nf_asd_valid(notifier, asd, true);
 		if (ret)
 			goto err_unlock;
 
@@ -661,7 +659,7 @@  int __v4l2_async_nf_add_subdev(struct v4l2_async_notifier *notifier,
 
 	mutex_lock(&list_lock);
 
-	ret = v4l2_async_nf_asd_valid(notifier, asd, -1);
+	ret = v4l2_async_nf_asd_valid(notifier, asd, false);
 	if (ret)
 		goto unlock;