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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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