diff mbox series

[30/30] fbdev: Make support for userspace interfaces configurable

Message ID 20230605144812.15241-31-tzimmermann@suse.de (mailing list archive)
State Superseded
Headers show
Series fbdev: Make userspace interfaces optional | expand

Commit Message

Thomas Zimmermann June 5, 2023, 2:48 p.m. UTC
Add Kconfig option CONFIG_FB_DEVICE and make the virtual fbdev
device optional. If the new option has not been selected, fbdev
does not create a files in devfs or sysfs.

Most modern Linux systems run a DRM-based graphics stack that uses
the kernel's framebuffer console, but has otherwise deprecated fbdev
support. Yet fbdev userspace interfaces are still present.

The option makes it possible to use the fbdev subsystem as console
implementation without support for userspace. This closes potential
entry points to manipulate kernel or I/O memory via framebuffers. It
also prevents the execution of driver code via ioctl or sysfs, both
of which might allow malicious software to exploit bugs in the fbdev
code.

A small number of fbdev drivers require struct fbinfo.dev to be
initialized, usually for the support of sysfs interface. Make these
drivers depend on FB_DEVICE. They can later be fixed if necessary.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/staging/fbtft/Kconfig            |  1 +
 drivers/video/fbdev/Kconfig              | 12 +++++++++
 drivers/video/fbdev/core/Makefile        |  7 +++---
 drivers/video/fbdev/core/fb_internal.h   | 32 ++++++++++++++++++++++++
 drivers/video/fbdev/omap2/omapfb/Kconfig |  2 +-
 include/linux/fb.h                       |  2 ++
 6 files changed, 52 insertions(+), 4 deletions(-)

Comments

Greg KH June 5, 2023, 3:03 p.m. UTC | #1
On Mon, Jun 05, 2023 at 04:48:12PM +0200, Thomas Zimmermann wrote:
> Add Kconfig option CONFIG_FB_DEVICE and make the virtual fbdev
> device optional. If the new option has not been selected, fbdev
> does not create a files in devfs or sysfs.
> 
> Most modern Linux systems run a DRM-based graphics stack that uses
> the kernel's framebuffer console, but has otherwise deprecated fbdev
> support. Yet fbdev userspace interfaces are still present.
> 
> The option makes it possible to use the fbdev subsystem as console
> implementation without support for userspace. This closes potential
> entry points to manipulate kernel or I/O memory via framebuffers. It
> also prevents the execution of driver code via ioctl or sysfs, both
> of which might allow malicious software to exploit bugs in the fbdev
> code.
> 
> A small number of fbdev drivers require struct fbinfo.dev to be
> initialized, usually for the support of sysfs interface. Make these
> drivers depend on FB_DEVICE. They can later be fixed if necessary.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
kernel test robot June 5, 2023, 9:45 p.m. UTC | #2
Hi Thomas,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20230605]
[cannot apply to drm-misc/drm-misc-next lee-backlight/for-backlight-next staging/staging-testing staging/staging-next staging/staging-linus linus/master lee-backlight/for-backlight-fixes v6.4-rc5 v6.4-rc4 v6.4-rc3 v6.4-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/backlight-bd6107-Compare-against-struct-fb_info-device/20230605-225002
base:   next-20230605
patch link:    https://lore.kernel.org/r/20230605144812.15241-31-tzimmermann%40suse.de
patch subject: [PATCH 30/30] fbdev: Make support for userspace interfaces configurable
config: powerpc-randconfig-r036-20230605 (https://download.01.org/0day-ci/archive/20230606/202306060547.528pADrX-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 4faf3aaf28226a4e950c103a14f6fc1d1fdabb1b)
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/d309f36af8a1ee56ef56e54287ca6d2bf7d239de
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/backlight-bd6107-Compare-against-struct-fb_info-device/20230605-225002
        git checkout d309f36af8a1ee56ef56e54287ca6d2bf7d239de
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=powerpc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash drivers/video/fbdev/mb862xx/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306060547.528pADrX-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/video/fbdev/mb862xx/mb862xxfbdrv.c:793:15: error: no member named 'dev' in 'struct fb_info'
           dev_dbg(fbi->dev, "%s release\n", fbi->fix.id);
                   ~~~  ^
   include/linux/dev_printk.h:163:26: note: expanded from macro 'dev_dbg'
                   dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__); \
                                          ^~~
   include/linux/dev_printk.h:129:22: note: expanded from macro 'dev_printk'
                   _dev_printk(level, dev, fmt, ##__VA_ARGS__);            \
                                      ^~~
   1 error generated.


vim +793 drivers/video/fbdev/mb862xx/mb862xxfbdrv.c

17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  785  
3a2ab02ddfacb0 drivers/video/fbdev/mb862xx/mb862xxfbdrv.c Uwe Kleine-König   2023-03-19  786  static void of_platform_mb862xx_remove(struct platform_device *ofdev)
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  787  {
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  788  	struct fb_info *fbi = dev_get_drvdata(&ofdev->dev);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  789  	struct mb862xxfb_par *par = fbi->par;
28f65c11f2ffb3 drivers/video/mb862xx/mb862xxfbdrv.c       Joe Perches        2011-06-09  790  	resource_size_t res_size = resource_size(par->res);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  791  	unsigned long reg;
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  792  
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06 @793  	dev_dbg(fbi->dev, "%s release\n", fbi->fix.id);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  794  
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  795  	/* display off */
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  796  	reg = inreg(disp, GC_DCM1);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  797  	reg &= ~(GC_DCM01_DEN | GC_DCM01_L0E);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  798  	outreg(disp, GC_DCM1, reg);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  799  
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  800  	/* disable interrupts */
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  801  	outreg(host, GC_IMASK, 0);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  802  
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  803  	free_irq(par->irq, (void *)par);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  804  	irq_dispose_mapping(par->irq);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  805  
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  806  	device_remove_file(&ofdev->dev, &dev_attr_dispregs);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  807  
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  808  	unregister_framebuffer(fbi);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  809  	fb_dealloc_cmap(&fbi->cmap);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  810  
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  811  	iounmap(par->mmio_base);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  812  	iounmap(par->fb_base);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  813  
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  814  	release_mem_region(par->res->start, res_size);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  815  	framebuffer_release(fbi);
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  816  }
17a1217e12d8c8 drivers/video/mb862xx/mb862xxfb.c          Anatolij Gustschin 2008-11-06  817
Geert Uytterhoeven June 7, 2023, 8:48 a.m. UTC | #3
Hi Thomas,

Thanks for your patch!

On Mon, Jun 5, 2023 at 4:48 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Add Kconfig option CONFIG_FB_DEVICE and make the virtual fbdev
> device optional. If the new option has not been selected, fbdev
> does not create a files in devfs or sysfs.
>
> Most modern Linux systems run a DRM-based graphics stack that uses
> the kernel's framebuffer console, but has otherwise deprecated fbdev
> support. Yet fbdev userspace interfaces are still present.
>
> The option makes it possible to use the fbdev subsystem as console
> implementation without support for userspace. This closes potential
> entry points to manipulate kernel or I/O memory via framebuffers. It

I'd leave out the part about manipulating kernel memory, as that's a
driver bug, if possible.

> also prevents the execution of driver code via ioctl or sysfs, both
> of which might allow malicious software to exploit bugs in the fbdev
> code.

Of course disabling ioctls reduces the attack surface, and this is not
limited to fbdev... ;-)

I'm wondering if it would be worthwhile to optionally provide a more
limited userspace API for real fbdev drivers:
  1. No access to MMIO, only to the mapped frame buffer,
  2. No driver-specific ioctls, only the standard ones.

> A small number of fbdev drivers require struct fbinfo.dev to be
> initialized, usually for the support of sysfs interface. Make these
> drivers depend on FB_DEVICE. They can later be fixed if necessary.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

> --- a/drivers/video/fbdev/Kconfig
> +++ b/drivers/video/fbdev/Kconfig
> @@ -57,6 +57,15 @@ config FIRMWARE_EDID
>           combination with certain motherboards and monitors are known to
>           suffer from this problem.
>
> +config FB_DEVICE
> +        bool "Provide legacy /dev/fb* device"

Perhaps "default y if !DRM", although that does not help for a
mixed drm/fbdev kernel build?

Or reserve "FB" for real fbdev drivers, and introduce a new FB_CORE,
to be selected by both FB and DRM_FBDEV_EMULATION?
Then FB_DEVICE can depend on FB_CORE, and default to y if FB.

> +        depends on FB
> +        help
> +         Say Y here if you want the legacy /dev/fb* device file. It's
> +         only required if you have userspace programs that depend on
> +         fbdev for graphics output. This does not effect the framebuffer

affect

> +         console.
> +
>  config FB_DDC
>         tristate
>         depends on FB

Gr{oetje,eeting}s,

                        Geert
Thomas Zimmermann June 7, 2023, 3:15 p.m. UTC | #4
Hi

Am 07.06.23 um 10:48 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> Thanks for your patch!
> 
> On Mon, Jun 5, 2023 at 4:48 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Add Kconfig option CONFIG_FB_DEVICE and make the virtual fbdev
>> device optional. If the new option has not been selected, fbdev
>> does not create a files in devfs or sysfs.
>>
>> Most modern Linux systems run a DRM-based graphics stack that uses
>> the kernel's framebuffer console, but has otherwise deprecated fbdev
>> support. Yet fbdev userspace interfaces are still present.
>>
>> The option makes it possible to use the fbdev subsystem as console
>> implementation without support for userspace. This closes potential
>> entry points to manipulate kernel or I/O memory via framebuffers. It
> 
> I'd leave out the part about manipulating kernel memory, as that's a
> driver bug, if possible.

The driver/core distinction is somewhat fuzzy: the recent bug with OOB 
access was introduced accidentally in shared helper code within DRM.

And whenever I want to modify the fbdev code, I have to start bugfixing 
first. Just look at this patchset. A good number of the patches are 
bugfixes. Driver or not, I no longer trust any of the fbdev code to get 
anything right.


> 
>> also prevents the execution of driver code via ioctl or sysfs, both
>> of which might allow malicious software to exploit bugs in the fbdev
>> code.
> 
> Of course disabling ioctls reduces the attack surface, and this is not
> limited to fbdev... ;-)

Other subsystems should do the same where possible. The specific problem 
with DRM-plus-fbdev is that the fbdev device doesn't provide any 
additional value. It's too limited in functionality (even by fbdev 
standards), a possible source for bugs, and today's userspace wants DRM 
functionality.


> 
> I'm wondering if it would be worthwhile to optionally provide a more
> limited userspace API for real fbdev drivers:
>    1. No access to MMIO, only to the mapped frame buffer,
>    2. No driver-specific ioctls, only the standard ones.

That issue is only relevant to fbdev drivers and would be a separate 
patchset. My concern lies with the current distributions, which don't 
need the fbdev device and shouldn't provide it for the given reasons.


> 
>> A small number of fbdev drivers require struct fbinfo.dev to be
>> initialized, usually for the support of sysfs interface. Make these
>> drivers depend on FB_DEVICE. They can later be fixed if necessary.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
>> --- a/drivers/video/fbdev/Kconfig
>> +++ b/drivers/video/fbdev/Kconfig
>> @@ -57,6 +57,15 @@ config FIRMWARE_EDID
>>            combination with certain motherboards and monitors are known to
>>            suffer from this problem.
>>
>> +config FB_DEVICE
>> +        bool "Provide legacy /dev/fb* device"
> 
> Perhaps "default y if !DRM", although that does not help for a
> mixed drm/fbdev kernel build?

We could simply set it to "default y".  But OTOH is it worth making it a 
default? Distributions will set it to the value they need/want. The very 
few people that build their own kernels to get certain fbdev drivers 
will certainly be able to enable the option by hand as well.


> 
> Or reserve "FB" for real fbdev drivers, and introduce a new FB_CORE,
> to be selected by both FB and DRM_FBDEV_EMULATION?
> Then FB_DEVICE can depend on FB_CORE, and default to y if FB.

That wouldn't work. In Tumbleweed, we still have efifb and vesafb 
enabled under certain conditions; merely for the kernel console. We'd 
have to enable CONFIG_FB, which would bring back the device.

Best regards
Thomas

> 
>> +        depends on FB
>> +        help
>> +         Say Y here if you want the legacy /dev/fb* device file. It's
>> +         only required if you have userspace programs that depend on
>> +         fbdev for graphics output. This does not effect the framebuffer
> 
> affect
> 
>> +         console.
>> +
>>   config FB_DDC
>>          tristate
>>          depends on FB
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>
Geert Uytterhoeven June 7, 2023, 3:24 p.m. UTC | #5
Hi Thomas,

On Wed, Jun 7, 2023 at 5:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 07.06.23 um 10:48 schrieb Geert Uytterhoeven:
> > On Mon, Jun 5, 2023 at 4:48 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> --- a/drivers/video/fbdev/Kconfig
> >> +++ b/drivers/video/fbdev/Kconfig
> >> @@ -57,6 +57,15 @@ config FIRMWARE_EDID
> >>            combination with certain motherboards and monitors are known to
> >>            suffer from this problem.
> >>
> >> +config FB_DEVICE
> >> +        bool "Provide legacy /dev/fb* device"
> >
> > Perhaps "default y if !DRM", although that does not help for a
> > mixed drm/fbdev kernel build?
>
> We could simply set it to "default y".  But OTOH is it worth making it a
> default? Distributions will set it to the value they need/want. The very
> few people that build their own kernels to get certain fbdev drivers
> will certainly be able to enable the option by hand as well.

Defaulting to "n" (the default) means causing regressions when these
few people use an existing defconfig.

> > Or reserve "FB" for real fbdev drivers, and introduce a new FB_CORE,
> > to be selected by both FB and DRM_FBDEV_EMULATION?
> > Then FB_DEVICE can depend on FB_CORE, and default to y if FB.
>
> That wouldn't work. In Tumbleweed, we still have efifb and vesafb
> enabled under certain conditions; merely for the kernel console. We'd
> have to enable CONFIG_FB, which would bring back the device.

"Default y" does not mean that you cannot disable FB_DEVICE, so
you are not forced to bring back the device?

Gr{oetje,eeting}s,

                        Geert
Javier Martinez Canillas June 7, 2023, 11:07 p.m. UTC | #6
Geert Uytterhoeven <geert@linux-m68k.org> writes:

Hello Geert and Thomas,

> Hi Thomas,
>
> On Wed, Jun 7, 2023 at 5:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 07.06.23 um 10:48 schrieb Geert Uytterhoeven:
>> > On Mon, Jun 5, 2023 at 4:48 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> >> --- a/drivers/video/fbdev/Kconfig
>> >> +++ b/drivers/video/fbdev/Kconfig
>> >> @@ -57,6 +57,15 @@ config FIRMWARE_EDID
>> >>            combination with certain motherboards and monitors are known to
>> >>            suffer from this problem.
>> >>
>> >> +config FB_DEVICE
>> >> +        bool "Provide legacy /dev/fb* device"
>> >
>> > Perhaps "default y if !DRM", although that does not help for a
>> > mixed drm/fbdev kernel build?
>>
>> We could simply set it to "default y".  But OTOH is it worth making it a
>> default? Distributions will set it to the value they need/want. The very
>> few people that build their own kernels to get certain fbdev drivers
>> will certainly be able to enable the option by hand as well.
>
> Defaulting to "n" (the default) means causing regressions when these
> few people use an existing defconfig.
>

Having "dfault y if !DRM" makes sense to me. I guess is a corner case but
at least it won't silently be disabled for users that only want fbdev as
Geert mentioned.

I wouldn't call it a regression though, because AFAIK the Kconfig options
are not a stable API ?

>> > Or reserve "FB" for real fbdev drivers, and introduce a new FB_CORE,
>> > to be selected by both FB and DRM_FBDEV_EMULATION?
>> > Then FB_DEVICE can depend on FB_CORE, and default to y if FB.

Funny that you mention because it's exactly what I attempted in the past:

https://lore.kernel.org/all/20210827100531.1578604-1-javierm@redhat.com/T/#u

>>
>> That wouldn't work. In Tumbleweed, we still have efifb and vesafb
>> enabled under certain conditions; merely for the kernel console. We'd
>> have to enable CONFIG_FB, which would bring back the device.
>
> "Default y" does not mean that you cannot disable FB_DEVICE, so
> you are not forced to bring back the device?
>

I think we can have both to make the kernel more configurable:

1) Allow to only disable fbdev user-space APIs (/dev/fb?, /proc/fb, etc),
   which is what the series is doing with the new FB_DEVICE config symbol.

2) Allow to disable all "native" fbdev drivers and only keep the DRM fbdev
   emulation layer. That's what my series attempted to do with the FB_CORE
   Kconfig symbol.

I believe that there are use cases for both, for example as Thomas' said
many distros are disabling all the fbdev drivers and their user-space only
requires DRM/KMS, so makes sense to not expose any fbdev uAPI at all.

But may be that other users want the opposite, they have an old user-space
that requires fbdev, but is running on newer hardware that only have a DRM
driver. So they will want DRM fbdev emulation but none fbdev driver at all.

That's why I think that FB_DEVICE and FB_CORE are complementary and we can
support any combination of the two, if you agree there are uses for either.
Thomas Zimmermann June 9, 2023, 7:09 a.m. UTC | #7
Hi Geert and Javier

Am 08.06.23 um 01:07 schrieb Javier Martinez Canillas:
> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> 
> Hello Geert and Thomas,
> 
>> Hi Thomas,
>>
>> On Wed, Jun 7, 2023 at 5:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>> Am 07.06.23 um 10:48 schrieb Geert Uytterhoeven:
>>>> On Mon, Jun 5, 2023 at 4:48 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>> --- a/drivers/video/fbdev/Kconfig
>>>>> +++ b/drivers/video/fbdev/Kconfig
>>>>> @@ -57,6 +57,15 @@ config FIRMWARE_EDID
>>>>>             combination with certain motherboards and monitors are known to
>>>>>             suffer from this problem.
>>>>>
>>>>> +config FB_DEVICE
>>>>> +        bool "Provide legacy /dev/fb* device"
>>>>
>>>> Perhaps "default y if !DRM", although that does not help for a
>>>> mixed drm/fbdev kernel build?
>>>
>>> We could simply set it to "default y".  But OTOH is it worth making it a
>>> default? Distributions will set it to the value they need/want. The very
>>> few people that build their own kernels to get certain fbdev drivers
>>> will certainly be able to enable the option by hand as well.
>>
>> Defaulting to "n" (the default) means causing regressions when these
>> few people use an existing defconfig.
>>
> 
> Having "dfault y if !DRM" makes sense to me. I guess is a corner case but
> at least it won't silently be disabled for users that only want fbdev as
> Geert mentioned.

IMHO the rational behind such conditionals are mostly what "we make up 
here in the discussion", but not something based on real-world feedback. 
So I'd strongly prefer a clear n or y setting here.

> 
> I wouldn't call it a regression though, because AFAIK the Kconfig options
> are not a stable API ?

IIRC in the past there have been concerns about changing Kconfig 
defaults. If we go with "default n", we'd apparently do something similar.

> 
>>>> Or reserve "FB" for real fbdev drivers, and introduce a new FB_CORE,
>>>> to be selected by both FB and DRM_FBDEV_EMULATION?
>>>> Then FB_DEVICE can depend on FB_CORE, and default to y if FB.
> 
> Funny that you mention because it's exactly what I attempted in the past:
> 
> https://lore.kernel.org/all/20210827100531.1578604-1-javierm@redhat.com/T/#u
> 
>>>
>>> That wouldn't work. In Tumbleweed, we still have efifb and vesafb
>>> enabled under certain conditions; merely for the kernel console. We'd
>>> have to enable CONFIG_FB, which would bring back the device.
>>
>> "Default y" does not mean that you cannot disable FB_DEVICE, so
>> you are not forced to bring back the device?
>>
> 
> I think we can have both to make the kernel more configurable:
> 
> 1) Allow to only disable fbdev user-space APIs (/dev/fb?, /proc/fb, etc),
>     which is what the series is doing with the new FB_DEVICE config symbol.
> 
> 2) Allow to disable all "native" fbdev drivers and only keep the DRM fbdev
>     emulation layer. That's what my series attempted to do with the FB_CORE
>     Kconfig symbol.
> 
> I believe that there are use cases for both, for example as Thomas' said
> many distros are disabling all the fbdev drivers and their user-space only
> requires DRM/KMS, so makes sense to not expose any fbdev uAPI at all.
> 
> But may be that other users want the opposite, they have an old user-space
> that requires fbdev, but is running on newer hardware that only have a DRM
> driver. So they will want DRM fbdev emulation but none fbdev driver at all.
> 
> That's why I think that FB_DEVICE and FB_CORE are complementary and we can
> support any combination of the two, if you agree there are uses for either.

I still don't understand the value of such an extra compile-time option? 
  Either you have fbdev userspace, then you want the device; or you 
don't then it's better to disable it entirely. I don't see much of a 
difference between DRM and fbdev drivers here.

I'd also question the argument that there's even fbdev userspace out 
there. It was never popular in it's heyday and definitely hasn't 
improved since then. Even the 3 people who still ask for fbdev support 
here only seem to care about the performance of the framebuffer console, 
but never about userspace.

So I'd like to propose a different solution: on top of the current 
patchset, let's make an fbdev module option that enables the device. If 
CONFIG_FB_DEVICE has been enabled, the option would switch the 
functionality on and off. A Kconfig option would set the default.  With 
such a setup, distributions can disable the fbdev device by default. 
And the few users with the odd system that has fbdev userspace can still 
enable the fbdev device at boot time.

Best regards
Thomas

>
Geert Uytterhoeven June 9, 2023, 7:29 a.m. UTC | #8
Hi Thomas,

On Fri, Jun 9, 2023 at 9:09 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 08.06.23 um 01:07 schrieb Javier Martinez Canillas:
> > Geert Uytterhoeven <geert@linux-m68k.org> writes:
> >> On Wed, Jun 7, 2023 at 5:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>> Am 07.06.23 um 10:48 schrieb Geert Uytterhoeven:
> >>>> On Mon, Jun 5, 2023 at 4:48 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>>>> --- a/drivers/video/fbdev/Kconfig
> >>>>> +++ b/drivers/video/fbdev/Kconfig
> >>>>> @@ -57,6 +57,15 @@ config FIRMWARE_EDID
> >>>>>             combination with certain motherboards and monitors are known to
> >>>>>             suffer from this problem.
> >>>>>
> >>>>> +config FB_DEVICE
> >>>>> +        bool "Provide legacy /dev/fb* device"
> >>>>
> >>>> Perhaps "default y if !DRM", although that does not help for a
> >>>> mixed drm/fbdev kernel build?
> >>>
> >>> We could simply set it to "default y".  But OTOH is it worth making it a
> >>> default? Distributions will set it to the value they need/want. The very
> >>> few people that build their own kernels to get certain fbdev drivers
> >>> will certainly be able to enable the option by hand as well.
> >>
> >> Defaulting to "n" (the default) means causing regressions when these
> >> few people use an existing defconfig.
> >>
> >
> > Having "dfault y if !DRM" makes sense to me. I guess is a corner case but
> > at least it won't silently be disabled for users that only want fbdev as
> > Geert mentioned.
>
> IMHO the rational behind such conditionals are mostly what "we make up
> here in the discussion", but not something based on real-world feedback.
> So I'd strongly prefer a clear n or y setting here.
>
> >
> > I wouldn't call it a regression though, because AFAIK the Kconfig options
> > are not a stable API ?
>
> IIRC in the past there have been concerns about changing Kconfig
> defaults. If we go with "default n", we'd apparently do something similar.
>
> >
> >>>> Or reserve "FB" for real fbdev drivers, and introduce a new FB_CORE,
> >>>> to be selected by both FB and DRM_FBDEV_EMULATION?
> >>>> Then FB_DEVICE can depend on FB_CORE, and default to y if FB.
> >
> > Funny that you mention because it's exactly what I attempted in the past:
> >
> > https://lore.kernel.org/all/20210827100531.1578604-1-javierm@redhat.com/T/#u
> >
> >>>
> >>> That wouldn't work. In Tumbleweed, we still have efifb and vesafb
> >>> enabled under certain conditions; merely for the kernel console. We'd
> >>> have to enable CONFIG_FB, which would bring back the device.
> >>
> >> "Default y" does not mean that you cannot disable FB_DEVICE, so
> >> you are not forced to bring back the device?
> >
> > I think we can have both to make the kernel more configurable:
> >
> > 1) Allow to only disable fbdev user-space APIs (/dev/fb?, /proc/fb, etc),
> >     which is what the series is doing with the new FB_DEVICE config symbol.
> >
> > 2) Allow to disable all "native" fbdev drivers and only keep the DRM fbdev
> >     emulation layer. That's what my series attempted to do with the FB_CORE
> >     Kconfig symbol.
> >
> > I believe that there are use cases for both, for example as Thomas' said
> > many distros are disabling all the fbdev drivers and their user-space only
> > requires DRM/KMS, so makes sense to not expose any fbdev uAPI at all.
> >
> > But may be that other users want the opposite, they have an old user-space
> > that requires fbdev, but is running on newer hardware that only have a DRM
> > driver. So they will want DRM fbdev emulation but none fbdev driver at all.
> >
> > That's why I think that FB_DEVICE and FB_CORE are complementary and we can
> > support any combination of the two, if you agree there are uses for either.
>
> I still don't understand the value of such an extra compile-time option?
>   Either you have fbdev userspace, then you want the device; or you
> don't then it's better to disable it entirely. I don't see much of a
> difference between DRM and fbdev drivers here.

If you have DRM and are running a Linux desktop, you are probably
using DRM userspace.
If you have fbdev, and are using graphics, you have no choice but
using an fbdev userspace.

So with FB_CORE, you can have default y if you have a real fbdev driver,
and default n if you have only DRM drivers.

> I'd also question the argument that there's even fbdev userspace out
> there. It was never popular in it's heyday and definitely hasn't
> improved since then. Even the 3 people who still ask for fbdev support

There's X.org, DirectFB, SDL, ...

What do you think low-end embedded devices with an out-of-tree[*]
fbdev driver are using?

[*] There's been a moratorium on new fbdev drivers for about a decade.

> here only seem to care about the performance of the framebuffer console,
> but never about userspace.

Unless you go for heavy graphics and 3D, a simple GUI with some
buttons and text requires less performance than scrolling a full-screen
graphical text console...

> So I'd like to propose a different solution: on top of the current
> patchset, let's make an fbdev module option that enables the device. If
> CONFIG_FB_DEVICE has been enabled, the option would switch the
> functionality on and off. A Kconfig option would set the default.  With
> such a setup, distributions can disable the fbdev device by default.
> And the few users with the odd system that has fbdev userspace can still
> enable the fbdev device at boot time.

Hmm... That makes it even more complicated...

Gr{oetje,eeting}s,

                        Geert
Thomas Zimmermann June 9, 2023, 8 a.m. UTC | #9
Hi

Am 09.06.23 um 09:29 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Fri, Jun 9, 2023 at 9:09 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 08.06.23 um 01:07 schrieb Javier Martinez Canillas:
>>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
>>>> On Wed, Jun 7, 2023 at 5:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>> Am 07.06.23 um 10:48 schrieb Geert Uytterhoeven:
>>>>>> On Mon, Jun 5, 2023 at 4:48 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>>>> --- a/drivers/video/fbdev/Kconfig
>>>>>>> +++ b/drivers/video/fbdev/Kconfig
>>>>>>> @@ -57,6 +57,15 @@ config FIRMWARE_EDID
>>>>>>>              combination with certain motherboards and monitors are known to
>>>>>>>              suffer from this problem.
>>>>>>>
>>>>>>> +config FB_DEVICE
>>>>>>> +        bool "Provide legacy /dev/fb* device"
>>>>>>
>>>>>> Perhaps "default y if !DRM", although that does not help for a
>>>>>> mixed drm/fbdev kernel build?
>>>>>
>>>>> We could simply set it to "default y".  But OTOH is it worth making it a
>>>>> default? Distributions will set it to the value they need/want. The very
>>>>> few people that build their own kernels to get certain fbdev drivers
>>>>> will certainly be able to enable the option by hand as well.
>>>>
>>>> Defaulting to "n" (the default) means causing regressions when these
>>>> few people use an existing defconfig.
>>>>
>>>
>>> Having "dfault y if !DRM" makes sense to me. I guess is a corner case but
>>> at least it won't silently be disabled for users that only want fbdev as
>>> Geert mentioned.
>>
>> IMHO the rational behind such conditionals are mostly what "we make up
>> here in the discussion", but not something based on real-world feedback.
>> So I'd strongly prefer a clear n or y setting here.
>>
>>>
>>> I wouldn't call it a regression though, because AFAIK the Kconfig options
>>> are not a stable API ?
>>
>> IIRC in the past there have been concerns about changing Kconfig
>> defaults. If we go with "default n", we'd apparently do something similar.
>>
>>>
>>>>>> Or reserve "FB" for real fbdev drivers, and introduce a new FB_CORE,
>>>>>> to be selected by both FB and DRM_FBDEV_EMULATION?
>>>>>> Then FB_DEVICE can depend on FB_CORE, and default to y if FB.
>>>
>>> Funny that you mention because it's exactly what I attempted in the past:
>>>
>>> https://lore.kernel.org/all/20210827100531.1578604-1-javierm@redhat.com/T/#u
>>>
>>>>>
>>>>> That wouldn't work. In Tumbleweed, we still have efifb and vesafb
>>>>> enabled under certain conditions; merely for the kernel console. We'd
>>>>> have to enable CONFIG_FB, which would bring back the device.
>>>>
>>>> "Default y" does not mean that you cannot disable FB_DEVICE, so
>>>> you are not forced to bring back the device?
>>>
>>> I think we can have both to make the kernel more configurable:
>>>
>>> 1) Allow to only disable fbdev user-space APIs (/dev/fb?, /proc/fb, etc),
>>>      which is what the series is doing with the new FB_DEVICE config symbol.
>>>
>>> 2) Allow to disable all "native" fbdev drivers and only keep the DRM fbdev
>>>      emulation layer. That's what my series attempted to do with the FB_CORE
>>>      Kconfig symbol.
>>>
>>> I believe that there are use cases for both, for example as Thomas' said
>>> many distros are disabling all the fbdev drivers and their user-space only
>>> requires DRM/KMS, so makes sense to not expose any fbdev uAPI at all.
>>>
>>> But may be that other users want the opposite, they have an old user-space
>>> that requires fbdev, but is running on newer hardware that only have a DRM
>>> driver. So they will want DRM fbdev emulation but none fbdev driver at all.
>>>
>>> That's why I think that FB_DEVICE and FB_CORE are complementary and we can
>>> support any combination of the two, if you agree there are uses for either.
>>
>> I still don't understand the value of such an extra compile-time option?
>>    Either you have fbdev userspace, then you want the device; or you
>> don't then it's better to disable it entirely. I don't see much of a
>> difference between DRM and fbdev drivers here.
> 
> If you have DRM and are running a Linux desktop, you are probably
> using DRM userspace.
> If you have fbdev, and are using graphics, you have no choice but
> using an fbdev userspace.
> 
> So with FB_CORE, you can have default y if you have a real fbdev driver,
> and default n if you have only DRM drivers.
> 
>> I'd also question the argument that there's even fbdev userspace out
>> there. It was never popular in it's heyday and definitely hasn't
>> improved since then. Even the 3 people who still ask for fbdev support
> 
> There's X.org, DirectFB, SDL, ...

None of these examples has a dependency on fbdev. They can freely switch 
backends and have moved to DRM. Anything program utilizing these 
examples has no dependency on fbdev either.

When I say "userspace" in this context, it's the one old program that 
supports nothing but fbdev. TBH I'm having problems to come up with 
examples.

> 
> What do you think low-end embedded devices with an out-of-tree[*]
> fbdev driver are using?

And those do not count either. IIRC Android used to be built on top of 
fbdev devices. I'm not sure if they have moved to DRM by now. But 
embedded uses dedicated kernels and kernel configs.  It's easy for them 
to set FB_DEVICE=y.  We're not going to take away the fbdev device entirely.

> 
> [*] There's been a moratorium on new fbdev drivers for about a decade.
> 
>> here only seem to care about the performance of the framebuffer console,
>> but never about userspace.
> 
> Unless you go for heavy graphics and 3D, a simple GUI with some
> buttons and text requires less performance than scrolling a full-screen
> graphical text console...
> 
>> So I'd like to propose a different solution: on top of the current
>> patchset, let's make an fbdev module option that enables the device. If
>> CONFIG_FB_DEVICE has been enabled, the option would switch the
>> functionality on and off. A Kconfig option would set the default.  With
>> such a setup, distributions can disable the fbdev device by default.
>> And the few users with the odd system that has fbdev userspace can still
>> enable the fbdev device at boot time.
> 
> Hmm... That makes it even more complicated...

No, that makes things a lot easier for distros. Everyone else (custom 
builds, embedded) is not affected by this change. Desktop distros are 
really the only affected party I see here. "We" (I'm at Suse) have to 
support all kinds of users with just a few generic offerings. And if I 
can disable the fbdev device by default and give the very few fbdev 
users a workaround, it's a very good tradeoff.

Best regards
Thomas

> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>
Geert Uytterhoeven June 9, 2023, 9:14 a.m. UTC | #10
Hi Thomas,

On Fri, Jun 9, 2023 at 10:00 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 09.06.23 um 09:29 schrieb Geert Uytterhoeven:
> > On Fri, Jun 9, 2023 at 9:09 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> Am 08.06.23 um 01:07 schrieb Javier Martinez Canillas:
> >>> Geert Uytterhoeven <geert@linux-m68k.org> writes:
> >>>> On Wed, Jun 7, 2023 at 5:15 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>>>> Am 07.06.23 um 10:48 schrieb Geert Uytterhoeven:
> >>>>>> On Mon, Jun 5, 2023 at 4:48 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>>>>>> --- a/drivers/video/fbdev/Kconfig
> >>>>>>> +++ b/drivers/video/fbdev/Kconfig
> >>>>>>> @@ -57,6 +57,15 @@ config FIRMWARE_EDID
> >>>>>>>              combination with certain motherboards and monitors are known to
> >>>>>>>              suffer from this problem.
> >>>>>>>
> >>>>>>> +config FB_DEVICE
> >>>>>>> +        bool "Provide legacy /dev/fb* device"
> >>>>>>
> >>>>>> Perhaps "default y if !DRM", although that does not help for a
> >>>>>> mixed drm/fbdev kernel build?
> >>>>>
> >>>>> We could simply set it to "default y".  But OTOH is it worth making it a
> >>>>> default? Distributions will set it to the value they need/want. The very
> >>>>> few people that build their own kernels to get certain fbdev drivers
> >>>>> will certainly be able to enable the option by hand as well.
> >>>>
> >>>> Defaulting to "n" (the default) means causing regressions when these
> >>>> few people use an existing defconfig.
> >>>>
> >>>
> >>> Having "dfault y if !DRM" makes sense to me. I guess is a corner case but
> >>> at least it won't silently be disabled for users that only want fbdev as
> >>> Geert mentioned.
> >>
> >> IMHO the rational behind such conditionals are mostly what "we make up
> >> here in the discussion", but not something based on real-world feedback.
> >> So I'd strongly prefer a clear n or y setting here.
> >>
> >>>
> >>> I wouldn't call it a regression though, because AFAIK the Kconfig options
> >>> are not a stable API ?
> >>
> >> IIRC in the past there have been concerns about changing Kconfig
> >> defaults. If we go with "default n", we'd apparently do something similar.
> >>
> >>>
> >>>>>> Or reserve "FB" for real fbdev drivers, and introduce a new FB_CORE,
> >>>>>> to be selected by both FB and DRM_FBDEV_EMULATION?
> >>>>>> Then FB_DEVICE can depend on FB_CORE, and default to y if FB.
> >>>
> >>> Funny that you mention because it's exactly what I attempted in the past:
> >>>
> >>> https://lore.kernel.org/all/20210827100531.1578604-1-javierm@redhat.com/T/#u
> >>>
> >>>>>
> >>>>> That wouldn't work. In Tumbleweed, we still have efifb and vesafb
> >>>>> enabled under certain conditions; merely for the kernel console. We'd
> >>>>> have to enable CONFIG_FB, which would bring back the device.
> >>>>
> >>>> "Default y" does not mean that you cannot disable FB_DEVICE, so
> >>>> you are not forced to bring back the device?
> >>>
> >>> I think we can have both to make the kernel more configurable:
> >>>
> >>> 1) Allow to only disable fbdev user-space APIs (/dev/fb?, /proc/fb, etc),
> >>>      which is what the series is doing with the new FB_DEVICE config symbol.
> >>>
> >>> 2) Allow to disable all "native" fbdev drivers and only keep the DRM fbdev
> >>>      emulation layer. That's what my series attempted to do with the FB_CORE
> >>>      Kconfig symbol.
> >>>
> >>> I believe that there are use cases for both, for example as Thomas' said
> >>> many distros are disabling all the fbdev drivers and their user-space only
> >>> requires DRM/KMS, so makes sense to not expose any fbdev uAPI at all.
> >>>
> >>> But may be that other users want the opposite, they have an old user-space
> >>> that requires fbdev, but is running on newer hardware that only have a DRM
> >>> driver. So they will want DRM fbdev emulation but none fbdev driver at all.
> >>>
> >>> That's why I think that FB_DEVICE and FB_CORE are complementary and we can
> >>> support any combination of the two, if you agree there are uses for either.
> >>
> >> I still don't understand the value of such an extra compile-time option?
> >>    Either you have fbdev userspace, then you want the device; or you
> >> don't then it's better to disable it entirely. I don't see much of a
> >> difference between DRM and fbdev drivers here.
> >
> > If you have DRM and are running a Linux desktop, you are probably
> > using DRM userspace.
> > If you have fbdev, and are using graphics, you have no choice but
> > using an fbdev userspace.
> >
> > So with FB_CORE, you can have default y if you have a real fbdev driver,
> > and default n if you have only DRM drivers.
> >
> >> I'd also question the argument that there's even fbdev userspace out
> >> there. It was never popular in it's heyday and definitely hasn't
> >> improved since then. Even the 3 people who still ask for fbdev support
> >
> > There's X.org, DirectFB, SDL, ...
>
> None of these examples has a dependency on fbdev. They can freely switch
> backends and have moved to DRM. Anything program utilizing these
> examples has no dependency on fbdev either.

Indeed, these examples do not depend on fbdev, it's the other way
around.  How does it help if your userspace now also supports DRM,
but you are using an fbdev graphics driver?  The DRM drivers do not
cover all graphics hardware yet.

> When I say "userspace" in this context, it's the one old program that
> supports nothing but fbdev. TBH I'm having problems to come up with
> examples.

Even if you cannot find such an old program, that doesn't matter much,
if you are using an fbdev graphics driver...

> > What do you think low-end embedded devices with an out-of-tree[*]
> > fbdev driver are using?
>
> And those do not count either. IIRC Android used to be built on top of
> fbdev devices. I'm not sure if they have moved to DRM by now. But
> embedded uses dedicated kernels and kernel configs.  It's easy for them
> to set FB_DEVICE=y.  We're not going to take away the fbdev device entirely.

The point is that we do not suddenly disable functionality that users
may depend on. While "make oldconfig" will show users the new
FB_DEVICE question, (and hopefully they'll notice), "make olddefconfig"
and "make <foo>_defconfig" won't, possibly causing regressions.
Without a suitable default, you should IMHO at least update all
defconfigs that enable any fbdev drivers.

I guess you do remember the fall-out from f611b1e7624ccdbd ("drm:
Avoid circular dependencies for CONFIG_FB"), after which lots of defconfigs
had to gain CONFIG_FB=y?

Gr{oetje,eeting}s,

                        Geert
Javier Martinez Canillas June 9, 2023, 9:59 a.m. UTC | #11
Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Hi
>

[...]
 
>>> I'd also question the argument that there's even fbdev userspace out
>>> there. It was never popular in it's heyday and definitely hasn't
>>> improved since then. Even the 3 people who still ask for fbdev support
>> 
>> There's X.org, DirectFB, SDL, ...
>
> None of these examples has a dependency on fbdev. They can freely switch 
> backends and have moved to DRM. Anything program utilizing these 
> examples has no dependency on fbdev either.
>
> When I say "userspace" in this context, it's the one old program that 
> supports nothing but fbdev. TBH I'm having problems to come up with 
> examples.
>

I personally have two real world examples that can give to you :)

1) I've a IoT device at home that has a bunch of sensors (temperatury,
   humidity, etc) and a SSD1306 display panel to report that. It just
   has small fbdev program to print that info. I could probably port
   to KMS but didn't feel like it. Found a fbdev program that I could
   modify and got the job done.

2) I built a portable retro console for my kids, that uses a ST7735R
   LCD panel. The software I use is https://www.retroarch.com/ which
   uses fbdev by default (I believe that supports a KMS mode but out
   of the box it works with fbdev and that's better tested by them.
   
So even when I'm not interested and don't want to enable any of the
fbdev drivers, I want to use the ssd130x and st7735r DRM drivers and
the DRM fbdev emulation layer.

In other words, there's real use cases for supporting fbdev programs
with DRM drivers. Now, I agree with this patch-set and probably will
disable the user-space fbdev interface in Fedora, but on my embedded
projects I will probably keep it enabled.

That's why I think that we should support the following combinations:

* fbdev drivers + DRM fbdev emulation + fbdev user-space
* only DRM drivers without fbdev emulation nor fbdev user-space (your series)
* only DRM fbdev emulation + fbdev user-space enabled (FB_CORE)
Geert Uytterhoeven June 9, 2023, 10:10 a.m. UTC | #12
Hi Javier,

On Fri, Jun 9, 2023 at 11:59 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> >>> I'd also question the argument that there's even fbdev userspace out
> >>> there. It was never popular in it's heyday and definitely hasn't
> >>> improved since then. Even the 3 people who still ask for fbdev support
> >>
> >> There's X.org, DirectFB, SDL, ...
> >
> > None of these examples has a dependency on fbdev. They can freely switch
> > backends and have moved to DRM. Anything program utilizing these
> > examples has no dependency on fbdev either.
> >
> > When I say "userspace" in this context, it's the one old program that
> > supports nothing but fbdev. TBH I'm having problems to come up with
> > examples.
> >
>
> I personally have two real world examples that can give to you :)
>
> 1) I've a IoT device at home that has a bunch of sensors (temperatury,
>    humidity, etc) and a SSD1306 display panel to report that. It just
>    has small fbdev program to print that info. I could probably port
>    to KMS but didn't feel like it. Found a fbdev program that I could
>    modify and got the job done.
>
> 2) I built a portable retro console for my kids, that uses a ST7735R
>    LCD panel. The software I use is https://www.retroarch.com/ which
>    uses fbdev by default (I believe that supports a KMS mode but out
>    of the box it works with fbdev and that's better tested by them.
>
> So even when I'm not interested and don't want to enable any of the
> fbdev drivers, I want to use the ssd130x and st7735r DRM drivers and
> the DRM fbdev emulation layer.
>
> In other words, there's real use cases for supporting fbdev programs
> with DRM drivers. Now, I agree with this patch-set and probably will
> disable the user-space fbdev interface in Fedora, but on my embedded
> projects I will probably keep it enabled.
>
> That's why I think that we should support the following combinations:
>
> * fbdev drivers + DRM fbdev emulation + fbdev user-space

"fbdev drivers + fbdev user-space", I assume?

> * only DRM drivers without fbdev emulation nor fbdev user-space (your series)

Thomas' series includes fbdev emulation here, for the text console.

> * only DRM fbdev emulation + fbdev user-space enabled (FB_CORE)

Gr{oetje,eeting}s,

                        Geert
Javier Martinez Canillas June 9, 2023, 10:24 a.m. UTC | #13
Geert Uytterhoeven <geert@linux-m68k.org> writes:


>> * fbdev drivers + DRM fbdev emulation + fbdev user-space
>
> "fbdev drivers + fbdev user-space", I assume?
>

Right, I meant to also include "only fbdev drivers + fbdev user-space"
but forgot :)

>> * only DRM drivers without fbdev emulation nor fbdev user-space (your series)
>
> Thomas' series includes fbdev emulation here, for the text console.
>

Yes, I meant fbdev emulation for user-space. Probably should had included
fbcon in the options too...

But what I tried to say is that we need all combination of DRM drivers,
fbdev drivers, DRM emulation for fbcon and emulation for fbdev user-space.

And I think that Thomas' series + a FB_CORE as you suggested and done in
my old series should be enough to have that.

--
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat
Thomas Zimmermann June 9, 2023, 11:04 a.m. UTC | #14
Hi

Am 09.06.23 um 11:14 schrieb Geert Uytterhoeven:
[...]
> 
>>> What do you think low-end embedded devices with an out-of-tree[*]
>>> fbdev driver are using?
>>
>> And those do not count either. IIRC Android used to be built on top of
>> fbdev devices. I'm not sure if they have moved to DRM by now. But
>> embedded uses dedicated kernels and kernel configs.  It's easy for them
>> to set FB_DEVICE=y.  We're not going to take away the fbdev device entirely.
> 
> The point is that we do not suddenly disable functionality that users
> may depend on. While "make oldconfig" will show users the new
> FB_DEVICE question, (and hopefully they'll notice), "make olddefconfig"
> and "make <foo>_defconfig" won't, possibly causing regressions.
> Without a suitable default, you should IMHO at least update all
> defconfigs that enable any fbdev drivers.

Didn't I already say that we should make it "default y" if that's 
preferable in practice?

Best regards
Thomas

> 
> I guess you do remember the fall-out from f611b1e7624ccdbd ("drm:
> Avoid circular dependencies for CONFIG_FB"), after which lots of defconfigs
> had to gain CONFIG_FB=y?
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>
Geert Uytterhoeven June 9, 2023, 11:22 a.m. UTC | #15
Hi Thomas,

On Fri, Jun 9, 2023 at 1:04 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 09.06.23 um 11:14 schrieb Geert Uytterhoeven:
> [...]
> >
> >>> What do you think low-end embedded devices with an out-of-tree[*]
> >>> fbdev driver are using?
> >>
> >> And those do not count either. IIRC Android used to be built on top of
> >> fbdev devices. I'm not sure if they have moved to DRM by now. But
> >> embedded uses dedicated kernels and kernel configs.  It's easy for them
> >> to set FB_DEVICE=y.  We're not going to take away the fbdev device entirely.
> >
> > The point is that we do not suddenly disable functionality that users
> > may depend on. While "make oldconfig" will show users the new
> > FB_DEVICE question, (and hopefully they'll notice), "make olddefconfig"
> > and "make <foo>_defconfig" won't, possibly causing regressions.
> > Without a suitable default, you should IMHO at least update all
> > defconfigs that enable any fbdev drivers.
>
> Didn't I already say that we should make it "default y" if that's
> preferable in practice?

OK, sorry, I seem to have missed that part.

Gr{oetje,eeting}s,

                        Geert
Javier Martinez Canillas June 9, 2023, 11:27 a.m. UTC | #16
Thomas Zimmermann <tzimmermann@suse.de> writes:

[...]

>> 
>> So with FB_CORE, you can have default y if you have a real fbdev driver,
>> and default n if you have only DRM drivers.
>> 

All this discussion about FB_CORE is unrelated to your series though and
that is covered by enabling CONFIG_FB_DEVICE. I think that there's value
on a FB_CORE option to allow disabling all the fbdev drivers with a single
option but still keep DRM_FBDEV_EMULATION enabled.

But I can repost my old series on top of this patch-set once it lands.
Sam Ravnborg June 11, 2023, 4:37 p.m. UTC | #17
Hi Thomas,

On Mon, Jun 05, 2023 at 04:48:12PM +0200, Thomas Zimmermann wrote:
> Add Kconfig option CONFIG_FB_DEVICE and make the virtual fbdev
> device optional. If the new option has not been selected, fbdev
> does not create a files in devfs or sysfs.
s/ a//
> 
> Most modern Linux systems run a DRM-based graphics stack that uses
> the kernel's framebuffer console, but has otherwise deprecated fbdev
> support. Yet fbdev userspace interfaces are still present.
> 
> The option makes it possible to use the fbdev subsystem as console
> implementation without support for userspace. This closes potential
> entry points to manipulate kernel or I/O memory via framebuffers. It
> also prevents the execution of driver code via ioctl or sysfs, both
> of which might allow malicious software to exploit bugs in the fbdev
> code.
> 
> A small number of fbdev drivers require struct fbinfo.dev to be
> initialized, usually for the support of sysfs interface. Make these
> drivers depend on FB_DEVICE. They can later be fixed if necessary.
Should that be a TODO in gpu/todo.rst?
Otherwise the amount of people knowing about this
is very close to 1.
As an alternative add a TODO to each Kconfig file.

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/staging/fbtft/Kconfig            |  1 +
>  drivers/video/fbdev/Kconfig              | 12 +++++++++
>  drivers/video/fbdev/core/Makefile        |  7 +++---
>  drivers/video/fbdev/core/fb_internal.h   | 32 ++++++++++++++++++++++++
>  drivers/video/fbdev/omap2/omapfb/Kconfig |  2 +-
>  include/linux/fb.h                       |  2 ++
>  6 files changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
> index 4d29e8c1014e..5dda3c65a38e 100644
> --- a/drivers/staging/fbtft/Kconfig
> +++ b/drivers/staging/fbtft/Kconfig
> @@ -2,6 +2,7 @@
>  menuconfig FB_TFT
>  	tristate "Support for small TFT LCD display modules"
>  	depends on FB && SPI
> +	depends on FB_DEVICE
>  	depends on GPIOLIB || COMPILE_TEST
>  	select FB_SYS_FILLRECT
>  	select FB_SYS_COPYAREA
> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
> index 6df9bd09454a..48d9a14f889c 100644
> --- a/drivers/video/fbdev/Kconfig
> +++ b/drivers/video/fbdev/Kconfig
> @@ -57,6 +57,15 @@ config FIRMWARE_EDID
>  	  combination with certain motherboards and monitors are known to
>  	  suffer from this problem.
>  
> +config FB_DEVICE
> +        bool "Provide legacy /dev/fb* device"
> +        depends on FB
> +        help
> +	  Say Y here if you want the legacy /dev/fb* device file. It's
> +	  only required if you have userspace programs that depend on
> +	  fbdev for graphics output. This does not effect the framebuffer
> +	  console.
tabs to spaces to indent the above correct.

> +
>  config FB_DDC
>  	tristate
>  	depends on FB
> @@ -1545,6 +1554,7 @@ config FB_3DFX_I2C
>  config FB_VOODOO1
>  	tristate "3Dfx Voodoo Graphics (sst1) support"
>  	depends on FB && PCI
> +	depends on FB_DEVICE
>  	select FB_CFB_FILLRECT
>  	select FB_CFB_COPYAREA
>  	select FB_CFB_IMAGEBLIT
> @@ -1862,6 +1872,7 @@ config FB_SH_MOBILE_LCDC
>  	tristate "SuperH Mobile LCDC framebuffer support"
>  	depends on FB && HAVE_CLK && HAS_IOMEM
>  	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> +	depends on FB_DEVICE
>  	select FB_SYS_FILLRECT
>  	select FB_SYS_COPYAREA
>  	select FB_SYS_IMAGEBLIT
> @@ -1930,6 +1941,7 @@ config FB_SMSCUFX
>  config FB_UDL
>  	tristate "Displaylink USB Framebuffer support"
>  	depends on FB && USB
> +	depends on FB_DEVICE
>  	select FB_MODE_HELPERS
>  	select FB_SYS_FILLRECT
>  	select FB_SYS_COPYAREA
> diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
> index 125d24f50c36..d5e8772620f8 100644
> --- a/drivers/video/fbdev/core/Makefile
> +++ b/drivers/video/fbdev/core/Makefile
> @@ -2,12 +2,13 @@
>  obj-$(CONFIG_FB_NOTIFY)           += fb_notify.o
>  obj-$(CONFIG_FB)                  += fb.o
>  fb-y                              := fb_backlight.o \
> -                                     fb_devfs.o \
>                                       fb_info.o \
> -                                     fb_procfs.o \
> -                                     fbmem.o fbmon.o fbcmap.o fbsysfs.o \
> +                                     fbmem.o fbmon.o fbcmap.o \
>                                       modedb.o fbcvt.o fb_cmdline.o fb_io_fops.o
>  fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
> +fb-$(CONFIG_FB_DEVICE)            += fb_devfs.o \
> +                                     fb_procfs.o \
> +                                     fbsysfs.o
Maybe change this to one line to avoid '\'?

>  
>  ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE),y)
>  fb-y				  += fbcon.o bitblit.o softcursor.o
> diff --git a/drivers/video/fbdev/core/fb_internal.h b/drivers/video/fbdev/core/fb_internal.h
> index 0b43c0cd5096..b8a28466db79 100644
> --- a/drivers/video/fbdev/core/fb_internal.h
> +++ b/drivers/video/fbdev/core/fb_internal.h
> @@ -3,12 +3,22 @@
>  #ifndef _FB_INTERNAL_H
>  #define _FB_INTERNAL_H
>  
> +#include <linux/device.h>
>  #include <linux/fb.h>
>  #include <linux/mutex.h>
>  
>  /* fb_devfs.c */
> +#if defined(CONFIG_FB_DEVICE)
>  int fb_register_chrdev(void);
>  void fb_unregister_chrdev(void);
> +#else
> +static inline int fb_register_chrdev(void)
> +{
> +	return 0;
> +}
> +static inline void fb_unregister_chrdev(void)
> +{ }
> +#endif
>  
>  /* fbmem.c */
>  extern struct class *fb_class;
> @@ -19,11 +29,33 @@ struct fb_info *get_fb_info(unsigned int idx);
>  void put_fb_info(struct fb_info *fb_info);
>  
>  /* fb_procfs.c */
> +#if defined(CONFIG_FB_DEVICE)
>  int fb_init_procfs(void);
>  void fb_cleanup_procfs(void);
> +#else
> +static inline int fb_init_procfs(void)
> +{
> +	return 0;
> +}
> +static inline void fb_cleanup_procfs(void)
> +{ }
> +#endif
>  
>  /* fbsysfs.c */
> +#if defined(CONFIG_FB_DEVICE)
>  int fb_device_create(struct fb_info *fb_info);
>  void fb_device_destroy(struct fb_info *fb_info);
> +#else
> +static inline int fb_device_create(struct fb_info *fb_info)
> +{
> +	get_device(fb_info->device); // as in device_add()
> +
> +	return 0;
> +}
> +static inline void fb_device_destroy(struct fb_info *fb_info)
> +{
> +	put_device(fb_info->device); // as in device_del()
> +}
> +#endif
I do not see why fb_device_{create,destroy} needs to call
{get,put}_device - and it is not explained.
A short explanation in the commit maybe?

With my comments addressed:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Note: I do not engage in the thread about the best Kconfig
solution - I trust the involved people will find a good solution.

	Sam

>  
>  #endif
> diff --git a/drivers/video/fbdev/omap2/omapfb/Kconfig b/drivers/video/fbdev/omap2/omapfb/Kconfig
> index 69f9cb03507e..21069fdb7cc2 100644
> --- a/drivers/video/fbdev/omap2/omapfb/Kconfig
> +++ b/drivers/video/fbdev/omap2/omapfb/Kconfig
> @@ -5,9 +5,9 @@ config OMAP2_VRFB
>  menuconfig FB_OMAP2
>  	tristate "OMAP2+ frame buffer support"
>  	depends on FB
> +	depends on FB_DEVICE
>  	depends on DRM_OMAP = n
>  	depends on GPIOLIB
> -
>  	select FB_OMAP2_DSS
>  	select OMAP2_VRFB if ARCH_OMAP2 || ARCH_OMAP3
>  	select FB_CFB_FILLRECT
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index 541a0e3ce21f..40ed1028160c 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -481,7 +481,9 @@ struct fb_info {
>  
>  	const struct fb_ops *fbops;
>  	struct device *device;		/* This is the parent */
> +#if defined(CONFIG_FB_DEVICE)
>  	struct device *dev;		/* This is this fb device */
> +#endif
>  	int class_flag;                    /* private sysfs flags */
>  #ifdef CONFIG_FB_TILEBLITTING
>  	struct fb_tile_ops *tileops;    /* Tile Blitting */
> -- 
> 2.40.1
Thomas Zimmermann June 12, 2023, 6:47 a.m. UTC | #18
Hi Sam

Am 11.06.23 um 18:37 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Mon, Jun 05, 2023 at 04:48:12PM +0200, Thomas Zimmermann wrote:
>> Add Kconfig option CONFIG_FB_DEVICE and make the virtual fbdev
>> device optional. If the new option has not been selected, fbdev
>> does not create a files in devfs or sysfs.
> s/ a//
>>
>> Most modern Linux systems run a DRM-based graphics stack that uses
>> the kernel's framebuffer console, but has otherwise deprecated fbdev
>> support. Yet fbdev userspace interfaces are still present.
>>
>> The option makes it possible to use the fbdev subsystem as console
>> implementation without support for userspace. This closes potential
>> entry points to manipulate kernel or I/O memory via framebuffers. It
>> also prevents the execution of driver code via ioctl or sysfs, both
>> of which might allow malicious software to exploit bugs in the fbdev
>> code.
>>
>> A small number of fbdev drivers require struct fbinfo.dev to be
>> initialized, usually for the support of sysfs interface. Make these
>> drivers depend on FB_DEVICE. They can later be fixed if necessary.
> Should that be a TODO in gpu/todo.rst?
> Otherwise the amount of people knowing about this
> is very close to 1.

Ha! Yes, I'll add a TODO item. Good idea.

Best regards
Thomas

> As an alternative add a TODO to each Kconfig file.
> 
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/staging/fbtft/Kconfig            |  1 +
>>   drivers/video/fbdev/Kconfig              | 12 +++++++++
>>   drivers/video/fbdev/core/Makefile        |  7 +++---
>>   drivers/video/fbdev/core/fb_internal.h   | 32 ++++++++++++++++++++++++
>>   drivers/video/fbdev/omap2/omapfb/Kconfig |  2 +-
>>   include/linux/fb.h                       |  2 ++
>>   6 files changed, 52 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
>> index 4d29e8c1014e..5dda3c65a38e 100644
>> --- a/drivers/staging/fbtft/Kconfig
>> +++ b/drivers/staging/fbtft/Kconfig
>> @@ -2,6 +2,7 @@
>>   menuconfig FB_TFT
>>   	tristate "Support for small TFT LCD display modules"
>>   	depends on FB && SPI
>> +	depends on FB_DEVICE
>>   	depends on GPIOLIB || COMPILE_TEST
>>   	select FB_SYS_FILLRECT
>>   	select FB_SYS_COPYAREA
>> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
>> index 6df9bd09454a..48d9a14f889c 100644
>> --- a/drivers/video/fbdev/Kconfig
>> +++ b/drivers/video/fbdev/Kconfig
>> @@ -57,6 +57,15 @@ config FIRMWARE_EDID
>>   	  combination with certain motherboards and monitors are known to
>>   	  suffer from this problem.
>>   
>> +config FB_DEVICE
>> +        bool "Provide legacy /dev/fb* device"
>> +        depends on FB
>> +        help
>> +	  Say Y here if you want the legacy /dev/fb* device file. It's
>> +	  only required if you have userspace programs that depend on
>> +	  fbdev for graphics output. This does not effect the framebuffer
>> +	  console.
> tabs to spaces to indent the above correct.
> 
>> +
>>   config FB_DDC
>>   	tristate
>>   	depends on FB
>> @@ -1545,6 +1554,7 @@ config FB_3DFX_I2C
>>   config FB_VOODOO1
>>   	tristate "3Dfx Voodoo Graphics (sst1) support"
>>   	depends on FB && PCI
>> +	depends on FB_DEVICE
>>   	select FB_CFB_FILLRECT
>>   	select FB_CFB_COPYAREA
>>   	select FB_CFB_IMAGEBLIT
>> @@ -1862,6 +1872,7 @@ config FB_SH_MOBILE_LCDC
>>   	tristate "SuperH Mobile LCDC framebuffer support"
>>   	depends on FB && HAVE_CLK && HAS_IOMEM
>>   	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
>> +	depends on FB_DEVICE
>>   	select FB_SYS_FILLRECT
>>   	select FB_SYS_COPYAREA
>>   	select FB_SYS_IMAGEBLIT
>> @@ -1930,6 +1941,7 @@ config FB_SMSCUFX
>>   config FB_UDL
>>   	tristate "Displaylink USB Framebuffer support"
>>   	depends on FB && USB
>> +	depends on FB_DEVICE
>>   	select FB_MODE_HELPERS
>>   	select FB_SYS_FILLRECT
>>   	select FB_SYS_COPYAREA
>> diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
>> index 125d24f50c36..d5e8772620f8 100644
>> --- a/drivers/video/fbdev/core/Makefile
>> +++ b/drivers/video/fbdev/core/Makefile
>> @@ -2,12 +2,13 @@
>>   obj-$(CONFIG_FB_NOTIFY)           += fb_notify.o
>>   obj-$(CONFIG_FB)                  += fb.o
>>   fb-y                              := fb_backlight.o \
>> -                                     fb_devfs.o \
>>                                        fb_info.o \
>> -                                     fb_procfs.o \
>> -                                     fbmem.o fbmon.o fbcmap.o fbsysfs.o \
>> +                                     fbmem.o fbmon.o fbcmap.o \
>>                                        modedb.o fbcvt.o fb_cmdline.o fb_io_fops.o
>>   fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
>> +fb-$(CONFIG_FB_DEVICE)            += fb_devfs.o \
>> +                                     fb_procfs.o \
>> +                                     fbsysfs.o
> Maybe change this to one line to avoid '\'?
> 
>>   
>>   ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE),y)
>>   fb-y				  += fbcon.o bitblit.o softcursor.o
>> diff --git a/drivers/video/fbdev/core/fb_internal.h b/drivers/video/fbdev/core/fb_internal.h
>> index 0b43c0cd5096..b8a28466db79 100644
>> --- a/drivers/video/fbdev/core/fb_internal.h
>> +++ b/drivers/video/fbdev/core/fb_internal.h
>> @@ -3,12 +3,22 @@
>>   #ifndef _FB_INTERNAL_H
>>   #define _FB_INTERNAL_H
>>   
>> +#include <linux/device.h>
>>   #include <linux/fb.h>
>>   #include <linux/mutex.h>
>>   
>>   /* fb_devfs.c */
>> +#if defined(CONFIG_FB_DEVICE)
>>   int fb_register_chrdev(void);
>>   void fb_unregister_chrdev(void);
>> +#else
>> +static inline int fb_register_chrdev(void)
>> +{
>> +	return 0;
>> +}
>> +static inline void fb_unregister_chrdev(void)
>> +{ }
>> +#endif
>>   
>>   /* fbmem.c */
>>   extern struct class *fb_class;
>> @@ -19,11 +29,33 @@ struct fb_info *get_fb_info(unsigned int idx);
>>   void put_fb_info(struct fb_info *fb_info);
>>   
>>   /* fb_procfs.c */
>> +#if defined(CONFIG_FB_DEVICE)
>>   int fb_init_procfs(void);
>>   void fb_cleanup_procfs(void);
>> +#else
>> +static inline int fb_init_procfs(void)
>> +{
>> +	return 0;
>> +}
>> +static inline void fb_cleanup_procfs(void)
>> +{ }
>> +#endif
>>   
>>   /* fbsysfs.c */
>> +#if defined(CONFIG_FB_DEVICE)
>>   int fb_device_create(struct fb_info *fb_info);
>>   void fb_device_destroy(struct fb_info *fb_info);
>> +#else
>> +static inline int fb_device_create(struct fb_info *fb_info)
>> +{
>> +	get_device(fb_info->device); // as in device_add()
>> +
>> +	return 0;
>> +}
>> +static inline void fb_device_destroy(struct fb_info *fb_info)
>> +{
>> +	put_device(fb_info->device); // as in device_del()
>> +}
>> +#endif
> I do not see why fb_device_{create,destroy} needs to call
> {get,put}_device - and it is not explained.
> A short explanation in the commit maybe?
> 
> With my comments addressed:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> Note: I do not engage in the thread about the best Kconfig
> solution - I trust the involved people will find a good solution.
> 
> 	Sam
> 
>>   
>>   #endif
>> diff --git a/drivers/video/fbdev/omap2/omapfb/Kconfig b/drivers/video/fbdev/omap2/omapfb/Kconfig
>> index 69f9cb03507e..21069fdb7cc2 100644
>> --- a/drivers/video/fbdev/omap2/omapfb/Kconfig
>> +++ b/drivers/video/fbdev/omap2/omapfb/Kconfig
>> @@ -5,9 +5,9 @@ config OMAP2_VRFB
>>   menuconfig FB_OMAP2
>>   	tristate "OMAP2+ frame buffer support"
>>   	depends on FB
>> +	depends on FB_DEVICE
>>   	depends on DRM_OMAP = n
>>   	depends on GPIOLIB
>> -
>>   	select FB_OMAP2_DSS
>>   	select OMAP2_VRFB if ARCH_OMAP2 || ARCH_OMAP3
>>   	select FB_CFB_FILLRECT
>> diff --git a/include/linux/fb.h b/include/linux/fb.h
>> index 541a0e3ce21f..40ed1028160c 100644
>> --- a/include/linux/fb.h
>> +++ b/include/linux/fb.h
>> @@ -481,7 +481,9 @@ struct fb_info {
>>   
>>   	const struct fb_ops *fbops;
>>   	struct device *device;		/* This is the parent */
>> +#if defined(CONFIG_FB_DEVICE)
>>   	struct device *dev;		/* This is this fb device */
>> +#endif
>>   	int class_flag;                    /* private sysfs flags */
>>   #ifdef CONFIG_FB_TILEBLITTING
>>   	struct fb_tile_ops *tileops;    /* Tile Blitting */
>> -- 
>> 2.40.1
Thomas Zimmermann June 12, 2023, 7 a.m. UTC | #19
Hi

Am 11.06.23 um 18:37 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Mon, Jun 05, 2023 at 04:48:12PM +0200, Thomas Zimmermann wrote:
>> Add Kconfig option CONFIG_FB_DEVICE and make the virtual fbdev
>> device optional. If the new option has not been selected, fbdev
>> does not create a files in devfs or sysfs.
> s/ a//
>>
>> Most modern Linux systems run a DRM-based graphics stack that uses
>> the kernel's framebuffer console, but has otherwise deprecated fbdev
>> support. Yet fbdev userspace interfaces are still present.
>>
>> The option makes it possible to use the fbdev subsystem as console
>> implementation without support for userspace. This closes potential
>> entry points to manipulate kernel or I/O memory via framebuffers. It
>> also prevents the execution of driver code via ioctl or sysfs, both
>> of which might allow malicious software to exploit bugs in the fbdev
>> code.
>>
>> A small number of fbdev drivers require struct fbinfo.dev to be
>> initialized, usually for the support of sysfs interface. Make these
>> drivers depend on FB_DEVICE. They can later be fixed if necessary.
> Should that be a TODO in gpu/todo.rst?
> Otherwise the amount of people knowing about this
> is very close to 1.
> As an alternative add a TODO to each Kconfig file.
> 
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/staging/fbtft/Kconfig            |  1 +
>>   drivers/video/fbdev/Kconfig              | 12 +++++++++
>>   drivers/video/fbdev/core/Makefile        |  7 +++---
>>   drivers/video/fbdev/core/fb_internal.h   | 32 ++++++++++++++++++++++++
>>   drivers/video/fbdev/omap2/omapfb/Kconfig |  2 +-
>>   include/linux/fb.h                       |  2 ++
>>   6 files changed, 52 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
>> index 4d29e8c1014e..5dda3c65a38e 100644
>> --- a/drivers/staging/fbtft/Kconfig
>> +++ b/drivers/staging/fbtft/Kconfig
>> @@ -2,6 +2,7 @@
>>   menuconfig FB_TFT
>>   	tristate "Support for small TFT LCD display modules"
>>   	depends on FB && SPI
>> +	depends on FB_DEVICE
>>   	depends on GPIOLIB || COMPILE_TEST
>>   	select FB_SYS_FILLRECT
>>   	select FB_SYS_COPYAREA
>> diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
>> index 6df9bd09454a..48d9a14f889c 100644
>> --- a/drivers/video/fbdev/Kconfig
>> +++ b/drivers/video/fbdev/Kconfig
>> @@ -57,6 +57,15 @@ config FIRMWARE_EDID
>>   	  combination with certain motherboards and monitors are known to
>>   	  suffer from this problem.
>>   
>> +config FB_DEVICE
>> +        bool "Provide legacy /dev/fb* device"
>> +        depends on FB
>> +        help
>> +	  Say Y here if you want the legacy /dev/fb* device file. It's
>> +	  only required if you have userspace programs that depend on
>> +	  fbdev for graphics output. This does not effect the framebuffer
>> +	  console.
> tabs to spaces to indent the above correct.
> 
>> +
>>   config FB_DDC
>>   	tristate
>>   	depends on FB
>> @@ -1545,6 +1554,7 @@ config FB_3DFX_I2C
>>   config FB_VOODOO1
>>   	tristate "3Dfx Voodoo Graphics (sst1) support"
>>   	depends on FB && PCI
>> +	depends on FB_DEVICE
>>   	select FB_CFB_FILLRECT
>>   	select FB_CFB_COPYAREA
>>   	select FB_CFB_IMAGEBLIT
>> @@ -1862,6 +1872,7 @@ config FB_SH_MOBILE_LCDC
>>   	tristate "SuperH Mobile LCDC framebuffer support"
>>   	depends on FB && HAVE_CLK && HAS_IOMEM
>>   	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
>> +	depends on FB_DEVICE
>>   	select FB_SYS_FILLRECT
>>   	select FB_SYS_COPYAREA
>>   	select FB_SYS_IMAGEBLIT
>> @@ -1930,6 +1941,7 @@ config FB_SMSCUFX
>>   config FB_UDL
>>   	tristate "Displaylink USB Framebuffer support"
>>   	depends on FB && USB
>> +	depends on FB_DEVICE
>>   	select FB_MODE_HELPERS
>>   	select FB_SYS_FILLRECT
>>   	select FB_SYS_COPYAREA
>> diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
>> index 125d24f50c36..d5e8772620f8 100644
>> --- a/drivers/video/fbdev/core/Makefile
>> +++ b/drivers/video/fbdev/core/Makefile
>> @@ -2,12 +2,13 @@
>>   obj-$(CONFIG_FB_NOTIFY)           += fb_notify.o
>>   obj-$(CONFIG_FB)                  += fb.o
>>   fb-y                              := fb_backlight.o \
>> -                                     fb_devfs.o \
>>                                        fb_info.o \
>> -                                     fb_procfs.o \
>> -                                     fbmem.o fbmon.o fbcmap.o fbsysfs.o \
>> +                                     fbmem.o fbmon.o fbcmap.o \
>>                                        modedb.o fbcvt.o fb_cmdline.o fb_io_fops.o
>>   fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
>> +fb-$(CONFIG_FB_DEVICE)            += fb_devfs.o \
>> +                                     fb_procfs.o \
>> +                                     fbsysfs.o
> Maybe change this to one line to avoid '\'?
> 
>>   
>>   ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE),y)
>>   fb-y				  += fbcon.o bitblit.o softcursor.o
>> diff --git a/drivers/video/fbdev/core/fb_internal.h b/drivers/video/fbdev/core/fb_internal.h
>> index 0b43c0cd5096..b8a28466db79 100644
>> --- a/drivers/video/fbdev/core/fb_internal.h
>> +++ b/drivers/video/fbdev/core/fb_internal.h
>> @@ -3,12 +3,22 @@
>>   #ifndef _FB_INTERNAL_H
>>   #define _FB_INTERNAL_H
>>   
>> +#include <linux/device.h>
>>   #include <linux/fb.h>
>>   #include <linux/mutex.h>
>>   
>>   /* fb_devfs.c */
>> +#if defined(CONFIG_FB_DEVICE)
>>   int fb_register_chrdev(void);
>>   void fb_unregister_chrdev(void);
>> +#else
>> +static inline int fb_register_chrdev(void)
>> +{
>> +	return 0;
>> +}
>> +static inline void fb_unregister_chrdev(void)
>> +{ }
>> +#endif
>>   
>>   /* fbmem.c */
>>   extern struct class *fb_class;
>> @@ -19,11 +29,33 @@ struct fb_info *get_fb_info(unsigned int idx);
>>   void put_fb_info(struct fb_info *fb_info);
>>   
>>   /* fb_procfs.c */
>> +#if defined(CONFIG_FB_DEVICE)
>>   int fb_init_procfs(void);
>>   void fb_cleanup_procfs(void);
>> +#else
>> +static inline int fb_init_procfs(void)
>> +{
>> +	return 0;
>> +}
>> +static inline void fb_cleanup_procfs(void)
>> +{ }
>> +#endif
>>   
>>   /* fbsysfs.c */
>> +#if defined(CONFIG_FB_DEVICE)
>>   int fb_device_create(struct fb_info *fb_info);
>>   void fb_device_destroy(struct fb_info *fb_info);
>> +#else
>> +static inline int fb_device_create(struct fb_info *fb_info)
>> +{
>> +	get_device(fb_info->device); // as in device_add()
>> +
>> +	return 0;
>> +}
>> +static inline void fb_device_destroy(struct fb_info *fb_info)
>> +{
>> +	put_device(fb_info->device); // as in device_del()
>> +}
>> +#endif
> I do not see why fb_device_{create,destroy} needs to call
> {get,put}_device - and it is not explained.
> A short explanation in the commit maybe?

Ok, I'll do that.

This get_device() is normally done within device_create() from within 
register_framebuffer(). The put is within unregister_frambuffer(). 
Without, someone could attempt to unplug the Linux hardware device and 
our pointer would go stale. So we have to hold the reference within fbdev.

Best regards
Thomas

> 
> With my comments addressed:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> Note: I do not engage in the thread about the best Kconfig
> solution - I trust the involved people will find a good solution.
> 
> 	Sam
> 
>>   
>>   #endif
>> diff --git a/drivers/video/fbdev/omap2/omapfb/Kconfig b/drivers/video/fbdev/omap2/omapfb/Kconfig
>> index 69f9cb03507e..21069fdb7cc2 100644
>> --- a/drivers/video/fbdev/omap2/omapfb/Kconfig
>> +++ b/drivers/video/fbdev/omap2/omapfb/Kconfig
>> @@ -5,9 +5,9 @@ config OMAP2_VRFB
>>   menuconfig FB_OMAP2
>>   	tristate "OMAP2+ frame buffer support"
>>   	depends on FB
>> +	depends on FB_DEVICE
>>   	depends on DRM_OMAP = n
>>   	depends on GPIOLIB
>> -
>>   	select FB_OMAP2_DSS
>>   	select OMAP2_VRFB if ARCH_OMAP2 || ARCH_OMAP3
>>   	select FB_CFB_FILLRECT
>> diff --git a/include/linux/fb.h b/include/linux/fb.h
>> index 541a0e3ce21f..40ed1028160c 100644
>> --- a/include/linux/fb.h
>> +++ b/include/linux/fb.h
>> @@ -481,7 +481,9 @@ struct fb_info {
>>   
>>   	const struct fb_ops *fbops;
>>   	struct device *device;		/* This is the parent */
>> +#if defined(CONFIG_FB_DEVICE)
>>   	struct device *dev;		/* This is this fb device */
>> +#endif
>>   	int class_flag;                    /* private sysfs flags */
>>   #ifdef CONFIG_FB_TILEBLITTING
>>   	struct fb_tile_ops *tileops;    /* Tile Blitting */
>> -- 
>> 2.40.1
diff mbox series

Patch

diff --git a/drivers/staging/fbtft/Kconfig b/drivers/staging/fbtft/Kconfig
index 4d29e8c1014e..5dda3c65a38e 100644
--- a/drivers/staging/fbtft/Kconfig
+++ b/drivers/staging/fbtft/Kconfig
@@ -2,6 +2,7 @@ 
 menuconfig FB_TFT
 	tristate "Support for small TFT LCD display modules"
 	depends on FB && SPI
+	depends on FB_DEVICE
 	depends on GPIOLIB || COMPILE_TEST
 	select FB_SYS_FILLRECT
 	select FB_SYS_COPYAREA
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index 6df9bd09454a..48d9a14f889c 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -57,6 +57,15 @@  config FIRMWARE_EDID
 	  combination with certain motherboards and monitors are known to
 	  suffer from this problem.
 
+config FB_DEVICE
+        bool "Provide legacy /dev/fb* device"
+        depends on FB
+        help
+	  Say Y here if you want the legacy /dev/fb* device file. It's
+	  only required if you have userspace programs that depend on
+	  fbdev for graphics output. This does not effect the framebuffer
+	  console.
+
 config FB_DDC
 	tristate
 	depends on FB
@@ -1545,6 +1554,7 @@  config FB_3DFX_I2C
 config FB_VOODOO1
 	tristate "3Dfx Voodoo Graphics (sst1) support"
 	depends on FB && PCI
+	depends on FB_DEVICE
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
@@ -1862,6 +1872,7 @@  config FB_SH_MOBILE_LCDC
 	tristate "SuperH Mobile LCDC framebuffer support"
 	depends on FB && HAVE_CLK && HAS_IOMEM
 	depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
+	depends on FB_DEVICE
 	select FB_SYS_FILLRECT
 	select FB_SYS_COPYAREA
 	select FB_SYS_IMAGEBLIT
@@ -1930,6 +1941,7 @@  config FB_SMSCUFX
 config FB_UDL
 	tristate "Displaylink USB Framebuffer support"
 	depends on FB && USB
+	depends on FB_DEVICE
 	select FB_MODE_HELPERS
 	select FB_SYS_FILLRECT
 	select FB_SYS_COPYAREA
diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
index 125d24f50c36..d5e8772620f8 100644
--- a/drivers/video/fbdev/core/Makefile
+++ b/drivers/video/fbdev/core/Makefile
@@ -2,12 +2,13 @@ 
 obj-$(CONFIG_FB_NOTIFY)           += fb_notify.o
 obj-$(CONFIG_FB)                  += fb.o
 fb-y                              := fb_backlight.o \
-                                     fb_devfs.o \
                                      fb_info.o \
-                                     fb_procfs.o \
-                                     fbmem.o fbmon.o fbcmap.o fbsysfs.o \
+                                     fbmem.o fbmon.o fbcmap.o \
                                      modedb.o fbcvt.o fb_cmdline.o fb_io_fops.o
 fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
+fb-$(CONFIG_FB_DEVICE)            += fb_devfs.o \
+                                     fb_procfs.o \
+                                     fbsysfs.o
 
 ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE),y)
 fb-y				  += fbcon.o bitblit.o softcursor.o
diff --git a/drivers/video/fbdev/core/fb_internal.h b/drivers/video/fbdev/core/fb_internal.h
index 0b43c0cd5096..b8a28466db79 100644
--- a/drivers/video/fbdev/core/fb_internal.h
+++ b/drivers/video/fbdev/core/fb_internal.h
@@ -3,12 +3,22 @@ 
 #ifndef _FB_INTERNAL_H
 #define _FB_INTERNAL_H
 
+#include <linux/device.h>
 #include <linux/fb.h>
 #include <linux/mutex.h>
 
 /* fb_devfs.c */
+#if defined(CONFIG_FB_DEVICE)
 int fb_register_chrdev(void);
 void fb_unregister_chrdev(void);
+#else
+static inline int fb_register_chrdev(void)
+{
+	return 0;
+}
+static inline void fb_unregister_chrdev(void)
+{ }
+#endif
 
 /* fbmem.c */
 extern struct class *fb_class;
@@ -19,11 +29,33 @@  struct fb_info *get_fb_info(unsigned int idx);
 void put_fb_info(struct fb_info *fb_info);
 
 /* fb_procfs.c */
+#if defined(CONFIG_FB_DEVICE)
 int fb_init_procfs(void);
 void fb_cleanup_procfs(void);
+#else
+static inline int fb_init_procfs(void)
+{
+	return 0;
+}
+static inline void fb_cleanup_procfs(void)
+{ }
+#endif
 
 /* fbsysfs.c */
+#if defined(CONFIG_FB_DEVICE)
 int fb_device_create(struct fb_info *fb_info);
 void fb_device_destroy(struct fb_info *fb_info);
+#else
+static inline int fb_device_create(struct fb_info *fb_info)
+{
+	get_device(fb_info->device); // as in device_add()
+
+	return 0;
+}
+static inline void fb_device_destroy(struct fb_info *fb_info)
+{
+	put_device(fb_info->device); // as in device_del()
+}
+#endif
 
 #endif
diff --git a/drivers/video/fbdev/omap2/omapfb/Kconfig b/drivers/video/fbdev/omap2/omapfb/Kconfig
index 69f9cb03507e..21069fdb7cc2 100644
--- a/drivers/video/fbdev/omap2/omapfb/Kconfig
+++ b/drivers/video/fbdev/omap2/omapfb/Kconfig
@@ -5,9 +5,9 @@  config OMAP2_VRFB
 menuconfig FB_OMAP2
 	tristate "OMAP2+ frame buffer support"
 	depends on FB
+	depends on FB_DEVICE
 	depends on DRM_OMAP = n
 	depends on GPIOLIB
-
 	select FB_OMAP2_DSS
 	select OMAP2_VRFB if ARCH_OMAP2 || ARCH_OMAP3
 	select FB_CFB_FILLRECT
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 541a0e3ce21f..40ed1028160c 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -481,7 +481,9 @@  struct fb_info {
 
 	const struct fb_ops *fbops;
 	struct device *device;		/* This is the parent */
+#if defined(CONFIG_FB_DEVICE)
 	struct device *dev;		/* This is this fb device */
+#endif
 	int class_flag;                    /* private sysfs flags */
 #ifdef CONFIG_FB_TILEBLITTING
 	struct fb_tile_ops *tileops;    /* Tile Blitting */