Re: [PATCH v3 2/4] tpm: Simplify locality handling

[Jarkko Sakkinen]

What the heck is "simplification" and what that has to do with fixing
anything? I don't understand your terminology. 

On Sat, May 01, 2021 at 03:57:25PM +0200, Lino Sanfilippo wrote:
> Currently the TPM (default) locality is claimed and released for each
> access to the TPM registers which require a claimed locality. This results
> in locality claim/release combos at various code places. For interrupt
> handling we also need such a combo in the interrupt handler (for clearing
> the interrupts) which makes the locality handling even more complicated
> since now we also have to synchronize concurrent accesses in process and in
> interrupt context.
> 
> Since currently the driver uses only one locality anyway, avoid the
> increasing complexity by claiming it once at driver startup and only
> releasing it at driver shutdown.
> 
> Due to the simplifications the functions tpm_request_locality() and
> tpm_relinquish_locality() are not needed any more an can be removed.
> 
> As a side-effect these modifications fix a bug which results in the
> following warning when using TPM 2:
> 
> TPM returned invalid status
> WARNING: CPU: 0 PID: 874 at drivers/char/tpm/tpm_tis_core.c:249 tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
> Modules linked in: tpm_tis_spi tpm_tis_core tpm sha256_generic cfg80211 rfkill 8021q garp stp llc spidev v3d gpu_sched vc4 bcm2835_codec(C) cec raspberrypi_hwmon drm_kms_helper drm bcm2835_isp(C) v4l2_mem2mem bcm2835_v4l2(C) bcm2835_mmal_vchiq(C) videobuf2_vmalloc videobuf2_dma_contig snd_bcm2835(C) videobuf2_memops drm_panel_orientation_quirks videobuf2_v4l2 videobuf2_common snd_soc_core snd_compress videodev snd_pcm_dmaengine spi_bcm2835 snd_pcm mc vc_sm_cma(C) snd_timer snd syscopyarea rpivid_mem sysfillrect sysimgblt fb_sys_fops backlight uio_pdrv_genirq uio ip_tables x_tables ipv6 [last unloaded: tpm]
> CPU: 0 PID: 874 Comm: kworker/u8:1 Tainted: G        WC        5.11.0-rc2-LS-HOME+ #1
> Hardware name: Raspberry Pi Compute Module 4 Rev 1.0 (DT)
> Workqueue: events_unbound async_run_entry_fn
> pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> pc : tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
> lr : tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
> sp : ffffffc0127e38f0
> x29: ffffffc0127e38f0 x28: ffffffc011238000
> x27: 0000000000000016 x26: 000000000000017a
> x25: 0000000000000014 x24: ffffff8047f60000
> x23: 0000000000000000 x22: 0000000000000016
> x21: ffffff8044e8a480 x20: 0000000000000000
> x19: ffffffc011238000 x18: ffffffc011238948
> x17: 0000000000000000 x16: 0000000000000000
> x15: ffffffc01141be81 x14: ffffffffffffffff
> x13: ffffffc01141be7e x12: ffffffffffffffff
> x11: ffffff807fb797f0 x10: 00000000000019b0
> x9 : ffffffc0127e38f0 x8 : 766e692064656e72
> x7 : 0000000000000000 x6 : ffffffc011239000
> x5 : ffffff807fb628b8 x4 : 0000000000000000
> x3 : 0000000000000027 x2 : 0000000000000000
> x1 : 6818b6f22fdef800 x0 : 0000000000000000
> Call trace:
> tpm_tis_status+0xbc/0xc8 [tpm_tis_core]
> tpm_tis_send_data+0x58/0x250 [tpm_tis_core]
> tpm_tis_send_main+0x50/0x128 [tpm_tis_core]
> tpm_tis_send+0x4c/0xe0 [tpm_tis_core]
> tpm_transmit+0xd0/0x350 [tpm]
> tpm_transmit_cmd+0x3c/0xc0 [tpm]
> tpm2_get_tpm_pt+0x124/0x1e8 [tpm]
> tpm_tis_probe_irq_single+0x198/0x364 [tpm_tis_core]
> tpm_tis_core_init+0x304/0x520 [tpm_tis_core]
> tpm_tis_spi_init+0x5c/0x78 [tpm_tis_spi]
> tpm_tis_spi_probe+0x80/0x98 [tpm_tis_spi]
> tpm_tis_spi_driver_probe+0x4c/0x60 [tpm_tis_spi]
> spi_probe+0x84/0xf0
> really_probe+0x118/0x420
> driver_probe_device+0x5c/0xc0
> __driver_attach_async_helper+0x64/0x68
> async_run_entry_fn+0x48/0x150
> process_one_work+0x15c/0x4d0
> worker_thread+0x50/0x490
> kthread+0x118/0x150
> ret_from_fork+0x10/0x1c
> 
> The reason for this issue is that in case of TPM 2 function
> tpm2_get_timeouts() which executes a TPM command is called without a
> claimed locality. Since with this patch the locality is taken once at
> driver startup and never released before shutdown the issue does not occur
> any more.

Please rather create fix that fixes the issue exactly in the code path.
I don't want to worry what other things this might do "as a side-effect".

Also, lacks the explanation why this patch fixes the issue.

> Fixes: a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@xxxxxx>
> ---
>  drivers/char/tpm/tpm-chip.c     | 40 ---------------------------------
>  drivers/char/tpm/tpm_tis_core.c | 35 +++++++++--------------------
>  include/linux/tpm.h             |  3 ---
>  3 files changed, 10 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index ddaeceb7e109..a09b6523261e 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -32,35 +32,6 @@ struct class *tpm_class;
>  struct class *tpmrm_class;
>  dev_t tpm_devt;
>  
> -static int tpm_request_locality(struct tpm_chip *chip)
> -{
> -	int rc;
> -
> -	if (!chip->ops->request_locality)
> -		return 0;
> -
> -	rc = chip->ops->request_locality(chip, 0);
> -	if (rc < 0)
> -		return rc;
> -
> -	chip->locality = rc;
> -	return 0;
> -}
> -
> -static void tpm_relinquish_locality(struct tpm_chip *chip)
> -{
> -	int rc;
> -
> -	if (!chip->ops->relinquish_locality)
> -		return;
> -
> -	rc = chip->ops->relinquish_locality(chip, chip->locality);
> -	if (rc)
> -		dev_err(&chip->dev, "%s: : error %d\n", __func__, rc);
> -
> -	chip->locality = -1;
> -}
> -
>  static int tpm_cmd_ready(struct tpm_chip *chip)
>  {
>  	if (!chip->ops->cmd_ready)
> @@ -103,17 +74,8 @@ int tpm_chip_start(struct tpm_chip *chip)
>  
>  	tpm_clk_enable(chip);
>  
> -	if (chip->locality == -1) {
> -		ret = tpm_request_locality(chip);
> -		if (ret) {
> -			tpm_clk_disable(chip);
> -			return ret;
> -		}
> -	}
> -
>  	ret = tpm_cmd_ready(chip);
>  	if (ret) {
> -		tpm_relinquish_locality(chip);
>  		tpm_clk_disable(chip);
>  		return ret;
>  	}
> @@ -133,7 +95,6 @@ EXPORT_SYMBOL_GPL(tpm_chip_start);
>  void tpm_chip_stop(struct tpm_chip *chip)
>  {
>  	tpm_go_idle(chip);
> -	tpm_relinquish_locality(chip);
>  	tpm_clk_disable(chip);
>  }
>  EXPORT_SYMBOL_GPL(tpm_chip_stop);
> @@ -392,7 +353,6 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
>  		goto out;
>  	}
>  
> -	chip->locality = -1;
>  	return chip;
>  
>  out:
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index a12992ae2a3e..f892b1ae46f2 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -626,9 +626,6 @@ static int probe_itpm(struct tpm_chip *chip)
>  	if (vendor != TPM_VID_INTEL)
>  		return 0;
>  
> -	if (request_locality(chip, 0) != 0)
> -		return -EBUSY;
> -
>  	rc = tpm_tis_send_data(chip, cmd_getticks, len);
>  	if (rc == 0)
>  		goto out;
> @@ -647,7 +644,6 @@ static int probe_itpm(struct tpm_chip *chip)
>  
>  out:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, priv->locality);
>  
>  	return rc;
>  }
> @@ -707,22 +703,13 @@ static int tpm_tis_gen_interrupt(struct tpm_chip *chip)
>  	const char *desc = "attempting to generate an interrupt";
>  	u32 cap2;
>  	cap_t cap;
> -	int ret;
>  
>  	/* TPM 2.0 */
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		return tpm2_get_tpm_pt(chip, 0x100, &cap2, desc);
>  
>  	/* TPM 1.2 */
> -	ret = request_locality(chip, 0);
> -	if (ret < 0)
> -		return ret;
> -
> -	ret = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
> -
> -	release_locality(chip, 0);
> -
> -	return ret;
> +	return tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, desc, 0);
>  }
>  
>  /* Register the IRQ and issue a command that will cause an interrupt. If an
> @@ -836,6 +823,7 @@ void tpm_tis_remove(struct tpm_chip *chip)
>  
>  	tpm_tis_write32(priv, reg, ~TPM_GLOBAL_INT_ENABLE & interrupt);
>  
> +	release_locality(chip, 0);
>  	tpm_tis_clkrun_enable(chip, false);
>  
>  	if (priv->ilb_base_addr)
> @@ -963,6 +951,14 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		goto out_err;
>  	}
>  
> +	rc = request_locality(chip, 0);
> +	if (rc)
> +		goto out_err;
> +
> +	rc = tpm_chip_start(chip);
> +	if (rc)
> +		goto out_err;
> +
>  	/* Take control of the TPM's interrupt hardware and shut it off */
>  	rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
>  	if (rc < 0)
> @@ -973,9 +969,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  	intmask &= ~TPM_GLOBAL_INT_ENABLE;
>  	tpm_tis_write32(priv, TPM_INT_ENABLE(priv->locality), intmask);
>  
> -	rc = tpm_chip_start(chip);
> -	if (rc)
> -		goto out_err;
>  	rc = tpm2_probe(chip);
>  	tpm_chip_stop(chip);
>  	if (rc)
> @@ -1036,15 +1029,7 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		 * to make sure it works. May as well use that command to set the
>  		 * proper timeouts for the driver.
>  		 */
> -
> -		rc = request_locality(chip, 0);
> -		if (rc < 0)
> -			goto out_err;
> -
>  		rc = tpm_get_timeouts(chip);
> -
> -		release_locality(chip, 0);
> -
>  		if (rc) {
>  			dev_err(dev, "Could not get TPM timeouts and durations\n");
>  			rc = -ENODEV;
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index aa11fe323c56..7a68832b14bb 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -167,9 +167,6 @@ struct tpm_chip {
>  	u32 last_cc;
>  	u32 nr_commands;
>  	u32 *cc_attrs_tbl;
> -
> -	/* active locality */
> -	int locality;
>  };
>  
>  #define TPM_HEADER_SIZE		10
> -- 
> 2.31.1
> 

/Jarkko