linux-kernel - Re: [PATCH] fbdev: mb862xxfbdrv: Make CONFIG_FB

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ewca4jzmahwdl47rbojxtynbizu2vuompjxrprsz7aelovnvao@kzpxjjbjj6px>
Date: Mon, 6 Oct 2025 10:07:31 +0200
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Javier Garcia <rampxxxx@...il.com>
Cc: deller@....de, tzimmermann@...e.de, linux-fbdev@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, shuah@...nel.org
Subject: Re: [PATCH] fbdev: mb862xxfbdrv: Make CONFIG_FB_DEVICE optional

Hello,

On Sun, Oct 05, 2025 at 07:38:12PM +0200, Javier Garcia wrote:
> This patch wraps the relevant code blocks with #ifdef CONFIG_FB_DEVICE,
> allowing the driver to be built and used even if CONFIG_FB_DEVICE is not selected.
> 
> The sysfs only give access to show some controller and cursor registers so
> it's not needed to allow driver works correctly.
> 
> Signed-off-by: Javier Garcia <rampxxxx@...il.com>

I don't understand this patch. CONFIG_FB_DEVICE is about legacy /dev/fb*
stuff, and this patch make the creation of a sysfs file dependent on
that.

> diff --git a/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c b/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c
> index ade88e7bc760..a691a5fefb25 100644
> --- a/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c
> +++ b/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c
> @@ -530,6 +530,7 @@ static int mb862xxfb_init_fbinfo(struct fb_info *fbi)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_FB_DEVICE
>  /*
>   * show some display controller and cursor registers
>   */
> @@ -569,6 +570,7 @@ static ssize_t dispregs_show(struct device *dev,
>  }
>  
>  static DEVICE_ATTR_RO(dispregs);
> +#endif
>  
>  static irqreturn_t mb862xx_intr(int irq, void *dev_id)
>  {
> @@ -759,9 +761,11 @@ static int of_platform_mb862xx_probe(struct platform_device *ofdev)
>  
>  	dev_set_drvdata(dev, info);
>  
> +#ifdef CONFIG_FB_DEVICE
>  	if (device_create_file(dev, &dev_attr_dispregs))
>  		dev_err(dev, "Can't create sysfs regdump file\n");
>  	return 0;
> +#endif

That looks wrong. Without CONFIG_FB_DEVICE set the success return is
removed and all resources are freed essentially making the
CONFIG_FB_MB862XX_LIME part of the driver unusable.

>  rel_cmap:
>  	fb_dealloc_cmap(&info->cmap);
> @@ -801,7 +805,9 @@ static void of_platform_mb862xx_remove(struct platform_device *ofdev)
>  	free_irq(par->irq, (void *)par);
>  	irq_dispose_mapping(par->irq);
>  
> +#ifdef CONFIG_FB_DEVICE
>  	device_remove_file(&ofdev->dev, &dev_attr_dispregs);
> +#endif
>  
>  	unregister_framebuffer(fbi);
>  	fb_dealloc_cmap(&fbi->cmap);
> @@ -1101,8 +1107,10 @@ static int mb862xx_pci_probe(struct pci_dev *pdev,
>  
>  	pci_set_drvdata(pdev, info);
>  
> +#ifdef CONFIG_FB_DEVICE
>  	if (device_create_file(dev, &dev_attr_dispregs))
>  		dev_err(dev, "Can't create sysfs regdump file\n");
> +#endif
>  
>  	if (par->type == BT_CARMINE)
>  		outreg(ctrl, GC_CTRL_INT_MASK, GC_CARMINE_INT_EN);
> @@ -1151,7 +1159,9 @@ static void mb862xx_pci_remove(struct pci_dev *pdev)
>  
>  	mb862xx_i2c_exit(par);
>  
> +#ifdef CONFIG_FB_DEVICE
>  	device_remove_file(&pdev->dev, &dev_attr_dispregs);
> +#endif
>  
>  	unregister_framebuffer(fbi);
>  	fb_dealloc_cmap(&fbi->cmap);

The amount of ifdefs could be greatly reduced using the following patch:

diff --git a/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c b/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c
index a691a5fefb25..3f79dfc27a53 100644
--- a/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c
+++ b/drivers/video/fbdev/mb862xx/mb862xxfbdrv.c
@@ -530,7 +530,6 @@ static int mb862xxfb_init_fbinfo(struct fb_info *fbi)
 	return 0;
 }
 
-#ifdef CONFIG_FB_DEVICE
 /*
  * show some display controller and cursor registers
  */
@@ -570,7 +569,6 @@ static ssize_t dispregs_show(struct device *dev,
 }
 
 static DEVICE_ATTR_RO(dispregs);
-#endif
 
 static irqreturn_t mb862xx_intr(int irq, void *dev_id)
 {
@@ -761,11 +759,9 @@ static int of_platform_mb862xx_probe(struct platform_device *ofdev)
 
 	dev_set_drvdata(dev, info);
 
-#ifdef CONFIG_FB_DEVICE
-	if (device_create_file(dev, &dev_attr_dispregs))
+	if (IS_ENABLED(CONFIG_FB_DEVICE) && device_create_file(dev, &dev_attr_dispregs))
 		dev_err(dev, "Can't create sysfs regdump file\n");
 	return 0;
-#endif
 
 rel_cmap:
 	fb_dealloc_cmap(&info->cmap);
@@ -805,9 +801,8 @@ static void of_platform_mb862xx_remove(struct platform_device *ofdev)
 	free_irq(par->irq, (void *)par);
 	irq_dispose_mapping(par->irq);
 
-#ifdef CONFIG_FB_DEVICE
-	device_remove_file(&ofdev->dev, &dev_attr_dispregs);
-#endif
+	if (IS_ENABLED(CONFIG_FB_DEVICE))
+		device_remove_file(&ofdev->dev, &dev_attr_dispregs);
 
 	unregister_framebuffer(fbi);
 	fb_dealloc_cmap(&fbi->cmap);
@@ -1107,10 +1102,8 @@ static int mb862xx_pci_probe(struct pci_dev *pdev,
 
 	pci_set_drvdata(pdev, info);
 
-#ifdef CONFIG_FB_DEVICE
-	if (device_create_file(dev, &dev_attr_dispregs))
+	if (IS_ENABLED(CONFIG_FB_DEVICE) && device_create_file(dev, &dev_attr_dispregs))
 		dev_err(dev, "Can't create sysfs regdump file\n");
-#endif
 
 	if (par->type == BT_CARMINE)
 		outreg(ctrl, GC_CTRL_INT_MASK, GC_CARMINE_INT_EN);
@@ -1159,9 +1152,8 @@ static void mb862xx_pci_remove(struct pci_dev *pdev)
 
 	mb862xx_i2c_exit(par);
 
-#ifdef CONFIG_FB_DEVICE
-	device_remove_file(&pdev->dev, &dev_attr_dispregs);
-#endif
+	if (IS_ENABLED(CONFIG_FB_DEVICE))
+		device_remove_file(&pdev->dev, &dev_attr_dispregs);
 
 	unregister_framebuffer(fbi);
 	fb_dealloc_cmap(&fbi->cmap);

(It would still be questionable however to make the device file creation
dependent on FB_DEVICE.)

Best regards
Uwe