Re: [PATCH v3] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send()

[Jarkko Sakkinen]

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@xxxxxxxxxx>
> > > > Reviewed-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> > > > Signed-off-by: George Wilson <gcwilson@xxxxxxxxxxxxx>
> > > > Tested-by: Linh Pham <phaml@xxxxxxxxxx>
> > > > Fixes: 132f76294744 ("Add new device driver to support IBM vTPM")
> > > 
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> > 
> > 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