Received-SPF: pass (google.com: domain of linux-kernel+bounces-131777-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Feedback-ID: i36794607:Fastmail
User-Agent: Cyrus-JMAP/3.11.0-alpha0-333-gbfea15422e-fm-20240327.001-gbfea1542
Precedence: bulk
MIME-Version: 1.0
Message-Id: <9d9fa267-067e-421b-9a39-aa178b913298@app.fastmail.com>
In-Reply-To: <4cdb6399d803d37a883d1a2fbe119c0a14610106.camel@linux.ibm.com>
References: <20240404105840.3396821-1-schnelle@linux.ibm.com>
 <20240404105840.3396821-2-schnelle@linux.ibm.com>
 <95a63afe-ccd7-4551-86af-00b7fb0d8ff9@app.fastmail.com>
 <4cdb6399d803d37a883d1a2fbe119c0a14610106.camel@linux.ibm.com>
Date: Thu, 04 Apr 2024 17:56:47 +0200
From: "Arnd Bergmann" <arnd@kernel.org>
To: "Niklas Schnelle" <schnelle@linux.ibm.com>,
 "Peter Huewe" <peterhuewe@gmx.de>, "Jarkko Sakkinen" <jarkko@kernel.org>
Cc: linux-integrity@vger.kernel.org, "Heiko Carstens" <hca@linux.ibm.com>,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] char: tpm: handle HAS_IOPORT dependencies
Content-Type: text/plain

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);
 	}