archive mirror
 help / color / mirror / Atom feed
From: Jarkko Sakkinen <>
Cc: Peter Huewe <>, Jason Gunthorpe <>,
	Arnd Bergmann <>,
	Greg Kroah-Hartman <>,,
Subject: Re: [PATCH] tpm_crb: fix fTPM on AMD Zen+ CPUs
Date: Sun, 08 Sep 2019 00:49:11 +0300	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Wed, 2019-09-04 at 22:03 +0300, wrote:
> From: Ivan Lazeev <>
> Bug link:
> cmd/rsp buffers are expected to be in the same ACPI region.
> For Zen+ CPUs BIOS's might report two different regions, some of
> them also report region sizes inconsistent with values from TPM
> registers.
> Work around the issue by storing ACPI regions declared for the
> device in a list, then using it to find entry corresponding
> to each buffer. Use information from the entry to map each resource
> at most once and make buffer size consistent with the length of the
> region.
> Signed-off-by: Ivan Lazeev <>

Can you add the relevant pieces of /proc/iomem output to the commit
message so we can see the memory configuration? I don't have the
hardware available where this kind of situation occurs.

> ---
> Tested on ASRock x470 ITX motherboard with Ryzen 2600X CPU.
> However, I don't have any other hardware to test the changes on and no
> expertise to be sure that other TPMs won't break as a result.

You can still ask yourself that if the hardware describes the memory
with only one region do the code flows reduce mostly to do the same as
before this patch. It is not the same as testing with such hardware but
it is still a good mental exercise.

To summarize, I do get "not having hardware" part but completely
disqualify "not having expertise" part as the expertise needed here has
nothing specific to do with TPMs :-)

>  drivers/char/tpm/tpm_crb.c | 137 +++++++++++++++++++++++++++----------
>  1 file changed, 101 insertions(+), 36 deletions(-)
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index e59f1f91d7f3..0bcfe52db5d6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -91,7 +91,6 @@ enum crb_status {
>  struct crb_priv {
>  	u32 sm;
>  	const char *hid;
> -	void __iomem *iobase;
>  	struct crb_regs_head __iomem *regs_h;
>  	struct crb_regs_tail __iomem *regs_t;
>  	u8 __iomem *cmd;
> @@ -108,6 +107,12 @@ struct tpm2_crb_smc {
>  	u32 smc_func_id;
>  };
> +struct crb_resource {
> +	struct resource io_res;
> +	void __iomem *iobase;
> +	struct list_head node;
> +};

Please rename 'node' as 'list' for the sake of coherency with the rest
of the kernel and io_res as iores for the sake of coherency with the
other fields.

I think it would be cleaner to do the following:

1. Start with empty resource list.
2. Everytime crb_map_res() is called it does check the existing
   resources and does a ACPI walk on need basis at most adding
   one new entry.

In other words, crb_map_res() gets the list and appends one entry when

Overhead of doing multiple walks is irrelevant here. The simplicity and
clarity win.


  reply	other threads:[~2019-09-07 21:49 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-04 19:03 [PATCH] tpm_crb: fix fTPM on AMD Zen+ CPUs ivan.lazeev
2019-09-07 21:49 ` Jarkko Sakkinen [this message]
2019-09-07 23:58   ` Jarkko Sakkinen
2019-09-10 12:32 ` Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).