[PATCH 1/1] media: v4l: subdev: Make link validation safer

[PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

Link validation currently accesses invalid pointers if the link passed to it
is not between two sub-devices. This is of course a driver bug.

Ignore the error but print a debug message, as this is how it used to work
previously.

Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 1bebcda2bd20c..dd911180ec899 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1209,6 +1209,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
 	struct v4l2_subdev_state *source_state, *sink_state;
 	int ret;
 
+	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
+		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
+			     link->sink->entity->name);
+		return 0;
+	}
+	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
+		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
+			     link->source->entity->name);
+		return 0;
+	}
+
 	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
 	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
 
-- 
2.30.2

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Tomi Valkeinen]

Hi Sakari,

On 09/03/2023 14:27, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 1bebcda2bd20c..dd911180ec899 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1209,6 +1209,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
>   	struct v4l2_subdev_state *source_state, *sink_state;
>   	int ret;
>   
> +	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
> +		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			     link->sink->entity->name);
> +		return 0;
> +	}
> +	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
> +		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			     link->source->entity->name);
> +		return 0;
> +	}
> +
>   	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
>   	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
>   

I think this is supposed to be v2. You missed my RB, but here it is again:

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

Also, the commit message says "debug message", but it's a warn now.

  Tomi

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

On Thu, Mar 09, 2023 at 02:27:16PM +0200, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>

This was intended to be v2. V1 is here:

<URL:https://lore.kernel.org/linux-media/ZAIryJV7XNpW8lbY@kekkonen.localdomain/T/#m12c7e39e154a799b1034388d0c63ce0f65979a1b>

-- 
Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

On Thu, Mar 09, 2023 at 02:34:28PM +0200, Tomi Valkeinen wrote:
> I think this is supposed to be v2. You missed my RB, but here it is again:
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> 
> Also, the commit message says "debug message", but it's a warn now.

Thanks, I'll address this while applying it.

-- 
Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Hans de Goede]

Hi Sakari, 

On 3/9/23 13:27, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>

FWIW you have my Reported-by in there twice now ...

Regards,

Hans




> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 1bebcda2bd20c..dd911180ec899 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1209,6 +1209,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
>  	struct v4l2_subdev_state *source_state, *sink_state;
>  	int ret;
>  
> +	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
> +		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			     link->sink->entity->name);
> +		return 0;
> +	}
> +	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
> +		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			     link->source->entity->name);
> +		return 0;
> +	}
> +
>  	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
>  	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
>  

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

On Thu, Mar 09, 2023 at 01:43:50PM +0100, Hans de Goede wrote:
> Hi Sakari, 
> 
> On 3/9/23 13:27, Sakari Ailus wrote:
> > Link validation currently accesses invalid pointers if the link passed to it
> > is not between two sub-devices. This is of course a driver bug.
> > 
> > Ignore the error but print a debug message, as this is how it used to work
> > previously.
> > 
> > Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> > Reported-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> FWIW you have my Reported-by in there twice now ...

Ah, thanks. I'll drop the first tag.

-- 
Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Tomi Valkeinen]

On 09/03/2023 14:56, Sakari Ailus wrote:
> On Thu, Mar 09, 2023 at 01:43:50PM +0100, Hans de Goede wrote:
>> Hi Sakari,
>>
>> On 3/9/23 13:27, Sakari Ailus wrote:
>>> Link validation currently accesses invalid pointers if the link passed to it
>>> is not between two sub-devices. This is of course a driver bug.
>>>
>>> Ignore the error but print a debug message, as this is how it used to work
>>> previously.
>>>
>>> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
>>
>> FWIW you have my Reported-by in there twice now ...
> 
> Ah, thanks. I'll drop the first tag.

$ git grep Reported-and-tested-by Documentation/
Documentation/process/maintainer-tip.rst:Please do not use combined tags, e.g. ``Reported-and-tested-by``, as

  Tomi

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

On Thu, Mar 09, 2023 at 02:57:52PM +0200, Tomi Valkeinen wrote:
> On 09/03/2023 14:56, Sakari Ailus wrote:
> > On Thu, Mar 09, 2023 at 01:43:50PM +0100, Hans de Goede wrote:
> > > Hi Sakari,
> > > 
> > > On 3/9/23 13:27, Sakari Ailus wrote:
> > > > Link validation currently accesses invalid pointers if the link passed to it
> > > > is not between two sub-devices. This is of course a driver bug.
> > > > 
> > > > Ignore the error but print a debug message, as this is how it used to work
> > > > previously.
> > > > 
> > > > Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> > > > Reported-by: Hans de Goede <hdegoede@redhat.com>
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
> > > 
> > > FWIW you have my Reported-by in there twice now ...
> > 
> > Ah, thanks. I'll drop the first tag.
> 
> $ git grep Reported-and-tested-by Documentation/
> Documentation/process/maintainer-tip.rst:Please do not use combined tags, e.g. ``Reported-and-tested-by``, as

Uh-oh. Well, I'll just split that. Thanks.

-- 
Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Hans de Goede]

Hi,

On 3/9/23 14:00, Sakari Ailus wrote:
> On Thu, Mar 09, 2023 at 02:57:52PM +0200, Tomi Valkeinen wrote:
>> On 09/03/2023 14:56, Sakari Ailus wrote:
>>> On Thu, Mar 09, 2023 at 01:43:50PM +0100, Hans de Goede wrote:
>>>> Hi Sakari,
>>>>
>>>> On 3/9/23 13:27, Sakari Ailus wrote:
>>>>> Link validation currently accesses invalid pointers if the link passed to it
>>>>> is not between two sub-devices. This is of course a driver bug.
>>>>>
>>>>> Ignore the error but print a debug message, as this is how it used to work
>>>>> previously.
>>>>>
>>>>> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
>>>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>>> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
>>>>
>>>> FWIW you have my Reported-by in there twice now ...
>>>
>>> Ah, thanks. I'll drop the first tag.
>>
>> $ git grep Reported-and-tested-by Documentation/
>> Documentation/process/maintainer-tip.rst:Please do not use combined tags, e.g. ``Reported-and-tested-by``, as
> 
> Uh-oh. Well, I'll just split that. Thanks.

This is new to me too, but good to know. I'll try to get rid of the habbit
of using the combined tags then.

Regards,

Hans

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Thu, Mar 09, 2023 at 02:27:16PM +0200, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 1bebcda2bd20c..dd911180ec899 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1209,6 +1209,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
>  	struct v4l2_subdev_state *source_state, *sink_state;
>  	int ret;
>  
> +	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
> +		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			     link->sink->entity->name);

Printing the whole link would make it easier to debug the problem:

		pr_warn_once("sink of link '%s':%u->'%s':%u is not a V4L2 sub-device, driver bug!\n",
			     link->source->entity->name, link->source->index);
			     link->sink->entity->name, link->sink->index);

Same below. With this, and the other comments in the mail thread
addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> +		return 0;
> +	}
> +	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
> +		pr_warn_once("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			     link->source->entity->name);
> +		return 0;
> +	}
> +
>  	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
>  	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

On Thu, Mar 09, 2023 at 01:09:11PM +0100, Hans de Goede wrote:
> Hi Sakari,
> 
> On 3/2/23 21:22, Sakari Ailus wrote:
> > Link validation currently accesses invalid pointers if the link passed to it
> > is not between two sub-devices. This is of course a driver bug.
> > 
> > Ignore the error but print a debug message, as this is how it used to work
> > previously.
> > 
> > Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> > Reported-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Hi Hans,
> > 
> > Could you test this?
> 
> Yes, thank you for the fix.
> 
> I can confirm that this fixes the oops on my Surface Go 1 and
> that it makes the IPU3 attached cameras work again:
> 
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> or better just replace the Reported-by with:
> 
> Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>

Thanks!

I'm putting this to my fixes PR I'm about to send soon.

-- 
Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Hans de Goede]

Hi Sakari,

On 3/2/23 21:22, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Hans,
> 
> Could you test this?

Yes, thank you for the fix.

I can confirm that this fixes the oops on my Surface Go 1 and
that it makes the IPU3 attached cameras work again:

Tested-by: Hans de Goede <hdegoede@redhat.com>

or better just replace the Reported-by with:

Reported-and-tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans




> The bug is of course in the ImgU driver and this reverts to the old
> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
> needs to be fixed and I think we could make this return an error at the same
> time. Right now I can't be sure the ImgU driver is the only one suffering
> from this, but if so, it's likely to be broken anyway.
> 
> - Sakari
> 
>  drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index dff1d9be7841..a6c80096586e 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -1224,6 +1224,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
>  	struct v4l2_subdev_state *source_state, *sink_state;
>  	int ret;
>  
> +	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
> +		pr_debug("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			 link->sink->entity->name);
> +		return 0;
> +	}
> +	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
> +		pr_debug("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
> +			 link->source->entity->name);
> +		return 0;
> +	}
> +
>  	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
>  	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
>  

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

Hi,

On Fri, Mar 03, 2023 at 04:06:11PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/3/23 12:36, Tomi Valkeinen wrote:
> > On 03/03/2023 13:30, Sakari Ailus wrote:
> >> Hi Tomi,
> >>
> >> On Fri, Mar 03, 2023 at 10:41:27AM +0200, Tomi Valkeinen wrote:
> >>> On 02/03/2023 22:22, Sakari Ailus wrote:
> >>>> Link validation currently accesses invalid pointers if the link passed to it
> >>>> is not between two sub-devices. This is of course a driver bug.
> >>>>
> >>>> Ignore the error but print a debug message, as this is how it used to work
> >>>> previously.
> >>>>
> >>>> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> >>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
> >>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> >>>> ---
> >>>> Hi Hans,
> >>>>
> >>>> Could you test this?
> >>>>
> >>>> The bug is of course in the ImgU driver and this reverts to the old
> >>>> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
> >>>> needs to be fixed and I think we could make this return an error at the same
> >>>> time. Right now I can't be sure the ImgU driver is the only one suffering
> >>>> from this, but if so, it's likely to be broken anyway.
> >>>
> >>> Maybe it should be at least a warn? How do we catch other broken drivers
> >>> otherwise?
> >>
> >> The purpose of this patch is just to restore the old behaviour, and merge
> >> it as a fix to v6.3 (via Cc'ing stable). I agree this should be made an
> >> error but I'd like that change to be present in the media tree for some
> >> time first.
> > 
> > I meant that keep it returning 0 (no error), but instead of a debug print, use pr_warn. Or maybe pr_warn_once for now.
> 
> Switching to pr_warn_once() sounds reasonable to me.

I'll send v2 with that.

-- 
Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Hans de Goede]

Hi,

On 3/3/23 12:36, Tomi Valkeinen wrote:
> On 03/03/2023 13:30, Sakari Ailus wrote:
>> Hi Tomi,
>>
>> On Fri, Mar 03, 2023 at 10:41:27AM +0200, Tomi Valkeinen wrote:
>>> On 02/03/2023 22:22, Sakari Ailus wrote:
>>>> Link validation currently accesses invalid pointers if the link passed to it
>>>> is not between two sub-devices. This is of course a driver bug.
>>>>
>>>> Ignore the error but print a debug message, as this is how it used to work
>>>> previously.
>>>>
>>>> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
>>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>>> ---
>>>> Hi Hans,
>>>>
>>>> Could you test this?
>>>>
>>>> The bug is of course in the ImgU driver and this reverts to the old
>>>> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
>>>> needs to be fixed and I think we could make this return an error at the same
>>>> time. Right now I can't be sure the ImgU driver is the only one suffering
>>>> from this, but if so, it's likely to be broken anyway.
>>>
>>> Maybe it should be at least a warn? How do we catch other broken drivers
>>> otherwise?
>>
>> The purpose of this patch is just to restore the old behaviour, and merge
>> it as a fix to v6.3 (via Cc'ing stable). I agree this should be made an
>> error but I'd like that change to be present in the media tree for some
>> time first.
> 
> I meant that keep it returning 0 (no error), but instead of a debug print, use pr_warn. Or maybe pr_warn_once for now.

Switching to pr_warn_once() sounds reasonable to me.

I'll try to give this a test-run sometime next week.

Regards,

Hans

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Tomi Valkeinen]

On 03/03/2023 13:30, Sakari Ailus wrote:
> Hi Tomi,
> 
> On Fri, Mar 03, 2023 at 10:41:27AM +0200, Tomi Valkeinen wrote:
>> On 02/03/2023 22:22, Sakari Ailus wrote:
>>> Link validation currently accesses invalid pointers if the link passed to it
>>> is not between two sub-devices. This is of course a driver bug.
>>>
>>> Ignore the error but print a debug message, as this is how it used to work
>>> previously.
>>>
>>> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
>>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>> Hi Hans,
>>>
>>> Could you test this?
>>>
>>> The bug is of course in the ImgU driver and this reverts to the old
>>> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
>>> needs to be fixed and I think we could make this return an error at the same
>>> time. Right now I can't be sure the ImgU driver is the only one suffering
>>> from this, but if so, it's likely to be broken anyway.
>>
>> Maybe it should be at least a warn? How do we catch other broken drivers
>> otherwise?
> 
> The purpose of this patch is just to restore the old behaviour, and merge
> it as a fix to v6.3 (via Cc'ing stable). I agree this should be made an
> error but I'd like that change to be present in the media tree for some
> time first.

I meant that keep it returning 0 (no error), but instead of a debug 
print, use pr_warn. Or maybe pr_warn_once for now.

  Tomi

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

Hi Tomi,

On Fri, Mar 03, 2023 at 10:41:27AM +0200, Tomi Valkeinen wrote:
> On 02/03/2023 22:22, Sakari Ailus wrote:
> > Link validation currently accesses invalid pointers if the link passed to it
> > is not between two sub-devices. This is of course a driver bug.
> > 
> > Ignore the error but print a debug message, as this is how it used to work
> > previously.
> > 
> > Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> > Reported-by: Hans de Goede <hdegoede@redhat.com>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > Hi Hans,
> > 
> > Could you test this?
> > 
> > The bug is of course in the ImgU driver and this reverts to the old
> > pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
> > needs to be fixed and I think we could make this return an error at the same
> > time. Right now I can't be sure the ImgU driver is the only one suffering
> > from this, but if so, it's likely to be broken anyway.
> 
> Maybe it should be at least a warn? How do we catch other broken drivers
> otherwise?

The purpose of this patch is just to restore the old behaviour, and merge
it as a fix to v6.3 (via Cc'ing stable). I agree this should be made an
error but I'd like that change to be present in the media tree for some
time first.

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

Thanks!

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/1] media: v4l: subdev: Make link validation safer

[Tomi Valkeinen]

On 02/03/2023 22:22, Sakari Ailus wrote:
> Link validation currently accesses invalid pointers if the link passed to it
> is not between two sub-devices. This is of course a driver bug.
> 
> Ignore the error but print a debug message, as this is how it used to work
> previously.
> 
> Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
> Reported-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> Hi Hans,
> 
> Could you test this?
> 
> The bug is of course in the ImgU driver and this reverts to the old
> pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
> needs to be fixed and I think we could make this return an error at the same
> time. Right now I can't be sure the ImgU driver is the only one suffering
> from this, but if so, it's likely to be broken anyway.

Maybe it should be at least a warn? How do we catch other broken drivers 
otherwise?

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

  Tomi

[PATCH 1/1] media: v4l: subdev: Make link validation safer

[Sakari Ailus]

Link validation currently accesses invalid pointers if the link passed to it
is not between two sub-devices. This is of course a driver bug.

Ignore the error but print a debug message, as this is how it used to work
previously.

Fixes: a6b995ed03ff ("media: subdev: use streams in v4l2_subdev_link_validate()")
Reported-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
Hi Hans,

Could you test this?

The bug is of course in the ImgU driver and this reverts to the old
pre-streams behaviour. It silently fails instead of oopsing. The ImgU driver
needs to be fixed and I think we could make this return an error at the same
time. Right now I can't be sure the ImgU driver is the only one suffering
from this, but if so, it's likely to be broken anyway.

- Sakari

 drivers/media/v4l2-core/v4l2-subdev.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index dff1d9be7841..a6c80096586e 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -1224,6 +1224,17 @@ int v4l2_subdev_link_validate(struct media_link *link)
 	struct v4l2_subdev_state *source_state, *sink_state;
 	int ret;
 
+	if (!is_media_entity_v4l2_subdev(link->sink->entity)) {
+		pr_debug("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
+			 link->sink->entity->name);
+		return 0;
+	}
+	if (!is_media_entity_v4l2_subdev(link->source->entity)) {
+		pr_debug("entity \"%s\" not a V4L2 sub-device, driver bug!\n",
+			 link->source->entity->name);
+		return 0;
+	}
+
 	sink_sd = media_entity_to_v4l2_subdev(link->sink->entity);
 	source_sd = media_entity_to_v4l2_subdev(link->source->entity);
 
-- 
2.30.2