Re: [PATCH v15 02/28] tpm: Move TPM1 specific definitions and functions to new headers

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, Dec 15, 2025 at 03:32:50PM -0800, Ross Philipson wrote:
> This gathers all the TPM1 definitions and structures into two separate
> header files (public tpm1.h and private tpm1_structs.h). The definitions
> moved to these files correspond to the TCG specification for TPM 1 family:
> 
> TPM 1.2 Main Specification
>  -  https://trustedcomputinggroup.org/resource/tpm-main-specification/
> 
> Note that the structures were pulled into tpm1_structs.h to allow their
> external reuse.
> 
> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> ---
>  drivers/char/tpm/tpm.h          | 98 +--------------------------------
>  drivers/char/tpm/tpm1-cmd.c     |  5 --
>  drivers/char/tpm/tpm1_structs.h | 97 ++++++++++++++++++++++++++++++++

I think you are overcomplicating the patch set and doing more
than you really need to do.

I.e. structs could go also just as well to tpm_command.h. We
will deal with if/when that file ever grows too large. It's
absolutely not a priority for this patch set.

>  include/linux/tpm1.h            | 34 +++++++++++-
>  4 files changed, 132 insertions(+), 102 deletions(-)
>  create mode 100644 drivers/char/tpm/tpm1_structs.h
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index ca391b2a211c..1f9f8540eede 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -50,105 +50,9 @@ enum tpm_addr {
>  	TPM_ADDR = 0x4E,
>  };
>  
> -#define TPM_WARN_RETRY          0x800
> -#define TPM_WARN_DOING_SELFTEST 0x802
> -#define TPM_ERR_DEACTIVATED     0x6
> -#define TPM_ERR_DISABLED        0x7
> -#define TPM_ERR_FAILEDSELFTEST  0x1C
> -#define TPM_ERR_INVALID_POSTINIT 38
> -
> -#define TPM_TAG_RQU_COMMAND 193
> -
>  /* TPM2 specific constants. */
>  #define TPM2_SPACE_BUFFER_SIZE		16384 /* 16 kB */
>  
> -struct	stclear_flags_t {
> -	__be16	tag;
> -	u8	deactivated;
> -	u8	disableForceClear;
> -	u8	physicalPresence;
> -	u8	physicalPresenceLock;
> -	u8	bGlobalLock;
> -} __packed;
> -
> -struct tpm1_version {
> -	u8 major;
> -	u8 minor;
> -	u8 rev_major;
> -	u8 rev_minor;
> -} __packed;
> -
> -struct tpm1_version2 {
> -	__be16 tag;
> -	struct tpm1_version version;
> -} __packed;
> -
> -struct	timeout_t {
> -	__be32	a;
> -	__be32	b;
> -	__be32	c;
> -	__be32	d;
> -} __packed;
> -
> -struct duration_t {
> -	__be32	tpm_short;
> -	__be32	tpm_medium;
> -	__be32	tpm_long;
> -} __packed;
> -
> -struct permanent_flags_t {
> -	__be16	tag;
> -	u8	disable;
> -	u8	ownership;
> -	u8	deactivated;
> -	u8	readPubek;
> -	u8	disableOwnerClear;
> -	u8	allowMaintenance;
> -	u8	physicalPresenceLifetimeLock;
> -	u8	physicalPresenceHWEnable;
> -	u8	physicalPresenceCMDEnable;
> -	u8	CEKPUsed;
> -	u8	TPMpost;
> -	u8	TPMpostLock;
> -	u8	FIPS;
> -	u8	operator;
> -	u8	enableRevokeEK;
> -	u8	nvLocked;
> -	u8	readSRKPub;
> -	u8	tpmEstablished;
> -	u8	maintenanceDone;
> -	u8	disableFullDALogicInfo;
> -} __packed;
> -
> -typedef union {
> -	struct	permanent_flags_t perm_flags;
> -	struct	stclear_flags_t	stclear_flags;
> -	__u8	owned;
> -	__be32	num_pcrs;
> -	struct tpm1_version version1;
> -	struct tpm1_version2 version2;
> -	__be32	manufacturer_id;
> -	struct timeout_t  timeout;
> -	struct duration_t duration;
> -} cap_t;
> -
> -enum tpm_capabilities {
> -	TPM_CAP_FLAG = 4,
> -	TPM_CAP_PROP = 5,
> -	TPM_CAP_VERSION_1_1 = 0x06,
> -	TPM_CAP_VERSION_1_2 = 0x1A,
> -};
> -
> -enum tpm_sub_capabilities {
> -	TPM_CAP_PROP_PCR = 0x101,
> -	TPM_CAP_PROP_MANUFACTURER = 0x103,
> -	TPM_CAP_FLAG_PERM = 0x108,
> -	TPM_CAP_FLAG_VOL = 0x109,
> -	TPM_CAP_PROP_OWNER = 0x111,
> -	TPM_CAP_PROP_TIS_TIMEOUT = 0x115,
> -	TPM_CAP_PROP_TIS_DURATION = 0x120,
> -};
> -
>  enum tpm2_pt_props {
>  	TPM2_PT_NONE = 0x00000000,
>  	TPM2_PT_GROUP = 0x00000100,
> @@ -229,6 +133,8 @@ enum tpm2_pt_props {
>   * compiler warnings about stack frame size. */
>  #define TPM_MAX_RNG_DATA	128
>  
> +#include "tpm1_structs.h"
> +
>  extern const struct class tpm_class;
>  extern const struct class tpmrm_class;
>  extern dev_t tpm_devt;
> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
> index f29827b454d2..02f20a0aa37d 100644
> --- a/drivers/char/tpm/tpm1-cmd.c
> +++ b/drivers/char/tpm/tpm1-cmd.c
> @@ -505,11 +505,6 @@ ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
>  }
>  EXPORT_SYMBOL_GPL(tpm1_getcap);
>  
> -struct tpm1_get_random_out {
> -	__be32 rng_data_len;
> -	u8 rng_data[TPM_MAX_RNG_DATA];
> -} __packed;
> -
>  /**
>   * tpm1_get_random() - get random bytes from the TPM's RNG
>   * @chip:	a &struct tpm_chip instance
> diff --git a/drivers/char/tpm/tpm1_structs.h b/drivers/char/tpm/tpm1_structs.h
> new file mode 100644
> index 000000000000..ad21376af5ab
> --- /dev/null
> +++ b/drivers/char/tpm/tpm1_structs.h
> @@ -0,0 +1,97 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2004 IBM Corporation
> + * Copyright (C) 2015 Intel Corporation
> + *
> + * Authors:
> + * Leendert van Doorn <leendert@watson.ibm.com>
> + * Dave Safford <safford@watson.ibm.com>
> + * Reiner Sailer <sailer@watson.ibm.com>
> + * Kylene Hall <kjhall@us.ibm.com>
> + *
> + * Maintained by: <tpmdd-devel@lists.sourceforge.net>
> + *
> + * Device driver for TCG/TCPA TPM (trusted platform module).
> + * Specifications at www.trustedcomputinggroup.org
> + */
> +
> +#ifndef __TPM1_STRUCTS_H__
> +#define __TPM1_STRUCTS_H__
> +
> +struct	stclear_flags_t {
> +	__be16	tag;
> +	u8	deactivated;
> +	u8	disableForceClear;
> +	u8	physicalPresence;
> +	u8	physicalPresenceLock;
> +	u8	bGlobalLock;
> +} __packed;


Don't retain alignment.

> +
> +struct tpm1_version {
> +	u8 major;
> +	u8 minor;
> +	u8 rev_major;
> +	u8 rev_minor;
> +} __packed;
> +
> +struct tpm1_version2 {
> +	__be16 tag;
> +	struct tpm1_version version;
> +} __packed;
> +
> +struct	timeout_t {
> +	__be32	a;
> +	__be32	b;
> +	__be32	c;
> +	__be32	d;
> +} __packed;
> +
> +struct duration_t {
> +	__be32	tpm_short;
> +	__be32	tpm_medium;
> +	__be32	tpm_long;
> +} __packed;
> +
> +struct permanent_flags_t {
> +	__be16	tag;
> +	u8	disable;
> +	u8	ownership;
> +	u8	deactivated;
> +	u8	readPubek;
> +	u8	disableOwnerClear;
> +	u8	allowMaintenance;
> +	u8	physicalPresenceLifetimeLock;
> +	u8	physicalPresenceHWEnable;
> +	u8	physicalPresenceCMDEnable;
> +	u8	CEKPUsed;
> +	u8	TPMpost;
> +	u8	TPMpostLock;
> +	u8	FIPS;
> +	u8	operator;
> +	u8	enableRevokeEK;
> +	u8	nvLocked;
> +	u8	readSRKPub;
> +	u8	tpmEstablished;
> +	u8	maintenanceDone;
> +	u8	disableFullDALogicInfo;
> +} __packed;
> +
> +/* Gather all capabilities related information info one type */
> +typedef union {
> +	struct	permanent_flags_t perm_flags;
> +	struct	stclear_flags_t	stclear_flags;
> +	__u8	owned;
> +	__be32	num_pcrs;
> +	struct tpm1_version version1;
> +	struct tpm1_version2 version2;
> +	__be32	manufacturer_id;
> +	struct timeout_t  timeout;
> +	struct duration_t duration;
> +} cap_t;

Don't retain alignment here.

> +
> +struct tpm1_get_random_out {
> +	__be32 rng_data_len;
> +	u8 rng_data[TPM_MAX_RNG_DATA];
> +} __packed;
> +
> +#endif
> diff --git a/include/linux/tpm1.h b/include/linux/tpm1.h
> index 54c6c211eb9e..5fad94ac8d15 100644
> --- a/include/linux/tpm1.h
> +++ b/include/linux/tpm1.h
> @@ -47,7 +47,39 @@ enum tpm_command_ordinals {
>  	TPM_ORD_UNSEAL			= 24,
>  };
>  
> -/* Other constants */
> +enum tpm_capabilities {
> +	TPM_CAP_FLAG		= 4,
> +	TPM_CAP_PROP		= 5,
> +	TPM_CAP_VERSION_1_1	= 0x06,
> +	TPM_CAP_VERSION_1_2	= 0x1A,
> +};
> +
> +enum tpm_sub_capabilities {
> +	TPM_CAP_PROP_PCR		= 0x101,
> +	TPM_CAP_PROP_MANUFACTURER	= 0x103,
> +	TPM_CAP_FLAG_PERM		= 0x108,
> +	TPM_CAP_FLAG_VOL		= 0x109,
> +	TPM_CAP_PROP_OWNER		= 0x111,
> +	TPM_CAP_PROP_TIS_TIMEOUT	= 0x115,
> +	TPM_CAP_PROP_TIS_DURATION	= 0x120,
> +};
> +
> +/* Return Codes */
> +enum tpm_return_codes {
> +	TPM_BASE_MASK			= 0,
> +	TPM_NON_FATAL_MASK		= 0x00000800,
> +	TPM_SUCCESS			= TPM_BASE_MASK + 0,
> +	TPM_ERR_DEACTIVATED		= TPM_BASE_MASK + 6,
> +	TPM_ERR_DISABLED		= TPM_BASE_MASK + 7,
> +	TPM_ERR_FAIL			= TPM_BASE_MASK + 9,
> +	TPM_ERR_FAILEDSELFTEST		= TPM_BASE_MASK + 28,
> +	TPM_ERR_INVALID_POSTINIT	= TPM_BASE_MASK + 38,
> +	TPM_ERR_INVALID_FAMILY		= TPM_BASE_MASK + 55,
> +	TPM_WARN_RETRY			= TPM_BASE_MASK + TPM_NON_FATAL_MASK + 0,
> +	TPM_WARN_DOING_SELFTEST		= TPM_BASE_MASK + TPM_NON_FATAL_MASK + 2,
> +};
> +
> +/* Misc. constants */

These constants should be relocated in a separate patch.

>  #define SRKHANDLE                       0x40000000
>  #define TPM_NONCE_SIZE                  20
>  #define TPM_ST_CLEAR			1
> -- 
> 2.43.7
> 

BR, Jarkko