Re: [PATCH v2 2/2] i2c: mux: Add NXP PCA9641 I2C bus master arbiter driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i2c: pca9641: missing interrupt clear in handler causes IRQ storm
- [High] i2c: pca9641: probe error path leaves IRQ active during polling fallback
- [High] i2c: pca9641: select_chan() illegally sleeps in atomic context
--

commit 1eafadafa36c19cfd93cd95319667d5c31725ee1
Author: Shiv Prakash Gupta <shivprakash.gupta@nxp.com>

i2c: mux: Add NXP PCA9641 I2C bus master arbiter driver

This patch adds a driver for the NXP PCA9641 2-to-1 I2C bus master arbiter.
It introduces support for interrupt-assisted arbitration to avoid busy-polling,
as well as a fallback to polling mode when interrupts are not configured.

> diff --git a/drivers/i2c/muxes/i2c-mux-pca9641.c b/drivers/i2c/muxes/i2c-mux-pca9641.c
> new file mode 100644
> index 0000000000000..6e6f29fdd6d49
> --- /dev/null
> +++ b/drivers/i2c/muxes/i2c-mux-pca9641.c
[ ... ]
> +static irqreturn_t pca9641_irq_handler(int irq, void *dev_id)
> +{
> +	struct pca9641 *data = dev_id;
> +
> +	complete(&data->lock_grant_comp);
> +	return IRQ_HANDLED;
> +}

[Severity: High]
Does this threaded handler need to clear the device-level sticky interrupt
status before returning IRQ_HANDLED?

If the physical line remains asserted because the device-level interrupt
status isn't cleared here, returning IRQ_HANDLED will cause the generic IRQ
core to unmask the line, immediately triggering another hardware interrupt.
This would set up an infinite loop of RT-priority threaded handler executions,
starving the normal-priority task running pca9641_select_chan() and
potentially livelocking the system.

[ ... ]
> +static int pca9641_select_chan(struct i2c_mux_core *muxc, u32 chan)
> +{
> +	struct pca9641 *data = i2c_mux_priv(muxc);
> +	struct i2c_client *client = data->client;
> +	unsigned long timeout = jiffies + 2 * client->adapter->timeout;
> +	int ctrl, ret;
> +
> +	if (data->irq > 0) {
> +		reinit_completion(&data->lock_grant_comp);
> +
> +		ctrl = pca9641_reg_read(client, PCA9641_CONTR);
> +		if (ctrl < 0)
> +			return ctrl;
> +
> +		if (ctrl & PCA9641_CTL_LOCK_GRANT)
> +			goto set_bus_connect;
> +
> +		ret = pca9641_reg_write(client, PCA9641_CONTR,
> +					(u8)((ctrl & ~PCA9641_CTL_BUS_CONNECT) |
> +					     PCA9641_CTL_LOCK_REQ));
> +		if (ret < 0)
> +			return ret;
> +
> +		if (!wait_for_completion_timeout(&data->lock_grant_comp,
> +						 client->adapter->timeout)) {

[Severity: High]
Can this unconditionally call a sleeping synchronization primitive in atomic
context?

When an atomic I2C transfer is requested, the i2c-mux core executes in atomic
context with interrupts disabled and calls pca9641_select_chan(). Calling
wait_for_completion_timeout() here invokes schedule(), which explicitly
sleeps. This violates context constraints and would trigger a scheduling while
atomic bug.

> +			ctrl = pca9641_reg_read(client, PCA9641_CONTR);
> +			if (ctrl < 0)
> +				return ctrl;
> +			if (!(ctrl & PCA9641_CTL_LOCK_GRANT)) {
> +				dev_warn(&client->dev,
> +					 "Timed out waiting for bus grant\n");
> +				/*
> +				 * Clear our latched LOCK_REQ so the arbiter is
> +				 * not left holding a request for a master that
> +				 * has given up, which would block the peer.
> +				 */
> +				pca9641_release_bus(client);
> +				return -ETIMEDOUT;
> +			}

[Severity: High]
Does this error path return without clearing the pending interrupt status?

If so, it would leave the unmasked line asserted, which also guarantees an
immediate IRQ storm.

[ ... ]
> +	do {
> +		ret = pca9641_arbitrate(client);
> +		if (ret)
> +			return ret < 0 ? ret : 0;
> +
> +		if (data->select_timeout <= PCA9641_DELAY_SHORT)
> +			udelay(data->select_timeout);
> +		else
> +			msleep(data->select_timeout / 1000);

[Severity: High]
Is it safe to unconditionally call msleep() here?

During an atomic I2C transfer, calling msleep() would sleep while interrupts
are disabled, violating context constraints. Additionally, this polling loop
relies on time_is_after_eq_jiffies(), but jiffies do not advance when timer
interrupts are disabled, leading to an infinite spin loop.

Could the driver check i2c_in_atomic_xfer_mode() and implement a safe delay
loop using udelay() for atomic context?

> +	} while (time_is_after_eq_jiffies(timeout));
> +
> +	dev_warn(&client->dev, "Failed to acquire I2C bus, timed out\n");
> +	/* Drop our latched LOCK_REQ so the peer master is not blocked. */
> +	pca9641_release_bus(client);
> +	return -ETIMEDOUT;
> +}
[ ... ]
> +static int pca9641_probe(struct i2c_client *client)
> +{
[ ... ]
> +		/* Handler is ready; now enable LOCK_GRANT + BUS_LOST. */
> +		ret = i2c_smbus_write_byte_data(client, PCA9641_INT_MSK,
> +						PCA9641_INT_MSK_ARB);

[Severity: High]
Does this unmask the BUS_LOST interrupt, which doesn't seem to be handled
anywhere in the driver?

If BUS_LOST fires, returning from the interrupt handler without clearing it
would guarantee an immediate and permanent IRQ storm.

> +		if (ret < 0) {
> +			dev_warn(&client->dev,
> +				 "Failed to enable interrupts (%d); using polling\n",
> +				 ret);
> +			goto add_adapter;
> +		}

[Severity: High]
Does this error path fall back to polling mode without freeing the previously
requested threaded IRQ handler?

If the hardware later asserts the IRQ line (e.g. from spurious noise or
because it was already enabled prior to probe), the active handler will execute
and return IRQ_HANDLED. Because pca9641_select_chan() operates in polling mode
(data->irq == -1) and never waits on the completion, the interrupt status is
never cleared, triggering a permanent IRQ storm and system lockup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917085852.552079-1-shivprakash.gupta@nxp.com?part=2