Re: [PATCHv2 1/2] v4l2-dev: add /sys media_dev attr for video devices

[Hans Verkuil <hverkuil-cisco@xs4all.nl>]

On 01/02/2021 21:26, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Mon, Feb 01, 2021 at 10:36:58AM +0100, Hans Verkuil wrote:
>> Create a media_dev attribute in /sys for each video device
>> which contains the media device major and minor number (or
>> is empty if there is no associated media device).
>>
>> It is not created if the CONFIG_MEDIA_CONTROLLER is not
>> defined.
>>
>> This makes it possible for applications like v4l2-compliance
>> to find the associated media controller of a video device.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/v4l2-core/v4l2-dev.c | 48 +++++++++++++++++++++++++++++-
>>  1 file changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index b6a72d297775..85b94b25aba2 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -87,13 +87,59 @@ static ssize_t name_show(struct device *cd,
>>  }
>>  static DEVICE_ATTR_RO(name);
>>  
>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>> +static ssize_t media_dev_show(struct device *cd,
>> +			 struct device_attribute *attr, char *buf)
>> +{
>> +	struct video_device *vdev = to_video_device(cd);
>> +	struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
>> +
>> +	buf[0] = '\0';
>> +	if (!v4l2_dev->mdev)
>> +		return 0;
>> +	return sprintf(buf, "%u:%u\n",
>> +		       MAJOR(v4l2_dev->mdev->devnode->dev.devt),
>> +		       MINOR(v4l2_dev->mdev->devnode->dev.devt));
> 
> Could v4l2-dev->mdev be set to null between time of check and time of
> use, or are sysfs properties guaranteed to be removed first ?

After checking device_del() I see that these attributes are removed
before the device node itself is removed. However, I am not 100% certain
that all drivers will postpone unregistering the media device node until
all other device nodes are unregistered.

I think it would be safer to copy v4l2_dev->mdev->devnode->dev.devt into
struct video_device at registration time. It's more robust.

> 
> I'm still not convinced that this is the right way to go from a
> userspace point of view. I believe we should shift from the paradigm of
> a video node belonging to a media device to a media device that contains
> video nodes. This means that userspace should use the media device as
> the entry point, and find video nodes from the media graph, instead of
> going the other way around. That's the only sensible way to handle
> complex devices, and is really a mindset change that should be pushed to
> all userspace applications. It will obviously take time and effort, but
> if we don't start by eating our own dogfood, we'll never succeed.
> 
> I'm thus not opposed to this patch series so much that I would like it
> to not being merged, but I believe it's a step in the wrong direction.
> With time I've learnt that I can't prevent every step I consider wrong
> to be taken (and I also make mistakes, so who knows :-)).

I completely agree with you, but the reality is that many V4L2 drivers do
not use the media controller, and that is not something that will change.

I honestly do not see why having a reference to the actual associated media
device would be a bad thing: it will only ensure that v4l2-ctl/compliance
can tell the user that that device is part of a media controller.

I don't see how or why applications would want to abuse this.

I'll post a v3 of this series.

Regards,

	Hans

> 
>> +}
>> +
>> +static DEVICE_ATTR_RO(media_dev);
>> +#endif
>> +
>> +static umode_t video_device_attr_is_visible(struct kobject *kobj,
>> +					    struct attribute *attr, int n)
>> +{
>> +	struct video_device *vdev = to_video_device(kobj_to_dev(kobj));
>> +
>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>> +	if (attr == &dev_attr_media_dev.attr) {
>> +		struct v4l2_device *v4l2_dev = vdev->v4l2_dev;
>> +
>> +		if (!v4l2_dev->mdev)
>> +			return 0;
>> +	}
>> +#endif
>> +	return attr->mode;
>> +}
>> +
>>  static struct attribute *video_device_attrs[] = {
>>  	&dev_attr_name.attr,
>>  	&dev_attr_dev_debug.attr,
>>  	&dev_attr_index.attr,
>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>> +	&dev_attr_media_dev.attr,
>> +#endif
>>  	NULL,
>>  };
>> -ATTRIBUTE_GROUPS(video_device);
>> +
>> +static const struct attribute_group video_device_group = {
>> +	.is_visible = video_device_attr_is_visible,
>> +	.attrs = video_device_attrs,
>> +};
>> +
>> +static const struct attribute_group *video_device_groups[] = {
>> +	&video_device_group,
>> +	NULL
>> +};
>>  
>>  /*
>>   *	Active devices
>