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@xxxxxxxxx>
> > > ---
> > >   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@xxxxxxxxxxxxxxx