| Message ID | 20200318234927.206075-1-gcwilson@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send() | expand |
On Wed, Mar 18, 2020 at 07:49:27PM -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 ("Add new device driver to support IBM vTPM") Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Thank you. /Jarkko
On Thu, Mar 19, 2020 at 09:50:16PM +0200, Jarkko Sakkinen wrote: > On Wed, Mar 18, 2020 at 07:49:27PM -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 ("Add new device driver to support IBM vTPM") > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Unfortunately have to take that back because it has checkpatch errors: $ scripts/checkpatch.pl 0001-tpm-ibmvtpm-retry-on-H_CLOSED-in-tpm_ibmvtpm_send.patch WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) #11: “partner partition suspended” transport event disables the associated CRQ WARNING: Prefer using '"%s...", __func__' to using 'ibmvtpm_crq_send_init', this function's name, in a string #61: FILE: drivers/char/tpm/tpm_ibmvtpm.c:152: + "ibmvtpm_crq_send_init failed rc=%d\n", rc); Also the fixes tag is incorrect. Should be: Fixes: 132f76294744 ("drivers/char/tpm: Add new device driver to support IBM vTPM") /Jarkko
On Thu, Mar 19, 2020 at 09:55:03PM +0200, Jarkko Sakkinen wrote: > On Thu, Mar 19, 2020 at 09:50:16PM +0200, Jarkko Sakkinen wrote: > > On Wed, Mar 18, 2020 at 07:49:27PM -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 ("Add new device driver to support IBM vTPM") > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > Unfortunately have to take that back because it has checkpatch > errors: > > $ scripts/checkpatch.pl 0001-tpm-ibmvtpm-retry-on-H_CLOSED-in-tpm_ibmvtpm_send.patch > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) > #11: > “partner partition suspended” transport event disables the associated CRQ I'd noticed that but it appears to be a spurious checkpatch warning. The line is 73 chars long, the same as the first line of the commit description. Maybe the quotes throw it off? > > WARNING: Prefer using '"%s...", __func__' to using 'ibmvtpm_crq_send_init', this function's name, in a string > #61: FILE: drivers/char/tpm/tpm_ibmvtpm.c:152: > + "ibmvtpm_crq_send_init failed rc=%d\n", rc); I didn't change that error string because it's in an unmodified existing function that I moved above the caller so a declaration wasn't required. All other examples in the file are the same. I'm of course happy to change it in this function if you think it's appropriate to do so. > > Also the fixes tag is incorrect. Should be: > > Fixes: 132f76294744 ("drivers/char/tpm: Add new device driver to support IBM vTPM") I see it done different ways, mostly without the path, even for the TPM drivers. For example, there's no path in Stefan's "[PATCH v7 2/3] tpm: ibmvtpm: Wait for buffer to be set before proceeding." I'm certainly happy to change it, however, and it's good to know that's the preferred style going forward. Separate topic: Since this fixes a migration hang, do you think it should also be cc'd to stable? > > /Jarkko
On Thu, Mar 19, 2020 at 06:15:52PM -0500, George Wilson wrote: > On Thu, Mar 19, 2020 at 09:55:03PM +0200, Jarkko Sakkinen wrote: > > On Thu, Mar 19, 2020 at 09:50:16PM +0200, Jarkko Sakkinen wrote: > > > On Wed, Mar 18, 2020 at 07:49:27PM -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 ("Add new device driver to support IBM vTPM") > > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > Unfortunately have to take that back because it has checkpatch > > errors: > > > > $ scripts/checkpatch.pl 0001-tpm-ibmvtpm-retry-on-H_CLOSED-in-tpm_ibmvtpm_send.patch > > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) > > #11: > > “partner partition suspended” transport event disables the associated CRQ > > I'd noticed that but it appears to be a spurious checkpatch warning. > The line is 73 chars long, the same as the first line of the commit > description. Maybe the quotes throw it off? Lets just ignore this warning. > > > > WARNING: Prefer using '"%s...", __func__' to using 'ibmvtpm_crq_send_init', this function's name, in a string > > #61: FILE: drivers/char/tpm/tpm_ibmvtpm.c:152: > > + "ibmvtpm_crq_send_init failed rc=%d\n", rc); > > I didn't change that error string because it's in an unmodified existing > function that I moved above the caller so a declaration wasn't required. > All other examples in the file are the same. I'm of course happy to > change it in this function if you think it's appropriate to do so. What you are saying makes sense to me but given that it is rather minuscule change I'd just sweep it away. > > > > Also the fixes tag is incorrect. Should be: > > > > Fixes: 132f76294744 ("drivers/char/tpm: Add new device driver to support IBM vTPM") > > I see it done different ways, mostly without the path, even for the TPM > drivers. For example, there's no path in Stefan's "[PATCH v7 2/3] tpm: > ibmvtpm: Wait for buffer to be set before proceeding." I'm certainly > happy to change it, however, and it's good to know that's the preferred > style going forward. "If your patch fixes a bug in a specific commit, e.g. you found an issue using git bisect, please use the ‘Fixes:’ tag with the first 12 characters of the SHA-1 ID, and the one line summary." https://www.kernel.org/doc/html/v5.5/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > Separate topic: Since this fixes a migration hang, do you think it > should also be cc'd to stable? Sure, it would make sense. /Jarkko
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c index 78cc52690177..d097d47e0efa 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, + "ibmvtpm_crq_send_init failed rc=%d\n", 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);