| Message ID | 20200317214600.9561-1-gcwilson@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send() | expand |
On Tue, Mar 17, 2020 at 05:46:00PM -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> I think this should have a fixes tag. > --- > drivers/char/tpm/tpm_ibmvtpm.c | 23 ++++++++++++++++++----- > 1 file changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c > index 78cc52690177..ba3bd503e080 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> > * > @@ -25,6 +25,8 @@ > #include "tpm.h" > #include "tpm_ibmvtpm.h" > > +static int tpm_ibmvtpm_resume(struct device *dev); > + Instead, reposition this function before tpm_ibmvtpm_send(). > static const char tpm_ibmvtpm_driver_name[] = "tpm_ibmvtpm"; > > static const struct vio_device_id tpm_ibmvtpm_device_table[] = { > @@ -147,6 +149,7 @@ static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count) > { > struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev); > int rc, sig; > + bool retry = true; Cosmetic: would be nice to have inits when possible in reverse Christmas tree order i.e. struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev); bool retry = true; int rc, sig; It is way more pleasing for the eye when you have to read the source code. > if (!ibmvtpm->rtce_buf) { > dev_err(ibmvtpm->dev, "ibmvtpm device is not ready\n"); > @@ -179,18 +182,28 @@ 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); > + A dangling newline character. > 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) > -- > 2.24.1 > /Jarkko
On Wed, Mar 18, 2020 at 10:43:18PM +0200, Jarkko Sakkinen wrote: > > static const char tpm_ibmvtpm_driver_name[] = "tpm_ibmvtpm"; > > > > static const struct vio_device_id tpm_ibmvtpm_device_table[] = { > > @@ -147,6 +149,7 @@ static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count) > > { > > struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev); > > int rc, sig; > > + bool retry = true; > > Cosmetic: would be nice to have inits when possible in reverse > Christmas tree order i.e. > > struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev); > bool retry = true; > int rc, sig; > > It is way more pleasing for the eye when you have to read the source > code. I thought only netdev insisted on that :O Jason
On Wed, Mar 18, 2020 at 07:35:42PM -0300, Jason Gunthorpe wrote: > On Wed, Mar 18, 2020 at 10:43:18PM +0200, Jarkko Sakkinen wrote: > > > static const char tpm_ibmvtpm_driver_name[] = "tpm_ibmvtpm"; > > > > > > static const struct vio_device_id tpm_ibmvtpm_device_table[] = { > > > @@ -147,6 +149,7 @@ static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count) > > > { > > > struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev); > > > int rc, sig; > > > + bool retry = true; > > > > Cosmetic: would be nice to have inits when possible in reverse > > Christmas tree order i.e. > > > > struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev); > > bool retry = true; > > int rc, sig; > > > > It is way more pleasing for the eye when you have to read the source > > code. > > I thought only netdev insisted on that :O x86 seems to prefer too and I agree with the idea. But as I said only that it would be nice. /Jarkko
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c index 78cc52690177..ba3bd503e080 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> * @@ -25,6 +25,8 @@ #include "tpm.h" #include "tpm_ibmvtpm.h" +static int tpm_ibmvtpm_resume(struct device *dev); + static const char tpm_ibmvtpm_driver_name[] = "tpm_ibmvtpm"; static const struct vio_device_id tpm_ibmvtpm_device_table[] = { @@ -147,6 +149,7 @@ static int tpm_ibmvtpm_send(struct tpm_chip *chip, u8 *buf, size_t count) { struct ibmvtpm_dev *ibmvtpm = dev_get_drvdata(&chip->dev); int rc, sig; + bool retry = true; if (!ibmvtpm->rtce_buf) { dev_err(ibmvtpm->dev, "ibmvtpm device is not ready\n"); @@ -179,18 +182,28 @@ 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)