| Message ID | 20231220103713.113386-16-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Media device lifetime management | expand |
On 20/12/2023 11:36, Sakari Ailus wrote: > Assign the media device minor to -1 if it has not been previously > assigned. There's no dependence to this yes but it will be required by yes -> yet > patch "media: mc: Implement best effort media device removal safety sans > refcount" soon. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> After fixing that somewhat confusing typo above: Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/mc/mc-devnode.c | 9 ++++++++- > include/media/media-devnode.h | 2 +- > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c > index 717408791a7c..5057c48f8870 100644 > --- a/drivers/media/mc/mc-devnode.c > +++ b/drivers/media/mc/mc-devnode.c > @@ -59,7 +59,8 @@ static void media_devnode_release(struct device *cd) > { > struct media_devnode *devnode = to_media_devnode(cd); > > - media_devnode_free_minor(devnode->minor); > + if (devnode->minor != -1) > + media_devnode_free_minor(devnode->minor); > > /* Release media_devnode and perform other cleanups as needed. */ > if (devnode->release) > @@ -212,6 +213,7 @@ static const struct file_operations media_devnode_fops = { > void media_devnode_init(struct media_devnode *devnode) > { > device_initialize(&devnode->dev); > + devnode->minor = -1; > } > > int __must_check media_devnode_register(struct media_devnode *devnode, > @@ -220,6 +222,9 @@ int __must_check media_devnode_register(struct media_devnode *devnode, > int minor; > int ret; > > + if (devnode->minor != -1) > + return -EINVAL; > + > /* Part 1: Find a free minor number */ > mutex_lock(&media_devnode_lock); > minor = find_first_zero_bit(media_devnode_nums, MEDIA_NUM_DEVICES); > @@ -261,6 +266,8 @@ int __must_check media_devnode_register(struct media_devnode *devnode, > cdev_add_error: > media_devnode_free_minor(devnode->minor); > > + devnode->minor = -1; > + > return ret; > } > > diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h > index 6d46c658be21..d050f54f252a 100644 > --- a/include/media/media-devnode.h > +++ b/include/media/media-devnode.h > @@ -60,7 +60,7 @@ struct media_file_operations { > * @dev: pointer to struct &device containing the media controller device > * @cdev: struct cdev pointer character device > * @parent: parent device > - * @minor: device node minor number > + * @minor: device node minor number, -1 if unassigned > * @flags: flags, combination of the ``MEDIA_FLAG_*`` constants > * @release: release callback called at the end of ``media_devnode_release()`` > * routine at media-device.c.
Hi Sakari, Thank you for the patch. On Wed, Dec 20, 2023 at 12:36:59PM +0200, Sakari Ailus wrote: > Assign the media device minor to -1 if it has not been previously > assigned. There's no dependence to this yes but it will be required by > patch "media: mc: Implement best effort media device removal safety sans > refcount" soon. After a quick look at that patch, I don't see the dependency I'm afraid. Could you please explain it better ? > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/mc/mc-devnode.c | 9 ++++++++- > include/media/media-devnode.h | 2 +- > 2 files changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c > index 717408791a7c..5057c48f8870 100644 > --- a/drivers/media/mc/mc-devnode.c > +++ b/drivers/media/mc/mc-devnode.c > @@ -59,7 +59,8 @@ static void media_devnode_release(struct device *cd) > { > struct media_devnode *devnode = to_media_devnode(cd); > > - media_devnode_free_minor(devnode->minor); > + if (devnode->minor != -1) > + media_devnode_free_minor(devnode->minor); Should the test be moved to media_devnode_free_minor() ? > > /* Release media_devnode and perform other cleanups as needed. */ > if (devnode->release) > @@ -212,6 +213,7 @@ static const struct file_operations media_devnode_fops = { > void media_devnode_init(struct media_devnode *devnode) > { > device_initialize(&devnode->dev); > + devnode->minor = -1; > } > > int __must_check media_devnode_register(struct media_devnode *devnode, > @@ -220,6 +222,9 @@ int __must_check media_devnode_register(struct media_devnode *devnode, > int minor; > int ret; > > + if (devnode->minor != -1) > + return -EINVAL; > + > /* Part 1: Find a free minor number */ > mutex_lock(&media_devnode_lock); > minor = find_first_zero_bit(media_devnode_nums, MEDIA_NUM_DEVICES); > @@ -261,6 +266,8 @@ int __must_check media_devnode_register(struct media_devnode *devnode, > cdev_add_error: > media_devnode_free_minor(devnode->minor); > > + devnode->minor = -1; > + > return ret; > } > > diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h > index 6d46c658be21..d050f54f252a 100644 > --- a/include/media/media-devnode.h > +++ b/include/media/media-devnode.h > @@ -60,7 +60,7 @@ struct media_file_operations { > * @dev: pointer to struct &device containing the media controller device > * @cdev: struct cdev pointer character device > * @parent: parent device > - * @minor: device node minor number > + * @minor: device node minor number, -1 if unassigned > * @flags: flags, combination of the ``MEDIA_FLAG_*`` constants > * @release: release callback called at the end of ``media_devnode_release()`` > * routine at media-device.c.
Hi Laurent, On Wed, Feb 07, 2024 at 12:58:15PM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Wed, Dec 20, 2023 at 12:36:59PM +0200, Sakari Ailus wrote: > > Assign the media device minor to -1 if it has not been previously > > assigned. There's no dependence to this yes but it will be required by > > patch "media: mc: Implement best effort media device removal safety sans > > refcount" soon. > > After a quick look at that patch, I don't see the dependency I'm afraid. > Could you please explain it better ? > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/mc/mc-devnode.c | 9 ++++++++- > > include/media/media-devnode.h | 2 +- > > 2 files changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c > > index 717408791a7c..5057c48f8870 100644 > > --- a/drivers/media/mc/mc-devnode.c > > +++ b/drivers/media/mc/mc-devnode.c > > @@ -59,7 +59,8 @@ static void media_devnode_release(struct device *cd) > > { > > struct media_devnode *devnode = to_media_devnode(cd); > > > > - media_devnode_free_minor(devnode->minor); > > + if (devnode->minor != -1) > > + media_devnode_free_minor(devnode->minor); > > Should the test be moved to media_devnode_free_minor() ? The intention here is to free the minor number once and only once. Moving the test to media_devnode_free_minor() is not an option as the devnode may be released already by that time. See the patch the commit message refers to. > > > > > /* Release media_devnode and perform other cleanups as needed. */ > > if (devnode->release) > > @@ -212,6 +213,7 @@ static const struct file_operations media_devnode_fops = { > > void media_devnode_init(struct media_devnode *devnode) > > { > > device_initialize(&devnode->dev); > > + devnode->minor = -1; > > } > > > > int __must_check media_devnode_register(struct media_devnode *devnode, > > @@ -220,6 +222,9 @@ int __must_check media_devnode_register(struct media_devnode *devnode, > > int minor; > > int ret; > > > > + if (devnode->minor != -1) > > + return -EINVAL; > > + > > /* Part 1: Find a free minor number */ > > mutex_lock(&media_devnode_lock); > > minor = find_first_zero_bit(media_devnode_nums, MEDIA_NUM_DEVICES); > > @@ -261,6 +266,8 @@ int __must_check media_devnode_register(struct media_devnode *devnode, > > cdev_add_error: > > media_devnode_free_minor(devnode->minor); > > > > + devnode->minor = -1; > > + > > return ret; > > } > > > > diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h > > index 6d46c658be21..d050f54f252a 100644 > > --- a/include/media/media-devnode.h > > +++ b/include/media/media-devnode.h > > @@ -60,7 +60,7 @@ struct media_file_operations { > > * @dev: pointer to struct &device containing the media controller device > > * @cdev: struct cdev pointer character device > > * @parent: parent device > > - * @minor: device node minor number > > + * @minor: device node minor number, -1 if unassigned > > * @flags: flags, combination of the ``MEDIA_FLAG_*`` constants > > * @release: release callback called at the end of ``media_devnode_release()`` > > * routine at media-device.c. >
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c index 717408791a7c..5057c48f8870 100644 --- a/drivers/media/mc/mc-devnode.c +++ b/drivers/media/mc/mc-devnode.c @@ -59,7 +59,8 @@ static void media_devnode_release(struct device *cd) { struct media_devnode *devnode = to_media_devnode(cd); - media_devnode_free_minor(devnode->minor); + if (devnode->minor != -1) + media_devnode_free_minor(devnode->minor); /* Release media_devnode and perform other cleanups as needed. */ if (devnode->release) @@ -212,6 +213,7 @@ static const struct file_operations media_devnode_fops = { void media_devnode_init(struct media_devnode *devnode) { device_initialize(&devnode->dev); + devnode->minor = -1; } int __must_check media_devnode_register(struct media_devnode *devnode, @@ -220,6 +222,9 @@ int __must_check media_devnode_register(struct media_devnode *devnode, int minor; int ret; + if (devnode->minor != -1) + return -EINVAL; + /* Part 1: Find a free minor number */ mutex_lock(&media_devnode_lock); minor = find_first_zero_bit(media_devnode_nums, MEDIA_NUM_DEVICES); @@ -261,6 +266,8 @@ int __must_check media_devnode_register(struct media_devnode *devnode, cdev_add_error: media_devnode_free_minor(devnode->minor); + devnode->minor = -1; + return ret; } diff --git a/include/media/media-devnode.h b/include/media/media-devnode.h index 6d46c658be21..d050f54f252a 100644 --- a/include/media/media-devnode.h +++ b/include/media/media-devnode.h @@ -60,7 +60,7 @@ struct media_file_operations { * @dev: pointer to struct &device containing the media controller device * @cdev: struct cdev pointer character device * @parent: parent device - * @minor: device node minor number + * @minor: device node minor number, -1 if unassigned * @flags: flags, combination of the ``MEDIA_FLAG_*`` constants * @release: release callback called at the end of ``media_devnode_release()`` * routine at media-device.c.
Assign the media device minor to -1 if it has not been previously assigned. There's no dependence to this yes but it will be required by patch "media: mc: Implement best effort media device removal safety sans refcount" soon. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/mc/mc-devnode.c | 9 ++++++++- include/media/media-devnode.h | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-)