[v2,3/7] media: sun4i_csi: Don't use v4l2_subdev_link_validate() for video device

  The v4l2_subdev_link_validate() function is a helper designed to
validate links whose sink is a subdev. When called on a link whose sink
is a video device, it only prints a warning and returns. Its usage in
the sun4i_csi driver is wrong, leaving the link from the sub4i_csi
subdev to the capture video device unvalidated.

Planned improvements to the v4l2_subdev_link_validate() function will
turn the warning into an error, breaking the sun4i_csi driver. As an
interim measure, move the warning to the sun4i_csi driver in a custom
validation handler, and drop the call to the helper.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Acked-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
  

Hi,

On 22/08/2024 18:45, Laurent Pinchart wrote:
> The v4l2_subdev_link_validate() function is a helper designed to
> validate links whose sink is a subdev. When called on a link whose sink
> is a video device, it only prints a warning and returns. Its usage in
> the sun4i_csi driver is wrong, leaving the link from the sub4i_csi
> subdev to the capture video device unvalidated.
> 
> Planned improvements to the v4l2_subdev_link_validate() function will
> turn the warning into an error, breaking the sun4i_csi driver. As an
> interim measure, move the warning to the sun4i_csi driver in a custom
> validation handler, and drop the call to the helper.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Acked-by: Chen-Yu Tsai <wens@csie.org>
> ---
>   drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> index dbb26c7b2f8d..d07e980aba61 100644
> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> @@ -35,8 +35,15 @@ struct sun4i_csi_traits {
>   	bool has_isp;
>   };
>   
> +static int sun4i_csi_video_link_validate(struct media_link *link)
> +{
> +	dev_warn_once(link->graph_obj.mdev->dev,
> +		      "Driver bug: link validation not implemented\n");
> +	return 0;
> +}
> +
>   static const struct media_entity_operations sun4i_csi_video_entity_ops = {
> -	.link_validate = v4l2_subdev_link_validate,
> +	.link_validate = sun4i_csi_video_link_validate,
>   };
>   
>   static const struct media_entity_operations sun4i_csi_subdev_entity_ops = {

I fear this might just leave it broken, but I don't have a better idea.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>

  Tomi
  

On Mon, Aug 26, 2024 at 02:04:44PM +0300, Tomi Valkeinen wrote:
> On 22/08/2024 18:45, Laurent Pinchart wrote:
> > The v4l2_subdev_link_validate() function is a helper designed to
> > validate links whose sink is a subdev. When called on a link whose sink
> > is a video device, it only prints a warning and returns. Its usage in
> > the sun4i_csi driver is wrong, leaving the link from the sub4i_csi
> > subdev to the capture video device unvalidated.
> > 
> > Planned improvements to the v4l2_subdev_link_validate() function will
> > turn the warning into an error, breaking the sun4i_csi driver. As an
> > interim measure, move the warning to the sun4i_csi driver in a custom
> > validation handler, and drop the call to the helper.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Acked-by: Chen-Yu Tsai <wens@csie.org>
> > ---
> >   drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> > index dbb26c7b2f8d..d07e980aba61 100644
> > --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> > +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c
> > @@ -35,8 +35,15 @@ struct sun4i_csi_traits {
> >   	bool has_isp;
> >   };
> >   
> > +static int sun4i_csi_video_link_validate(struct media_link *link)
> > +{
> > +	dev_warn_once(link->graph_obj.mdev->dev,
> > +		      "Driver bug: link validation not implemented\n");
> > +	return 0;
> > +}
> > +
> >   static const struct media_entity_operations sun4i_csi_video_entity_ops = {
> > -	.link_validate = v4l2_subdev_link_validate,
> > +	.link_validate = sun4i_csi_video_link_validate,
> >   };
> >   
> >   static const struct media_entity_operations sun4i_csi_subdev_entity_ops = {
> 
> I fear this might just leave it broken, but I don't have a better idea.

I think it would be fairly easy to implement support for proper
validation in the driver, but it would require access to a device for
testing. This patch doesn't worsen the issue, or make fixing it more
difficult, so I think it's OK for now until someone can send a real fix.

> Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>