[PATCH 0/5] fbdev: devm_register_framebuffer() and some fixes for efifb

[PATCH 0/5] fbdev: devm_register_framebuffer() and some fixes for efifb

[Thomas Weißschuh]

Some random optimization and fixes I came up with while looking at
efifb.c.

I can't get the efifb driver to probe on my hardware,
so this is only compile-tested.
If somebody could actually test it, that would be great.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (5):
      fbdev/efifb: Use stack memory for screeninfo structs
      fbdev/efifb: Register sysfs groups through driver core
      fbdev: Introduce devm_register_framebuffer()
      fbdev/efifb: Use devm_register_framebuffer()
      fbdev/efifb: Use driver-private screen_info for sysfs

 drivers/video/fbdev/core/fbmem.c | 24 +++++++++++++++
 drivers/video/fbdev/efifb.c      | 63 +++++++++++++++-------------------------
 include/linux/fb.h               |  1 +
 3 files changed, 49 insertions(+), 39 deletions(-)
---
base-commit: 3e9bff3bbe1355805de919f688bef4baefbfd436
change-id: 20240827-efifb-sysfs-ea08d70c8c46

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/5] fbdev/efifb: Use stack memory for screeninfo structs

[Thomas Weißschuh]

These variables are only used inside efifb_probe().
Afterwards they are using memory unnecessarily.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/video/fbdev/efifb.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 8dd82afb3452..8bfe0ccbc67a 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -52,24 +52,6 @@ struct efifb_par {
 	resource_size_t size;
 };
 
-static struct fb_var_screeninfo efifb_defined = {
-	.activate		= FB_ACTIVATE_NOW,
-	.height			= -1,
-	.width			= -1,
-	.right_margin		= 32,
-	.upper_margin		= 16,
-	.lower_margin		= 4,
-	.vsync_len		= 4,
-	.vmode			= FB_VMODE_NONINTERLACED,
-};
-
-static struct fb_fix_screeninfo efifb_fix = {
-	.id			= "EFI VGA",
-	.type			= FB_TYPE_PACKED_PIXELS,
-	.accel			= FB_ACCEL_NONE,
-	.visual			= FB_VISUAL_TRUECOLOR,
-};
-
 static int efifb_setcolreg(unsigned regno, unsigned red, unsigned green,
 			   unsigned blue, unsigned transp,
 			   struct fb_info *info)
@@ -357,6 +339,24 @@ static int efifb_probe(struct platform_device *dev)
 	char *option = NULL;
 	efi_memory_desc_t md;
 
+	struct fb_var_screeninfo efifb_defined = {
+		.activate		= FB_ACTIVATE_NOW,
+		.height			= -1,
+		.width			= -1,
+		.right_margin		= 32,
+		.upper_margin		= 16,
+		.lower_margin		= 4,
+		.vsync_len		= 4,
+		.vmode			= FB_VMODE_NONINTERLACED,
+	};
+
+	struct fb_fix_screeninfo efifb_fix = {
+		.id			= "EFI VGA",
+		.type			= FB_TYPE_PACKED_PIXELS,
+		.accel			= FB_ACCEL_NONE,
+		.visual			= FB_VISUAL_TRUECOLOR,
+	};
+
 	/*
 	 * If we fail probing the device, the kernel might try a different
 	 * driver. We get a copy of the attached screen_info, so that we can

-- 
2.46.0

Re: [PATCH 1/5] fbdev/efifb: Use stack memory for screeninfo structs

[Helge Deller]

On 8/27/24 17:25, Thomas Weißschuh wrote:
> These variables are only used inside efifb_probe().
> Afterwards they are using memory unnecessarily.

Did you check if this change really saves some memory?
With your change, the compiler will either create a hidden
structure which it uses then, or it generates assembly
instructions to fill the struct at runtime.
Both options may not actually reduce the memory footprint...

Another option might be to mark the static struct __initdata
if you expect another card to take over before the memory is
freed at runtime. But I'm not sure if it's worth possible
implications.

Helge

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>   drivers/video/fbdev/efifb.c | 36 ++++++++++++++++++------------------
>   1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 8dd82afb3452..8bfe0ccbc67a 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -52,24 +52,6 @@ struct efifb_par {
>   	resource_size_t size;
>   };
>
> -static struct fb_var_screeninfo efifb_defined = {
> -	.activate		= FB_ACTIVATE_NOW,
> -	.height			= -1,
> -	.width			= -1,
> -	.right_margin		= 32,
> -	.upper_margin		= 16,
> -	.lower_margin		= 4,
> -	.vsync_len		= 4,
> -	.vmode			= FB_VMODE_NONINTERLACED,
> -};
> -
> -static struct fb_fix_screeninfo efifb_fix = {
> -	.id			= "EFI VGA",
> -	.type			= FB_TYPE_PACKED_PIXELS,
> -	.accel			= FB_ACCEL_NONE,
> -	.visual			= FB_VISUAL_TRUECOLOR,
> -};
> -
>   static int efifb_setcolreg(unsigned regno, unsigned red, unsigned green,
>   			   unsigned blue, unsigned transp,
>   			   struct fb_info *info)
> @@ -357,6 +339,24 @@ static int efifb_probe(struct platform_device *dev)
>   	char *option = NULL;
>   	efi_memory_desc_t md;
>
> +	struct fb_var_screeninfo efifb_defined = {
> +		.activate		= FB_ACTIVATE_NOW,
> +		.height			= -1,
> +		.width			= -1,
> +		.right_margin		= 32,
> +		.upper_margin		= 16,
> +		.lower_margin		= 4,
> +		.vsync_len		= 4,
> +		.vmode			= FB_VMODE_NONINTERLACED,
> +	};
> +
> +	struct fb_fix_screeninfo efifb_fix = {
> +		.id			= "EFI VGA",
> +		.type			= FB_TYPE_PACKED_PIXELS,
> +		.accel			= FB_ACCEL_NONE,
> +		.visual			= FB_VISUAL_TRUECOLOR,
> +	};
> +
>   	/*
>   	 * If we fail probing the device, the kernel might try a different
>   	 * driver. We get a copy of the attached screen_info, so that we can
>

Re: [PATCH 1/5] fbdev/efifb: Use stack memory for screeninfo structs

[Thomas Weißschuh]

On 2024-08-28 19:42:51+0000, Helge Deller wrote:
> On 8/27/24 17:25, Thomas Weißschuh wrote:
> > These variables are only used inside efifb_probe().
> > Afterwards they are using memory unnecessarily.
> 
> Did you check if this change really saves some memory?

Nope...

> With your change, the compiler will either create a hidden
> structure which it uses then, or it generates assembly
> instructions to fill the struct at runtime.
> Both options may not actually reduce the memory footprint...

Thanks for the explanation, it makes sense.

On advantage of the on-stack data would be future-proofing.
Efi efifb_probe() overrides some fields in these structs only in certain
codepaths then the globally shared data could become inconsistent.
But that's not the case today.

> Another option might be to mark the static struct __initdata
> if you expect another card to take over before the memory is
> freed at runtime. But I'm not sure if it's worth possible
> implications.

Agreed.

> Helge
> 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >   drivers/video/fbdev/efifb.c | 36 ++++++++++++++++++------------------
> >   1 file changed, 18 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> > index 8dd82afb3452..8bfe0ccbc67a 100644
> > --- a/drivers/video/fbdev/efifb.c
> > +++ b/drivers/video/fbdev/efifb.c
> > @@ -52,24 +52,6 @@ struct efifb_par {
> >   	resource_size_t size;
> >   };
> > 
> > -static struct fb_var_screeninfo efifb_defined = {
> > -	.activate		= FB_ACTIVATE_NOW,
> > -	.height			= -1,
> > -	.width			= -1,
> > -	.right_margin		= 32,
> > -	.upper_margin		= 16,
> > -	.lower_margin		= 4,
> > -	.vsync_len		= 4,
> > -	.vmode			= FB_VMODE_NONINTERLACED,
> > -};
> > -
> > -static struct fb_fix_screeninfo efifb_fix = {
> > -	.id			= "EFI VGA",
> > -	.type			= FB_TYPE_PACKED_PIXELS,
> > -	.accel			= FB_ACCEL_NONE,
> > -	.visual			= FB_VISUAL_TRUECOLOR,
> > -};
> > -
> >   static int efifb_setcolreg(unsigned regno, unsigned red, unsigned green,
> >   			   unsigned blue, unsigned transp,
> >   			   struct fb_info *info)
> > @@ -357,6 +339,24 @@ static int efifb_probe(struct platform_device *dev)
> >   	char *option = NULL;
> >   	efi_memory_desc_t md;
> > 
> > +	struct fb_var_screeninfo efifb_defined = {
> > +		.activate		= FB_ACTIVATE_NOW,
> > +		.height			= -1,
> > +		.width			= -1,
> > +		.right_margin		= 32,
> > +		.upper_margin		= 16,
> > +		.lower_margin		= 4,
> > +		.vsync_len		= 4,
> > +		.vmode			= FB_VMODE_NONINTERLACED,
> > +	};
> > +
> > +	struct fb_fix_screeninfo efifb_fix = {
> > +		.id			= "EFI VGA",
> > +		.type			= FB_TYPE_PACKED_PIXELS,
> > +		.accel			= FB_ACCEL_NONE,
> > +		.visual			= FB_VISUAL_TRUECOLOR,
> > +	};
> > +
> >   	/*
> >   	 * If we fail probing the device, the kernel might try a different
> >   	 * driver. We get a copy of the attached screen_info, so that we can
> > 
> 

[PATCH 2/5] fbdev/efifb: Register sysfs groups through driver core

[Thomas Weißschuh]

The driver core can register and cleanup sysfs groups already.
Make use of that functionality to simplify the error handling and
cleanup.

Also avoid a UAF race during unregistering where the sysctl attributes
were usable after the info struct was freed.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/video/fbdev/efifb.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 8bfe0ccbc67a..d36b95856dd0 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -561,15 +561,10 @@ static int efifb_probe(struct platform_device *dev)
 		break;
 	}
 
-	err = sysfs_create_groups(&dev->dev.kobj, efifb_groups);
-	if (err) {
-		pr_err("efifb: cannot add sysfs attrs\n");
-		goto err_unmap;
-	}
 	err = fb_alloc_cmap(&info->cmap, 256, 0);
 	if (err < 0) {
 		pr_err("efifb: cannot allocate colormap\n");
-		goto err_groups;
+		goto err_unmap;
 	}
 
 	err = devm_aperture_acquire_for_platform_device(dev, par->base, par->size);
@@ -587,8 +582,6 @@ static int efifb_probe(struct platform_device *dev)
 
 err_fb_dealloc_cmap:
 	fb_dealloc_cmap(&info->cmap);
-err_groups:
-	sysfs_remove_groups(&dev->dev.kobj, efifb_groups);
 err_unmap:
 	if (mem_flags & (EFI_MEMORY_UC | EFI_MEMORY_WC))
 		iounmap(info->screen_base);
@@ -608,12 +601,12 @@ static void efifb_remove(struct platform_device *pdev)
 
 	/* efifb_destroy takes care of info cleanup */
 	unregister_framebuffer(info);
-	sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
 }
 
 static struct platform_driver efifb_driver = {
 	.driver = {
 		.name = "efi-framebuffer",
+		.dev_groups = efifb_groups,
 	},
 	.probe = efifb_probe,
 	.remove_new = efifb_remove,

-- 
2.46.0

Re: [PATCH 2/5] fbdev/efifb: Register sysfs groups through driver core

[Helge Deller]

On 8/27/24 17:25, Thomas Weißschuh wrote:
> The driver core can register and cleanup sysfs groups already.
> Make use of that functionality to simplify the error handling and
> cleanup.
>
> Also avoid a UAF race during unregistering where the sysctl attributes
> were usable after the info struct was freed.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

I've added your patches #2, #3, #4 and #5 of this series to the fbdev git tree.
I tend to not take patch #1 as already explained there...

Thanks!
Helge



> ---
>   drivers/video/fbdev/efifb.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index 8bfe0ccbc67a..d36b95856dd0 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -561,15 +561,10 @@ static int efifb_probe(struct platform_device *dev)
>   		break;
>   	}
>
> -	err = sysfs_create_groups(&dev->dev.kobj, efifb_groups);
> -	if (err) {
> -		pr_err("efifb: cannot add sysfs attrs\n");
> -		goto err_unmap;
> -	}
>   	err = fb_alloc_cmap(&info->cmap, 256, 0);
>   	if (err < 0) {
>   		pr_err("efifb: cannot allocate colormap\n");
> -		goto err_groups;
> +		goto err_unmap;
>   	}
>
>   	err = devm_aperture_acquire_for_platform_device(dev, par->base, par->size);
> @@ -587,8 +582,6 @@ static int efifb_probe(struct platform_device *dev)
>
>   err_fb_dealloc_cmap:
>   	fb_dealloc_cmap(&info->cmap);
> -err_groups:
> -	sysfs_remove_groups(&dev->dev.kobj, efifb_groups);
>   err_unmap:
>   	if (mem_flags & (EFI_MEMORY_UC | EFI_MEMORY_WC))
>   		iounmap(info->screen_base);
> @@ -608,12 +601,12 @@ static void efifb_remove(struct platform_device *pdev)
>
>   	/* efifb_destroy takes care of info cleanup */
>   	unregister_framebuffer(info);
> -	sysfs_remove_groups(&pdev->dev.kobj, efifb_groups);
>   }
>
>   static struct platform_driver efifb_driver = {
>   	.driver = {
>   		.name = "efi-framebuffer",
> +		.dev_groups = efifb_groups,
>   	},
>   	.probe = efifb_probe,
>   	.remove_new = efifb_remove,
>

[PATCH 3/5] fbdev: Introduce devm_register_framebuffer()

[Thomas Weißschuh]

Introduce a device-managed variant of register_framebuffer() which
automatically unregisters the framebuffer on device destruction.
This can simplify the error handling and resource management in drivers.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/video/fbdev/core/fbmem.c | 24 ++++++++++++++++++++++++
 include/linux/fb.h               |  1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 4c4ad0a86a50..d17a2daa2483 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -544,6 +544,30 @@ unregister_framebuffer(struct fb_info *fb_info)
 }
 EXPORT_SYMBOL(unregister_framebuffer);
 
+static void devm_unregister_framebuffer(void *data)
+{
+	struct fb_info *info = data;
+
+	unregister_framebuffer(info);
+}
+
+/**
+ *	devm_register_framebuffer - resource-managed frame buffer device registration
+ *	@dev: device the framebuffer belongs to
+ *	@fb_info: frame buffer info structure
+ *
+ *	Registers a frame buffer device @fb_info to device @dev.
+ *
+ *	Returns negative errno on error, or zero for success.
+ *
+ */
+int
+devm_register_framebuffer(struct device *dev, struct fb_info *fb_info)
+{
+	return devm_add_action_or_reset(dev, devm_unregister_framebuffer, fb_info);
+}
+EXPORT_SYMBOL(devm_register_framebuffer);
+
 /**
  *	fb_set_suspend - low level driver signals suspend
  *	@info: framebuffer affected
diff --git a/include/linux/fb.h b/include/linux/fb.h
index db7d97b10964..abf6643ebcaf 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -601,6 +601,7 @@ extern ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
 /* fbmem.c */
 extern int register_framebuffer(struct fb_info *fb_info);
 extern void unregister_framebuffer(struct fb_info *fb_info);
+extern int devm_register_framebuffer(struct device *dev, struct fb_info *fb_info);
 extern char* fb_get_buffer_offset(struct fb_info *info, struct fb_pixmap *buf, u32 size);
 extern void fb_pad_unaligned_buffer(u8 *dst, u32 d_pitch, u8 *src, u32 idx,
 				u32 height, u32 shift_high, u32 shift_low, u32 mod);

-- 
2.46.0

Re: [PATCH 3/5] fbdev: Introduce devm_register_framebuffer()

[Thomas Weißschuh]

Hi everybody,

On 2024-08-27 17:25:14+0000, Thomas Weißschuh wrote:
> Introduce a device-managed variant of register_framebuffer() which
> automatically unregisters the framebuffer on device destruction.
> This can simplify the error handling and resource management in drivers.

Bert reported that this series broke his framebuffer ([0], [1]).

[0] https://lore.kernel.org/lkml/20240829224124.2978-1-spasswolf@web.de/
[1] https://lore.kernel.org/lkml/20240829230438.3226-1-spasswolf@web.de/

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  drivers/video/fbdev/core/fbmem.c | 24 ++++++++++++++++++++++++
>  include/linux/fb.h               |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 4c4ad0a86a50..d17a2daa2483 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -544,6 +544,30 @@ unregister_framebuffer(struct fb_info *fb_info)

[..]

> +/**
> + *	devm_register_framebuffer - resource-managed frame buffer device registration
> + *	@dev: device the framebuffer belongs to
> + *	@fb_info: frame buffer info structure
> + *
> + *	Registers a frame buffer device @fb_info to device @dev.
> + *
> + *	Returns negative errno on error, or zero for success.
> + *
> + */
> +int
> +devm_register_framebuffer(struct device *dev, struct fb_info *fb_info)
> +{
> +	return devm_add_action_or_reset(dev, devm_unregister_framebuffer, fb_info);
> +}
> +EXPORT_SYMBOL(devm_register_framebuffer);

This implementation is wrong, it never actually registers the
framebuffer. It should look like this:

int
devm_register_framebuffer(struct device *dev, struct fb_info *fb_info)
{
	int ret;

	ret = register_framebuffer(fb_info);
	if (ret)
		return ret;

	return devm_add_action_or_reset(dev, devm_unregister_framebuffer, fb_info);
}
EXPORT_SYMBOL(devm_register_framebuffer);

Bert, could you test this?
Helge, do you want me to resend the series, minus the original patch 1?

Re: [PATCH 3/5] fbdev: Introduce devm_register_framebuffer()

[Bert Karwatzki]

Am Freitag, dem 30.08.2024 um 09:17 +0200 schrieb Thomas Weißschuh:
> Hi everybody,
>
> On 2024-08-27 17:25:14+0000, Thomas Weißschuh wrote:
> > Introduce a device-managed variant of register_framebuffer() which
> > automatically unregisters the framebuffer on device destruction.
> > This can simplify the error handling and resource management in drivers.
>
> Bert reported that this series broke his framebuffer ([0], [1]).
>
> [0] https://lore.kernel.org/lkml/20240829224124.2978-1-spasswolf@web.de/
> [1] https://lore.kernel.org/lkml/20240829230438.3226-1-spasswolf@web.de/
>
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  drivers/video/fbdev/core/fbmem.c | 24 ++++++++++++++++++++++++
> >  include/linux/fb.h               |  1 +
> >  2 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> > index 4c4ad0a86a50..d17a2daa2483 100644
> > --- a/drivers/video/fbdev/core/fbmem.c
> > +++ b/drivers/video/fbdev/core/fbmem.c
> > @@ -544,6 +544,30 @@ unregister_framebuffer(struct fb_info *fb_info)
>
> [..]
>
> > +/**
> > + *	devm_register_framebuffer - resource-managed frame buffer device registration
> > + *	@dev: device the framebuffer belongs to
> > + *	@fb_info: frame buffer info structure
> > + *
> > + *	Registers a frame buffer device @fb_info to device @dev.
> > + *
> > + *	Returns negative errno on error, or zero for success.
> > + *
> > + */
> > +int
> > +devm_register_framebuffer(struct device *dev, struct fb_info *fb_info)
> > +{
> > +	return devm_add_action_or_reset(dev, devm_unregister_framebuffer, fb_info);
> > +}
> > +EXPORT_SYMBOL(devm_register_framebuffer);
>
> This implementation is wrong, it never actually registers the
> framebuffer. It should look like this:
>
> int
> devm_register_framebuffer(struct device *dev, struct fb_info *fb_info)
> {
> 	int ret;
>
> 	ret = register_framebuffer(fb_info);
> 	if (ret)
> 		return ret;
>
> 	return devm_add_action_or_reset(dev, devm_unregister_framebuffer, fb_info);
> }
> EXPORT_SYMBOL(devm_register_framebuffer);
>
> Bert, could you test this?
> Helge, do you want me to resend the series, minus the original patch 1?

Yes, this works for me. Thanks!

Bert Karwatzki

Re: [PATCH 3/5] fbdev: Introduce devm_register_framebuffer()

[Helge Deller]

On 8/30/24 10:25, Bert Karwatzki wrote:
> Am Freitag, dem 30.08.2024 um 09:17 +0200 schrieb Thomas Weißschuh:
>> Hi everybody,
>>
>> On 2024-08-27 17:25:14+0000, Thomas Weißschuh wrote:
>>> Introduce a device-managed variant of register_framebuffer() which
>>> automatically unregisters the framebuffer on device destruction.
>>> This can simplify the error handling and resource management in drivers.
>>
>> Bert reported that this series broke his framebuffer ([0], [1]).
>>
>> [0] https://lore.kernel.org/lkml/20240829224124.2978-1-spasswolf@web.de/
>> [1] https://lore.kernel.org/lkml/20240829230438.3226-1-spasswolf@web.de/
>>
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>> ---
>>>   drivers/video/fbdev/core/fbmem.c | 24 ++++++++++++++++++++++++
>>>   include/linux/fb.h               |  1 +
>>>   2 files changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>>> index 4c4ad0a86a50..d17a2daa2483 100644
>>> --- a/drivers/video/fbdev/core/fbmem.c
>>> +++ b/drivers/video/fbdev/core/fbmem.c
>>> @@ -544,6 +544,30 @@ unregister_framebuffer(struct fb_info *fb_info)
>>
>> [..]
>>
>>> +/**
>>> + *	devm_register_framebuffer - resource-managed frame buffer device registration
>>> + *	@dev: device the framebuffer belongs to
>>> + *	@fb_info: frame buffer info structure
>>> + *
>>> + *	Registers a frame buffer device @fb_info to device @dev.
>>> + *
>>> + *	Returns negative errno on error, or zero for success.
>>> + *
>>> + */
>>> +int
>>> +devm_register_framebuffer(struct device *dev, struct fb_info *fb_info)
>>> +{
>>> +	return devm_add_action_or_reset(dev, devm_unregister_framebuffer, fb_info);
>>> +}
>>> +EXPORT_SYMBOL(devm_register_framebuffer);
>>
>> This implementation is wrong, it never actually registers the
>> framebuffer. It should look like this:
>>
>> int
>> devm_register_framebuffer(struct device *dev, struct fb_info *fb_info)
>> {
>> 	int ret;
>>
>> 	ret = register_framebuffer(fb_info);
>> 	if (ret)
>> 		return ret;
>>
>> 	return devm_add_action_or_reset(dev, devm_unregister_framebuffer, fb_info);
>> }
>> EXPORT_SYMBOL(devm_register_framebuffer);
>>
>> Bert, could you test this?
>> Helge, do you want me to resend the series, minus the original patch 1?
>
> Yes, this works for me. Thanks!

Good.

Thomas, please just resend the fixed patch #3 (this one).
And maybe you want to document devm_unregister_framebuffer()
similiar to what you added for devm_register_framebuffer() ?

Helge

[PATCH 4/5] fbdev/efifb: Use devm_register_framebuffer()

[Thomas Weißschuh]

This simplifies the error handling.
Also the drvdata slot is now unused and can be used for other usecases.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/video/fbdev/efifb.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index d36b95856dd0..7215973ef602 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -449,7 +449,6 @@ static int efifb_probe(struct platform_device *dev)
 		err = -ENOMEM;
 		goto err_release_mem;
 	}
-	platform_set_drvdata(dev, info);
 	par = info->par;
 	info->pseudo_palette = par->pseudo_palette;
 
@@ -572,7 +571,7 @@ static int efifb_probe(struct platform_device *dev)
 		pr_err("efifb: cannot acquire aperture\n");
 		goto err_fb_dealloc_cmap;
 	}
-	err = register_framebuffer(info);
+	err = devm_register_framebuffer(&dev->dev, info);
 	if (err < 0) {
 		pr_err("efifb: cannot register framebuffer\n");
 		goto err_fb_dealloc_cmap;
@@ -595,21 +594,12 @@ static int efifb_probe(struct platform_device *dev)
 	return err;
 }
 
-static void efifb_remove(struct platform_device *pdev)
-{
-	struct fb_info *info = platform_get_drvdata(pdev);
-
-	/* efifb_destroy takes care of info cleanup */
-	unregister_framebuffer(info);
-}
-
 static struct platform_driver efifb_driver = {
 	.driver = {
 		.name = "efi-framebuffer",
 		.dev_groups = efifb_groups,
 	},
 	.probe = efifb_probe,
-	.remove_new = efifb_remove,
 };
 
 builtin_platform_driver(efifb_driver);

-- 
2.46.0

[PATCH 5/5] fbdev/efifb: Use driver-private screen_info for sysfs

[Thomas Weißschuh]

Since commit b9cfd1d271ab ("fbdev/efifb: Use screen_info pointer from device")
efifb uses a local copy of screen_info and applies its modifications
there. Adapt the sysfs attributes to also work with the custom copy
instead of the unmodified platform data.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/video/fbdev/efifb.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index 7215973ef602..1f86a07bf292 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -304,7 +304,7 @@ static ssize_t name##_show(struct device *dev,				\
 			   struct device_attribute *attr,		\
 			   char *buf)					\
 {									\
-	struct screen_info *si = dev_get_platdata(dev);			\
+	struct screen_info *si = dev_get_drvdata(dev);			\
 	if (!si)							\
 		return -ENODEV;						\
 	return sprintf(buf, fmt "\n", (si->lfb_##name));		\
@@ -369,6 +369,8 @@ static int efifb_probe(struct platform_device *dev)
 	if (!si)
 		return -ENOMEM;
 
+	dev_set_drvdata(&dev->dev, si);
+
 	if (si->orig_video_isVGA != VIDEO_TYPE_EFI)
 		return -ENODEV;
 

-- 
2.46.0