[SeaBIOS] [PATCH 3/3] tpm: Add a menu for TPM configuration

[Peter Stuge]

Sorry, should have sent all in one email.

Stefan Berger wrote:
> +    if (enable)
> +        dprintf(DEBUG_tcg, "Return code from TPM_PhysicalEnable = 0x%08x\n",
> +                *returnCode);
> +    else
> +        dprintf(DEBUG_tcg, "Return code from TPM_PhysicalDisable = 0x%08x\n",
> +                *returnCode);

Maybe simplify this to one string with %s + enable ? "Enable" : "Disable" ?


> +err_exit:
> +    if (enable)
> +        dprintf(DEBUG_tcg, "TCGBIOS: Enabling the TPM failed.\n");
> +    else
> +        dprintf(DEBUG_tcg, "TCGBIOS: Disabling the TPM failed.\n");

Same here.


> +    dprintf(DEBUG_tcg, "TCGBIOS: TPM malfunctioning (line %d).\n", __LINE__);

See last email. This dprintf() seems a bit redundant with the error
in the lines above.


> +    if (pf.flags[PERM_FLAG_IDX_DISABLE]) {
> +        if (verbose)
> +            printf("TPM must first be enable.\n");

Typo: enabled


> +tpm_process_cfg(const tpm_bios_cfg *cfg, int verbose, u32 *returnCode)
..
> +    if (rc)
> +        printf("Op %d: An error occurred: 0x%x TPM return code: 0x%x\n",
> +               cfg->op, rc, *returnCode);

Maybe add a %s __func__ or some word about where this happened.


> +show_tpm_menu(int state, int next_scancodes[7])

Are scancodes really 32-bit? If you use a fixed size that's
presumably to save a bit of memory. I would prefer next_scancodes to
be dynamically sized according to some number of possible scancodes
in an enum or somesuch which is known at compile time, and for it to
be an array of unsigned char/uint8_t or uint16_t, as appropriate for
the hardware. IIRC scancodes are 8 bit?


> +    if (state & TPM_STATE_ENABLED)
> +        printf("  Enabled");
> +    else
> +        printf("  Disabled");

I'd use %s ? : for these but that's a matter of taste.


> +void
> +tpm_menu(void)
> +{
..
> +    printf("The Trusted Platform Module (TPM) is a hardware device in "
> +           "this machine.\n"

This is not universally true. I believe that the ME implements TPM in
software on recent platforms.


In this patch there is also an msleep(2000) call. Please avoid adding
unneccessary delays to the code.


Thanks!

//Peter