This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
https://bugs.archlinux.org/user/38129 https://bugzilla.kernel.org/show_bug.cgi?id=217631
Fixes: e644b2f498d297a928efcb7ff6f900c27f8b788e Cc: stable@vger.kernel.org Reported-by: roubro1991@gmail.com Signed-off-by: Christian Hesse mail@eworm.de --- drivers/char/tpm/tpm_tis.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 7db3593941ea..2979f8b9aaa0 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -114,6 +114,14 @@ static int tpm_tis_disable_irq(const struct dmi_system_id *d) }
static const struct dmi_system_id tpm_tis_dmi_table[] = { + { + .callback = tpm_tis_disable_irq, + .ident = "Framework Laptop Intel 12th gen", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Framework"), + DMI_MATCH(DMI_PRODUCT_VERSION, "A4"), + }, + }, { .callback = tpm_tis_disable_irq, .ident = "ThinkPad T490s",
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
https://bugs.archlinux.org/user/38129 https://bugzilla.kernel.org/show_bug.cgi?id=217631
Fixes: e644b2f498d297a928efcb7ff6f900c27f8b788e Cc: stable@vger.kernel.org Reported-by: roubro1991@gmail.com Signed-off-by: Christian Hesse mail@eworm.de --- drivers/char/tpm/tpm_tis.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 2979f8b9aaa0..a8bcd2134e6b 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -122,6 +122,14 @@ static const struct dmi_system_id tpm_tis_dmi_table[] = { DMI_MATCH(DMI_PRODUCT_VERSION, "A4"), }, }, + { + .callback = tpm_tis_disable_irq, + .ident = "Framework Laptop Intel 13th gen", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Framework"), + DMI_MATCH(DMI_PRODUCT_VERSION, "A6"), + }, + }, { .callback = tpm_tis_disable_irq, .ident = "ThinkPad T490s",
Hi, Thorsten here, the Linux kernel's regression tracker. I'm not a proper reviewer for patches like this, but nevertheless two small things:
On 10.07.23 15:38, Christian Hesse wrote:
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
Thx for working on this!
Not sure why this link might be helpful. Did you maybe mean to include another one? Like this one?
https://community.frame.work/t/boot-and-shutdown-hangs-with-arch-linux-kerne...
https://bugzilla.kernel.org/show_bug.cgi?id=217631
Fixes: e644b2f498d297a928efcb7ff6f900c27f8b788e
This should be:
Fixes: e644b2f498d ("tpm, tpm_tis: Enable interrupt test")
Cc: stable@vger.kernel.org Reported-by: roubro1991@gmail.com
The bugzilla link from above above should be here like this instead:
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217631
BTW, checkpatch.pl should have mentioned the two latter things.
HTH, Ciao, Thorsten
Signed-off-by: Christian Hesse mail@eworm.de
drivers/char/tpm/tpm_tis.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 7db3593941ea..2979f8b9aaa0 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -114,6 +114,14 @@ static int tpm_tis_disable_irq(const struct dmi_system_id *d) } static const struct dmi_system_id tpm_tis_dmi_table[] = {
- {
.callback = tpm_tis_disable_irq,.ident = "Framework Laptop Intel 12th gen",.matches = {DMI_MATCH(DMI_SYS_VENDOR, "Framework"),DMI_MATCH(DMI_PRODUCT_VERSION, "A4"),},- }, { .callback = tpm_tis_disable_irq, .ident = "ThinkPad T490s",
On Mon, Jul 10, 2023 at 03:38:35PM +0200, Christian Hesse wrote:
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
https://bugs.archlinux.org/user/38129 https://bugzilla.kernel.org/show_bug.cgi?id=217631
Add "Link:" tags?
Fixes: e644b2f498d297a928efcb7ff6f900c27f8b788e
As per the kernel documentation, this should be:
Fixes: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
otherwise you're going to get some emails from the linux-next bot.
thanks,
greg k-h
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
Link: https://community.frame.work/t/boot-and-shutdown-hangs-with-arch-linux-kerne... Fixes: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test") Cc: stable@vger.kernel.org Reported-by: roubro1991@gmail.com Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217631 Signed-off-by: Christian Hesse mail@eworm.de --- drivers/char/tpm/tpm_tis.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 7db3593941ea..2979f8b9aaa0 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -114,6 +114,14 @@ static int tpm_tis_disable_irq(const struct dmi_system_id *d) }
static const struct dmi_system_id tpm_tis_dmi_table[] = { + { + .callback = tpm_tis_disable_irq, + .ident = "Framework Laptop Intel 12th gen", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Framework"), + DMI_MATCH(DMI_PRODUCT_VERSION, "A4"), + }, + }, { .callback = tpm_tis_disable_irq, .ident = "ThinkPad T490s",
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
Link: https://community.frame.work/t/boot-and-shutdown-hangs-with-arch-linux-kerne... Fixes: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test") Cc: stable@vger.kernel.org Reported-by: roubro1991@gmail.com Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217631 Signed-off-by: Christian Hesse mail@eworm.de --- drivers/char/tpm/tpm_tis.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 2979f8b9aaa0..a8bcd2134e6b 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -122,6 +122,14 @@ static const struct dmi_system_id tpm_tis_dmi_table[] = { DMI_MATCH(DMI_PRODUCT_VERSION, "A4"), }, }, + { + .callback = tpm_tis_disable_irq, + .ident = "Framework Laptop Intel 13th gen", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Framework"), + DMI_MATCH(DMI_PRODUCT_VERSION, "A6"), + }, + }, { .callback = tpm_tis_disable_irq, .ident = "ThinkPad T490s",
On 10.07.23 16:28, Christian Hesse wrote:
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
Link: https://community.frame.work/t/boot-and-shutdown-hangs-with-arch-linux-kerne... Fixes: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test") Cc: stable@vger.kernel.org Reported-by: roubro1991@gmail.com Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217631
Thx again for working on this. FWIW, a quote from a recent comment there FYI:
``` mapleleaf 2023-07-10 16:37:03 UTC
I have the same problem and I own a Framework 12th-gen, but for whatever reason my DMI_PRODUCT_VERSION is A8 instead of A6...
$ sudo dmidecode -s baseboard-version A8
[tag] [reply] [−] Private Comment 13 mapleleaf 2023-07-10 16:41:29 UTC
And also:
$ sudo dmidecode -s system-version A8
```
drivers/char/tpm/tpm_tis.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 7db3593941ea..2979f8b9aaa0 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -114,6 +114,14 @@ static int tpm_tis_disable_irq(const struct dmi_system_id *d) } static const struct dmi_system_id tpm_tis_dmi_table[] = {
- {
.callback = tpm_tis_disable_irq,.ident = "Framework Laptop Intel 12th gen",.matches = {DMI_MATCH(DMI_SYS_VENDOR, "Framework"),DMI_MATCH(DMI_PRODUCT_VERSION, "A4"),},- }, { .callback = tpm_tis_disable_irq, .ident = "ThinkPad T490s",
On Mon, Jul 10, 2023 at 04:28:43PM +0200, Christian Hesse wrote:
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
I can't help but feel like we are doing something wrong in the Linux driver that we keep having IRQ problems.
Surely Windows uses the IRQ on these devices? How does it work reliably there?
Jason
On Mon, 2023-07-10 at 15:01 -0300, Jason Gunthorpe wrote:
On Mon, Jul 10, 2023 at 04:28:43PM +0200, Christian Hesse wrote:
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
I can't help but feel like we are doing something wrong in the Linux driver that we keep having IRQ problems.
Surely Windows uses the IRQ on these devices? How does it work reliably there?
Jason
I'm about to send a PR to Linus with a pile of IRQ fixes for v6.4 feature.
BR, Jarkko
On Mon, 2023-07-10 at 15:01 -0300, Jason Gunthorpe wrote:
On Mon, Jul 10, 2023 at 04:28:43PM +0200, Christian Hesse wrote:
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
I can't help but feel like we are doing something wrong in the Linux driver that we keep having IRQ problems.
Surely Windows uses the IRQ on these devices? How does it work reliably there?
I've had some friends in the MS Linux team ask around to find out if anyone in the windows group is willing to divulge a little on this. My suspicion is that if interrupts are enabled at all on Windows TIS, it's via a whitelist.
James
On Mon, Jul 10, 2023 at 03:01:33PM -0300, Jason Gunthorpe wrote:
On Mon, Jul 10, 2023 at 04:28:43PM +0200, Christian Hesse wrote:
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
I can't help but feel like we are doing something wrong in the Linux driver that we keep having IRQ problems.
Surely Windows uses the IRQ on these devices? How does it work reliably there?
Jason
I worry about it as well, especially as more vendors get added to the list. On the other hand it seems like every 6-12 months I am interacting with vendors that repeatedly forget to have reserved memory regions added to IVRS and DMAR tables for devices that need them. So I guess it is possible for the problem to be on their end as well. For at least one case that someone looked at back in May, it looked like he could see the issue looking at a schematic for the system.
Regards, Jerry
Christian Hesse mail@eworm.de on Mon, 2023/07/10 16:28:
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
Uh, looks like this is not quite right. The product version specifies the CPU "level" across generations (for example "i5-1240P" vs. "i7-1260P" vs. "i7-1280P"). So I guess we should match on the product name instead...
I will send an updated set of patches. Would be nice if anybody could verify this...
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
Link: https://community.frame.work/t/boot-and-shutdown-hangs-with-arch-linux-kerne... Fixes: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test") Cc: stable@vger.kernel.org Reported-by: roubro1991@gmail.com Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217631 Signed-off-by: Christian Hesse mail@eworm.de --- drivers/char/tpm/tpm_tis.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 7db3593941ea..52bb8b642207 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -114,6 +114,14 @@ static int tpm_tis_disable_irq(const struct dmi_system_id *d) }
static const struct dmi_system_id tpm_tis_dmi_table[] = { + { + .callback = tpm_tis_disable_irq, + .ident = "Framework Laptop (12th Gen Intel Core)", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Framework"), + DMI_MATCH(DMI_PRODUCT_NAME, "Laptop (12th Gen Intel Core)"), + }, + }, { .callback = tpm_tis_disable_irq, .ident = "ThinkPad T490s",
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
Link: https://community.frame.work/t/boot-and-shutdown-hangs-with-arch-linux-kerne... Fixes: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test") Cc: stable@vger.kernel.org Reported-by: roubro1991@gmail.com Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217631 Signed-off-by: Christian Hesse mail@eworm.de --- drivers/char/tpm/tpm_tis.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 52bb8b642207..f8a8587c66f3 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -122,6 +122,14 @@ static const struct dmi_system_id tpm_tis_dmi_table[] = { DMI_MATCH(DMI_PRODUCT_NAME, "Laptop (12th Gen Intel Core)"), }, }, + { + .callback = tpm_tis_disable_irq, + .ident = "Framework Laptop (13th Gen Intel Core)", + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "Framework"), + DMI_MATCH(DMI_PRODUCT_NAME, "Laptop (13th Gen Intel Core)"), + }, + }, { .callback = tpm_tis_disable_irq, .ident = "ThinkPad T490s",
On Mon, 2023-07-10 at 23:13 +0200, Christian Hesse wrote:
Christian Hesse mail@eworm.de on Mon, 2023/07/10 16:28:
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
Uh, looks like this is not quite right. The product version specifies the CPU "level" across generations (for example "i5-1240P" vs. "i7-1260P" vs. "i7-1280P"). So I guess we should match on the product name instead...
I will send an updated set of patches. Would be nice if anybody could verify this...
OK, this good to hear! I've been late with my pull request (past rc1) because of kind of conflicting timing with Finnish holiday season and relocating my home office.
I'll replace v2 patches with v3 and send the PR for rc2 after that. So unluck turned into luck this time :-)
Thank you for spotting this!
BR, Jarkko
On Tue, 2023-07-11 at 00:29 +0300, Jarkko Sakkinen wrote:
On Mon, 2023-07-10 at 23:13 +0200, Christian Hesse wrote:
Christian Hesse mail@eworm.de on Mon, 2023/07/10 16:28:
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
Uh, looks like this is not quite right. The product version specifies the CPU "level" across generations (for example "i5-1240P" vs. "i7-1260P" vs. "i7-1280P"). So I guess we should match on the product name instead...
I will send an updated set of patches. Would be nice if anybody could verify this...
OK, this good to hear! I've been late with my pull request (past rc1) because of kind of conflicting timing with Finnish holiday season and relocating my home office.
I'll replace v2 patches with v3 and send the PR for rc2 after that. So unluck turned into luck this time :-)
Thank you for spotting this!
Please, sanity check before I send the PR for rc2:
https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/
BR, Jarkko
Jarkko Sakkinen jarkko@kernel.org on Tue, 2023/07/11 00:51:
On Tue, 2023-07-11 at 00:29 +0300, Jarkko Sakkinen wrote:
OK, this good to hear! I've been late with my pull request (past rc1) because of kind of conflicting timing with Finnish holiday season and relocating my home office.
I'll replace v2 patches with v3 and send the PR for rc2 after that. So unluck turned into luck this time :-)
Thank you for spotting this!
Please, sanity check before I send the PR for rc2:
https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/
Looks good and works as expected on Framework Laptop 12th Gen, verified by me and someone in the linked bugzilla ticket. I do not have Framework Laptop 13th Gen available for testing.
Looks like there are general workarounds by disabling interrupts after a number of unhandled IRQs. Will this still go in?
On Wed Jul 12, 2023 at 6:48 AM UTC, Christian Hesse wrote:
Jarkko Sakkinen jarkko@kernel.org on Tue, 2023/07/11 00:51:
On Tue, 2023-07-11 at 00:29 +0300, Jarkko Sakkinen wrote:
OK, this good to hear! I've been late with my pull request (past rc1) because of kind of conflicting timing with Finnish holiday season and relocating my home office.
I'll replace v2 patches with v3 and send the PR for rc2 after that. So unluck turned into luck this time :-)
Thank you for spotting this!
Please, sanity check before I send the PR for rc2:
https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/
Looks good and works as expected on Framework Laptop 12th Gen, verified by me and someone in the linked bugzilla ticket. I do not have Framework Laptop 13th Gen available for testing.
Looks like there are general workarounds by disabling interrupts after a number of unhandled IRQs. Will this still go in? -- main(a){char*c=/* Schoene Gruesse */"B?IJj;MEH" "CX:;",b;for(a/* Best regards my address: */=0;b=c[a++];) putchar(b-1/(/* Chris cc -ox -xc - && ./x */b/42*2-3)*42);}
So I think it should, what is deny listed now will stay like that even if Lino's patch is included.
BR, Jarkko
On Tue, 2023-07-11 at 00:29 +0300, Jarkko Sakkinen wrote:
On Mon, 2023-07-10 at 23:13 +0200, Christian Hesse wrote:
OK, this good to hear! I've been late with my pull request (past rc1) because of kind of conflicting timing with Finnish holiday season and relocating my home office.
I'll replace v2 patches with v3 and send the PR for rc2 after that. So unluck turned into luck this time :-)
Thank you for spotting this!
I want to say: this issue is NOT limited to Framework laptops.
For example this MSI gen12 i5-1240P laptop also suffers from same problem: Manufacturer: Micro-Star International Co., Ltd. Product Name: Summit E13FlipEvo A12MT Version: REV:1.0 SKU Number: 13P3.1 Family: Summit
So, probably just blacklisting affected models is not the best solution...
On 11.07.23 14:41, Grundik wrote:
On Tue, 2023-07-11 at 00:29 +0300, Jarkko Sakkinen wrote:
On Mon, 2023-07-10 at 23:13 +0200, Christian Hesse wrote:
OK, this good to hear! I've been late with my pull request (past rc1) because of kind of conflicting timing with Finnish holiday season and relocating my home office.
I'll replace v2 patches with v3 and send the PR for rc2 after that. So unluck turned into luck this time :-)
Thank you for spotting this!
I want to say: this issue is NOT limited to Framework laptops.
For example this MSI gen12 i5-1240P laptop also suffers from same problem: Manufacturer: Micro-Star International Co., Ltd. Product Name: Summit E13FlipEvo A12MT Version: REV:1.0 SKU Number: 13P3.1 Family: Summit
So, probably just blacklisting affected models is not the best solution...
Hmmm. Jarkko, you earlier in the thread said: "I'm about to send a PR to Linus with a pile of IRQ fixes for v6.4 feature.". Could any of those potentially help Grundik or those with the Framework laptop? Is that worth testing?
Ciao, Thorsten
On Tue, 2023-07-11 at 15:41 +0300, Grundik wrote:
On Tue, 2023-07-11 at 00:29 +0300, Jarkko Sakkinen wrote:
On Mon, 2023-07-10 at 23:13 +0200, Christian Hesse wrote:
OK, this good to hear! I've been late with my pull request (past rc1) because of kind of conflicting timing with Finnish holiday season and relocating my home office.
I'll replace v2 patches with v3 and send the PR for rc2 after that. So unluck turned into luck this time :-)
Thank you for spotting this!
I want to say: this issue is NOT limited to Framework laptops.
For example this MSI gen12 i5-1240P laptop also suffers from same problem: Manufacturer: Micro-Star International Co., Ltd. Product Name: Summit E13FlipEvo A12MT Version: REV:1.0 SKU Number: 13P3.1 Family: Summit
So, probably just blacklisting affected models is not the best solution...
It will be supplemented with
https://lore.kernel.org/linux-integrity/CTYXI8TL7C36.2SCWH82FAZWBO@suppilova...
Together they should fairly sustainable framework.
Lino, can you add the same fixes tag as for this. It would probably ignore inline comments to keep the patch minimal since it is a critical fix. Just do the renames, remove inline comments and send v3.
For tpm_tis_check_for_interrupt_storm(), you can could rename it simply as tpm_tis_update_unhandle_irqs() as that it what it does (my review did not include a suggestion for this).
This way I think it should be fairly trivial to get a version that can be landed.
To put short: 1. Do the renames as suggested, they are good enough for me. 2. Drop inline comments, their usefulness is somewhat questionable and they increase the diff. 3. Generally aim for minimal diff but I think this should be good enough if you do steps 1 and 2.
If you don't have the time at hand, I can carefully do these cleanups and apply the patch. If you have the time and motivation, go ahead and send v3.
BR, Jarkko
Hi,
On 11.07.23 23:50, Jarkko Sakkinen wrote:
On Tue, 2023-07-11 at 15:41 +0300, Grundik wrote:
On Tue, 2023-07-11 at 00:29 +0300, Jarkko Sakkinen wrote:
On Mon, 2023-07-10 at 23:13 +0200, Christian Hesse wrote:
OK, this good to hear! I've been late with my pull request (past rc1) because of kind of conflicting timing with Finnish holiday season and relocating my home office.
I'll replace v2 patches with v3 and send the PR for rc2 after that. So unluck turned into luck this time :-)
Thank you for spotting this!
I want to say: this issue is NOT limited to Framework laptops.
For example this MSI gen12 i5-1240P laptop also suffers from same problem: Manufacturer: Micro-Star International Co., Ltd. Product Name: Summit E13FlipEvo A12MT Version: REV:1.0 SKU Number: 13P3.1 Family: Summit
So, probably just blacklisting affected models is not the best solution...
It will be supplemented with
https://lore.kernel.org/linux-integrity/CTYXI8TL7C36.2SCWH82FAZWBO@suppilova...
Together they should fairly sustainable framework.
Lino, can you add the same fixes tag as for this. It would probably ignore inline comments to keep the patch minimal since it is a critical fix. Just do the renames, remove inline comments and send v3.
For tpm_tis_check_for_interrupt_storm(), you can could rename it simply as tpm_tis_update_unhandle_irqs() as that it what it does (my review did not include a suggestion for this).
This way I think it should be fairly trivial to get a version that can be landed.
To put short:
- Do the renames as suggested, they are good enough for me.
- Drop inline comments, their usefulness is somewhat questionable and they increase the diff.
- Generally aim for minimal diff but I think this should be good enough if you do steps 1 and 2.
If you don't have the time at hand, I can carefully do these cleanups and apply the patch. If you have the time and motivation, go ahead and send v3.
BR, Jarkko
No problem, I can do this. But note that the next version will be v4 not v3 (as v3 is the version you recently reviewed).
Regards, Lino
On Wed, 2023-07-12 at 00:50 +0300, Jarkko Sakkinen wrote:
I want to say: this issue is NOT limited to Framework laptops.
For example this MSI gen12 i5-1240P laptop also suffers from same problem: Manufacturer: Micro-Star International Co., Ltd. Product Name: Summit E13FlipEvo A12MT Version: REV:1.0 SKU Number: 13P3.1 Family: Summit
So, probably just blacklisting affected models is not the best solution...
It will be supplemented with
https://lore.kernel.org/linux-integrity/CTYXI8TL7C36.2SCWH82FAZWBO@suppilova...
Together they should fairly sustainable framework.
Unfortunately, they dont. Problem still occurs in debian 6.5-rc4 kernel, with forementioned laptop. According to sources, these patches are applied in that kernel version.
What can I do to assist fixing it?..
On 06.08.23 18:30, Grundik wrote:
On Wed, 2023-07-12 at 00:50 +0300, Jarkko Sakkinen wrote:
I want to say: this issue is NOT limited to Framework laptops.
For example this MSI gen12 i5-1240P laptop also suffers from same problem: Manufacturer: Micro-Star International Co., Ltd. Product Name: Summit E13FlipEvo A12MT
[...]
It will be supplemented with https://lore.kernel.org/linux-integrity/CTYXI8TL7C36.2SCWH82FAZWBO@suppilova...
Together they should fairly sustainable framework.
Unfortunately, they dont. Problem still occurs in debian 6.5-rc4 kernel, with forementioned laptop. According to sources, these patches are applied in that kernel version.
Jarkko & Lino, did you see this msg Grundik posted that about a week ago? It looks like there is still something wrong there that need attention. Or am I missing something?
FWIW, two more users reported that they still see similar problems with recent 6.4.y kernels that contain the "tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs" patch. Both also with MSI laptops:
https://bugzilla.kernel.org/show_bug.cgi?id=217631#c18 https://bugzilla.kernel.org/show_bug.cgi?id=217631#c20
No reply either afaics.
Ciao, Thorsten
On Fri, 2023-08-11 at 10:18 +0200, Thorsten Leemhuis wrote:
Jarkko & Lino, did you see this msg Grundik posted that about a week ago? It looks like there is still something wrong there that need attention. Or am I missing something?
FWIW, two more users reported that they still see similar problems with recent 6.4.y kernels that contain the "tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs" patch. Both also with MSI laptops:
https://bugzilla.kernel.org/show_bug.cgi?id=217631#c18 https://bugzilla.kernel.org/show_bug.cgi?id=217631#c20
No reply either afaics.
As I said before: it does not looks like blacklisting is a good solution at all.
If there are general fix, then blacklisting only makes testing of that fix more difficult. If general fix works, why blacklist? If it does not work and its impossible to figure out why — maybe there should be kernel boot option to select between polling/irq instead of/in addition to hard-coded blacklist.
Unfortunately, its very hard to test this fixes on my side: since TPM is not a module, but compiled into kernel itself, it requires recompiling a whole kernel, which is quite a task for a laptop. But I will try my best, if needed.
On Fri Aug 11, 2023 at 1:44 PM EEST, Grundik wrote:
On Fri, 2023-08-11 at 10:18 +0200, Thorsten Leemhuis wrote:
Jarkko & Lino, did you see this msg Grundik posted that about a week ago? It looks like there is still something wrong there that need attention. Or am I missing something?
FWIW, two more users reported that they still see similar problems with recent 6.4.y kernels that contain the "tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs" patch. Both also with MSI laptops:
https://bugzilla.kernel.org/show_bug.cgi?id=217631#c18 https://bugzilla.kernel.org/show_bug.cgi?id=217631#c20
No reply either afaics.
As I said before: it does not looks like blacklisting is a good solution at all.
If there are general fix, then blacklisting only makes testing of that fix more difficult. If general fix works, why blacklist? If it does not work and its impossible to figure out why — maybe there should be kernel boot option to select between polling/irq instead of/in addition to hard-coded blacklist.
Unfortunately, its very hard to test this fixes on my side: since TPM is not a module, but compiled into kernel itself, it requires recompiling a whole kernel, which is quite a task for a laptop. But I will try my best, if needed.
I agree with this.
Can you check my response to Thorsten and share your opinions?
BR, Jarkko
On Fri Aug 11, 2023 at 11:18 AM EEST, Thorsten Leemhuis wrote:
On 06.08.23 18:30, Grundik wrote:
On Wed, 2023-07-12 at 00:50 +0300, Jarkko Sakkinen wrote:
I want to say: this issue is NOT limited to Framework laptops.
For example this MSI gen12 i5-1240P laptop also suffers from same problem: Manufacturer: Micro-Star International Co., Ltd. Product Name: Summit E13FlipEvo A12MT
[...]
It will be supplemented with https://lore.kernel.org/linux-integrity/CTYXI8TL7C36.2SCWH82FAZWBO@suppilova...
Together they should fairly sustainable framework.
Unfortunately, they dont. Problem still occurs in debian 6.5-rc4 kernel, with forementioned laptop. According to sources, these patches are applied in that kernel version.
Jarkko & Lino, did you see this msg Grundik posted that about a week ago? It looks like there is still something wrong there that need attention. Or am I missing something?
FWIW, two more users reported that they still see similar problems with recent 6.4.y kernels that contain the "tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs" patch. Both also with MSI laptops:
https://bugzilla.kernel.org/show_bug.cgi?id=217631#c18 https://bugzilla.kernel.org/show_bug.cgi?id=217631#c20
No reply either afaics.
Ciao, Thorsten
I was planning to send a PR to Linus with a quirk for MSI GS66 Stealth 11UG, and apparently this bug report would add two additional MSI entries. This is becoming quickly a maintenance hell.
For Lenovo, I also added patch that will categorically disable irqs for all with vendor "LENOVO" because there was already six entries, and I got patch for 7th.
Now we at least know that IRQs TPMs are somewhat broken so it is time to make decisions to make this converge to something.
I see two long-standing options:
A. Move from deny list to allow list when considering using IRQs. This can be supplemented with a kernel command-line parameter to enforce IRQs and ignore the allow list (and IRQ storm detection provides additional measure in case you try to enforce) B. Change deny list to match only vendors for the time being. This can be supplemented with a allow list that is processed after the deny list for models where IRQs are known to work.
This would the implementation of (B):
static const struct dmi_system_id tpm_tis_dmi_table[] = { { .callback = tpm_tis_disable_irq, .matches = { DMI_MATCH(DMI_SYS_VENDOR, "Framework"), }, }, { .callback = tpm_tis_disable_irq, .ident = "MSI GS66 Stealth 11UG", .matches = { DMI_MATCH(DMI_SYS_VENDOR, "Micro-Star International Co., Ltd."), }, }, { .callback = tpm_tis_disable_irq, .matches = { DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), }, }, { .callback = tpm_tis_disable_irq, .matches = { DMI_MATCH(DMI_SYS_VENDOR, "TUXEDO"), }, }, { .callback = tpm_tis_disable_irq, .matches = { DMI_MATCH(DMI_SYS_VENDOR, "AAEON"), }, }, {} };
BR, Jarkko
On Fri Aug 11, 2023 at 8:22 PM EEST, Jarkko Sakkinen wrote:
On Fri Aug 11, 2023 at 11:18 AM EEST, Thorsten Leemhuis wrote:
On 06.08.23 18:30, Grundik wrote:
On Wed, 2023-07-12 at 00:50 +0300, Jarkko Sakkinen wrote:
I want to say: this issue is NOT limited to Framework laptops.
For example this MSI gen12 i5-1240P laptop also suffers from same problem: Manufacturer: Micro-Star International Co., Ltd. Product Name: Summit E13FlipEvo A12MT
[...]
It will be supplemented with https://lore.kernel.org/linux-integrity/CTYXI8TL7C36.2SCWH82FAZWBO@suppilova...
Together they should fairly sustainable framework.
Unfortunately, they dont. Problem still occurs in debian 6.5-rc4 kernel, with forementioned laptop. According to sources, these patches are applied in that kernel version.
Jarkko & Lino, did you see this msg Grundik posted that about a week ago? It looks like there is still something wrong there that need attention. Or am I missing something?
FWIW, two more users reported that they still see similar problems with recent 6.4.y kernels that contain the "tpm,tpm_tis: Disable interrupts after 1000 unhandled IRQs" patch. Both also with MSI laptops:
https://bugzilla.kernel.org/show_bug.cgi?id=217631#c18 https://bugzilla.kernel.org/show_bug.cgi?id=217631#c20
No reply either afaics.
Ciao, Thorsten
I was planning to send a PR to Linus with a quirk for MSI GS66 Stealth 11UG, and apparently this bug report would add two additional MSI entries. This is becoming quickly a maintenance hell.
For Lenovo, I also added patch that will categorically disable irqs for all with vendor "LENOVO" because there was already six entries, and I got patch for 7th.
Now we at least know that IRQs TPMs are somewhat broken so it is time to make decisions to make this converge to something.
I see two long-standing options:
A. Move from deny list to allow list when considering using IRQs. This can be supplemented with a kernel command-line parameter to enforce IRQs and ignore the allow list (and IRQ storm detection provides additional measure in case you try to enforce) B. Change deny list to match only vendors for the time being. This can be supplemented with a allow list that is processed after the deny list for models where IRQs are known to work.
This would the implementation of (B):
static const struct dmi_system_id tpm_tis_dmi_table[] = { { .callback = tpm_tis_disable_irq, .matches = { DMI_MATCH(DMI_SYS_VENDOR, "Framework"), }, }, { .callback = tpm_tis_disable_irq, .ident = "MSI GS66 Stealth 11UG", .matches = { DMI_MATCH(DMI_SYS_VENDOR, "Micro-Star International Co., Ltd."), }, }, { .callback = tpm_tis_disable_irq, .matches = { DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"), }, }, { .callback = tpm_tis_disable_irq, .matches = { DMI_MATCH(DMI_SYS_VENDOR, "TUXEDO"), }, }, { .callback = tpm_tis_disable_irq, .matches = { DMI_MATCH(DMI_SYS_VENDOR, "AAEON"), }, }, {} };
BR, Jarkko
I.e. this is a non-sustainable solution:
if (vendor && product) { dev_info(&chip->dev, "Consider adding the following entry to tpm_tis_dmi_table:\n"); dev_info(&chip->dev, "\tDMI_SYS_VENDOR: %s\n", vendor); dev_info(&chip->dev, "\tDMI_PRODUCT_VERSION: %s\n", product); }
This is also super time consuming and takes the focus away from more important matters (like most likely the AMD rng fix would have gone smoother without these getting in the way all the time).
BR, Jarkko
On Fri, 2023-08-11 at 20:40 +0300, Jarkko Sakkinen wrote:
On Fri Aug 11, 2023 at 8:22 PM EEST, Jarkko Sakkinen wrote:
On Fri Aug 11, 2023 at 11:18 AM EEST, Thorsten Leemhuis wrote:
I see two long-standing options:
A. Move from deny list to allow list when considering using IRQs. This can be supplemented with a kernel command-line parameter to enforce IRQs and ignore the allow list (and IRQ storm detection provides additional measure in case you try to enforce) B. Change deny list to match only vendors for the time being. This can be supplemented with a allow list that is processed after the deny list for models where IRQs are known to work.
[...]
This is also super time consuming and takes the focus away from more important matters (like most likely the AMD rng fix would have gone smoother without these getting in the way all the time).
Main problem of any list is maintaining of them. So, I think there should not be any black or white lists at all. Module should work with reasonable default (polling is the one, which lived without problems for years and years due to bug, as I understand), and probably a boot option to force IRQ. Maybe module should warn user to try that option.
I don't know: is it even worth it to use IRQ, if it so problematic? Are there any significant advantages of that? I understand, polling is a resource consumer, but its just TPM, which is used mainly at the boot time, is it worth it?
On Fri Aug 11, 2023 at 9:47 PM EEST, Grundik wrote:
On Fri, 2023-08-11 at 20:40 +0300, Jarkko Sakkinen wrote:
On Fri Aug 11, 2023 at 8:22 PM EEST, Jarkko Sakkinen wrote:
On Fri Aug 11, 2023 at 11:18 AM EEST, Thorsten Leemhuis wrote:
I see two long-standing options:
A. Move from deny list to allow list when considering using IRQs. This can be supplemented with a kernel command-line parameter to enforce IRQs and ignore the allow list (and IRQ storm detection provides additional measure in case you try to enforce) B. Change deny list to match only vendors for the time being. This can be supplemented with a allow list that is processed after the deny list for models where IRQs are known to work.
[...]
This is also super time consuming and takes the focus away from more important matters (like most likely the AMD rng fix would have gone smoother without these getting in the way all the time).
Main problem of any list is maintaining of them. So, I think there should not be any black or white lists at all. Module should work with reasonable default (polling is the one, which lived without problems for years and years due to bug, as I understand), and probably a boot option to force IRQ. Maybe module should warn user to try that option.
I don't know: is it even worth it to use IRQ, if it so problematic? Are there any significant advantages of that? I understand, polling is a resource consumer, but its just TPM, which is used mainly at the boot time, is it worth it?
+1
Thanks for sharing your opinion. I'll take the necessary steps.
BR, Jarkko
On Fri, 2023-08-11 at 23:01 +0300, Jarkko Sakkinen wrote:
Thanks for sharing your opinion. I'll take the necessary steps.
I was thinking... Maybe I'm wrong, maybe I mistaking, but isnt this TPM located inside of the CPU chip? So that issue is not specific to laptop vendor or model, but its CPU-specific.
My MSI A12MT laptop has i5-1240P, Framework laptops mentioned in this thread, also has i5-1240P CPU. Unfortunately there are no such information about other affected models, but could it be just that CPU line?
On Sat Aug 12, 2023 at 2:28 PM EEST, Grundik wrote:
On Fri, 2023-08-11 at 23:01 +0300, Jarkko Sakkinen wrote:
Thanks for sharing your opinion. I'll take the necessary steps.
I was thinking... Maybe I'm wrong, maybe I mistaking, but isnt this TPM located inside of the CPU chip? So that issue is not specific to laptop vendor or model, but its CPU-specific.
Nope, tpm_tis is MMIO interface for so called FIFO register interface, it could be backed e.g. by SPI [*]. It is a physical layer for MMIO. There's other backends too such as tpm_tis_i2c where kernel is directly exposed to the physical layer.
My MSI A12MT laptop has i5-1240P, Framework laptops mentioned in this thread, also has i5-1240P CPU. Unfortunately there are no such information about other affected models, but could it be just that CPU line?
[*] https://lkml.org/lkml/2023/8/14/1065
BR, Jarkko
On Fri, 11 Aug 2023 at 10:22, Jarkko Sakkinen jarkko@kernel.org wrote:
I was planning to send a PR to Linus with a quirk for MSI GS66 Stealth 11UG, and apparently this bug report would add two additional MSI entries. This is becoming quickly a maintenance hell.
Honestly, what would be the immediate effects of just not enabling the TPM irq by default at all, and making it an explicit opt-in?
When a common solution is to just disable the TPM in the BIOS entirely, and the end result is a working system, I really get the teeling that this is all pain for very very little gain.
Would anybody even notice if we just disabled it by default and added a "if you really want it, use 'tpm=irq' kernel command line"?
Hmm?
Linus
On Fri Aug 11, 2023 at 9:55 PM EEST, Linus Torvalds wrote:
On Fri, 11 Aug 2023 at 10:22, Jarkko Sakkinen jarkko@kernel.org wrote:
I was planning to send a PR to Linus with a quirk for MSI GS66 Stealth 11UG, and apparently this bug report would add two additional MSI entries. This is becoming quickly a maintenance hell.
Honestly, what would be the immediate effects of just not enabling the TPM irq by default at all, and making it an explicit opt-in?
When a common solution is to just disable the TPM in the BIOS entirely, and the end result is a working system, I really get the teeling that this is all pain for very very little gain.
Would anybody even notice if we just disabled it by default and added a "if you really want it, use 'tpm=irq' kernel command line"?
Hmm?
Linus
It would be in line with what I proposed in [1] (option A), and also what Grundik said in this thread.
Right now IRQs should be only enabled by a command-line option, and what "allow list" would mean in practice should really be something that would be advertised e.g. Device Tree or ACPI, not really part of the solution implemented right now.
I'll send a patch ASAP, which makes this feature opt-in.
[1] https://lore.kernel.org/linux-integrity/CUPW0XP1RFXI.162GZ78E46TBJ@suppilova...
BR, Jarkko
On Mon, 2023-07-10 at 15:38 +0200, Christian Hesse wrote:
This device suffer an irq storm, so add it in tpm_tis_dmi_table to force polling.
https://bugs.archlinux.org/user/38129 https://bugzilla.kernel.org/show_bug.cgi?id=217631
Fixes: e644b2f498d297a928efcb7ff6f900c27f8b788e Cc: stable@vger.kernel.org Reported-by: roubro1991@gmail.com Signed-off-by: Christian Hesse mail@eworm.de
drivers/char/tpm/tpm_tis.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index 7db3593941ea..2979f8b9aaa0 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -114,6 +114,14 @@ static int tpm_tis_disable_irq(const struct dmi_system_id *d) } static const struct dmi_system_id tpm_tis_dmi_table[] = {
- {
.callback = tpm_tis_disable_irq,.ident = "Framework Laptop Intel 12th gen",.matches = {DMI_MATCH(DMI_SYS_VENDOR, "Framework"),DMI_MATCH(DMI_PRODUCT_VERSION, "A4"),},- }, { .callback = tpm_tis_disable_irq, .ident = "ThinkPad T490s",
Hi, no other issues except fixes tag should be in both:
Fixes: e644b2f498d2 ("tpm, tpm_tis: Enable interrupt test")
I.e.
# $1 = commit ID function git-fixes { git --no-pager log --format='Fixes: %h ("%s")' --abbrev=12 -1 $1; }
I'll update them, thank you.
BR, Jarkko
linux-stable-mirror@lists.linaro.org
-
Christian Hesse -
Christian Hesse -
Greg KH -
Grundik -
James Bottomley -
Jarkko Sakkinen -
Jason Gunthorpe -
Jerry Snitselaar -
Lino Sanfilippo -
Linus Torvalds -
Linux regression tracking (Thorsten Leemhuis) -
Thorsten Leemhuis