diff mbox series

[v4,10/26] media: mc: Clear minor number reservation at unregistration time

Message ID 20240610100530.1107771-11-sakari.ailus@linux.intel.com (mailing list archive)
State New
Headers
Series Media device lifetime management |

Commit Message

Sakari Ailus June 10, 2024, 10:05 a.m. UTC 
  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(-)

Comments

Hans Verkuil June 27, 2024, 6:43 a.m. UTC | #1 
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);
>  }
>  
>  /*
Sakari Ailus June 27, 2024, 6:58 a.m. UTC | #2 
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.
Sakari Ailus June 27, 2024, 7:10 a.m. UTC | #3 
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?
Hans Verkuil June 27, 2024, 7:22 a.m. UTC | #4 
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 mbox series

Patch

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);
 }
 
 /*