[v1,2/2] media: v4l2-subdev: Relax warnings in link validation

  Before v6.3, the v4l2_subdev_link_validate() helper would ignore links
whose source was a video device and sink a subdev. The helper was (and
still is) used by drivers in such cases, in particular for subdevs with
multiple sink pads, some connected to video devices and some to other
subdevs.

Then, commit a6b995ed03ff ("media: subdev: use streams in
v4l2_subdev_link_validate()") assumed the entities on the two sides of a
link are both subdevs, and caused crashes in drivers that use the helper
with subdev sink pads connected to video devices. Commit 55f1ecb11990
("media: v4l: subdev: Make link validation safer"), merged in v6.4,
fixed the crash by adding an explicit check with a pr_warn_once(),
mentioning a driver bug.

Links between a subdev and a video device need to be validated, and
v4l2_subdev_link_validate() can't handle that. Drivers typically handle
this validation manually at stream start time (either in the .streamon()
ioctl handler, or in the .start_streaming() vb2 queue operation),
instead of implementing a custom .link_validate() handler. Forbidding
usage of v4l2_subdev_link_validate() as the .link_validate() handler
would thus force all subdev drivers that mix source links to subdev and
video devices to implement a custom .link_validate() handler that
returns immediately for the video device links and call
v4l2_subdev_link_validate() for subdev links. This would create lots of
duplicated code for no real gain. Instead, relax the check in
v4l2_ctrl_modify_range() to ignore links from a video device to a subdev
without printing any warning.

Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 30 ++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)
  

Hi Laurent,

Thanks for the patch.

On Mon, Nov 13, 2023 at 12:17:18PM +0200, Laurent Pinchart wrote:
> Before v6.3, the v4l2_subdev_link_validate() helper would ignore links
> whose source was a video device and sink a subdev. The helper was (and
> still is) used by drivers in such cases, in particular for subdevs with
> multiple sink pads, some connected to video devices and some to other
> subdevs.
> 
> Then, commit a6b995ed03ff ("media: subdev: use streams in
> v4l2_subdev_link_validate()") assumed the entities on the two sides of a
> link are both subdevs, and caused crashes in drivers that use the helper
> with subdev sink pads connected to video devices. Commit 55f1ecb11990
> ("media: v4l: subdev: Make link validation safer"), merged in v6.4,
> fixed the crash by adding an explicit check with a pr_warn_once(),
> mentioning a driver bug.
> 
> Links between a subdev and a video device need to be validated, and
> v4l2_subdev_link_validate() can't handle that. Drivers typically handle
> this validation manually at stream start time (either in the .streamon()
> ioctl handler, or in the .start_streaming() vb2 queue operation),
> instead of implementing a custom .link_validate() handler. Forbidding

While some do the validation as part of the streamon callback, it'd be
nicer to move this to the link_validate callback instead: this is what the
callback is for. I'd presume not may drivers depend on
v4l2_subdev_link_validate() fail silently on non-subdevices as the issue
hasn't been reported before while the patch that seems to have broken this
was merged in 6.3.

Not failing silently in link_validate also ensures the validation gets
done: there have been drivers (more than one) that have simply missed the
link validation due to the issue (non-sub-device entity on one end) being
silently ignored by default.

> usage of v4l2_subdev_link_validate() as the .link_validate() handler
> would thus force all subdev drivers that mix source links to subdev and
> video devices to implement a custom .link_validate() handler that
> returns immediately for the video device links and call
> v4l2_subdev_link_validate() for subdev links. This would create lots of
> duplicated code for no real gain. Instead, relax the check in
> v4l2_ctrl_modify_range() to ignore links from a video device to a subdev
> without printing any warning.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 30 ++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 67d43206ce32..b00be1d57e05 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1356,15 +1356,31 @@ int v4l2_subdev_link_validate(struct media_link *link)
>  	struct v4l2_subdev *source_sd, *sink_sd;
>  	struct v4l2_subdev_state *source_state, *sink_state;
>  	bool states_locked;
> +	bool is_sink_subdev;
> +	bool is_source_subdev;
>  	int ret;
>  
> -	if (!is_media_entity_v4l2_subdev(link->sink->entity) ||
> -	    !is_media_entity_v4l2_subdev(link->source->entity)) {
> -		pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> -			     !is_media_entity_v4l2_subdev(link->sink->entity) ?
> -			     "sink" : "source",
> -			     link->source->entity->name, link->source->index,
> -			     link->sink->entity->name, link->sink->index);
> +	is_sink_subdev = is_media_entity_v4l2_subdev(link->sink->entity);
> +	is_source_subdev = is_media_entity_v4l2_subdev(link->source->entity);
> +
> +	if (!is_sink_subdev || !is_source_subdev) {
> +		/*
> +		 * Do not print the warning if the source is a video device and
> +		 * the sink a subdev. This is a valid use case, to allow usage
> +		 * of this helper by subdev drivers that have multiple sink
> +		 * pads, some connected to video devices and some connected to
> +		 * other subdevs. The video device to subdev link is typically
> +		 * validated manually by the driver at stream start time in such
> +		 * cases.
> +		 */
> +		if (!is_sink_subdev ||
> +		    !is_media_entity_v4l2_video_device(link->source->entity))
> +			pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> +				     !is_sink_subdev ? "sink" : "source",
> +				     link->source->entity->name,
> +				     link->source->index,
> +				     link->sink->entity->name,
> +				     link->sink->index);
>  		return 0;
>  	}
>
  

On Mon, Nov 13, 2023 at 11:06:34AM +0000, Sakari Ailus wrote:
> Hi Laurent,
> 
> Thanks for the patch.
> 
> On Mon, Nov 13, 2023 at 12:17:18PM +0200, Laurent Pinchart wrote:
> > Before v6.3, the v4l2_subdev_link_validate() helper would ignore links
> > whose source was a video device and sink a subdev. The helper was (and
> > still is) used by drivers in such cases, in particular for subdevs with
> > multiple sink pads, some connected to video devices and some to other
> > subdevs.
> > 
> > Then, commit a6b995ed03ff ("media: subdev: use streams in
> > v4l2_subdev_link_validate()") assumed the entities on the two sides of a
> > link are both subdevs, and caused crashes in drivers that use the helper
> > with subdev sink pads connected to video devices. Commit 55f1ecb11990
> > ("media: v4l: subdev: Make link validation safer"), merged in v6.4,
> > fixed the crash by adding an explicit check with a pr_warn_once(),
> > mentioning a driver bug.
> > 
> > Links between a subdev and a video device need to be validated, and
> > v4l2_subdev_link_validate() can't handle that. Drivers typically handle
> > this validation manually at stream start time (either in the .streamon()
> > ioctl handler, or in the .start_streaming() vb2 queue operation),
> > instead of implementing a custom .link_validate() handler. Forbidding
> 
> While some do the validation as part of the streamon callback, it'd be
> nicer to move this to the link_validate callback instead: this is what the
> callback is for.

Is there any driver that does so already ?

> I'd presume not may drivers depend on
> v4l2_subdev_link_validate() fail silently on non-subdevices as the issue
> hasn't been reported before while the patch that seems to have broken this
> was merged in 6.3.

At least the VSP1 driver depends on it. The i.MX8MP ISI driver does as
well, it got merged in v6.4, so the crash in v6.3 didn't get noticed.
I'd be surprised if the omap3isp driver also didn't depend on this
behaviour when operating in M2M mode.

> Not failing silently in link_validate also ensures the validation gets
> done: there have been drivers (more than one) that have simply missed the
> link validation due to the issue (non-sub-device entity on one end) being
> silently ignored by default.

If there's a consensus that subdev to video device link validation need
to be done in the .link_validate() operation, then work needs to be done
to make that happen. I'd like to see at least one proof of concept
implemented, to make sure there are no unforeseen issues. Are you
volunteering ? :-)

In the meantime, most drivers (possibly all) validate that link manually
outside of the .link_validate() operation, so this is the de facto
typical mode of operation. I don't think it can be considered as a
driver bug until we make a reasonable effort to convert drivers to a new
model. It's not fair to just add a pr_warn_once() without having done
some of the work needed to move to a new model.

> > usage of v4l2_subdev_link_validate() as the .link_validate() handler
> > would thus force all subdev drivers that mix source links to subdev and
> > video devices to implement a custom .link_validate() handler that
> > returns immediately for the video device links and call
> > v4l2_subdev_link_validate() for subdev links. This would create lots of
> > duplicated code for no real gain. Instead, relax the check in
> > v4l2_ctrl_modify_range() to ignore links from a video device to a subdev
> > without printing any warning.
> > 
> > Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-subdev.c | 30 ++++++++++++++++++++-------
> >  1 file changed, 23 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 67d43206ce32..b00be1d57e05 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -1356,15 +1356,31 @@ int v4l2_subdev_link_validate(struct media_link *link)
> >  	struct v4l2_subdev *source_sd, *sink_sd;
> >  	struct v4l2_subdev_state *source_state, *sink_state;
> >  	bool states_locked;
> > +	bool is_sink_subdev;
> > +	bool is_source_subdev;
> >  	int ret;
> >  
> > -	if (!is_media_entity_v4l2_subdev(link->sink->entity) ||
> > -	    !is_media_entity_v4l2_subdev(link->source->entity)) {
> > -		pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> > -			     !is_media_entity_v4l2_subdev(link->sink->entity) ?
> > -			     "sink" : "source",
> > -			     link->source->entity->name, link->source->index,
> > -			     link->sink->entity->name, link->sink->index);
> > +	is_sink_subdev = is_media_entity_v4l2_subdev(link->sink->entity);
> > +	is_source_subdev = is_media_entity_v4l2_subdev(link->source->entity);
> > +
> > +	if (!is_sink_subdev || !is_source_subdev) {
> > +		/*
> > +		 * Do not print the warning if the source is a video device and
> > +		 * the sink a subdev. This is a valid use case, to allow usage
> > +		 * of this helper by subdev drivers that have multiple sink
> > +		 * pads, some connected to video devices and some connected to
> > +		 * other subdevs. The video device to subdev link is typically
> > +		 * validated manually by the driver at stream start time in such
> > +		 * cases.
> > +		 */
> > +		if (!is_sink_subdev ||
> > +		    !is_media_entity_v4l2_video_device(link->source->entity))
> > +			pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> > +				     !is_sink_subdev ? "sink" : "source",
> > +				     link->source->entity->name,
> > +				     link->source->index,
> > +				     link->sink->entity->name,
> > +				     link->sink->index);
> >  		return 0;
> >  	}
> >
  

On 13/11/2023 12:17, Laurent Pinchart wrote:
> Before v6.3, the v4l2_subdev_link_validate() helper would ignore links
> whose source was a video device and sink a subdev. The helper was (and

Didn't it also ignore links where the sink is a video device?

> still is) used by drivers in such cases, in particular for subdevs with
> multiple sink pads, some connected to video devices and some to other
> subdevs.
> 
> Then, commit a6b995ed03ff ("media: subdev: use streams in
> v4l2_subdev_link_validate()") assumed the entities on the two sides of a
> link are both subdevs, and caused crashes in drivers that use the helper
> with subdev sink pads connected to video devices. Commit 55f1ecb11990
> ("media: v4l: subdev: Make link validation safer"), merged in v6.4,
> fixed the crash by adding an explicit check with a pr_warn_once(),
> mentioning a driver bug.
> 
> Links between a subdev and a video device need to be validated, and
> v4l2_subdev_link_validate() can't handle that. Drivers typically handle
> this validation manually at stream start time (either in the .streamon()
> ioctl handler, or in the .start_streaming() vb2 queue operation),
> instead of implementing a custom .link_validate() handler. Forbidding
> usage of v4l2_subdev_link_validate() as the .link_validate() handler
> would thus force all subdev drivers that mix source links to subdev and
> video devices to implement a custom .link_validate() handler that
> returns immediately for the video device links and call
> v4l2_subdev_link_validate() for subdev links. This would create lots of
> duplicated code for no real gain. Instead, relax the check in
> v4l2_ctrl_modify_range() to ignore links from a video device to a subdev

v4l2_ctrl_modify_range? copy-paste error?

Should the check also be relaxed wrt. video device as a sink?

> without printing any warning.

As Sakari said, it would make sense to use .link_validate() to validate 
all links.

But if this warning is getting printed at the moment, then I think this 
is a valid fix (maybe with the sink side handled too, if needed).

  Tomi

> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>   drivers/media/v4l2-core/v4l2-subdev.c | 30 ++++++++++++++++++++-------
>   1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 67d43206ce32..b00be1d57e05 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1356,15 +1356,31 @@ int v4l2_subdev_link_validate(struct media_link *link)
>   	struct v4l2_subdev *source_sd, *sink_sd;
>   	struct v4l2_subdev_state *source_state, *sink_state;
>   	bool states_locked;
> +	bool is_sink_subdev;
> +	bool is_source_subdev;
>   	int ret;
>   
> -	if (!is_media_entity_v4l2_subdev(link->sink->entity) ||
> -	    !is_media_entity_v4l2_subdev(link->source->entity)) {
> -		pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> -			     !is_media_entity_v4l2_subdev(link->sink->entity) ?
> -			     "sink" : "source",
> -			     link->source->entity->name, link->source->index,
> -			     link->sink->entity->name, link->sink->index);
> +	is_sink_subdev = is_media_entity_v4l2_subdev(link->sink->entity);
> +	is_source_subdev = is_media_entity_v4l2_subdev(link->source->entity);
> +
> +	if (!is_sink_subdev || !is_source_subdev) {
> +		/*
> +		 * Do not print the warning if the source is a video device and
> +		 * the sink a subdev. This is a valid use case, to allow usage
> +		 * of this helper by subdev drivers that have multiple sink
> +		 * pads, some connected to video devices and some connected to
> +		 * other subdevs. The video device to subdev link is typically
> +		 * validated manually by the driver at stream start time in such
> +		 * cases.
> +		 */
> +		if (!is_sink_subdev ||
> +		    !is_media_entity_v4l2_video_device(link->source->entity))
> +			pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> +				     !is_sink_subdev ? "sink" : "source",
> +				     link->source->entity->name,
> +				     link->source->index,
> +				     link->sink->entity->name,
> +				     link->sink->index);
>   		return 0;
>   	}
>
  

Hi Laurent,

On Mon, Nov 13, 2023 at 01:37:54PM +0200, Laurent Pinchart wrote:
> On Mon, Nov 13, 2023 at 11:06:34AM +0000, Sakari Ailus wrote:
> > Hi Laurent,
> > 
> > Thanks for the patch.
> > 
> > On Mon, Nov 13, 2023 at 12:17:18PM +0200, Laurent Pinchart wrote:
> > > Before v6.3, the v4l2_subdev_link_validate() helper would ignore links
> > > whose source was a video device and sink a subdev. The helper was (and
> > > still is) used by drivers in such cases, in particular for subdevs with
> > > multiple sink pads, some connected to video devices and some to other
> > > subdevs.
> > > 
> > > Then, commit a6b995ed03ff ("media: subdev: use streams in
> > > v4l2_subdev_link_validate()") assumed the entities on the two sides of a
> > > link are both subdevs, and caused crashes in drivers that use the helper
> > > with subdev sink pads connected to video devices. Commit 55f1ecb11990
> > > ("media: v4l: subdev: Make link validation safer"), merged in v6.4,
> > > fixed the crash by adding an explicit check with a pr_warn_once(),
> > > mentioning a driver bug.
> > > 
> > > Links between a subdev and a video device need to be validated, and
> > > v4l2_subdev_link_validate() can't handle that. Drivers typically handle
> > > this validation manually at stream start time (either in the .streamon()
> > > ioctl handler, or in the .start_streaming() vb2 queue operation),
> > > instead of implementing a custom .link_validate() handler. Forbidding
> > 
> > While some do the validation as part of the streamon callback, it'd be
> > nicer to move this to the link_validate callback instead: this is what the
> > callback is for.
> 
> Is there any driver that does so already ?

Based on a quick look IPU3-CIO2 and vimc do that. I guess there indeed are
also other drivers besides the vsp driver that do not, on the other hand.
But this is what I would have expected drivers to do all along: the
link_validate callback is where you validate the formats across the link
match, independently of whether the link is between sub-devices or a
sub-device and something else.

> 
> > I'd presume not may drivers depend on
> > v4l2_subdev_link_validate() fail silently on non-subdevices as the issue
> > hasn't been reported before while the patch that seems to have broken this
> > was merged in 6.3.
> 
> At least the VSP1 driver depends on it. The i.MX8MP ISI driver does as
> well, it got merged in v6.4, so the crash in v6.3 didn't get noticed.
> I'd be surprised if the omap3isp driver also didn't depend on this
> behaviour when operating in M2M mode.
> 
> > Not failing silently in link_validate also ensures the validation gets
> > done: there have been drivers (more than one) that have simply missed the
> > link validation due to the issue (non-sub-device entity on one end) being
> > silently ignored by default.
> 
> If there's a consensus that subdev to video device link validation need
> to be done in the .link_validate() operation, then work needs to be done
> to make that happen. I'd like to see at least one proof of concept
> implemented, to make sure there are no unforeseen issues. Are you
> volunteering ? :-)

I'd hope the driver authors to provide fixes: they have the hardware to
test, too.

> 
> In the meantime, most drivers (possibly all) validate that link manually
> outside of the .link_validate() operation, so this is the de facto
> typical mode of operation. I don't think it can be considered as a

That is not entirely correct. While such drivers exist, these are still a
subset of all drivers (performing link validation between sub-device and
video nodes).

> driver bug until we make a reasonable effort to convert drivers to a new
> model. It's not fair to just add a pr_warn_once() without having done
> some of the work needed to move to a new model.

I don't think the model really got changed. The documentation implies this,
it does not explicitly document what the driver is expected to do. I can
write a documentation patch for this.

Given that there are surprisingly more than one driver that seem to depend
on this, at the moment, I'm fine with reverting back to ignoring the error.
But we should still print a warning to signal the driver should be fixed.

I can address the omap3isp driver. The IPU6 ISYS driver already uses
link_validate callback.

Also cc Hans.

> 
> > > usage of v4l2_subdev_link_validate() as the .link_validate() handler
> > > would thus force all subdev drivers that mix source links to subdev and
> > > video devices to implement a custom .link_validate() handler that
> > > returns immediately for the video device links and call
> > > v4l2_subdev_link_validate() for subdev links. This would create lots of
> > > duplicated code for no real gain. Instead, relax the check in
> > > v4l2_ctrl_modify_range() to ignore links from a video device to a subdev
> > > without printing any warning.
> > > 
> > > Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-subdev.c | 30 ++++++++++++++++++++-------
> > >  1 file changed, 23 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index 67d43206ce32..b00be1d57e05 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -1356,15 +1356,31 @@ int v4l2_subdev_link_validate(struct media_link *link)
> > >  	struct v4l2_subdev *source_sd, *sink_sd;
> > >  	struct v4l2_subdev_state *source_state, *sink_state;
> > >  	bool states_locked;
> > > +	bool is_sink_subdev;
> > > +	bool is_source_subdev;
> > >  	int ret;
> > >  
> > > -	if (!is_media_entity_v4l2_subdev(link->sink->entity) ||
> > > -	    !is_media_entity_v4l2_subdev(link->source->entity)) {
> > > -		pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> > > -			     !is_media_entity_v4l2_subdev(link->sink->entity) ?
> > > -			     "sink" : "source",
> > > -			     link->source->entity->name, link->source->index,
> > > -			     link->sink->entity->name, link->sink->index);
> > > +	is_sink_subdev = is_media_entity_v4l2_subdev(link->sink->entity);
> > > +	is_source_subdev = is_media_entity_v4l2_subdev(link->source->entity);
> > > +
> > > +	if (!is_sink_subdev || !is_source_subdev) {
> > > +		/*
> > > +		 * Do not print the warning if the source is a video device and
> > > +		 * the sink a subdev. This is a valid use case, to allow usage
> > > +		 * of this helper by subdev drivers that have multiple sink
> > > +		 * pads, some connected to video devices and some connected to
> > > +		 * other subdevs. The video device to subdev link is typically
> > > +		 * validated manually by the driver at stream start time in such
> > > +		 * cases.
> > > +		 */
> > > +		if (!is_sink_subdev ||
> > > +		    !is_media_entity_v4l2_video_device(link->source->entity))
> > > +			pr_warn_once("%s of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
> > > +				     !is_sink_subdev ? "sink" : "source",
> > > +				     link->source->entity->name,
> > > +				     link->source->index,
> > > +				     link->sink->entity->name,
> > > +				     link->sink->index);
> > >  		return 0;
> > >  	}
> > >  
>