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

Message ID 20151203181932.GA22973@obsidianresearch.com
State New
Headers show

Commit Message

Jason Gunthorpe Dec. 3, 2015, 6:19 p.m. UTC
On Thu, Dec 03, 2015 at 08:00:42AM +0200, Jarkko Sakkinen wrote:

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

Here is the updated series:

https://github.com/jgunthorpe/linux/commits/for-jarkko

What does your dmesg say?

It really isn't OK to hardwire an address for acpi devices, so I've
added something like this. Just completely guessing that control_pa is
where the BIOS is hiding the base address. Maybe it is cca->cmd_pa ?

>From c9f7c0465008657f7fc7880496f68f4a1b3b4a26 Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Date: Thu, 3 Dec 2015 10:58:56 -0700
Subject: [PATCH 3/5] tpm_tis: Do not fall back to a hardcoded address for TPM2

If the ACPI tables do not declare a memory resource for the TPM2
then do not just fall back to the x86 default base address.

WIP: Guess that the control_address is the base address for the
TIS 1.2 memory mapped interface.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/char/tpm/tpm_tis.c | 50 +++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 30 deletions(-)

Comments

Jarkko Sakkinen Dec. 6, 2015, 4:02 a.m. UTC | #1
On Thu, Dec 03, 2015 at 11:19:32AM -0700, Jason Gunthorpe wrote:
> On Thu, Dec 03, 2015 at 08:00:42AM +0200, Jarkko Sakkinen wrote:
> 
> > 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.
> 
> Here is the updated series:
> 
> https://github.com/jgunthorpe/linux/commits/for-jarkko
> 
> What does your dmesg say?
> 
> It really isn't OK to hardwire an address for acpi devices, so I've
> added something like this. Just completely guessing that control_pa is
> where the BIOS is hiding the base address. Maybe it is cca->cmd_pa ?

I'm a bit confused about the discussion because Martin replied that
tpm_tis used to get the address range before applying this series.

And pnp_driver in the backend for TPM 1.x devices grabs the address
range from DSDT.

/Jarkko

> From c9f7c0465008657f7fc7880496f68f4a1b3b4a26 Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Date: Thu, 3 Dec 2015 10:58:56 -0700
> Subject: [PATCH 3/5] tpm_tis: Do not fall back to a hardcoded address for TPM2
> 
> If the ACPI tables do not declare a memory resource for the TPM2
> then do not just fall back to the x86 default base address.
> 
> WIP: Guess that the control_address is the base address for the
> TIS 1.2 memory mapped interface.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/char/tpm/tpm_tis.c | 50 +++++++++++++++++++---------------------------
>  1 file changed, 20 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index fecd27b45fd1..6b28f8003425 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -122,39 +122,11 @@ static inline int is_itpm(struct acpi_device *dev)
>  {
>  	return has_hid(dev, "INTC0102");
>  }
> -
> -static inline int is_fifo(struct acpi_device *dev)
> -{
> -	struct acpi_table_tpm2 *tbl;
> -	acpi_status st;
> -
> -	/* TPM 1.2 FIFO */
> -	if (!has_hid(dev, "MSFT0101"))
> -		return 1;
> -
> -	st = acpi_get_table(ACPI_SIG_TPM2, 1,
> -			    (struct acpi_table_header **) &tbl);
> -	if (ACPI_FAILURE(st)) {
> -		dev_err(&dev->dev, "failed to get TPM2 ACPI table\n");
> -		return 0;
> -	}
> -
> -	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
> -		return 0;
> -
> -	/* TPM 2.0 FIFO */
> -	return 1;
> -}
>  #else
>  static inline int is_itpm(struct acpi_device *dev)
>  {
>  	return 0;
>  }
> -
> -static inline int is_fifo(struct acpi_device *dev)
> -{
> -	return 1;
> -}
>  #endif
>  
>  /* Before we attempt to access the TPM we must see that the valid bit is set.
> @@ -980,11 +952,21 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
>  
>  static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>  {
> +	struct acpi_table_tpm2 *tbl;
> +	acpi_status st;
>  	struct list_head resources;
> -	struct tpm_info tpm_info = tis_default_info;
> +	struct tpm_info tpm_info = {};
>  	int ret;
>  
> -	if (!is_fifo(acpi_dev))
> +	st = acpi_get_table(ACPI_SIG_TPM2, 1,
> +			    (struct acpi_table_header **) &tbl);
> +	if (ACPI_FAILURE(st)) {
> +		dev_err(&acpi_dev->dev,
> +			FW_BUG "failed to get TPM2 ACPI table\n");
> +		return -ENODEV;
> +	}
> +
> +	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
>  		return -ENODEV;
>  
>  	INIT_LIST_HEAD(&resources);
> @@ -996,6 +978,14 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>  
>  	acpi_dev_free_resource_list(&resources);
>  
> +	if (tpm_info.start == 0 && tpm_info.len == 0) {
> +		tpm_info.start = tbl->control_address;
> +		tpm_info.len = TIS_MEM_LEN;
> +		dev_err(&acpi_dev->dev,
> +			FW_BUG "TPM2 ACPI table does not define a memory resource, using 0x%lx instead\n",
> +			tpm_info.start);
> +	}
> +
>  	if (is_itpm(acpi_dev))
>  		itpm = true;
>  
> -- 
> 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
Jarkko Sakkinen Dec. 6, 2015, 4:15 a.m. UTC | #2
On Sun, Dec 06, 2015 at 06:02:26AM +0200, Jarkko Sakkinen wrote:
> On Thu, Dec 03, 2015 at 11:19:32AM -0700, Jason Gunthorpe wrote:
> > On Thu, Dec 03, 2015 at 08:00:42AM +0200, Jarkko Sakkinen wrote:
> > 
> > > 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.
> > 
> > Here is the updated series:
> > 
> > https://github.com/jgunthorpe/linux/commits/for-jarkko
> > 
> > What does your dmesg say?
> > 
> > It really isn't OK to hardwire an address for acpi devices, so I've
> > added something like this. Just completely guessing that control_pa is
> > where the BIOS is hiding the base address. Maybe it is cca->cmd_pa ?
> 
> I'm a bit confused about the discussion because Martin replied that
> tpm_tis used to get the address range before applying this series.
> 
> And pnp_driver in the backend for TPM 1.x devices grabs the address
> range from DSDT.

You can completely ignore this question. I saw Martins reply with a fix for
"tpm_tis: Use devm_ioremap_resource" that you should squash into that
change. So it's proved that TPM ACPI device objects do not always have a
memory resource. Good.

I think these changes are important but there's no really reason to rush
them. Maybe, since there's been a lot of commentary, it'd be better to
resubmit a new revision of the series to the mailing list so that it can
be peer-reviewed once again.

> /Jarkko

/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, 4:20 a.m. UTC | #3
On Sun, Dec 06, 2015 at 06:15:44AM +0200, Jarkko Sakkinen wrote:
> On Sun, Dec 06, 2015 at 06:02:26AM +0200, Jarkko Sakkinen wrote:
> > On Thu, Dec 03, 2015 at 11:19:32AM -0700, Jason Gunthorpe wrote:
> > > On Thu, Dec 03, 2015 at 08:00:42AM +0200, Jarkko Sakkinen wrote:
> > > 
> > > > 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.
> > > 
> > > Here is the updated series:
> > > 
> > > https://github.com/jgunthorpe/linux/commits/for-jarkko
> > > 
> > > What does your dmesg say?
> > > 
> > > It really isn't OK to hardwire an address for acpi devices, so I've
> > > added something like this. Just completely guessing that control_pa is
> > > where the BIOS is hiding the base address. Maybe it is cca->cmd_pa ?
> > 
> > I'm a bit confused about the discussion because Martin replied that
> > tpm_tis used to get the address range before applying this series.
> > 
> > And pnp_driver in the backend for TPM 1.x devices grabs the address
> > range from DSDT.
> 
> You can completely ignore this question. I saw Martins reply with a fix for
> "tpm_tis: Use devm_ioremap_resource" that you should squash into that
> change. So it's proved that TPM ACPI device objects do not always have a
> memory resource. Good.
> 
> I think these changes are important but there's no really reason to rush
> them. Maybe, since there's been a lot of commentary, it'd be better to
> resubmit a new revision of the series to the mailing list so that it can
> be peer-reviewed once again.

Maybe even there could be a common tpm_tcg driver once the common code
has been factored out (at some point, lets take this step by step and
fix the issues first). Transmit functions are not heavy and ACPI stuff
is mostly the same.

/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:15 a.m. UTC | #4
On Sun, Dec 06, 2015 at 06:15:44AM +0200, Jarkko Sakkinen wrote:
> You can completely ignore this question. I saw Martins reply with a fix for
> "tpm_tis: Use devm_ioremap_resource" that you should squash into that
> change.

It isn't quite the right fix - but I've added something that should be OK
now that Martin has debugged it.

> So it's proved that TPM ACPI device objects do not always have a
> memory resource. Good.

Hopefully.. It is so confusing because tpm_tis had that wrong
fallback, and tpm_crb incorrectly doesn't use the resource
structure. So we've never actually had anything that requires the
resource struct for 2.0 until now.

> resubmit a new revision of the series to the mailing list so that it can
> be peer-reviewed once again.

If Martin is happy I will send them to the list, the latest patches
are on my github:

https://github.com/jgunthorpe/linux/commits/for-jarkko

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. 7, 2015, 8:06 a.m. UTC | #5
> > I'm a bit confused about the discussion because Martin replied that
> > tpm_tis used to get the address range before applying this series.
> > 
> > And pnp_driver in the backend for TPM 1.x devices grabs the address
> > range from DSDT.
> 
> You can completely ignore this question. I saw Martins reply with a fix for
> "tpm_tis: Use devm_ioremap_resource" that you should squash into that
> change. So it's proved that TPM ACPI device objects do not always have a
> memory resource. Good.

Repeat, the memory resource DOES exist on my system. Not sure what proof
you saw there.

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
Jarkko Sakkinen Dec. 7, 2015, 8:56 a.m. UTC | #6
On Mon, Dec 07, 2015 at 09:06:50AM +0100, Wilck, Martin wrote:
> > > I'm a bit confused about the discussion because Martin replied that
> > > tpm_tis used to get the address range before applying this series.
> > > 
> > > And pnp_driver in the backend for TPM 1.x devices grabs the address
> > > range from DSDT.
> > 
> > You can completely ignore this question. I saw Martins reply with a fix for
> > "tpm_tis: Use devm_ioremap_resource" that you should squash into that
> > change. So it's proved that TPM ACPI device objects do not always have a
> > memory resource. Good.
> 
> Repeat, the memory resource DOES exist on my system. Not sure what proof
> you saw there.

Ok, lets go this through.

I deduced this from two facts:

* It used to have memory resource as conditional and as a fallback use
  fixed value.
* Your workaround reverted the situation to this.

Did I understand something incorrectly?

/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, 9:52 a.m. UTC | #7
> > > You can completely ignore this question. I saw Martins reply with a fix for
> > > "tpm_tis: Use devm_ioremap_resource" that you should squash into that
> > > change. So it's proved that TPM ACPI device objects do not always have a
> > > memory resource. Good.
> > 
> > Repeat, the memory resource DOES exist on my system. Not sure what proof
> > you saw there.
> 
> Ok, lets go this through.
> 
> I deduced this from two facts:
> 
> * It used to have memory resource as conditional and as a fallback use
>   fixed value.
> * Your workaround reverted the situation to this.
> 
> Did I understand something incorrectly?

The problem in my case didn't occur because ACPI was lacking a resource.
It has one "extra" resource that Jason's original code didn't
recognize. 

Jason's code was wrongly assuming that a resource that isn't of type
"IRQ" has to be of type "MEMORY". If I print out the resource types
encountered in tpm_check_resource(), I get
ACPI_RESOURCE_TYPE_FIXED_MEMORY32  (0x0a) first, followed by
ACPI_RESOURCE_TYPE_END_TAG (0x07). The latter was mistakenly used by
Jason't code as a memory resource. This is how ACPI ResourceTemplates
work (a list with an end marker). The correct solution is to always
check the return value of acpi_dev_resource_memory(), as it's currently
implemented in Jason't current "for-jarkko" branch.

Martin


> 
> /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. 7, 2015, 10:16 a.m. UTC | #8
On Mon, Dec 07, 2015 at 10:52:51AM +0100, Wilck, Martin wrote:
> > > > You can completely ignore this question. I saw Martins reply with a fix for
> > > > "tpm_tis: Use devm_ioremap_resource" that you should squash into that
> > > > change. So it's proved that TPM ACPI device objects do not always have a
> > > > memory resource. Good.
> > > 
> > > Repeat, the memory resource DOES exist on my system. Not sure what proof
> > > you saw there.
> > 
> > Ok, lets go this through.
> > 
> > I deduced this from two facts:
> > 
> > * It used to have memory resource as conditional and as a fallback use
> >   fixed value.
> > * Your workaround reverted the situation to this.
> > 
> > Did I understand something incorrectly?
> 
> The problem in my case didn't occur because ACPI was lacking a resource.
> It has one "extra" resource that Jason's original code didn't
> recognize. 
> 
> Jason's code was wrongly assuming that a resource that isn't of type
> "IRQ" has to be of type "MEMORY". If I print out the resource types
> encountered in tpm_check_resource(), I get
> ACPI_RESOURCE_TYPE_FIXED_MEMORY32  (0x0a) first, followed by
> ACPI_RESOURCE_TYPE_END_TAG (0x07). The latter was mistakenly used by
> Jason't code as a memory resource. This is how ACPI ResourceTemplates
> work (a list with an end marker). The correct solution is to always
> check the return value of acpi_dev_resource_memory(), as it's currently
> implemented in Jason't current "for-jarkko" branch.

Aah. Right.

> Martin

/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_tis.c b/drivers/char/tpm/tpm_tis.c
index fecd27b45fd1..6b28f8003425 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -122,39 +122,11 @@  static inline int is_itpm(struct acpi_device *dev)
 {
 	return has_hid(dev, "INTC0102");
 }
-
-static inline int is_fifo(struct acpi_device *dev)
-{
-	struct acpi_table_tpm2 *tbl;
-	acpi_status st;
-
-	/* TPM 1.2 FIFO */
-	if (!has_hid(dev, "MSFT0101"))
-		return 1;
-
-	st = acpi_get_table(ACPI_SIG_TPM2, 1,
-			    (struct acpi_table_header **) &tbl);
-	if (ACPI_FAILURE(st)) {
-		dev_err(&dev->dev, "failed to get TPM2 ACPI table\n");
-		return 0;
-	}
-
-	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
-		return 0;
-
-	/* TPM 2.0 FIFO */
-	return 1;
-}
 #else
 static inline int is_itpm(struct acpi_device *dev)
 {
 	return 0;
 }
-
-static inline int is_fifo(struct acpi_device *dev)
-{
-	return 1;
-}
 #endif
 
 /* Before we attempt to access the TPM we must see that the valid bit is set.
@@ -980,11 +952,21 @@  static int tpm_check_resource(struct acpi_resource *ares, void *data)
 
 static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
 {
+	struct acpi_table_tpm2 *tbl;
+	acpi_status st;
 	struct list_head resources;
-	struct tpm_info tpm_info = tis_default_info;
+	struct tpm_info tpm_info = {};
 	int ret;
 
-	if (!is_fifo(acpi_dev))
+	st = acpi_get_table(ACPI_SIG_TPM2, 1,
+			    (struct acpi_table_header **) &tbl);
+	if (ACPI_FAILURE(st)) {
+		dev_err(&acpi_dev->dev,
+			FW_BUG "failed to get TPM2 ACPI table\n");
+		return -ENODEV;
+	}
+
+	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
 		return -ENODEV;
 
 	INIT_LIST_HEAD(&resources);
@@ -996,6 +978,14 @@  static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
 
 	acpi_dev_free_resource_list(&resources);
 
+	if (tpm_info.start == 0 && tpm_info.len == 0) {
+		tpm_info.start = tbl->control_address;
+		tpm_info.len = TIS_MEM_LEN;
+		dev_err(&acpi_dev->dev,
+			FW_BUG "TPM2 ACPI table does not define a memory resource, using 0x%lx instead\n",
+			tpm_info.start);
+	}
+
 	if (is_itpm(acpi_dev))
 		itpm = true;