Re: [PATCH 1/1] media: subdev: Make get_fmt on INTERNAL pads without STREAMS an error

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 07/10/2026 11:48, Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, Oct 07, 2026 at 11:43:35AM +0200, Hans Verkuil wrote:
>> On 06/10/2026 11:29, Sakari Ailus wrote:
>>> Internal pads should only be accessible to file handles with
>>> V4L2_SUBDEV_CLIENT_CAP_STREAMS flag set. Return an error otherwise.
>>>
>>> Fixes: 49cdf56876d3 ("media: mc: Add INTERNAL pad flag")
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> ---
>>>  drivers/media/v4l2-core/v4l2-subdev.c | 7 ++++++-
>>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>>> index a07d77e584c3..7cb5a40f0f8c 100644
>>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>>> @@ -854,8 +854,13 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>>>  	case VIDIOC_SUBDEV_G_FMT: {
>>>  		struct v4l2_subdev_format *format = arg;
>>>  
>>> -		if (!client_supports_streams)
>>> +		if (!client_supports_streams) {
>>> +			if (format->pad < sd->entity.num_pads &&
>>> +			    sd->entity.pads[format->pad].flags & MEDIA_PAD_FL_INTERNAL)
>>> +				return -EINVAL;
>>> +
>>>  			format->stream = 0;
>>> +		}
>>>  
>>>  		memset(format->reserved, 0, sizeof(format->reserved));
>>>  		memset(format->format.reserved, 0, sizeof(format->format.reserved));
>>>
>>
>> There is no documentation that I can find that says that MEDIA_PAD_FL_INTERNAL is only available
>> if V4L2_SUBDEV_CLIENT_CAP_STREAMS is set.
>>
>> I think this needs some more thought: if internal pads are only available if that cap is set,
>> then I expect that a lot more ioctls will need this check. In that case the check should become
>> a helper function.
>>
>> But how does this affect e.g. G_TOPOLOGY or ENUM_LINKS? If the cap is not set, should internal
>> pads still be reported?
> 
> We don't have client capabilities on MC side, at least not right now. The
> INTERNAL pads are intended to be used in very special circumstances and for
> that we do have sub-device capability flags. They were introduced in this
> cycle and if we allow wider access to them now, there could be issues
> blocking that as the interface they expose isn't intended to be used
> without these capability flags.

So are the internal pads exposed or not through these MC ioctls? That's not clear
to me. It's not documented either.

> 
> I'll rework this to apply to the rest of the pad-related IOCTLs.
> 
>>
>> And you need checks in v4l2-compliance, ensuring that trying to access internal pads without
>> that cap will indeed fail.
> 
> I can add that.
> 
>>
>> Ideally you would like to have this emulated in vimc as well.
>>
>> In other words, I think this needs more work, both in the core and w.r.t. documentation.
> 
> The INTERNAL pad flag was introduced as the Maxim serdes driver needed it
> and we thought it's fine to merge it early; the bulk of the documentation
> resides in the depths of my metadata series.
> 

Ah, that's not something I was aware of (or may have been aware of, but forgotten). Adding
a new uAPI requires a lot more care: proper documentation, v4l2-compliance tests, if at all
possible emulation support in one of the test drivers.

Neither was it reviewed by the media maintainers (me or Mauro). I'm really not happy about
that. uAPI changes have to be reviewed by the media maintainers.

Before rc1 is released you must have a patch series ready (and reviewed by me) adding proper
documentation, fixing issues like the one addressed in this patch, and a patch for
v4l2-compliance.

Ideally also a patch to emulate this in the test driver (probably vimc), but that will
likely take more time.

If it is not properly documented etc., then I might decide to revert this driver + uAPI
change in 7.4-rcX.

As media committer it is your responsibility to ensure that the media maintainers have
reviewed uAPI changes and are OK with it. That clearly didn't happen here.

Regards,

	Hans