| Message ID | 20240826075303.3964392-1-oocheret@cisco.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] iTCO_wdt: ignore NMI_NOW bit on register comparison | expand |
Hi, On Mon, Aug 26, 2024 at 12:53:01AM -0700, Oleksandr Ocheretnyi wrote: > Commit da23b6faa8bf ("watchdog: iTCO: Add support for Cannon Lake > PCH iTCO") does not ignore NMI_NOW bit on write operation to TCO1_CNT > register what causes unexpected NMIs due to NMI_NOW bit inversion > during regular crashkernel's workflow with following logs: > > iTCO_vendor_support: vendor-support=0 > iTCO_wdt iTCO_wdt: unable to reset NO_REBOOT flag, device > disabled by hardware/BIOS > > This change clears NMI_NOW bit in the TCO1_CNT register to have no > effect on NMI_NOW bit inversion what can cause NMI immediately. > > Fixes: da23b6faa8bf ("watchdog: iTCO: Add support for Cannon Lake PCH iTCO") > Signed-off-by: Oleksandr Ocheretnyi <oocheret@cisco.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 264857d314da..679c115ef7d3 100644 > --- a/drivers/watchdog/iTCO_wdt.c > +++ b/drivers/watchdog/iTCO_wdt.c > @@ -224,7 +224,7 @@ static int update_no_reboot_bit_cnt(void *priv, bool set) > val |= BIT(0); > else > val &= ~BIT(0); > - outw(val, TCO1_CNT(p)); > + outw(val & ~BIT(8), TCO1_CNT(p)); I suggest adding some #define for the magical number 8 so that it is easier for anyone looking at this driver to figure out what it is doing. Otherwise looks good to me, thanks! > newval = inw(TCO1_CNT(p)); > > /* make sure the update is successful */ > -- > 2.39.3
On 8/26/24 08:12, Guenter Roeck wrote: > On 8/26/24 04:18, Mika Westerberg wrote: >> Hi, >> >> On Mon, Aug 26, 2024 at 12:53:01AM -0700, Oleksandr Ocheretnyi wrote: >>> Commit da23b6faa8bf ("watchdog: iTCO: Add support for Cannon Lake >>> PCH iTCO") does not ignore NMI_NOW bit on write operation to TCO1_CNT >>> register what causes unexpected NMIs due to NMI_NOW bit inversion >>> during regular crashkernel's workflow with following logs: >>> >>> iTCO_vendor_support: vendor-support=0 >>> iTCO_wdt iTCO_wdt: unable to reset NO_REBOOT flag, device >>> disabled by hardware/BIOS >>> >>> This change clears NMI_NOW bit in the TCO1_CNT register to have no >>> effect on NMI_NOW bit inversion what can cause NMI immediately. >>> >>> Fixes: da23b6faa8bf ("watchdog: iTCO: Add support for Cannon Lake PCH iTCO") >>> Signed-off-by: Oleksandr Ocheretnyi <oocheret@cisco.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 264857d314da..679c115ef7d3 100644 >>> --- a/drivers/watchdog/iTCO_wdt.c >>> +++ b/drivers/watchdog/iTCO_wdt.c >>> @@ -224,7 +224,7 @@ static int update_no_reboot_bit_cnt(void *priv, bool set) >>> val |= BIT(0); >>> else >>> val &= ~BIT(0); >>> - outw(val, TCO1_CNT(p)); >>> + outw(val & ~BIT(8), TCO1_CNT(p)); >> >> I suggest adding some #define for the magical number 8 so that it is >> easier for anyone looking at this driver to figure out what it is doing. >> >> Otherwise looks good to me, thanks! >> > > Not really; it appears to be hiding a change in code which is supposed to do > something different (it is supposed to set or clear the no_reboot bit), > with no explanation whatsoever. That warrants a comment in the code. > Also, I would prefer > val = inw(TCO1_CNT(p)) & ~BIT(8); > On top of that, I fail to understand "on register comparison" in the subject. What register comparison ? Guenter
On 8/26/24 08:15, Guenter Roeck wrote: > On 8/26/24 08:12, Guenter Roeck wrote: >> On 8/26/24 04:18, Mika Westerberg wrote: >>> Hi, >>> >>> On Mon, Aug 26, 2024 at 12:53:01AM -0700, Oleksandr Ocheretnyi wrote: >>>> Commit da23b6faa8bf ("watchdog: iTCO: Add support for Cannon Lake >>>> PCH iTCO") does not ignore NMI_NOW bit on write operation to TCO1_CNT >>>> register what causes unexpected NMIs due to NMI_NOW bit inversion >>>> during regular crashkernel's workflow with following logs: >>>> >>>> iTCO_vendor_support: vendor-support=0 >>>> iTCO_wdt iTCO_wdt: unable to reset NO_REBOOT flag, device >>>> disabled by hardware/BIOS >>>> >>>> This change clears NMI_NOW bit in the TCO1_CNT register to have no >>>> effect on NMI_NOW bit inversion what can cause NMI immediately. >>>> >>>> Fixes: da23b6faa8bf ("watchdog: iTCO: Add support for Cannon Lake PCH iTCO") >>>> Signed-off-by: Oleksandr Ocheretnyi <oocheret@cisco.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 264857d314da..679c115ef7d3 100644 >>>> --- a/drivers/watchdog/iTCO_wdt.c >>>> +++ b/drivers/watchdog/iTCO_wdt.c >>>> @@ -224,7 +224,7 @@ static int update_no_reboot_bit_cnt(void *priv, bool set) >>>> val |= BIT(0); >>>> else >>>> val &= ~BIT(0); >>>> - outw(val, TCO1_CNT(p)); >>>> + outw(val & ~BIT(8), TCO1_CNT(p)); >>> >>> I suggest adding some #define for the magical number 8 so that it is >>> easier for anyone looking at this driver to figure out what it is doing. >>> >>> Otherwise looks good to me, thanks! >>> >> >> Not really; it appears to be hiding a change in code which is supposed to do >> something different (it is supposed to set or clear the no_reboot bit), >> with no explanation whatsoever. That warrants a comment in the code. >> Also, I would prefer >> val = inw(TCO1_CNT(p)) & ~BIT(8); >> > > On top of that, I fail to understand "on register comparison" in the subject. > What register comparison ? > Sorry, one more: The comment will need to explain why clearing bit 8 is needed here but not for any other writes to TCO1_CNT. Obviously this isn't just "ignore bit on write" but something more. Guenter
On 9/12/24 07:19, Oleksandr Ocheretnyi wrote: > Commit da23b6faa8bf ("watchdog: iTCO: Add support for Cannon Lake > PCH iTCO") does not mask NMI_NOW bit during TCO1_CNT register's > value comparison for update_no_reboot_bit() call causing following > failure: > > ... > iTCO_vendor_support: vendor-support=0 > iTCO_wdt iTCO_wdt: unable to reset NO_REBOOT flag, device > disabled by hardware/BIOS > ... > > and this can lead to unexpected NMIs later during regular > crashkernel's workflow because of watchdog probe call failures. > > This change masks NMI_NOW bit for TCO1_CNT register values to > avoid unexpected NMI_NOW bit inversions. > > Fixes: da23b6faa8bf ("watchdog: iTCO: Add support for Cannon Lake PCH iTCO") > Signed-off-by: Oleksandr Ocheretnyi <oocheret@cisco.com> > --- > drivers/watchdog/iTCO_wdt.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c > index 264857d314da..46c09359588f 100644 > --- a/drivers/watchdog/iTCO_wdt.c > +++ b/drivers/watchdog/iTCO_wdt.c > @@ -82,6 +82,12 @@ > #define TCO2_CNT(p) (TCOBASE(p) + 0x0a) /* TCO2 Control Register */ > #define TCOv2_TMR(p) (TCOBASE(p) + 0x12) /* TCOv2 Timer Initial Value*/ > > +/* NMI_NOW is bit 8 of TCO1_CNT register > + * Read/Write > + * This bit is implemented as RW but has no effect on HW. > + */ > +#define NMI_NOW BIT(8) > + > /* internal variables */ > struct iTCO_wdt_private { > struct watchdog_device wddev; > @@ -217,15 +223,24 @@ static int update_no_reboot_bit_mem(void *priv, bool set) > static int update_no_reboot_bit_cnt(void *priv, bool set) > { > struct iTCO_wdt_private *p = priv; > - u16 val, newval; > - > - val = inw(TCO1_CNT(p)); > + u16 val, newval, mask = (~NMI_NOW); > + Unnecessary (). Either case, please just mask against ~NMI_NOW directly. The mask variable is not necessary. > + /* writing back 1b1 to NMI_NOW of TCO1_CNT register Standard multi-line comments, please. Thanks, Guenter > + * causes NMI_NOW bit inversion what consequently does > + * not allow to perform the register's value comparison > + * properly. > + * > + * NMI_NOW bit masking for TCO1_CNT register values > + * helps to avoid possible NMI_NOW bit inversions on > + * following write operation. > + */ > + val = inw(TCO1_CNT(p)) & mask; > if (set) > val |= BIT(0); > else > val &= ~BIT(0); > outw(val, TCO1_CNT(p)); > - newval = inw(TCO1_CNT(p)); > + newval = inw(TCO1_CNT(p)) & mask; > > /* make sure the update is successful */ > return val != newval ? -EIO : 0;
On 9/12/24 07:19, Oleksandr Ocheretnyi wrote: > Commit da23b6faa8bf ("watchdog: iTCO: Add support for Cannon Lake > PCH iTCO") does not mask NMI_NOW bit during TCO1_CNT register's > value comparison for update_no_reboot_bit() call causing following > failure: > > ... > iTCO_vendor_support: vendor-support=0 > iTCO_wdt iTCO_wdt: unable to reset NO_REBOOT flag, device > disabled by hardware/BIOS > ... > > and this can lead to unexpected NMIs later during regular > crashkernel's workflow because of watchdog probe call failures. > > This change masks NMI_NOW bit for TCO1_CNT register values to > avoid unexpected NMI_NOW bit inversions. > > Fixes: da23b6faa8bf ("watchdog: iTCO: Add support for Cannon Lake PCH iTCO") > Signed-off-by: Oleksandr Ocheretnyi <oocheret@cisco.com> > --- Oh, and change log goes here. Guenter
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c index 264857d314da..679c115ef7d3 100644 --- a/drivers/watchdog/iTCO_wdt.c +++ b/drivers/watchdog/iTCO_wdt.c @@ -224,7 +224,7 @@ static int update_no_reboot_bit_cnt(void *priv, bool set) val |= BIT(0); else val &= ~BIT(0); - outw(val, TCO1_CNT(p)); + outw(val & ~BIT(8), TCO1_CNT(p)); newval = inw(TCO1_CNT(p)); /* make sure the update is successful */
Commit da23b6faa8bf ("watchdog: iTCO: Add support for Cannon Lake PCH iTCO") does not ignore NMI_NOW bit on write operation to TCO1_CNT register what causes unexpected NMIs due to NMI_NOW bit inversion during regular crashkernel's workflow with following logs: iTCO_vendor_support: vendor-support=0 iTCO_wdt iTCO_wdt: unable to reset NO_REBOOT flag, device disabled by hardware/BIOS This change clears NMI_NOW bit in the TCO1_CNT register to have no effect on NMI_NOW bit inversion what can cause NMI immediately. Fixes: da23b6faa8bf ("watchdog: iTCO: Add support for Cannon Lake PCH iTCO") Signed-off-by: Oleksandr Ocheretnyi <oocheret@cisco.com> --- drivers/watchdog/iTCO_wdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)