Re: [PATCH v6] tpm: separate cmd_ready/go_idle from runtime_pm

[Jarkko Sakkinen]

On Fri, Jun 08, 2018 at 12:20:40AM +0300, Tomas Winkler wrote:
> We cannot use go_idle cmd_ready commands via runtime_pm handles
> as with the introduction of localities this is no longer an optional
> feature, while runtime pm can be not enabled.
> Though cmd_ready/go_idle provides a power saving, it's also a part of
> TPM2 protocol and should be called explicitly.
> This patch exposes cmd_read/go_idle via tpm class ops and removes
> runtime pm support as it is not used by any driver.
> 
> A new tpm transmit flag is added TPM_TRANSMIT_NESTED, which implies
> TPM_TRANSMIT_UNLOCKED and TPM_TRANSMIT_RAW. Both are needed to resolve
> tpm spaces and locality request recursive calls to tpm_transmit().
> 
> New wrappers are added tpm_cmd_ready() and tpm_go_idle() to
> streamline tpm_try_transmit code.
> 
> tpm_crb no longer needs own power saving functions and can drop using
> tpm_pm_suspend/resume.
> 
> This patch cannot be really separated from the locality fix.
> Fixes: 888d867df441 (tpm: cmd_ready command can be issued only after granting locality)
> 
> 
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 888d867df441 (tpm: cmd_ready command can be issued only after granting locality)
> Signed-off-by: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> ---
> V2-3:resent
> V4: 1. Use tpm_pm_suspend/resume in tpm_crb
>     2. Drop cmd_ready/go_idle around tpm_chip_register, as there is no
>        usage counter like in runtime_pm.
> V5: 1. add tpm_cmd_ready and tpm_go_idle wrappers.
>     2. Abuse TPM_TRANSMIT_UNLOCKED flag to resolve tpm space
>        recursion.
> V6: 1. Add new flags TPM_TRANSMIT_NESTED which implies
>        TPM_TRANSMIT_UNLOCKED and TPM_TRANSMIT_RAW, as not all
>        unlocked flows are recursive i.e. tpm2_unseal_trusted.
> 
>  drivers/char/tpm/tpm-interface.c  |  51 +++++++++++++++----
>  drivers/char/tpm/tpm.h            |  13 ++++-
>  drivers/char/tpm/tpm2-space.c     |  12 ++---
>  drivers/char/tpm/tpm_crb.c        | 101 ++++++++++----------------------------
>  drivers/char/tpm/tpm_vtpm_proxy.c |   2 +-
>  include/linux/tpm.h               |   2 +
>  6 files changed, 88 insertions(+), 93 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index e32f6e85dc6d..1abd8261651b 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -29,7 +29,6 @@
>  #include <linux/mutex.h>
>  #include <linux/spinlock.h>
>  #include <linux/freezer.h>
> -#include <linux/pm_runtime.h>
>  #include <linux/tpm_eventlog.h>
>  
>  #include "tpm.h"
> @@ -369,10 +368,13 @@ static int tpm_validate_command(struct tpm_chip *chip,
>  	return -EINVAL;
>  }
>  
> -static int tpm_request_locality(struct tpm_chip *chip)
> +static int tpm_request_locality(struct tpm_chip *chip, unsigned int flags)
>  {
>  	int rc;
>  
> +	if (flags & __TPM_TRANSMIT_RAW)
> +		return 0;
> +
>  	if (!chip->ops->request_locality)
>  		return 0;
>  
> @@ -385,10 +387,13 @@ static int tpm_request_locality(struct tpm_chip *chip)
>  	return 0;
>  }
>  
> -static void tpm_relinquish_locality(struct tpm_chip *chip)
> +static void tpm_relinquish_locality(struct tpm_chip *chip, unsigned int flags)
>  {
>  	int rc;
>  
> +	if (flags & __TPM_TRANSMIT_RAW)
> +		return;
> +
>  	if (!chip->ops->relinquish_locality)
>  		return;
>  
> @@ -399,6 +404,28 @@ static void tpm_relinquish_locality(struct tpm_chip *chip)
>  	chip->locality = -1;
>  }
>  
> +static int tpm_cmd_ready(struct tpm_chip *chip, unsigned int flags)
> +{
> +	if (flags & __TPM_TRANSMIT_RAW)
> +		return 0;
> +
> +	if (!chip->ops->cmd_ready)
> +		return 0;
> +
> +	return chip->ops->cmd_ready(chip);
> +}
> +
> +static int tpm_go_idle(struct tpm_chip *chip, unsigned int flags)
> +{
> +	if (flags & __TPM_TRANSMIT_RAW)
> +		return 0;
> +
> +	if (!chip->ops->go_idle)
> +		return 0;
> +
> +	return chip->ops->go_idle(chip);
> +}
> +
>  static ssize_t tpm_try_transmit(struct tpm_chip *chip,
>  				struct tpm_space *space,
>  				u8 *buf, size_t bufsiz,
> @@ -449,14 +476,15 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
>  	/* Store the decision as chip->locality will be changed. */
>  	need_locality = chip->locality == -1;
>  
> -	if (!(flags & TPM_TRANSMIT_RAW) && need_locality) {
> -		rc = tpm_request_locality(chip);
> +	if (need_locality) {
> +		rc = tpm_request_locality(chip, flags);
>  		if (rc < 0)
>  			goto out_no_locality;
>  	}
>  
> -	if (chip->dev.parent)
> -		pm_runtime_get_sync(chip->dev.parent);
> +	rc = tpm_cmd_ready(chip, flags);
> +	if (rc)
> +		goto out;
>  
>  	rc = tpm2_prepare_space(chip, space, ordinal, buf);
>  	if (rc)
> @@ -516,13 +544,16 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip,
>  	}
>  
>  	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> +	if (rc)
> +		dev_err(&chip->dev, "tpm2_commit_space: error %d\n", rc);
>  
>  out:
> -	if (chip->dev.parent)
> -		pm_runtime_put_sync(chip->dev.parent);
> +	rc = tpm_go_idle(chip, flags);
> +	if (rc)
> +		goto out;
>  
>  	if (need_locality)
> -		tpm_relinquish_locality(chip);
> +		tpm_relinquish_locality(chip, flags);
>  
>  out_no_locality:
>  	if (chip->ops->clk_enable != NULL)
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 4426649e431c..beb0a763016c 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -511,9 +511,18 @@ extern const struct file_operations tpm_fops;
>  extern const struct file_operations tpmrm_fops;
>  extern struct idr dev_nums_idr;
>  
> +/**
> + * enum tpm_transmit_flags
> + *
> + * @TPM_TRANSMIT_UNLOCKED: used to lock sequence of tpm_transmit calls.
> + * @__TPM_TRANSMIT_RAW: prevent recursive calls into setup steps
> + *                    (go idle, locality,..). Don't use directly.
> + * @TPM_TRANSMIT_NESTED: Use from nested tpm_transmit calls
> + */
>  enum tpm_transmit_flags {
> -	TPM_TRANSMIT_UNLOCKED	= BIT(0),
> -	TPM_TRANSMIT_RAW	= BIT(1),
> +	TPM_TRANSMIT_UNLOCKED   = BIT(0),
> +	__TPM_TRANSMIT_RAW      = BIT(1),

NAK for the naming convention.

/Jarkko