* Re: [PATCH v2] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call
2024-09-12 14:19 ` [PATCH v2] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call Oleksandr Ocheretnyi
@ 2024-09-12 21:15 ` Guenter Roeck
2024-09-12 21:15 ` Guenter Roeck
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Guenter Roeck @ 2024-09-12 21:15 UTC (permalink / raw)
To: Oleksandr Ocheretnyi
Cc: jdelvare, linux-kernel, linux-watchdog, mika.westerberg, wim, wsa,
xe-linux-external
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;
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call
2024-09-12 14:19 ` [PATCH v2] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call Oleksandr Ocheretnyi
2024-09-12 21:15 ` Guenter Roeck
@ 2024-09-12 21:15 ` Guenter Roeck
2024-09-13 19:14 ` [PATCH v3] " Oleksandr Ocheretnyi
2024-09-13 5:29 ` [PATCH v2] " kernel test robot
2024-09-13 7:58 ` kernel test robot
3 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2024-09-12 21:15 UTC (permalink / raw)
To: Oleksandr Ocheretnyi
Cc: jdelvare, linux-kernel, linux-watchdog, mika.westerberg, wim, wsa,
xe-linux-external
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v3] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call
2024-09-12 21:15 ` Guenter Roeck
@ 2024-09-13 19:14 ` Oleksandr Ocheretnyi
2024-09-17 1:09 ` Guenter Roeck
0 siblings, 1 reply; 14+ messages in thread
From: Oleksandr Ocheretnyi @ 2024-09-13 19:14 UTC (permalink / raw)
To: linux
Cc: jdelvare, linux-kernel, linux-watchdog, mika.westerberg, oocheret,
wim, wsa, xe-linux-external
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>
---
V2 -> V3: Removed not necessary variable 'mask', updated multi-line comments
V1 -> V2: Provided comments and macro define with bit description
drivers/watchdog/iTCO_wdt.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
index 264857d314da..dd297dcd524c 100644
--- a/drivers/watchdog/iTCO_wdt.c
+++ b/drivers/watchdog/iTCO_wdt.c
@@ -82,6 +82,13 @@
#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;
@@ -219,13 +226,23 @@ 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));
+ /*
+ * writing back 1b1 to NMI_NOW of TCO1_CNT register
+ * 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)) & ~NMI_NOW;
if (set)
val |= BIT(0);
else
val &= ~BIT(0);
outw(val, TCO1_CNT(p));
- newval = inw(TCO1_CNT(p));
+ newval = inw(TCO1_CNT(p)) & ~NMI_NOW;
/* make sure the update is successful */
return val != newval ? -EIO : 0;
--
2.39.3
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v3] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call
2024-09-13 19:14 ` [PATCH v3] " Oleksandr Ocheretnyi
@ 2024-09-17 1:09 ` Guenter Roeck
2024-10-07 15:57 ` Oleksandr Ocheretnyi
0 siblings, 1 reply; 14+ messages in thread
From: Guenter Roeck @ 2024-09-17 1:09 UTC (permalink / raw)
To: Oleksandr Ocheretnyi
Cc: jdelvare, linux-kernel, linux-watchdog, mika.westerberg, wim, wsa,
xe-linux-external
On 9/13/24 12:14, 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>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> V2 -> V3: Removed not necessary variable 'mask', updated multi-line comments
> V1 -> V2: Provided comments and macro define with bit description
>
> drivers/watchdog/iTCO_wdt.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/watchdog/iTCO_wdt.c b/drivers/watchdog/iTCO_wdt.c
> index 264857d314da..dd297dcd524c 100644
> --- a/drivers/watchdog/iTCO_wdt.c
> +++ b/drivers/watchdog/iTCO_wdt.c
> @@ -82,6 +82,13 @@
> #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;
> @@ -219,13 +226,23 @@ 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));
> + /*
> + * writing back 1b1 to NMI_NOW of TCO1_CNT register
> + * 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)) & ~NMI_NOW;
> if (set)
> val |= BIT(0);
> else
> val &= ~BIT(0);
> outw(val, TCO1_CNT(p));
> - newval = inw(TCO1_CNT(p));
> + newval = inw(TCO1_CNT(p)) & ~NMI_NOW;
>
> /* make sure the update is successful */
> return val != newval ? -EIO : 0;
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH v3] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call
2024-09-17 1:09 ` Guenter Roeck
@ 2024-10-07 15:57 ` Oleksandr Ocheretnyi
2024-10-08 16:33 ` Mika Westerberg
0 siblings, 1 reply; 14+ messages in thread
From: Oleksandr Ocheretnyi @ 2024-10-07 15:57 UTC (permalink / raw)
To: mika.westerberg
Cc: linux, jdelvare, linux-kernel, linux-watchdog, wim, wsa,
xe-linux-external
Hello Mika,
> 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.
Are the changes with #define NMI_NOW bit fine for you?
Best regards,
Oleksandr
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call
2024-10-07 15:57 ` Oleksandr Ocheretnyi
@ 2024-10-08 16:33 ` Mika Westerberg
0 siblings, 0 replies; 14+ messages in thread
From: Mika Westerberg @ 2024-10-08 16:33 UTC (permalink / raw)
To: Oleksandr Ocheretnyi
Cc: linux, jdelvare, linux-kernel, linux-watchdog, wim, wsa,
xe-linux-external
On Mon, Oct 07, 2024 at 08:57:02AM -0700, Oleksandr Ocheretnyi wrote:
> Hello Mika,
>
> > 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.
>
> Are the changes with #define NMI_NOW bit fine for you?
Sure,
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call
2024-09-12 14:19 ` [PATCH v2] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call Oleksandr Ocheretnyi
2024-09-12 21:15 ` Guenter Roeck
2024-09-12 21:15 ` Guenter Roeck
@ 2024-09-13 5:29 ` kernel test robot
2024-09-13 7:58 ` kernel test robot
3 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2024-09-13 5:29 UTC (permalink / raw)
To: Oleksandr Ocheretnyi, linux
Cc: oe-kbuild-all, jdelvare, linux-kernel, linux-watchdog,
mika.westerberg, oocheret, wim, wsa, xe-linux-external
Hi Oleksandr,
kernel test robot noticed the following build warnings:
[auto build test WARNING on westeri-thunderbolt/next]
[also build test WARNING on linus/master v6.11-rc7 next-20240912]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Oleksandr-Ocheretnyi/iTCO_wdt-mask-NMI_NOW-bit-for-update_no_reboot_bit-call/20240912-222231
base: https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git next
patch link: https://lore.kernel.org/r/20240912141931.2447826-1-oocheret%40cisco.com
patch subject: [PATCH v2] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call
config: i386-buildonly-randconfig-003-20240913 (https://download.01.org/0day-ci/archive/20240913/202409131302.oGABipcM-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240913/202409131302.oGABipcM-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409131302.oGABipcM-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/watchdog/iTCO_wdt.c: In function 'update_no_reboot_bit_cnt':
>> drivers/watchdog/iTCO_wdt.c:226:33: warning: conversion from 'long unsigned int' to 'u16' {aka 'short unsigned int'} changes value from '4294967039' to '65279' [-Woverflow]
226 | u16 val, newval, mask = (~NMI_NOW);
| ^
vim +226 drivers/watchdog/iTCO_wdt.c
222
223 static int update_no_reboot_bit_cnt(void *priv, bool set)
224 {
225 struct iTCO_wdt_private *p = priv;
> 226 u16 val, newval, mask = (~NMI_NOW);
227
228 /* writing back 1b1 to NMI_NOW of TCO1_CNT register
229 * causes NMI_NOW bit inversion what consequently does
230 * not allow to perform the register's value comparison
231 * properly.
232 *
233 * NMI_NOW bit masking for TCO1_CNT register values
234 * helps to avoid possible NMI_NOW bit inversions on
235 * following write operation.
236 */
237 val = inw(TCO1_CNT(p)) & mask;
238 if (set)
239 val |= BIT(0);
240 else
241 val &= ~BIT(0);
242 outw(val, TCO1_CNT(p));
243 newval = inw(TCO1_CNT(p)) & mask;
244
245 /* make sure the update is successful */
246 return val != newval ? -EIO : 0;
247 }
248
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call
2024-09-12 14:19 ` [PATCH v2] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call Oleksandr Ocheretnyi
` (2 preceding siblings ...)
2024-09-13 5:29 ` [PATCH v2] " kernel test robot
@ 2024-09-13 7:58 ` kernel test robot
3 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2024-09-13 7:58 UTC (permalink / raw)
To: Oleksandr Ocheretnyi, linux
Cc: llvm, oe-kbuild-all, jdelvare, linux-kernel, linux-watchdog,
mika.westerberg, oocheret, wim, wsa, xe-linux-external
Hi Oleksandr,
kernel test robot noticed the following build warnings:
[auto build test WARNING on westeri-thunderbolt/next]
[also build test WARNING on linus/master v6.11-rc7 next-20240912]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Oleksandr-Ocheretnyi/iTCO_wdt-mask-NMI_NOW-bit-for-update_no_reboot_bit-call/20240912-222231
base: https://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt.git next
patch link: https://lore.kernel.org/r/20240912141931.2447826-1-oocheret%40cisco.com
patch subject: [PATCH v2] iTCO_wdt: mask NMI_NOW bit for update_no_reboot_bit() call
config: i386-buildonly-randconfig-001-20240913 (https://download.01.org/0day-ci/archive/20240913/202409131548.X0MGdN1r-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240913/202409131548.X0MGdN1r-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409131548.X0MGdN1r-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/watchdog/iTCO_wdt.c:226:27: warning: implicit conversion from 'unsigned long' to 'u16' (aka 'unsigned short') changes value from 4294967039 to 65279 [-Wconstant-conversion]
226 | u16 val, newval, mask = (~NMI_NOW);
| ~~~~ ^~~~~~~~
1 warning generated.
vim +226 drivers/watchdog/iTCO_wdt.c
222
223 static int update_no_reboot_bit_cnt(void *priv, bool set)
224 {
225 struct iTCO_wdt_private *p = priv;
> 226 u16 val, newval, mask = (~NMI_NOW);
227
228 /* writing back 1b1 to NMI_NOW of TCO1_CNT register
229 * causes NMI_NOW bit inversion what consequently does
230 * not allow to perform the register's value comparison
231 * properly.
232 *
233 * NMI_NOW bit masking for TCO1_CNT register values
234 * helps to avoid possible NMI_NOW bit inversions on
235 * following write operation.
236 */
237 val = inw(TCO1_CNT(p)) & mask;
238 if (set)
239 val |= BIT(0);
240 else
241 val &= ~BIT(0);
242 outw(val, TCO1_CNT(p));
243 newval = inw(TCO1_CNT(p)) & mask;
244
245 /* make sure the update is successful */
246 return val != newval ? -EIO : 0;
247 }
248
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread