[Skiboot] [PATCH] tpm_i2c_nuvoton: check TPM vendor id register during probe

[Oliver O'Halloran]

On Fri, Mar 6, 2020 at 1:34 PM Oliver O'Halloran <oohall at gmail.com> wrote:
>
> 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;

One more thing: Should we be doing goto disable; here rather than
continue? The kernel will try to use the TPM later on since it's still
in the DT.

> > +               }
> >                 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