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

Hi,

Thanks for the comments.

On Mon, 22 Jun 2026 at 11:42, <[email protected]> wrote:
>
> 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 <[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.
>
> > 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?
No, the INTCR2 is accessed only by the WD driver, and reads occur only
during the WD probe.
>
> 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.
the INTCR2 is not contains Write-1-to-Clear (W1C) status bits.
>
> Could regmap_update_bits() or a W1C-safe write be used here instead?
It is not needed here.
>
> > +                     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.
Will be change in V3
>
> > +             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

Thanks,

Tomer