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] [day] [month] [year] [list]
Message-ID: <CABPJ0vjtr2e=PMPF7AAi-Q=CrwgdHoO6HDb+G8PWF8T6b2_6Vw@mail.gmail.com>
Date: Mon, 6 Oct 2025 18:10:16 +0200
From: Javier Garcia <rampxxxx@...il.com>
To: Uwe Kleine-König <u.kleine-koenig@...libre.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 Mon, 6 Oct 2025 at 10:07, Uwe Kleine-König
<u.kleine-koenig@...libre.com> wrote:
>
> 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.

This is part of the TODO
https://www.kernel.org/doc/html/latest/gpu/todo.html#remove-driver-dependencies-on-fb-device

I think the idea is to make the driver independent of CONFIG_FB_DEVICE .

>
> > 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.

Thanks, I'll fix that.

And add your suggestion instead of the `ifdef`.

>
> >  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.)

https://www.kernel.org/doc/html/latest/gpu/todo.html#remove-driver-dependencies-on-fb-device

>
> Best regards
> Uwe