[PATCH v2 1/1] media: v4l2-subdev: Fail {enable,disable}_streams and s_streaming nicely

[PATCH v2 1/1] media: v4l2-subdev: Fail {enable,disable}_streams and s_streaming nicely

[Sakari Ailus]

If a sub-device does not set enable_streams() and disable_streams() pad
ops while it sets the s_stream() video op to
v4l2_subdev_s_stream_helper(), enabling or disabling streaming either way
on the sub-device will result calling v4l2_subdev_s_stream_helper() and
v4l2_subdev_{enable,disable}_streams() recursively, exhausting the stack.
Return -ENOIOCTLCMD in this case to handle the situation gracefully.

Reported-by: David Heidelberg <david@ixit.cz>
Fixes: b62949ddaa52 ("media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()")
Cc: stable@vger.kernel.org
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v1:

- Better patch description.

 drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 831c69c958b8..f8ea4afc6cbb 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -2334,11 +2334,13 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
 
 	/* Fallback on .s_stream() if .enable_streams() isn't available. */
 	use_s_stream = !v4l2_subdev_has_op(sd, pad, enable_streams);
-
-	if (!use_s_stream)
+	if (!use_s_stream) {
 		state = v4l2_subdev_lock_and_get_active_state(sd);
-	else
+	} else {
+		if (!v4l2_subdev_has_op(sd, video, s_stream))
+			return -ENOIOCTLCMD;
 		state = NULL;
+	}
 
 	/*
 	 * Verify that the requested streams exist and that they are not
@@ -2435,11 +2437,13 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
 
 	/* Fallback on .s_stream() if .disable_streams() isn't available. */
 	use_s_stream = !v4l2_subdev_has_op(sd, pad, disable_streams);
-
-	if (!use_s_stream)
+	if (!use_s_stream) {
 		state = v4l2_subdev_lock_and_get_active_state(sd);
-	else
+	} else {
+		if (!v4l2_subdev_has_op(sd, video, s_stream))
+			return -ENOIOCTLCMD;
 		state = NULL;
+	}
 
 	/*
 	 * Verify that the requested streams exist and that they are not
-- 
2.47.3

Re: [PATCH v2 1/1] media: v4l2-subdev: Fail {enable,disable}_streams and s_streaming nicely

[Laurent Pinchart]

On Mon, Mar 23, 2026 at 12:56:55PM +0200, Sakari Ailus wrote:
> If a sub-device does not set enable_streams() and disable_streams() pad
> ops while it sets the s_stream() video op to
> v4l2_subdev_s_stream_helper(), enabling or disabling streaming either way
> on the sub-device will result calling v4l2_subdev_s_stream_helper() and
> v4l2_subdev_{enable,disable}_streams() recursively, exhausting the stack.
> Return -ENOIOCTLCMD in this case to handle the situation gracefully.

This is not a valid use case, right ? Can we WARN() ?

> Reported-by: David Heidelberg <david@ixit.cz>
> Fixes: b62949ddaa52 ("media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> since v1:
> 
> - Better patch description.
> 
>  drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 831c69c958b8..f8ea4afc6cbb 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -2334,11 +2334,13 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
>  
>  	/* Fallback on .s_stream() if .enable_streams() isn't available. */
>  	use_s_stream = !v4l2_subdev_has_op(sd, pad, enable_streams);
> -
> -	if (!use_s_stream)
> +	if (!use_s_stream) {
>  		state = v4l2_subdev_lock_and_get_active_state(sd);
> -	else
> +	} else {
> +		if (!v4l2_subdev_has_op(sd, video, s_stream))
> +			return -ENOIOCTLCMD;
>  		state = NULL;
> +	}
>  
>  	/*
>  	 * Verify that the requested streams exist and that they are not
> @@ -2435,11 +2437,13 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
>  
>  	/* Fallback on .s_stream() if .disable_streams() isn't available. */
>  	use_s_stream = !v4l2_subdev_has_op(sd, pad, disable_streams);
> -
> -	if (!use_s_stream)
> +	if (!use_s_stream) {
>  		state = v4l2_subdev_lock_and_get_active_state(sd);
> -	else
> +	} else {
> +		if (!v4l2_subdev_has_op(sd, video, s_stream))
> +			return -ENOIOCTLCMD;
>  		state = NULL;
> +	}
>  
>  	/*
>  	 * Verify that the requested streams exist and that they are not

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/1] media: v4l2-subdev: Fail {enable,disable}_streams and s_streaming nicely

[Sakari Ailus]

Hi Laurent,

On Mon, Mar 23, 2026 at 01:10:42PM +0200, Laurent Pinchart wrote:
> On Mon, Mar 23, 2026 at 12:56:55PM +0200, Sakari Ailus wrote:
> > If a sub-device does not set enable_streams() and disable_streams() pad
> > ops while it sets the s_stream() video op to
> > v4l2_subdev_s_stream_helper(), enabling or disabling streaming either way
> > on the sub-device will result calling v4l2_subdev_s_stream_helper() and
> > v4l2_subdev_{enable,disable}_streams() recursively, exhausting the stack.
> > Return -ENOIOCTLCMD in this case to handle the situation gracefully.
> 
> This is not a valid use case, right ? Can we WARN() ?

Partly yes, and partly it requires a driver bug.

The CCS driver soon will differentiate its ops for source (scaler/binner
and CSI-2 receiver) sub-device, but only pad ops, not video ops. This means
that on non-source sub-devices the s_stream pad op is set to
v4l2_subdev_s_stream_helper() while {enable,disable}_streams() pad ops are
NULL.

It also takes a bug in the camss driver to trigger this -- it enables
streaming on all sub-devices of the pipeline instead of the next upstream
sub-device. That bug should be fixed but it's a separate issue.

> 
> > Reported-by: David Heidelberg <david@ixit.cz>
> > Fixes: b62949ddaa52 ("media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > since v1:
> > 
> > - Better patch description.
> > 
> >  drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++------
> >  1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 831c69c958b8..f8ea4afc6cbb 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -2334,11 +2334,13 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> >  
> >  	/* Fallback on .s_stream() if .enable_streams() isn't available. */
> >  	use_s_stream = !v4l2_subdev_has_op(sd, pad, enable_streams);
> > -
> > -	if (!use_s_stream)
> > +	if (!use_s_stream) {
> >  		state = v4l2_subdev_lock_and_get_active_state(sd);
> > -	else
> > +	} else {
> > +		if (!v4l2_subdev_has_op(sd, video, s_stream))
> > +			return -ENOIOCTLCMD;
> >  		state = NULL;
> > +	}
> >  
> >  	/*
> >  	 * Verify that the requested streams exist and that they are not
> > @@ -2435,11 +2437,13 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
> >  
> >  	/* Fallback on .s_stream() if .disable_streams() isn't available. */
> >  	use_s_stream = !v4l2_subdev_has_op(sd, pad, disable_streams);
> > -
> > -	if (!use_s_stream)
> > +	if (!use_s_stream) {
> >  		state = v4l2_subdev_lock_and_get_active_state(sd);
> > -	else
> > +	} else {
> > +		if (!v4l2_subdev_has_op(sd, video, s_stream))
> > +			return -ENOIOCTLCMD;
> >  		state = NULL;
> > +	}
> >  
> >  	/*
> >  	 * Verify that the requested streams exist and that they are not
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 1/1] media: v4l2-subdev: Fail {enable,disable}_streams and s_streaming nicely

[Laurent Pinchart]

On Mon, Mar 23, 2026 at 01:44:14PM +0200, Sakari Ailus wrote:
> On Mon, Mar 23, 2026 at 01:10:42PM +0200, Laurent Pinchart wrote:
> > On Mon, Mar 23, 2026 at 12:56:55PM +0200, Sakari Ailus wrote:
> > > If a sub-device does not set enable_streams() and disable_streams() pad
> > > ops while it sets the s_stream() video op to
> > > v4l2_subdev_s_stream_helper(), enabling or disabling streaming either way
> > > on the sub-device will result calling v4l2_subdev_s_stream_helper() and
> > > v4l2_subdev_{enable,disable}_streams() recursively, exhausting the stack.
> > > Return -ENOIOCTLCMD in this case to handle the situation gracefully.
> > 
> > This is not a valid use case, right ? Can we WARN() ?
> 
> Partly yes, and partly it requires a driver bug.
> 
> The CCS driver soon will differentiate its ops for source (scaler/binner
> and CSI-2 receiver) sub-device, but only pad ops, not video ops. This means
> that on non-source sub-devices the s_stream pad op is set to
> v4l2_subdev_s_stream_helper() while {enable,disable}_streams() pad ops are
> NULL.
> 
> It also takes a bug in the camss driver to trigger this -- it enables
> streaming on all sub-devices of the pipeline instead of the next upstream
> sub-device. That bug should be fixed but it's a separate issue.

Sounds like a candidate for a WARN() then, doesn't it ?

> > > Reported-by: David Heidelberg <david@ixit.cz>
> > > Fixes: b62949ddaa52 ("media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > since v1:
> > > 
> > > - Better patch description.
> > > 
> > >  drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++------
> > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index 831c69c958b8..f8ea4afc6cbb 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -2334,11 +2334,13 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> > >  
> > >  	/* Fallback on .s_stream() if .enable_streams() isn't available. */
> > >  	use_s_stream = !v4l2_subdev_has_op(sd, pad, enable_streams);
> > > -
> > > -	if (!use_s_stream)
> > > +	if (!use_s_stream) {
> > >  		state = v4l2_subdev_lock_and_get_active_state(sd);
> > > -	else
> > > +	} else {
> > > +		if (!v4l2_subdev_has_op(sd, video, s_stream))
> > > +			return -ENOIOCTLCMD;
> > >  		state = NULL;
> > > +	}
> > >  
> > >  	/*
> > >  	 * Verify that the requested streams exist and that they are not
> > > @@ -2435,11 +2437,13 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
> > >  
> > >  	/* Fallback on .s_stream() if .disable_streams() isn't available. */
> > >  	use_s_stream = !v4l2_subdev_has_op(sd, pad, disable_streams);
> > > -
> > > -	if (!use_s_stream)
> > > +	if (!use_s_stream) {
> > >  		state = v4l2_subdev_lock_and_get_active_state(sd);
> > > -	else
> > > +	} else {
> > > +		if (!v4l2_subdev_has_op(sd, video, s_stream))
> > > +			return -ENOIOCTLCMD;
> > >  		state = NULL;
> > > +	}
> > >  
> > >  	/*
> > >  	 * Verify that the requested streams exist and that they are not

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/1] media: v4l2-subdev: Fail {enable,disable}_streams and s_streaming nicely

[Sakari Ailus]

Hi Laurent,

On Wed, Mar 25, 2026 at 04:07:35PM +0200, Laurent Pinchart wrote:
> On Mon, Mar 23, 2026 at 01:44:14PM +0200, Sakari Ailus wrote:
> > On Mon, Mar 23, 2026 at 01:10:42PM +0200, Laurent Pinchart wrote:
> > > On Mon, Mar 23, 2026 at 12:56:55PM +0200, Sakari Ailus wrote:
> > > > If a sub-device does not set enable_streams() and disable_streams() pad
> > > > ops while it sets the s_stream() video op to
> > > > v4l2_subdev_s_stream_helper(), enabling or disabling streaming either way
> > > > on the sub-device will result calling v4l2_subdev_s_stream_helper() and
> > > > v4l2_subdev_{enable,disable}_streams() recursively, exhausting the stack.
> > > > Return -ENOIOCTLCMD in this case to handle the situation gracefully.
> > > 
> > > This is not a valid use case, right ? Can we WARN() ?
> > 
> > Partly yes, and partly it requires a driver bug.
> > 
> > The CCS driver soon will differentiate its ops for source (scaler/binner
> > and CSI-2 receiver) sub-device, but only pad ops, not video ops. This means
> > that on non-source sub-devices the s_stream pad op is set to
> > v4l2_subdev_s_stream_helper() while {enable,disable}_streams() pad ops are
> > NULL.
> > 
> > It also takes a bug in the camss driver to trigger this -- it enables
> > streaming on all sub-devices of the pipeline instead of the next upstream
> > sub-device. That bug should be fixed but it's a separate issue.
> 
> Sounds like a candidate for a WARN() then, doesn't it ?

I'll add WARN_ON_ONCE() for using v4l2_subdev_s_straem_helper() without
both {enable,disable}_streams()_ in v3. I came to think also that this
check can be moved to the helper where I think it'll look nicer.

> 
> > > > Reported-by: David Heidelberg <david@ixit.cz>
> > > > Fixes: b62949ddaa52 ("media: subdev: Support single-stream case in v4l2_subdev_enable/disable_streams()")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > > since v1:
> > > > 
> > > > - Better patch description.
> > > > 
> > > >  drivers/media/v4l2-core/v4l2-subdev.c | 16 ++++++++++------
> > > >  1 file changed, 10 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > index 831c69c958b8..f8ea4afc6cbb 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > @@ -2334,11 +2334,13 @@ int v4l2_subdev_enable_streams(struct v4l2_subdev *sd, u32 pad,
> > > >  
> > > >  	/* Fallback on .s_stream() if .enable_streams() isn't available. */
> > > >  	use_s_stream = !v4l2_subdev_has_op(sd, pad, enable_streams);
> > > > -
> > > > -	if (!use_s_stream)
> > > > +	if (!use_s_stream) {
> > > >  		state = v4l2_subdev_lock_and_get_active_state(sd);
> > > > -	else
> > > > +	} else {
> > > > +		if (!v4l2_subdev_has_op(sd, video, s_stream))
> > > > +			return -ENOIOCTLCMD;
> > > >  		state = NULL;
> > > > +	}
> > > >  
> > > >  	/*
> > > >  	 * Verify that the requested streams exist and that they are not
> > > > @@ -2435,11 +2437,13 @@ int v4l2_subdev_disable_streams(struct v4l2_subdev *sd, u32 pad,
> > > >  
> > > >  	/* Fallback on .s_stream() if .disable_streams() isn't available. */
> > > >  	use_s_stream = !v4l2_subdev_has_op(sd, pad, disable_streams);
> > > > -
> > > > -	if (!use_s_stream)
> > > > +	if (!use_s_stream) {
> > > >  		state = v4l2_subdev_lock_and_get_active_state(sd);
> > > > -	else
> > > > +	} else {
> > > > +		if (!v4l2_subdev_has_op(sd, video, s_stream))
> > > > +			return -ENOIOCTLCMD;
> > > >  		state = NULL;
> > > > +	}
> > > >  
> > > >  	/*
> > > >  	 * Verify that the requested streams exist and that they are not
> 

-- 
Regards,

Sakari Ailus