Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider: - [High] The `set_timeout` and `set_pretimeout` operations update software structures but fail to configure hardware registers when active. - [High] Bypassing core IRQ safety checks for shared interrupts with a dummy primary handler, causing severe system stalls. - [High] Hardware system reset is entirely disabled if configured for an alternate event without a pretimeout. - [High] Incorrect bitmask check for IRQ support causes probe failure. - [High] Driver illegally passes a raw hardware IRQ line number to the IRQ subsystem instead of a mapped virtual IRQ. - [Medium] Error paths bypass the IOREG_LOCK command, leaving the EC configuration space permanently unlocked. - [Medium] Accumulating negative error codes with bitwise OR and blindly continuing I/O operations. - [Medium] Endianness bug when reading single-byte hardware registers into an `int` pointer. --
--- Patch [6]: [PATCH v2 6/8] Add Advantech EIO Watchdog driver --- commit a7197d0a8b335e3355964cf7322548e44ad2700e Author: Ramiro Oliveira <[email protected]> Add Advantech EIO Watchdog driver This commit adds the driver to control the Advantech EIO Watchdog block, this block is included in the Advantech EIO Embedded Controller. > diff --git a/drivers/watchdog/eio_wdt.c b/drivers/watchdog/eio_wdt.c > new file mode 100644 > index 0000000000000..b3c036a004b30 > --- /dev/null > +++ b/drivers/watchdog/eio_wdt.c > @@ -0,0 +1,641 @@ [ ... ] > +static int wdt_set_timeout(struct watchdog_device *wdd, unsigned int timeout) > +{ > + wdd->timeout = timeout; > + return 0; > +} [Severity: High] Does this code apply the new timeout to the hardware? It updates the software structure, but it appears to omit updating the hardware registers. When userspace calls WDIOC_SETTIMEOUT, the core updates the structures and pings the watchdog, but without calling wdt_set_config() here, the new timeout might never be applied to the hardware unless explicitly stopped and started. > + > +static int wdt_set_pretimeout(struct watchdog_device *wdd, unsigned int > pretimeout) > +{ > + wdd->pretimeout = pretimeout; > + return 0; > +} [Severity: High] Similarly to the set_timeout callback, does this code omit configuring the hardware registers with the new pretimeout value? [ ... ] > +static int wdt_set_config(struct eio_wdt_dev *eio_wdt) > +{ > + int ret, type; > + u32 event_time = 0; > + u32 reset_time = 0; > + > + if (eio_wdt->event_type > EVENT_PIN) > + return -EFAULT; > + > + /* Calculate event time and reset time */ > + if (eio_wdt->wdd.pretimeout && eio_wdt->wdd.timeout) { > + if (eio_wdt->wdd.timeout < eio_wdt->wdd.pretimeout) > + return -EINVAL; > + > + reset_time = eio_wdt->wdd.timeout; > + event_time = eio_wdt->wdd.timeout - eio_wdt->wdd.pretimeout; > + > + } else if (eio_wdt->wdd.timeout) { > + reset_time = eio_wdt->event_type ? 0 : > eio_wdt->wdd.timeout; > + event_time = eio_wdt->event_type ? eio_wdt->wdd.timeout : 0; > + } [Severity: High] If configured for an alternate event without a pretimeout, does this disable the hardware fallback reset? Setting reset_time to 0 means the hardware reset is entirely disabled, and the driver relies exclusively on the software ISR to call emergency_restart(). If the kernel hangs with interrupts disabled, the ISR will never run, which might leave the system permanently deadlocked. [ ... ] > +static int wdt_get_irq_io(struct eio_wdt_dev *eio_wdt) > +{ > + int ret = 0; > + int idx = EIO_PNP_INDEX; > + int data = EIO_PNP_DATA; > + struct regmap *map = eio_wdt->iomap; > + > + mutex_lock(&eio_wdt->core->mutex); > + > + /* Unlock EC IO port */ > + ret |= regmap_write(map, idx, IOREG_UNLOCK); > + ret |= regmap_write(map, idx, IOREG_UNLOCK); > + > + /* Select logical device to PMC */ > + ret |= regmap_write(map, idx, IOREG_LDN); > + ret |= regmap_write(map, data, IOREG_LDN_PMCIO); > + > + /* Get IRQ number */ > + ret |= regmap_write(map, idx, IOREG_IRQ); > + ret |= regmap_read(map, data, &eio_wdt->irq); [Severity: Medium] Can accumulating the results of regmap_write() and regmap_read() with a bitwise OR cause issues with negative error codes? Furthermore, if a critical step like selecting the logical device fails, this code doesn't appear to halt, proceeding to write to whatever logical device was already selected. > + > + /* Lock back */ > + ret |= regmap_write(map, idx, IOREG_LOCK); > + > + mutex_unlock(&eio_wdt->core->mutex); > + > + return ret ? -EIO : 0; > +} > + > +static int wdt_get_irq_pmc(struct eio_wdt_dev *eio_wdt) > +{ > + return pmc_read(eio_wdt->mfd, REG_IRQ_NUMBER, &eio_wdt->irq); > +} [Severity: Medium] Will this corrupt the integer on big-endian systems? pmc_read() receives a void pointer and, based on the size (1 byte for REG_IRQ_NUMBER), copies a single byte into the lowest memory address of the pointer. Since eio_wdt->irq is an int, a big-endian system would put this byte in the most significant byte location, leaving the integer value corrupted. > + > +static int wdt_get_irq(struct eio_wdt_dev *eio_wdt) > +{ > + int ret; > + > + if (!(eio_wdt->support & BIT(EVENT_IRQ))) [Severity: High] Does this check for the incorrect bitmask? EVENT_IRQ is 2, so this checks BIT(2), but the support flags define SUPPORT_IRQ as BIT(4). This typo could force the initialization to fail if the hardware supports IRQs but doesn't set BIT(2). [ ... ] > +static int wdt_set_irq_io(struct eio_wdt_dev *eio_wdt) > +{ > + int ret = 0; > + int idx = EIO_PNP_INDEX; > + int data = EIO_PNP_DATA; > + struct regmap *map = eio_wdt->iomap; > + > + mutex_lock(&eio_wdt->core->mutex); > + > + /* Unlock EC IO port */ > + ret = regmap_write(map, idx, IOREG_UNLOCK); > + if (ret) > + goto unlock; > + ret = regmap_write(map, idx, IOREG_UNLOCK); > + if (ret) > + goto unlock; [ ... ] > + /* Lock back */ > + ret = regmap_write(map, idx, IOREG_LOCK); > + > +unlock: > + mutex_unlock(&eio_wdt->core->mutex); > + return ret; > +} [Severity: Medium] Does the error path here bypass the IOREG_LOCK command? If any regmap_write() fails after the initial unlock, the jump to unlock only releases the software mutex, skipping the IOREG_LOCK command (0xAA) and leaving the EC IO port open. [ ... ] > +static int wdt_set_irq(struct eio_wdt_dev *eio_wdt) > +{ > + int ret; > + > + if (!(eio_wdt->support & BIT(EVENT_IRQ))) [Severity: High] Similar to wdt_get_irq(), does this check BIT(2) instead of BIT(4)? [ ... ] > +static irqreturn_t wdt_isr(int irq, void *arg) > +{ > + return IRQ_WAKE_THREAD; > +} [Severity: High] Could unconditionally returning IRQ_WAKE_THREAD from a dummy handler cause severe system stalls for shared interrupts? The kernel generally forbids requesting an IRQF_SHARED | IRQF_ONESHOT interrupt without a custom primary handler that checks if its device actually triggered the interrupt. When any device on the shared line fires, this handler will blindly wake the thread and mask the IRQ line for the duration of the slow reads in the threaded handler, which could cripple other devices sharing the line. [ ... ] > + if (eio_wdt->event_type == EVENT_IRQ) { > + ret = devm_request_threaded_irq(dev, eio_wdt->irq, > + wdt_isr, wdt_threaded_isr, > + IRQF_SHARED | IRQF_ONESHOT, > pdev->name, > + eio_wdt); [Severity: High] Does this pass a raw hardware IRQ line number directly to the IRQ subsystem? The driver reads the IRQ number directly from the EC into eio_wdt->irq and passes it here. The Linux IRQ subsystem requires a mapped virtual IRQ (virq). Without mapping it via an irqdomain or ACPI, the raw hardware number is unlikely to trigger correctly. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
