From mboxrd@z Thu Jan 1 00:00:00 1970 From: Miquel Raynal Date: Thu, 19 Jul 2018 22:31:27 +0200 Subject: [U-Boot] [PATCH 3/6] tpm: allow TPM v1 and v2 to be compiled at the same time In-Reply-To: References: <20180714121633.15180-1-miquel.raynal@bootlin.com> <20180714121633.15180-4-miquel.raynal@bootlin.com> Message-ID: <20180719223127.6e24da32@xps13> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: u-boot@lists.denx.de Hi Simon, Simon Glass wrote on Wed, 18 Jul 2018 19:31:41 -0600: > Hi Miquel, >=20 > On 14 July 2018 at 06:16, Miquel Raynal wrote: > > While there is probably no reason to do so in a real life situation, it > > will allow to compile test both stacks with the same sandbox defconfig. > > > > As we cannot define two 'tpm' commands at the same time, the command for > > TPM v1 is still called 'tpm' and the one for TPM v2 'tpm2'. While this > > is the exact command name that must be written into eg. test files, any > > user already using the TPM v2 stack can continue using just 'tpm' as > > command as long as TPM v1 support is not compiled (because U-Boot prompt > > will search for the closest command named after 'tpm'.b) > > > > The command set can also be changed at runtime (not supported yet, but > > ready to be), but as one can compile only either one stack or the other, > > there is still one spot in the code where conditionals are used: to > > retrieve the v1 or v2 command set. > > > > Signed-off-by: Miquel Raynal > > --- > > cmd/tpm-common.c | 24 +++++++++++++++++++++++- > > cmd/tpm-v1.c | 2 +- > > cmd/tpm-v2.c | 4 ++-- > > drivers/tpm/Kconfig | 7 ++----- > > drivers/tpm/tpm-uclass.c | 13 ++++++++++--- > > drivers/tpm/tpm2_tis_sandbox.c | 11 +++++++++++ > > drivers/tpm/tpm2_tis_spi.c | 15 +++++++++++++++ > > include/tpm-common.h | 36 ++++++++++++++++++++++++++++++----= -- > > lib/tpm-common.c | 4 ++++ > > lib/tpm-utils.h | 9 +++++++++ > > 10 files changed, 107 insertions(+), 18 deletions(-) > > =20 >=20 > Reviewed-by: Simon Glass >=20 > Some comments below. >=20 > > diff --git a/cmd/tpm-common.c b/cmd/tpm-common.c > > index 6cf9fcc9ac..69cc02b599 100644 > > --- a/cmd/tpm-common.c > > +++ b/cmd/tpm-common.c > > @@ -273,12 +273,34 @@ int do_tpm_init(cmd_tbl_t *cmdtp, int flag, int a= rgc, char * const argv[]) > > int do_tpm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > { > > cmd_tbl_t *tpm_commands, *cmd; > > + struct tpm_chip_priv *priv; > > + struct udevice *dev; > > unsigned int size; > > + int rc; =20 >=20 > Can we use 'ret'? That is more common. Of course. >=20 > > > > if (argc < 2) > > return CMD_RET_USAGE; > > > > - tpm_commands =3D get_tpm_commands(&size); > > + rc =3D get_tpm(&dev); > > + if (rc) > > + return rc; > > + > > + priv =3D dev_get_uclass_priv(dev); > > + > > + switch (priv->version) { > > +#if defined(CONFIG_TPM_V1) > > + case TPM_V1: =20 >=20 > if (IS_ENABLED(CONFIG_TPM_V1)) >=20 > (I think #ifdef is worse) Actually, in the present state, get_tpmx_commands symbol is only available if the file if TPM_Vx is selected in Kconfig. I created two dummy functions in tpm-common.h which return NULL for each symbol if their respective TPM version is not compiled. This way it removes any conditionals in the code. >=20 > > + tpm_commands =3D get_tpm1_commands(&size); > > + break; > > +#endif > > +#if defined(CONFIG_TPM_V2) > > + case TPM_V2: > > + tpm_commands =3D get_tpm2_commands(&size); > > + break; > > +#endif > > + default: > > + return CMD_RET_USAGE; > > + } [...] > > > > +static int sandbox_tpm2_set_version(struct udevice *dev) > > +{ > > + struct tpm_chip_priv *priv =3D dev_get_uclass_priv(dev); > > + > > + /* Use the TPM v2 stack */ > > + priv->version =3D TPM_V2; =20 >=20 > Don't you think this could be done in the probe() method? When I first wrote this I faced some issues. As I did not remembered why it would fail I tried again and indeed, moving this selection in the probe works. >=20 > > + > > + return 0; > > +} > > + [...] > > @@ -65,6 +78,16 @@ struct tpm_chip_priv { > > * to bytes which are sent and received. > > */ > > struct tpm_ops { > > + /** > > + * set_version() - Set the right TPM stack version to use > > + * > > + * Default is TPM_V1. TPM of newer versions can implement this > > + * optional hook to set another value. > > + * > > + * @dev: TPM device > > + */ > > + int (*set_version)(struct udevice *dev); =20 >=20 > I think this could just be a helper function provided by the uclass, > rather than a device method. Actually there is no need for this hook or any helper anymore, I removed them all as the version selection is done in the driver ->probe(). I'll let other people that would need to change the version at runtime adding support for that if they want, it should not be too tricky anyway. Thanks, Miqu=C3=A8l