Re: [PATCH 1/1] char: tpm: handle HAS_IOPORT dependencies

[Arnd Bergmann]

On Thu, Apr 4, 2024, at 17:41, Niklas Schnelle wrote:
>
> Good find! I do see the same #ifdef in v5 but maybe something else
> changed. I think this was also hidden during both my local test builds
> and kernel test robot because of the PNP -> ACPI || ISA dependency
> which I think implies HAS_IOPORT. So unless I'm missing something we
> can't currently get here with HAS_IOPORT=n. Maybe that changed?

Rihgt, I just found that as well, so the TCG_INFINEON driver
won't ever be enabled without CONFIG_HAS_IOPORT after all.

> I'm now thinking maybe keeping TPM_INF_IO_PORT is the cleaner choice.
> It saves us 4 lines of #ifdeffery at the cost of one sometimes "unused"
> define.

Agreed. I tried it out for reference, but it does get quite ugly,
see below. Alternatively, we could just add a 'depends on HAS_IOPORT'
to this driver after all. Even if it can be used on MMIO, it might
never actually be built without PIO.

     Arnd

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 418c9ed59ffd..852bb9344788 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -157,7 +157,7 @@ config TCG_ATMEL
 
 config TCG_INFINEON
 	tristate "Infineon Technologies TPM Interface"
-	depends on PNP
+	depends on PNP || COMPILE_TEST
 	help
 	  If you have a TPM security chip from Infineon Technologies
 	  (either SLD 9630 TT 1.1 or SLB 9635 TT 1.2) say Yes and it
diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
index 99c6e565ec8d..768ca65960d8 100644
--- a/drivers/char/tpm/tpm_infineon.c
+++ b/drivers/char/tpm/tpm_infineon.c
@@ -51,10 +51,19 @@ struct tpm_inf_dev {
 
 static struct tpm_inf_dev tpm_dev;
 
+static inline bool tpm_is_ioport(struct tpm_inf_dev *dev)
+{
+#ifdef CONFIG_HAS_IOPORT
+	return tpm_dev.iotype == TPM_INF_IO_PORT;
+#else
+	return false;
+#endif
+}
+
 static inline void tpm_data_out(unsigned char data, unsigned char offset)
 {
 #ifdef CONFIG_HAS_IOPORT
-	if (tpm_dev.iotype == TPM_INF_IO_PORT)
+	if (tpm_is_ioport(&tpm_dev))
 		outb(data, tpm_dev.data_regs + offset);
 	else
 #endif
@@ -64,7 +73,7 @@ static inline void tpm_data_out(unsigned char data, unsigned char offset)
 static inline unsigned char tpm_data_in(unsigned char offset)
 {
 #ifdef CONFIG_HAS_IOPORT
-	if (tpm_dev.iotype == TPM_INF_IO_PORT)
+	if (tpm_is_ioport(&tpm_dev))
 		return inb(tpm_dev.data_regs + offset);
 #endif
 	return readb(tpm_dev.mem_base + tpm_dev.data_regs + offset);
@@ -73,7 +82,7 @@ static inline unsigned char tpm_data_in(unsigned char 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)
+	if (tpm_is_ioport(&tpm_dev))
 		outb(data, tpm_dev.config_port + offset);
 	else
 #endif
@@ -83,7 +92,7 @@ static inline void tpm_config_out(unsigned char data, unsigned char offset)
 static inline unsigned char tpm_config_in(unsigned char offset)
 {
 #ifdef CONFIG_HAS_IOPORT
-	if (tpm_dev.iotype == TPM_INF_IO_PORT)
+	if (tpm_is_ioport(&tpm_dev))
 		return inb(tpm_dev.config_port + offset);
 #endif
 	return readb(tpm_dev.mem_base + tpm_dev.index_off + offset);
@@ -404,6 +413,7 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
 	const char *chipname;
 	struct tpm_chip *chip;
 
+#ifdef CONFIG_HAS_IOPORT
 	/* read IO-ports through PnP */
 	if (pnp_port_valid(dev, 0) && pnp_port_valid(dev, 1) &&
 	    !(pnp_port_flags(dev, 0) & IORESOURCE_DISABLED)) {
@@ -436,8 +446,10 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
 			rc = -EINVAL;
 			goto err_last;
 		}
-	} else if (pnp_mem_valid(dev, 0) &&
-		   !(pnp_mem_flags(dev, 0) & IORESOURCE_DISABLED)) {
+	} else
+#endif
+	if (pnp_mem_valid(dev, 0) &&
+	   !(pnp_mem_flags(dev, 0) & IORESOURCE_DISABLED)) {
 
 		tpm_dev.iotype = TPM_INF_IO_MEM;
 
@@ -540,10 +552,10 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
 			 "vendor id 0x%x%x (Infineon), "
 			 "product id 0x%02x%02x"
 			 "%s\n",
-			 tpm_dev.iotype == TPM_INF_IO_PORT ?
+			 tpm_is_ioport(&tpm_dev) ?
 			 tpm_dev.config_port :
 			 tpm_dev.map_base + tpm_dev.index_off,
-			 tpm_dev.iotype == TPM_INF_IO_PORT ?
+			 tpm_is_ioport(&tpm_dev) ?
 			 tpm_dev.data_regs :
 			 tpm_dev.map_base + tpm_dev.data_regs,
 			 version[0], version[1],
@@ -567,7 +579,7 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
 	}
 
 err_release_region:
-	if (tpm_dev.iotype == TPM_INF_IO_PORT) {
+	if (tpm_is_ioport(&tpm_dev)) {
 		release_region(tpm_dev.data_regs, tpm_dev.data_size);
 		release_region(tpm_dev.config_port, tpm_dev.config_size);
 	} else {
@@ -585,11 +597,14 @@ static void tpm_inf_pnp_remove(struct pnp_dev *dev)
 
 	tpm_chip_unregister(chip);
 
-	if (tpm_dev.iotype == TPM_INF_IO_PORT) {
+#ifdef CONFIG_HAS_IOPORT
+	if (tpm_is_ioport(&tpm_dev)) {
 		release_region(tpm_dev.data_regs, tpm_dev.data_size);
 		release_region(tpm_dev.config_port,
 			       tpm_dev.config_size);
-	} else {
+	} else
+#endif
+	{
 		iounmap(tpm_dev.mem_base);
 		release_mem_region(tpm_dev.map_base, tpm_dev.map_size);
 	}