Patch "tpm_tis: reserve chip for duration of tpm_tis_core_init" has been added to the 5.4-stable tree

This is a note to let you know that I've just added the patch titled

tpm_tis: reserve chip for duration of tpm_tis_core_init

to the 5.4-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
tpm_tis-reserve-chip-for-duration-of-tpm_tis_core_init.patch
and it can be found in the queue-5.4 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

From 21df4a8b6018b842d4db181a8b24166006bad3cd Mon Sep 17 00:00:00 2001
From: Jerry Snitselaar <[email protected]>
Date: Wed, 11 Dec 2019 16:54:55 -0700
Subject: tpm_tis: reserve chip for duration of tpm_tis_core_init

From: Jerry Snitselaar <[email protected]>

commit 21df4a8b6018b842d4db181a8b24166006bad3cd upstream.

Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when issuing commands to the tpm during initialization, just reserve the chip after wait_startup, and release it when we are ready to call tpm_chip_register.

Cc Christian Bundy <[email protected]>

Cc Dan Williams <[email protected]>

Cc Peter Huewe <[email protected]>

Cc Jarkko Sakkinen <[email protected]>

Cc Jason Gunthorpe <[email protected]>

Cc Stefan Berger <[email protected]>

Cc [email protected]

Cc [email protected]

Fixes a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")

Fixes 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")

Suggested-by Jarkko Sakkinen <[email protected]>

Reviewed-by Jarkko Sakkinen <[email protected]>

Signed-off-by Jerry Snitselaar <[email protected]>

Signed-off-by Jarkko Sakkinen <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/char/tpm/tpm_tis_core.c |   35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

drivers/char/tpm/tpm_tis_core.c +18 -17

@@ -899,13 +899,13 @@ int tpm_tis_core_init(struct device *dev
 
        if (wait_startup(chip, 0) != 0) {
                rc = -ENODEV;
-               goto out_err;
+               goto err_start;
        }
 
        /* Take control of the TPM's interrupt hardware and shut it off */
        rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
        if (rc < 0)
-               goto out_err;
+               goto err_start;
 
        intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
                   TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;

@@ -914,21 +914,21 @@ int tpm_tis_core_init(struct device *dev
 
        rc = tpm_chip_start(chip);
        if (rc)
-               goto out_err;
+               goto err_start;
+
        rc = tpm2_probe(chip);
-       tpm_chip_stop(chip);
        if (rc)
-               goto out_err;
+               goto err_probe;
 
        rc = tpm_tis_read32(priv, TPM_DID_VID(0), &vendor);
        if (rc < 0)
-               goto out_err;
+               goto err_probe;
 
        priv->manufacturer_id = vendor;
 
        rc = tpm_tis_read8(priv, TPM_RID(0), &rid);
        if (rc < 0)
-               goto out_err;
+               goto err_probe;
 
        dev_info(dev, "%s TPM (device-id 0x%X, rev-id %d)\n",
                 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2",

@@ -937,13 +937,13 @@ int tpm_tis_core_init(struct device *dev
        probe = probe_itpm(chip);
        if (probe < 0) {
                rc = -ENODEV;
-               goto out_err;
+               goto err_probe;
        }
 
        /* Figure out the capabilities */
        rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
        if (rc < 0)
-               goto out_err;
+               goto err_probe;
 
        dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
                intfcaps);

@@ -977,10 +977,9 @@ int tpm_tis_core_init(struct device *dev
                if (tpm_get_timeouts(chip)) {
                        dev_err(dev, "Could not get TPM timeouts and durations\n");
                        rc = -ENODEV;
-                       goto out_err;
+                       goto err_probe;
                }
 
-               tpm_chip_start(chip);
                chip->flags |= TPM_CHIP_FLAG_IRQ;
                if (irq) {
                        tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,

@@ -991,18 +990,20 @@ int tpm_tis_core_init(struct device *dev
                } else {
                        tpm_tis_probe_irq(chip, intmask);
                }
-               tpm_chip_stop(chip);
        }
 
+       tpm_chip_stop(chip);
+
        rc = tpm_chip_register(chip);
        if (rc)
-               goto out_err;
-
-       if (chip->ops->clk_enable != NULL)
-               chip->ops->clk_enable(chip, false);
+               goto err_start;
 
        return 0;
-out_err:
+
+err_probe:
+       tpm_chip_stop(chip);
+
+err_start:
        if ((chip->ops != NULL) && (chip->ops->clk_enable != NULL))
                chip->ops->clk_enable(chip, false);
 

Patches currently in stable-queue which might be from [email protected] are

queue-5.4/iommu-fix-kasan-use-after-free-in-iommu_insert_resv_region.patch queue-5.4/iommu-vt-d-fix-dmar-pte-read-access-not-set-error.patch queue-5.4/iommu-set-group-default-domain-before-creating-direct-mappings.patch queue-5.4/tpm_tis-reserve-chip-for-duration-of-tpm_tis_core_init.patch queue-5.4/iommu-vt-d-allocate-reserved-region-for-isa-with-correct-permission.patch queue-5.4/iommu-vt-d-set-isa-bridge-reserved-region-as-relaxable.patch

Hi Greg,

Please drop the:

Fixes 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")

...tag. I had asked Jarkko to do that here:

https://lore.kernel.org/r/CAPcyv4h60z889bfbiwvVhsj6MxmOPiPY8ZuPB_skxkZx-N+OGw@mail.gmail.com/

...but it didn't get picked up.

Quoted 160 lines · On Mon, Dec 23, 2019 at 9:37 AM <[email protected]> wrote:

This is a note to let you know that I've just added the patch titled

tpm_tis: reserve chip for duration of tpm_tis_core_init

to the 5.4-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
tpm_tis-reserve-chip-for-duration-of-tpm_tis_core_init.patch
and it can be found in the queue-5.4 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

From 21df4a8b6018b842d4db181a8b24166006bad3cd Mon Sep 17 00:00:00 2001
From: Jerry Snitselaar <[email protected]>
Date: Wed, 11 Dec 2019 16:54:55 -0700
Subject: tpm_tis: reserve chip for duration of tpm_tis_core_init

From: Jerry Snitselaar <[email protected]>

commit 21df4a8b6018b842d4db181a8b24166006bad3cd upstream.

Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when issuing commands to the tpm during initialization, just reserve the chip after wait_startup, and release it when we are ready to call tpm_chip_register.

Cc Christian Bundy <[email protected]>

Cc Dan Williams <[email protected]>

Cc Peter Huewe <[email protected]>

Cc Jarkko Sakkinen <[email protected]>

Cc Jason Gunthorpe <[email protected]>

Cc Stefan Berger <[email protected]>

Cc [email protected]

Cc [email protected]

Fixes a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")

Fixes 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")

Suggested-by Jarkko Sakkinen <[email protected]>

Reviewed-by Jarkko Sakkinen <[email protected]>

Signed-off-by Jerry Snitselaar <[email protected]>

Signed-off-by Jarkko Sakkinen <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/char/tpm/tpm_tis_core.c |   35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

drivers/char/tpm/tpm_tis_core.c +18 -17

@@ -899,13 +899,13 @@ int tpm_tis_core_init(struct device *dev
 
        if (wait_startup(chip, 0) != 0) {
                rc = -ENODEV;
-               goto out_err;
+               goto err_start;
        }
 
        /* Take control of the TPM's interrupt hardware and shut it off */
        rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
        if (rc < 0)
-               goto out_err;
+               goto err_start;
 
        intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
                   TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;

@@ -914,21 +914,21 @@ int tpm_tis_core_init(struct device *dev
 
        rc = tpm_chip_start(chip);
        if (rc)
-               goto out_err;
+               goto err_start;
+
        rc = tpm2_probe(chip);
-       tpm_chip_stop(chip);
        if (rc)
-               goto out_err;
+               goto err_probe;
 
        rc = tpm_tis_read32(priv, TPM_DID_VID(0), &vendor);
        if (rc < 0)
-               goto out_err;
+               goto err_probe;
 
        priv->manufacturer_id = vendor;
 
        rc = tpm_tis_read8(priv, TPM_RID(0), &rid);
        if (rc < 0)
-               goto out_err;
+               goto err_probe;
 
        dev_info(dev, "%s TPM (device-id 0x%X, rev-id %d)\n",
                 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2",

@@ -937,13 +937,13 @@ int tpm_tis_core_init(struct device *dev
        probe = probe_itpm(chip);
        if (probe < 0) {
                rc = -ENODEV;
-               goto out_err;
+               goto err_probe;
        }
 
        /* Figure out the capabilities */
        rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
        if (rc < 0)
-               goto out_err;
+               goto err_probe;
 
        dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
                intfcaps);

@@ -977,10 +977,9 @@ int tpm_tis_core_init(struct device *dev
                if (tpm_get_timeouts(chip)) {
                        dev_err(dev, "Could not get TPM timeouts and durations\n");
                        rc = -ENODEV;
-                       goto out_err;
+                       goto err_probe;
                }
 
-               tpm_chip_start(chip);
                chip->flags |= TPM_CHIP_FLAG_IRQ;
                if (irq) {
                        tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,

@@ -991,18 +990,20 @@ int tpm_tis_core_init(struct device *dev
                } else {
                        tpm_tis_probe_irq(chip, intmask);
                }
-               tpm_chip_stop(chip);
        }
 
+       tpm_chip_stop(chip);
+
        rc = tpm_chip_register(chip);
        if (rc)
-               goto out_err;
-
-       if (chip->ops->clk_enable != NULL)
-               chip->ops->clk_enable(chip, false);
+               goto err_start;
 
        return 0;
-out_err:
+
+err_probe:
+       tpm_chip_stop(chip);
+
+err_start:
        if ((chip->ops != NULL) && (chip->ops->clk_enable != NULL))
                chip->ops->clk_enable(chip, false);
 

Patches currently in stable-queue which might be from [email protected] are

queue-5.4/iommu-fix-kasan-use-after-free-in-iommu_insert_resv_region.patch queue-5.4/iommu-vt-d-fix-dmar-pte-read-access-not-set-error.patch queue-5.4/iommu-set-group-default-domain-before-creating-direct-mappings.patch queue-5.4/tpm_tis-reserve-chip-for-duration-of-tpm_tis_core_init.patch queue-5.4/iommu-vt-d-allocate-reserved-region-for-isa-with-correct-permission.patch queue-5.4/iommu-vt-d-set-isa-bridge-reserved-region-as-relaxable.patch

Hi,

It is getting done on the PR. Hold on for testing and I'll send the PR later today.

/Jarkko

Quoted 172 lines · Dan Williams

Hi Greg,

Please drop the:

Fixes 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")

...tag. I had asked Jarkko to do that here:

https://lore.kernel.org/r/CAPcyv4h60z889bfbiwvVhsj6MxmOPiPY8ZuPB_skxkZx-N+OGw@mail.gmail.com/

...but it didn't get picked up.

Quoted 159 lines · On Mon, Dec 23, 2019 at 9:37 AM <[email protected]> wrote:

This is a note to let you know that I've just added the patch titled

tpm_tis: reserve chip for duration of tpm_tis_core_init

to the 5.4-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
tpm_tis-reserve-chip-for-duration-of-tpm_tis_core_init.patch
and it can be found in the queue-5.4 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

From 21df4a8b6018b842d4db181a8b24166006bad3cd Mon Sep 17 00:00:00 2001
From: Jerry Snitselaar <[email protected]>
Date: Wed, 11 Dec 2019 16:54:55 -0700
Subject: tpm_tis: reserve chip for duration of tpm_tis_core_init

From: Jerry Snitselaar <[email protected]>

commit 21df4a8b6018b842d4db181a8b24166006bad3cd upstream.

Instead of repeatedly calling tpm_chip_start/tpm_chip_stop when issuing commands to the tpm during initialization, just reserve the chip after wait_startup, and release it when we are ready to call tpm_chip_register.

Cc Christian Bundy <[email protected]>

Cc Dan Williams <[email protected]>

Cc Peter Huewe <[email protected]>

Cc Jarkko Sakkinen <[email protected]>

Cc Jason Gunthorpe <[email protected]>

Cc Stefan Berger <[email protected]>

Cc [email protected]

Cc [email protected]

Fixes a3fbfae82b4c ("tpm: take TPM chip power gating out of tpm_transmit()")

Fixes 5b359c7c4372 ("tpm_tis_core: Turn on the TPM before probing IRQ's")

Suggested-by Jarkko Sakkinen <[email protected]>

Reviewed-by Jarkko Sakkinen <[email protected]>

Signed-off-by Jerry Snitselaar <[email protected]>

Signed-off-by Jarkko Sakkinen <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---
 drivers/char/tpm/tpm_tis_core.c |   35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

drivers/char/tpm/tpm_tis_core.c +18 -17

@@ -899,13 +899,13 @@ int tpm_tis_core_init(struct device *dev
 
        if (wait_startup(chip, 0) != 0) {
                rc = -ENODEV;
-               goto out_err;
+               goto err_start;
        }
 
        /* Take control of the TPM's interrupt hardware and shut it off */
        rc = tpm_tis_read32(priv, TPM_INT_ENABLE(priv->locality), &intmask);
        if (rc < 0)
-               goto out_err;
+               goto err_start;
 
        intmask |= TPM_INTF_CMD_READY_INT | TPM_INTF_LOCALITY_CHANGE_INT |
                   TPM_INTF_DATA_AVAIL_INT | TPM_INTF_STS_VALID_INT;

@@ -914,21 +914,21 @@ int tpm_tis_core_init(struct device *dev
 
        rc = tpm_chip_start(chip);
        if (rc)
-               goto out_err;
+               goto err_start;
+
        rc = tpm2_probe(chip);
-       tpm_chip_stop(chip);
        if (rc)
-               goto out_err;
+               goto err_probe;
 
        rc = tpm_tis_read32(priv, TPM_DID_VID(0), &vendor);
        if (rc < 0)
-               goto out_err;
+               goto err_probe;
 
        priv->manufacturer_id = vendor;
 
        rc = tpm_tis_read8(priv, TPM_RID(0), &rid);
        if (rc < 0)
-               goto out_err;
+               goto err_probe;
 
        dev_info(dev, "%s TPM (device-id 0x%X, rev-id %d)\n",
                 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2",

@@ -937,13 +937,13 @@ int tpm_tis_core_init(struct device *dev
        probe = probe_itpm(chip);
        if (probe < 0) {
                rc = -ENODEV;
-               goto out_err;
+               goto err_probe;
        }
 
        /* Figure out the capabilities */
        rc = tpm_tis_read32(priv, TPM_INTF_CAPS(priv->locality), &intfcaps);
        if (rc < 0)
-               goto out_err;
+               goto err_probe;
 
        dev_dbg(dev, "TPM interface capabilities (0x%x):\n",
                intfcaps);

@@ -977,10 +977,9 @@ int tpm_tis_core_init(struct device *dev
                if (tpm_get_timeouts(chip)) {
                        dev_err(dev, "Could not get TPM timeouts and durations\n");
                        rc = -ENODEV;
-                       goto out_err;
+                       goto err_probe;
                }
 
-               tpm_chip_start(chip);
                chip->flags |= TPM_CHIP_FLAG_IRQ;
                if (irq) {
                        tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,

@@ -991,18 +990,20 @@ int tpm_tis_core_init(struct device *dev
                } else {
                        tpm_tis_probe_irq(chip, intmask);
                }
-               tpm_chip_stop(chip);
        }
 
+       tpm_chip_stop(chip);
+
        rc = tpm_chip_register(chip);
        if (rc)
-               goto out_err;
-
-       if (chip->ops->clk_enable != NULL)
-               chip->ops->clk_enable(chip, false);
+               goto err_start;
 
        return 0;
-out_err:
+
+err_probe:
+       tpm_chip_stop(chip);
+
+err_start:
        if ((chip->ops != NULL) && (chip->ops->clk_enable != NULL))
                chip->ops->clk_enable(chip, false);
 

Patches currently in stable-queue which might be from [email protected] are

queue-5.4/iommu-fix-kasan-use-after-free-in-iommu_insert_resv_region.patch queue-5.4/iommu-vt-d-fix-dmar-pte-read-access-not-set-error.patch queue-5.4/iommu-set-group-default-domain-before-creating-direct-mappings.patch queue-5.4/tpm_tis-reserve-chip-for-duration-of-tpm_tis_core_init.patch queue-5.4/iommu-vt-d-allocate-reserved-region-for-isa-with-correct-permission.patch queue-5.4/iommu-vt-d-set-isa-bridge-reserved-region-as-relaxable.patch

Dan, please also test the branch and tell if other patches are needed. I'm a bit blind with this as I don't have direct access to the faulting hardware. Thanks. [*]

[*] https://lkml.org/lkml/2019/12/27/12

Quoted 5 lines · Jarkko Sakkinen

Dan, please also test the branch and tell if other patches are needed. I'm a bit blind with this as I don't have direct access to the faulting hardware. Thanks. [*]

[*] https://lkml.org/lkml/2019/12/27/12

Given that:

1. I cannot reproduce the bug locally.
2. Neither of the patches have any appropriate tags (tested-by and reviewed-by). [*]

I'm sorry but how am I expected to include these patches?

Quoted 10 lines · Jarkko Sakkinen

Quoted 5 lines · Jarkko Sakkinen

Dan, please also test the branch and tell if other patches are needed. I'm a bit blind with this as I don't have direct access to the faulting hardware. Thanks. [*]

[*] https://lkml.org/lkml/2019/12/27/12

Given that:

Quoted 5 lines

1. I cannot reproduce the bug locally.
2. Neither of the patches have any appropriate tags (tested-by and reviewed-by). [*]

I'm sorry but how am I expected to include these patches?

Thanks for the branch, I'll get it tested on the failing hardware. Might be a few days due to holiday lag.

This looked like the wrong revert to me, and testing confirms that this does not fix the problem.

As I mentioned in the original report [1] the commit that bisect flagged was:

5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's

That commit moved tpm_chip_start() before irq probing. Commit 21df4a8b6018 "tpm_tis: reserve chip for duration of tpm_tis_core_init" does not appear to change anything in that regard.

Perhaps this hardware has always had broken interrupts and needs to be quirked off? I'm trying an experiment with tpm_tis_core.interrupts=0 workaround.

[1]: https://lore.kernel.org/linux-integrity/CAA9_cmeLnHK4y+usQaWo72nUG3RNsripuZnS-koY4XTRC+mwJA@mail.gmail.com/

Quoted 11 lines · Dan Williams

This looked like the wrong revert to me, and testing confirms that this does not fix the problem.

As I mentioned in the original report [1] the commit that bisect flagged was:

5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's

That commit moved tpm_chip_start() before irq probing. Commit 21df4a8b6018 "tpm_tis: reserve chip for duration of tpm_tis_core_init" does not appear to change anything in that regard.

Quoted 6 lines

Perhaps this hardware has always had broken interrupts and needs to be quirked off? I'm trying an experiment with tpm_tis_core.interrupts=0 workaround.

[1]: https://lore.kernel.org/linux-integrity/CAA9_cmeLnHK4y+usQaWo72nUG3RNsripuZnS-koY4XTRC+mwJA@mail.gmail.com/

I think for short term, yes, it is better to revert the commits that make things more broken.

for-linus-v5.5-rc5 branch contains three commits that exactly do this i.e. the reverts that Stefan sent and revert to Jerry's earlier commit.

After that is out of the table it is easier to analyze how the code should be actually refactored. Like, I have no idea when I get local HW that can reproduce this and Jerry still seems to have the same issue. It'd be nice make the exactly right changes instead of reverts but situation is what it is.

Please check the branch and ACK/NAK if I can add tested-by's (and other tags).

/Jarkko

Quoted 11 lines · Dan Williams

This looked like the wrong revert to me, and testing confirms that this does not fix the problem.

As I mentioned in the original report [1] the commit that bisect flagged was:

5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's

That commit moved tpm_chip_start() before irq probing. Commit 21df4a8b6018 "tpm_tis: reserve chip for duration of tpm_tis_core_init" does not appear to change anything in that regard.

Quoted 6 lines

Perhaps this hardware has always had broken interrupts and needs to be quirked off? I'm trying an experiment with tpm_tis_core.interrupts=0 workaround.

[1]: https://lore.kernel.org/linux-integrity/CAA9_cmeLnHK4y+usQaWo72nUG3RNsripuZnS-koY4XTRC+mwJA@mail.gmail.com/

I think for short term, yes, it is better to revert the commits that make things more broken.

for-linus-v5.5-rc5 branch contains three commits that exactly do this i.e. the reverts that Stefan sent and revert to Jerry's earlier commit.

After that is out of the table it is easier to analyze how the code should be actually refactored. Like, I have no idea when I get local HW that can reproduce this and Jerry still seems to have the same issue. It'd be nice make the exactly right changes instead of reverts but situation is what it is.

Please check the branch and ACK/NAK if I can add tested-by's (and other tags).

/Jarkko

Quoted 26 lines · Jarkko Sakkinen

Quoted 11 lines · Dan Williams

This looked like the wrong revert to me, and testing confirms that this does not fix the problem.

As I mentioned in the original report [1] the commit that bisect flagged was:

5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's

That commit moved tpm_chip_start() before irq probing. Commit 21df4a8b6018 "tpm_tis: reserve chip for duration of tpm_tis_core_init" does not appear to change anything in that regard.

Quoted 6 lines

Perhaps this hardware has always had broken interrupts and needs to be quirked off? I'm trying an experiment with tpm_tis_core.interrupts=0 workaround.

[1]: https://lore.kernel.org/linux-integrity/CAA9_cmeLnHK4y+usQaWo72nUG3RNsripuZnS-koY4XTRC+mwJA@mail.gmail.com/

I think for short term, yes, it is better to revert the commits that make things more broken.

for-linus-v5.5-rc5 branch contains three commits that exactly do this i.e. the reverts that Stefan sent and revert to Jerry's earlier commit.

Quoted 6 lines

After that is out of the table it is easier to analyze how the code should be actually refactored. Like, I have no idea when I get local HW that can reproduce this and Jerry still seems to have the same issue. It'd be nice make the exactly right changes instead of reverts but situation is what it is.

The only other thought I had was moving the tpm_chip_start/stop into tpm_tis_probe_irq_single around the tpm_tis_gen_interrupt call. I don't know why doing the clkrun bit after setting the interrupt register values would matter, but I'm not sure what else there is that would be different than when that stuff was happening in down in tpm_try_transmit. Without hardware to poke at it is hard to get anywhere.

Christian, were you having any issues with interrupts? You system was going into this code as well.

Quoted 2 lines

Christian, were you having any issues with interrupts? You system was going into this code as well.

Unfortunately I'm now unable to test, sorry for the trouble. I replaced my BIOS with UEFI firmware and the problem has disappeared. Please let me know if there is anything else I can do to help.

Christian

Quoted 7 lines · Christian Bundy

Quoted 2 lines

Christian, were you having any issues with interrupts? You system was going into this code as well.

Unfortunately I'm now unable to test, sorry for the trouble. I replaced my BIOS with UEFI firmware and the problem has disappeared. Please let me know if there is anything else I can do to help.

Quoted 1 line

Christian

Takashi wrote yesterday [*]:

"I'm building a test kernel package based on 5.5-rc4 with Jarkko's revert patches"

[*] https://bugzilla.kernel.org/show_bug.cgi?id=205935

/Jarkko

Quoted 11 lines · Jarkko Sakkinen

Quoted 8 lines · Christian Bundy

Quoted 2 lines

Christian, were you having any issues with interrupts? You system was going into this code as well.

Unfortunately I'm now unable to test, sorry for the trouble. I replaced my BIOS with UEFI firmware and the problem has disappeared. Please let me know if there is anything else I can do to help.

Christian

Quoted 4 lines

Takashi wrote yesterday [*]:

"I'm building a test kernel package based on 5.5-rc4 with Jarkko's revert patches"

Nice, I also built one of those. Just waiting for access to the system again to gather results.

Quoted 14 lines · Dan Williams

Quoted 11 lines · Jarkko Sakkinen

Quoted 8 lines · Christian Bundy

Quoted 2 lines

Christian, were you having any issues with interrupts? You system was going into this code as well.

Unfortunately I'm now unable to test, sorry for the trouble. I replaced my BIOS with UEFI firmware and the problem has disappeared. Please let me know if there is anything else I can do to help.

Christian

Quoted 7 lines

Quoted 4 lines

Takashi wrote yesterday [*]:

"I'm building a test kernel package based on 5.5-rc4 with Jarkko's revert patches"

Nice, I also built one of those. Just waiting for access to the system again to gather results.

Ok, it looks good.

Tested-by Dan Williams <[email protected]>

Tested-by Xiaoping Zhou <[email protected]>

It does report:

[    2.546660] tpm_tis IFX0740:00: 2.0 TPM (device-id 0x1B, rev-id 16)
[    2.546823] tpm tpm0: tpm_try_transmit: send(): error -5

[    2.546824] tpm tpm0: [Firmware Bug]: TPM interrupt not working,

polling instead

...at boot, but tpm2_nvlist works ok.

Quoted 17 lines · Dan Williams

Quoted 14 lines · Jarkko Sakkinen

Quoted 8 lines · Christian Bundy

Quoted 2 lines

Christian, were you having any issues with interrupts? You system was going into this code as well.

Unfortunately I'm now unable to test, sorry for the trouble. I replaced my BIOS with UEFI firmware and the problem has disappeared. Please let me know if there is anything else I can do to help.

Christian

Takashi wrote yesterday [*]:

"I'm building a test kernel package based on 5.5-rc4 with Jarkko's revert patches"

Quoted 2 lines

Nice, I also built one of those. Just waiting for access to the system again to gather results.

Ok, it looks good.

Tested-by Dan Williams <[email protected]>

Tested-by Xiaoping Zhou <[email protected]>

Quoted 10 lines · Jarkko Sakkinen

Quoted 8 lines · Christian Bundy

Quoted 2 lines

Christian, were you having any issues with interrupts? You system was going into this code as well.

Unfortunately I'm now unable to test, sorry for the trouble. I replaced my BIOS with UEFI firmware and the problem has disappeared. Please let me know if there is anything else I can do to help.

Christian

Quoted 4 lines

Takashi wrote yesterday [*]:

"I'm building a test kernel package based on 5.5-rc4 with Jarkko's revert patches"

Nice, I also built one of those. Just waiting for access to the system again to gather results.

Quoted 3 lines

[*] https://bugzilla.kernel.org/show_bug.cgi?id=205935

/Jarkko

Quoted 8 lines · Christian Bundy

Quoted 2 lines

Christian, were you having any issues with interrupts? You system was going into this code as well.

Unfortunately I'm now unable to test, sorry for the trouble. I replaced my BIOS with UEFI firmware and the problem has disappeared. Please let me know if there is anything else I can do to help.

Christian

Takashi wrote yesterday [*]:

"I'm building a test kernel package based on 5.5-rc4 with Jarkko's revert patches"

[*] https://bugzilla.kernel.org/show_bug.cgi?id=205935

/Jarkko

Quoted 15 lines · Jarkko Sakkinen

Quoted 8 lines · Christian Bundy

Quoted 2 lines

Christian, were you having any issues with interrupts? You system was going into this code as well.

Unfortunately I'm now unable to test, sorry for the trouble. I replaced my BIOS with UEFI firmware and the problem has disappeared. Please let me know if there is anything else I can do to help.

Christian

Takashi wrote yesterday [*]:

"I'm building a test kernel package based on 5.5-rc4 with Jarkko's revert patches"

Quoted 4 lines

[*] https://bugzilla.kernel.org/show_bug.cgi?id=205935

/Jarkko

You can add my ack to the reverts.

Quoted 15 lines · Jarkko Sakkinen

Quoted 8 lines · Christian Bundy

Quoted 2 lines

Christian, were you having any issues with interrupts? You system was going into this code as well.

Unfortunately I'm now unable to test, sorry for the trouble. I replaced my BIOS with UEFI firmware and the problem has disappeared. Please let me know if there is anything else I can do to help.

Christian

Takashi wrote yesterday [*]:

"I'm building a test kernel package based on 5.5-rc4 with Jarkko's revert patches"

Quoted 4 lines

[*] https://bugzilla.kernel.org/show_bug.cgi?id=205935

/Jarkko

You can add my ack to the reverts.

Quoted 1 line

You can add my ack to the reverts.

I'll actually send a patch set to linux-integrity (ETA 1-2h) and you can tag it then.

Better to have sanity check before I send the PR so please check the patches and give your feedback ASAP. Please also the check that the commit messages look good and also that the cover letter as I'll use that as basis for my pull request message.

Quoted 2 lines

Christian, were you having any issues with interrupts? You system was going into this code as well.

Unfortunately I'm now unable to test, sorry for the trouble. I replaced my BIOS with UEFI firmware and the problem has disappeared. Please let me know if there is anything else I can do to help.

Christian

Quoted 13 lines · Dan Williams

Quoted 10 lines · Jarkko Sakkinen

Quoted 5 lines · Jarkko Sakkinen

Dan, please also test the branch and tell if other patches are needed. I'm a bit blind with this as I don't have direct access to the faulting hardware. Thanks. [*]

[*] https://lkml.org/lkml/2019/12/27/12

Given that:

Quoted 8 lines

Quoted 5 lines

1. I cannot reproduce the bug locally.
2. Neither of the patches have any appropriate tags (tested-by and reviewed-by). [*]

I'm sorry but how am I expected to include these patches?

Thanks for the branch, I'll get it tested on the failing hardware. Might be a few days due to holiday lag.

This looked like the wrong revert to me, and testing confirms that this does not fix the problem.

As I mentioned in the original report [1] the commit that bisect flagged was:

5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's

That commit moved tpm_chip_start() before irq probing. Commit 21df4a8b6018 "tpm_tis: reserve chip for duration of tpm_tis_core_init" does not appear to change anything in that regard.

Perhaps this hardware has always had broken interrupts and needs to be quirked off? I'm trying an experiment with tpm_tis_core.interrupts=0 workaround.

[1]: https://lore.kernel.org/linux-integrity/CAA9_cmeLnHK4y+usQaWo72nUG3RNsripuZnS-koY4XTRC+mwJA@mail.gmail.com/

Quoted 30 lines · Dan Williams

Quoted 13 lines · Dan Williams

Quoted 10 lines · Jarkko Sakkinen

Quoted 5 lines · Jarkko Sakkinen

Dan, please also test the branch and tell if other patches are needed. I'm a bit blind with this as I don't have direct access to the faulting hardware. Thanks. [*]

[*] https://lkml.org/lkml/2019/12/27/12

Given that:

Quoted 8 lines

Quoted 5 lines

1. I cannot reproduce the bug locally.
2. Neither of the patches have any appropriate tags (tested-by and reviewed-by). [*]

I'm sorry but how am I expected to include these patches?

Thanks for the branch, I'll get it tested on the failing hardware. Might be a few days due to holiday lag.

This looked like the wrong revert to me, and testing confirms that this does not fix the problem.

As I mentioned in the original report [1] the commit that bisect flagged was:

5b359c7c4372 tpm_tis_core: Turn on the TPM before probing IRQ's

Quoted 8 lines

That commit moved tpm_chip_start() before irq probing. Commit 21df4a8b6018 "tpm_tis: reserve chip for duration of tpm_tis_core_init" does not appear to change anything in that regard.

Perhaps this hardware has always had broken interrupts and needs to be quirked off? I'm trying an experiment with tpm_tis_core.interrupts=0 workaround.

Hi Dan,

Just to make sure I understand correctly are you saying you still have the screaming interrupt with the flag commit reverted,

or that it is
polling instead of using interrupts [2]? Was that testing with both commits reverted, or just the flag commit?

What kernel were you
running before you saw the issue with 5.3 stable?

On that kernel you
weren't seeing the polling message, and interrupts were working?

Are
you able to boot a 5.0 kernel on the system? It would be interesting to see how it was behaving before the power gating changes. I think it would be using polling due to how the code behaves because of that flag. It looks like without the flag being enabled by Stefan's commit TPM_GLOBAL_INT_ENABLE will never get cleared because tpm_tis_probe_irq_single expects tpm_tis_send to clear it if there is a problem, and without the flag being set that whole section of code is skipped.

Quoted 5 lines · Jarkko Sakkinen

Dan, please also test the branch and tell if other patches are needed. I'm a bit blind with this as I don't have direct access to the faulting hardware. Thanks. [*]

[*] https://lkml.org/lkml/2019/12/27/12

Given that:

1. I cannot reproduce the bug locally.
2. Neither of the patches have any appropriate tags (tested-by and reviewed-by). [*]

I'm sorry but how am I expected to include these patches?

[*] https://patchwork.kernel.org/project/linux-integrity/list/?series=208305

/Jarkko