diff mbox series

[v4] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send()

Message ID 20200320032758.228088-1-gcwilson@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series [v4] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send() | expand

Commit Message

George Wilson March 20, 2020, 3:27 a.m. UTC 
tpm_ibmvtpm_send() can fail during PowerVM Live Partition Mobility resume
with an H_CLOSED return from ibmvtpm_send_crq().  The PAPR says, 'The
"partner partition suspended" transport event disables the associated CRQ
such that any H_SEND_CRQ hcall() to the associated CRQ returns H_Closed
until the CRQ has been explicitly enabled using the H_ENABLE_CRQ hcall.'
This patch adds a check in tpm_ibmvtpm_send() for an H_CLOSED return from
ibmvtpm_send_crq() and in that case calls tpm_ibmvtpm_resume() and
retries the ibmvtpm_send_crq() once.

Reported-by: Linh Pham <phaml@us.ibm.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: George Wilson <gcwilson@linux.ibm.com>
Tested-by: Linh Pham <phaml@us.ibm.com>
Fixes: 132f76294744 ("drivers/char/tpm: Add new device driver to support IBM vTPM")
Cc: stable@vger.kernel.org
---
 drivers/char/tpm/tpm_ibmvtpm.c | 136 ++++++++++++++++++---------------
 1 file changed, 73 insertions(+), 63 deletions(-)

Comments

Jarkko Sakkinen March 20, 2020, 2:12 p.m. UTC | #1 
On Thu, Mar 19, 2020 at 11:27:58PM -0400, George Wilson wrote:
> tpm_ibmvtpm_send() can fail during PowerVM Live Partition Mobility resume
> with an H_CLOSED return from ibmvtpm_send_crq().  The PAPR says, 'The
> "partner partition suspended" transport event disables the associated CRQ
> such that any H_SEND_CRQ hcall() to the associated CRQ returns H_Closed
> until the CRQ has been explicitly enabled using the H_ENABLE_CRQ hcall.'
> This patch adds a check in tpm_ibmvtpm_send() for an H_CLOSED return from
> ibmvtpm_send_crq() and in that case calls tpm_ibmvtpm_resume() and
> retries the ibmvtpm_send_crq() once.
> 
> Reported-by: Linh Pham <phaml@us.ibm.com>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: George Wilson <gcwilson@linux.ibm.com>
> Tested-by: Linh Pham <phaml@us.ibm.com>
> Fixes: 132f76294744 ("drivers/char/tpm: Add new device driver to support IBM vTPM")
> Cc: stable@vger.kernel.org

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Thanks. Applied now.

/Jarkko
Sasha Levin March 20, 2020, 5:59 p.m. UTC | #2 
Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 132f76294744 ("drivers/char/tpm: Add new device driver to support IBM vTPM").

The bot has tested the following trees: v5.5.10, v5.4.26, v4.19.111, v4.14.173, v4.9.216, v4.4.216.

v5.5.10: Build OK!
v5.4.26: Build OK!
v4.19.111: Build OK!
v4.14.173: Build OK!
v4.9.216: Failed to apply! Possible dependencies:
    02ae1382882f ("tpm: redefine read_log() to handle ACPI/OF at runtime")
    2528a64664f8 ("tpm: define a generic open() method for ascii & bios measurements")
    402149c6470d ("tpm: vtpm_proxy: Suppress error logging when in closed state")
    4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware event log")
    745b361e989a ("tpm: infrastructure for TPM spaces")
    748935eeb72c ("tpm: have event log use the tpm_chip")
    7518a21a9da3 ("tpm: drop tpm1_chip_register(/unregister)")
    b1a9b7b602c5 ("tpm: replace symbolic permission with octal for securityfs files")
    cd9b7631a888 ("tpm: replace dynamically allocated bios_dir with a static array")
    f5595f5baa30 ("tpm: Unify the send callback behaviour")

v4.4.216: Failed to apply! Possible dependencies:
    02ae1382882f ("tpm: redefine read_log() to handle ACPI/OF at runtime")
    036bb38ffb3e ("tpm_tis: Ensure interrupts are disabled when the driver starts")
    23d06ff700f5 ("tpm: drop tpm_atmel specific fields from tpm_vendor_specific")
    25112048cd59 ("tpm: rework tpm_get_timeouts()")
    402149c6470d ("tpm: vtpm_proxy: Suppress error logging when in closed state")
    41a5e1cf1fe1 ("tpm/tpm_tis: Split tpm_tis driver into a core and TCG TIS compliant phy")
    4d627e672bd0 ("tpm_tis: Do not fall back to a hardcoded address for TPM2")
    4eea703caaac ("tpm: drop 'iobase' from struct tpm_vendor_specific")
    51dd43dff74b ("tpm_tis: Use devm_ioremap_resource")
    55a889c2cb13 ("tpm_crb: Use the common ACPI definition of struct acpi_tpm2")
    56671c893e0e ("tpm: drop 'locality' from struct tpm_vendor_specific")
    570a36097f30 ("tpm: drop 'irq' from struct tpm_vendor_specific")
    57dacc2b4ce5 ("tpm: tpm_tis: Share common data between phys")
    745b361e989a ("tpm: infrastructure for TPM spaces")
    7ab4032fa579 ("tpm_tis: Get rid of the duplicate IRQ probing code")
    d30b8e4f68ef ("tpm: cleanup tpm_tis_remove()")
    d4956524f1b0 ("tpm: drop manufacturer_id from struct tpm_vendor_specific")
    e3837e74a06d ("tpm_tis: Refactor the interrupt setup")
    ee1779840d09 ("tpm: drop 'base' from struct tpm_vendor_specific")
    ef7b81dc7864 ("tpm_tis: Disable interrupt auto probing on a per-device basis")
    f5595f5baa30 ("tpm: Unify the send callback behaviour")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?
George Wilson March 25, 2020, 3:45 p.m. UTC | #3 
On Fri, Mar 20, 2020 at 05:59:44PM +0000, Sasha Levin wrote:
> Hi
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag
> fixing commit: 132f76294744 ("drivers/char/tpm: Add new device driver to support IBM vTPM").
> 
> The bot has tested the following trees: v5.5.10, v5.4.26, v4.19.111, v4.14.173, v4.9.216, v4.4.216.
> 
> v5.5.10: Build OK!
> v5.4.26: Build OK!
> v4.19.111: Build OK!
> v4.14.173: Build OK!
> v4.9.216: Failed to apply! Possible dependencies:
>     02ae1382882f ("tpm: redefine read_log() to handle ACPI/OF at runtime")
>     2528a64664f8 ("tpm: define a generic open() method for ascii & bios measurements")
>     402149c6470d ("tpm: vtpm_proxy: Suppress error logging when in closed state")
>     4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware event log")
>     745b361e989a ("tpm: infrastructure for TPM spaces")
>     748935eeb72c ("tpm: have event log use the tpm_chip")
>     7518a21a9da3 ("tpm: drop tpm1_chip_register(/unregister)")
>     b1a9b7b602c5 ("tpm: replace symbolic permission with octal for securityfs files")
>     cd9b7631a888 ("tpm: replace dynamically allocated bios_dir with a static array")
>     f5595f5baa30 ("tpm: Unify the send callback behaviour")
> 
> v4.4.216: Failed to apply! Possible dependencies:
>     02ae1382882f ("tpm: redefine read_log() to handle ACPI/OF at runtime")
>     036bb38ffb3e ("tpm_tis: Ensure interrupts are disabled when the driver starts")
>     23d06ff700f5 ("tpm: drop tpm_atmel specific fields from tpm_vendor_specific")
>     25112048cd59 ("tpm: rework tpm_get_timeouts()")
>     402149c6470d ("tpm: vtpm_proxy: Suppress error logging when in closed state")
>     41a5e1cf1fe1 ("tpm/tpm_tis: Split tpm_tis driver into a core and TCG TIS compliant phy")
>     4d627e672bd0 ("tpm_tis: Do not fall back to a hardcoded address for TPM2")
>     4eea703caaac ("tpm: drop 'iobase' from struct tpm_vendor_specific")
>     51dd43dff74b ("tpm_tis: Use devm_ioremap_resource")
>     55a889c2cb13 ("tpm_crb: Use the common ACPI definition of struct acpi_tpm2")
>     56671c893e0e ("tpm: drop 'locality' from struct tpm_vendor_specific")
>     570a36097f30 ("tpm: drop 'irq' from struct tpm_vendor_specific")
>     57dacc2b4ce5 ("tpm: tpm_tis: Share common data between phys")
>     745b361e989a ("tpm: infrastructure for TPM spaces")
>     7ab4032fa579 ("tpm_tis: Get rid of the duplicate IRQ probing code")
>     d30b8e4f68ef ("tpm: cleanup tpm_tis_remove()")
>     d4956524f1b0 ("tpm: drop manufacturer_id from struct tpm_vendor_specific")
>     e3837e74a06d ("tpm_tis: Refactor the interrupt setup")
>     ee1779840d09 ("tpm: drop 'base' from struct tpm_vendor_specific")
>     ef7b81dc7864 ("tpm_tis: Disable interrupt auto probing on a per-device basis")
>     f5595f5baa30 ("tpm: Unify the send callback behaviour")
> 
> 
> NOTE: The patch will not be queued to stable trees until it is upstream.
> 
> How should we proceed with this patch?

Hi Sasha,

I've backported the patch to both 4.9.y and 4.4.y.

> 
> -- 
> Thanks
> Sasha
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 78cc52690177..e82013d587b4 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -1,6 +1,6 @@ 
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * Copyright (C) 2012 IBM Corporation
+ * Copyright (C) 2012-2020 IBM Corporation
  *
  * Author: Ashley Lai <ashleydlai@gmail.com>
  *
@@ -133,6 +133,64 @@  static int tpm_ibmvtpm_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 	return len;
 }
 
+/**
+ * ibmvtpm_crq_send_init - Send a CRQ initialize message
+ * @ibmvtpm:	vtpm device struct
+ *
+ * Return:
+ *	0 on success.
+ *	Non-zero on failure.
+ */
+static int ibmvtpm_crq_send_init(struct ibmvtpm_dev *ibmvtpm)
+{
+	int rc;
+
+	rc = ibmvtpm_send_crq_word(ibmvtpm->vdev, INIT_CRQ_CMD);
+	if (rc != H_SUCCESS)
+		dev_err(ibmvtpm->dev,
+			"%s failed rc=%d\n", __func__, rc);
+
+	return rc;
+}
+
+/**
+ * tpm_ibmvtpm_resume - Resume from suspend
+ *
+ * @dev:	device struct
+ *
+ * Return: Always 0.
+ */
+static int tpm_ibmvtpm_resume(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev);
+	int rc = 0;
+
+	do {
+		if (rc)
+			msleep(100);
+		rc = plpar_hcall_norets(H_ENABLE_CRQ,
+					ibmvtpm->vdev->unit_address);
+	} while (rc == H_IN_PROGRESS || rc == H_BUSY || H_IS_LONG_BUSY(rc));
+
+	if (rc) {
+		dev_err(dev, "Error enabling ibmvtpm rc=%d\n", rc);
+		return rc;
+	}
+
+	rc = vio_enable_interrupts(ibmvtpm->vdev);
+	if (rc) {
+		dev_err(dev, "Error vio_enable_interrupts rc=%d\n", rc);
+		return rc;
+	}
+
+	rc = ibmvtpm_crq_send_init(ibmvtpm);
+	if (rc)
+		dev_err(dev, "Error send_init rc=%d\n", rc);
+
+	return rc;
+}
+
 /**
  * tpm_ibmvtpm_send() - Send a TPM command
  * @chip:	tpm chip struct
@@ -146,6 +204,7 @@  static int tpm_ibmvtpm_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
 {
 	struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev);
+	bool retry = true;
 	int rc, sig;
 
 	if (!ibmvtpm->rtce_buf) {
@@ -179,18 +238,27 @@  static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count)
 	 */
 	ibmvtpm->tpm_processing_cmd = true;
 
+again:
 	rc = ibmvtpm_send_crq(ibmvtpm->vdev,
 			IBMVTPM_VALID_CMD, VTPM_TPM_COMMAND,
 			count, ibmvtpm->rtce_dma_handle);
 	if (rc != H_SUCCESS) {
+		/*
+		 * H_CLOSED can be returned after LPM resume.  Call
+		 * tpm_ibmvtpm_resume() to re-enable the CRQ then retry
+		 * ibmvtpm_send_crq() once before failing.
+		 */
+		if (rc == H_CLOSED && retry) {
+			tpm_ibmvtpm_resume(ibmvtpm->dev);
+			retry = false;
+			goto again;
+		}
 		dev_err(ibmvtpm->dev, "tpm_ibmvtpm_send failed rc=%d\n", rc);
-		rc = 0;
 		ibmvtpm->tpm_processing_cmd = false;
-	} else
-		rc = 0;
+	}
 
 	spin_unlock(&ibmvtpm->rtce_lock);
-	return rc;
+	return 0;
 }
 
 static void tpm_ibmvtpm_cancel(struct tpm_chip *chip)
@@ -268,26 +336,6 @@  static int ibmvtpm_crq_send_init_complete(struct ibmvtpm_dev *ibmvtpm)
 	return rc;
 }
 
-/**
- * ibmvtpm_crq_send_init - Send a CRQ initialize message
- * @ibmvtpm:	vtpm device struct
- *
- * Return:
- *	0 on success.
- *	Non-zero on failure.
- */
-static int ibmvtpm_crq_send_init(struct ibmvtpm_dev *ibmvtpm)
-{
-	int rc;
-
-	rc = ibmvtpm_send_crq_word(ibmvtpm->vdev, INIT_CRQ_CMD);
-	if (rc != H_SUCCESS)
-		dev_err(ibmvtpm->dev,
-			"ibmvtpm_crq_send_init failed rc=%d\n", rc);
-
-	return rc;
-}
-
 /**
  * tpm_ibmvtpm_remove - ibm vtpm remove entry point
  * @vdev:	vio device struct
@@ -400,44 +448,6 @@  static int ibmvtpm_reset_crq(struct ibmvtpm_dev *ibmvtpm)
 				  ibmvtpm->crq_dma_handle, CRQ_RES_BUF_SIZE);
 }
 
-/**
- * tpm_ibmvtpm_resume - Resume from suspend
- *
- * @dev:	device struct
- *
- * Return: Always 0.
- */
-static int tpm_ibmvtpm_resume(struct device *dev)
-{
-	struct tpm_chip *chip = dev_get_drvdata(dev);
-	struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev);
-	int rc = 0;
-
-	do {
-		if (rc)
-			msleep(100);
-		rc = plpar_hcall_norets(H_ENABLE_CRQ,
-					ibmvtpm->vdev->unit_address);
-	} while (rc == H_IN_PROGRESS || rc == H_BUSY || H_IS_LONG_BUSY(rc));
-
-	if (rc) {
-		dev_err(dev, "Error enabling ibmvtpm rc=%d\n", rc);
-		return rc;
-	}
-
-	rc = vio_enable_interrupts(ibmvtpm->vdev);
-	if (rc) {
-		dev_err(dev, "Error vio_enable_interrupts rc=%d\n", rc);
-		return rc;
-	}
-
-	rc = ibmvtpm_crq_send_init(ibmvtpm);
-	if (rc)
-		dev_err(dev, "Error send_init rc=%d\n", rc);
-
-	return rc;
-}
-
 static bool tpm_ibmvtpm_req_canceled(struct tpm_chip *chip, u8 status)
 {
 	return (status == 0);