On Sat, Oct 24, 2020 at 03:07:44PM +0300, Jarkko Sakkinen wrote:
> On Mon, Oct 19, 2020 at 04:16:32PM -0700, Jerry Snitselaar wrote:
> >
> > James Bottomley @ 2020-10-01 11:09 MST:
> >
> > > The TPM TIS specification says the TPM signals the acquisition of
> > > locality when the TMP_ACCESS_REQUEST_USE bit goes to one *and* the
> > > TPM_ACCESS_REQUEST_USE bit goes to zero. Currently we only check the
> > > former not the latter, so check both. Adding the check on
> > > TPM_ACCESS_REQUEST_USE should fix the case where the locality is
> > > re-requested before the TPM has released it. In this case the
> > > locality may get released briefly before it is reacquired, which
> > > causes all sorts of problems. However, with the added check,
> > > TPM_ACCESS_REQUEST_USE should remain 1 until the second request for
> > > the locality is granted.
> > >
> > > Signed-off-by: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> > >
> > > ---
> > >
> > > v2: added this patch
> > > ---
> > > drivers/char/tpm/tpm_tis_core.c | 3 ++-
> > > 1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > > index 92c51c6cfd1b..f3ecde8df47d 100644
> > > --- a/drivers/char/tpm/tpm_tis_core.c
> > > +++ b/drivers/char/tpm/tpm_tis_core.c
> > > @@ -125,7 +125,8 @@ static bool check_locality(struct tpm_chip *chip, int l)
> > > if (rc < 0)
> > > return false;
> > >
> > > - if ((access & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) ==
> > > + if ((access & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID
> > > + | TPM_ACCESS_REQUEST_USE)) ==
> > > (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) {
> > > priv->locality = l;
> > > return true;
> >
> > Reviewed-by: Jerry Snitselaar <jsnitsel@xxxxxxxxxx>
>
> Thank you.
>
> James, few minor remarks.
>
> Given that the failing commit is in the GIT, just for reference
>
> Fixes: 27084efee0c3 ("[PATCH] tpm: driver for next generation TPM chips")
> Cc: stable@xxxxxxxxxxxxxx
>
> I want fixes tags for everything that has a legit Git commit, even
> if it spans through all the existing stable kernels. It's valuable
> information to have in the Git log.
>
> Another thing I noticed is that would be less ugly put everything
> in the same line, as checkpatch requirement have been relaxed.
>
> Finally, please put a verbose inline comment before the condition.
> tpm_tis driver is complicated enough that it should be better
> documented. After months pass things tend to fade away and wrong
> decisions might be made because of that. You can probably derive
> it from the already nice and verbose commit message, so let's
> take advantage of it.
>
> About recent debate on patch changelogs. I talked with Dave Hansen
> about this, and he said that in x86 tree, they are the standard, but
> the practice depends per tree.
>
> After thinking about this, and writing per patch changelogs for the
> next iteration of the SGX patch set, my standing point is that either
> works as it is properly written and maintained.
This can be merged immediately once the remarks have been taken care
of (i.e. do not except tested-by for 1/5).
/Jarkko
