Re: [tpmdd-devel] [PATCH v2 0/3] tpm_tis: Clean up force module parameter

[Jason Gunthorpe <jgunthorpe@obsidianresearch.com>]

On Wed, Dec 02, 2015 at 11:27:27AM -0700, Jason Gunthorpe wrote:

> I'm guessing that if the driver probe order is tpm_crb,tpm_tis then
> things work because tpm_crb will claim the device first? Otherwise
> tpm_tis claims these things unconditionally? If the probe order is
> reversed things become broken?

Okay, I didn't find the is_fifo before, so that make sense

But this:

> What is the address tpm_tis should be using? I see two things, it
> either uses the x86 default address or it expects the ACPI to have a
> MEM resource. AFAIK ACPI should never rely on hard wired addresses, so
> I removed that code in this series. Perhaps tpm_tis should be using
> control_area_pa ? Will ACPI ever present a struct resource? (if yes,
> why isn't tpm_crb using one?)

Is then still a problem. On Martin's system the MSFT0101 device does
not have a struct resource attached to it. Does any system, or is this
just dead code?

Should the control_area_pa be used?

Martin: could you try this (along with the other hunk to prevent the
oops):

diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 8a3509cb10da..6824a00ba513 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -138,6 +138,8 @@ static inline int is_fifo(struct acpi_device *dev)
 	if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
 		return 0;
 
+	dev_err(&dev->dev, "control area pa is %x\n", tbl->control_area_pa);
+
 	/* TPM 2.0 FIFO */
 	return 1;
 }

Hoping to see it print 0xFED40000

> There is also something wrong with the endianness in the acpi
> stuff. I don't see endianness conversions in other acpi places, so I
> wonder if the ones in tpm_crb are correct. If they are correct then
> the struct needs le/be notations and there are some missing
> conversions.

I've made a patch to take care of this and move every thing to the
include/acpi/actbl2.h definitions, which is why I didn't notice
is_fifo in the first place...

Jason