On Wed, Sep 04, 2019 at 10:03:32PM +0300, ivan.lazeev@xxxxxxxxx wrote: > From: Ivan Lazeev <ivan.lazeev@xxxxxxxxx> > > Bug link: https://bugzilla.kernel.org/show_bug.cgi?id=195657 > > 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 <ivan.lazeev@xxxxxxxxx> > --- 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 now. > 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->io_res.name = 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: https://elixir.bootlin.com/linux/latest/source/drivers/acpi/resource.c#L622 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. /Jarkko
