Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 20 Oct 2018 04:06:54 +0300 (EEST)
From:   Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To:     Tomas Winkler <tomas.winkler@intel.com>
cc:     Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
        Jason Gunthorpe <jgg@ziepe.ca>,
        Nayna Jain <nayna@linux.vnet.ibm.com>,
        Alexander Usyskin <alexander.usyskin@intel.com>,
        Tadeusz Struk <tadeusz.struk@intel.com>,
        linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 12/21] tpm: factor out tpm_startup function
In-Reply-To: <20181019182307.17745-13-tomas.winkler@intel.com>
Message-ID: <alpine.DEB.2.21.1810200406270.9963@jsakkine-mobl1>
References: <20181019182307.17745-1-tomas.winkler@intel.com> <20181019182307.17745-13-tomas.winkler@intel.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 19 Oct 2018, Tomas Winkler wrote:
> TPM manual startup is used only from within TPM 1.x or TPM 2.x
> code, hence remove tpm_startup() function from tpm-interface.c
> and add two static functions implementations tpm1_startup()
> and tpm2_startup() into to tpm1-cmd.c and tpm2-cmd.c respectively.
>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> V2-V2: Resend.
> V4: Fix the commit message.
> V5: 1. A small fix in the kdoc.
>    2. Fixed Jarkko's name in Tested-by.
> V6: Rebase.
> V7: Resend.
>
> drivers/char/tpm/tpm-interface.c | 41 ----------------------------------------
> drivers/char/tpm/tpm.h           |  1 -
> drivers/char/tpm/tpm1-cmd.c      | 37 +++++++++++++++++++++++++++++++++++-
> drivers/char/tpm/tpm2-cmd.c      | 34 +++++++++++++++++++++++++++++++--
> 4 files changed, 68 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index e7f220f691f9..54b81700561b 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -414,47 +414,6 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
> }
> EXPORT_SYMBOL_GPL(tpm_transmit_cmd);
>
> -#define TPM_ORD_STARTUP 153
> -#define TPM_ST_CLEAR 1
> -
> -/**
> - * tpm_startup - turn on the TPM
> - * @chip: TPM chip to use
> - *
> - * Normally the firmware should start the TPM. This function is provided as a
> - * workaround if this does not happen. A legal case for this could be for
> - * example when a TPM emulator is used.
> - *
> - * Return: same as tpm_transmit_cmd()
> - */
> -int tpm_startup(struct tpm_chip *chip)
> -{
> -	struct tpm_buf buf;
> -	int rc;
> -
> -	dev_info(&chip->dev, "starting up the TPM manually\n");
> -
> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> -		rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_STARTUP);
> -		if (rc < 0)
> -			return rc;
> -
> -		tpm_buf_append_u16(&buf, TPM2_SU_CLEAR);
> -	} else {
> -		rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_STARTUP);
> -		if (rc < 0)
> -			return rc;
> -
> -		tpm_buf_append_u16(&buf, TPM_ST_CLEAR);
> -	}
> -
> -	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
> -			      "attempting to start the TPM");
> -
> -	tpm_buf_destroy(&buf);
> -	return rc;
> -}
> -
> int tpm_get_timeouts(struct tpm_chip *chip)
> {
> 	if (chip->flags & TPM_CHIP_FLAG_HAVE_TIMEOUTS)
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 754f7bcb15fa..2eb73f6966c3 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -540,7 +540,6 @@ ssize_t tpm_transmit_cmd(struct tpm_chip *chip, struct tpm_space *space,
> 			 void *buf, size_t bufsiz,
> 			 size_t min_rsp_body_length, unsigned int flags,
> 			 const char *desc);
> -int tpm_startup(struct tpm_chip *chip);
> int tpm_get_timeouts(struct tpm_chip *);
>
> int tpm1_pm_suspend(struct tpm_chip *chip, int tpm_suspend_pcr);
> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
> index 3bd9f1fa77ce..8a84db315676 100644
> --- a/drivers/char/tpm/tpm1-cmd.c
> +++ b/drivers/char/tpm/tpm1-cmd.c
> @@ -308,6 +308,40 @@ unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
> 		return duration;
> }
>
> +#define TPM_ORD_STARTUP 153
> +#define TPM_ST_CLEAR 1
> +
> +/**
> + * tpm_startup() - turn on the TPM
> + * @chip: TPM chip to use
> + *
> + * Normally the firmware should start the TPM. This function is provided as a
> + * workaround if this does not happen. A legal case for this could be for
> + * example when a TPM emulator is used.
> + *
> + * Return: same as tpm_transmit_cmd()
> + */
> +static int tpm1_startup(struct tpm_chip *chip)
> +{
> +	struct tpm_buf buf;
> +	int rc;
> +
> +	dev_info(&chip->dev, "starting up the TPM manually\n");
> +
> +	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_STARTUP);
> +	if (rc < 0)
> +		return rc;
> +
> +	tpm_buf_append_u16(&buf, TPM_ST_CLEAR);
> +
> +	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
> +			      "attempting to start the TPM");
> +
> +	tpm_buf_destroy(&buf);
> +
> +	return rc;
> +}
> +
> int tpm1_get_timeouts(struct tpm_chip *chip)
> {
> 	cap_t cap;
> @@ -317,7 +351,7 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
> 	rc = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap, NULL,
> 			 sizeof(cap.timeout));
> 	if (rc == TPM_ERR_INVALID_POSTINIT) {
> -		if (tpm_startup(chip))
> +		if (tpm1_startup(chip))
> 			return rc;
>
> 		rc = tpm1_getcap(chip, TPM_CAP_PROP_TIS_TIMEOUT, &cap,
> @@ -727,3 +761,4 @@ int tpm1_pm_suspend(struct tpm_chip *chip, int tpm_suspend_pcr)
>
> 	return rc;
> }
> +
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index dd2e98f4de08..6ca4fc0a0d6f 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -948,6 +948,36 @@ static int tpm2_get_cc_attrs_tbl(struct tpm_chip *chip)
> 	return rc;
> }
>
> +/**
> + * tpm2_startup - turn on the TPM
> + * @chip: TPM chip to use
> + *
> + * Normally the firmware should start the TPM. This function is provided as a
> + * workaround if this does not happen. A legal case for this could be for
> + * example when a TPM emulator is used.
> + *
> + * Return: same as tpm_transmit_cmd()
> + */
> +
> +static int tpm2_startup(struct tpm_chip *chip)
> +{
> +	struct tpm_buf buf;
> +	int rc;
> +
> +	dev_info(&chip->dev, "starting up the TPM manually\n");
> +
> +	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_STARTUP);
> +	if (rc < 0)
> +		return rc;
> +
> +	tpm_buf_append_u16(&buf, TPM2_SU_CLEAR);
> +	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
> +			      "attempting to start the TPM");
> +	tpm_buf_destroy(&buf);
> +
> +	return rc;
> +}
> +
> /**
>  * tpm2_auto_startup - Perform the standard automatic TPM initialization
>  *                     sequence
> @@ -959,7 +989,7 @@ int tpm2_auto_startup(struct tpm_chip *chip)
> {
> 	int rc;
>
> -	rc = tpm_get_timeouts(chip);
> +	rc = tpm2_get_timeouts(chip);
> 	if (rc)
> 		goto out;
>
> @@ -968,7 +998,7 @@ int tpm2_auto_startup(struct tpm_chip *chip)
> 		goto out;
>
> 	if (rc == TPM2_RC_INITIALIZE) {
> -		rc = tpm_startup(chip);
> +		rc = tpm2_startup(chip);
> 		if (rc)
> 			goto out;
>
> -- 
> 2.14.4
>
>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko