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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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;
>  

-- 
Regards,

Laurent Pinchart