| Message ID | 20240305-class_cleanup-media-v1-1-53e1538973cd@marliere.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: constify struct class usage | expand |
On 05/03/2024 14:26, 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 dvb_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 <gregkh@linuxfoundation.org> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Signed-off-by: Ricardo B. Marliere <ricardo@marliere.net> > --- > drivers/media/dvb-core/dvbdev.c | 23 +++++++++++++---------- > 1 file changed, 13 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c > index 733d0bc4b4cc..dcbf68b00240 100644 > --- a/drivers/media/dvb-core/dvbdev.c > +++ b/drivers/media/dvb-core/dvbdev.c > @@ -78,7 +78,13 @@ static const u8 minor_type[] = { > #define MAX_DVB_MINORS (DVB_MAX_ADAPTERS * 64) > #endif > > -static struct class *dvb_class; > +static int dvb_uevent(const struct device *dev, struct kobj_uevent_env *env); > +static char *dvb_devnode(const struct device *dev, umode_t *mode); Forward references are typically something you want to avoid. Looking at the code, I think it makes sense to just move those two functions to just before this dvb_class. > +static const struct class dvb_class = { > + .name = "dvb", > + .dev_uevent = dvb_uevent, > + .devnode = dvb_devnode, > +}; > > static struct dvb_device *dvb_minors[MAX_DVB_MINORS]; > static DECLARE_RWSEM(minor_rwsem); Also move the dvb_class (+ the two functions) to after this line. I think that's a more suitable place for this. > @@ -561,7 +567,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, > return ret; > } > > - clsdev = device_create(dvb_class, adap->device, > + clsdev = device_create(&dvb_class, adap->device, > MKDEV(DVB_MAJOR, minor), > dvbdev, "dvb%d.%s%d", adap->num, dnames[type], id); > if (IS_ERR(clsdev)) { > @@ -600,7 +606,7 @@ void dvb_remove_device(struct dvb_device *dvbdev) > > dvb_media_device_free(dvbdev); > > - device_destroy(dvb_class, MKDEV(DVB_MAJOR, dvbdev->minor)); > + device_destroy(&dvb_class, MKDEV(DVB_MAJOR, dvbdev->minor)); > > list_del(&dvbdev->list_head); > } > @@ -1096,13 +1102,10 @@ static int __init init_dvbdev(void) > goto error; > } > > - dvb_class = class_create("dvb"); > - if (IS_ERR(dvb_class)) { > - retval = PTR_ERR(dvb_class); > + retval = class_register(&dvb_class); > + if (retval != 0) This can just be 'if (retval)'. > goto error; > - } > - dvb_class->dev_uevent = dvb_uevent; > - dvb_class->devnode = dvb_devnode; > + > return 0; > > error: > @@ -1115,7 +1118,7 @@ static void __exit exit_dvbdev(void) > { > struct dvbdevfops_node *node, *next; > > - class_destroy(dvb_class); > + class_unregister(&dvb_class); > cdev_del(&dvb_device_cdev); > unregister_chrdev_region(MKDEV(DVB_MAJOR, 0), MAX_DVB_MINORS); > > Regards, Hans
Hi Hans! On 8 Apr 11:47, Hans Verkuil wrote: > On 05/03/2024 14:26, 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 dvb_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 <gregkh@linuxfoundation.org> > > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Signed-off-by: Ricardo B. Marliere <ricardo@marliere.net> > > --- > > drivers/media/dvb-core/dvbdev.c | 23 +++++++++++++---------- > > 1 file changed, 13 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c > > index 733d0bc4b4cc..dcbf68b00240 100644 > > --- a/drivers/media/dvb-core/dvbdev.c > > +++ b/drivers/media/dvb-core/dvbdev.c > > @@ -78,7 +78,13 @@ static const u8 minor_type[] = { > > #define MAX_DVB_MINORS (DVB_MAX_ADAPTERS * 64) > > #endif > > > > -static struct class *dvb_class; > > +static int dvb_uevent(const struct device *dev, struct kobj_uevent_env *env); > > +static char *dvb_devnode(const struct device *dev, umode_t *mode); > > Forward references are typically something you want to avoid. > > Looking at the code, I think it makes sense to just move those two functions > to just before this dvb_class. Ack. > > > +static const struct class dvb_class = { > > + .name = "dvb", > > + .dev_uevent = dvb_uevent, > > + .devnode = dvb_devnode, > > +}; > > > > static struct dvb_device *dvb_minors[MAX_DVB_MINORS]; > > static DECLARE_RWSEM(minor_rwsem); > > Also move the dvb_class (+ the two functions) to after this line. I think that's > a more suitable place for this. Ack. > > > @@ -561,7 +567,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, > > return ret; > > } > > > > - clsdev = device_create(dvb_class, adap->device, > > + clsdev = device_create(&dvb_class, adap->device, > > MKDEV(DVB_MAJOR, minor), > > dvbdev, "dvb%d.%s%d", adap->num, dnames[type], id); > > if (IS_ERR(clsdev)) { > > @@ -600,7 +606,7 @@ void dvb_remove_device(struct dvb_device *dvbdev) > > > > dvb_media_device_free(dvbdev); > > > > - device_destroy(dvb_class, MKDEV(DVB_MAJOR, dvbdev->minor)); > > + device_destroy(&dvb_class, MKDEV(DVB_MAJOR, dvbdev->minor)); > > > > list_del(&dvbdev->list_head); > > } > > @@ -1096,13 +1102,10 @@ static int __init init_dvbdev(void) > > goto error; > > } > > > > - dvb_class = class_create("dvb"); > > - if (IS_ERR(dvb_class)) { > > - retval = PTR_ERR(dvb_class); > > + retval = class_register(&dvb_class); > > + if (retval != 0) > > This can just be 'if (retval)'. But then it would be on a different style than the rest of the function: static int __init init_dvbdev(void) { int retval; dev_t dev = MKDEV(DVB_MAJOR, 0); retval = register_chrdev_region(dev, MAX_DVB_MINORS, "DVB"); if (retval != 0) { pr_err("dvb-core: unable to get major %d\n", DVB_MAJOR); return retval; } cdev_init(&dvb_device_cdev, &dvb_device_fops); retval = cdev_add(&dvb_device_cdev, dev, MAX_DVB_MINORS); if (retval != 0) { pr_err("dvb-core: unable register character device\n"); goto error; } retval = class_register(&dvb_class); if (retval != 0) goto error; return 0; error: cdev_del(&dvb_device_cdev); unregister_chrdev_region(dev, MAX_DVB_MINORS); return retval; } > > > goto error; > > - } > > - dvb_class->dev_uevent = dvb_uevent; > > - dvb_class->devnode = dvb_devnode; > > + > > return 0; > > > > error: > > @@ -1115,7 +1118,7 @@ static void __exit exit_dvbdev(void) > > { > > struct dvbdevfops_node *node, *next; > > > > - class_destroy(dvb_class); > > + class_unregister(&dvb_class); > > cdev_del(&dvb_device_cdev); > > unregister_chrdev_region(MKDEV(DVB_MAJOR, 0), MAX_DVB_MINORS); > > > > > > Regards, > > Hans
On 08/04/2024 12:47, Ricardo B. Marliere wrote: > Hi Hans! > > On 8 Apr 11:47, Hans Verkuil wrote: >> On 05/03/2024 14:26, 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 dvb_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 <gregkh@linuxfoundation.org> >>> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>> Signed-off-by: Ricardo B. Marliere <ricardo@marliere.net> >>> --- >>> drivers/media/dvb-core/dvbdev.c | 23 +++++++++++++---------- >>> 1 file changed, 13 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c >>> index 733d0bc4b4cc..dcbf68b00240 100644 >>> --- a/drivers/media/dvb-core/dvbdev.c >>> +++ b/drivers/media/dvb-core/dvbdev.c >>> @@ -78,7 +78,13 @@ static const u8 minor_type[] = { >>> #define MAX_DVB_MINORS (DVB_MAX_ADAPTERS * 64) >>> #endif >>> >>> -static struct class *dvb_class; >>> +static int dvb_uevent(const struct device *dev, struct kobj_uevent_env *env); >>> +static char *dvb_devnode(const struct device *dev, umode_t *mode); >> >> Forward references are typically something you want to avoid. >> >> Looking at the code, I think it makes sense to just move those two functions >> to just before this dvb_class. > > Ack. > >> >>> +static const struct class dvb_class = { >>> + .name = "dvb", >>> + .dev_uevent = dvb_uevent, >>> + .devnode = dvb_devnode, >>> +}; >>> >>> static struct dvb_device *dvb_minors[MAX_DVB_MINORS]; >>> static DECLARE_RWSEM(minor_rwsem); >> >> Also move the dvb_class (+ the two functions) to after this line. I think that's >> a more suitable place for this. > > Ack. > >> >>> @@ -561,7 +567,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, >>> return ret; >>> } >>> >>> - clsdev = device_create(dvb_class, adap->device, >>> + clsdev = device_create(&dvb_class, adap->device, >>> MKDEV(DVB_MAJOR, minor), >>> dvbdev, "dvb%d.%s%d", adap->num, dnames[type], id); >>> if (IS_ERR(clsdev)) { >>> @@ -600,7 +606,7 @@ void dvb_remove_device(struct dvb_device *dvbdev) >>> >>> dvb_media_device_free(dvbdev); >>> >>> - device_destroy(dvb_class, MKDEV(DVB_MAJOR, dvbdev->minor)); >>> + device_destroy(&dvb_class, MKDEV(DVB_MAJOR, dvbdev->minor)); >>> >>> list_del(&dvbdev->list_head); >>> } >>> @@ -1096,13 +1102,10 @@ static int __init init_dvbdev(void) >>> goto error; >>> } >>> >>> - dvb_class = class_create("dvb"); >>> - if (IS_ERR(dvb_class)) { >>> - retval = PTR_ERR(dvb_class); >>> + retval = class_register(&dvb_class); >>> + if (retval != 0) >> >> This can just be 'if (retval)'. > > But then it would be on a different style than the rest of the function: Ah, I hadn't noticed that. Let's keep it as you proposed then. Regards, Hans > > static int __init init_dvbdev(void) > { > int retval; > dev_t dev = MKDEV(DVB_MAJOR, 0); > > retval = register_chrdev_region(dev, MAX_DVB_MINORS, "DVB"); > if (retval != 0) { > pr_err("dvb-core: unable to get major %d\n", DVB_MAJOR); > return retval; > } > > cdev_init(&dvb_device_cdev, &dvb_device_fops); > retval = cdev_add(&dvb_device_cdev, dev, MAX_DVB_MINORS); > if (retval != 0) { > pr_err("dvb-core: unable register character device\n"); > goto error; > } > > retval = class_register(&dvb_class); > if (retval != 0) > goto error; > > return 0; > > error: > cdev_del(&dvb_device_cdev); > unregister_chrdev_region(dev, MAX_DVB_MINORS); > return retval; > } > > >> >>> goto error; >>> - } >>> - dvb_class->dev_uevent = dvb_uevent; >>> - dvb_class->devnode = dvb_devnode; >>> + >>> return 0; >>> >>> error: >>> @@ -1115,7 +1118,7 @@ static void __exit exit_dvbdev(void) >>> { >>> struct dvbdevfops_node *node, *next; >>> >>> - class_destroy(dvb_class); >>> + class_unregister(&dvb_class); >>> cdev_del(&dvb_device_cdev); >>> unregister_chrdev_region(MKDEV(DVB_MAJOR, 0), MAX_DVB_MINORS); >>> >>> >> >> Regards, >> >> Hans
diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c index 733d0bc4b4cc..dcbf68b00240 100644 --- a/drivers/media/dvb-core/dvbdev.c +++ b/drivers/media/dvb-core/dvbdev.c @@ -78,7 +78,13 @@ static const u8 minor_type[] = { #define MAX_DVB_MINORS (DVB_MAX_ADAPTERS * 64) #endif -static struct class *dvb_class; +static int dvb_uevent(const struct device *dev, struct kobj_uevent_env *env); +static char *dvb_devnode(const struct device *dev, umode_t *mode); +static const struct class dvb_class = { + .name = "dvb", + .dev_uevent = dvb_uevent, + .devnode = dvb_devnode, +}; static struct dvb_device *dvb_minors[MAX_DVB_MINORS]; static DECLARE_RWSEM(minor_rwsem); @@ -561,7 +567,7 @@ int dvb_register_device(struct dvb_adapter *adap, struct dvb_device **pdvbdev, return ret; } - clsdev = device_create(dvb_class, adap->device, + clsdev = device_create(&dvb_class, adap->device, MKDEV(DVB_MAJOR, minor), dvbdev, "dvb%d.%s%d", adap->num, dnames[type], id); if (IS_ERR(clsdev)) { @@ -600,7 +606,7 @@ void dvb_remove_device(struct dvb_device *dvbdev) dvb_media_device_free(dvbdev); - device_destroy(dvb_class, MKDEV(DVB_MAJOR, dvbdev->minor)); + device_destroy(&dvb_class, MKDEV(DVB_MAJOR, dvbdev->minor)); list_del(&dvbdev->list_head); } @@ -1096,13 +1102,10 @@ static int __init init_dvbdev(void) goto error; } - dvb_class = class_create("dvb"); - if (IS_ERR(dvb_class)) { - retval = PTR_ERR(dvb_class); + retval = class_register(&dvb_class); + if (retval != 0) goto error; - } - dvb_class->dev_uevent = dvb_uevent; - dvb_class->devnode = dvb_devnode; + return 0; error: @@ -1115,7 +1118,7 @@ static void __exit exit_dvbdev(void) { struct dvbdevfops_node *node, *next; - class_destroy(dvb_class); + class_unregister(&dvb_class); cdev_del(&dvb_device_cdev); unregister_chrdev_region(MKDEV(DVB_MAJOR, 0), MAX_DVB_MINORS);
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 dvb_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 <gregkh@linuxfoundation.org> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Signed-off-by: Ricardo B. Marliere <ricardo@marliere.net> --- drivers/media/dvb-core/dvbdev.c | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-)