Re: [PATCH v12 2/6] mfd: add NXP MC33978/MC34978 core driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Sustained hardware faults cause a 100% CPU lockup in the event handling loop.
- [Medium] Transient fault detection logic is dead code due to pipelined SPI dummy fetch filtering.
--

commit a9e570c4deed793a38c78aa7e88c2bfc68e7b1cd
Author: Oleksij Rempel <o.rempel@pengutronix.de>

    mfd: add NXP MC33978/MC34978 core driver

Add core Multi-Function Device (MFD) driver for the NXP MC33978 and MC34978
Multiple Switch Detection Interfaces (MSDI).

> diff --git a/drivers/platform/misc/mc33978-core.c b/drivers/platform/misc/mc33978-core.c
> index 0000000000000..287b524aed98a
> --- /dev/null
> +++ b/drivers/platform/misc/mc33978-core.c

[ ... ]

> 	/*
> 	 * Transient fault detection (§9.10.27): FAULT_STAT latches on fault
> 	 * occurrence and persists until Fault register (0x42) read clears it.
> 	 * Hwmon driver reads Fault register, which triggers regmap SPI read with
> 	 * pipelined frames: REQ frame (old response, FAULT_STAT=1 latched) and
> 	 * PIPE frame (Fault register response clears latch). If fault condition
> 	 * cleared between latch and read, FAULT_STAT won't re-latch (datasheet:
> 	 * "immediately set again if condition still present").
> 	 *
> 	 * Transient detection: hw_flags contains harvested FAULT_STAT bit
> 	 * (latched evidence from REQ frame), but bus_fault_active=0 and
> 	 * cached_fault_active=0 (PIPE frame showed cleared, condition gone).
> 	 * This proves brief fault (VBATP voltage glitch, threshold oscillation)
> 	 * occurred and cleared before Fault register read. Edge-triggered
> 	 * consumers need this event notification.
> 	 *
> 	 * WAKE_BIT exclusion: WAKE_BIT is synthetic (bus_fault_active changes),
> 	 * not hardware evidence. Including it causes phantom transients when
> 	 * normal faults deassert (WAKE_BIT present but no actual transient).
> 	 */
> 	transient = !changed && !fault_active && !cached_fault &&
> 		    (hw_flags & MC33978_HI_BYTE_STAT_FAULT);

Is it possible that the transient fault detection logic is never executed?

Since mc33978_prepare_messages() hardcodes the dummy fetch for the PIPE frame
to be MC33978_REG_CHECK, the REQ frame in the next SPI read will contain the
response to REG_CHECK.

However, mc33978_rx_decode() explicitly sets has_status = false for
MC33978_REG_CHECK and returns -ENODATA.

This skips status harvesting for the REQ frame in mc33978_spi_read(), meaning
the latched FAULT_STAT evidence might never be harvested.

[ ... ]

> static void mc33978_process_single_event(struct mc33978_core_data *cdata)
> {
> 	unsigned int harvested;
> 	unsigned int pin_state;
> 	int ret;
> 
> 	/*
> 	 * Grab harvested_flags BEFORE hardware read. Flags harvested during
> 	 * the read trigger another loop pass. This intentionally forces a
> 	 * redundant SPI read on most interrupts, but is necessary to prevent
> 	 * lost events when concurrent regmap access races with IRQ thread
> 	 * (hardware has clear-on-read INT_flg).
> 	 */
> 	harvested = atomic_xchg(&cdata->harvested_flags, 0);
> 
> 	ret = regmap_read(cdata->map, MC33978_REG_READ_IN, &pin_state);
> 	if (ret)
> 		dev_err_ratelimited(&cdata->spi->dev, "failed to read pin state: %d\n",
> 				    ret);
> 	else
> 		mc33978_handle_pin_changes(cdata, pin_state);
> 
> 	mc33978_handle_fault_condition(cdata, harvested);
> }
> 
> static void mc33978_handle_events(struct mc33978_core_data *cdata)
> {
> 	guard(mutex)(&cdata->event_lock);
> 
> 	do {
> 		mc33978_process_single_event(cdata);
> 	} while (atomic_read(&cdata->harvested_flags) != 0);

Can a sustained hardware fault cause a soft lockup in this event loop?

If a persistent physical fault exists, the hardware continuously latches
FAULT_STAT=1. The regmap_read() to MC33978_REG_READ_IN inside
mc33978_process_single_event() triggers mc33978_spi_read(), which then
calls mc33978_harvest_status().

Because mc33978_harvest_status() unconditionally adds
MC33978_HI_BYTE_STAT_FAULT back into cdata->harvested_flags, the
atomic_read(&cdata->harvested_flags) check would perpetually evaluate
to true.

> }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515160537.115808-1-o.rempel@pengutronix.de?part=2