[PATCH v6 5/5] fbdev: Make registered_fb[] private to fbmem.c

[PATCH v6 5/5] fbdev: Make registered_fb[] private to fbmem.c

[Javier Martinez Canillas]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

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 <lkp@intel.com>
Cc: Jens Frederich <jfrederich@gmail.com>
Cc: Jon Nettleton <jon.nettleton@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-staging@lists.linux.dev
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Helge Deller <deller@gmx.de>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: Zhen Lei <thunder.leizhen@huawei.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Xiyu Yang <xiyuyang19@fudan.edu.cn>
Cc: linux-fbdev@vger.kernel.org
Cc: Zheyu Ma <zheyuma97@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

(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 e0720fef0ee6..bdb08b665b43 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(&info->lock);
-- 
2.36.1

Re: [PATCH v6 5/5] fbdev: Make registered_fb[] private to fbmem.c

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4447 bytes --]

Hi Javier

Am 07.06.22 um 20:23 schrieb Javier Martinez Canillas:
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Well except when the olpc dcon fbdev driver is enabled, that thing
> digs around in there in rather unfixable ways.

There is fb_client_register() to set up a 'client' on top of an fbdev. 
The client would then get messages about modesetting, blanks, removals, 
etc. But you'd probably need an OLPC to convert dcon, and the mechanism 
itself is somewhat unloved these days.

Your patch complicates the fbdev code AFAICT. So I'd either drop it or, 
even better, build a nicer interface for dcon.

The dcon driver appears to look only at the first entry. Maybe add 
fb_info_get_by_index() and fb_info_put() and export those. They would be 
trivial wrappers somewhere in fbmem.c:

#if IS_ENABLED(CONFIG_FB_OLPC_DCON)
struct fb_info *fb_info_get_by_index(unsigned int index)
{
	return get_fb_info(index);
}
EXPORT_SYMBOL()
void fb_info_put(struct fb_info *fb_info)
{
	put_fb_info(fb_info);
}
EXPORT_SYMBOL()
#endif

In dcon itself, using the new interfaces will actually acquire a 
reference to keep the display alive. The code at [1] could be replaced. 
And a call to fb_info_put() needs to go into dcon_remove(). [2]

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v5.18.2/source/drivers/staging/olpc_dcon/olpc_dcon.c#L605
[2] 
https://elixir.bootlin.com/linux/v5.18.2/source/drivers/staging/olpc_dcon/olpc_dcon.c#L688

> 
> Cc oldc_dcon maintainers as fyi.
> 
> v2: I typoed the config name (0day)
> 
> Cc: kernel test robot <lkp@intel.com>
> Cc: Jens Frederich <jfrederich@gmail.com>
> Cc: Jon Nettleton <jon.nettleton@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-staging@lists.linux.dev
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Cc: Zhen Lei <thunder.leizhen@huawei.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Xiyu Yang <xiyuyang19@fudan.edu.cn>
> Cc: linux-fbdev@vger.kernel.org
> Cc: Zheyu Ma <zheyuma97@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> 
> (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 e0720fef0ee6..bdb08b665b43 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(&info->lock);

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v6 5/5] fbdev: Make registered_fb[] private to fbmem.c

[Javier Martinez Canillas]

Hello Thomas,

On 6/9/22 13:49, Thomas Zimmermann wrote:
> Hi Javier
> 
> Am 07.06.22 um 20:23 schrieb Javier Martinez Canillas:
>> From: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Well except when the olpc dcon fbdev driver is enabled, that thing
>> digs around in there in rather unfixable ways.
> 
> There is fb_client_register() to set up a 'client' on top of an fbdev. 
> The client would then get messages about modesetting, blanks, removals, 
> etc. But you'd probably need an OLPC to convert dcon, and the mechanism 
> itself is somewhat unloved these days.
> 
> Your patch complicates the fbdev code AFAICT. So I'd either drop it or, 
> even better, build a nicer interface for dcon.
> 
> The dcon driver appears to look only at the first entry. Maybe add 
> fb_info_get_by_index() and fb_info_put() and export those. They would be 
> trivial wrappers somewhere in fbmem.c:
> 
> #if IS_ENABLED(CONFIG_FB_OLPC_DCON)
> struct fb_info *fb_info_get_by_index(unsigned int index)
> {
> 	return get_fb_info(index);
> }
> EXPORT_SYMBOL()
> void fb_info_put(struct fb_info *fb_info)
> {
> 	put_fb_info(fb_info);
> }
> EXPORT_SYMBOL()
> #endif
> 
> In dcon itself, using the new interfaces will actually acquire a 
> reference to keep the display alive. The code at [1] could be replaced. 
> And a call to fb_info_put() needs to go into dcon_remove(). [2]
> 

Thanks for your suggestions, that makes sense to me. I'll drop this
patch from the set and post as a follow-up a different approach as
you suggested.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Mark olpc_dcon BROKEN [Was: [PATCH v6 5/5] fbdev: Make registered_fb[] private to fbmem.c]

[Sam Ravnborg]

Hi Javier.

On Thu, Jun 09, 2022 at 03:09:21PM +0200, Javier Martinez Canillas wrote:
> Hello Thomas,
> 
> On 6/9/22 13:49, Thomas Zimmermann wrote:
> > Hi Javier
> > 
> > Am 07.06.22 um 20:23 schrieb Javier Martinez Canillas:
> >> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>
> >> Well except when the olpc dcon fbdev driver is enabled, that thing
> >> digs around in there in rather unfixable ways.
> > 
> > There is fb_client_register() to set up a 'client' on top of an fbdev. 
> > The client would then get messages about modesetting, blanks, removals, 
> > etc. But you'd probably need an OLPC to convert dcon, and the mechanism 
> > itself is somewhat unloved these days.
> > 
> > Your patch complicates the fbdev code AFAICT. So I'd either drop it or, 
> > even better, build a nicer interface for dcon.
> > 
> > The dcon driver appears to look only at the first entry. Maybe add 
> > fb_info_get_by_index() and fb_info_put() and export those. They would be 
> > trivial wrappers somewhere in fbmem.c:
> > 
> > #if IS_ENABLED(CONFIG_FB_OLPC_DCON)
> > struct fb_info *fb_info_get_by_index(unsigned int index)
> > {
> > 	return get_fb_info(index);
> > }
> > EXPORT_SYMBOL()
> > void fb_info_put(struct fb_info *fb_info)
> > {
> > 	put_fb_info(fb_info);
> > }
> > EXPORT_SYMBOL()
> > #endif
> > 
> > In dcon itself, using the new interfaces will actually acquire a 
> > reference to keep the display alive. The code at [1] could be replaced. 
> > And a call to fb_info_put() needs to go into dcon_remove(). [2]
> > 
> 
> Thanks for your suggestions, that makes sense to me. I'll drop this
> patch from the set and post as a follow-up a different approach as
> you suggested.

To repeat myself from irc.
olpc_dcon is a staging driver and we should avoid inventing anything in
core code for to make staging drivers works.
Geert suggested EXPORT_SYMPBOL_NS_GPL() that could work and narrow it
down to olpc_dcon.
The better approach is to mark said driver BROKEN and then someone can
fix it it there is anyone who cares.
Last commit to olpc_dcon was in 2019: e40219d5e4b2177bfd4d885e7b64e3b236af40ac
and maybe Jerry Lin cares enough to fix it.

Added Jerry and Greg to the mail.

	Sam

Re: Mark olpc_dcon BROKEN [Was: [PATCH v6 5/5] fbdev: Make registered_fb[] private to fbmem.c]

[Javier Martinez Canillas]

Hello Sam,

On 6/9/22 19:23, Sam Ravnborg wrote:

[snip]

> 
> To repeat myself from irc.
> olpc_dcon is a staging driver and we should avoid inventing anything in
> core code for to make staging drivers works.
> Geert suggested EXPORT_SYMPBOL_NS_GPL() that could work and narrow it
> down to olpc_dcon.
> The better approach is to mark said driver BROKEN and then someone can
> fix it it there is anyone who cares.
> Last commit to olpc_dcon was in 2019: e40219d5e4b2177bfd4d885e7b64e3b236af40ac
> and maybe Jerry Lin cares enough to fix it.
> 
> Added Jerry and Greg to the mail.
> 
> 	Sam
> 

That does sound like the best approach indeed. And if the driver is kept
BROKEN for a few releases then it can just remove it from the kernel. If
someone still uses/cares about the driver, they can fix it as you said,
and it could even be ported to DRM if is something that's still useful.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat