diff mbox series

[6/6] USB: file.c: make usb class a static const structure

Message ID 20230620094412.508580-12-gregkh@linuxfoundation.org (mailing list archive)
State New, archived
Headers show
Series [1/6] USB: roles: make role_class a static const structure | expand

Commit Message

Greg KH June 20, 2023, 9:44 a.m. UTC 
From: Ivan Orlov <ivan.orlov0322@gmail.com>

Now that the driver core allows for struct class to be in read-only
memory, remove the class field of the usb_class structure and
create the usbmisc_class static class structure declared at build time
which places it into read-only memory, instead of having it to be
dynamically allocated at load time.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/usb/core/file.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

Comments

Greg KH June 20, 2023, 2:22 p.m. UTC | #1 
On Tue, Jun 20, 2023 at 11:44:18AM +0200, Greg Kroah-Hartman wrote:
> From: Ivan Orlov <ivan.orlov0322@gmail.com>
> 
> Now that the driver core allows for struct class to be in read-only
> memory, remove the class field of the usb_class structure and
> create the usbmisc_class static class structure declared at build time
> which places it into read-only memory, instead of having it to be
> dynamically allocated at load time.
> 
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Ivan Orlov <ivan.orlov0322@gmail.com>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/usb/core/file.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
> index c4ed3310e069..0e16a9c048dd 100644
> --- a/drivers/usb/core/file.c
> +++ b/drivers/usb/core/file.c
> @@ -59,7 +59,6 @@ static const struct file_operations usb_fops = {
>  
>  static struct usb_class {
>  	struct kref kref;
> -	struct class *class;
>  } *usb_class;

Is this structure needed anymore at all now that the thing the kref was
"protecting" is gone?  I think it can be dropped entirely, right?

thanks,

greg k-h
Ivan Orlov June 21, 2023, 11:08 a.m. UTC | #2 
On 20.06.2023 18:22, Greg Kroah-Hartman wrote:
> Is this structure needed anymore at all now that the thing the kref was
> "protecting" is gone?  I think it can be dropped entirely, right?
> 
> thanks,
> 
> greg k-h

As I understood after reading the code, this kref is used for checking 
how many devices uses our class and testing when we can unregister it.

As we register our class only when the first device is registered, I 
think the best solution is to move this kref structure out of the 
usb_class structure as we still need it for detecting when we could 
finally unregister our class. What do you think about it?
Greg KH June 21, 2023, 12:48 p.m. UTC | #3 
On Wed, Jun 21, 2023 at 03:08:07PM +0400, Ivan Orlov wrote:
> On 20.06.2023 18:22, Greg Kroah-Hartman wrote:
> > Is this structure needed anymore at all now that the thing the kref was
> > "protecting" is gone?  I think it can be dropped entirely, right?
> > 
> > thanks,
> > 
> > greg k-h
> 
> As I understood after reading the code, this kref is used for checking how
> many devices uses our class and testing when we can unregister it.
> 
> As we register our class only when the first device is registered, I think
> the best solution is to move this kref structure out of the usb_class
> structure as we still need it for detecting when we could finally unregister
> our class. What do you think about it?

I think we should make it simpler, allocate the class when we start up,
and free it when we shut down, which guarantees that all users of the
class are removed at that time as this is part of the usb core code.

No need to be fancy anymore with the dynamic creation/removal of the
class, it's just not worth it :)

thanks,

greg k-h
Ivan Orlov June 21, 2023, 1:06 p.m. UTC | #4 
On 6/21/23 16:48, Greg Kroah-Hartman wrote:
> I think we should make it simpler, allocate the class when we start up,
> and free it when we shut down, which guarantees that all users of the
> class are removed at that time as this is part of the usb core code.
> 
> No need to be fancy anymore with the dynamic creation/removal of the
> class, it's just not worth it :)
> 
> thanks,
> 
> greg k-h

Alright, it sounds really reasonable, let's do it in this way :)

I'll add init_usb_class call to the 'usb_init' function and the 
corresponding releasing function call to the 'usb_exit' function in the 
'drivers/usb/core/usb.c' file. So we would register class at startup and 
unregister it when shutting down.
Greg KH June 21, 2023, 1:29 p.m. UTC | #5 
On Wed, Jun 21, 2023 at 05:06:34PM +0400, Ivan Orlov wrote:
> On 6/21/23 16:48, Greg Kroah-Hartman wrote:
> > I think we should make it simpler, allocate the class when we start up,
> > and free it when we shut down, which guarantees that all users of the
> > class are removed at that time as this is part of the usb core code.
> > 
> > No need to be fancy anymore with the dynamic creation/removal of the
> > class, it's just not worth it :)
> > 
> > thanks,
> > 
> > greg k-h
> 
> Alright, it sounds really reasonable, let's do it in this way :)
> 
> I'll add init_usb_class call to the 'usb_init' function and the
> corresponding releasing function call to the 'usb_exit' function in the
> 'drivers/usb/core/usb.c' file. So we would register class at startup and
> unregister it when shutting down.
> 

Totally reasonable, thanks for doing this work.

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
index c4ed3310e069..0e16a9c048dd 100644
--- a/drivers/usb/core/file.c
+++ b/drivers/usb/core/file.c
@@ -59,7 +59,6 @@  static const struct file_operations usb_fops = {
 
 static struct usb_class {
 	struct kref kref;
-	struct class *class;
 } *usb_class;
 
 static char *usb_devnode(const struct device *dev, umode_t *mode)
@@ -72,6 +71,11 @@  static char *usb_devnode(const struct device *dev, umode_t *mode)
 	return drv->devnode(dev, mode);
 }
 
+static struct class usbmisc_class = {
+	.name		= "usbmisc",
+	.devnode	= usb_devnode,
+};
+
 static int init_usb_class(void)
 {
 	int result = 0;
@@ -88,15 +92,12 @@  static int init_usb_class(void)
 	}
 
 	kref_init(&usb_class->kref);
-	usb_class->class = class_create("usbmisc");
-	if (IS_ERR(usb_class->class)) {
-		result = PTR_ERR(usb_class->class);
-		printk(KERN_ERR "class_create failed for usb devices\n");
+	result = class_register(&usbmisc_class);
+	if (result) {
 		kfree(usb_class);
 		usb_class = NULL;
 		goto exit;
 	}
-	usb_class->class->devnode = usb_devnode;
 
 exit:
 	return result;
@@ -105,7 +106,7 @@  static int init_usb_class(void)
 static void release_usb_class(struct kref *kref)
 {
 	/* Ok, we cheat as we know we only have one usb_class */
-	class_destroy(usb_class->class);
+	class_unregister(&usbmisc_class);
 	kfree(usb_class);
 	usb_class = NULL;
 }
@@ -200,7 +201,7 @@  int usb_register_dev(struct usb_interface *intf,
 
 	/* create a usb class device for this usb interface */
 	snprintf(name, sizeof(name), class_driver->name, minor - minor_base);
-	intf->usb_dev = device_create(usb_class->class, &intf->dev,
+	intf->usb_dev = device_create(&usbmisc_class, &intf->dev,
 				      MKDEV(USB_MAJOR, minor), class_driver,
 				      "%s", kbasename(name));
 	if (IS_ERR(intf->usb_dev)) {
@@ -234,7 +235,7 @@  void usb_deregister_dev(struct usb_interface *intf,
 		return;
 
 	dev_dbg(&intf->dev, "removing %d minor\n", intf->minor);
-	device_destroy(usb_class->class, MKDEV(USB_MAJOR, intf->minor));
+	device_destroy(&usbmisc_class, MKDEV(USB_MAJOR, intf->minor));
 
 	down_write(&minor_rwsem);
 	usb_minors[intf->minor] = NULL;