[PATCH 30/30] fbdev: Make support for userspace interfaces configurable
Thomas Zimmermann
tzimmermann at suse.de
Mon Jun 12 07:00:05 UTC 2023
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 at 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 at 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
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230612/e6d15397/attachment.sig>
More information about the dri-devel
mailing list