linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: George Wilson <gcwilson@linux.ibm.com>
Cc: linux-integrity@vger.kernel.org,
	Alexey Kardashevskiy <aik@ozlabs.ru>,
	Stefan Berger <stefanb@linux.ibm.com>,
	Nayna Jain <nayna@linux.vnet.ibm.com>,
	Jason Gunthorpe <jgg@ziepe.ca>,
	linux-kernel@vger.kernel.org, Linh Pham <phaml@us.ibm.com>
Subject: Re: [PATCH v3] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send()
Date: Fri, 20 Mar 2020 04:00:00 +0200	[thread overview]
Message-ID: <20200320020000.GB183331@linux.intel.com> (raw)
In-Reply-To: <20200319231552.GA25351@us.ibm.com>

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

      reply	other threads:[~2020-03-20  2:00 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-18 23:49 [PATCH v3] tpm: ibmvtpm: retry on H_CLOSED in tpm_ibmvtpm_send() George Wilson
2020-03-19 19:50 ` Jarkko Sakkinen
2020-03-19 19:55   ` Jarkko Sakkinen
2020-03-19 23:15     ` George Wilson
2020-03-20  2:00       ` Jarkko Sakkinen [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200320020000.GB183331@linux.intel.com \
    --to=jarkko.sakkinen@linux.intel.com \
    --cc=aik@ozlabs.ru \
    --cc=gcwilson@linux.ibm.com \
    --cc=jgg@ziepe.ca \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nayna@linux.vnet.ibm.com \
    --cc=phaml@us.ibm.com \
    --cc=stefanb@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).