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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Stale POR status flags are not cleared before enabling event processing, causing a spurious hardware fault interrupt.
- [High] The pipelined SPI read message forcefully toggles chip select without guaranteeing a minimum CS inactive time.
- [Low] The validation check for the IRQ domain size contains an off-by-one error.
--

commit cbdb6fa84cbdc2a33e2c2e0cdc333ff4eef528f4
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). This driver provides
an IRQ domain, event processing, and interfaces for child devices.

> diff --git a/drivers/mfd/mc33978.c b/drivers/mfd/mc33978.c
> index 0000000000000..d46b76ca91473
> --- /dev/null
> +++ b/drivers/mfd/mc33978.c
[ ... ]
> +static int mc33978_mfd_probe(struct spi_device *spi)
> +{
[ ... ]
> +	ret = mc33978_core_init(dev, spi, &ddata->domain);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Validate IRQ domain size before passing to child devices.
> +	 * hwirq_max is inclusive, we need MC33978_NUM_IRQS entries (0..22).
> +	 */
> +	if (ddata->domain->hwirq_max < MC33978_NUM_IRQS - 1)

Is this bounds check correct?

For a linear IRQ domain, domain->hwirq_max represents the exclusive upper
bound (the size of the domain). Since the driver requires 23 hwirqs (0
through 22, where 22 is MC33978_HWIRQ_FAULT), hwirq_max must be at least 23.

Checking if hwirq_max < 22 allows a domain size of 22 to pass the validation,
which only supports hwirqs 0-21 and would fail when requesting hwirq 22.

Should this be checked as < MC33978_NUM_IRQS instead?

[ ... ]
> diff --git a/drivers/platform/misc/mc33978-core.c b/drivers/platform/misc/mc33978-core.c
> index 0000000000000..b5b3388b270bd
> --- /dev/null
> +++ b/drivers/platform/misc/mc33978-core.c
[ ... ]
> +static void mc33978_prepare_messages(struct mc33978_core_data *cdata)
> +{
[ ... ]
> +	/* --- Prepare Read Message (2 Frames) --- */
> +	spi_message_init(&cdata->msg_read);
> +
> +	/* Frame 1: Request */
> +	cdata->xfer_read[MC33978_FRAME_REQ].tx_buf =
> +		cdata->tx_frame[MC33978_FRAME_REQ];
> +	cdata->xfer_read[MC33978_FRAME_REQ].rx_buf =
> +		cdata->rx_frame[MC33978_FRAME_REQ];
> +	cdata->xfer_read[MC33978_FRAME_REQ].len = MC33978_FRAME_LEN;
> +	/* Latch command */
> +	cdata->xfer_read[MC33978_FRAME_REQ].cs_change = 1;

Does this transfer need an explicit cs_inactive delay?

Setting cs_change to 1 without populating the cs_inactive delay structure
can cause the SPI core to toggle the Chip Select line too quickly on modern
hardware-accelerated controllers. This might violate the MC33978's minimum CS
inactive time requirements for latching the command, causing a communication
regression.

[ ... ]
> +int mc33978_core_init(struct device *dev, struct spi_device *spi,
> +		      struct irq_domain **domain_out)
> +{
[ ... ]
> +	/*
> +	 * Prime the cached pin state under lock to prevent spurious events.
> +	 * Work scheduling is disabled (irq_ready=false) to prevent the work
> +	 * feedback loop that would occur during init: regmap_read() harvests
> +	 * status -> schedules work -> work does regmap_read() -> schedules more
> +	 * work -> infinite loop on single-core systems where work monopolizes
> +	 * CPU before init can complete.
> +	 */
> +	scoped_guard(mutex, &cdata->event_lock) {
> +		ret = regmap_read(cdata->map, MC33978_REG_READ_IN,
> +				  &cdata->cached_pin_state);
> +	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to read initial pin state\n");
> +
> +	if (spi->irq <= 0)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "no valid IRQ provided for INT_B pin\n");
> +
> +	/*
> +	 * Not using IRQF_SHARED: threaded handler with IRQF_ONESHOT may hold
> +	 * line masked too long on slow SPI, making shared operation impractical.
> +	 */
> +	ret = devm_request_threaded_irq(dev, spi->irq,
> +					NULL,
> +					mc33978_irq_thread,
> +					IRQF_ONESHOT,
> +					dev_name(dev), cdata);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to request IRQ\n");
> +
> +	/*
> +	 * Enable work scheduling now that IRQ handler is registered.
> +	 * This prevents the work feedback loop during initialization while
> +	 * allowing proper event processing after setup completes.
> +	 */
> +	scoped_guard(spinlock_irqsave, &cdata->state_lock)
> +		cdata->irq_ready = true;

Could this cause a regression by triggering a spurious hardware fault
interrupt?

During initialization, regmap accesses (like reading MC33978_REG_READ_IN above)
harvest the initial Power-On Reset status flags into cdata->harvested_flags.
Since irq_ready is false, the event worker is correctly prevented from running.

However, the stale harvested flags are not cleared before irq_ready is set to
true. When a child driver makes its first regmap read, mc33978_harvest_status()
will run, see irq_ready == true with a non-zero harvested_flags, and schedule
the event_work.

The worker will then process the stale FAULT_STAT bit, see that the live bus
fault is false, and incorrectly treat it as a transient fault, dispatching a
spurious interrupt to the child driver. 

Should atomic_set(&cdata->harvested_flags, 0) be called right before setting
irq_ready to true?

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