Re: [PATCH v5 7/7] fbdev: Make registered_fb[] private to fbmem.c
On Wed, May 11, 2022 at 07:34:38PM +0200, Javier Martinez Canillas wrote: > Hello Guenter, > > On 5/11/22 19:17, Guenter Roeck wrote: > > On 5/11/22 10:00, Sam Ravnborg wrote: > > [snip] > > >>> struct fb_info *registered_fb[FB_MAX] __read_mostly; > >>> -EXPORT_SYMBOL(registered_fb); > >>> - > >>> int num_registered_fb __read_mostly; > >>> +#if IS_ENABLED(CONFIG_FB_OLPC_DCON) > >>> +EXPORT_SYMBOL(registered_fb); > >>> EXPORT_SYMBOL(num_registered_fb); > >>> +#endif > >> > >> It is stuff like this I refer to as "ugly" in the comment above. > >> > > > > My "solution" for that kind of thing is to use a namespace, > > such as > > > > EXPORT_SYMBOL_NS(registered_fb, FB_OLPC_DCON); > > EXPORT_SYMBOL_NS(num_registered_fb, FB_OLPC_DCON); > > > > Using a namespace in this case is indeed a great idea I think. > > I've used in the past to limit the export of a symbol for within a driver > that could be scattered across different compilations units, but it never > occurred to me using it to limit symbols exported by core code. > > > and import it from the offending code. That avoids ifdefs > > while at the same time limiting the use of the symbols > > to the expected scope. Of course that could be abused but > > that abuse would be obvious. > > > > Agreed. For the next revision, besides using an namespaced export symbol > as you suggested, I'll include a comment to make clear that it shouldn't > by any other driver and FB_OLPC_DCON fixed instead. A very nice compromise, thanks Guenter and Javier. Sam
Re: [PATCH v5 7/7] fbdev: Make registered_fb[] private to fbmem.c
Hello Guenter, On 5/11/22 19:17, Guenter Roeck wrote: > On 5/11/22 10:00, Sam Ravnborg wrote: [snip] >>> struct fb_info *registered_fb[FB_MAX] __read_mostly; >>> -EXPORT_SYMBOL(registered_fb); >>> - >>> int num_registered_fb __read_mostly; >>> +#if IS_ENABLED(CONFIG_FB_OLPC_DCON) >>> +EXPORT_SYMBOL(registered_fb); >>> EXPORT_SYMBOL(num_registered_fb); >>> +#endif >> >> It is stuff like this I refer to as "ugly" in the comment above. >> > > My "solution" for that kind of thing is to use a namespace, > such as > > EXPORT_SYMBOL_NS(registered_fb, FB_OLPC_DCON); > EXPORT_SYMBOL_NS(num_registered_fb, FB_OLPC_DCON); > Using a namespace in this case is indeed a great idea I think. I've used in the past to limit the export of a symbol for within a driver that could be scattered across different compilations units, but it never occurred to me using it to limit symbols exported by core code. > and import it from the offending code. That avoids ifdefs > while at the same time limiting the use of the symbols > to the expected scope. Of course that could be abused but > that abuse would be obvious. > Agreed. For the next revision, besides using an namespaced export symbol as you suggested, I'll include a comment to make clear that it shouldn't by any other driver and FB_OLPC_DCON fixed instead. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v5 7/7] fbdev: Make registered_fb[] private to fbmem.c
On 5/11/22 10:00, Sam Ravnborg wrote: Hi Javier. On Wed, May 11, 2022 at 01:32:30PM +0200, Javier Martinez Canillas wrote: From: Daniel Vetter Well except when the olpc dcon fbdev driver is enabled, that thing digs around in there in rather unfixable ways. Cc oldc_dcon maintainers as fyi. Another way to fix this is to mark FB_OLPC_DCON and add a TODO entry to fix this. We are really not supposed to carry such special code around to keep staging working. I know this may not be a popular viewpoint, but just look at the ugly workarounds required here. Sam v2: I typoed the config name (0day) Cc: kernel test robot Cc: Jens Frederich Cc: Jon Nettleton Cc: Greg Kroah-Hartman Cc: linux-stag...@lists.linux.dev Signed-off-by: Daniel Vetter Signed-off-by: Daniel Vetter Reviewed-by: Javier Martinez Canillas Cc: Daniel Vetter Cc: Helge Deller Cc: Matthew Wilcox Cc: Sam Ravnborg Cc: Tetsuo Handa Cc: Zhen Lei Cc: Alex Deucher Cc: Xiyu Yang Cc: linux-fb...@vger.kernel.org Cc: Zheyu Ma Cc: Guenter Roeck Signed-off-by: Javier Martinez Canillas --- (no changes since v1) drivers/video/fbdev/core/fbmem.c | 8 ++-- include/linux/fb.h | 7 +++ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 265efa189bcc..6cab5f4c1fb3 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -50,10 +50,14 @@ static DEFINE_MUTEX(registration_lock); struct fb_info *registered_fb[FB_MAX] __read_mostly; -EXPORT_SYMBOL(registered_fb); - int num_registered_fb __read_mostly; +#if IS_ENABLED(CONFIG_FB_OLPC_DCON) +EXPORT_SYMBOL(registered_fb); EXPORT_SYMBOL(num_registered_fb); +#endif It is stuff like this I refer to as "ugly" in the comment above. My "solution" for that kind of thing is to use a namespace, such as EXPORT_SYMBOL_NS(registered_fb, FB_OLPC_DCON); EXPORT_SYMBOL_NS(num_registered_fb, FB_OLPC_DCON); and import it from the offending code. That avoids ifdefs while at the same time limiting the use of the symbols to the expected scope. Of course that could be abused but that abuse would be obvious. Guenter
Re: [PATCH v5 7/7] fbdev: Make registered_fb[] private to fbmem.c
Hi Javier. On Wed, May 11, 2022 at 01:32:30PM +0200, Javier Martinez Canillas wrote: > From: Daniel Vetter > > Well except when the olpc dcon fbdev driver is enabled, that thing > digs around in there in rather unfixable ways. > > Cc oldc_dcon maintainers as fyi. Another way to fix this is to mark FB_OLPC_DCON and add a TODO entry to fix this. We are really not supposed to carry such special code around to keep staging working. I know this may not be a popular viewpoint, but just look at the ugly workarounds required here. Sam > > v2: I typoed the config name (0day) > > Cc: kernel test robot > Cc: Jens Frederich > Cc: Jon Nettleton > Cc: Greg Kroah-Hartman > Cc: linux-stag...@lists.linux.dev > Signed-off-by: Daniel Vetter > Signed-off-by: Daniel Vetter > Reviewed-by: Javier Martinez Canillas > Cc: Daniel Vetter > Cc: Helge Deller > Cc: Matthew Wilcox > Cc: Sam Ravnborg > Cc: Tetsuo Handa > Cc: Zhen Lei > Cc: Alex Deucher > Cc: Xiyu Yang > Cc: linux-fb...@vger.kernel.org > Cc: Zheyu Ma > Cc: Guenter Roeck > Signed-off-by: Javier Martinez Canillas > --- > > (no changes since v1) > > drivers/video/fbdev/core/fbmem.c | 8 ++-- > include/linux/fb.h | 7 +++ > 2 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/video/fbdev/core/fbmem.c > b/drivers/video/fbdev/core/fbmem.c > index 265efa189bcc..6cab5f4c1fb3 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -50,10 +50,14 @@ > static DEFINE_MUTEX(registration_lock); > > struct fb_info *registered_fb[FB_MAX] __read_mostly; > -EXPORT_SYMBOL(registered_fb); > - > int num_registered_fb __read_mostly; > +#if IS_ENABLED(CONFIG_FB_OLPC_DCON) > +EXPORT_SYMBOL(registered_fb); > EXPORT_SYMBOL(num_registered_fb); > +#endif It is stuff like this I refer to as "ugly" in the comment above. Sam
[PATCH v5 7/7] fbdev: Make registered_fb[] private to fbmem.c
From: Daniel Vetter
Well except when the olpc dcon fbdev driver is enabled, that thing
digs around in there in rather unfixable ways.
Cc oldc_dcon maintainers as fyi.
v2: I typoed the config name (0day)
Cc: kernel test robot
Cc: Jens Frederich
Cc: Jon Nettleton
Cc: Greg Kroah-Hartman
Cc: linux-stag...@lists.linux.dev
Signed-off-by: Daniel Vetter
Signed-off-by: Daniel Vetter
Reviewed-by: Javier Martinez Canillas
Cc: Daniel Vetter
Cc: Helge Deller
Cc: Matthew Wilcox
Cc: Sam Ravnborg
Cc: Tetsuo Handa
Cc: Zhen Lei
Cc: Alex Deucher
Cc: Xiyu Yang
Cc: linux-fb...@vger.kernel.org
Cc: Zheyu Ma
Cc: Guenter Roeck
Signed-off-by: Javier Martinez Canillas
---
(no changes since v1)
drivers/video/fbdev/core/fbmem.c | 8 ++--
include/linux/fb.h | 7 +++
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 265efa189bcc..6cab5f4c1fb3 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -50,10 +50,14 @@
static DEFINE_MUTEX(registration_lock);
struct fb_info *registered_fb[FB_MAX] __read_mostly;
-EXPORT_SYMBOL(registered_fb);
-
int num_registered_fb __read_mostly;
+#if IS_ENABLED(CONFIG_FB_OLPC_DCON)
+EXPORT_SYMBOL(registered_fb);
EXPORT_SYMBOL(num_registered_fb);
+#endif
+#define for_each_registered_fb(i) \
+ for (i = 0; i < FB_MAX; i++)\
+ if (!registered_fb[i]) {} else
bool fb_center_logo __read_mostly;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index bbe1e4571899..c563e24b6293 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -632,16 +632,15 @@ extern int fb_get_color_depth(struct fb_var_screeninfo
*var,
extern int fb_get_options(const char *name, char **option);
extern int fb_new_modelist(struct fb_info *info);
+#if IS_ENABLED(CONFIG_FB_OLPC_DCON)
extern struct fb_info *registered_fb[FB_MAX];
+
extern int num_registered_fb;
+#endif
extern bool fb_center_logo;
extern int fb_logo_count;
extern struct class *fb_class;
-#define for_each_registered_fb(i) \
- for (i = 0; i < FB_MAX; i++)\
- if (!registered_fb[i]) {} else
-
static inline void lock_fb_info(struct fb_info *info)
{
mutex_lock(>lock);
--
2.35.1
