Re: [PATCH 1/2 v4] tpm: Factor out common startup code

[Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Wed, Jun 29, 2016 at 10:52:38AM +0300, andrew zamansky wrote:
> From: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> 
> The TCG standard startup sequence (get timeouts, tpm startup, etc) for
> TPM and TPM2 chips is being open coded in many drivers, move it into
> the core code.
> 
> tpm_tis and tpm_crb are used as the basis for the core code
> implementation and the easy drivers are converted. In the process
> several small drivers bugs relating to error handling this flow
> are fixed.
> 
> For now the flag TPM_OPS_AUTO_STARTUP is optional to allow a staged
> driver roll out, but ultimately all drivers should use this flow and
> the flag removed. Some drivers still do not implement the startup
> sequence at all and will need to be tested with it enabled.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Tested-by: Andrew Zamansky <andrew.zamansky-KrzQf0k3Iz9BDgjK7y7TUQ@public.gmane.org>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Now this looks good to me. Just have to test this.

/Jarkko

> ---
>  drivers/char/tpm/st33zp24/st33zp24.c |  4 +---
>  drivers/char/tpm/tpm-chip.c          |  9 +++++++
>  drivers/char/tpm/tpm-interface.c     | 27 +++++++++++++++++++++
>  drivers/char/tpm/tpm.h               |  4 ++--
>  drivers/char/tpm/tpm2-cmd.c          | 46 ++++++++++++++++++++++++++++++++----
>  drivers/char/tpm/tpm_crb.c           | 10 +-------
>  drivers/char/tpm/tpm_i2c_atmel.c     |  6 +----
>  drivers/char/tpm/tpm_i2c_infineon.c  |  4 +---
>  drivers/char/tpm/tpm_i2c_nuvoton.c   |  7 +-----
>  drivers/char/tpm/tpm_tis_core.c      | 24 +------------------
>  drivers/char/tpm/tpm_vtpm_proxy.c    |  9 +------
>  include/linux/tpm.h                  |  5 ++++
>  12 files changed, 92 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/char/tpm/st33zp24/st33zp24.c b/drivers/char/tpm/st33zp24/st33zp24.c
> index a7c99a2..c2ee304 100644
> --- a/drivers/char/tpm/st33zp24/st33zp24.c
> +++ b/drivers/char/tpm/st33zp24/st33zp24.c
> @@ -505,6 +505,7 @@ static bool st33zp24_req_canceled(struct tpm_chip *chip, u8 status)
>  }
>  
>  static const struct tpm_class_ops st33zp24_tpm = {
> +	.flags = TPM_OPS_AUTO_STARTUP,
>  	.send = st33zp24_send,
>  	.recv = st33zp24_recv,
>  	.cancel = st33zp24_cancel,
> @@ -592,9 +593,6 @@ int st33zp24_probe(void *phy_id, const struct st33zp24_phy_ops *ops,
>  		tpm_gen_interrupt(chip);
>  	}
>  
> -	tpm_get_timeouts(chip);
> -	tpm_do_selftest(chip);
> -
>  	return tpm_chip_register(chip);
>  _tpm_clean_answer:
>  	dev_info(&chip->dev, "TPM initialization fail\n");
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 5a2f043..e595013 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -354,6 +354,15 @@ int tpm_chip_register(struct tpm_chip *chip)
>  {
>  	int rc;
>  
> +	if (chip->ops->flags & TPM_OPS_AUTO_STARTUP) {
> +		if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +			rc = tpm2_auto_startup(chip);
> +		else
> +			rc = tpm1_auto_startup(chip);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	rc = tpm1_chip_register(chip);
>  	if (rc)
>  		return rc;
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 5e3c1b6..1abe2d7 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -843,6 +843,33 @@ int tpm_do_selftest(struct tpm_chip *chip)
>  }
>  EXPORT_SYMBOL_GPL(tpm_do_selftest);
>  
> +/**
> + * tpm1_auto_startup - Perform the standard automatic TPM initialization
> + *                     sequence
> + * @chip: TPM chip to use
> + *
> + * Returns 0 on success, < 0 in case of fatal error.
> + */
> +int tpm1_auto_startup(struct tpm_chip *chip)
> +{
> +	int rc;
> +
> +	rc = tpm_get_timeouts(chip);
> +	if (rc)
> +		goto out;
> +	rc = tpm_do_selftest(chip);
> +	if (rc) {
> +		dev_err(&chip->dev, "TPM self test failed\n");
> +		goto out;
> +	}
> +
> +	return rc;
> +out:
> +	if (rc > 0)
> +		rc = -ENODEV;
> +	return rc;
> +}
> +
>  int tpm_send(u32 chip_num, void *cmd, size_t buflen)
>  {
>  	struct tpm_chip *chip;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 8890df2..3e32d5b 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -484,6 +484,7 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len,
>  			 const char *desc);
>  extern int tpm_get_timeouts(struct tpm_chip *);
>  extern void tpm_gen_interrupt(struct tpm_chip *);
> +int tpm1_auto_startup(struct tpm_chip *chip);
>  extern int tpm_do_selftest(struct tpm_chip *);
>  extern unsigned long tpm_calc_ordinal_duration(struct tpm_chip *, u32);
>  extern int tpm_pm_suspend(struct device *);
> @@ -526,10 +527,9 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
>  ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
>  			u32 *value, const char *desc);
>  
> -extern int tpm2_startup(struct tpm_chip *chip, u16 startup_type);
> +int tpm2_auto_startup(struct tpm_chip *chip);
>  extern void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
>  extern unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *, u32);
> -extern int tpm2_do_selftest(struct tpm_chip *chip);
>  extern int tpm2_gen_interrupt(struct tpm_chip *chip);
>  extern int tpm2_probe(struct tpm_chip *chip);
>  #endif
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index a1673dc..fe6d0d7 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -728,7 +728,7 @@ static const struct tpm_input_header tpm2_startup_header = {
>   * returned it remarks a POSIX error code. If a positive number is returned
>   * it remarks a TPM error.
>   */
> -int tpm2_startup(struct tpm_chip *chip, u16 startup_type)
> +static int tpm2_startup(struct tpm_chip *chip, u16 startup_type)
>  {
>  	struct tpm2_cmd cmd;
>  
> @@ -738,7 +738,6 @@ int tpm2_startup(struct tpm_chip *chip, u16 startup_type)
>  	return tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
>  				"attempting to start the TPM");
>  }
> -EXPORT_SYMBOL_GPL(tpm2_startup);
>  
>  #define TPM2_SHUTDOWN_IN_SIZE \
>  	(sizeof(struct tpm_input_header) + \
> @@ -854,7 +853,7 @@ static int tpm2_start_selftest(struct tpm_chip *chip, bool full)
>   * returned it remarks a POSIX error code. If a positive number is returned
>   * it remarks a TPM error.
>   */
> -int tpm2_do_selftest(struct tpm_chip *chip)
> +static int tpm2_do_selftest(struct tpm_chip *chip)
>  {
>  	int rc;
>  	unsigned int loops;
> @@ -894,7 +893,6 @@ int tpm2_do_selftest(struct tpm_chip *chip)
>  
>  	return rc;
>  }
> -EXPORT_SYMBOL_GPL(tpm2_do_selftest);
>  
>  /**
>   * tpm2_gen_interrupt() - generate an interrupt
> @@ -942,3 +940,43 @@ int tpm2_probe(struct tpm_chip *chip)
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(tpm2_probe);
> +
> +/**
> + * tpm2_auto_startup - Perform the standard automatic TPM initialization
> + *                     sequence
> + * @chip: TPM chip to use
> + *
> + * Returns 0 on success, < 0 in case of fatal error.
> + */
> +int tpm2_auto_startup(struct tpm_chip *chip)
> +{
> +	int rc;
> +
> +	rc = tpm_get_timeouts(chip);
> +	if (rc)
> +		goto out;
> +
> +	rc = tpm2_do_selftest(chip);
> +	if (rc != TPM2_RC_INITIALIZE) {
> +		dev_err(&chip->dev, "TPM self test failed\n");
> +		goto out;
> +	}
> +
> +	if (rc == TPM2_RC_INITIALIZE) {
> +		rc = tpm2_startup(chip, TPM2_SU_CLEAR);
> +		if (rc)
> +			goto out;
> +
> +		rc = tpm2_do_selftest(chip);
> +		if (rc) {
> +			dev_err(&chip->dev, "TPM self test failed\n");
> +			goto out;
> +		}
> +	}
> +
> +	return rc;
> +out:
> +	if (rc > 0)
> +		rc = -ENODEV;
> +	return rc;
> +}
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 1b8e1b5..018c3825 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -188,6 +188,7 @@ static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
>  }
>  
>  static const struct tpm_class_ops tpm_crb = {
> +	.flags = TPM_OPS_AUTO_STARTUP,
>  	.status = crb_status,
>  	.recv = crb_recv,
>  	.send = crb_send,
> @@ -200,7 +201,6 @@ static const struct tpm_class_ops tpm_crb = {
>  static int crb_init(struct acpi_device *device, struct crb_priv *priv)
>  {
>  	struct tpm_chip *chip;
> -	int rc;
>  
>  	chip = tpmm_chip_alloc(&device->dev, &tpm_crb);
>  	if (IS_ERR(chip))
> @@ -210,14 +210,6 @@ static int crb_init(struct acpi_device *device, struct crb_priv *priv)
>  	chip->acpi_dev_handle = device->handle;
>  	chip->flags = TPM_CHIP_FLAG_TPM2;
>  
> -	rc = tpm_get_timeouts(chip);
> -	if (rc)
> -		return rc;
> -
> -	rc = tpm2_do_selftest(chip);
> -	if (rc)
> -		return rc;
> -
>  	return tpm_chip_register(chip);
>  }
>  
> diff --git a/drivers/char/tpm/tpm_i2c_atmel.c b/drivers/char/tpm/tpm_i2c_atmel.c
> index c37aa72..95ce2e9 100644
> --- a/drivers/char/tpm/tpm_i2c_atmel.c
> +++ b/drivers/char/tpm/tpm_i2c_atmel.c
> @@ -141,6 +141,7 @@ static bool i2c_atmel_req_canceled(struct tpm_chip *chip, u8 status)
>  }
>  
>  static const struct tpm_class_ops i2c_atmel = {
> +	.flags = TPM_OPS_AUTO_STARTUP,
>  	.status = i2c_atmel_read_status,
>  	.recv = i2c_atmel_recv,
>  	.send = i2c_atmel_send,
> @@ -179,11 +180,6 @@ static int i2c_atmel_probe(struct i2c_client *client,
>  	/* There is no known way to probe for this device, and all version
>  	 * information seems to be read via TPM commands. Thus we rely on the
>  	 * TPM startup process in the common code to detect the device. */
> -	if (tpm_get_timeouts(chip))
> -		return -ENODEV;
> -
> -	if (tpm_do_selftest(chip))
> -		return -ENODEV;
>  
>  	return tpm_chip_register(chip);
>  }
> diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
> index a426b6f..62ee44e 100644
> --- a/drivers/char/tpm/tpm_i2c_infineon.c
> +++ b/drivers/char/tpm/tpm_i2c_infineon.c
> @@ -567,6 +567,7 @@ static bool tpm_tis_i2c_req_canceled(struct tpm_chip *chip, u8 status)
>  }
>  
>  static const struct tpm_class_ops tpm_tis_i2c = {
> +	.flags = TPM_OPS_AUTO_STARTUP,
>  	.status = tpm_tis_i2c_status,
>  	.recv = tpm_tis_i2c_recv,
>  	.send = tpm_tis_i2c_send,
> @@ -619,9 +620,6 @@ static int tpm_tis_i2c_init(struct device *dev)
>  
>  	tpm_dev.chip = chip;
>  
> -	tpm_get_timeouts(chip);
> -	tpm_do_selftest(chip);
> -
>  	return tpm_chip_register(chip);
>  out_release:
>  	release_locality(chip, tpm_dev.locality, 1);
> diff --git a/drivers/char/tpm/tpm_i2c_nuvoton.c b/drivers/char/tpm/tpm_i2c_nuvoton.c
> index 4e32094..e8ff362 100644
> --- a/drivers/char/tpm/tpm_i2c_nuvoton.c
> +++ b/drivers/char/tpm/tpm_i2c_nuvoton.c
> @@ -461,6 +461,7 @@ static bool i2c_nuvoton_req_canceled(struct tpm_chip *chip, u8 status)
>  }
>  
>  static const struct tpm_class_ops tpm_i2c = {
> +	.flags = TPM_OPS_AUTO_STARTUP,
>  	.status = i2c_nuvoton_read_status,
>  	.recv = i2c_nuvoton_recv,
>  	.send = i2c_nuvoton_send,
> @@ -609,12 +610,6 @@ static int i2c_nuvoton_probe(struct i2c_client *client,
>  		}
>  	}
>  
> -	if (tpm_get_timeouts(chip))
> -		return -ENODEV;
> -
> -	if (tpm_do_selftest(chip))
> -		return -ENODEV;
> -
>  	return tpm_chip_register(chip);
>  }
>  
> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> index 03a06b3..de6d25d 100644
> --- a/drivers/char/tpm/tpm_tis_core.c
> +++ b/drivers/char/tpm/tpm_tis_core.c
> @@ -643,6 +643,7 @@ void tpm_tis_remove(struct tpm_chip *chip)
>  EXPORT_SYMBOL_GPL(tpm_tis_remove);
>  
>  static const struct tpm_class_ops tpm_tis = {
> +	.flags = TPM_OPS_AUTO_STARTUP,
>  	.status = tpm_tis_status,
>  	.recv = tpm_tis_recv,
>  	.send = tpm_tis_send,
> @@ -778,29 +779,6 @@ int tpm_tis_core_init(struct device *dev, struct tpm_tis_data *priv, int irq,
>  		}
>  	}
>  
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> -		rc = tpm2_do_selftest(chip);
> -		if (rc == TPM2_RC_INITIALIZE) {
> -			dev_warn(dev, "Firmware has not started TPM\n");
> -			rc  = tpm2_startup(chip, TPM2_SU_CLEAR);
> -			if (!rc)
> -				rc = tpm2_do_selftest(chip);
> -		}
> -
> -		if (rc) {
> -			dev_err(dev, "TPM self test failed\n");
> -			if (rc > 0)
> -				rc = -ENODEV;
> -			goto out_err;
> -		}
> -	} else {
> -		if (tpm_do_selftest(chip)) {
> -			dev_err(dev, "TPM self test failed\n");
> -			rc = -ENODEV;
> -			goto out_err;
> -		}
> -	}
> -
>  	return tpm_chip_register(chip);
>  out_err:
>  	tpm_tis_remove(chip);
> diff --git a/drivers/char/tpm/tpm_vtpm_proxy.c b/drivers/char/tpm/tpm_vtpm_proxy.c
> index 86e27e8..9a94033 100644
> --- a/drivers/char/tpm/tpm_vtpm_proxy.c
> +++ b/drivers/char/tpm/tpm_vtpm_proxy.c
> @@ -346,6 +346,7 @@ static bool vtpm_proxy_tpm_req_canceled(struct tpm_chip  *chip, u8 status)
>  }
>  
>  static const struct tpm_class_ops vtpm_proxy_tpm_ops = {
> +	.flags = TPM_OPS_AUTO_STARTUP,
>  	.recv = vtpm_proxy_tpm_op_recv,
>  	.send = vtpm_proxy_tpm_op_send,
>  	.cancel = vtpm_proxy_tpm_op_cancel,
> @@ -366,14 +367,6 @@ static void vtpm_proxy_work(struct work_struct *work)
>  						   work);
>  	int rc;
>  
> -	if (proxy_dev->flags & VTPM_PROXY_FLAG_TPM2)
> -		rc = tpm2_startup(proxy_dev->chip, TPM2_SU_CLEAR);
> -	else
> -		rc = tpm_get_timeouts(proxy_dev->chip);
> -
> -	if (rc)
> -		goto err;
> -
>  	rc = tpm_chip_register(proxy_dev->chip);
>  	if (rc)
>  		goto err;
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 706e63e..da158f0 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -33,7 +33,12 @@ struct tpm_chip;
>  struct trusted_key_payload;
>  struct trusted_key_options;
>  
> +enum TPM_OPS_FLAGS {
> +	TPM_OPS_AUTO_STARTUP = BIT(0),
> +};
> +
>  struct tpm_class_ops {
> +	unsigned int flags;
>  	const u8 req_complete_mask;
>  	const u8 req_complete_val;
>  	bool (*req_canceled)(struct tpm_chip *chip, u8 status);
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html