[Skiboot] [PATCH] tpm_i2c_nuvoton: check TPM vendor id register during probe
Oliver O'Halloran
oohall at gmail.com
Fri Mar 6 13:34:00 AEDT 2020
On Fri, Mar 6, 2020 at 5:55 AM Eric Richter <erichte at linux.ibm.com> wrote:
>
> The driver for the nuvoton i2c TPM does not currently check if there is
> a functional TPM at the bus and address given by the device tree.
>
> This patch adds a simple check of the TPM vendor id register, compares
> against the known expected value for the chip, skips registering it if
> the chip is inaccessible or returns an unexpected id.
>
> Signed-off-by: Eric Richter <erichte at linux.ibm.com>
> ---
> libstb/drivers/tpm_i2c_nuvoton.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/libstb/drivers/tpm_i2c_nuvoton.c b/libstb/drivers/tpm_i2c_nuvoton.c
> index 3679ddaf..882018e4 100644
> --- a/libstb/drivers/tpm_i2c_nuvoton.c
> +++ b/libstb/drivers/tpm_i2c_nuvoton.c
> @@ -30,6 +30,7 @@
> #define TPM_BURST_COUNT 0x01
> #define TPM_DATA_FIFO_W 0x20
> #define TPM_DATA_FIFO_R 0x40
> +#define TPM_VID_DID 0x60
>
> /* Bit masks for the TPM STATUS register */
> #define TPM_STS_VALID 0x80
> @@ -42,6 +43,8 @@
> /* TPM Driver values */
> #define MAX_STSVALID_POLLS 5
> #define TPM_TIMEOUT_INTERVAL 10
> +#define TPM_650_VENDOR_ID 0x5010FE00
If only we knew the name of the vendor...
> +#define TPM_VENDOR_ID_MASK 0xFFFFFF00
>
> static struct tpm_dev *tpm_device = NULL;
>
> @@ -552,6 +555,7 @@ void tpm_i2c_nuvoton_probe(void)
> struct dt_node *node = NULL;
> struct i2c_bus *bus;
> const char *name;
> + uint32_t vendor = 0;
>
> dt_for_each_compatible(dt_root, node, "nuvoton,npct650") {
> if (!dt_node_is_enabled(node))
> @@ -588,6 +592,19 @@ void tpm_i2c_nuvoton_probe(void)
> "found, tpm node parent %p\n", node->parent);
> goto disable;
> }
> + /* ensure there's really the TPM we expect at that address */
> + if (tpm_i2c_request_send(tpm_device, SMBUS_READ, TPM_VID_DID,
> + 1, &vendor, sizeof(vendor)) ||
> + (vendor & TPM_VENDOR_ID_MASK) != TPM_650_VENDOR_ID) {
> + prlog(PR_ERR, "NUVOTON: i2c device inaccessible, or "
> + "expected vendor id mismatch\n");
> + if (vendor) {
> + prlog(PR_ERR, "NUVOTON: expected 0x%X, "
> + "got 0x%X\n", TPM_650_VENDOR_ID, vendor);
> + }
The usual pattern for this is something like:
rc = tpm_i2c_request(...);
if (rc) {
prlog("i2c failed");
continue;
}
if (vendor & TPM_VENDOR_ID_MASK) != TPM_650_VENDOR_ID) {
prlog("vendor id mismatch")
continue;
}
The control flow is cleaner, there's less indent stacking, and there's
no multi-line if conditionals which are always a mess. The way
tpm_i2c_nuvoton_probe() is structed makes it slightly awkward since
you need to duplicate the cleanup, but it's still better IMO.
> + free(tpm_device);
> + continue;
> + }
> if (tpm_register_chip(node, tpm_device,
> &tpm_i2c_nuvoton_driver)) {
> free(tpm_device);
> --
> 2.21.1
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
More information about the Skiboot
mailing list