Re: [RESEND PATCH v3 16/32] media: v4l: async: Drop duplicate handling when adding connections

[Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>]

Hi Sakari,

On Thu, May 25, 2023 at 12:15:59PM +0300, Sakari Ailus wrote:
> The connections are checked for duplicates already when the notifier is
> registered. This is effectively a sanity check for driver (and possibly
> obscure firmware) bugs. Don't do this when adding the connection.

Isn't it better to have this sanity check when the connection is added,
instead of later when the notifier is registered ? The latter is more
difficult to debug. If you want to avoid duplicate checks, could we drop
the one at notifier registration time ?

> Retain the int return type for now. It'll be needed very soon again.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 23 +++++++----------------
>  1 file changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index f51f0c37210c9..5dfc6d5f6a7c3 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -475,8 +475,7 @@ v4l2_async_nf_has_async_match_entry(struct v4l2_async_notifier *notifier,
>   */
>  static bool
>  v4l2_async_nf_has_async_match(struct v4l2_async_notifier *notifier,
> -			      struct v4l2_async_match_desc *match,
> -			      bool skip_self)
> +			      struct v4l2_async_match_desc *match)
>  {
>  	struct v4l2_async_connection *asc;
>  
> @@ -484,7 +483,7 @@ v4l2_async_nf_has_async_match(struct v4l2_async_notifier *notifier,
>  
>  	/* Check that an asd is not being added more than once. */
>  	list_for_each_entry(asc, &notifier->asc_list, asc_entry) {
> -		if (skip_self && &asc->match == match)
> +		if (&asc->match == match)
>  			break;
>  		if (v4l2_async_match_equal(&asc->match, match))
>  			return true;
> @@ -499,16 +498,14 @@ v4l2_async_nf_has_async_match(struct v4l2_async_notifier *notifier,
>  }
>  
>  static int v4l2_async_nf_match_valid(struct v4l2_async_notifier *notifier,
> -				     struct v4l2_async_match_desc *match,
> -				     bool skip_self)
> +				     struct v4l2_async_match_desc *match)
>  {
>  	struct device *dev = notifier_dev(notifier);
>  
>  	switch (match->type) {
>  	case V4L2_ASYNC_MATCH_TYPE_I2C:
>  	case V4L2_ASYNC_MATCH_TYPE_FWNODE:
> -		if (v4l2_async_nf_has_async_match(notifier, match,
> -						  skip_self)) {
> +		if (v4l2_async_nf_has_async_match(notifier, match)) {
>  			dev_dbg(dev, "v4l2-async: match descriptor already listed in a notifier\n");
>  			return -EEXIST;
>  		}
> @@ -539,7 +536,7 @@ static int __v4l2_async_nf_register(struct v4l2_async_notifier *notifier)
>  	mutex_lock(&list_lock);
>  
>  	list_for_each_entry(asc, &notifier->asc_list, asc_entry) {
> -		ret = v4l2_async_nf_match_valid(notifier, &asc->match, true);
> +		ret = v4l2_async_nf_match_valid(notifier, &asc->match);
>  		if (ret)
>  			goto err_unlock;
>  
> @@ -668,19 +665,13 @@ EXPORT_SYMBOL_GPL(v4l2_async_nf_cleanup);
>  static int __v4l2_async_nf_add_connection(struct v4l2_async_notifier *notifier,
>  					  struct v4l2_async_connection *asc)
>  {
> -	int ret;
> -
>  	mutex_lock(&list_lock);
>  
> -	ret = v4l2_async_nf_match_valid(notifier, &asc->match, false);
> -	if (ret)
> -		goto unlock;
> -
>  	list_add_tail(&asc->asc_entry, &notifier->asc_list);
>  
> -unlock:
>  	mutex_unlock(&list_lock);
> -	return ret;
> +
> +	return 0;
>  }
>  
>  struct v4l2_async_connection *

-- 
Regards,

Laurent Pinchart