lkml.org 
[lkml]   [2021]   [Jan]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v2] tpm_tis: Add missing tpm_request/relinquish_locality calls
    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
    >

    \
     
     \ /
      Last update: 2021-01-28 18:40    [W:4.150 / U:0.120 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site