[PATCH v2] fbdev: sh_mobile_lcdcfb: Restore the per-overlay sysfs attributes

[PATCH v2] fbdev: sh_mobile_lcdcfb: Restore the per-overlay sysfs attributes

[Karl Mehltretter]

The ovl_* files documented in
Documentation/ABI/testing/sysfs-devices-platform-sh_mobile_lcdc_fb no
longer exist.

Commit a979182a2453 ("fbdev: lcdcfb: Register sysfs groups through driver
core") moved the attributes from each overlay's framebuffer device to
the platform driver's dev_groups and renamed them overlay_*. There is
now only one set of attributes for the whole LCDC. The callbacks still
expect dev_get_drvdata() to return a struct fb_info, but the platform
device holds struct sh_mobile_lcdc_priv, so they access the wrong
structure.

Restore the documented names and register the group on each overlay's
framebuffer device with device_add_groups(). Remove it with
device_remove_groups() before unregistering the framebuffer. Skip
creation when dev_of_fbinfo(info) is NULL, as the old device_create_file()
calls did.

Drop the CONFIG_FB_DEVICE guard, since the driver already depends on it.

Fixes: a979182a2453 ("fbdev: lcdcfb: Register sysfs groups through driver core")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
---
Changes in v2:
- Use dev_of_fbinfo() for overlay sysfs registration and removal (Helge).

v1: https://lore.kernel.org/r/20260907021844.25497-1-kmehltretter@gmail.com/

Found while reviewing the sysfs ABI documentation. Shixiong confirmed
that using the platform driver's dev_groups is wrong here.

Compile-tested with Clang 22.1.8, ARCH=arm and W=1, using multi_v7_defconfig
with COMPILE_TEST=y, FB_DEVICE=y and FB_SH_MOBILE_LCDC=m.

Also checked FB_DEVICE=n: olddefconfig disables FB_SH_MOBILE_LCDC because
of its existing Kconfig dependency, and the framebuffer core compiles.
This does not test the driver with FB_DEVICE=n; removing that dependency
requires additional changes outside the overlay sysfs paths.

No runtime test; I have no SH-Mobile hardware.

 drivers/video/fbdev/sh_mobile_lcdcfb.c | 39 +++++++++++++++++---------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
index e8324b01700f..5743c96d5481 100644
--- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
+++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
@@ -1337,22 +1337,24 @@ overlay_rop3_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
-static DEVICE_ATTR_RW(overlay_alpha);
-static DEVICE_ATTR_RW(overlay_mode);
-static DEVICE_ATTR_RW(overlay_position);
-static DEVICE_ATTR_RW(overlay_rop3);
-
-static struct attribute *overlay_sysfs_attrs[] __maybe_unused = {
-	&dev_attr_overlay_alpha.attr,
-	&dev_attr_overlay_mode.attr,
-	&dev_attr_overlay_position.attr,
-	&dev_attr_overlay_rop3.attr,
+static struct device_attribute dev_attr_ovl_alpha =
+	__ATTR(ovl_alpha, 0644, overlay_alpha_show, overlay_alpha_store);
+static struct device_attribute dev_attr_ovl_mode =
+	__ATTR(ovl_mode, 0644, overlay_mode_show, overlay_mode_store);
+static struct device_attribute dev_attr_ovl_position =
+	__ATTR(ovl_position, 0644, overlay_position_show, overlay_position_store);
+static struct device_attribute dev_attr_ovl_rop3 =
+	__ATTR(ovl_rop3, 0644, overlay_rop3_show, overlay_rop3_store);
+
+static struct attribute *overlay_sysfs_attrs[] = {
+	&dev_attr_ovl_alpha.attr,
+	&dev_attr_ovl_mode.attr,
+	&dev_attr_ovl_position.attr,
+	&dev_attr_ovl_rop3.attr,
 	NULL,
 };
 
-#ifdef CONFIG_FB_DEVICE
 ATTRIBUTE_GROUPS(overlay_sysfs);
-#endif
 
 static const struct fb_fix_screeninfo sh_mobile_lcdc_overlay_fix  = {
 	.id =		"SH Mobile LCDC",
@@ -1510,9 +1512,10 @@ sh_mobile_lcdc_overlay_fb_unregister(struct sh_mobile_lcdc_overlay *ovl)
 {
 	struct fb_info *info = ovl->info;
 
-	if (info == NULL || info->dev == NULL)
+	if (!info || !dev_of_fbinfo(info))
 		return;
 
+	device_remove_groups(dev_of_fbinfo(info), overlay_sysfs_groups);
 	unregister_framebuffer(ovl->info);
 }
 
@@ -1530,6 +1533,15 @@ sh_mobile_lcdc_overlay_fb_register(struct sh_mobile_lcdc_overlay *ovl)
 	if (ret < 0)
 		return ret;
 
+	/* The framebuffer device is optional, see fb_device_create(). */
+	if (dev_of_fbinfo(info)) {
+		ret = device_add_groups(dev_of_fbinfo(info), overlay_sysfs_groups);
+		if (ret < 0) {
+			unregister_framebuffer(info);
+			return ret;
+		}
+	}
+
 	dev_info(lcdc->dev, "registered %s/overlay %u as %dx%d %dbpp.\n",
 		 dev_name(lcdc->dev), ovl->index, info->var.xres,
 		 info->var.yres, info->var.bits_per_pixel);
@@ -2637,7 +2649,6 @@ static int sh_mobile_lcdc_probe(struct platform_device *pdev)
 static struct platform_driver sh_mobile_lcdc_driver = {
 	.driver		= {
 		.name		= "sh_mobile_lcdc_fb",
-		.dev_groups	= overlay_sysfs_groups,
 		.pm		= &sh_mobile_lcdc_dev_pm_ops,
 	},
 	.probe		= sh_mobile_lcdc_probe,

base-commit: 986c24e0fe44f844b44d365b71ce831947f50298
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v2] fbdev: sh_mobile_lcdcfb: Restore the per-overlay sysfs attributes

[Helge Deller]

On 9/12/26 12:21, Karl Mehltretter wrote:
> The ovl_* files documented in
> Documentation/ABI/testing/sysfs-devices-platform-sh_mobile_lcdc_fb no
> longer exist.
> 
> Commit a979182a2453 ("fbdev: lcdcfb: Register sysfs groups through driver
> core") moved the attributes from each overlay's framebuffer device to
> the platform driver's dev_groups and renamed them overlay_*. There is
> now only one set of attributes for the whole LCDC. The callbacks still
> expect dev_get_drvdata() to return a struct fb_info, but the platform
> device holds struct sh_mobile_lcdc_priv, so they access the wrong
> structure.
> 
> Restore the documented names and register the group on each overlay's
> framebuffer device with device_add_groups(). Remove it with
> device_remove_groups() before unregistering the framebuffer. Skip
> creation when dev_of_fbinfo(info) is NULL, as the old device_create_file()
> calls did.
> 
> Drop the CONFIG_FB_DEVICE guard, since the driver already depends on it.
> 
> Fixes: a979182a2453 ("fbdev: lcdcfb: Register sysfs groups through driver core")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Karl Mehltretter <kmehltretter@gmail.com>
> ---
> Changes in v2:
> - Use dev_of_fbinfo() for overlay sysfs registration and removal (Helge).
> 
> v1: https://lore.kernel.org/r/20260907021844.25497-1-kmehltretter@gmail.com/
> 
> Found while reviewing the sysfs ABI documentation. Shixiong confirmed
> that using the platform driver's dev_groups is wrong here.
> 
> Compile-tested with Clang 22.1.8, ARCH=arm and W=1, using multi_v7_defconfig
> with COMPILE_TEST=y, FB_DEVICE=y and FB_SH_MOBILE_LCDC=m.
> 
> Also checked FB_DEVICE=n: olddefconfig disables FB_SH_MOBILE_LCDC because
> of its existing Kconfig dependency, and the framebuffer core compiles.
> This does not test the driver with FB_DEVICE=n; removing that dependency
> requires additional changes outside the overlay sysfs paths.
> 
> No runtime test; I have no SH-Mobile hardware.
> 
>   drivers/video/fbdev/sh_mobile_lcdcfb.c | 39 +++++++++++++++++---------
>   1 file changed, 25 insertions(+), 14 deletions(-)
applied.

Thanks!
Helge

Re: [PATCH v2] fbdev: sh_mobile_lcdcfb: Restore the per-overlay sysfs attributes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Framebuffer structure (`fb_info`) Use-After-Free and registration leak if `fb_device_create` fails.
- [High] Concurrent sysfs stores and fbdev ioctls cause data races and hardware register clobbering due to missing locks.
--

commit 2445fda1d6b2eaa304debdee2b9155ce76236747
Author: Karl Mehltretter <kmehltretter@gmail.com>

fbdev: sh_mobile_lcdcfb: Restore the per-overlay sysfs attributes

This commit restores the per-overlay sysfs attributes by renaming them and
registering the sysfs groups on each overlay's framebuffer device instead
of the platform driver's dev_groups. It also fixes incorrect dev_get_drvdata()
accesses caused by an earlier regression.

> diff --git a/drivers/video/fbdev/sh_mobile_lcdcfb.c b/drivers/video/fbdev/sh_mobile_lcdcfb.c
> index e8324b01700f6..5743c96d54814 100644
> --- a/drivers/video/fbdev/sh_mobile_lcdcfb.c
> +++ b/drivers/video/fbdev/sh_mobile_lcdcfb.c
[ ... ]
> @@ -1337,22 +1337,24 @@ overlay_rop3_store(struct device *dev, struct device_attribute *attr,
>  	return count;
>  }
>  
> -static DEVICE_ATTR_RW(overlay_alpha);
> -static DEVICE_ATTR_RW(overlay_mode);
> -static DEVICE_ATTR_RW(overlay_position);
> -static DEVICE_ATTR_RW(overlay_rop3);
> -
> -static struct attribute *overlay_sysfs_attrs[] __maybe_unused = {
> -	&dev_attr_overlay_alpha.attr,
> -	&dev_attr_overlay_mode.attr,
> -	&dev_attr_overlay_position.attr,
> -	&dev_attr_overlay_rop3.attr,
> +static struct device_attribute dev_attr_ovl_alpha =
> +	__ATTR(ovl_alpha, 0644, overlay_alpha_show, overlay_alpha_store);

[Severity: High]
This isn't a bug introduced by this patch, but do these sysfs store
callbacks need locking to prevent hardware state corruption?

Functions like overlay_alpha_store() and overlay_rop3_store() modify
overlay state variables and call sh_mobile_lcdc_overlay_setup() to perform
direct I/O writes to the LCDC hardware registers without acquiring any locks.

Concurrently, standard fbdev ioctls like fb_blank() modify the same state
and call sh_mobile_lcdc_overlay_setup() from sh_mobile_lcdc_overlay_blank()
while properly holding info->lock.

Could concurrent sysfs writes and fbdev ioctls interleave register writes and
clobber the hardware state?

[ ... ]
> @@ -1510,9 +1512,10 @@ sh_mobile_lcdc_overlay_fb_unregister(struct sh_mobile_lcdc_overlay *ovl)
>  {
>  	struct fb_info *info = ovl->info;
>  
> -	if (info == NULL || info->dev == NULL)
> +	if (!info || !dev_of_fbinfo(info))
>  		return;

[Severity: High]
This is a pre-existing issue, but does this early return cause a memory leak
and use-after-free in the fbdev core?

If fb_device_create() fails due to memory pressure or sysfs conflicts,
info->dev is set to NULL but register_framebuffer() still returns success.
When tearing down the device, this check for dev_of_fbinfo() evaluates to
true and causes an early return, bypassing unregister_framebuffer().

Later, sh_mobile_lcdc_overlay_fb_cleanup() calls framebuffer_release(), which
frees the fb_info structure while it is still linked in the fbdev core's
registered_fb array.

Can unregister_framebuffer() be called regardless of whether device creation
succeeded, both here and in sh_mobile_lcdc_channel_fb_unregister()?

>  
> +	device_remove_groups(dev_of_fbinfo(info), overlay_sysfs_groups);
>  	unregister_framebuffer(ovl->info);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912102152.86744-1-kmehltretter@gmail.com?part=1