| Message ID | 20201001180925.13808-2-James.Bottomley@HansenPartnership.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jarkko Sakkinen |
| Headers | show |
| Series | tpm_tis: fix interrupts (again) | expand |
On Thu, Oct 01, 2020 at 11:09:21AM -0700, James Bottomley wrote: > 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 Put a reference to the section. I'm *guessing* that the spec is https://trustedcomputinggroup.org/resource/pc-client-work-group-pc-client-specific-tpm-interface-specification-tis Please have this and also location in this spec. > 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. The description is really good and understandable otherwise. For me it is not obvious at all, why this is missing a fixes tag? > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > --- > > v2: added this patch Use the cover letter for the changelog. I'm afraid that I might miss these otherwise. > --- > 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; > -- > 2.28.0 > /Jarkko
On Mon, 2020-10-05 at 18:34 +0300, Jarkko Sakkinen wrote: > On Thu, Oct 01, 2020 at 11:09:21AM -0700, James Bottomley wrote: > > 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 > > Put a reference to the section. > > I'm *guessing* that the spec is > > https://trustedcomputinggroup.org/resource/pc-client-work-group-pc-client-specific-tpm-interface-specification-tis > > Please have this and also location in this spec. I can, but the TCG reorganizes its website every few months, so no URLs like that are permanent. > > 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. > > The description is really good and understandable otherwise. > > For me it is not obvious at all, why this is missing a fixes > tag? It's been there ever since the initial commit: commit 27084efee0c3dc0eb15b5ed750aa9f1adb3983c3 Author: Leendert van Doorn <leendert@watson.ibm.com> Date: Sat Apr 22 02:38:03 2006 -0700 [PATCH] tpm: driver for next generation TPM chips > > Signed-off-by: James Bottomley < > > James.Bottomley@HansenPartnership.com> > > > > --- > > > > v2: added this patch > > Use the cover letter for the changelog. I'm afraid that I might > miss these otherwise. Submitting patches actually recommends doing this ... I think we want to keep to standard kernel process, but I can gather them in the cover letter as well. James
On Mon, Oct 05, 2020 at 12:00:57PM -0700, James Bottomley wrote: > On Mon, 2020-10-05 at 18:34 +0300, Jarkko Sakkinen wrote: > > On Thu, Oct 01, 2020 at 11:09:21AM -0700, James Bottomley wrote: > > > 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 > > > > Put a reference to the section. > > > > I'm *guessing* that the spec is > > > > https://trustedcomputinggroup.org/resource/pc-client-work-group-pc-client-specific-tpm-interface-specification-tis > > > > Please have this and also location in this spec. > > I can, but the TCG reorganizes its website every few months, so no URLs > like that are permanent. OK, that's good enough excuse :-( Let's then ignore this comment. Just would had save trouble in future if that wasn't the case. > > > 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. > > > > The description is really good and understandable otherwise. > > > > For me it is not obvious at all, why this is missing a fixes > > tag? > > It's been there ever since the initial commit: > > commit 27084efee0c3dc0eb15b5ed750aa9f1adb3983c3 > Author: Leendert van Doorn <leendert@watson.ibm.com> > Date: Sat Apr 22 02:38:03 2006 -0700 > > [PATCH] tpm: driver for next generation TPM chips Then just "Cc: stable@vger.kernel.org" should do. > > > Signed-off-by: James Bottomley < > > > James.Bottomley@HansenPartnership.com> > > > > > > --- > > > > > > v2: added this patch > > > > Use the cover letter for the changelog. I'm afraid that I might > > miss these otherwise. > > Submitting patches actually recommends doing this ... I think we want > to keep to standard kernel process, but I can gather them in the cover > letter as well. Most of the patch sets that I encounter have the cover letter in the changelog and usually it is great for getting overall image what is happening. In section 14 of "submitting patches" there is a remark that the area just after the diffstat marker is a good place to store this kind of information. I have not find any explicit instruction for patch sets, i.e. I just trust the "majority vote". > James /Jarkko
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@HansenPartnership.com> > > --- > > 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@redhat.com>
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@HansenPartnership.com> > > > > --- > > > > 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@redhat.com> 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@ger.kernel.org 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. /Jarkko
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@HansenPartnership.com> > > > > > > --- > > > > > > 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@redhat.com> > > 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@ger.kernel.org > > 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
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;
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@HansenPartnership.com> --- v2: added this patch --- drivers/char/tpm/tpm_tis_core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)