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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The first watchdog instance on NPCM750 clears the global RESSR register without caching it, causing subsequent watchdog instances to permanently lose their reset status.
- [High] Non-atomic read-modify-write of the shared INTCR2 register clobbers concurrent modifications and incorrectly handles W1C semantics.
--

commit dafe1267618ef0158376caf5f3acee9d7fe7b1b7
Author: Tomer Maimon <tmaimon77@gmail.com>

watchdog: npcm: add bootstatus support

The NPCM750 uses RESSR and the NPCM845 uses INTCR2 to latch reset
indications. Read those bits during probe and map them into watchdog
bootstatus flags.

> diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c
> index e62ea054bc618..98660419ec3f0 100644
> --- a/drivers/watchdog/npcm_wdt.c
> +++ b/drivers/watchdog/npcm_wdt.c
[ ... ]
> @@ -169,6 +250,92 @@ static bool npcm_is_running(struct watchdog_device *wdd)
>  	return readl(wdt->reg) & NPCM_WTE;
>  }
>  
> +static void npcm_get_reset_status(struct npcm_wdt *wdt, struct device *dev,
> +				  const struct npcm_wdt_data *data,
> +				  resource_size_t start)
> +{
[ ... ]
> +	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 & NPCM8XX_RST) {
> +			ret = regmap_write(gcr_regmap, NPCM_INTCR2_OFFSET,
> +					   rstval & ~NPCM8XX_RST);

[Severity: High]
Does this non-atomic read-modify-write risk clobbering concurrent modifications
to the shared INTCR2 register?

Since INTCR2 is a shared global control register accessed by multiple drivers,
using regmap_read() followed by regmap_write() drops the regmap lock between
operations. Any concurrent modifications to INTCR2 by other drivers could be
overwritten.

Also, if INTCR2 contains Write-1-to-Clear (W1C) status bits, writing
`rstval & ~NPCM8XX_RST` might incorrectly write 1s to all other active bits
(clearing them) and 0s to the watchdog bits.

Could regmap_update_bits() or a W1C-safe write be used here instead?

> +			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;
> +		}
> +
> +		ret = regmap_write(gcr_regmap, NPCM_RESSR_OFFSET, rstval);

[Severity: High]
Will the first watchdog instance on NPCM750 clear the global RESSR register
without caching it, causing subsequent instances to lose their reset status?

The NPCM750 SoC has three watchdog instances that share the global RESSR
register. When the first watchdog probes, it reads RESSR and immediately
clears it by writing the value back. Because the value is not cached
(unlike the SCRPAD10 mechanism used for NPCM845), when the remaining
watchdogs probe, they will read 0 from RESSR and permanently lose their
boot status indications.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622083046.3189603-1-tmaimon77@gmail.com?part=3