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

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

On Wed, Dec 02, 2015 at 11:27:27AM -0700, Jason Gunthorpe wrote:
> On Tue, Dec 01, 2015 at 11:33:51PM +0200, Jarkko Sakkinen wrote:
> > On Tue, Dec 01, 2015 at 11:58:26AM -0700, Jason Gunthorpe wrote:
> 
> > I went through the patches and didn't see anything that would shock me
> > enough not to apply the patches in the current if they also work when
> > tested *but* are these release critical for Linux v4.4?
> 
> Jarkko,
> 
> Can you explain how
> 
> commit 399235dc6e95400a1322a9999e92073bc572f0c8
> Author: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Date:   Tue Sep 29 00:32:19 2015 +0300
> 
>     tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0
> 
> Is supposed to work? I get the jist of the idea, but I'm not seeing
> how it can work reliably..

The idea is that circulate the problem that pnp driver infra can pass at
most 7 character device IDs and MSFT0101 (used for TPM2 devices) has 8
characters. They have disjoint sets of device IDs so both cannot ever
attach. I don't know who was idiot enough to invent 8 character device
ID for TPM2 devices but that's the reality.

It's not a perfect fix but I couldn't figure out anything more clever
at that time. And nobody else was paying attention to the issue so
I had to do something and people who reported bug tested the patch and
were happy so I'm confident I did the right thing in the situation.

> The idea is to pass off TPM2_START_FIFO to tpm_tis?
> 
> 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?
> 
> 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?)

Doesn't also PNP driver do this assumption when the backend is ACPI?

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

/Jarkko