Re: [PATCH v5] tpm_tis: Add missing tpm_request/relinquish_locality() calls

[Jarkko Sakkinen]

On Fri, Feb 12, 2021 at 12:06:00PM +0100, Lukasz Majczak wrote:
> There are missing calls to tpm_request_locality() before the calls to
> the tpm_get_timeouts() and tpm_tis_probe_irq_single() - both functions
> internally send commands to the tpm using tpm_tis_send_data()
> which in turn, at the very beginning, calls the tpm_tis_status().
> This one tries to read TPM_STS register, what fails and propagates
> this error upward. The read fails due to lack of acquired locality,
> as it is described in
> TCG PC Client Platform TPM Profile (PTP) Specification,
> paragraph 6.1 FIFO Interface Locality Usage per Register,
> Table 39 Register Behavior Based on Locality Setting for FIFO
> - a read attempt to TPM_STS_x Registers returns 0xFF in case of lack
> of locality. The described situation manifests itself with
> the following warning trace:
> 
> [    4.324298] TPM returned invalid status
> [    4.324806] WARNING: CPU: 2 PID: 1 at drivers/char/tpm/tpm_tis_core.c:275 tpm_tis_status+0x86/0x8f

The commit message is has great description  of the background, but
it does not have description what the commit does. Please describe
this in imperative form, e.g. "Export tpm_request_locality() and ..."
and "Call tpm_request_locality() before ...". You get the idea.

It's also lacking expalanation of the implementation path, i.e.
why you are not using tpm_chip_start() and tpm_chip_stop().

> 
> Tested on Samsung Chromebook Pro (Caroline), TPM 1.2 (SLB 9670)

Empty line here.

Also, add:

Cc: stable@xxxxxxxxxxxxxxx

> Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")

Remove empty line.

> Signed-off-by: Lukasz Majczak <lma@xxxxxxxxxxxx>
> Reviewed-by: Guenter Roeck <linux@xxxxxxxxxxxx>


> ---
> 
> Hi
> 
> I have tried to clean all the pointed issues, but decided to stay with 
> tpm_request/relinquish_locality() calls instead of using tpm_chip_start/stop(),
> the rationale behind this is that, in this case only locality is requested, there
> is no need to enable/disable the clock, the similar case is present in
> the probe_itpm() function.

I would prefer to use the "same same" if it does not cause any extra harm
instead of new exports. That will also make the fix more compact. So don't
agree with this reasoning. Also the commit message lacks *any* reasoning.

> One more clarification is that, the TPM present on my test machine is the SLB 9670
> (not Cr50).
> 
> Best regards,
> Lukasz
> 
> Changes:
> v4->v5:
> * Fixed style, typos, clarified commit message
> 
>  drivers/char/tpm/tpm-chip.c      |  6 ++++--
>  drivers/char/tpm/tpm-interface.c | 13 ++++++++++---
>  drivers/char/tpm/tpm.h           |  2 ++
>  drivers/char/tpm/tpm_tis_core.c  | 14 +++++++++++---
>  4 files changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index ddaeceb7e109..ce9c2650fbe5 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;
>  
> @@ -46,8 +46,9 @@ static int tpm_request_locality(struct tpm_chip *chip)
>  	chip->locality = rc;
>  	return 0;
>  }
> +EXPORT_SYMBOL_GPL(tpm_request_locality);
>  
> -static void tpm_relinquish_locality(struct tpm_chip *chip)
> +void tpm_relinquish_locality(struct tpm_chip *chip)
>  {
>  	int rc;
>  
> @@ -60,6 +61,7 @@ static void tpm_relinquish_locality(struct tpm_chip *chip)
>  
>  	chip->locality = -1;
>  }
> +EXPORT_SYMBOL_GPL(tpm_relinquish_locality);
>  
>  static int tpm_cmd_ready(struct tpm_chip *chip)
>  {
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 1621ce818705..2a9001d329f2 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -241,10 +241,17 @@ int tpm_get_timeouts(struct tpm_chip *chip)
>  	if (chip->flags & TPM_CHIP_FLAG_HAVE_TIMEOUTS)
>  		return 0;
>  
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +	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 431919d5f48a..d4f381d6356e 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -708,11 +708,19 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
>  	u32 cap2;
>  	cap_t cap;
>  
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +	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
> -- 
> 2.25.1
> 
>