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: Tue, 10 Sep 2019 13:32:55 +0100	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Wed, Sep 04, 2019 at 10:03:32PM +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 <>
> ---

Partial re-review. Please ignore the comment about multiple ACPI walks
in my previous review. Most of the other comments hold (like partial
/proc/iomem output *must* be included).

One remark I forgot is that this the v3 of your patch. Please version
your patches properly and provide the full changelog in v4. There is no
documentation what happend between v2 and v3.

Please provide more verbose explanation what the patch does. "Using it"
does not tell at all *what* is used and *how*. Please introduce the new
data structures and explain how they are used and what they replace and
so forth (you probably get the idea).

The commit message is definitely the weakest link of this patch right

>  static int crb_check_resource(struct acpi_resource *ares, void *data)
>  {
> -	struct resource *io_res = data;
> +	struct list_head *list = data;
> +	struct crb_resource *cres;
>  	struct resource_win win;
>  	struct resource *res = &(win.res);
>  	if (acpi_dev_resource_memory(ares, res) ||
>  	    acpi_dev_resource_address_space(ares, &win)) {
> -		*io_res = *res;
> -		io_res->name = NULL;
> +		cres = kzalloc(sizeof(*cres), GFP_KERNEL);
> +		if (!cres)
> +			return -ENOMEM;
> +
> +		cres->io_res = *res;
> +		cres-> = NULL;
> +
> +		list_add_tail(&cres->node, list);
>  	}
>  	return 1;

I think that you might get away just by collecting acpi_resources and
pass that to your helpers. See the documentation of the function on
how to make it collect all the resources:

Of course you then have to move acpi_dev_free_resource_list() to be
called after you've done ioremaps.

This was the essential thing that I wanted to remark compared to my
previous review.


      parent reply	other threads:[~2019-09-10 12:33 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
2019-09-07 23:58   ` Jarkko Sakkinen
2019-09-10 12:32 ` Jarkko Sakkinen [this message]

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).