On Sun, Sep 27, 2020 at 01:06:03PM -0700, James Bottomley wrote:
> On Tue, 2019-11-26 at 08:17 -0500, Stefan Berger wrote:
> > From: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> >
> > Revert the patch that was turning the TPM on before probing for IRQs.
> >
> > Fixes: 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing
> > IRQ's")
> > Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
> > Reported-by: Jerry Snitselaar <jsnitsel@xxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> > drivers/char/tpm/tpm_tis_core.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_core.c
> > b/drivers/char/tpm/tpm_tis_core.c
> > index 5dc52c4e2292..27c6ca031e23 100644
> > --- a/drivers/char/tpm/tpm_tis_core.c
> > +++ b/drivers/char/tpm/tpm_tis_core.c
> > @@ -1059,7 +1059,6 @@ int tpm_tis_core_init(struct device *dev,
> > struct tpm_tis_data *priv, int irq,
> > goto out_err;
> > }
> >
> > - tpm_chip_start(chip);
> > if (irq) {
> > tpm_tis_probe_irq_single(chip, intmask,
> > IRQF_SHARED,
> > irq);
> > @@ -1069,7 +1068,6 @@ int tpm_tis_core_init(struct device *dev,
> > struct tpm_tis_data *priv, int irq,
> > } else {
> > tpm_tis_probe_irq(chip, intmask);
> > }
> > - tpm_chip_stop(chip);
> > }
> >
> > rc = tpm_chip_register(chip);
>
> This patch is completely bogus: it's not a full revert of what it
> claims to be. With this patch applied all my TIS TPMs are returning
> 0xff to the status reads because the locality hasn't been properly
> requested. The chip has to be started somewhere for the interrupt
> probe to work on these TPMs ... what the original patch did was
> eliminate a bunch of start/stops for a global one. However, if the
> global one isn't working we should have gone back to the bunch of
> smaller ones i.e. a full revert.
>
> The only real manifestation of the problems this patch causes is that
> interrupts never get enabled on TIS TPMs that have this issue, but they
> still work via polling.
>
> The below is what fixes this for me with the minimum possible extend of
> additional chip start/stop in the code. This should be checked against
> the previous failing laptops.
>
> James
>
> ---
>
> From: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> Subject: [PATCH] tpm_tis: fix interrupt probing
>
> When we send a command into the TPM core, the TPM must be started
> otherwise the register reads can be bogus. There have been several
> bug reports about doing this inside the TIS core, so fix the issue by
> adding an external version of the tpm2_get_tpm_pt() call which adds a
> tpm ops get/put to set up the TPM correctly before the command is
> sent.
>
> Fixes: aa4a63dd9816 (tpm: Revert "tpm_tis_core: Turn on the TPM before probing IRQ's")
> Signed-off-by: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> ---
> drivers/char/tpm/tpm.h | 2 ++
> drivers/char/tpm/tpm2-cmd.c | 30 ++++++++++++++++++++++++++++++
> drivers/char/tpm/tpm_tis_core.c | 2 +-
> 3 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 947d1db0a5cc..041b0b5bd2a5 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -223,6 +223,8 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
> ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
> u32 *value, const char *desc);
> +ssize_t tpm2_get_tpm_pt_cmd(struct tpm_chip *chip, u32 property_id,
> + u32 *value, const char *desc);
>
> ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
> int tpm2_auto_startup(struct tpm_chip *chip);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index eff1f12d981a..9b84158c5a9e 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -407,6 +407,36 @@ ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value,
> }
> EXPORT_SYMBOL_GPL(tpm2_get_tpm_pt);
>
> +/**
> + * tpm2_get_tpm_pt_cmd() - get value of a TPM_CAP_TPM_PROPERTIES type property
> + * @chip: a &tpm_chip instance
> + * @property_id: property ID.
> + * @value: output variable.
> + * @desc: passed to tpm_transmit_cmd()
> + *
> + * This calls the necessary tpm_try_get_ops()/tpm_put_ops() around
> + * tpm2_get_tpm_pt() and must be called where it is used stand alone
> + * outside the core code.
> + *
> + * Return:
> + * 0 on success,
> + * -errno or a TPM return code otherwise
> + */
> +ssize_t tpm2_get_tpm_pt_cmd(struct tpm_chip *chip, u32 property_id, u32 *value,
> + const char *desc)
> +{
> + ssize_t rc;
> +
> + rc = tpm_try_get_ops(chip);
> + if (rc)
> + return rc;
> + rc = tpm2_get_tpm_pt(chip, property_id, value, desc);
> + tpm_put_ops(chip);
> +
> + return rc;
> +}
> +EXPORT_SYMBOL_GPL(tpm2_get_tpm_pt_cmd);
Hi, same comment as for the other, i.e. rename the function that does
not have get/put_ops as __tpm2_get_tpm_pt(). Otherwise, fine. Thank
you.
/Jarkko
