Re: [PATCH v3 2/7] mfd: add NXP MC33978/MC34978 core driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [High] Out-of-bounds stack read and big-endian logic bug due to casting `u32` pointer to `unsigned long *`.
- [High] Missing check for valid virtual IRQ before calling `handle_nested_irq()` can cause a NULL pointer dereference.
- [High] Missing `cancel_work_sync()` for `event_work` in the driver cleanup path leads to a Use-After-Free on unbind or probe failure.
- [High] An SPI communication failure causes an infinite loop in the event handling thread, leading to a CPU soft lockup.
- [High] Setting IRQ_DOMAIN_FLAG_HIERARCHY breaks MFD child IRQ resolution, causing the hwmon driver to receive an invalid IRQ.
- [High] Missing memory barriers in the lockless event scheduling Dekker pattern can cause lost events.
- [High] The background harvesting mechanism races with the IRQ thread, potentially causing spurious IRQ_NONE returns on a shared interrupt line.
- [Medium] Passing complex pointers (cell arrays) directly via match table `.data` fields violates MFD subsystem guidelines.
- [Medium] Using `PLATFORM_DEVID_NONE` instead of `PLATFORM_DEVID_AUTO` breaks multi-instance support and causes sysfs collisions.
- [Medium] Data race on IRQ configuration fields between the event processing thread and IRQ configuration callbacks.
- [Medium] Missing bounds check on `hwirq` in `mc33978_irq_domain_alloc` leads to undefined behavior during bit shifts.
- [Low] Incorrect naming of the private data structure and instance variable.
- [Low] Non-standard commit message prefix and capitalization.
--

--- Patch [2]: [PATCH v3 2/7] mfd: add NXP MC33978/MC34978 core driver ---
commit ec976efb2461e1504417bd0ba87b3eb7ebe2cc78
Author: Oleksij Rempel <o.rempel@pengutronix.de>

mfd: add NXP MC33978/MC34978 core driver

This commit adds the core MFD driver for the NXP MC33978 and MC34978
Multiple Switch Detection Interfaces. It handles SPI communications with
a custom regmap bus, power sequencing, and interrupt demultiplexing. It
also implements inline status harvesting from SPI responses to detect
events.

> mfd: add NXP MC33978/MC34978 core driver

[Severity: Low]
Does this commit subject follow the subsystem guidelines? The MFD subsystem
expects the prefix format mfd: <Driver>: <Capitalized description>.

> diff --git a/drivers/mfd/mc33978.c b/drivers/mfd/mc33978.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/mfd/mc33978.c
[ ... ]
> +struct mc33978_mfd_priv {
> +	struct spi_device *spi;
> +	struct regmap *map;

[Severity: Low]
Does this struct name violate the subsystem naming conventions? The subsystem
rules state that private data structures should be named after the device,
avoiding mfd or priv, and the instance variable should be named ddata.

[ ... ]
> +static int mc33978_irq_domain_alloc(struct irq_domain *domain,
> +				    unsigned int virq,
> +				    unsigned int nr_irqs, void *arg)
> +{
> +	struct mc33978_mfd_priv *mc = domain->host_data;
> +	struct irq_fwspec *fwspec = arg;
> +	irq_hw_number_t hwirq;
> +	int i;
> +
> +	if (fwspec->param_count < 1)
> +		return -EINVAL;
> +
> +	hwirq = fwspec->param[0];
> +
> +	for (i = 0; i < nr_irqs; i++) {

[Severity: Medium]
Could this hwirq value exceed the valid range and cause undefined behavior?

There is no bounds check on hwirq here to ensure it falls within 0 to 22.
If a malformed device tree supplies an out-of-bounds hwirq, subsequent
calls to IRQ chip callbacks will execute a bit shift exceeding the variable
width.

[ ... ]
> +static bool mc33978_handle_pin_changes(struct mc33978_mfd_priv *mc,
> +				       unsigned int pin_state)
> +{
> +	u32 fired_pins = 0;
> +	u32 changed_pins;
> +	int i;
> +
> +	changed_pins = pin_state ^ mc->cached_pin_state;
> +	if (!changed_pins)
> +		return false;
> +
> +	mc->cached_pin_state = pin_state;
> +	changed_pins &= mc->cached_pin_mask;

[Severity: Medium]
Can this read of mc->cached_pin_mask race with concurrent updates?

The fields mc->cached_pin_mask, mc->irq_rise, and mc->irq_fall are written
inside IRQ chip callbacks while protected by mc->irq_lock.

However, they are read here under mc->event_lock without holding
irq_lock. Concurrent IRQ configurations on another CPU could cause data
races, potentially leading the driver to misroute or drop interrupts.

> +
> +	if (!changed_pins)
> +		return false;
> +
> +	fired_pins |= (changed_pins & pin_state) & mc->irq_rise;
> +	fired_pins |= (changed_pins & ~pin_state) & mc->irq_fall;
> +
> +	for_each_set_bit(i, (unsigned long *)&fired_pins, MC33978_NUM_PINS) {

[Severity: High]
Can this cast cause an out-of-bounds stack read and a big-endian logic bug?

The for_each_set_bit() macro expects an 8-byte aligned unsigned long pointer,
but it is passed a pointer to a 4-byte u32 stack variable (fired_pins).

On big-endian 64-bit systems, the 4-byte fired_pins becomes the most
significant 32 bits of the read 64-bit word, while the least significant
32 bits contain uninitialized stack memory. The loop will exclusively iterate
over the garbage stack bits instead of the actual fired_pins value.

> +		int virq = irq_find_mapping(mc->domain, i);
> +
> +		handle_nested_irq(virq);

[Severity: High]
Does this unconditionally pass a potentially invalid virq to handle_nested_irq()?

Since mc->cached_pin_mask is read locklessly under event_lock, a concurrent
call to free_irq() can dispose of the mapping right after the bit is checked.
irq_find_mapping() will then return 0.

Passing 0 to handle_nested_irq() resolves IRQ 0 to a null irq_desc and
crashes when acquiring desc->lock. A check for virq > 0 is required here.

> +	}
> +
> +	return true;
> +}
[ ... ]
> +static bool mc33978_process_single_event(struct mc33978_mfd_priv *mc)
> +{
> +	unsigned int pin_state;
> +	bool handled = false;
> +	u8 hw_flags;
> +	int ret;
> +
> +	ret = regmap_read(mc->map, MC33978_REG_READ_IN, &pin_state);
> +	if (ret)
> +		return false;

[Severity: High]
Will an SPI communication failure cause an infinite loop here?

If regmap_read() fails, the function immediately returns false before
executing the atomic_xchg(&mc->harvested_flags, 0).

Because harvested_flags is never cleared on this error path, the
while (atomic_read(&mc->harvested_flags) != 0) condition in
mc33978_handle_events() remains true endlessly, hanging the thread.

[ ... ]
> +static bool mc33978_handle_events(struct mc33978_mfd_priv *mc)
> +{
> +	bool handled = false;
> +
> +	guard(mutex)(&mc->event_lock);
> +
> +	do {
> +		atomic_set(&mc->is_handling, 1);
> +
> +		if (mc33978_process_single_event(mc))
> +			handled = true;
> +
> +		atomic_set(&mc->is_handling, 0);
> +
> +	} while (atomic_read(&mc->harvested_flags) != 0);

[Severity: High]
Are memory barriers needed here to prevent lost events?

The lockless communication between the thread and the SPI status harvester
relies on a Dekker-like pattern.

Void RMW atomic operations (atomic_or, atomic_set) do not imply full
memory barriers. Weakly-ordered CPUs can reorder the loads and stores.

The handler might evaluate atomic_read(&mc->harvested_flags) before its write
to is_handling becomes visible, causing both threads to assume the other
will process the event. smp_mb() or smp_mb__after_atomic() are required.

> +
> +	return handled;
> +}
> +
> +static irqreturn_t mc33978_irq_thread(int irq, void *data)
> +{
> +	return mc33978_handle_events(data) ? IRQ_HANDLED : IRQ_NONE;

[Severity: High]
Can the background harvesting mechanism race with this IRQ thread?

If an interrupt is physically asserted and the IRQ thread is scheduled, but a
concurrent SPI transaction runs first, mc33978_harvest_status() will capture
the event and schedule event_work.

The workqueue processes the event and clears it. When the IRQ thread executes,
it finds no events to process and returns IRQ_NONE. Over time, enough
spurious IRQ_NONE returns on an IRQF_SHARED line will trigger the kernel to
shut down the IRQ line entirely.

> +}
> +
> +static int mc33978_irq_init(struct mc33978_mfd_priv *mc)
> +{
> +	struct device *dev = &mc->spi->dev;
> +	int ret;
> +
> +	mutex_init(&mc->irq_lock);
> +
> +	/*
> +	 * Create IRQ domain with 23 interrupts:
> +	 * - hwirq 0-21: Pin change interrupts (22 pins)
> +	 * - hwirq 22: Fault interrupt (for hwmon driver)
> +	 */
> +	mc->domain = irq_domain_add_linear(dev->of_node, MC33978_NUM_PINS + 1,
> +					   &mc33978_irq_domain_ops, mc);
> +	if (!mc->domain)
> +		return dev_err_probe(dev, -ENOMEM, "Failed to create IRQ domain\n");
> +
> +	mc->domain->flags |= IRQ_DOMAIN_FLAG_HIERARCHY;

[Severity: High]
Does setting this flag break MFD child IRQ resolution?

The MFD core relies on irq_create_mapping() to translate the mfd_cell's
DEFINE_RES_IRQ(MC33978_HWIRQ_FAULT) resource to a Linux virq.

However, irq_create_mapping() explicitly aborts and returns 0 when called on
an IRQ domain bearing the IRQ_DOMAIN_FLAG_HIERARCHY flag. As a result, the
child device is instantiated with IRQ 0 and will never receive fault
interrupts.

[ ... ]
> +static void mc33978_harvest_status(struct mc33978_mfd_priv *mc, u8 status)
> +{
> +	if (!status)
> +		return;
> +
> +	atomic_or(status, &mc->harvested_flags);
> +
> +	if (!atomic_read(&mc->is_handling))

[Severity: High]
Does this Dekker-like pattern require memory barriers?

As mentioned above in mc33978_handle_events(), weakly-ordered CPUs can reorder
the loads and stores. The harvester might evaluate
!atomic_read(&mc->is_handling) before its store to harvested_flags becomes
visible, resulting in lost events.

[ ... ]
> +static int mc33978_probe(struct spi_device *spi)
> +{
> +	const struct mc33978_data *match_data;
> +	struct device *dev = &spi->dev;
> +	struct mc33978_mfd_priv *mc;
> +	int ret;
[ ... ]
> +	mutex_init(&mc->event_lock);
> +	INIT_WORK(&mc->event_work, mc33978_event_work);

[Severity: High]
Is event_work properly cancelled during driver cleanup?

The core driver uses devres to manage the mc struct lifetime but fails to
register a devres action to cancel mc->event_work.

If a child device triggers a background SPI harvest during its devres teardown
phase, the workqueue is scheduled. The parent driver's devres teardown then
completes and frees the mc structure, leading to a use-after-free when the
workqueue executes.

[ ... ]
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> +				   match_data->cells, match_data->num_cells,
> +				   NULL, 0, mc->domain);

[Severity: Medium]
Will using PLATFORM_DEVID_NONE break multi-instance support?

MFD guidelines dictate preferring PLATFORM_DEVID_AUTO. Using
PLATFORM_DEVID_NONE prevents the MFD core from appending instance indices to
the child device names. This causes identical names on the platform bus for
multiple parent devices, leading to driver load failures.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add MFD child devices\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mc33978_of_match[] = {
> +	{ .compatible = "nxp,mc33978", .data = &mc33978_match_data },

[Severity: Medium]
Is passing a complex pointer directly via the match table .data field safe?

The MFD subsystem guidelines forbid passing complex pointers (such as
structure wrapping mfd_cell arrays) through the .data field of of_device_id
or spi_device_id tables.

To avoid memory safety hazards and initialization ordering races, the
driver should use an enum or integer ID to select the static array in the
probe function instead.

> +	{ .compatible = "nxp,mc34978", .data = &mc34978_match_data },
> +	{ }
> +};

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