Re: [PATCH] media: v4l2-async: fix binding async subdevs with multiple source ports

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Philipp,

On Wed, Aug 10, 2022 at 12:48:48PM +0200, Philipp Zabel wrote:
> Asynchronous subdevice probing on imx-media with imx6-mipi-csi2 is
> broken since commit 1f391df44607 ("media: v4l2-async: Use endpoints in
> __v4l2_async_nf_add_fwnode_remote()").
> 
> This is a side effect of imx6-mipi-csi2 having a single subdevice with
> four separate source ports connected to different subdevices. Before,
> the imx-media-csi and video-mux devices registered async sub-devices
> via device fwnode that matched the imx6-mipi-csi2 device on their
> respective notifiers. This caused the first asd to be put on the
> notifier waiting list, and the other three to return -EEXIST and be
> ignored.
> 
> With async subdev registration via endpoint fwnode, all four asds are
> distinct and three of them keep dangling on their notifiers after the
> first one is bound.
> 
> This patch modifies __v4l2_async_nf_has_async_subdev() to consider
> asds matching different fwnode endpoints of the same sub-device equal
> if the latter is already bound and matches via device fwnode. Further,
> v4l2_async_register_subdev() is modified to remove dangling duplicate
> asds that were registered before the sub-device was available to check
> its fwnode.
> 
> Fixes: 1f391df44607 ("media: v4l2-async: Use endpoints in __v4l2_async_nf_add_fwnode_remote()")
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 43 ++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 2f1b718a9189..b24220ed7077 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -452,6 +452,22 @@ __v4l2_async_nf_has_async_subdev(struct v4l2_async_notifier *notifier,
>  
>  		if (asd_equal(asd, sd->asd))
>  			return true;
> +
> +		/*
> +		 * If the asd matches an endpoint fwnode, handle sub-devices
> +		 * with device fwnode that were already matched by another asd
> +		 * with a different endpoint fwnode on the same device.
> +		 */
> +		if (asd->match_type == V4L2_ASYNC_MATCH_FWNODE &&
> +		    fwnode_graph_is_endpoint(asd->match.fwnode) &&
> +		    sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode)) {
> +			struct fwnode_handle *devnode;
> +
> +			devnode = fwnode_graph_get_port_parent(asd->match.fwnode);
> +			fwnode_handle_put(devnode);
> +			if (devnode == sd->fwnode)
> +				return true;
> +		}
>  	}
>  
>  	return false;
> @@ -749,6 +765,24 @@ __v4l2_async_nf_add_i2c(struct v4l2_async_notifier *notifier, int adapter_id,
>  }
>  EXPORT_SYMBOL_GPL(__v4l2_async_nf_add_i2c);
>  
> +static void v4l2_async_remove_duplicate_matches(struct v4l2_subdev *sd)
> +{
> +	struct v4l2_async_notifier *notifier;
> +
> +	lockdep_assert_held(&list_lock);
> +
> +	list_for_each_entry(notifier, &notifier_list, list) {
> +		struct v4l2_async_subdev *asd;
> +
> +		asd = v4l2_async_find_match(notifier, sd);
> +		if (!asd)
> +			continue;
> +
> +		/* Remove from the waiting list */
> +		list_del(&asd->list);

This leaves asd->list pointers dangling.

> +	}
> +}
> +
>  int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  {
>  	struct v4l2_async_notifier *subdev_notifier;
> @@ -783,6 +817,15 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd)
>  		if (ret)
>  			goto err_unbind;
>  
> +		/*
> +		 * At this point the asd is removed from the notifier wait list.
> +		 * There might be other notifiers with asds matching different
> +		 * fwnode endpoints of the same sd remaining. If the sd matches
> +		 * by device fwnode, remove the dangling asds.
> +		 */
> +		if (sd->fwnode && !fwnode_graph_is_endpoint(sd->fwnode))
> +			v4l2_async_remove_duplicate_matches(sd);
> +
>  		ret = v4l2_async_nf_try_complete(notifier);
>  		if (ret)
>  			goto err_unbind;

This patch is essentially a workaround, not a fix.

The root of the problem is that registering async sub-devices (and thus
registering a sub-device later) and link creation (via bound callback) are
handled together. They should be separated instead, at least in the
v4l2-async framework if not in the interfaces. We could at least have a
helper doing the both using existing API for devices that have a single
source pad.

While merging this might not break anything as such, it would certainly
make fixing the underlying problem later on much harder as you'd need to
take drivers depending on it into account --- for which I, for instance,
don't have hardware for.

I'm thus not overly enthusiastic of the idea of merging this.

-- 
Kind regards,

Sakari Ailus