Re: [PATCH v8 2/3] i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing hardware reset deassertion in probe leaves I2C controller non-functional
- [High] Uninitialized stack variable `byte` is leaked to the I2C bus in target mode
- [High] Unconditional dereference of `i2c->target` causes kernel panic in target interrupt handler
- [High] Race condition in `ma35d1_i2c_write_ctl` swallows pending target interrupts and corrupts bus state
- [Low] Swapped inline documentation comments for arbitration lost statuses
--

--- Patch [2]: [PATCH v8 2/3] i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support ---
commit 6daea0deaf0ce019dd9ceedda9e214429b4994a4
Author: Zi-Yu Chen <[email protected]>
Subject: i2c: ma35d1: Add Nuvoton MA35D1 I2C driver support

This commit adds I2C support for the Nuvoton MA35D1 SoC. The controller
supports standard, fast, and fast-plus modes, providing both controller
and target functionality.

> diff --git a/drivers/i2c/busses/i2c-ma35d1.c b/drivers/i2c/busses/i2c-ma35d1.c
> new file mode 100644
> index 0000000000000..645fd7171104d
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ma35d1.c
[ ... ]
> +static void ma35d1_i2c_write_ctl(struct ma35d1_i2c *i2c, unsigned int ctl)
> +{
> +	unsigned int val;
> +
> +	guard(spinlock_irqsave)(&i2c->lock);
> +	val = readl(i2c->regs + MA35_CTL0);
> +	val &= ~(MA35_CTL_STA_SI_AA | MA35_CTL_STO);
> +	val |= ctl;
> +	writel(val, i2c->regs + MA35_CTL0);
> +}

[Severity: High]
Does this read-modify-write risk dropping the AA bit or clearing a pending
target interrupt? When a master transfer is started via
ma35d1_i2c_write_ctl(i2c, MA35_CTL_STA_SI) called from ma35d1_i2c_doxfer(), it
omits the MA35_CTL_AA flag. If a target interrupt is pending (SI=1), writing a
1 to SI clears the flag without processing the interrupt, potentially aborting
target transactions and breaking bus state.

[ ... ]
> +static irqreturn_t ma35d1_i2c_irq_target_trx(struct ma35d1_i2c *i2c,
> +					     unsigned long i2c_status)
> +{
> +	unsigned char byte;

[Severity: High]
Can this function be reached when i2c->target is NULL? 

If a master transfer encounters an arbitration loss (such as
MA35_S_RECE_ARB_LOST or MA35_ADDR_TRAN_ARB_LOST), the hardware drops into
target mode and triggers this interrupt. If no target backend is currently
registered, passing a NULL i2c->target to i2c_slave_event() further down
will cause a kernel panic.

> +
> +	switch (i2c_status) {
> +	case MA35_S_RECE_ARB_LOST:
> +		/*
> +		 * Arbitration lost during address transmission phase.
> +		 * The hardware switches to Target Transmitter mode when
> +		 * our own SLA+R is detected on the bus.
> +		 */

[Severity: Low]
Are the inline comments for MA35_S_RECE_ARB_LOST and MA35_ADDR_TRAN_ARB_LOST
swapped? This comment mentions Target Transmitter (SLA+R), but the code below
triggers I2C_SLAVE_WRITE_REQUESTED which corresponds to Target Receiver
(SLA+W).

> +		i2c->err = -EAGAIN;
> +		ma35d1_i2c_controller_complete(i2c);
> +		i2c_slave_event(i2c->target, I2C_SLAVE_WRITE_REQUESTED, &byte);
> +		break;
[ ... ]
> +	case MA35_ADDR_TRAN_ARB_LOST:
> +		/*
> +		 * Arbitration lost during address transmission phase.
> +		 * The hardware switches to Target Receiver mode when
> +		 * our own SLA+W is detected on the bus.
> +		 */
> +		i2c->err = -EAGAIN;
> +		ma35d1_i2c_controller_complete(i2c);
> +		i2c_slave_event(i2c->target, I2C_SLAVE_READ_REQUESTED, &byte);
> +		writel(byte, i2c->regs + MA35_DAT);

[Severity: High]
Could this write uninitialized stack memory to the I2C data bus? 

The local variable byte is passed to i2c_slave_event(). If the slave backend
returns an error or fails to populate the value, byte remains uninitialized,
and writel() will leak kernel stack data onto the physical bus.

[ ... ]
> +static int ma35d1_i2c_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	i2c->regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(i2c->regs))
> +		return PTR_ERR(i2c->regs);
> +
> +	i2c->rst = devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +	if (IS_ERR(i2c->rst))
> +		return dev_err_probe(dev, PTR_ERR(i2c->rst),
> +				     "failed to get reset control\n");
> +
> +	/* Setup info block for the I2C core */

[Severity: High]
Is there a missing reset_control_deassert(i2c->rst) here? 

The probe function acquires the reset control exclusively, but never seems
to deassert it before initialization. If the bootloader leaves the hardware
in an asserted reset state, will the controller remain non-functional?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2