diff mbox

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

Message ID 20151202191155.GA2832@obsidianresearch.com
State New
Headers show

Commit Message

Jason Gunthorpe Dec. 2, 2015, 7:11 p.m. UTC 
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):


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

------------------------------------------------------------------------------
Go from Idea to Many App Stores Faster with Intel(R) XDK
Give your users amazing mobile app experiences with Intel(R) XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140

Comments

Jarkko Sakkinen Dec. 3, 2015, 6 a.m. UTC | #1 
On Wed, Dec 02, 2015 at 12:11:55PM -0700, Jason Gunthorpe wrote:
> 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?

I guess it'd be more realiable. In my NUC the current fix works and the
people who tested it. If you supply me a fix that changes it to use that
I can test it and this will give also coverage to the people who tested
my original fix.

/Jarkko

------------------------------------------------------------------------------
Go from Idea to Many App Stores Faster with Intel(R) XDK
Give your users amazing mobile app experiences with Intel(R) XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
Martin Wilck Dec. 3, 2015, 8:30 a.m. UTC | #2 
On Mi, 2015-12-02 at 12:11 -0700, Jason Gunthorpe wrote:

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

ACPI defines a mem resource corresponding to the standard TIS memory
area on my system, and it used to be detected fine with Jarkko's patch.
Somehow your latest changes broke it, not sure why.

Martin

------------------------------------------------------------------------------
Go from Idea to Many App Stores Faster with Intel(R) XDK
Give your users amazing mobile app experiences with Intel(R) XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
Jason Gunthorpe Dec. 3, 2015, 5 p.m. UTC | #3 
On Thu, Dec 03, 2015 at 09:30:30AM +0100, Wilck, Martin wrote:
> On Mi, 2015-12-02 at 12:11 -0700, Jason Gunthorpe wrote:
> 
> > > 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?
> 
> ACPI defines a mem resource corresponding to the standard TIS memory
> area on my system, and it used to be detected fine with Jarkko's patch.
> Somehow your latest changes broke it, not sure why.

Are you certain? Based on what you sent me, that output is only
possible if there is no mem resource.

With the prior arrangement no mem resource means the x86 default
address is used, which is the only way I can see how your system
works.

Jason

------------------------------------------------------------------------------
Go from Idea to Many App Stores Faster with Intel(R) XDK
Give your users amazing mobile app experiences with Intel(R) XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
Martin Wilck Dec. 4, 2015, 8:39 a.m. UTC | #4 
On Do, 2015-12-03 at 10:00 -0700, Jason Gunthorpe wrote:
> On Thu, Dec 03, 2015 at 09:30:30AM +0100, Wilck, Martin wrote:
> > On Mi, 2015-12-02 at 12:11 -0700, Jason Gunthorpe wrote:
> > 
> > > > 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?
> > 
> > ACPI defines a mem resource corresponding to the standard TIS memory
> > area on my system, and it used to be detected fine with Jarkko's patch.
> > Somehow your latest changes broke it, not sure why.
> 
> Are you certain? Based on what you sent me, that output is only
> possible if there is no mem resource.

Yes, I am certain. I checked the DSDT, and I put a debug statement right
after the resource detection in tpm_tis.

Martin


> With the prior arrangement no mem resource means the x86 default
> address is used, which is the only way I can see how your system
> works.



> 
> Jason
------------------------------------------------------------------------------
Go from Idea to Many App Stores Faster with Intel(R) XDK
Give your users amazing mobile app experiences with Intel(R) XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
diff mbox

Patch

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;
 }