Re: [PATCH v4 3/3] watchdog: npcm: add bootstatus support

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Non-atomic read-modify-write on shared syscon registers (INTCR2/RESSR) introduces a TOCTOU race and mishandles W1C semantics.
- [High] Overloading standard watchdog API boot status flags with generic SoC reset reasons violates UAPI semantics.
--

commit dac8bb6ae40a2614c654ebac31d7674dfeed0f47
Author: Tomer Maimon <[email protected]>

watchdog: npcm: add bootstatus support

This commit adds bootstatus support to the NPCM watchdog driver. It reads
the RESSR and INTCR2 registers via the sysgcr regmap during probe and maps
them into watchdog bootstatus flags.

> diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c
> index e62ea054bc618..82ff3956dcf21 100644
> --- a/drivers/watchdog/npcm_wdt.c
> +++ b/drivers/watchdog/npcm_wdt.c

[ ... ]

> +static const struct npcm_wdt_status_map npcm750_status_map[] = {
> +	{ NPCM_PORST, WDIOF_OVERHEAT },
> +	{ NPCM_CORST, WDIOF_FANFAULT },
> +	{ NPCM_SWR1RST, WDIOF_EXTERN1 },
> +	{ NPCM_SWR2RST, WDIOF_EXTERN2 },
> +	{ NPCM_SWR3RST, WDIOF_POWERUNDER },
> +	{ NPCM_SWR4RST, WDIOF_POWEROVER },
> +};

[Severity: High]
Does this map standard SoC resets to specific watchdog error conditions?

If NPCM_PORST (Power-On Reset) triggers WDIOF_OVERHEAT, and NPCM_CORST
(Core Reset) triggers WDIOF_FANFAULT, won't userspace daemons receive
fabricated hardware failure events on normal cold boots or warm reboots?

[ ... ]

> +static const struct npcm_wdt_status_map npcm845_status_map[] = {
> +	{ NPCM_PORST, WDIOF_OVERHEAT },
> +	{ NPCM_CORST, WDIOF_FANFAULT },
> +	{ NPCM_SWR1RST, WDIOF_EXTERN1 },
> +	{ NPCM_SWR2RST, WDIOF_EXTERN2 },
> +	{ NPCM_SWR3RST, WDIOF_POWERUNDER },
> +	{ NPCM8XX_TIP_RESET, WDIOF_POWEROVER },
> +};

[Severity: High]
Are these reset-to-status mappings similarly overloading the standard
watchdog API boot status flags for the NPCM845?

[ ... ]

> +	if (of_device_is_compatible(dev->of_node, "nuvoton,npcm845-wdt")) {
> +		ret = regmap_read(gcr_regmap, NPCM_INTCR2_OFFSET, &rstval);
> +		if (ret) {
> +			dev_warn(dev, "Failed to read INTCR2 reset status: %d\n",
> +				 ret);
> +			return;
> +		}
> +
> +		if (rstval & NPCM_RST) {
> +			ret = regmap_write(gcr_regmap, NPCM_INTCR2_OFFSET,
> +					   rstval & ~NPCM_RST);

[Severity: High]
Is it safe to use a non-atomic read-modify-write on this shared syscon
register?

If INTCR2 is a Read-Write register, this regmap_read() followed by
regmap_write() introduces a time-of-check to time-of-use race that could
overwrite and lose concurrent modifications made by other drivers sharing this
sysgcr regmap.

Alternatively, if the reset bits are Write-1-to-Clear (W1C), writing
rstval & ~NPCM_RST writes 1s to all OTHER active status bits. This would
unintentionally clear them while writing 0 to the target watchdog bit, failing
to clear it.

> +			if (ret) {
> +				dev_warn(dev,
> +					 "Failed to clear INTCR2 reset status: %d\n",
> +					 ret);
> +				return;
> +			}

[ ... ]

> +	} else if (of_device_is_compatible(dev->of_node, "nuvoton,npcm750-wdt")) {
> +		ret = regmap_read(gcr_regmap, NPCM_RESSR_OFFSET, &rstval);
> +		if (ret) {
> +			dev_warn(dev, "Failed to read RESSR reset status: %d\n",
> +				 ret);
> +			return;
> +		}
> +
> +		if (rstval & NPCM_RST) {
> +			ret = regmap_write(gcr_regmap, NPCM_RESSR_OFFSET,
> +					   rstval & ~NPCM_RST);

[Severity: High]
Does this write to NPCM_RESSR_OFFSET have the same non-atomic
read-modify-write or W1C clearing issues as INTCR2 above?

> +			if (ret) {
> +				dev_warn(dev, "Failed to clear RESSR reset status: %d\n", ret);
> +				return;
> +			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3