| Message ID | d84f8e06-f646-8b43-d063-fb11f4827044@siemens.com |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: iTCO_wdt: Fix detection of SMI-off case | expand |
On 26/07/21 13:46, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > Obviously, the test needs to run against the register content, not its > address. > > Fixes: cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on second timeout") > Reported-by: Mantas Mikulėnas <grawity@gmail.com> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > drivers/watchdog/iTCO_wdt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c > index b3f604669e2c..643c6c2d0b72 100644 > --- a/drivers/watchdog/iTCO_wdt.c > +++ b/drivers/watchdog/iTCO_wdt.c > @@ -362,7 +362,7 @@ static int iTCO_wdt_set_timeout(struct watchdog_device *wd_dev, unsigned int t) > * Otherwise, the BIOS generally reboots when the SMI triggers. > */ > if (p->smi_res && > - (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) > + (inl(SMI_EN(p)) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) > tmrval /= 2; > > /* from the specs: */ > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Cc: stable@vger.kernel.org (the latter because cb011044e34c has been picked up by stable kernels already). Paolo
On 26.07.21 14:03, Paolo Bonzini wrote: > On 26/07/21 13:46, Jan Kiszka wrote: >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> Obviously, the test needs to run against the register content, not its >> address. >> >> Fixes: cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on >> second timeout") >> Reported-by: Mantas Mikulėnas <grawity@gmail.com> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> --- >> drivers/watchdog/iTCO_wdt.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c >> index b3f604669e2c..643c6c2d0b72 100644 >> --- a/drivers/watchdog/iTCO_wdt.c >> +++ b/drivers/watchdog/iTCO_wdt.c >> @@ -362,7 +362,7 @@ static int iTCO_wdt_set_timeout(struct >> watchdog_device *wd_dev, unsigned int t) >> * Otherwise, the BIOS generally reboots when the SMI triggers. >> */ >> if (p->smi_res && >> - (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) >> + (inl(SMI_EN(p)) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | >> GBL_SMI_EN)) >> tmrval /= 2; >> /* from the specs: */ >> > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > Cc: stable@vger.kernel.org > > (the latter because cb011044e34c has been picked up by stable kernels > already). > Thanks. Originally wanted to add stable myself, but I'm still unsure whether this is the privilege of the subsystem maintainer or should also be done by contributors. Jan
On 7/26/21 4:46 AM, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > Obviously, the test needs to run against the register content, not its > address. > > Fixes: cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on second timeout") > Reported-by: Mantas Mikulėnas <grawity@gmail.com> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/watchdog/iTCO_wdt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c > index b3f604669e2c..643c6c2d0b72 100644 > --- a/drivers/watchdog/iTCO_wdt.c > +++ b/drivers/watchdog/iTCO_wdt.c > @@ -362,7 +362,7 @@ static int iTCO_wdt_set_timeout(struct watchdog_device *wd_dev, unsigned int t) > * Otherwise, the BIOS generally reboots when the SMI triggers. > */ > if (p->smi_res && > - (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) > + (inl(SMI_EN(p)) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) > tmrval /= 2; > > /* from the specs: */ >
On 7/26/21 6:40 AM, Andy Shevchenko wrote: > On Mon, Jul 26, 2021 at 3:04 PM Jan Kiszka <jan.kiszka@siemens.com> wrote: >> >> On 26.07.21 14:01, Andy Shevchenko wrote: >>> On Mon, Jul 26, 2021 at 2:46 PM Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>> >>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>> >>>> Obviously, the test needs to run against the register content, not its >>>> address. >>>> >>>> Fixes: cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on second timeout") >>>> Reported-by: Mantas Mikulėnas <grawity@gmail.com> >>> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>> >>> Missed SoB of the submitter (hint: configure your Git to make sure >>> that submitter and author are the same in terms of name-email). >> >> The signed off is there. Not sure what you are referring to. > > Nope. It's not. The sign of that is the From: line in the body of the > email. It happens when the submitter != author. And SoB of the former > one is absent. But what is strange is that reading them here I haven't > found the difference. Maybe one is in UTF-8 while the other is not and > a unicode character degraded to Latin-1 or so? > I have no idea why there is an additional From:, but both From: tags in the e-mail source are exact matches, and both match the name and e-mail address in Signed-off-by:. I agree with Jan, the SoB is there. Guenter
On Mon, Jul 26, 2021 at 5:05 PM Jan Kiszka <jan.kiszka@siemens.com> wrote: > > On 26.07.21 15:59, Guenter Roeck wrote: > > On 7/26/21 6:40 AM, Andy Shevchenko wrote: > >> On Mon, Jul 26, 2021 at 3:04 PM Jan Kiszka <jan.kiszka@siemens.com> > >> wrote: > >>> > >>> On 26.07.21 14:01, Andy Shevchenko wrote: > >>>> On Mon, Jul 26, 2021 at 2:46 PM Jan Kiszka <jan.kiszka@siemens.com> > >>>> wrote: > >>>>> > >>>>> From: Jan Kiszka <jan.kiszka@siemens.com> > >>>>> > >>>>> Obviously, the test needs to run against the register content, not its > >>>>> address. > >>>>> > >>>>> Fixes: cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on > >>>>> second timeout") > >>>>> Reported-by: Mantas Mikulėnas <grawity@gmail.com> > >>>> > >>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > >>>> > >>>> Missed SoB of the submitter (hint: configure your Git to make sure > >>>> that submitter and author are the same in terms of name-email). > >>> > >>> The signed off is there. Not sure what you are referring to. > >> > >> Nope. It's not. The sign of that is the From: line in the body of the > >> email. It happens when the submitter != author. And SoB of the former > >> one is absent. But what is strange is that reading them here I haven't > >> found the difference. Maybe one is in UTF-8 while the other is not and > >> a unicode character degraded to Latin-1 or so? > >> > > > > I have no idea why there is an additional From:, but both From: > > tags in the e-mail source are exact matches, and both match the > > name and e-mail address in Signed-off-by:. I agree with Jan, > > the SoB is there. > > There is one unknown in this equation, and that is the anti-email system > operated by a our IT and some company in Redmond. Hmm... The From: in the body is the result of the `git format-patch` I believe. So, two (or more?) possibilities here: 1) your configuration enforces it to always put From: (something new to me); 2) the submitter and author are not the same (see also: https://github.com/git/git/commit/a90804752f6ab2b911882d47fafb6c2b78f447c3); 3) ...anything else...? > But I haven't received > any complaints that my outgoing emails are negatively affected by it > (incoming are, but that's a different story...). If you received > something mangled, Andy, please share the source of that email. I'm > happy to escalate internally - and externally. I believe I see it in the same way as lore, i.e. https://lore.kernel.org/linux-watchdog/d84f8e06-f646-8b43-d063-fb11f4827044@siemens.com/raw > For the potential case they were mangled or in case I'm submitting via a > real email provider, my scripts always add a "From:" to the body of my > patches. Outgoing, that From matched my Signed-off.
On Mon, Jul 26, 2021 at 2:46 PM Jan Kiszka <jan.kiszka@siemens.com> wrote: > > From: Jan Kiszka <jan.kiszka@siemens.com> > > Obviously, the test needs to run against the register content, not its > address. > > Fixes: cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on second timeout") > Reported-by: Mantas Mikulėnas <grawity@gmail.com> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > drivers/watchdog/iTCO_wdt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c > index b3f604669e2c..643c6c2d0b72 100644 > --- a/drivers/watchdog/iTCO_wdt.c > +++ b/drivers/watchdog/iTCO_wdt.c > @@ -362,7 +362,7 @@ static int iTCO_wdt_set_timeout(struct watchdog_device *wd_dev, unsigned int t) > * Otherwise, the BIOS generally reboots when the SMI triggers. > */ > if (p->smi_res && > - (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) > + (inl(SMI_EN(p)) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) > tmrval /= 2; > > /* from the specs: */ > -- > 2.26.2 Tested-by: Mantas Mikulėnas <grawity@gmail.com>
On 26.07.21 16:51, Andy Shevchenko wrote: > On Mon, Jul 26, 2021 at 5:05 PM Jan Kiszka <jan.kiszka@siemens.com> wrote: >> >> On 26.07.21 15:59, Guenter Roeck wrote: >>> On 7/26/21 6:40 AM, Andy Shevchenko wrote: >>>> On Mon, Jul 26, 2021 at 3:04 PM Jan Kiszka <jan.kiszka@siemens.com> >>>> wrote: >>>>> >>>>> On 26.07.21 14:01, Andy Shevchenko wrote: >>>>>> On Mon, Jul 26, 2021 at 2:46 PM Jan Kiszka <jan.kiszka@siemens.com> >>>>>> wrote: >>>>>>> >>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>>>>> >>>>>>> Obviously, the test needs to run against the register content, not its >>>>>>> address. >>>>>>> >>>>>>> Fixes: cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on >>>>>>> second timeout") >>>>>>> Reported-by: Mantas Mikulėnas <grawity@gmail.com> >>>>>> >>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>> >>>>>> Missed SoB of the submitter (hint: configure your Git to make sure >>>>>> that submitter and author are the same in terms of name-email). >>>>> >>>>> The signed off is there. Not sure what you are referring to. >>>> >>>> Nope. It's not. The sign of that is the From: line in the body of the >>>> email. It happens when the submitter != author. And SoB of the former >>>> one is absent. But what is strange is that reading them here I haven't >>>> found the difference. Maybe one is in UTF-8 while the other is not and >>>> a unicode character degraded to Latin-1 or so? >>>> >>> >>> I have no idea why there is an additional From:, but both From: >>> tags in the e-mail source are exact matches, and both match the >>> name and e-mail address in Signed-off-by:. I agree with Jan, >>> the SoB is there. >> >> There is one unknown in this equation, and that is the anti-email system >> operated by a our IT and some company in Redmond. > > Hmm... The From: in the body is the result of the `git format-patch` I believe. > So, two (or more?) possibilities here: > 1) your configuration enforces it to always put From: (something new to me); Yes, it does, as I explained in my other reply. That's a safety net because you never have full control over what some mail servers do to the first From. > 2) the submitter and author are not the same (see also: > https://github.com/git/git/commit/a90804752f6ab2b911882d47fafb6c2b78f447c3); > 3) ...anything else...? > >> But I haven't received >> any complaints that my outgoing emails are negatively affected by it >> (incoming are, but that's a different story...). If you received >> something mangled, Andy, please share the source of that email. I'm >> happy to escalate internally - and externally. > > I believe I see it in the same way as lore, i.e. > https://lore.kernel.org/linux-watchdog/d84f8e06-f646-8b43-d063-fb11f4827044@siemens.com/raw Perfect, then all is fine as it should be (and no time for O365 bashing, today). Jan
On 26.07.21 13:46, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > Obviously, the test needs to run against the register content, not its > address. > > Fixes: cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on second timeout") > Reported-by: Mantas Mikulėnas <grawity@gmail.com> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > drivers/watchdog/iTCO_wdt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c > index b3f604669e2c..643c6c2d0b72 100644 > --- a/drivers/watchdog/iTCO_wdt.c > +++ b/drivers/watchdog/iTCO_wdt.c > @@ -362,7 +362,7 @@ static int iTCO_wdt_set_timeout(struct watchdog_device *wd_dev, unsigned int t) > * Otherwise, the BIOS generally reboots when the SMI triggers. > */ > if (p->smi_res && > - (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) > + (inl(SMI_EN(p)) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) > tmrval /= 2; > > /* from the specs: */ > Ping, this is still missing in master. Stable kernels had the revert, but 5.14 will need this. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux
On 20.08.21 15:45, Jan Kiszka wrote: > On 26.07.21 13:46, Jan Kiszka wrote: >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> Obviously, the test needs to run against the register content, not its >> address. >> >> Fixes: cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on second timeout") >> Reported-by: Mantas Mikulėnas <grawity@gmail.com> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> --- >> drivers/watchdog/iTCO_wdt.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c >> index b3f604669e2c..643c6c2d0b72 100644 >> --- a/drivers/watchdog/iTCO_wdt.c >> +++ b/drivers/watchdog/iTCO_wdt.c >> @@ -362,7 +362,7 @@ static int iTCO_wdt_set_timeout(struct watchdog_device *wd_dev, unsigned int t) >> * Otherwise, the BIOS generally reboots when the SMI triggers. >> */ >> if (p->smi_res && >> - (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) >> + (inl(SMI_EN(p)) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) >> tmrval /= 2; >> >> /* from the specs: */ >> > > Ping, this is still missing in master. Stable kernels had the revert, > but 5.14 will need this. > Second reminder: 5.14 is out and now broken. Is the patch queued somewhere? I do not see it in the watchdog staging branch. Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux
On 8/30/21 12:47 PM, Jan Kiszka wrote: > On 20.08.21 15:45, Jan Kiszka wrote: >> On 26.07.21 13:46, Jan Kiszka wrote: >>> From: Jan Kiszka <jan.kiszka@siemens.com> >>> >>> Obviously, the test needs to run against the register content, not its >>> address. >>> >>> Fixes: cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on second timeout") >>> Reported-by: Mantas Mikulėnas <grawity@gmail.com> >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>> --- >>> drivers/watchdog/iTCO_wdt.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c >>> index b3f604669e2c..643c6c2d0b72 100644 >>> --- a/drivers/watchdog/iTCO_wdt.c >>> +++ b/drivers/watchdog/iTCO_wdt.c >>> @@ -362,7 +362,7 @@ static int iTCO_wdt_set_timeout(struct watchdog_device *wd_dev, unsigned int t) >>> * Otherwise, the BIOS generally reboots when the SMI triggers. >>> */ >>> if (p->smi_res && >>> - (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) >>> + (inl(SMI_EN(p)) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) >>> tmrval /= 2; >>> >>> /* from the specs: */ >>> >> >> Ping, this is still missing in master. Stable kernels had the revert, >> but 5.14 will need this. >> > > Second reminder: 5.14 is out and now broken. Is the patch queued > somewhere? I do not see it in the watchdog staging branch. > I had it in my own watchdog-next branch for about a month. Usually Wim picks it up from there or from the mainling list; he handles all upstreaming. Wim ? Guenter
Hi All, > On 8/30/21 12:47 PM, Jan Kiszka wrote: > >On 20.08.21 15:45, Jan Kiszka wrote: > >>On 26.07.21 13:46, Jan Kiszka wrote: > >>>From: Jan Kiszka <jan.kiszka@siemens.com> > >>> > >>>Obviously, the test needs to run against the register content, not its > >>>address. > >>> > >>>Fixes: cb011044e34c ("watchdog: iTCO_wdt: Account for rebooting on second timeout") > >>>Reported-by: Mantas Mikulėnas <grawity@gmail.com> > >>>Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > >>>--- > >>> drivers/watchdog/iTCO_wdt.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>>diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c > >>>index b3f604669e2c..643c6c2d0b72 100644 > >>>--- a/drivers/watchdog/iTCO_wdt.c > >>>+++ b/drivers/watchdog/iTCO_wdt.c > >>>@@ -362,7 +362,7 @@ static int iTCO_wdt_set_timeout(struct watchdog_device *wd_dev, unsigned int t) > >>> * Otherwise, the BIOS generally reboots when the SMI triggers. > >>> */ > >>> if (p->smi_res && > >>>- (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) > >>>+ (inl(SMI_EN(p)) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) > >>> tmrval /= 2; > >>> /* from the specs: */ > >>> > >> > >>Ping, this is still missing in master. Stable kernels had the revert, > >>but 5.14 will need this. > >> > > > >Second reminder: 5.14 is out and now broken. Is the patch queued > >somewhere? I do not see it in the watchdog staging branch. > > > > I had it in my own watchdog-next branch for about a month. > Usually Wim picks it up from there or from the mainling list; > he handles all upstreaming. Wim ? This one is in linux-watchdog-next since 22 Aug. Working on getting it upstream now. Kind regards, Wim.
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c index b3f604669e2c..643c6c2d0b72 100644 --- a/drivers/watchdog/iTCO_wdt.c +++ b/drivers/watchdog/iTCO_wdt.c @@ -362,7 +362,7 @@ static int iTCO_wdt_set_timeout(struct watchdog_device *wd_dev, unsigned int t) * Otherwise, the BIOS generally reboots when the SMI triggers. */ if (p->smi_res && - (SMI_EN(p) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) + (inl(SMI_EN(p)) & (TCO_EN | GBL_SMI_EN)) != (TCO_EN | GBL_SMI_EN)) tmrval /= 2; /* from the specs: */