[tpmdd-devel] tpm_crb: Use the common ACPI definition of struct acpi_tpm2
diff mbox

Message ID 20151202191900.GA12489@obsidianresearch.com
State New
Headers show

Commit Message

Jason Gunthorpe Dec. 2, 2015, 7:19 p.m. UTC
include/acpi/actbl2.h is the proper place for these definitions
and the needed TPM2 ones have been there since
commit 413d4a6defe0 ("ACPICA: Update TPM2 ACPI table")

This also drops the le32_to_cpu for members of this table,
consistent with other ACPI stuff.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/char/tpm/tpm.h     |  7 -------
 drivers/char/tpm/tpm_crb.c | 24 ++++++++----------------
 drivers/char/tpm/tpm_tis.c |  2 +-
 3 files changed, 9 insertions(+), 24 deletions(-)

Comments

Jarkko Sakkinen Dec. 3, 2015, 8:27 a.m. UTC | #1
On Wed, Dec 02, 2015 at 12:19:00PM -0700, Jason Gunthorpe wrote:
> include/acpi/actbl2.h is the proper place for these definitions
> and the needed TPM2 ones have been there since
> commit 413d4a6defe0 ("ACPICA: Update TPM2 ACPI table")
> 
> This also drops the le32_to_cpu for members of this table,
> consistent with other ACPI stuff.

Why le32_to_cpu() is not needed?

/Jarkko

> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/char/tpm/tpm.h     |  7 -------
>  drivers/char/tpm/tpm_crb.c | 24 ++++++++----------------
>  drivers/char/tpm/tpm_tis.c |  2 +-
>  3 files changed, 9 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 542a80cbfd9c..28b477e8da6a 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -128,13 +128,6 @@ enum tpm2_startup_types {
>  	TPM2_SU_STATE	= 0x0001,
>  };
>  
> -enum tpm2_start_method {
> -	TPM2_START_ACPI = 2,
> -	TPM2_START_FIFO = 6,
> -	TPM2_START_CRB = 7,
> -	TPM2_START_CRB_WITH_ACPI = 8,
> -};
> -
>  struct tpm_chip;
>  
>  struct tpm_vendor_specific {
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 8342cf51ffdc..5b74ad8ca7fd 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,14 +34,6 @@ enum crb_defaults {
>  	CRB_ACPI_START_INDEX = 1,
>  };
>  
> -struct acpi_tpm2 {
> -	struct acpi_table_header hdr;
> -	u16 platform_class;
> -	u16 reserved;
> -	u64 control_area_pa;
> -	u32 start_method;
> -} __packed;
> -
>  enum crb_ca_request {
>  	CRB_CA_REQ_GO_IDLE	= BIT(0),
>  	CRB_CA_REQ_CMD_READY	= BIT(1),
> @@ -207,7 +199,7 @@ static const struct tpm_class_ops tpm_crb = {
>  static int crb_acpi_add(struct acpi_device *device)
>  {
>  	struct tpm_chip *chip;
> -	struct acpi_tpm2 *buf;
> +	struct acpi_table_tpm2 *buf;
>  	struct crb_priv *priv;
>  	struct device *dev = &device->dev;
>  	acpi_status status;
> @@ -223,7 +215,8 @@ static int crb_acpi_add(struct acpi_device *device)
>  	}
>  
>  	/* Should the FIFO driver handle this? */
> -	if (buf->start_method == TPM2_START_FIFO)
> +	sm = buf->start_method;
> +	if (sm == ACPI_TPM2_MEMORY_MAPPED)
>  		return -ENODEV;
>  
>  	chip = tpmm_chip_alloc(dev, &tpm_crb);
> @@ -232,7 +225,7 @@ static int crb_acpi_add(struct acpi_device *device)
>  
>  	chip->flags = TPM_CHIP_FLAG_TPM2;
>  
> -	if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
> +	if (buf->header.length < sizeof(*buf)) {
>  		dev_err(dev, "TPM2 ACPI table has wrong size");
>  		return -EINVAL;
>  	}
> @@ -244,21 +237,20 @@ static int crb_acpi_add(struct acpi_device *device)
>  		return -ENOMEM;
>  	}
>  
> -	sm = le32_to_cpu(buf->start_method);
> -
>  	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
>  	 * report only ACPI start but in practice seems to require both
>  	 * ACPI start and CRB start.
>  	 */
> -	if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
> +	if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
>  	    !strcmp(acpi_device_hid(device), "MSFT0101"))
>  		priv->flags |= CRB_FL_CRB_START;
>  
> -	if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
> +	if (sm == ACPI_TPM2_START_METHOD ||
> +	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>  		priv->flags |= CRB_FL_ACPI_START;
>  
>  	priv->cca = (struct crb_control_area __iomem *)
> -		devm_ioremap_nocache(dev, buf->control_area_pa, 0x1000);
> +		devm_ioremap_nocache(dev, buf->control_address, 0x1000);
>  	if (!priv->cca) {
>  		dev_err(dev, "ioremap of the control area failed\n");
>  		return -ENOMEM;
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 8a3509cb10da..304323bdcaaa 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -135,7 +135,7 @@ static inline int is_fifo(struct acpi_device *dev)
>  		return 0;
>  	}
>  
> -	if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
> +	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
>  		return 0;
>  
>  	/* TPM 2.0 FIFO */
> -- 
> 2.1.4
> 

------------------------------------------------------------------------------
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:02 p.m. UTC | #2
On Thu, Dec 03, 2015 at 10:27:53AM +0200, Jarkko Sakkinen wrote:
> On Wed, Dec 02, 2015 at 12:19:00PM -0700, Jason Gunthorpe wrote:
> > include/acpi/actbl2.h is the proper place for these definitions
> > and the needed TPM2 ones have been there since
> > commit 413d4a6defe0 ("ACPICA: Update TPM2 ACPI table")
> > 
> > This also drops the le32_to_cpu for members of this table,
> > consistent with other ACPI stuff.
> 
> Why le32_to_cpu() is not needed?

Why is it needed?

None of the fields are marked le32 in the struct and no other ACPI
thing I could find swaps.

If swapping is needed then the struct fields must be declared as 'le'

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:24 a.m. UTC | #3
> > Why le32_to_cpu() is not needed?
> 
> Why is it needed?
> 
> None of the fields are marked le32 in the struct and no other ACPI
> thing I could find swaps.

TCG ACPI Specification
http://www.trustedcomputinggroup.org/files/static_page_files/DCD4188E-1A4B-B294-D050A155FB6F7385/TCG_ACPIGeneralSpecification_PublicReview.pdf

§7: "ACPI Tables have the little endian byte format defined in the ACPI
specification"

ACPI Spec 5.0
http://acpi.info/DOWNLOADS/ACPI_5_Errata%20A.pdf

§5.2 "All numeric values in ACPI-defined tables, blocks, and structures
are always encoded in little endian format."

So, unless the byte swapping is already done somewhere deeply in the
ACPICA code, it needs to be done by the driver.

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. 4, 2015, 6:29 p.m. UTC | #4
On Fri, Dec 04, 2015 at 09:24:05AM +0100, Wilck, Martin wrote:
> TCG ACPI Specification
> http://www.trustedcomputinggroup.org/files/static_page_files/DCD4188E-1A4B-B294-D050A155FB6F7385/TCG_ACPIGeneralSpecification_PublicReview.pdf
> 
> §7: "ACPI Tables have the little endian byte format defined in the ACPI
> specification"

Yes, I know.

Look at any caller to acpi_get_table and none byteswap the resulting
structure, even other things in TPM, covered by the same spec (eg
acpi_table_tcpa_server in tpm_acpi.c)

After a little research, I think Linux doesn't support ACPI on BE
systems at all.

https://lists.linaro.org/pipermail/linaro-kernel/2015-September/023455.html
http://bugs.acpica.org/show_bug.cgi?id=1082 (#4)

Which is probably why nothing using ACPI tables has the swap.

Converting ACPI to BE is a big job, there is no reason tpm_crb should
be outside the norm, and certainly no reason to do it wrong by
calling l32_to_cpu on a u32 value.

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
Jarkko Sakkinen Dec. 6, 2015, 1:25 a.m. UTC | #5
On Thu, Dec 03, 2015 at 10:02:17AM -0700, Jason Gunthorpe wrote:
> On Thu, Dec 03, 2015 at 10:27:53AM +0200, Jarkko Sakkinen wrote:
> > On Wed, Dec 02, 2015 at 12:19:00PM -0700, Jason Gunthorpe wrote:
> > > include/acpi/actbl2.h is the proper place for these definitions
> > > and the needed TPM2 ones have been there since
> > > commit 413d4a6defe0 ("ACPICA: Update TPM2 ACPI table")
> > > 
> > > This also drops the le32_to_cpu for members of this table,
> > > consistent with other ACPI stuff.
> > 
> > Why le32_to_cpu() is not needed?
> 
> Why is it needed?
> 
> None of the fields are marked le32 in the struct and no other ACPI
> thing I could find swaps.
> 
> If swapping is needed then the struct fields must be declared as 'le'

I'm just puzzling with three things:

1. Does it do conversion to the CPU format internally if you use the
   tables in a big-endian environment?
2. Isn't that code import from ACPICA? That would explains why it does
   not have tagged fields.
3. Could the reason be for such conversions not widely applied in the
   kernel that ACPI has been previously mostly used only in the PC/x86
   ecosystem?

AFAIK ACPI exist also for ARM64 servers, which can be a big-endian
environment.

> Jason

/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
Jarkko Sakkinen Dec. 6, 2015, 1:29 a.m. UTC | #6
On Fri, Dec 04, 2015 at 11:29:49AM -0700, Jason Gunthorpe wrote:
> On Fri, Dec 04, 2015 at 09:24:05AM +0100, Wilck, Martin wrote:
> > TCG ACPI Specification
> > http://www.trustedcomputinggroup.org/files/static_page_files/DCD4188E-1A4B-B294-D050A155FB6F7385/TCG_ACPIGeneralSpecification_PublicReview.pdf
> > 
> > §7: "ACPI Tables have the little endian byte format defined in the ACPI
> > specification"
> 
> Yes, I know.
> 
> Look at any caller to acpi_get_table and none byteswap the resulting
> structure, even other things in TPM, covered by the same spec (eg
> acpi_table_tcpa_server in tpm_acpi.c)
> 
> After a little research, I think Linux doesn't support ACPI on BE
> systems at all.
> 
> https://lists.linaro.org/pipermail/linaro-kernel/2015-September/023455.html
> http://bugs.acpica.org/show_bug.cgi?id=1082 (#4)
> 
> Which is probably why nothing using ACPI tables has the swap.
> 
> Converting ACPI to BE is a big job, there is no reason tpm_crb should
> be outside the norm, and certainly no reason to do it wrong by
> calling l32_to_cpu on a u32 value.

So you are suggesting that the driver should be made  more broken because
it's not common?

> Jason

/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
Jason Gunthorpe Dec. 7, 2015, 5:59 a.m. UTC | #7
On Sun, Dec 06, 2015 at 03:29:19AM +0200, Jarkko Sakkinen wrote:
> > Converting ACPI to BE is a big job, there is no reason tpm_crb should
> > be outside the norm, and certainly no reason to do it wrong by
> > calling l32_to_cpu on a u32 value.
> 
> So you are suggesting that the driver should be made  more broken because
> it's not common?

No, I'm suggesting it already made no sense. It wasn't doing the swaps
properly, it was missing swaps in some places, it didn't have the
proper static checker annotations, and the swaps serve no purpose
since the rest of ACPI is broken anyhow.

ARM ACPI is already restricted to LE only.

Someday someone might fix this, but they have to fix all of it, and
having a broken version in crb does nothing to help that future
person.

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
Jarkko Sakkinen Dec. 7, 2015, 6:15 a.m. UTC | #8
On Sun, Dec 06, 2015 at 10:59:50PM -0700, Jason Gunthorpe wrote:
> On Sun, Dec 06, 2015 at 03:29:19AM +0200, Jarkko Sakkinen wrote:
> > > Converting ACPI to BE is a big job, there is no reason tpm_crb should
> > > be outside the norm, and certainly no reason to do it wrong by
> > > calling l32_to_cpu on a u32 value.
> > 
> > So you are suggesting that the driver should be made  more broken because
> > it's not common?
> 
> No, I'm suggesting it already made no sense. It wasn't doing the swaps
> properly, it was missing swaps in some places, it didn't have the
> proper static checker annotations, and the swaps serve no purpose
> since the rest of ACPI is broken anyhow.

If the conversions are missing in some places, I'd rather see those
fixed.

> ARM ACPI is already restricted to LE only.

Do you have a reference that it will stay like that forever to the
eternity?

> Someday someone might fix this, but they have to fix all of it, and
> having a broken version in crb does nothing to help that future
> person.
> 
> Jason

/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
Jason Gunthorpe Dec. 7, 2015, 6:32 a.m. UTC | #9
On Mon, Dec 07, 2015 at 08:15:20AM +0200, Jarkko Sakkinen wrote:

> If the conversions are missing in some places, I'd rather see those
> fixed.

The conversions are missing in the majority of places.

It isn't maintainable, and I think it is flat out wrong, to swap
untagged fields. Mistakes will keep being made.

These are all unswapped in crb.
	if (buf->start_method == TPM2_START_FIFO)
	   if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
		devm_ioremap_nocache(dev, buf->control_area_pa,
		0x1000);

Everything in ppi is unswapped.

Every ACPI driver doesn't swap hdr.length.

> > ARM ACPI is already restricted to LE only.
> 
> Do you have a reference that it will stay like that forever to the
> eternity?

Eh? It is code. It can be changed. I don't care at all about
forever. If someone wants to fix all of ACPI to work on BE then they
can go and fix everything.

I care that the ACPI maintainers don't seem to care about this issue,
so I *know* TPM shouldn't be the leading edge here. Follow their lead.

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
Jarkko Sakkinen Dec. 7, 2015, 6:53 a.m. UTC | #10
On Sun, Dec 06, 2015 at 11:32:43PM -0700, Jason Gunthorpe wrote:
> On Mon, Dec 07, 2015 at 08:15:20AM +0200, Jarkko Sakkinen wrote:
> 
> > If the conversions are missing in some places, I'd rather see those
> > fixed.
> 
> The conversions are missing in the majority of places.
> 
> It isn't maintainable, and I think it is flat out wrong, to swap
> untagged fields. Mistakes will keep being made.

This is not really an argument as ACPICA is imported code.

I don't think it'd have tagged fields anyhow. Is your argument
actually that conversions should be done inside ACPICA?

/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. 7, 2015, 8:18 a.m. UTC | #11
On Fr, 2015-12-04 at 11:29 -0700, Jason Gunthorpe wrote:
> After a little research, I think Linux doesn't support ACPI on BE
> systems at all.
> 
> https://lists.linaro.org/pipermail/linaro-kernel/2015-September/023455.html
> http://bugs.acpica.org/show_bug.cgi?id=1082 (#4)
> 
> Which is probably why nothing using ACPI tables as the swap.

That comment is about iasl.

Comment #2 in the same bug says 'The ACPICA core uses macros to
implement "endian independence"'. I am wondering what that means for
ACPI in the kernel.

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. 7, 2015, 5:45 p.m. UTC | #12
On Mon, Dec 07, 2015 at 08:53:36AM +0200, Jarkko Sakkinen wrote:
> > It isn't maintainable, and I think it is flat out wrong, to swap
> > untagged fields. Mistakes will keep being made.
> 
> This is not really an argument as ACPICA is imported code.

No, the idea of 'imported code' is contrary to everything about the
Linux development process. If swaps are needed the ACPI maintainers
will have to modify those files to add endianness tags for Linux.

> I don't think it'd have tagged fields anyhow. Is your argument
> actually that conversions should be done inside ACPICA?

No idea what the ACPI maintainers would do.

I've updated the commit comment with your notes.

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
Jarkko Sakkinen Dec. 7, 2015, 6:50 p.m. UTC | #13
On Mon, Dec 07, 2015 at 10:45:16AM -0700, Jason Gunthorpe wrote:
> On Mon, Dec 07, 2015 at 08:53:36AM +0200, Jarkko Sakkinen wrote:
> > > It isn't maintainable, and I think it is flat out wrong, to swap
> > > untagged fields. Mistakes will keep being made.
> > 
> > This is not really an argument as ACPICA is imported code.
> 
> No, the idea of 'imported code' is contrary to everything about the
> Linux development process. If swaps are needed the ACPI maintainers
> will have to modify those files to add endianness tags for Linux.
> 
> > I don't think it'd have tagged fields anyhow. Is your argument
> > actually that conversions should be done inside ACPICA?
> 
> No idea what the ACPI maintainers would do.
> 
> I've updated the commit comment with your notes.

Thanks. It's good to have this documented anyhow.

> Jason

/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

Patch
diff mbox

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 542a80cbfd9c..28b477e8da6a 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -128,13 +128,6 @@  enum tpm2_startup_types {
 	TPM2_SU_STATE	= 0x0001,
 };
 
-enum tpm2_start_method {
-	TPM2_START_ACPI = 2,
-	TPM2_START_FIFO = 6,
-	TPM2_START_CRB = 7,
-	TPM2_START_CRB_WITH_ACPI = 8,
-};
-
 struct tpm_chip;
 
 struct tpm_vendor_specific {
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 8342cf51ffdc..5b74ad8ca7fd 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,14 +34,6 @@  enum crb_defaults {
 	CRB_ACPI_START_INDEX = 1,
 };
 
-struct acpi_tpm2 {
-	struct acpi_table_header hdr;
-	u16 platform_class;
-	u16 reserved;
-	u64 control_area_pa;
-	u32 start_method;
-} __packed;
-
 enum crb_ca_request {
 	CRB_CA_REQ_GO_IDLE	= BIT(0),
 	CRB_CA_REQ_CMD_READY	= BIT(1),
@@ -207,7 +199,7 @@  static const struct tpm_class_ops tpm_crb = {
 static int crb_acpi_add(struct acpi_device *device)
 {
 	struct tpm_chip *chip;
-	struct acpi_tpm2 *buf;
+	struct acpi_table_tpm2 *buf;
 	struct crb_priv *priv;
 	struct device *dev = &device->dev;
 	acpi_status status;
@@ -223,7 +215,8 @@  static int crb_acpi_add(struct acpi_device *device)
 	}
 
 	/* Should the FIFO driver handle this? */
-	if (buf->start_method == TPM2_START_FIFO)
+	sm = buf->start_method;
+	if (sm == ACPI_TPM2_MEMORY_MAPPED)
 		return -ENODEV;
 
 	chip = tpmm_chip_alloc(dev, &tpm_crb);
@@ -232,7 +225,7 @@  static int crb_acpi_add(struct acpi_device *device)
 
 	chip->flags = TPM_CHIP_FLAG_TPM2;
 
-	if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
+	if (buf->header.length < sizeof(*buf)) {
 		dev_err(dev, "TPM2 ACPI table has wrong size");
 		return -EINVAL;
 	}
@@ -244,21 +237,20 @@  static int crb_acpi_add(struct acpi_device *device)
 		return -ENOMEM;
 	}
 
-	sm = le32_to_cpu(buf->start_method);
-
 	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
 	 * report only ACPI start but in practice seems to require both
 	 * ACPI start and CRB start.
 	 */
-	if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
+	if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
 	    !strcmp(acpi_device_hid(device), "MSFT0101"))
 		priv->flags |= CRB_FL_CRB_START;
 
-	if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
+	if (sm == ACPI_TPM2_START_METHOD ||
+	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
 		priv->flags |= CRB_FL_ACPI_START;
 
 	priv->cca = (struct crb_control_area __iomem *)
-		devm_ioremap_nocache(dev, buf->control_area_pa, 0x1000);
+		devm_ioremap_nocache(dev, buf->control_address, 0x1000);
 	if (!priv->cca) {
 		dev_err(dev, "ioremap of the control area failed\n");
 		return -ENOMEM;
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index 8a3509cb10da..304323bdcaaa 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -135,7 +135,7 @@  static inline int is_fifo(struct acpi_device *dev)
 		return 0;
 	}
 
-	if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
+	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
 		return 0;
 
 	/* TPM 2.0 FIFO */