On Wed, 2019-09-04 at 22:03 +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> 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 necessary. Overhead of doing multiple walks is irrelevant here. The simplicity and clarity win. /Jarkko
