| Message ID | 20230605144812.15241-31-tzimmermann@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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 >
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
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.
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 >
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
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 >
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
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)
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
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
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 >
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
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.
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
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
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 */
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(-)