Re: [PATCH] bcachefs: chardev: make bch_chardev_class constant

[Hongbo Li]

On 2024/3/6 19:50, Ricardo B. Marliere wrote:

On  6 Mar 09:23, Hongbo Li wrote:

On 2024/3/6 4:21, Ricardo B. Marliere wrote:

Since commit 43a7206b0963 ("driver core: class: make class_register() take
a const *"), the driver core allows for struct class to be in read-only
memory, so move the bch_chardev_class structure to be declared at build
time placing it into read-only memory, instead of having to be dynamically
allocated at boot time.

Cc: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Suggested-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Signed-off-by: Ricardo B. Marliere <rica...@marliere.net>
---
   fs/bcachefs/chardev.c | 23 ++++++++++++-----------
   1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/fs/bcachefs/chardev.c b/fs/bcachefs/chardev.c
index 226b39c17667..af587453fd3d 100644
--- a/fs/bcachefs/chardev.c
+++ b/fs/bcachefs/chardev.c
@@ -940,7 +940,9 @@ static const struct file_operations bch_chardev_fops = {
   };

static int bch_chardev_major;

-static struct class *bch_chardev_class;
+static const struct class bch_chardev_class = {
+       .name = "bcachefs",
+};
   static struct device *bch_chardev;

void bch2_fs_chardev_exit(struct bch_fs *c)

@@ -957,7 +959,7 @@ int bch2_fs_chardev_init(struct bch_fs *c)
        if (c->minor < 0)
                return c->minor;

- c->chardev = device_create(bch_chardev_class, NULL,

+       c->chardev = device_create(&bch_chardev_class, NULL,
                                   MKDEV(bch_chardev_major, c->minor), c,
                                   "bcachefs%u-ctl", c->minor);
        if (IS_ERR(c->chardev))
@@ -968,26 +970,25 @@ int bch2_fs_chardev_init(struct bch_fs *c)

void bch2_chardev_exit(void)

   {
-       if (!IS_ERR_OR_NULL(bch_chardev_class))
-               device_destroy(bch_chardev_class,
-                              MKDEV(bch_chardev_major, U8_MAX));
-       if (!IS_ERR_OR_NULL(bch_chardev_class))
-               class_destroy(bch_chardev_class);
+       device_destroy(&bch_chardev_class, MKDEV(bch_chardev_major, U8_MAX));
+       class_unregister(&bch_chardev_class);
        if (bch_chardev_major > 0)
                unregister_chrdev(bch_chardev_major, "bcachefs");
   }

int __init bch2_chardev_init(void)

   {
+       int ret;
+
        bch_chardev_major = register_chrdev(0, "bcachefs-ctl", 
&bch_chardev_fops);
        if (bch_chardev_major < 0)
                return bch_chardev_major;

- bch_chardev_class = class_create("bcachefs");

-       if (IS_ERR(bch_chardev_class))
-               return PTR_ERR(bch_chardev_class);
+       ret = class_register(&bch_chardev_class);
+       if (ret)

Here, I think you should call class_unregister to relase resource which
allocated before. And the same thing shoud be done in other exception exit.

Hi Hongbo,

Thank you for the feedback. Did you mean that bch_chardev_major should
be unregistered if the class_register() call fails? Because if it does,
there is no need to call class_unregister().

+               return ret;

- bch_chardev = device_create(bch_chardev_class, NULL,

+       bch_chardev = device_create(&bch_chardev_class, NULL,
                                    MKDEV(bch_chardev_major, U8_MAX),
                                    NULL, "bcachefs-ctl");
        if (IS_ERR(bch_chardev))

like here..

Can you please elaborate?

Yes, this I mean. The example like this:

```c
   major = register_chrdev(0, xx, xx);
   class = class_create("bcachefs");
   if (IS_ERR(class))
        goto out_chrdev:

   ...
out_class:
    class_unregister(xxx);
out_chrdev:
    unregister_chrdev(xx, xx);
    ...
```

Although this was not part of your original intention, it is indeed a flaw.

Best regards,
-       Ricardo.

---
base-commit: 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
change-id: 20240305-bcachefs-27a4bb8b9f4f

Best regards,