Re: [PATCH 27/30] fbdev/core: Move procfs code to separate file

Hi Thomas,

On Mon, Jun 05, 2023 at 04:48:09PM +0200, Thomas Zimmermann wrote:
> Move fbdev's procfs code into a separate file and contain it in
> init and cleanup helpers. No functional changes.
Maybe add:
Delete the unused for_each_registered_fb while touching the code.

The change to use proc_remove is not really documented.
But code looks ok.

I am not fan that we have introduced a few globals again.
But it looks like the way to go for now.
> 
> Signed-off-by: Thomas Zimmermann <[email protected]>
With an improved changelog:
Reviewed-by: Sam Ravnborg <[email protected]>
> ---
>  drivers/video/fbdev/core/Makefile      |  1 +
>  drivers/video/fbdev/core/fb_internal.h | 12 ++++-
>  drivers/video/fbdev/core/fb_procfs.c   | 62 ++++++++++++++++++++++++++
>  drivers/video/fbdev/core/fbmem.c       | 51 +++------------------
>  4 files changed, 80 insertions(+), 46 deletions(-)
>  create mode 100644 drivers/video/fbdev/core/fb_procfs.c
> 
> diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
> index eee3295bc225..665320160f70 100644
> --- a/drivers/video/fbdev/core/Makefile
> +++ b/drivers/video/fbdev/core/Makefile
> @@ -3,6 +3,7 @@ obj-$(CONFIG_FB_NOTIFY)           += fb_notify.o
>  obj-$(CONFIG_FB)                  += fb.o
>  fb-y                              := fb_backlight.o \
>                                       fb_info.o \
> +                                     fb_procfs.o \
>                                       fbmem.o fbmon.o fbcmap.o fbsysfs.o \
>                                       modedb.o fbcvt.o fb_cmdline.o fb_io_fops.o
>  fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
> diff --git a/drivers/video/fbdev/core/fb_internal.h b/drivers/video/fbdev/core/fb_internal.h
> index 0b9640ae7a3d..51f7c9f04e45 100644
> --- a/drivers/video/fbdev/core/fb_internal.h
> +++ b/drivers/video/fbdev/core/fb_internal.h
> @@ -3,7 +3,17 @@
>  #ifndef _FB_INTERNAL_H
>  #define _FB_INTERNAL_H
>  
> -struct fb_info;
> +#include <linux/fb.h>
> +#include <linux/mutex.h>
> +
> +/* fbmem.c */
> +extern struct mutex registration_lock;
> +extern struct fb_info *registered_fb[FB_MAX];
> +extern int num_registered_fb;
> +
> +/* fb_procfs.c */
> +int fb_init_procfs(void);
> +void fb_cleanup_procfs(void);
>  
>  /* fbsysfs.c */
>  int fb_device_create(struct fb_info *fb_info);
> diff --git a/drivers/video/fbdev/core/fb_procfs.c b/drivers/video/fbdev/core/fb_procfs.c
> new file mode 100644
> index 000000000000..59641142f8aa
> --- /dev/null
> +++ b/drivers/video/fbdev/core/fb_procfs.c
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/proc_fs.h>
> +
> +#include "fb_internal.h"
> +
> +static struct proc_dir_entry *fb_proc_dir_entry;
> +
> +static void *fb_seq_start(struct seq_file *m, loff_t *pos)
> +{
> +	mutex_lock(&registration_lock);
> +
> +	return (*pos < FB_MAX) ? pos : NULL;
> +}
> +
> +static void fb_seq_stop(struct seq_file *m, void *v)
> +{
> +	mutex_unlock(&registration_lock);
> +}
> +
> +static void *fb_seq_next(struct seq_file *m, void *v, loff_t *pos)
> +{
> +	(*pos)++;
> +
> +	return (*pos < FB_MAX) ? pos : NULL;
> +}
> +
> +static int fb_seq_show(struct seq_file *m, void *v)
> +{
> +	int i = *(loff_t *)v;
> +	struct fb_info *fi = registered_fb[i];
> +
> +	if (fi)
> +		seq_printf(m, "%d %s\n", fi->node, fi->fix.id);
> +
> +	return 0;
> +}
> +
> +static const struct seq_operations __maybe_unused fb_proc_seq_ops = {
> +	.start	= fb_seq_start,
> +	.stop	= fb_seq_stop,
> +	.next	= fb_seq_next,
> +	.show	= fb_seq_show,
> +};
> +
> +int fb_init_procfs(void)
> +{
> +	struct proc_dir_entry *proc;
> +
> +	proc = proc_create_seq("fb", 0, NULL, &fb_proc_seq_ops);
> +	if (!proc)
> +		return -ENOMEM;
> +
> +	fb_proc_dir_entry = proc;
> +
> +	return 0;
> +}
> +
> +void fb_cleanup_procfs(void)
> +{
> +	proc_remove(fb_proc_dir_entry);
> +}
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 66532774d351..de1e45240161 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -24,9 +24,7 @@
>  #include <linux/vt.h>
>  #include <linux/init.h>
>  #include <linux/linux_logo.h>
> -#include <linux/proc_fs.h>
>  #include <linux/platform_device.h>
> -#include <linux/seq_file.h>
>  #include <linux/console.h>
>  #include <linux/kmod.h>
>  #include <linux/err.h>
> @@ -48,13 +46,9 @@
>  
>  #define FBPIXMAPSIZE	(1024 * 8)
>  
> -static DEFINE_MUTEX(registration_lock);
> -
> +DEFINE_MUTEX(registration_lock);
>  struct fb_info *registered_fb[FB_MAX] __read_mostly;
>  int num_registered_fb __read_mostly;
> -#define for_each_registered_fb(i)		\
> -	for (i = 0; i < FB_MAX; i++)		\
> -		if (!registered_fb[i]) {} else
>  
>  bool fb_center_logo __read_mostly;
>  
> @@ -705,40 +699,6 @@ int fb_show_logo(struct fb_info *info, int rotate) { return 0; }
>  EXPORT_SYMBOL(fb_prepare_logo);
>  EXPORT_SYMBOL(fb_show_logo);
>  
> -static void *fb_seq_start(struct seq_file *m, loff_t *pos)
> -{
> -	mutex_lock(&registration_lock);
> -	return (*pos < FB_MAX) ? pos : NULL;
> -}
> -
> -static void *fb_seq_next(struct seq_file *m, void *v, loff_t *pos)
> -{
> -	(*pos)++;
> -	return (*pos < FB_MAX) ? pos : NULL;
> -}
> -
> -static void fb_seq_stop(struct seq_file *m, void *v)
> -{
> -	mutex_unlock(&registration_lock);
> -}
> -
> -static int fb_seq_show(struct seq_file *m, void *v)
> -{
> -	int i = *(loff_t *)v;
> -	struct fb_info *fi = registered_fb[i];
> -
> -	if (fi)
> -		seq_printf(m, "%d %s\n", fi->node, fi->fix.id);
> -	return 0;
> -}
> -
> -static const struct seq_operations __maybe_unused proc_fb_seq_ops = {
> -	.start	= fb_seq_start,
> -	.next	= fb_seq_next,
> -	.stop	= fb_seq_stop,
> -	.show	= fb_seq_show,
> -};
> -
>  /*
>   * We hold a reference to the fb_info in file->private_data,
>   * but if the current registered fb has changed, we don't
> @@ -1624,8 +1584,9 @@ fbmem_init(void)
>  {
>  	int ret;
>  
> -	if (!proc_create_seq("fb", 0, NULL, &proc_fb_seq_ops))
> -		return -ENOMEM;
> +	ret = fb_init_procfs();
> +	if (ret)
> +		return ret;
>  
>  	ret = register_chrdev(FB_MAJOR, "fb", &fb_fops);
>  	if (ret) {
> @@ -1648,7 +1609,7 @@ fbmem_init(void)
>  err_class:
>  	unregister_chrdev(FB_MAJOR, "fb");
>  err_chrdev:
> -	remove_proc_entry("fb", NULL);
> +	fb_cleanup_procfs();
>  	return ret;
>  }
>  
> @@ -1659,7 +1620,7 @@ fbmem_exit(void)
>  {
>  	fb_console_exit();
>  
> -	remove_proc_entry("fb", NULL);
> +	fb_cleanup_procfs();
>  	class_destroy(fb_class);
>  	unregister_chrdev(FB_MAJOR, "fb");
>  }
> -- 
> 2.40.1