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

[Sakari Ailus <sakari.ailus@iki.fi>]

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