| Message ID | 20231220103713.113386-5-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Media device lifetime management | expand |
Hi Sakari, Thank you for the patch. On Wed, Dec 20, 2023 at 12:36:48PM +0200, Sakari Ailus wrote: > From: Logan Gunthorpe <logang@deltatee.com> > > Replace the open coded registration of the cdev and dev with the > new device_add_cdev() helper. The helper replaces a common pattern by > taking the proper reference against the parent device and adding both > the cdev and the device. > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> This reapplies a commit you've reverted in 02/29 in this series. I understand this is done to be able to apply the revert in 03/29 cleanly. Given that those three patches are consecutive, wouldn't it be better to squash 02/29, 03/29 and 04/29, with the commit message of 03/29 ? Otherwise, I would at least drop the Acked-by and Reviewed-by tags in the patches you reapply, as they've been reviewed in a different context. The same applies to patches 05/29, 06/29 and 07/29. > --- > drivers/media/cec/core/cec-core.c | 16 ++++------------ > drivers/media/mc/mc-devnode.c | 23 +++++++++-------------- > 2 files changed, 13 insertions(+), 26 deletions(-) > > diff --git a/drivers/media/cec/core/cec-core.c b/drivers/media/cec/core/cec-core.c > index 0645e68411fb..15494b46458a 100644 > --- a/drivers/media/cec/core/cec-core.c > +++ b/drivers/media/cec/core/cec-core.c > @@ -137,26 +137,19 @@ static int __must_check cec_devnode_register(struct cec_devnode *devnode, > > /* Part 2: Initialize and register the character device */ > cdev_init(&devnode->cdev, &cec_devnode_fops); > - devnode->cdev.kobj.parent = &devnode->dev.kobj; > devnode->cdev.owner = owner; > kobject_set_name(&devnode->cdev.kobj, "cec%d", devnode->minor); > > devnode->registered = true; > - ret = cdev_add(&devnode->cdev, devnode->dev.devt, 1); > - if (ret < 0) { > - pr_err("%s: cdev_add failed\n", __func__); > + ret = cdev_device_add(&devnode->cdev, &devnode->dev); > + if (ret) { > + pr_err("%s: cdev_device_add failed\n", __func__); > devnode->registered = false; > goto clr_bit; > } > > - ret = device_add(&devnode->dev); > - if (ret) > - goto cdev_del; > - > return 0; > > -cdev_del: > - cdev_del(&devnode->cdev); > clr_bit: > mutex_lock(&cec_devnode_lock); > clear_bit(devnode->minor, cec_devnode_nums); > @@ -202,8 +195,7 @@ static void cec_devnode_unregister(struct cec_adapter *adap) > cec_adap_enable(adap); > mutex_unlock(&adap->lock); > > - device_del(&devnode->dev); > - cdev_del(&devnode->cdev); > + cdev_device_del(&devnode->cdev, &devnode->dev); > put_device(&devnode->dev); > } > > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c > index 1e1792c3ae3f..fabcd646679b 100644 > --- a/drivers/media/mc/mc-devnode.c > +++ b/drivers/media/mc/mc-devnode.c > @@ -232,29 +232,24 @@ int __must_check media_devnode_register(struct media_device *mdev, > devnode->minor = minor; > devnode->media_dev = mdev; > > - /* Part 2: Initialize and register the character device */ > + /* Part 2: Initialize the media and character devices */ > cdev_init(&devnode->cdev, &media_devnode_fops); > devnode->cdev.owner = owner; > kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor); > > - ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), > - devnode->minor), 1); > - if (ret < 0) { > - pr_err("%s: cdev_add failed\n", __func__); > - goto error; > - } > - > - /* Part 3: Register the media device */ > devnode->dev.bus = &media_bus_type; > devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor); > devnode->dev.release = media_devnode_release; > if (devnode->parent) > devnode->dev.parent = devnode->parent; > dev_set_name(&devnode->dev, "media%d", devnode->minor); > - ret = device_register(&devnode->dev); > + device_initialize(&devnode->dev); > + > + /* Part 3: Add the media and character devices */ > + ret = cdev_device_add(&devnode->cdev, &devnode->dev); > if (ret < 0) { > - pr_err("%s: device_register failed\n", __func__); > - goto error; > + pr_err("%s: cdev_device_add failed\n", __func__); > + goto cdev_add_error; > } > > /* Part 4: Activate this minor. The char device can now be used. */ > @@ -262,9 +257,9 @@ int __must_check media_devnode_register(struct media_device *mdev, > > return 0; > > -error: > +cdev_add_error: > mutex_lock(&media_devnode_lock); > - cdev_del(&devnode->cdev); > + cdev_device_del(&devnode->cdev, &devnode->dev); > clear_bit(devnode->minor, media_devnode_nums); > mutex_unlock(&media_devnode_lock); >
On Wed, Feb 07, 2024 at 11:38:18AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Wed, Dec 20, 2023 at 12:36:48PM +0200, Sakari Ailus wrote: > > From: Logan Gunthorpe <logang@deltatee.com> > > > > Replace the open coded registration of the cdev and dev with the > > new device_add_cdev() helper. The helper replaces a common pattern by > > taking the proper reference against the parent device and adding both > > the cdev and the device. > > > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > This reapplies a commit you've reverted in 02/29 in this series. I > understand this is done to be able to apply the revert in 03/29 cleanly. > Given that those three patches are consecutive, wouldn't it be better to > squash 02/29, 03/29 and 04/29, with the commit message of 03/29 ? > Otherwise, I would at least drop the Acked-by and Reviewed-by tags in > the patches you reapply, as they've been reviewed in a different > context. > > The same applies to patches 05/29, 06/29 and 07/29. And especially to those patches actually. 06/29 has a single line change for the uvcvideo driver, the revert in 05/29 and re-revert in 07/29 seem overkill. It would also be nice to expand the commit messages of 03/29 and 06/29 to explain why the revert are needed. > > --- > > drivers/media/cec/core/cec-core.c | 16 ++++------------ > > drivers/media/mc/mc-devnode.c | 23 +++++++++-------------- > > 2 files changed, 13 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/media/cec/core/cec-core.c b/drivers/media/cec/core/cec-core.c > > index 0645e68411fb..15494b46458a 100644 > > --- a/drivers/media/cec/core/cec-core.c > > +++ b/drivers/media/cec/core/cec-core.c > > @@ -137,26 +137,19 @@ static int __must_check cec_devnode_register(struct cec_devnode *devnode, > > > > /* Part 2: Initialize and register the character device */ > > cdev_init(&devnode->cdev, &cec_devnode_fops); > > - devnode->cdev.kobj.parent = &devnode->dev.kobj; > > devnode->cdev.owner = owner; > > kobject_set_name(&devnode->cdev.kobj, "cec%d", devnode->minor); > > > > devnode->registered = true; > > - ret = cdev_add(&devnode->cdev, devnode->dev.devt, 1); > > - if (ret < 0) { > > - pr_err("%s: cdev_add failed\n", __func__); > > + ret = cdev_device_add(&devnode->cdev, &devnode->dev); > > + if (ret) { > > + pr_err("%s: cdev_device_add failed\n", __func__); > > devnode->registered = false; > > goto clr_bit; > > } > > > > - ret = device_add(&devnode->dev); > > - if (ret) > > - goto cdev_del; > > - > > return 0; > > > > -cdev_del: > > - cdev_del(&devnode->cdev); > > clr_bit: > > mutex_lock(&cec_devnode_lock); > > clear_bit(devnode->minor, cec_devnode_nums); > > @@ -202,8 +195,7 @@ static void cec_devnode_unregister(struct cec_adapter *adap) > > cec_adap_enable(adap); > > mutex_unlock(&adap->lock); > > > > - device_del(&devnode->dev); > > - cdev_del(&devnode->cdev); > > + cdev_device_del(&devnode->cdev, &devnode->dev); > > put_device(&devnode->dev); > > } > > > > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c > > index 1e1792c3ae3f..fabcd646679b 100644 > > --- a/drivers/media/mc/mc-devnode.c > > +++ b/drivers/media/mc/mc-devnode.c > > @@ -232,29 +232,24 @@ int __must_check media_devnode_register(struct media_device *mdev, > > devnode->minor = minor; > > devnode->media_dev = mdev; > > > > - /* Part 2: Initialize and register the character device */ > > + /* Part 2: Initialize the media and character devices */ > > cdev_init(&devnode->cdev, &media_devnode_fops); > > devnode->cdev.owner = owner; > > kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor); > > > > - ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), > > - devnode->minor), 1); > > - if (ret < 0) { > > - pr_err("%s: cdev_add failed\n", __func__); > > - goto error; > > - } > > - > > - /* Part 3: Register the media device */ > > devnode->dev.bus = &media_bus_type; > > devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor); > > devnode->dev.release = media_devnode_release; > > if (devnode->parent) > > devnode->dev.parent = devnode->parent; > > dev_set_name(&devnode->dev, "media%d", devnode->minor); > > - ret = device_register(&devnode->dev); > > + device_initialize(&devnode->dev); > > + > > + /* Part 3: Add the media and character devices */ > > + ret = cdev_device_add(&devnode->cdev, &devnode->dev); > > if (ret < 0) { > > - pr_err("%s: device_register failed\n", __func__); > > - goto error; > > + pr_err("%s: cdev_device_add failed\n", __func__); > > + goto cdev_add_error; > > } > > > > /* Part 4: Activate this minor. The char device can now be used. */ > > @@ -262,9 +257,9 @@ int __must_check media_devnode_register(struct media_device *mdev, > > > > return 0; > > > > -error: > > +cdev_add_error: > > mutex_lock(&media_devnode_lock); > > - cdev_del(&devnode->cdev); > > + cdev_device_del(&devnode->cdev, &devnode->dev); > > clear_bit(devnode->minor, media_devnode_nums); > > mutex_unlock(&media_devnode_lock); > >
Hi Laurent, On Wed, Feb 07, 2024 at 11:51:37AM +0200, Laurent Pinchart wrote: > On Wed, Feb 07, 2024 at 11:38:18AM +0200, Laurent Pinchart wrote: > > Hi Sakari, > > > > Thank you for the patch. > > > > On Wed, Dec 20, 2023 at 12:36:48PM +0200, Sakari Ailus wrote: > > > From: Logan Gunthorpe <logang@deltatee.com> > > > > > > Replace the open coded registration of the cdev and dev with the > > > new device_add_cdev() helper. The helper replaces a common pattern by > > > taking the proper reference against the parent device and adding both > > > the cdev and the device. > > > > > > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > > > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > This reapplies a commit you've reverted in 02/29 in this series. I > > understand this is done to be able to apply the revert in 03/29 cleanly. > > Given that those three patches are consecutive, wouldn't it be better to > > squash 02/29, 03/29 and 04/29, with the commit message of 03/29 ? > > Otherwise, I would at least drop the Acked-by and Reviewed-by tags in > > the patches you reapply, as they've been reviewed in a different > > context. > > > > The same applies to patches 05/29, 06/29 and 07/29. > > And especially to those patches actually. 06/29 has a single line change > for the uvcvideo driver, the revert in 05/29 and re-revert in 07/29 seem > overkill. The revert can't be applied as-is otherwise but I'm fine merging them. > > It would also be nice to expand the commit messages of 03/29 and 06/29 > to explain why the revert are needed. I can add that. These are basically improvements in the code but depend on commit a087ce704b80. Conceptually, it'd get quite difficult as what's really needed here is to get back to an earlier state, this is not development over the said commit. I'll drop the acks.
diff --git a/drivers/media/cec/core/cec-core.c b/drivers/media/cec/core/cec-core.c index 0645e68411fb..15494b46458a 100644 --- a/drivers/media/cec/core/cec-core.c +++ b/drivers/media/cec/core/cec-core.c @@ -137,26 +137,19 @@ static int __must_check cec_devnode_register(struct cec_devnode *devnode, /* Part 2: Initialize and register the character device */ cdev_init(&devnode->cdev, &cec_devnode_fops); - devnode->cdev.kobj.parent = &devnode->dev.kobj; devnode->cdev.owner = owner; kobject_set_name(&devnode->cdev.kobj, "cec%d", devnode->minor); devnode->registered = true; - ret = cdev_add(&devnode->cdev, devnode->dev.devt, 1); - if (ret < 0) { - pr_err("%s: cdev_add failed\n", __func__); + ret = cdev_device_add(&devnode->cdev, &devnode->dev); + if (ret) { + pr_err("%s: cdev_device_add failed\n", __func__); devnode->registered = false; goto clr_bit; } - ret = device_add(&devnode->dev); - if (ret) - goto cdev_del; - return 0; -cdev_del: - cdev_del(&devnode->cdev); clr_bit: mutex_lock(&cec_devnode_lock); clear_bit(devnode->minor, cec_devnode_nums); @@ -202,8 +195,7 @@ static void cec_devnode_unregister(struct cec_adapter *adap) cec_adap_enable(adap); mutex_unlock(&adap->lock); - device_del(&devnode->dev); - cdev_del(&devnode->cdev); + cdev_device_del(&devnode->cdev, &devnode->dev); put_device(&devnode->dev); } diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c index 1e1792c3ae3f..fabcd646679b 100644 --- a/drivers/media/mc/mc-devnode.c +++ b/drivers/media/mc/mc-devnode.c @@ -232,29 +232,24 @@ int __must_check media_devnode_register(struct media_device *mdev, devnode->minor = minor; devnode->media_dev = mdev; - /* Part 2: Initialize and register the character device */ + /* Part 2: Initialize the media and character devices */ cdev_init(&devnode->cdev, &media_devnode_fops); devnode->cdev.owner = owner; kobject_set_name(&devnode->cdev.kobj, "media%d", devnode->minor); - ret = cdev_add(&devnode->cdev, MKDEV(MAJOR(media_dev_t), - devnode->minor), 1); - if (ret < 0) { - pr_err("%s: cdev_add failed\n", __func__); - goto error; - } - - /* Part 3: Register the media device */ devnode->dev.bus = &media_bus_type; devnode->dev.devt = MKDEV(MAJOR(media_dev_t), devnode->minor); devnode->dev.release = media_devnode_release; if (devnode->parent) devnode->dev.parent = devnode->parent; dev_set_name(&devnode->dev, "media%d", devnode->minor); - ret = device_register(&devnode->dev); + device_initialize(&devnode->dev); + + /* Part 3: Add the media and character devices */ + ret = cdev_device_add(&devnode->cdev, &devnode->dev); if (ret < 0) { - pr_err("%s: device_register failed\n", __func__); - goto error; + pr_err("%s: cdev_device_add failed\n", __func__); + goto cdev_add_error; } /* Part 4: Activate this minor. The char device can now be used. */ @@ -262,9 +257,9 @@ int __must_check media_devnode_register(struct media_device *mdev, return 0; -error: +cdev_add_error: mutex_lock(&media_devnode_lock); - cdev_del(&devnode->cdev); + cdev_device_del(&devnode->cdev, &devnode->dev); clear_bit(devnode->minor, media_devnode_nums); mutex_unlock(&media_devnode_lock);