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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <85906cb2-5472-2c54-e854-136cba8e8d40@intel.com>
Date:   Thu, 24 Jan 2019 15:11:53 +0800
From:   xinwu <xinwu.liu@...el.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     mchehab@...nel.org, hans.verkuil@...co.com,
        niklas.soderlund+renesas@...natech.se, ezequiel@...labora.com,
        sque@...omium.org, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: v4l2-core: expose the device after it was
 registered.

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@...el.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: