watchdog: iTCO_wdt: Fix detection of SMI-off case

Commit Message

Jan Kiszka July 26, 2021, 11:46 a.m. UTC

From: Jan Kiszka <[e-mail]>

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 <[e-mail]>
Signed-off-by: Jan Kiszka <[e-mail]>
---
 drivers/watchdog/iTCO_wdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andy Shevchenko July 26, 2021, 12:01 p.m. UTC | #1

On Mon, Jul 26, 2021 at 2:46 PM Jan Kiszka <[e-mail]> wrote:
>
> From: Jan Kiszka <[e-mail]>
>
> 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 <[e-mail]>

> Signed-off-by: Jan Kiszka <[e-mail]>

Missed SoB of the submitter (hint: configure your Git to make sure
that submitter and author are the same in terms of name-email).

...

>         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;

There are so many parentheses, perhaps

#define TCO_GBL_SMI_EN   (TCO_EN | GBL_SMI_EN)
...
       if (p->smi_res &&
           (inl(SMI_EN(p)) & TCO_GBL_SMI_EN) != TCO_GBL_SMI_EN)
               tmrval /= 2;

?

Paolo Bonzini July 26, 2021, 12:03 p.m. UTC | #2

On 26/07/21 13:46, Jan Kiszka wrote:
> From: Jan Kiszka <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Jan Kiszka <[e-mail]>
> ---
>   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 <[e-mail]>
Cc: [e-mail]

(the latter because cb011044e34c has been picked up by stable kernels 
already).

Paolo

Jan Kiszka July 26, 2021, 12:04 p.m. UTC | #3

On 26.07.21 14:01, Andy Shevchenko wrote:
> On Mon, Jul 26, 2021 at 2:46 PM Jan Kiszka <[e-mail]> wrote:
>>
>> From: Jan Kiszka <[e-mail]>
>>
>> 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 <[e-mail]>
> 
>> Signed-off-by: Jan Kiszka <[e-mail]>
> 
> 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.

> 
> ...
> 
>>         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;
> 
> There are so many parentheses, perhaps
> 
> #define TCO_GBL_SMI_EN   (TCO_EN | GBL_SMI_EN)
> ...
>        if (p->smi_res &&
>            (inl(SMI_EN(p)) & TCO_GBL_SMI_EN) != TCO_GBL_SMI_EN)
>                tmrval /= 2;
> 
> ?
> 

Let's focus on the regression fix (and you could have mentioned that on
the original patch already).

Jan

Jan Kiszka July 26, 2021, 12:05 p.m. UTC | #4

On 26.07.21 14:03, Paolo Bonzini wrote:
> On 26/07/21 13:46, Jan Kiszka wrote:
>> From: Jan Kiszka <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Jan Kiszka <[e-mail]>
>> ---
>>   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 <[e-mail]>
> Cc: [e-mail]
> 
> (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

Andy Shevchenko July 26, 2021, 1:40 p.m. UTC | #5

On Mon, Jul 26, 2021 at 3:04 PM Jan Kiszka <[e-mail]> wrote:
>
> On 26.07.21 14:01, Andy Shevchenko wrote:
> > On Mon, Jul 26, 2021 at 2:46 PM Jan Kiszka <[e-mail]> wrote:
> >>
> >> From: Jan Kiszka <[e-mail]>
> >>
> >> 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 <[e-mail]>
> >
> >> Signed-off-by: Jan Kiszka <[e-mail]>
> >
> > 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?

Guenter Roeck July 26, 2021, 1:44 p.m. UTC | #6

On 7/26/21 4:46 AM, Jan Kiszka wrote:
> From: Jan Kiszka <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Jan Kiszka <[e-mail]>

Reviewed-by: Guenter Roeck <[e-mail]>

> ---
>   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: */
>

Guenter Roeck July 26, 2021, 1:51 p.m. UTC | #7

On 7/26/21 5:05 AM, Jan Kiszka wrote:
> On 26.07.21 14:03, Paolo Bonzini wrote:
>> On 26/07/21 13:46, Jan Kiszka wrote:
>>> From: Jan Kiszka <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Jan Kiszka <[e-mail]>
>>> ---
>>>    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 <[e-mail]>
>> Cc: [e-mail]
>>
>> (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.
> 

Normally it is done by maintainers.

Guenter

Guenter Roeck July 26, 2021, 1:59 p.m. UTC | #8

On 7/26/21 6:40 AM, Andy Shevchenko wrote:
> On Mon, Jul 26, 2021 at 3:04 PM Jan Kiszka <[e-mail]> wrote:
>>
>> On 26.07.21 14:01, Andy Shevchenko wrote:
>>> On Mon, Jul 26, 2021 at 2:46 PM Jan Kiszka <[e-mail]> wrote:
>>>>
>>>> From: Jan Kiszka <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>
>>>> Signed-off-by: Jan Kiszka <[e-mail]>
>>>
>>> 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

Jan Kiszka July 26, 2021, 2:05 p.m. UTC | #9

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 <[e-mail]>
>> wrote:
>>>
>>> On 26.07.21 14:01, Andy Shevchenko wrote:
>>>> On Mon, Jul 26, 2021 at 2:46 PM Jan Kiszka <[e-mail]>
>>>> wrote:
>>>>>
>>>>> From: Jan Kiszka <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>
>>>>> Signed-off-by: Jan Kiszka <[e-mail]>
>>>>
>>>> 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. 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.

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.

Jan

Andy Shevchenko July 26, 2021, 2:51 p.m. UTC | #10

On Mon, Jul 26, 2021 at 5:05 PM Jan Kiszka <[e-mail]> 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 <[e-mail]>
> >> wrote:
> >>>
> >>> On 26.07.21 14:01, Andy Shevchenko wrote:
> >>>> On Mon, Jul 26, 2021 at 2:46 PM Jan Kiszka <[e-mail]>
> >>>> wrote:
> >>>>>
> >>>>> From: Jan Kiszka <[e-mail]>
> >>>>>
> >>>>> 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 <[e-mail]>
> >>>>
> >>>>> Signed-off-by: Jan Kiszka <[e-mail]>
> >>>>
> >>>> 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/[e-mail]/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.

Mantas Mikulėnas July 26, 2021, 4:59 p.m. UTC | #11

On Mon, Jul 26, 2021 at 2:46 PM Jan Kiszka <[e-mail]> wrote:
>
> From: Jan Kiszka <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Jan Kiszka <[e-mail]>
> ---
>  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 <[e-mail]>

Jan Kiszka July 26, 2021, 5:10 p.m. UTC | #12

On 26.07.21 16:51, Andy Shevchenko wrote:
> On Mon, Jul 26, 2021 at 5:05 PM Jan Kiszka <[e-mail]> 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 <[e-mail]>
>>>> wrote:
>>>>>
>>>>> On 26.07.21 14:01, Andy Shevchenko wrote:
>>>>>> On Mon, Jul 26, 2021 at 2:46 PM Jan Kiszka <[e-mail]>
>>>>>> wrote:
>>>>>>>
>>>>>>> From: Jan Kiszka <[e-mail]>
>>>>>>>
>>>>>>> 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 <[e-mail]>
>>>>>>
>>>>>>> Signed-off-by: Jan Kiszka <[e-mail]>
>>>>>>
>>>>>> 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/[e-mail]/raw

Perfect, then all is fine as it should be (and no time for O365 bashing,
today).

Jan

Andy Shevchenko July 26, 2021, 9:45 p.m. UTC | #13

On Mon, Jul 26, 2021 at 8:10 PM Jan Kiszka <[e-mail]> wrote:
> On 26.07.21 16:51, Andy Shevchenko wrote:
> > On Mon, Jul 26, 2021 at 5:05 PM Jan Kiszka <[e-mail]> wrote:
> >> On 26.07.21 15:59, Guenter Roeck wrote:
> >>> On 7/26/21 6:40 AM, Andy Shevchenko wrote:

...

> >>> 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.

I see now. Thanks for your patience and elaboration, it's all good then!

> >  2) the submitter and author are not the same (see also:
> > https://github.com/git/git/commit/a90804752f6ab2b911882d47fafb6c2b78f447c3);
> >  3) ...anything else...?

Jan Kiszka Aug. 20, 2021, 1:45 p.m. UTC | #14

On 26.07.21 13:46, Jan Kiszka wrote:
> From: Jan Kiszka <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Jan Kiszka <[e-mail]>
> ---
>  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

Jan Kiszka Aug. 30, 2021, 7:47 p.m. UTC | #15

On 20.08.21 15:45, Jan Kiszka wrote:
> On 26.07.21 13:46, Jan Kiszka wrote:
>> From: Jan Kiszka <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Jan Kiszka <[e-mail]>
>> ---
>>  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

Guenter Roeck Aug. 30, 2021, 8 p.m. UTC | #16

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 <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Jan Kiszka <[e-mail]>
>>> ---
>>>   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

Wim Van Sebroeck Sept. 7, 2021, 8:14 p.m. UTC | #17

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 <[e-mail]>
> >>>
> >>>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 <[e-mail]>
> >>>Signed-off-by: Jan Kiszka <[e-mail]>
> >>>---
> >>>  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.

Patch

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: */