| Message ID | 20151203181932.GA22973@obsidianresearch.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
> > 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
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
> > > 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
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
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;