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@xxxxxxxxxxxx>
> ---
> 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
>
