Re: [PATCH 2/2] tpm: Revert "tpm_tis_core: Turn on the TPM before probing IRQ's"

[Jarkko Sakkinen]

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