| Message ID | 20220429135108.2781579-7-schnelle@linux.ibm.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | None | expand |
> Hello Niklas, > > On 29.04.22 15:50, Niklas Schnelle wrote: > > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends > > not being declared. We thus need to add this dependency and ifdef > > sections of code using inb()/outb() as alternative access methods. > > > > Co-developed-by: Arnd Bergmann <arnd@kernel.org> > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> > > [snip] > > > diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c > > index 9c924a1440a9..2d2ae37153ba 100644 > > --- a/drivers/char/tpm/tpm_infineon.c > > +++ b/drivers/char/tpm/tpm_infineon.c > > @@ -51,34 +51,40 @@ static struct tpm_inf_dev tpm_dev; > > > > static inline void tpm_data_out(unsigned char data, unsigned char offset) > > { > > +#ifdef CONFIG_HAS_IOPORT > > if (tpm_dev.iotype == TPM_INF_IO_PORT) > > outb(data, tpm_dev.data_regs + offset); > > else > > +#endif > > This looks ugly. Can't you declare inb/outb anyway and skip the definition, > so you can use IS_ENABLED() here instead? > > You can mark the declarations with __compiletime_error("some message"), so > if an IS_ENABLED() reference is not removed at compile time, you get some > readable error message instead of a link error. > > Cheers, > Ahmad I didn't know about __compiletime_error() that certainly sounds interesting even when using a normal #ifdef. That said either with the function not being declared or this __compiletime_error() mechanism I would think that using IS_ENABLED() relies on compiler optimizations not to compile in the missing/error function call, right? I'm not sure if that is something we should do.
Hello Niklas, On 29.04.22 16:23, Niklas Schnelle wrote: >> Hello Niklas, >> >> On 29.04.22 15:50, Niklas Schnelle wrote: >>> In a future patch HAS_IOPORT=n will result in inb()/outb() and friends >>> not being declared. We thus need to add this dependency and ifdef >>> sections of code using inb()/outb() as alternative access methods. >>> >>> Co-developed-by: Arnd Bergmann <arnd@kernel.org> >>> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> >> >> [snip] >> >>> diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c >>> index 9c924a1440a9..2d2ae37153ba 100644 >>> --- a/drivers/char/tpm/tpm_infineon.c >>> +++ b/drivers/char/tpm/tpm_infineon.c >>> @@ -51,34 +51,40 @@ static struct tpm_inf_dev tpm_dev; >>> >>> static inline void tpm_data_out(unsigned char data, unsigned char offset) >>> { >>> +#ifdef CONFIG_HAS_IOPORT >>> if (tpm_dev.iotype == TPM_INF_IO_PORT) >>> outb(data, tpm_dev.data_regs + offset); >>> else >>> +#endif >> >> This looks ugly. Can't you declare inb/outb anyway and skip the definition, >> so you can use IS_ENABLED() here instead? >> >> You can mark the declarations with __compiletime_error("some message"), so >> if an IS_ENABLED() reference is not removed at compile time, you get some >> readable error message instead of a link error. >> >> Cheers, >> Ahmad > > I didn't know about __compiletime_error() that certainly sounds > interesting even when using a normal #ifdef. > > That said either with the function not being declared or this > __compiletime_error() mechanism I would think that using IS_ENABLED() > relies on compiler optimizations not to compile in the missing/error > function call, right? I'm not sure if that is something we should do. Yes, it assumes your compiler is able to discard the body of an if (0), which we already assume, otherwise it wouldn't make sense for any existing code to use __compiletime_error(). To me this sounds much cleaner than #ifdefs in the midst of functions, which are a detriment to maintainability. Cheers, Ahmad > >
On Fri, 2022-04-29 at 16:33 +0200, Ahmad Fatoum wrote: > Hello Niklas, > > On 29.04.22 16:23, Niklas Schnelle wrote: > > > Hello Niklas, > > > > > > On 29.04.22 15:50, Niklas Schnelle wrote: > > > > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends > > > > not being declared. We thus need to add this dependency and ifdef > > > > sections of code using inb()/outb() as alternative access methods. > > > > > > > > Co-developed-by: Arnd Bergmann <arnd@kernel.org> > > > > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> > > > > > > [snip] > > > > > > > diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c > > > > index 9c924a1440a9..2d2ae37153ba 100644 > > > > --- a/drivers/char/tpm/tpm_infineon.c > > > > +++ b/drivers/char/tpm/tpm_infineon.c > > > > @@ -51,34 +51,40 @@ static struct tpm_inf_dev tpm_dev; > > > > > > > > static inline void tpm_data_out(unsigned char data, unsigned char offset) > > > > { > > > > +#ifdef CONFIG_HAS_IOPORT > > > > if (tpm_dev.iotype == TPM_INF_IO_PORT) > > > > outb(data, tpm_dev.data_regs + offset); > > > > else > > > > +#endif > > > > > > This looks ugly. Can't you declare inb/outb anyway and skip the definition, > > > so you can use IS_ENABLED() here instead? > > > > > > You can mark the declarations with __compiletime_error("some message"), so > > > if an IS_ENABLED() reference is not removed at compile time, you get some > > > readable error message instead of a link error. > > > > > > Cheers, > > > Ahmad > > > > I didn't know about __compiletime_error() that certainly sounds > > interesting even when using a normal #ifdef. > > > > That said either with the function not being declared or this > > __compiletime_error() mechanism I would think that using IS_ENABLED() > > relies on compiler optimizations not to compile in the missing/error > > function call, right? I'm not sure if that is something we should do. > > Yes, it assumes your compiler is able to discard the body of an if (0), > which we already assume, otherwise it wouldn't make sense for any existing > code to use __compiletime_error(). > > To me this sounds much cleaner than #ifdefs in the midst of functions, > which are a detriment to maintainability. > > Cheers, > Ahmad > Ok, makes sense. I'll look into using __compiletime_error() and IS_ENABLED().
diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig index 55f48375e3fe..463b82935e78 100644 --- a/drivers/char/Kconfig +++ b/drivers/char/Kconfig @@ -33,6 +33,7 @@ config TTY_PRINTK_LEVEL config PRINTER tristate "Parallel printer support" depends on PARPORT + depends on HAS_IOPORT help If you intend to attach a printer to the parallel port of your Linux box (as opposed to using a serial printer; if the connector at the @@ -346,7 +347,7 @@ config NVRAM config DEVPORT bool "/dev/port character device" - depends on ISA || PCI + depends on HAS_IOPORT default y help Say Y here if you want to support the /dev/port device. The /dev/port diff --git a/drivers/char/ipmi/Makefile b/drivers/char/ipmi/Makefile index 7ce790efad92..439bed4feb3a 100644 --- a/drivers/char/ipmi/Makefile +++ b/drivers/char/ipmi/Makefile @@ -5,13 +5,10 @@ ipmi_si-y := ipmi_si_intf.o ipmi_kcs_sm.o ipmi_smic_sm.o ipmi_bt_sm.o \ ipmi_si_hotmod.o ipmi_si_hardcode.o ipmi_si_platform.o \ - ipmi_si_port_io.o ipmi_si_mem_io.o -ifdef CONFIG_PCI -ipmi_si-y += ipmi_si_pci.o -endif -ifdef CONFIG_PARISC -ipmi_si-y += ipmi_si_parisc.o -endif + ipmi_si_mem_io.o +ipmi_si-$(CONFIG_HAS_IOPORT) += ipmi_si_port_io.o +ipmi_si-$(CONFIG_PCI) += ipmi_si_pci.o +ipmi_si-$(CONFIG_PARISC) += ipmi_si_parisc.o obj-$(CONFIG_IPMI_HANDLER) += ipmi_msghandler.o obj-$(CONFIG_IPMI_DEVICE_INTERFACE) += ipmi_devintf.o diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c index 64dedb3ef8ec..e8094b4007de 100644 --- a/drivers/char/ipmi/ipmi_si_intf.c +++ b/drivers/char/ipmi/ipmi_si_intf.c @@ -1881,7 +1881,8 @@ int ipmi_si_add_smi(struct si_sm_io *io) } if (!io->io_setup) { - if (io->addr_space == IPMI_IO_ADDR_SPACE) { + if (IS_ENABLED(CONFIG_HAS_IOPORT) && + io->addr_space == IPMI_IO_ADDR_SPACE) { io->io_setup = ipmi_si_port_setup; } else if (io->addr_space == IPMI_MEM_ADDR_SPACE) { io->io_setup = ipmi_si_mem_setup; diff --git a/drivers/char/ipmi/ipmi_si_pci.c b/drivers/char/ipmi/ipmi_si_pci.c index 74fa2055868b..b83d55685b22 100644 --- a/drivers/char/ipmi/ipmi_si_pci.c +++ b/drivers/char/ipmi/ipmi_si_pci.c @@ -97,6 +97,9 @@ static int ipmi_pci_probe(struct pci_dev *pdev, } if (pci_resource_flags(pdev, 0) & IORESOURCE_IO) { + if (!IS_ENABLED(CONFIG_HAS_IOPORT)) + return -ENXIO; + io.addr_space = IPMI_IO_ADDR_SPACE; io.io_setup = ipmi_si_port_setup; } else { diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig index 4a5516406c22..4bc52ed08015 100644 --- a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig @@ -137,6 +137,7 @@ config TCG_NSC config TCG_ATMEL tristate "Atmel TPM Interface" depends on PPC64 || HAS_IOPORT_MAP + depends on HAS_IOPORT help If you have a TPM security chip from Atmel say Yes and it will be accessible from within Linux. To compile this driver diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c index 9c924a1440a9..2d2ae37153ba 100644 --- a/drivers/char/tpm/tpm_infineon.c +++ b/drivers/char/tpm/tpm_infineon.c @@ -51,34 +51,40 @@ static struct tpm_inf_dev tpm_dev; static inline void tpm_data_out(unsigned char data, unsigned char offset) { +#ifdef CONFIG_HAS_IOPORT if (tpm_dev.iotype == TPM_INF_IO_PORT) outb(data, tpm_dev.data_regs + offset); else +#endif writeb(data, tpm_dev.mem_base + tpm_dev.data_regs + offset); } static inline unsigned char tpm_data_in(unsigned char offset) { +#ifdef CONFIG_HAS_IOPORT if (tpm_dev.iotype == TPM_INF_IO_PORT) return inb(tpm_dev.data_regs + offset); - else - return readb(tpm_dev.mem_base + tpm_dev.data_regs + offset); +#endif + return readb(tpm_dev.mem_base + tpm_dev.data_regs + offset); } static inline void tpm_config_out(unsigned char data, unsigned char offset) { +#ifdef CONFIG_HAS_IOPORT if (tpm_dev.iotype == TPM_INF_IO_PORT) outb(data, tpm_dev.config_port + offset); else +#endif writeb(data, tpm_dev.mem_base + tpm_dev.index_off + offset); } static inline unsigned char tpm_config_in(unsigned char offset) { +#ifdef CONFIG_HAS_IOPORT if (tpm_dev.iotype == TPM_INF_IO_PORT) return inb(tpm_dev.config_port + offset); - else - return readb(tpm_dev.mem_base + tpm_dev.index_off + offset); +#endif + return readb(tpm_dev.mem_base + tpm_dev.index_off + offset); } /* TPM header definitions */ diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index dc56b976d816..1efb58dc1b41 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -879,11 +879,6 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value) clkrun_val &= ~LPC_CLKRUN_EN; iowrite32(clkrun_val, data->ilb_base_addr + LPC_CNTRL_OFFSET); - /* - * Write any random value on port 0x80 which is on LPC, to make - * sure LPC clock is running before sending any TPM command. - */ - outb(0xCC, 0x80); } else { data->clkrun_enabled--; if (data->clkrun_enabled) @@ -894,13 +889,15 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value) /* Enable LPC CLKRUN# */ clkrun_val |= LPC_CLKRUN_EN; iowrite32(clkrun_val, data->ilb_base_addr + LPC_CNTRL_OFFSET); - - /* - * Write any random value on port 0x80 which is on LPC, to make - * sure LPC clock is running before sending any TPM command. - */ - outb(0xCC, 0x80); } + +#ifdef CONFIG_HAS_IOPORT + /* + * Write any random value on port 0x80 which is on LPC, to make + * sure LPC clock is running before sending any TPM command. + */ + outb(0xCC, 0x80); +#endif } static const struct tpm_class_ops tpm_tis = {
In a future patch HAS_IOPORT=n will result in inb()/outb() and friends not being declared. We thus need to add this dependency and ifdef sections of code using inb()/outb() as alternative access methods. Co-developed-by: Arnd Bergmann <arnd@kernel.org> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com> --- drivers/char/Kconfig | 3 ++- drivers/char/ipmi/Makefile | 11 ++++------- drivers/char/ipmi/ipmi_si_intf.c | 3 ++- drivers/char/ipmi/ipmi_si_pci.c | 3 +++ drivers/char/tpm/Kconfig | 1 + drivers/char/tpm/tpm_infineon.c | 14 ++++++++++---- drivers/char/tpm/tpm_tis_core.c | 19 ++++++++----------- 7 files changed, 30 insertions(+), 24 deletions(-)