| Message ID | 20240610100530.1107771-11-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Media device lifetime management | expand |
On 10/06/2024 12:05, Sakari Ailus wrote: > Clear the media device's minor number reservation at unregister time as > there's no need to keep it reserved for longer. This makes it possible to > reserve the same minor right after unregistration. Have you tested this? I'm not certain whether this won't cause kobject errors. If an app has the media device open when the device is unbound, then the old device node still is around until the application closes the fh. I'm not sure if you can create a new device node with the same minor while an old device node with the same minor is still around. V4L2 and CEC definitely both keep the old minor until the last user has gone. Regards, Hans > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/mc/mc-devnode.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c > index 38c472498f9e..f7ecabe469a4 100644 > --- a/drivers/media/mc/mc-devnode.c > +++ b/drivers/media/mc/mc-devnode.c > @@ -49,13 +49,6 @@ static void media_devnode_release(struct device *cd) > { > struct media_devnode *devnode = to_media_devnode(cd); > > - mutex_lock(&media_devnode_lock); > - > - /* Mark device node number as free */ > - clear_bit(devnode->minor, media_devnode_nums); > - > - mutex_unlock(&media_devnode_lock); > - > /* Release media_devnode and perform other cleanups as needed. */ > if (devnode->release) > devnode->release(devnode); > @@ -268,6 +261,10 @@ void media_devnode_unregister(struct media_devnode *devnode) > > cdev_del(&devnode->cdev); > device_unregister(&devnode->dev); > + > + mutex_lock(&media_devnode_lock); > + clear_bit(devnode->minor, media_devnode_nums); > + mutex_unlock(&media_devnode_lock); > } > > /*
Hi Hans, On Thu, Jun 27, 2024 at 08:43:47AM +0200, Hans Verkuil wrote: > On 10/06/2024 12:05, Sakari Ailus wrote: > > Clear the media device's minor number reservation at unregister time as > > there's no need to keep it reserved for longer. This makes it possible to > > reserve the same minor right after unregistration. > > Have you tested this? By unbinding and re-binding a device while file handles to the old Media device one are still around. > > I'm not certain whether this won't cause kobject errors. If an app has the > media device open when the device is unbound, then the old device node still > is around until the application closes the fh. I'm not sure if you can create > a new device node with the same minor while an old device node with the same > minor is still around. I added the patch based on Laurent's comments <URL:https://lore.kernel.org/linux-media/20240207100810.GG23702@pendragon.ideasonboard.com/>. > > V4L2 and CEC definitely both keep the old minor until the last user has gone.
On Thu, Jun 27, 2024 at 06:58:03AM +0000, Sakari Ailus wrote: > Hi Hans, > > On Thu, Jun 27, 2024 at 08:43:47AM +0200, Hans Verkuil wrote: > > On 10/06/2024 12:05, Sakari Ailus wrote: > > > Clear the media device's minor number reservation at unregister time as > > > there's no need to keep it reserved for longer. This makes it possible to > > > reserve the same minor right after unregistration. > > > > Have you tested this? > > By unbinding and re-binding a device while file handles to the old Media > device one are still around. Also in MC we don't have an array indexed by minor numbers of Media devices, as we have for V4L2 device nodes. How is it for CEC?
On 27/06/2024 09:10, Sakari Ailus wrote: > On Thu, Jun 27, 2024 at 06:58:03AM +0000, Sakari Ailus wrote: >> Hi Hans, >> >> On Thu, Jun 27, 2024 at 08:43:47AM +0200, Hans Verkuil wrote: >>> On 10/06/2024 12:05, Sakari Ailus wrote: >>>> Clear the media device's minor number reservation at unregister time as >>>> there's no need to keep it reserved for longer. This makes it possible to >>>> reserve the same minor right after unregistration. >>> >>> Have you tested this? >> >> By unbinding and re-binding a device while file handles to the old Media >> device one are still around. > > Also in MC we don't have an array indexed by minor numbers of Media > devices, as we have for V4L2 device nodes. > > How is it for CEC? > It doesn't have such an array either. I think that's V4L2 specific, and I wonder if it is really needed. That code is very old, so it wouldn't surprise me if it can be removed. Regards, Hans
diff --git a/drivers/media/mc/mc-devnode.c b/drivers/media/mc/mc-devnode.c index 38c472498f9e..f7ecabe469a4 100644 --- a/drivers/media/mc/mc-devnode.c +++ b/drivers/media/mc/mc-devnode.c @@ -49,13 +49,6 @@ static void media_devnode_release(struct device *cd) { struct media_devnode *devnode = to_media_devnode(cd); - mutex_lock(&media_devnode_lock); - - /* Mark device node number as free */ - clear_bit(devnode->minor, media_devnode_nums); - - mutex_unlock(&media_devnode_lock); - /* Release media_devnode and perform other cleanups as needed. */ if (devnode->release) devnode->release(devnode); @@ -268,6 +261,10 @@ void media_devnode_unregister(struct media_devnode *devnode) cdev_del(&devnode->cdev); device_unregister(&devnode->dev); + + mutex_lock(&media_devnode_lock); + clear_bit(devnode->minor, media_devnode_nums); + mutex_unlock(&media_devnode_lock); } /*
Clear the media device's minor number reservation at unregister time as there's no need to keep it reserved for longer. This makes it possible to reserve the same minor right after unregistration. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/mc/mc-devnode.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)