[PATCH] media: v4l2-core: expose the device after it was registered.

[PATCH] media: v4l2-core: expose the device after it was registered.

[Liu, Xinwu]

device_register exposes the device to userspace.

Therefore, while the register process is ongoing, the userspace program
will fail to open the device (ENODEV will be set to errno currently).
The program in userspace must re-open the device to cover this case.

It is more reasonable to expose the device after everythings ready.

Signed-off-by: Liu, Xinwu <xinwu.liu@intel.com>
---
 drivers/media/v4l2-core/v4l2-dev.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index d7528f8..01e5cc9 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -993,16 +993,11 @@ int __video_register_device(struct video_device *vdev,
 		goto cleanup;
 	}
 
-	/* Part 4: register the device with sysfs */
+	/* Part 4: Prepare to register the device */
 	vdev->dev.class = &video_class;
 	vdev->dev.devt = MKDEV(VIDEO_MAJOR, vdev->minor);
 	vdev->dev.parent = vdev->dev_parent;
 	dev_set_name(&vdev->dev, "%s%d", name_base, vdev->num);
-	ret = device_register(&vdev->dev);
-	if (ret < 0) {
-		pr_err("%s: device_register failed\n", __func__);
-		goto cleanup;
-	}
 	/* Register the release callback that will be called when the last
 	   reference to the device goes away. */
 	vdev->dev.release = v4l2_device_release;
@@ -1020,6 +1015,13 @@ int __video_register_device(struct video_device *vdev,
 	/* Part 6: Activate this minor. The char device can now be used. */
 	set_bit(V4L2_FL_REGISTERED, &vdev->flags);
 
+	/* Part 7: Register the device with sysfs */
+	ret = device_register(&vdev->dev);
+	if (ret < 0) {
+		pr_err("%s: device_register failed\n", __func__);
+		goto cleanup;
+	}
+
 	return 0;
 
 cleanup:
-- 
2.7.4

Re: [PATCH] media: v4l2-core: expose the device after it was registered.

[Sakari Ailus]

Hi Xinwu,

On Tue, Jan 22, 2019 at 04:34:44PM +0800, Liu, Xinwu wrote:
> device_register exposes the device to userspace.
> 
> Therefore, while the register process is ongoing, the userspace program
> will fail to open the device (ENODEV will be set to errno currently).
> The program in userspace must re-open the device to cover this case.
> 
> It is more reasonable to expose the device after everythings ready.
> 
> Signed-off-by: Liu, Xinwu <xinwu.liu@intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index d7528f8..01e5cc9 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -993,16 +993,11 @@ int __video_register_device(struct video_device *vdev,
>  		goto cleanup;
>  	}
>  
> -	/* Part 4: register the device with sysfs */
> +	/* Part 4: Prepare to register the device */
>  	vdev->dev.class = &video_class;
>  	vdev->dev.devt = MKDEV(VIDEO_MAJOR, vdev->minor);
>  	vdev->dev.parent = vdev->dev_parent;
>  	dev_set_name(&vdev->dev, "%s%d", name_base, vdev->num);
> -	ret = device_register(&vdev->dev);
> -	if (ret < 0) {
> -		pr_err("%s: device_register failed\n", __func__);
> -		goto cleanup;
> -	}
>  	/* Register the release callback that will be called when the last
>  	   reference to the device goes away. */
>  	vdev->dev.release = v4l2_device_release;
> @@ -1020,6 +1015,13 @@ int __video_register_device(struct video_device *vdev,
>  	/* Part 6: Activate this minor. The char device can now be used. */
>  	set_bit(V4L2_FL_REGISTERED, &vdev->flags);
>  
> +	/* Part 7: Register the device with sysfs */
> +	ret = device_register(&vdev->dev);
> +	if (ret < 0) {
> +		pr_err("%s: device_register failed\n", __func__);
> +		goto cleanup;

The error handling needs to reflect the change as well.

Speaking of which, the error handling appears to be somewhat broken here.
It should be fixed first before making changes to the registration order.

The problem the patch addresses is related to another problem: how to tell
the user space all devices in the media hardware complex have been
registered successfully. Order generally has been the node first, and only
then the entity. That suggests the order should probably be:

1. video_register_media_controller
2. set_bit(V4L2_FL_REGISTERED)
3. device_register

I wonder what Hans thinks.

> +	}
> +
>  	return 0;
>  
>  cleanup:

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH] media: v4l2-core: expose the device after it was registered.

[xinwu]

Hi Sakari,

Thanks for your response.


On 2019年01月22日 18:03, Sakari Ailus wrote:
> Hi Xinwu,
>
> On Tue, Jan 22, 2019 at 04:34:44PM +0800, Liu, Xinwu wrote:
>> device_register exposes the device to userspace.
>>
>> Therefore, while the register process is ongoing, the userspace program
>> will fail to open the device (ENODEV will be set to errno currently).
>> The program in userspace must re-open the device to cover this case.
>>
>> It is more reasonable to expose the device after everythings ready.
>>
>> Signed-off-by: Liu, Xinwu <xinwu.liu@intel.com>
>> ---
>>   drivers/media/v4l2-core/v4l2-dev.c | 14 ++++++++------
>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index d7528f8..01e5cc9 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -993,16 +993,11 @@ int __video_register_device(struct video_device *vdev,
>>   		goto cleanup;
>>   	}
>>   
>> -	/* Part 4: register the device with sysfs */
>> +	/* Part 4: Prepare to register the device */
>>   	vdev->dev.class = &video_class;
>>   	vdev->dev.devt = MKDEV(VIDEO_MAJOR, vdev->minor);
>>   	vdev->dev.parent = vdev->dev_parent;
>>   	dev_set_name(&vdev->dev, "%s%d", name_base, vdev->num);
>> -	ret = device_register(&vdev->dev);
>> -	if (ret < 0) {
>> -		pr_err("%s: device_register failed\n", __func__);
>> -		goto cleanup;
>> -	}
>>   	/* Register the release callback that will be called when the last
>>   	   reference to the device goes away. */
>>   	vdev->dev.release = v4l2_device_release;
>> @@ -1020,6 +1015,13 @@ int __video_register_device(struct video_device *vdev,
>>   	/* Part 6: Activate this minor. The char device can now be used. */
>>   	set_bit(V4L2_FL_REGISTERED, &vdev->flags);
>>   
>> +	/* Part 7: Register the device with sysfs */
>> +	ret = device_register(&vdev->dev);
>> +	if (ret < 0) {
>> +		pr_err("%s: device_register failed\n", __func__);
>> +		goto cleanup;
> The error handling needs to reflect the change as well.

Yes, I think it need to clear the V4L2_FL_REGISTERED also.
>
> Speaking of which, the error handling appears to be somewhat broken here.
> It should be fixed first before making changes to the registration order.

I guess you mean to call "put_device()" in this error handling.
>
> The problem the patch addresses is related to another problem: how to tell
> the user space all devices in the media hardware complex have been
> registered successfully. Order generally has been the node first, and only
> then the entity. That suggests the order should probably be:
>
> 1. video_register_media_controller
> 2. set_bit(V4L2_FL_REGISTERED)
> 3. device_register
>
> I wonder what Hans thinks.

Yes, that's my suggestion, thanks for the highlight. I also want to know 
if it's worth to make this change.
>
>> +	}
>> +
>>   	return 0;
>>   
>>   cleanup:

Re: [PATCH] media: v4l2-core: expose the device after it was registered.

[Sakari Ailus]

On Thu, Jan 24, 2019 at 03:11:53PM +0800, xinwu wrote:
> Hi Sakari,
> 
> Thanks for your response.
> 
> 
> On 2019年01月22日 18:03, Sakari Ailus wrote:
> > Hi Xinwu,
> > 
> > On Tue, Jan 22, 2019 at 04:34:44PM +0800, Liu, Xinwu wrote:
> > > device_register exposes the device to userspace.
> > > 
> > > Therefore, while the register process is ongoing, the userspace program
> > > will fail to open the device (ENODEV will be set to errno currently).
> > > The program in userspace must re-open the device to cover this case.
> > > 
> > > It is more reasonable to expose the device after everythings ready.
> > > 
> > > Signed-off-by: Liu, Xinwu <xinwu.liu@intel.com>
> > > ---
> > >   drivers/media/v4l2-core/v4l2-dev.c | 14 ++++++++------
> > >   1 file changed, 8 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > > index d7528f8..01e5cc9 100644
> > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > @@ -993,16 +993,11 @@ int __video_register_device(struct video_device *vdev,
> > >   		goto cleanup;
> > >   	}
> > > -	/* Part 4: register the device with sysfs */
> > > +	/* Part 4: Prepare to register the device */
> > >   	vdev->dev.class = &video_class;
> > >   	vdev->dev.devt = MKDEV(VIDEO_MAJOR, vdev->minor);
> > >   	vdev->dev.parent = vdev->dev_parent;
> > >   	dev_set_name(&vdev->dev, "%s%d", name_base, vdev->num);
> > > -	ret = device_register(&vdev->dev);
> > > -	if (ret < 0) {
> > > -		pr_err("%s: device_register failed\n", __func__);
> > > -		goto cleanup;
> > > -	}
> > >   	/* Register the release callback that will be called when the last
> > >   	   reference to the device goes away. */
> > >   	vdev->dev.release = v4l2_device_release;
> > > @@ -1020,6 +1015,13 @@ int __video_register_device(struct video_device *vdev,
> > >   	/* Part 6: Activate this minor. The char device can now be used. */
> > >   	set_bit(V4L2_FL_REGISTERED, &vdev->flags);
> > > +	/* Part 7: Register the device with sysfs */
> > > +	ret = device_register(&vdev->dev);
> > > +	if (ret < 0) {
> > > +		pr_err("%s: device_register failed\n", __func__);
> > > +		goto cleanup;
> > The error handling needs to reflect the change as well.
> 
> Yes, I think it need to clear the V4L2_FL_REGISTERED also.
> > 
> > Speaking of which, the error handling appears to be somewhat broken here.
> > It should be fixed first before making changes to the registration order.
> 
> I guess you mean to call "put_device()" in this error handling.

No, error handling is broken even without this patch: the return value from
video_register_media_controller() is ignored, for instance.

> > 
> > The problem the patch addresses is related to another problem: how to tell
> > the user space all devices in the media hardware complex have been
> > registered successfully. Order generally has been the node first, and only
> > then the entity. That suggests the order should probably be:
> > 
> > 1. video_register_media_controller
> > 2. set_bit(V4L2_FL_REGISTERED)
> > 3. device_register
> > 
> > I wonder what Hans thinks.
> 
> Yes, that's my suggestion, thanks for the highlight. I also want to know if
> it's worth to make this change.
> > 
> > > +	}
> > > +
> > >   	return 0;
> > >   cleanup:
> 

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH] media: v4l2-core: expose the device after it was registered.

[Hans Verkuil]

On 1/24/19 8:11 AM, xinwu wrote:
> Hi Sakari,
> 
> Thanks for your response.
> 
> 
> On 2019年01月22日 18:03, Sakari Ailus wrote:
>> Hi Xinwu,
>>
>> On Tue, Jan 22, 2019 at 04:34:44PM +0800, Liu, Xinwu wrote:
>>> device_register exposes the device to userspace.
>>>
>>> Therefore, while the register process is ongoing, the userspace program
>>> will fail to open the device (ENODEV will be set to errno currently).
>>> The program in userspace must re-open the device to cover this case.
>>>
>>> It is more reasonable to expose the device after everythings ready.
>>>
>>> Signed-off-by: Liu, Xinwu <xinwu.liu@intel.com>
>>> ---
>>>   drivers/media/v4l2-core/v4l2-dev.c | 14 ++++++++------
>>>   1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>>> index d7528f8..01e5cc9 100644
>>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>>> @@ -993,16 +993,11 @@ int __video_register_device(struct video_device *vdev,
>>>           goto cleanup;
>>>       }
>>>   -    /* Part 4: register the device with sysfs */
>>> +    /* Part 4: Prepare to register the device */
>>>       vdev->dev.class = &video_class;
>>>       vdev->dev.devt = MKDEV(VIDEO_MAJOR, vdev->minor);
>>>       vdev->dev.parent = vdev->dev_parent;
>>>       dev_set_name(&vdev->dev, "%s%d", name_base, vdev->num);
>>> -    ret = device_register(&vdev->dev);
>>> -    if (ret < 0) {
>>> -        pr_err("%s: device_register failed\n", __func__);
>>> -        goto cleanup;
>>> -    }
>>>       /* Register the release callback that will be called when the last
>>>          reference to the device goes away. */
>>>       vdev->dev.release = v4l2_device_release;
>>> @@ -1020,6 +1015,13 @@ int __video_register_device(struct video_device *vdev,
>>>       /* Part 6: Activate this minor. The char device can now be used. */
>>>       set_bit(V4L2_FL_REGISTERED, &vdev->flags);
>>>   +    /* Part 7: Register the device with sysfs */
>>> +    ret = device_register(&vdev->dev);
>>> +    if (ret < 0) {
>>> +        pr_err("%s: device_register failed\n", __func__);
>>> +        goto cleanup;
>> The error handling needs to reflect the change as well.
> 
> Yes, I think it need to clear the V4L2_FL_REGISTERED also.
>>
>> Speaking of which, the error handling appears to be somewhat broken here.
>> It should be fixed first before making changes to the registration order.
> 
> I guess you mean to call "put_device()" in this error handling.
>>
>> The problem the patch addresses is related to another problem: how to tell
>> the user space all devices in the media hardware complex have been
>> registered successfully. Order generally has been the node first, and only
>> then the entity. That suggests the order should probably be:
>>
>> 1. video_register_media_controller
>> 2. set_bit(V4L2_FL_REGISTERED)
>> 3. device_register
>>
>> I wonder what Hans thinks.

Sorry for the late reply, I missed this question.

The core problem is that video_register_device sets up all the internal
data structures *and* creates the device node. This should really be split up
(as is done in other subsystems) into an allocate that does everything except
create the device node, and a register that actually does that last step.

The lack of this separation means that a device that has to register multiple
devices can't have a consistent valid state before it starts registering
devices.

Compare it to drivers/media/cec/cec-core.c where this is done a lot better.

That said, it would be a huge job reworking this. Although it can be done
by creating new functions separate from the existing ones. E.g.:

int v4l2_dev_initialize(struct video_device *vdev);
int v4l2_dev_register(struct video_device *vdev);

v4l2_dev_initialize does everything except registering the device node. That's
done by v4l2_dev_register. This way drivers can initialize everything first,
ensuring a fully consistent state before actually registering the device nodes.

I think this would be very nice to have.

That said, for the current function I agree that the order should indeed be
changed according to the proposal. It's just doesn't address the full problem.

Regards,

	Hans

> 
> Yes, that's my suggestion, thanks for the highlight. I also want to know if it's worth to make this change.
>>
>>> +    }
>>> +
>>>       return 0;
>>>     cleanup:
>