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

Hi,

On Sun, 5 Jul 2026 at 17:10, <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
>
> New issues:
> - [High] Non-atomic read-modify-write on shared syscon registers corrupts concurrent updates or incorrectly clears W1C bits.
Not needed, only the WD using this register.
> - [High] The driver hijacks standardized watchdog bootstatus flags to report unrelated SoC reset reasons.
Approved by the maintainer.
> - [Low] The driver hardcodes physical MMIO base addresses to identify driver instances.
>
> Pre-existing issues:
> - [High] The driver leaks a clock enable reference on every timeout change and fails to clean up on probe errors.
> --
>
> commit 3d82c9ef067a4770a83258e13cbb0ed566d4998b
> Author: Tomer Maimon <[email protected]>
>
> 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. For
> NPCM845, cache the sampled INTCR2 state in SCRPAD10 after the reset status
> bits are cleared.
>
> > 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_rst_map npcm750_rst_map[] = {
> > +     { 0xf000801c, NPCM_WD0RST },
> > +     { 0xf000901c, NPCM_WD1RST },
> > +     { 0xf000a01c, NPCM_WD2RST },
> > +};
>
> [Severity: Low]
> Does this code hardcode physical MMIO base addresses to identify driver
> instances?
It is hardcoded.
>
> Matching the resource start address against hardcoded values in
> npcm_get_reset_status() can be brittle if the memory map changes in future
> SoC revisions or alternative device tree mappings. Is there a way to use
> device tree aliases or instance IDs to identify the hardware block instead?
>
> > +
> > +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 mapping hijack standardized watchdog bootstatus flags to report
> unrelated SoC reset reasons?
>
> Translating NPCM SoC-specific reset reasons into standard UAPI flags might
> cause issues in userspace. For example, WDIOF_OVERHEAT and WDIOF_FANFAULT
> are strictly defined in the UAPI to indicate thermal overheating and fan
> failures.
>
> Could system monitoring tools misinterpret a normal cold boot (NPCM_CORST)
> as a severe hardware failure and trigger unintended alerts or shutdowns?
>
> [ ... ]
>
> > +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 & NPCM_RST) {
> > +                     ret = regmap_write(gcr_regmap, NPCM_INTCR2_OFFSET,
> > +                                        rstval & ~NPCM_RST);
>
> [Severity: High]
> Can this non-atomic read-modify-write sequence on the shared syscon
> register corrupt concurrent updates?
>
> If INTCR2 (and RESSR below) are standard read-write registers, this
> sequence could silently overwrite modifications made by other drivers
> sharing the syscon.
>
> Alternatively, if these are Write-1-to-Clear (W1C) registers, writing
> rstval & ~NPCM_RST will write 1s to all other active status bits,
> inadvertently clearing them, while writing 0 to the watchdog bits,
> failing to clear the intended reset flags.
>
> Would it be safer to use regmap_update_bits() for atomic updates, or if
> W1C, write only the target bits without a read-modify-write?
>
> [ ... ]
>
> > @@ -220,6 +430,8 @@ static int npcm_wdt_probe(struct platform_device *pdev)
> >       /* Ensure timeout is able to be represented by the hardware */
> >       npcm_wdt_set_timeout(&wdt->wdd, wdt->wdd.timeout);
> >
> > +     npcm_get_reset_status(wdt, dev, data, start);
> > +
> >       if (npcm_is_running(&wdt->wdd)) {
> >               /* Restart with the default or device-tree specified timeout */
> >               npcm_wdt_start(&wdt->wdd);
> >               set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> >       }
> >
> >       ret = devm_request_irq(dev, irq, npcm_wdt_interrupt, 0, "watchdog",
> >                              wdt);
> >       if (ret)
> >               return ret;
>
> [Severity: High]
> This is a pre-existing issue, but does this code leak a clock enable
> reference?
>
> In npcm_wdt_probe(), if the hardware is running, npcm_wdt_start() is called:
>
> npcm_wdt_start()
>     clk_prepare_enable(wdt->clk);
>
> If subsequent probe steps fail (e.g., devm_request_irq() returns an error),
> probe returns without unwinding the clock enable.
>
> Additionally, npcm_wdt_set_timeout() calls npcm_wdt_start() when the watchdog
> is active. Since there is no matching disable on timeout change, every
> WDIOC_SETTIMEOUT ioctl increments the clock's refcount. This prevents
> the clock from ever being disabled when the watchdog is stopped, leading
> to a resource leak.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3

Thanks,

Tomer