| Message ID | 20230314121216.413434-8-schnelle@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Tue, 14 Mar 2023, Niklas Schnelle <schnelle@linux.ibm.com> wrote: > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends > not being declared. We thus need to add HAS_IOPORT as dependency for > those drivers using them. In the bochs driver there is optional MMIO > support detected at runtime, warn if this isn't taken when > HAS_IOPORT is not defined. Not that I care a whole lot, but there should really only be one warning or even failure to probe at bochs_hw_init() for !bochs->mmio && !IS_ENABLED(CONFIG_HAS_IOPORT), not warnings all over the place. Moreover, the config macro is CONFIG_HAS_IOPORT instead of HAS_IOPORT that you check for below. BR, Jani. > There is also a direct and hard coded use in cirrus.c which according to > the comment is only necessary during resume. Let's just skip this as > for example s390 which doesn't have I/O port support also doesen't > support suspend/resume. > > Co-developed-by: Arnd Bergmann <arnd@kernel.org> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> > --- > drivers/gpu/drm/qxl/Kconfig | 1 + > drivers/gpu/drm/tiny/bochs.c | 19 +++++++++++++++++++ > drivers/gpu/drm/tiny/cirrus.c | 2 ++ > 3 files changed, 22 insertions(+) > > diff --git a/drivers/gpu/drm/qxl/Kconfig b/drivers/gpu/drm/qxl/Kconfig > index ca3f51c2a8fe..d0e0d440c8d9 100644 > --- a/drivers/gpu/drm/qxl/Kconfig > +++ b/drivers/gpu/drm/qxl/Kconfig > @@ -2,6 +2,7 @@ > config DRM_QXL > tristate "QXL virtual GPU" > depends on DRM && PCI && MMU > + depends on HAS_IOPORT > select DRM_KMS_HELPER > select DRM_TTM > select DRM_TTM_HELPER > diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c > index 024346054c70..da4a5d53b003 100644 > --- a/drivers/gpu/drm/tiny/bochs.c > +++ b/drivers/gpu/drm/tiny/bochs.c > @@ -2,6 +2,7 @@ > > #include <linux/module.h> > #include <linux/pci.h> > +#include <asm/bug.h> > > #include <drm/drm_aperture.h> > #include <drm/drm_atomic_helper.h> > @@ -105,7 +106,11 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val) > > writeb(val, bochs->mmio + offset); > } else { > +#ifdef HAS_IOPORT > outb(val, ioport); > +#else > + WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT"); > +#endif > } > } > > @@ -119,7 +124,12 @@ static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport) > > return readb(bochs->mmio + offset); > } else { > +#ifdef HAS_IOPORT > return inb(ioport); > +#else > + WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT"); > + return 0xff; > +#endif > } > } > > @@ -132,8 +142,13 @@ static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg) > > ret = readw(bochs->mmio + offset); > } else { > +#ifdef HAS_IOPORT > outw(reg, VBE_DISPI_IOPORT_INDEX); > ret = inw(VBE_DISPI_IOPORT_DATA); > +#else > + WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT"); > + ret = 0xffff; > +#endif > } > return ret; > } > @@ -145,8 +160,12 @@ static void bochs_dispi_write(struct bochs_device *bochs, u16 reg, u16 val) > > writew(val, bochs->mmio + offset); > } else { > +#ifdef HAS_IOPORT > outw(reg, VBE_DISPI_IOPORT_INDEX); > outw(val, VBE_DISPI_IOPORT_DATA); > +#else > + WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT"); > +#endif > } > } > > diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c > index accfa52e78c5..9da89732c5ac 100644 > --- a/drivers/gpu/drm/tiny/cirrus.c > +++ b/drivers/gpu/drm/tiny/cirrus.c > @@ -308,8 +308,10 @@ static int cirrus_mode_set(struct cirrus_device *cirrus, > > cirrus_set_start_address(cirrus, 0); > > +#ifdef CONFIG_HAS_IOPORT > /* Unblank (needed on S3 resume, vgabios doesn't do it then) */ > outb(0x20, 0x3c0); > +#endif > > drm_dev_exit(idx); > return 0;
On Wed, 2023-03-15 at 13:54 +0200, Jani Nikula wrote: > On Tue, 14 Mar 2023, Niklas Schnelle <schnelle@linux.ibm.com> wrote: > > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends > > not being declared. We thus need to add HAS_IOPORT as dependency for > > those drivers using them. In the bochs driver there is optional MMIO > > support detected at runtime, warn if this isn't taken when > > HAS_IOPORT is not defined. > > Not that I care a whole lot, but there should really only be one warning > or even failure to probe at bochs_hw_init() for !bochs->mmio && > !IS_ENABLED(CONFIG_HAS_IOPORT), not warnings all over the place. > > Moreover, the config macro is CONFIG_HAS_IOPORT instead of HAS_IOPORT > that you check for below. > > BR, > Jani. > Good Find! Yes the #ifdefs need CONFIG_HAS_IOPORT I even had it correctly for cirrus in the same patch. For v4 I remove the separate warnings and instead went with the below: @@ -229,6 +242,10 @@ static int bochs_hw_init(struct drm_device *dev) return -ENOMEM; } } else { + if (!IS_ENABLED(CONFIG_HAS_IOPORT)) { + DRM_ERROR("I/O ports are not supported\n"); + return -EIO; + } ioaddr = VBE_DISPI_IOPORT_INDEX; iosize = 2; if (!request_region(ioaddr, iosize, "bochs-drm")) { Thanks, Niklas
diff --git a/drivers/gpu/drm/qxl/Kconfig b/drivers/gpu/drm/qxl/Kconfig index ca3f51c2a8fe..d0e0d440c8d9 100644 --- a/drivers/gpu/drm/qxl/Kconfig +++ b/drivers/gpu/drm/qxl/Kconfig @@ -2,6 +2,7 @@ config DRM_QXL tristate "QXL virtual GPU" depends on DRM && PCI && MMU + depends on HAS_IOPORT select DRM_KMS_HELPER select DRM_TTM select DRM_TTM_HELPER diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c index 024346054c70..da4a5d53b003 100644 --- a/drivers/gpu/drm/tiny/bochs.c +++ b/drivers/gpu/drm/tiny/bochs.c @@ -2,6 +2,7 @@ #include <linux/module.h> #include <linux/pci.h> +#include <asm/bug.h> #include <drm/drm_aperture.h> #include <drm/drm_atomic_helper.h> @@ -105,7 +106,11 @@ static void bochs_vga_writeb(struct bochs_device *bochs, u16 ioport, u8 val) writeb(val, bochs->mmio + offset); } else { +#ifdef HAS_IOPORT outb(val, ioport); +#else + WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT"); +#endif } } @@ -119,7 +124,12 @@ static u8 bochs_vga_readb(struct bochs_device *bochs, u16 ioport) return readb(bochs->mmio + offset); } else { +#ifdef HAS_IOPORT return inb(ioport); +#else + WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT"); + return 0xff; +#endif } } @@ -132,8 +142,13 @@ static u16 bochs_dispi_read(struct bochs_device *bochs, u16 reg) ret = readw(bochs->mmio + offset); } else { +#ifdef HAS_IOPORT outw(reg, VBE_DISPI_IOPORT_INDEX); ret = inw(VBE_DISPI_IOPORT_DATA); +#else + WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT"); + ret = 0xffff; +#endif } return ret; } @@ -145,8 +160,12 @@ static void bochs_dispi_write(struct bochs_device *bochs, u16 reg, u16 val) writew(val, bochs->mmio + offset); } else { +#ifdef HAS_IOPORT outw(reg, VBE_DISPI_IOPORT_INDEX); outw(val, VBE_DISPI_IOPORT_DATA); +#else + WARN_ONCE(1, "Non-MMIO bochs device needs HAS_IOPORT"); +#endif } } diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c index accfa52e78c5..9da89732c5ac 100644 --- a/drivers/gpu/drm/tiny/cirrus.c +++ b/drivers/gpu/drm/tiny/cirrus.c @@ -308,8 +308,10 @@ static int cirrus_mode_set(struct cirrus_device *cirrus, cirrus_set_start_address(cirrus, 0); +#ifdef CONFIG_HAS_IOPORT /* Unblank (needed on S3 resume, vgabios doesn't do it then) */ outb(0x20, 0x3c0); +#endif drm_dev_exit(idx); return 0;
In a future patch HAS_IOPORT=n will result in inb()/outb() and friends not being declared. We thus need to add HAS_IOPORT as dependency for those drivers using them. In the bochs driver there is optional MMIO support detected at runtime, warn if this isn't taken when HAS_IOPORT is not defined. There is also a direct and hard coded use in cirrus.c which according to the comment is only necessary during resume. Let's just skip this as for example s390 which doesn't have I/O port support also doesen't support suspend/resume. Co-developed-by: Arnd Bergmann <arnd@kernel.org> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> --- drivers/gpu/drm/qxl/Kconfig | 1 + drivers/gpu/drm/tiny/bochs.c | 19 +++++++++++++++++++ drivers/gpu/drm/tiny/cirrus.c | 2 ++ 3 files changed, 22 insertions(+)