Skip to contentSkip to conversation

Faulty commit "watchdog: iTCO_wdt: Account for rebooting on second timeout"

DISCUSSION Jean Delvare 5 participants 8 messages Aug 3, 2021 – Aug 6, 2021

Subsystems (2)
Stable branch Watchdog device drivers
lore
Share & tools
lore

Jean Delvare

author
#0 lore raw

Hi all,

Commit cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on second timeout") causes a regression on several systems. Symptoms are:
system reboots automatically after a short period of time if watchdog is enabled (by systemd for example). This has been reported in bugzilla:

https://bugzilla.kernel.org/show_bug.cgi?id=213809

Unfortunately this commit was backported to all stable kernel branches (4.14, 4.19, 5.4, 5.10, 5.12 and 5.13). I'm not sure why that is the case, BTW, as there is no Fixes tag and no Cc to stable@vger either. And the fix is not trivial, has apparently not seen enough testing, and addresses a problem that has a known and simple workaround. IMHO it should never have been accepted as a stable patch in the first place. Especially when the previous attempt to fix this issue already ended with a regression and a revert.

Anyway... After a glance at the patch, I see what looks like a nice thinko:

+ if (p->smi_res &&
+ (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN))

The author most certainly meant inl(SMI_EN(p)) (the register's value) and not SMI_EN(p) (the register's address).

Signature · 2 lines
Jean Delvare
SUSE L3 Support
#1 lore raw
Quoted 24 lines · Jean Delvare

Hi all,

Commit cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on second timeout") causes a regression on several systems. Symptoms are:
system reboots automatically after a short period of time if watchdog is enabled (by systemd for example). This has been reported in bugzilla:

https://bugzilla.kernel.org/show_bug.cgi?id=213809

Unfortunately this commit was backported to all stable kernel branches (4.14, 4.19, 5.4, 5.10, 5.12 and 5.13). I'm not sure why that is the case, BTW, as there is no Fixes tag and no Cc to stable@vger either. And the fix is not trivial, has apparently not seen enough testing, and addresses a problem that has a known and simple workaround. IMHO it should never have been accepted as a stable patch in the first place. Especially when the previous attempt to fix this issue already ended with a regression and a revert.

Anyway... After a glance at the patch, I see what looks like a nice thinko:

(quoted diff) +2 -0
+	if (p->smi_res &&
+	    (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN))
Quoted 3 lines

The author most certainly meant inl(SMI_EN(p)) (the register's value) and not SMI_EN(p) (the register's address).

https://lkml.org/lkml/2021/7/26/349

Jan

Signature · 2 lines
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
#2 lore raw
Quoted 22 lines · Jan Kiszka
Quoted 21 lines · Jean Delvare

Hi all,

Commit cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on second timeout") causes a regression on several systems. Symptoms are:
system reboots automatically after a short period of time if watchdog is enabled (by systemd for example). This has been reported in bugzilla:

https://bugzilla.kernel.org/show_bug.cgi?id=213809

Unfortunately this commit was backported to all stable kernel branches (4.14, 4.19, 5.4, 5.10, 5.12 and 5.13). I'm not sure why that is the case, BTW, as there is no Fixes tag and no Cc to stable@vger either. And the fix is not trivial, has apparently not seen enough testing, and addresses a problem that has a known and simple workaround. IMHO it should never have been accepted as a stable patch in the first place. Especially when the previous attempt to fix this issue already ended with a regression and a revert.

Anyway... After a glance at the patch, I see what looks like a nice thinko:

Quoted 9 lines
Quoted 6 lines
(quoted diff) +2 -0
+	if (p->smi_res &&
+	    (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN))

The author most certainly meant inl(SMI_EN(p)) (the register's value) and not SMI_EN(p) (the register's address).

https://lkml.org/lkml/2021/7/26/349

That's for the fix (in line with your analysis).

I was also wondering if backporting that quickly was needed. Didn't propose it, though.

Jan

Signature · 2 lines
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
maintainer
#3 lore raw
Quoted 23 lines · Jan Kiszka
Quoted 22 lines · Jan Kiszka
Quoted 21 lines · Jean Delvare

Hi all,

Commit cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on second timeout") causes a regression on several systems. Symptoms are:
system reboots automatically after a short period of time if watchdog is enabled (by systemd for example). This has been reported in bugzilla:

https://bugzilla.kernel.org/show_bug.cgi?id=213809

Unfortunately this commit was backported to all stable kernel branches (4.14, 4.19, 5.4, 5.10, 5.12 and 5.13). I'm not sure why that is the case, BTW, as there is no Fixes tag and no Cc to stable@vger either. And the fix is not trivial, has apparently not seen enough testing, and addresses a problem that has a known and simple workaround. IMHO it should never have been accepted as a stable patch in the first place. Especially when the previous attempt to fix this issue already ended with a regression and a revert.

Anyway... After a glance at the patch, I see what looks like a nice thinko:

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
(quoted diff) +2 -0
+	if (p->smi_res &&
+	    (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN))

The author most certainly meant inl(SMI_EN(p)) (the register's value) and not SMI_EN(p) (the register's address).

Yes, shame on me that I didn't see that.

Quoted 9 lines
Quoted 3 lines

https://lkml.org/lkml/2021/7/26/349

That's for the fix (in line with your analysis).

I was also wondering if backporting that quickly was needed. Didn't propose it, though.

I'd suggest to discuss that with Greg and Sasha. Backporting is pretty aggressive nowadays.

Guenter

#6 lore raw
Quoted 10 lines · Michael Marley
Quoted 2 lines · Jan Kiszka

https://lkml.org/lkml/2021/7/26/349

It fixes the problem for me (the person who opened the Bugzilla report) too, thanks!

Tested-by Michael Marley <[email protected]>

Thanks for the confirmation!

Yeah, sorry, that original mistake is truly mine. In fact, I wrote this code twice [1] but only messed it up here. Unfortunate that it spread so quickly. I'll try discuss this with stable people eventually, if it was a one off or if there are more such cases.

Jan

[1]
https://github.com/siemens/efibootguard/commit/aa89fe3cbd883198c23eaec43c4448fe9e8ae148

Signature · 2 lines
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux
#7 lore raw
In reply to Jean Delvare
Quoted 24 lines · Jean Delvare

Hi all,

Commit cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on second timeout") causes a regression on several systems. Symptoms are:
system reboots automatically after a short period of time if watchdog is enabled (by systemd for example). This has been reported in bugzilla:

https://bugzilla.kernel.org/show_bug.cgi?id=213809

Unfortunately this commit was backported to all stable kernel branches (4.14, 4.19, 5.4, 5.10, 5.12 and 5.13). I'm not sure why that is the case, BTW, as there is no Fixes tag and no Cc to stable@vger either. And the fix is not trivial, has apparently not seen enough testing, and addresses a problem that has a known and simple workaround. IMHO it should never have been accepted as a stable patch in the first place. Especially when the previous attempt to fix this issue already ended with a regression and a revert.

Anyway... After a glance at the patch, I see what looks like a nice thinko:

(quoted diff) +2 -0
+	if (p->smi_res &&
+	    (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN))
Quoted 2 lines

The author most certainly meant inl(SMI_EN(p)) (the register's value) and not SMI_EN(p) (the register's address).

Let me go revert this from the stable trees now, thanks for the report.

greg k-h