| Message ID | 20201001180925.13808-3-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:22AM -0700, James Bottomley wrote: > The current release locality code seems to be based on the > misunderstanding that the TPM interrupts when a locality is released: > it doesn't, only when the locality is acquired. > > Furthermore, there seems to be no point in waiting for the locality to > be released. All it does is penalize the last TPM user. However, if > there's no next TPM user, this is a pointless wait and if there is a > next TPM user, they'll pay the penalty waiting for the new locality > (or possibly not if it's the same as the old locality). > > Fix the code by making release_locality as simple write to release > with no waiting for completion. > > Fixes: 33bafe90824b ("tpm_tis: verify locality released before returning from release_locality") > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> So, if I got it right this is dependent on 1/5 to address Jerry's issue? I.e. if this has a fixes tag and previous does not, it will not fully fix the situation when backporting? /Jarkko
On Thu, Oct 01, 2020 at 11:09:22AM -0700, James Bottomley wrote: > The current release locality code seems to be based on the > misunderstanding that the TPM interrupts when a locality is released: > it doesn't, only when the locality is acquired. > > Furthermore, there seems to be no point in waiting for the locality to > be released. All it does is penalize the last TPM user. However, if > there's no next TPM user, this is a pointless wait and if there is a > next TPM user, they'll pay the penalty waiting for the new locality > (or possibly not if it's the same as the old locality). > > Fix the code by making release_locality as simple write to release > with no waiting for completion. > > Fixes: 33bafe90824b ("tpm_tis: verify locality released before returning from release_locality") > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > --- > > v2: added fixes Also, this should go to cover letter (with some detail on the fixes). /Jarkko
On Mon, 2020-10-05 at 20:02 +0300, Jarkko Sakkinen wrote: > On Thu, Oct 01, 2020 at 11:09:22AM -0700, James Bottomley wrote: > > The current release locality code seems to be based on the > > misunderstanding that the TPM interrupts when a locality is > > released: it doesn't, only when the locality is acquired. > > > > Furthermore, there seems to be no point in waiting for the locality > > to be released. All it does is penalize the last TPM > > user. However, if there's no next TPM user, this is a pointless > > wait and if there is a next TPM user, they'll pay the penalty > > waiting for the new locality (or possibly not if it's the same as > > the old locality). > > > > Fix the code by making release_locality as simple write to release > > with no waiting for completion. > > > > Fixes: 33bafe90824b ("tpm_tis: verify locality released before > > returning from release_locality") > > Signed-off-by: James Bottomley < > > James.Bottomley@HansenPartnership.com> > > So, if I got it right this is dependent on 1/5 to address Jerry's > issue? I.e. if this has a fixes tag and previous does not, it will > not fully fix the situation when backporting? Yes, exactly. Technically 1/5 isn't really fixing anything at all, it's changing from the current fix where we wait for the locality to be released at the back end of a TIS TPM operation to a new fix where we correctly check the conditions in the locality acquisition. After the new fix is done, we can eliminate all the wait code in locality release. James
On Mon, Oct 05, 2020 at 12:05:07PM -0700, James Bottomley wrote: > On Mon, 2020-10-05 at 20:02 +0300, Jarkko Sakkinen wrote: > > On Thu, Oct 01, 2020 at 11:09:22AM -0700, James Bottomley wrote: > > > The current release locality code seems to be based on the > > > misunderstanding that the TPM interrupts when a locality is > > > released: it doesn't, only when the locality is acquired. > > > > > > Furthermore, there seems to be no point in waiting for the locality > > > to be released. All it does is penalize the last TPM > > > user. However, if there's no next TPM user, this is a pointless > > > wait and if there is a next TPM user, they'll pay the penalty > > > waiting for the new locality (or possibly not if it's the same as > > > the old locality). > > > > > > Fix the code by making release_locality as simple write to release > > > with no waiting for completion. > > > > > > Fixes: 33bafe90824b ("tpm_tis: verify locality released before > > > returning from release_locality") > > > Signed-off-by: James Bottomley < > > > James.Bottomley@HansenPartnership.com> > > > > So, if I got it right this is dependent on 1/5 to address Jerry's > > issue? I.e. if this has a fixes tag and previous does not, it will > > not fully fix the situation when backporting? > > Yes, exactly. Technically 1/5 isn't really fixing anything at all, > it's changing from the current fix where we wait for the locality to be > released at the back end of a TIS TPM operation to a new fix where we > correctly check the conditions in the locality acquisition. After the > new fix is done, we can eliminate all the wait code in locality > release. > > James OK, ignore my changelog etc. cosmectic comments unless there is need for another revision. I will add the necessary tags. I'm holding with reviewed-by up until Jerry can get ack for these changes. If he ack's, the it's all good as far as I'm concerned. /Jarkko
James Bottomley @ 2020-10-01 11:09 MST: > The current release locality code seems to be based on the > misunderstanding that the TPM interrupts when a locality is released: > it doesn't, only when the locality is acquired. > > Furthermore, there seems to be no point in waiting for the locality to > be released. All it does is penalize the last TPM user. However, if > there's no next TPM user, this is a pointless wait and if there is a > next TPM user, they'll pay the penalty waiting for the new locality > (or possibly not if it's the same as the old locality). > > Fix the code by making release_locality as simple write to release > with no waiting for completion. > > Fixes: 33bafe90824b ("tpm_tis: verify locality released before returning from release_locality") > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > --- > > v2: added fixes > --- > drivers/char/tpm/tpm_tis_core.c | 47 +-------------------------------- > 1 file changed, 1 insertion(+), 46 deletions(-) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index f3ecde8df47d..431919d5f48a 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -135,58 +135,13 @@ static bool check_locality(struct tpm_chip *chip, int l) > return false; > } > > -static bool locality_inactive(struct tpm_chip *chip, int l) > -{ > - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > - int rc; > - u8 access; > - > - rc = tpm_tis_read8(priv, TPM_ACCESS(l), &access); > - if (rc < 0) > - return false; > - > - if ((access & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY)) > - == TPM_ACCESS_VALID) > - return true; > - > - return false; > -} > - > static int release_locality(struct tpm_chip *chip, int l) > { > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > - unsigned long stop, timeout; > - long rc; > > tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > > - stop = jiffies + chip->timeout_a; > - > - if (chip->flags & TPM_CHIP_FLAG_IRQ) { > -again: > - timeout = stop - jiffies; > - if ((long)timeout <= 0) > - return -1; > - > - rc = wait_event_interruptible_timeout(priv->int_queue, > - (locality_inactive(chip, l)), > - timeout); > - > - if (rc > 0) > - return 0; > - > - if (rc == -ERESTARTSYS && freezing(current)) { > - clear_thread_flag(TIF_SIGPENDING); > - goto again; > - } > - } else { > - do { > - if (locality_inactive(chip, l)) > - return 0; > - tpm_msleep(TPM_TIMEOUT); > - } while (time_before(jiffies, stop)); > - } > - return -1; > + return 0; > } > > static int request_locality(struct tpm_chip *chip, int l) Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Mon, Oct 19, 2020 at 04:17:59PM -0700, Jerry Snitselaar wrote: > > James Bottomley @ 2020-10-01 11:09 MST: > > > The current release locality code seems to be based on the > > misunderstanding that the TPM interrupts when a locality is released: > > it doesn't, only when the locality is acquired. > > > > Furthermore, there seems to be no point in waiting for the locality to > > be released. All it does is penalize the last TPM user. However, if > > there's no next TPM user, this is a pointless wait and if there is a > > next TPM user, they'll pay the penalty waiting for the new locality > > (or possibly not if it's the same as the old locality). > > > > Fix the code by making release_locality as simple write to release > > with no waiting for completion. > > > > Fixes: 33bafe90824b ("tpm_tis: verify locality released before returning from release_locality") > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > --- > > > > v2: added fixes > > --- > > drivers/char/tpm/tpm_tis_core.c | 47 +-------------------------------- > > 1 file changed, 1 insertion(+), 46 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > > index f3ecde8df47d..431919d5f48a 100644 > > --- a/drivers/char/tpm/tpm_tis_core.c > > +++ b/drivers/char/tpm/tpm_tis_core.c > > @@ -135,58 +135,13 @@ static bool check_locality(struct tpm_chip *chip, int l) > > return false; > > } > > > > -static bool locality_inactive(struct tpm_chip *chip, int l) > > -{ > > - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > - int rc; > > - u8 access; > > - > > - rc = tpm_tis_read8(priv, TPM_ACCESS(l), &access); > > - if (rc < 0) > > - return false; > > - > > - if ((access & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY)) > > - == TPM_ACCESS_VALID) > > - return true; > > - > > - return false; > > -} > > - > > static int release_locality(struct tpm_chip *chip, int l) > > { > > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > - unsigned long stop, timeout; > > - long rc; > > > > tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > > > > - stop = jiffies + chip->timeout_a; > > - > > - if (chip->flags & TPM_CHIP_FLAG_IRQ) { > > -again: > > - timeout = stop - jiffies; > > - if ((long)timeout <= 0) > > - return -1; > > - > > - rc = wait_event_interruptible_timeout(priv->int_queue, > > - (locality_inactive(chip, l)), > > - timeout); > > - > > - if (rc > 0) > > - return 0; > > - > > - if (rc == -ERESTARTSYS && freezing(current)) { > > - clear_thread_flag(TIF_SIGPENDING); > > - goto again; > > - } > > - } else { > > - do { > > - if (locality_inactive(chip, l)) > > - return 0; > > - tpm_msleep(TPM_TIMEOUT); > > - } while (time_before(jiffies, stop)); > > - } > > - return -1; > > + return 0; > > } > > > > static int request_locality(struct tpm_chip *chip, int l) > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> /Jarkko
On Sat, Oct 24, 2020 at 03:10:07PM +0300, Jarkko Sakkinen wrote: > On Mon, Oct 19, 2020 at 04:17:59PM -0700, Jerry Snitselaar wrote: > > > > James Bottomley @ 2020-10-01 11:09 MST: > > > > > The current release locality code seems to be based on the > > > misunderstanding that the TPM interrupts when a locality is released: > > > it doesn't, only when the locality is acquired. > > > > > > Furthermore, there seems to be no point in waiting for the locality to > > > be released. All it does is penalize the last TPM user. However, if > > > there's no next TPM user, this is a pointless wait and if there is a > > > next TPM user, they'll pay the penalty waiting for the new locality > > > (or possibly not if it's the same as the old locality). > > > > > > Fix the code by making release_locality as simple write to release > > > with no waiting for completion. > > > > > > Fixes: 33bafe90824b ("tpm_tis: verify locality released before returning from release_locality") > > > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> > > > > > > --- > > > > > > v2: added fixes > > > --- > > > drivers/char/tpm/tpm_tis_core.c | 47 +-------------------------------- > > > 1 file changed, 1 insertion(+), 46 deletions(-) > > > > > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > > > index f3ecde8df47d..431919d5f48a 100644 > > > --- a/drivers/char/tpm/tpm_tis_core.c > > > +++ b/drivers/char/tpm/tpm_tis_core.c > > > @@ -135,58 +135,13 @@ static bool check_locality(struct tpm_chip *chip, int l) > > > return false; > > > } > > > > > > -static bool locality_inactive(struct tpm_chip *chip, int l) > > > -{ > > > - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > > - int rc; > > > - u8 access; > > > - > > > - rc = tpm_tis_read8(priv, TPM_ACCESS(l), &access); > > > - if (rc < 0) > > > - return false; > > > - > > > - if ((access & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY)) > > > - == TPM_ACCESS_VALID) > > > - return true; > > > - > > > - return false; > > > -} > > > - > > > static int release_locality(struct tpm_chip *chip, int l) > > > { > > > struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > > > - unsigned long stop, timeout; > > > - long rc; > > > > > > tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); > > > > > > - stop = jiffies + chip->timeout_a; > > > - > > > - if (chip->flags & TPM_CHIP_FLAG_IRQ) { > > > -again: > > > - timeout = stop - jiffies; > > > - if ((long)timeout <= 0) > > > - return -1; > > > - > > > - rc = wait_event_interruptible_timeout(priv->int_queue, > > > - (locality_inactive(chip, l)), > > > - timeout); > > > - > > > - if (rc > 0) > > > - return 0; > > > - > > > - if (rc == -ERESTARTSYS && freezing(current)) { > > > - clear_thread_flag(TIF_SIGPENDING); > > > - goto again; > > > - } > > > - } else { > > > - do { > > > - if (locality_inactive(chip, l)) > > > - return 0; > > > - tpm_msleep(TPM_TIMEOUT); > > > - } while (time_before(jiffies, stop)); > > > - } > > > - return -1; > > > + return 0; > > > } > > > > > > static int request_locality(struct tpm_chip *chip, int l) > > > > Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com> > > Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org> Just noticed that the short summary is wrong: a fix is not a clean up. Maybe "tpm_tis: Invoke locality release without wait" ? I'm also open for other suggestions but the current is short summary does not describe the patch. /Jarkko
On Fri, 2020-10-30 at 14:17 +0200, Jarkko Sakkinen wrote: [...] > Just noticed that the short summary is wrong: a fix is not a clean > up. I don't really think this is a fix. What we currently have is working, just suboptimally. This makes it work optimally, which is better, but it's not fixing anything because apart from a speed up in operations there will be no user visible change. If we use the word "fix" it will excite all the automatic patch selectors for stable. > Maybe "tpm_tis: Invoke locality release without wait" ? I'm also open > for other suggestions but the current is short summary does not > describe the patch. That sounds fine, not having fix in the subject works for me. James
On Fri, Oct 30, 2020 at 08:47:30AM -0700, James Bottomley wrote: > On Fri, 2020-10-30 at 14:17 +0200, Jarkko Sakkinen wrote: > [...] > > Just noticed that the short summary is wrong: a fix is not a clean > > up. > > I don't really think this is a fix. What we currently have is working, > just suboptimally. This makes it work optimally, which is better, but > it's not fixing anything because apart from a speed up in operations > there will be no user visible change. If we use the word "fix" it will > excite all the automatic patch selectors for stable. > > > Maybe "tpm_tis: Invoke locality release without wait" ? I'm also open > > for other suggestions but the current is short summary does not > > describe the patch. > > That sounds fine, not having fix in the subject works for me. > > James OK, great. /Jarkko
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index f3ecde8df47d..431919d5f48a 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -135,58 +135,13 @@ static bool check_locality(struct tpm_chip *chip, int l) return false; } -static bool locality_inactive(struct tpm_chip *chip, int l) -{ - struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); - int rc; - u8 access; - - rc = tpm_tis_read8(priv, TPM_ACCESS(l), &access); - if (rc < 0) - return false; - - if ((access & (TPM_ACCESS_VALID | TPM_ACCESS_ACTIVE_LOCALITY)) - == TPM_ACCESS_VALID) - return true; - - return false; -} - static int release_locality(struct tpm_chip *chip, int l) { struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); - unsigned long stop, timeout; - long rc; tpm_tis_write8(priv, TPM_ACCESS(l), TPM_ACCESS_ACTIVE_LOCALITY); - stop = jiffies + chip->timeout_a; - - if (chip->flags & TPM_CHIP_FLAG_IRQ) { -again: - timeout = stop - jiffies; - if ((long)timeout <= 0) - return -1; - - rc = wait_event_interruptible_timeout(priv->int_queue, - (locality_inactive(chip, l)), - timeout); - - if (rc > 0) - return 0; - - if (rc == -ERESTARTSYS && freezing(current)) { - clear_thread_flag(TIF_SIGPENDING); - goto again; - } - } else { - do { - if (locality_inactive(chip, l)) - return 0; - tpm_msleep(TPM_TIMEOUT); - } while (time_before(jiffies, stop)); - } - return -1; + return 0; } static int request_locality(struct tpm_chip *chip, int l)
The current release locality code seems to be based on the misunderstanding that the TPM interrupts when a locality is released: it doesn't, only when the locality is acquired. Furthermore, there seems to be no point in waiting for the locality to be released. All it does is penalize the last TPM user. However, if there's no next TPM user, this is a pointless wait and if there is a next TPM user, they'll pay the penalty waiting for the new locality (or possibly not if it's the same as the old locality). Fix the code by making release_locality as simple write to release with no waiting for completion. Fixes: 33bafe90824b ("tpm_tis: verify locality released before returning from release_locality") Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com> --- v2: added fixes --- drivers/char/tpm/tpm_tis_core.c | 47 +-------------------------------- 1 file changed, 1 insertion(+), 46 deletions(-)