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

[Arnd Bergmann]

On Fri, Apr 5, 2024, at 11:23, Niklas Schnelle wrote:
>
> I just confirmed that keeping the define it also compiles but I do
> wonder if it's not even cleaner to just add an explicit HAS_IOPORT
> dependency and no new #ifdefs in the code. I'm willing to send a patch
> for any of these solutions though.

It depends a bit on where the driver is used in the end. We
currently set HAS_IOPORT on arm64 and riscv, but we could make
that dependent on which PCI host drivers are actually being
built, as a lot of modern hardware doesn't actually support
port I/O.

Is this driver still expected to be used on modern PCIe hosts
with no port I/O, or would new machines all use the i2c version?

If we do need the driver in configurations without CONFIG_HAS_IOPORT,
then the patch below wouldn't be awful (on top of the patch that
got merged already).

     Arnd

diff --git a/drivers/char/tpm/tpm_infineon.c b/drivers/char/tpm/tpm_infineon.c
index 99c6e565ec8d..5b45ad619900 100644
--- a/drivers/char/tpm/tpm_infineon.c
+++ b/drivers/char/tpm/tpm_infineon.c
@@ -37,7 +37,8 @@
 struct tpm_inf_dev {
 	int iotype;
 
-	void __iomem *mem_base;	/* MMIO ioremap'd addr */
+	void __iomem *data_base;	/* MMIO ioremap'd addr */
+	void __iomem *config_base;	/* MMIO ioremap'd config */
 	unsigned long map_base;	/* phys MMIO base */
 	unsigned long map_size;	/* MMIO region size */
 	unsigned int index_off;	/* index register offset */
@@ -53,40 +54,22 @@ 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);
+	iowrite8(data, tpm_dev.data_base + 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);
-#endif
-	return readb(tpm_dev.mem_base + tpm_dev.data_regs + offset);
+	return ioread8(tpm_dev.data_base + 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);
+	iowrite8(data, tpm_dev.config_base + 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);
-#endif
-	return readb(tpm_dev.mem_base + tpm_dev.index_off + offset);
+	return ioread8(tpm_dev.config_base + offset);
 }
 
 /* TPM header definitions */
@@ -425,16 +408,27 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
 			goto err_last;
 		}
 		/* publish my base address and request region */
+		tpm_dev.data_base = ioport_map(tpm_dev.data_regs, tpm_dev.data_size);
+		if (!tpm_dev.data_base) {
+			rc = -EINVAL;
+			goto err_last;
+		}
 		if (request_region(tpm_dev.data_regs, tpm_dev.data_size,
 				   "tpm_infineon0") == NULL) {
 			rc = -EINVAL;
+			ioport_unmap(tpm_dev.config_base);
 			goto err_last;
 		}
+		tpm_dev.config_base = ioport_map(tpm_dev.config_port, tpm_dev.config_size);
+		if (!tpm_dev.config_base) {
+			rc = -EINVAL;
+			goto err_release_data_region;
+		}
 		if (request_region(tpm_dev.config_port, tpm_dev.config_size,
 				   "tpm_infineon0") == NULL) {
 			release_region(tpm_dev.data_regs, tpm_dev.data_size);
 			rc = -EINVAL;
-			goto err_last;
+			goto err_release_data_region;
 		}
 	} else if (pnp_mem_valid(dev, 0) &&
 		   !(pnp_mem_flags(dev, 0) & IORESOURCE_DISABLED)) {
@@ -454,8 +448,8 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
 			goto err_last;
 		}
 
-		tpm_dev.mem_base = ioremap(tpm_dev.map_base, tpm_dev.map_size);
-		if (tpm_dev.mem_base == NULL) {
+		tpm_dev.data_base = ioremap(tpm_dev.map_base, tpm_dev.map_size);
+		if (tpm_dev.data_base == NULL) {
 			release_mem_region(tpm_dev.map_base, tpm_dev.map_size);
 			rc = -EINVAL;
 			goto err_last;
@@ -468,8 +462,7 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
 		 * seem like they could be placed anywhere within the MMIO
 		 * region, but lets just put them at zero offset.
 		 */
-		tpm_dev.index_off = TPM_ADDR;
-		tpm_dev.data_regs = 0x0;
+		tpm_dev.config_base = tpm_dev.data_base + TPM_ADDR;
 	} else {
 		rc = -EINVAL;
 		goto err_last;
@@ -568,10 +561,16 @@ static int tpm_inf_pnp_probe(struct pnp_dev *dev,
 
 err_release_region:
 	if (tpm_dev.iotype == TPM_INF_IO_PORT) {
-		release_region(tpm_dev.data_regs, tpm_dev.data_size);
+		ioport_unmap(tpm_dev.config_base);
 		release_region(tpm_dev.config_port, tpm_dev.config_size);
+	}
+
+err_release_data_region:
+	if (tpm_dev.iotype == TPM_INF_IO_PORT) {
+		ioport_unmap(tpm_dev.data_base);
+		release_region(tpm_dev.data_regs, tpm_dev.data_size);
 	} else {
-		iounmap(tpm_dev.mem_base);
+		iounmap(tpm_dev.data_base);
 		release_mem_region(tpm_dev.map_base, tpm_dev.map_size);
 	}
 
@@ -586,11 +585,13 @@ static void tpm_inf_pnp_remove(struct pnp_dev *dev)
 	tpm_chip_unregister(chip);
 
 	if (tpm_dev.iotype == TPM_INF_IO_PORT) {
+		ioport_unmap(tpm_dev.data_base);
 		release_region(tpm_dev.data_regs, tpm_dev.data_size);
+		ioport_unmap(tpm_dev.config_base);
 		release_region(tpm_dev.config_port,
 			       tpm_dev.config_size);
 	} else {
-		iounmap(tpm_dev.mem_base);
+		iounmap(tpm_dev.data_base);
 		release_mem_region(tpm_dev.map_base, tpm_dev.map_size);
 	}
 }