| Message ID | 20210128130753.1283534-1-lma@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] tpm_tis: Add missing tpm_request/relinquish_locality calls | expand |
Hi Lukasz, On Thu, Jan 28, 2021 at 02:07:53PM +0100, Lukasz Majczak wrote: > There is a missing call to tpm_request_locality before the call to > the tpm_get_timeouts() and tpm_tis_probe_irq_single(). As the current > approach might work for tpm2, it fails for tpm1.x - in that case > call to tpm_get_timeouts() or tpm_tis_probe_irq_single() > without locality fails and in turn causes tpm_tis_core_init() to fail. > Tested on Samsung Chromebook Pro (Caroline). > > Signed-off-by: Lukasz Majczak <lma@semihalf.com> > --- > Jarkko, James, Guenter > > I’m aware about the other thread, but it seems to be dead for a few months. > Here is the small patch as fixing this specific issue > would allow us to unblock the ChromeOs development. > We want to upstream all of our patches, > so the ChromeOs will not diverge even more, > so I'm hoping this could be applied, if you see it neat enough. > > Best regards, > Lukasz > > v1 -> v2: > - fixed typos > - as there is no need to enable clock, switched to > use only tpm_request/relinquish_locality calls > - narrowed down boundaries of tpm_request/relinquish_locality calls > > drivers/char/tpm/tpm-chip.c | 4 ++-- > drivers/char/tpm/tpm-interface.c | 11 +++++++++-- > drivers/char/tpm/tpm.h | 2 ++ > drivers/char/tpm/tpm_tis_core.c | 12 ++++++++++-- > 4 files changed, 23 insertions(+), 6 deletions(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index ddaeceb7e109..5351963a4b19 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -32,7 +32,7 @@ struct class *tpm_class; > struct class *tpmrm_class; > dev_t tpm_devt; > > -static int tpm_request_locality(struct tpm_chip *chip) > +int tpm_request_locality(struct tpm_chip *chip) > { > int rc; > > @@ -47,7 +47,7 @@ static int tpm_request_locality(struct tpm_chip *chip) > return 0; > } > > -static void tpm_relinquish_locality(struct tpm_chip *chip) > +void tpm_relinquish_locality(struct tpm_chip *chip) > { > int rc; > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index 1621ce818705..69309b2bea6a 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -243,8 +243,15 @@ int tpm_get_timeouts(struct tpm_chip *chip) > > if (chip->flags & TPM_CHIP_FLAG_TPM2) > return tpm2_get_timeouts(chip); > - else > - return tpm1_get_timeouts(chip); > + else { { } needed around if part of if/else statement. > + ssize_t ret = tpm_request_locality(chip); > + > + if (ret) > + return ret; > + ret = tpm1_get_timeouts(chip); > + tpm_relinquish_locality(chip); > + return ret; > + } > } > EXPORT_SYMBOL_GPL(tpm_get_timeouts); > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 947d1db0a5cc..8c13008437dd 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -193,6 +193,8 @@ static inline void tpm_msleep(unsigned int delay_msec) > > int tpm_chip_start(struct tpm_chip *chip); > void tpm_chip_stop(struct tpm_chip *chip); > +int tpm_request_locality(struct tpm_chip *chip); > +void tpm_relinquish_locality(struct tpm_chip *chip); > struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip); > __must_check int tpm_try_get_ops(struct tpm_chip *chip); > void tpm_put_ops(struct tpm_chip *chip); > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index 92c51c6cfd1b..0ae675e8cf2f 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -754,9 +754,17 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) > > if (chip->flags & TPM_CHIP_FLAG_TPM2) > return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc); > - else > - return tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, > + else { > + ssize_t ret = tpm_request_locality(chip); > + > + if (ret) > + return ret; > + ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, > 0); > + tpm_relinquish_locality(chip); > + return ret; > + } > + James' earlier patch does not address the problem in tpm_get_timeouts(). However, it states that "interrupt registers are only writeable in the current locality" and "TPM_CHIP_FLAG_IRQ never gets set initially". So the question is if the above is sufficient, or if James' patch (requesting locality for all of tpm_tis_probe_irq_single and setting TPM_CHIP_FLAG_IRQ) is needed. Unfortunately I can not answer that. Guenter > } > > /* Register the IRQ and issue a command that will cause an interrupt. If an > -- > 2.25.1 >
On Thu, 2021-01-28 at 14:07 +0100, Lukasz Majczak wrote: > There is a missing call to tpm_request_locality before the call to > the tpm_get_timeouts() and tpm_tis_probe_irq_single(). As the current > approach might work for tpm2, it fails for tpm1.x - in that case > call to tpm_get_timeouts() or tpm_tis_probe_irq_single() > without locality fails and in turn causes tpm_tis_core_init() to fail. > Tested on Samsung Chromebook Pro (Caroline). > > Signed-off-by: Lukasz Majczak <lma@semihalf.com> Is it possible that you test against linux-next and see if any problems still arise? I've applied the locality fixes from James. > --- > Jarkko, James, Guenter > > I’m aware about the other thread, but it seems to be dead for a few months. > Here is the small patch as fixing this specific issue > would allow us to unblock the ChromeOs development. > We want to upstream all of our patches, > so the ChromeOs will not diverge even more, > so I'm hoping this could be applied, if you see it neat enough. The usual approach is that you construct a series picking the pre-existing fixes and on top of that create your own, if any required. > Best regards, > Lukasz /Jarkko > > v1 -> v2: > - fixed typos > - as there is no need to enable clock, switched to > use only tpm_request/relinquish_locality calls > - narrowed down boundaries of tpm_request/relinquish_locality calls > > drivers/char/tpm/tpm-chip.c | 4 ++-- > drivers/char/tpm/tpm-interface.c | 11 +++++++++-- > drivers/char/tpm/tpm.h | 2 ++ > drivers/char/tpm/tpm_tis_core.c | 12 ++++++++++-- > 4 files changed, 23 insertions(+), 6 deletions(-) > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c > index ddaeceb7e109..5351963a4b19 100644 > --- a/drivers/char/tpm/tpm-chip.c > +++ b/drivers/char/tpm/tpm-chip.c > @@ -32,7 +32,7 @@ struct class *tpm_class; > struct class *tpmrm_class; > dev_t tpm_devt; > > -static int tpm_request_locality(struct tpm_chip *chip) > +int tpm_request_locality(struct tpm_chip *chip) > { > int rc; > > @@ -47,7 +47,7 @@ static int tpm_request_locality(struct tpm_chip *chip) > return 0; > } > > -static void tpm_relinquish_locality(struct tpm_chip *chip) > +void tpm_relinquish_locality(struct tpm_chip *chip) > { > int rc; > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c > index 1621ce818705..69309b2bea6a 100644 > --- a/drivers/char/tpm/tpm-interface.c > +++ b/drivers/char/tpm/tpm-interface.c > @@ -243,8 +243,15 @@ int tpm_get_timeouts(struct tpm_chip *chip) > > if (chip->flags & TPM_CHIP_FLAG_TPM2) > return tpm2_get_timeouts(chip); > - else > - return tpm1_get_timeouts(chip); > + else { > + ssize_t ret = tpm_request_locality(chip); > + > + if (ret) > + return ret; > + ret = tpm1_get_timeouts(chip); > + tpm_relinquish_locality(chip); > + return ret; > + } > } > EXPORT_SYMBOL_GPL(tpm_get_timeouts); > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 947d1db0a5cc..8c13008437dd 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -193,6 +193,8 @@ static inline void tpm_msleep(unsigned int delay_msec) > > int tpm_chip_start(struct tpm_chip *chip); > void tpm_chip_stop(struct tpm_chip *chip); > +int tpm_request_locality(struct tpm_chip *chip); > +void tpm_relinquish_locality(struct tpm_chip *chip); > struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip); > __must_check int tpm_try_get_ops(struct tpm_chip *chip); > void tpm_put_ops(struct tpm_chip *chip); > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index 92c51c6cfd1b..0ae675e8cf2f 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -754,9 +754,17 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) > > if (chip->flags & TPM_CHIP_FLAG_TPM2) > return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc); > - else > - return tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, > + else { > + ssize_t ret = tpm_request_locality(chip); > + > + if (ret) > + return ret; > + ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, > 0); > + tpm_relinquish_locality(chip); > + return ret; > + } > + > } > > /* Register the IRQ and issue a command that will cause an interrupt. If an
On Sat, 2021-01-30 at 22:40 +0200, Jarkko Sakkinen wrote: > On Thu, 2021-01-28 at 14:07 +0100, Lukasz Majczak wrote: > > There is a missing call to tpm_request_locality before the call to > > the tpm_get_timeouts() and tpm_tis_probe_irq_single(). As the current > > approach might work for tpm2, it fails for tpm1.x - in that case > > call to tpm_get_timeouts() or tpm_tis_probe_irq_single() > > without locality fails and in turn causes tpm_tis_core_init() to fail. > > Tested on Samsung Chromebook Pro (Caroline). > > > > Signed-off-by: Lukasz Majczak <lma@semihalf.com> > > Is it possible that you test against linux-next and see if any > problems still arise? I've applied the locality fixes from James. Also one nit: if any problems still persist, please add a snippet from klog to your commit message. /Jarkko
On Sun, Jan 31, 2021 at 10:43:05AM +0100, Dirk Gouders wrote: > Jarkko Sakkinen <jarkko@kernel.org> writes: > > > On Thu, 2021-01-28 at 14:07 +0100, Lukasz Majczak wrote: > >> There is a missing call to tpm_request_locality before the call to > >> the tpm_get_timeouts() and tpm_tis_probe_irq_single(). As the current > >> approach might work for tpm2, it fails for tpm1.x - in that case > >> call to tpm_get_timeouts() or tpm_tis_probe_irq_single() > >> without locality fails and in turn causes tpm_tis_core_init() to fail. > >> Tested on Samsung Chromebook Pro (Caroline). > >> > >> Signed-off-by: Lukasz Majczak <lma@semihalf.com> > > > > Is it possible that you test against linux-next and see if any > > problems still arise? I've applied the locality fixes from James. > > I tested current linux-next and the warning still appears, > unfortunately. > > I then incrementally applied further patches from James' series [1] and > after "[PATCH v2 4/5] tpm_tis: fix IRQ probing" the warning has gone: > > # dmesg | grep tpm > [ 7.220410] tpm_tis STM0125:00: 2.0 TPM (device-id 0x0, rev-id 78) > [ 7.322564] Modules linked in: iwlmvm(+) btusb btrtl btbcm btintel mac80211 amdgpu(+) iwlwifi drm_ttm_helper tpm_crb sdhci_pci ttm cqhci gpu_sched sdhci ccp cfg80211 rng_core tpm_tis tpm_tis_core tpm thinkpad_acpi(+) wmi nvram pinctrl_amd > > You might notice there is another warning but that is rtc related and I > still have to find out if that is something I should report. 1/5 and 2/5 are on the other hand unconditionally "right things" to do. In your case the resulting state will anyway just result a polling driver and the kernel state remains stable. WARN_ONCE() is clearly an overkill, as kernel does not have inconsistent state. Using WARN_ONCE() was a mistake. I sent this fix: https://lore.kernel.org/linux-integrity/3936843b-c0da-dd8c-8aa9-90aa3b49d525@linux.ibm.com/T/#t It will turn WARN_ONCE() to pr_warn_once() and also add the status code to klog entry. It's sufficient in this case. For 3/5-5/5 we need more time. /Jarkko
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index ddaeceb7e109..5351963a4b19 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -32,7 +32,7 @@ struct class *tpm_class; struct class *tpmrm_class; dev_t tpm_devt; -static int tpm_request_locality(struct tpm_chip *chip) +int tpm_request_locality(struct tpm_chip *chip) { int rc; @@ -47,7 +47,7 @@ static int tpm_request_locality(struct tpm_chip *chip) return 0; } -static void tpm_relinquish_locality(struct tpm_chip *chip) +void tpm_relinquish_locality(struct tpm_chip *chip) { int rc; diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 1621ce818705..69309b2bea6a 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -243,8 +243,15 @@ int tpm_get_timeouts(struct tpm_chip *chip) if (chip->flags & TPM_CHIP_FLAG_TPM2) return tpm2_get_timeouts(chip); - else - return tpm1_get_timeouts(chip); + else { + ssize_t ret = tpm_request_locality(chip); + + if (ret) + return ret; + ret = tpm1_get_timeouts(chip); + tpm_relinquish_locality(chip); + return ret; + } } EXPORT_SYMBOL_GPL(tpm_get_timeouts); diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 947d1db0a5cc..8c13008437dd 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -193,6 +193,8 @@ static inline void tpm_msleep(unsigned int delay_msec) int tpm_chip_start(struct tpm_chip *chip); void tpm_chip_stop(struct tpm_chip *chip); +int tpm_request_locality(struct tpm_chip *chip); +void tpm_relinquish_locality(struct tpm_chip *chip); struct tpm_chip *tpm_find_get_ops(struct tpm_chip *chip); __must_check int tpm_try_get_ops(struct tpm_chip *chip); void tpm_put_ops(struct tpm_chip *chip); diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index 92c51c6cfd1b..0ae675e8cf2f 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -754,9 +754,17 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip) if (chip->flags & TPM_CHIP_FLAG_TPM2) return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc); - else - return tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, + else { + ssize_t ret = tpm_request_locality(chip); + + if (ret) + return ret; + ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0); + tpm_relinquish_locality(chip); + return ret; + } + } /* Register the IRQ and issue a command that will cause an interrupt. If an
There is a missing call to tpm_request_locality before the call to the tpm_get_timeouts() and tpm_tis_probe_irq_single(). As the current approach might work for tpm2, it fails for tpm1.x - in that case call to tpm_get_timeouts() or tpm_tis_probe_irq_single() without locality fails and in turn causes tpm_tis_core_init() to fail. Tested on Samsung Chromebook Pro (Caroline). Signed-off-by: Lukasz Majczak <lma@semihalf.com> --- Jarkko, James, Guenter I’m aware about the other thread, but it seems to be dead for a few months. Here is the small patch as fixing this specific issue would allow us to unblock the ChromeOs development. We want to upstream all of our patches, so the ChromeOs will not diverge even more, so I'm hoping this could be applied, if you see it neat enough. Best regards, Lukasz v1 -> v2: - fixed typos - as there is no need to enable clock, switched to use only tpm_request/relinquish_locality calls - narrowed down boundaries of tpm_request/relinquish_locality calls drivers/char/tpm/tpm-chip.c | 4 ++-- drivers/char/tpm/tpm-interface.c | 11 +++++++++-- drivers/char/tpm/tpm.h | 2 ++ drivers/char/tpm/tpm_tis_core.c | 12 ++++++++++-- 4 files changed, 23 insertions(+), 6 deletions(-)