Re: [PATCH v7 2/3] i3c: master: Add Qualcomm I3C controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] i3c: qcom-geni: use_7e updated after m_param in geni_i3c_master_priv_xfers()
- [High] i3c: qcom-geni: broken I3C_ADDR_MASK and DAA handling in geni_i3c_perform_daa()
- [High] i3c: qcom-geni: invalid clock lookup and 1 MHz cycle count in geni_i3c_clk_map
- [High] i3c: qcom-geni: unsafe I2C DMA mapping and broken DMA reset handling
- [High] i3c: qcom-geni: broken short RX FIFO reads and early termination in geni_i3c_irq()
- [High] i3c: qcom-geni: NACK error overwrite and status bitmask mismatch in geni_i3c_handle_err()
- [High] i3c: qcom-geni: missing reinit_completion() and TX_RESET_DONE in _i3c_geni_execute_command()
- [High] i3c: qcom-geni: TX watermark race and unhandled IRQ storm in geni_i3c_irq()
- [Medium] i3c: qcom-geni: double geni_se_resources_off() call in geni_i3c_probe()
- [Low] i3c: qcom-geni: unused variable m_stat_mask in geni_i3c_irq()
--

commit 2ad15e60246cd7898205f431d98abb7e053d2d97
Author: Mukesh Kumar Savaliya <mukesh.savaliya@oss.qualcomm.com>

i3c: master: Add Qualcomm I3C controller driver

This commit adds a driver for the Qualcomm QUPv3 GENI-based I3C master
controller supporting I3C SDR and legacy I2C transfers.

> diff --git a/drivers/i3c/master/i3c-qcom-geni.c b/drivers/i3c/master/i3c-qcom-geni.c
> new file mode 100644
> index 0000000000000..2fee77e681e26
> --- /dev/null
> +++ b/drivers/i3c/master/i3c-qcom-geni.c
[ ... ]
> +#define I3C_AUTO_SUSPEND_DELAY	250
> +#define PACKING_BYTES_PER_WORD	4
> +#define XFER_TIMEOUT		250
> +
> +#define I3C_ADDR_MASK		(I2C_MAX_ADDR >> 1)
[ ... ]
> +struct geni_i3c_dev {
[ ... ]
> +	u8 *cur_buf;
> +	u8 last_addr;
> +	bool cur_is_write;
> +	int cur_len;
> +	int cur_idx;
> +	DECLARE_BITMAP(newaddrslots, 64);
> +
> +	const struct geni_i3c_clk_settings *clk_cfg;
> +	const struct geni_i3c_clk_settings *clk_od_cfg;
> +};

[Severity: High]
Since I2C_MAX_ADDR is defined as GENMASK(6, 0) (0x7f) in
include/linux/i3c/master.h, shifting it right by 1 makes I3C_ADDR_MASK 0x3f
(6 bits instead of 7 bits), and newaddrslots is only 64 bits wide instead of
I3C_MAX_ADDR + 1 (128).

Later in geni_i3c_perform_daa():

	addr = i3c_master_get_free_addr(&gi3c->ctrlr,
					gi3c->last_addr + 1);
	if (addr > I3C_ADDR_MASK)
		return -EINVAL;

	gi3c->last_addr = addr;
	set_bit(addr, gi3c->newaddrslots);
	...
	ret = i3c_geni_execute_write_command(gi3c, &xfer, tx_buf, 1);
	if (ret)
		break;

Doesn't this reject valid 7-bit dynamic addresses from 0x40 to 0x7d (as well
as truncating negative error codes from i3c_master_get_free_addr() into u8)?

Also, in geni_i3c_perform_daa() and geni_i3c_master_send_ccc_cmd():
- gi3c->last_addr is never reset between DAA runs, so repeated DAA
  invocations will only search above the previous address until the range is
  exhausted.
- set_bit(addr, gi3c->newaddrslots) is called before
  i3c_geni_execute_write_command() sends the dynamic address to the target.
  If that write fails, geni_i3c_perform_daa() breaks out and returns 0 with
  addr still set in newaddrslots, causing geni_i3c_master_do_daa() to call
  i3c_master_add_i3c_dev_locked() for an unassigned address.
- geni_i3c_master_send_ccc_cmd() ignores the return value of
  geni_i3c_perform_daa(gi3c).

Should I3C_ADDR_MASK be I3C_MAX_ADDR (0x7f) with newaddrslots sized to
I3C_MAX_ADDR + 1, and should set_bit(addr, gi3c->newaddrslots) happen only
after the address write succeeds?

[ ... ]
> +static const struct geni_i3c_clk_settings geni_i3c_clk_map[] = {
> +	{
> +		.clk_freq_out = 100 * HZ_PER_KHZ,
> +		.clk_src_freq = 19200 * HZ_PER_KHZ,
> +		.clk_div = 1,
> +		.i2c_t_high_cnt = 76,
> +		.i2c_t_low_cnt = 90,
> +		.i3c_t_high_cnt = 7,
> +		.i3c_t_cycle_cnt = 8,
> +		.i2c_t_cycle_cnt = 192,
> +	},
> +	{
> +		.clk_freq_out = 400 * HZ_PER_KHZ,
> +		.clk_src_freq = 19200 * HZ_PER_KHZ,
> +		.clk_div = 1,
> +		.i2c_t_high_cnt = 12,
> +		.i2c_t_low_cnt = 24,
> +		.i3c_t_high_cnt = 7,
> +		.i3c_t_cycle_cnt = 8,
> +		.i2c_t_cycle_cnt = 48
> +	},
> +	{
> +		.clk_freq_out = 1000 * HZ_PER_KHZ,
> +		.clk_src_freq = 19200 * HZ_PER_KHZ,
> +		.clk_div = 1,
> +		.i2c_t_high_cnt = 4,
> +		.i2c_t_low_cnt = 9,
> +		.i3c_t_high_cnt = 7,
> +		.i3c_t_cycle_cnt = 0,
> +		.i2c_t_cycle_cnt = 19
> +	},
> +	{
> +		.clk_freq_out = 12500 * HZ_PER_KHZ,
> +		.clk_src_freq = 100000 * HZ_PER_KHZ,
> +		.clk_div = 1,
> +		.i2c_t_high_cnt = 45,
> +		.i2c_t_low_cnt = 63,
> +		.i3c_t_high_cnt = 6,
> +		.i3c_t_cycle_cnt = 7,
> +		.i2c_t_cycle_cnt = 110
> +	}
> +};
> +
> +static int geni_i3c_clk_map_idx(struct geni_i3c_dev *gi3c)
> +{
> +	const struct geni_i3c_clk_settings *clk_table_idx = geni_i3c_clk_map;
> +	struct i3c_master_controller *m = &gi3c->ctrlr;
> +	struct i3c_bus *bus = i3c_master_get_bus(m);
> +
> +	for (int i = 0; i < ARRAY_SIZE(geni_i3c_clk_map); i++, clk_table_idx++) {
> +		if (clk_table_idx->clk_freq_out == bus->scl_rate.i3c &&
> +		    clk_table_idx->clk_src_freq == gi3c->clk_src_freq)
> +			gi3c->clk_cfg = clk_table_idx;
> +
> +		if (clk_table_idx->clk_freq_out == bus->scl_rate.i2c)
> +			gi3c->clk_od_cfg = clk_table_idx;
> +	}
> +
> +	if (!gi3c->clk_cfg || !gi3c->clk_od_cfg)
> +		return -EINVAL;
> +
> +	return 0;
> +}

[Severity: High]
How does geni_i3c_clk_map_idx() behave on a pure I3C bus or a mixed-slow
bus?

In i3c_bus_set_mode(), when bus->mode is I3C_BUS_MODE_PURE and no i2c-scl-hz
property is set in devicetree, bus->scl_rate.i2c remains 0. Because no entry
in geni_i3c_clk_map[] has clk_freq_out == 0, gi3c->clk_od_cfg stays NULL and
geni_i3c_master_bus_init() fails with -EINVAL.

Conversely, in I3C_BUS_MODE_MIXED_SLOW (where i3c_bus_set_mode() clamps
bus->scl_rate.i3c to bus->scl_rate.i2c), i3c_geni_resources_init() hardcodes
gi3c->clk_src_freq to 100 MHz while the 100 kHz, 400 kHz, and 1000 kHz
entries in geni_i3c_clk_map[] all have clk_src_freq set to 19.2 MHz, so
gi3c->clk_cfg stays NULL and geni_i3c_clk_map_idx() also returns -EINVAL.

Also, in the 1000 kHz entry of geni_i3c_clk_map[], i3c_t_cycle_cnt is 0 while
i3c_t_high_cnt is 7 (whereas the 100 kHz and 400 kHz entries use
i3c_t_cycle_cnt = 8). Is programming SE_I3C_SCL_CYCLE to 0 when
qcom_geni_i3c_conf(gi3c, OPEN_DRAIN_MODE) selects the 1000 kHz entry
intentional?

[ ... ]
> +static void geni_i3c_handle_err(struct geni_i3c_dev *gi3c, u32 status)
> +{
> +	dev_dbg(gi3c->se.dev, "len:%d, %s\n",
> +		gi3c->cur_len, gi3c->cur_is_write ? "write" : "read");
> +
> +	if (status & M_GP_IRQ_0_EN) {
> +		gi3c->err = -EINVAL;
> +		dev_dbg(gi3c->se.dev, "I3C slave early read termination\n");
> +	}
[ ... ]
> +	if (status & M_CMD_ABORT_EN) {
> +		gi3c->err = -ETIMEDOUT;
> +		dev_dbg(gi3c->se.dev, "Abort after timeout successful\n");
> +	}
> +
> +	if (status == GENI_TIMEOUT) {
> +		gi3c->err = -ETIMEDOUT;
> +		dev_dbg(gi3c->se.dev, "I3C transaction timed out\n");
> +	}
> +}

[Severity: High]
Won't geni_i3c_handle_err() clobber an earlier NACK error (-ENOTCONN or
-EBUSY) with -ETIMEDOUT when a transfer times out and aborts?

When a NACK interrupt fires without M_CMD_DONE_EN set, geni_i3c_irq() records
-ENOTCONN or -EBUSY in gi3c->err and waits for the timeout path in
_i3c_geni_execute_command() to abort the command. Both the timeout path
(passing GENI_TIMEOUT) and the subsequent M_CMD_ABORT_EN interrupt overwrite
gi3c->err with -ETIMEDOUT, so geni_i3c_master_send_ccc_cmd() no longer sees
-ENOTCONN or -EBUSY and cannot set cmd->err = I3C_ERROR_M2.

In addition, status in geni_i3c_handle_err() is checked both as an
SE_GENI_M_IRQ_STATUS bitmask and for equality with the enum constant
GENI_TIMEOUT (which has value 9, or M_CMD_FAILURE_EN | M_CMD_DONE_EN). And
if geni_i3c_irq() enters the error branch due to dm_rx_st & DM_I3C_CB_ERR or
M_CMD_FAILURE_EN alone, geni_i3c_handle_err(gi3c, m_stat) leaves gi3c->err as
0. Should gi3c->err only be updated when it is not already set, with a
fallback error code when none of the specific M_GP_IRQ_* bits match?

> +
> +static irqreturn_t geni_i3c_irq(int irq, void *dev)
> +{
> +	struct geni_i3c_dev *gi3c = dev;
> +	u32 m_stat_mask, dm_tx_st, dm_rx_st;
> +	u32 m_stat, rx_st, dma_mode;
> +
> +	m_stat = readl_relaxed(gi3c->se.base + SE_GENI_M_IRQ_STATUS);
> +	m_stat_mask = readl_relaxed(gi3c->se.base + SE_GENI_M_IRQ_EN);

[Severity: Low]
In geni_i3c_irq(), m_stat_mask is assigned here but never used, which will
trigger an unused-but-set-variable compiler warning. Should m_stat be masked
with m_stat_mask, or should m_stat_mask be removed?

> +	rx_st = readl_relaxed(gi3c->se.base + SE_GENI_RX_FIFO_STATUS);
> +	dm_tx_st = readl_relaxed(gi3c->se.base + SE_DMA_TX_IRQ_STAT);
> +	dm_rx_st = readl_relaxed(gi3c->se.base + SE_DMA_RX_IRQ_STAT);
> +	dma_mode = readl_relaxed(gi3c->se.base + SE_GENI_DMA_MODE_EN);
> +
> +	scoped_guard(spinlock, &gi3c->irq_lock) {
> +		if (m_stat & SE_I3C_ERR || dm_rx_st & DM_I3C_CB_ERR) {
> +			geni_i3c_handle_err(gi3c, m_stat);
> +			/* Disable the TX Watermark interrupt to stop TX */
> +			if (!dma_mode)
> +				writel_relaxed(0, gi3c->se.base + SE_GENI_TX_WATERMARK_REG);
> +		} else if (dma_mode) {
> +			dev_dbg(gi3c->se.dev, "i3c dma tx:0x%x, dma rx:0x%x\n", dm_tx_st, dm_rx_st);
> +		} else if ((m_stat & (M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN)) &&
> +			    !gi3c->cur_is_write && gi3c->cur_buf) {
> +			/* FIFO RX handling */
> +			u32 rxcnt = rx_st & RX_FIFO_WC_MSK;
> +			int avail = rxcnt * 4;
> +			int need  = gi3c->cur_len - gi3c->cur_idx;
> +			int n     = min(need, avail);
> +
> +			if (n > 0) {
> +				i3c_readl_fifo(gi3c->se.base + SE_GENI_RX_FIFOn,
> +					       gi3c->cur_buf + gi3c->cur_idx, n);
> +				gi3c->cur_idx += n;
> +			}

[Severity: High]
What happens in geni_i3c_irq() when an I3C target returns a valid short read
response (for example, a 2-byte response to a 5-byte GETMXDS or 3-byte GETMRL
CCC read) or terminates a read early via the T-bit?

First, RX_FIFO_WC_MSK in SE_GENI_RX_FIFO_STATUS counts 32-bit FIFO words, and
valid bytes in the final partial word are indicated by RX_LAST and
RX_LAST_BYTE_VALID_MSK. Computing avail = rxcnt * 4 rounds a 2-byte response
up to 4 bytes, so geni_i3c_master_send_ccc_cmd() sets
cmd->dests[i].payload.actual_len = gi3c->cur_idx to 4 (for GETMXDS, causing
i3c_master_getmxds_locked() to fail with -EIO) or 3 (for GETMRL, reading a
padding byte as max_ibi_len).

Second, M_GP_IRQ_0_EN ("I3C slave early read termination") is included in
SE_I3C_ERR and sets gi3c->err = -EINVAL in geni_i3c_handle_err(). Because
the FIFO RX handler is in an else-if branch after if (m_stat & SE_I3C_ERR ...),
an early-terminated read fails with -EINVAL and leaves the received words
unread in the RX FIFO.

> +		} else if ((m_stat & M_TX_FIFO_WATERMARK_EN) &&
> +			   gi3c->cur_is_write && gi3c->cur_buf) {
> +			/* FIFO TX handling */
> +			int avail = gi3c->tx_wm * 4;
> +			int need  = gi3c->cur_len - gi3c->cur_idx;
> +			int n     = min(need, avail);
> +
> +			if (n > 0) {
> +				i3c_writel_fifo(gi3c->se.base + SE_GENI_TX_FIFOn,
> +						(void *)(gi3c->cur_buf + gi3c->cur_idx), n);
> +				gi3c->cur_idx += n;
> +			}
> +		}
> +
> +		if (!dma_mode && gi3c->cur_idx == gi3c->cur_len)
> +			writel_relaxed(0, gi3c->se.base + SE_GENI_TX_WATERMARK_REG);
> +	}

[Severity: High]
Can M_TX_FIFO_WATERMARK_EN cause an unhandled interrupt storm if gi3c->cur_buf
becomes NULL while gi3c->cur_idx < gi3c->cur_len?

In _i3c_geni_execute_command():

	geni_se_setup_m_cmd(&gi3c->se, xfer->m_cmd, xfer->m_param);
	...
	if (xfer->mode == GENI_SE_FIFO && len > 0) /* Get FIFO IRQ */
		writel_relaxed(1, gi3c->se.base + SE_GENI_TX_WATERMARK_REG);

Because geni_se_setup_m_cmd() is called before enabling
SE_GENI_TX_WATERMARK_REG without holding gi3c->irq_lock, an immediate error
or abort interrupt can run first and clear SE_GENI_TX_WATERMARK_REG, after
which _i3c_geni_execute_command() writes 1 to SE_GENI_TX_WATERMARK_REG and
sets gi3c->cur_buf = NULL. Likewise, on a transfer timeout,
_i3c_geni_execute_command() sets gi3c->cur_buf = NULL without clearing
SE_GENI_TX_WATERMARK_REG.

When M_TX_FIFO_WATERMARK_EN then fires in geni_i3c_irq() with
gi3c->cur_buf == NULL and gi3c->cur_idx < gi3c->cur_len, neither the TX
branch nor the cur_idx == cur_len check disables SE_GENI_TX_WATERMARK_REG,
leaving the level-triggered watermark interrupt asserted continuously.

> +
> +	if (m_stat)
> +		writel_relaxed(m_stat, gi3c->se.base + SE_GENI_M_IRQ_CLEAR);
> +
> +	if (dma_mode) {
> +		if (dm_tx_st)
> +			writel_relaxed(dm_tx_st, gi3c->se.base + SE_DMA_TX_IRQ_CLR);
> +		if (dm_rx_st)
> +			writel_relaxed(dm_rx_st, gi3c->se.base + SE_DMA_RX_IRQ_CLR);
> +	}
> +
> +	/* if this is err with done-bit not set, handle that through timeout. */
> +	if (m_stat & (M_CMD_DONE_EN | M_CMD_ABORT_EN)) {
> +		writel_relaxed(0, gi3c->se.base + SE_GENI_TX_WATERMARK_REG);
> +		complete(&gi3c->done);
> +	} else if ((dm_tx_st & TX_DMA_DONE) || (dm_rx_st & (RX_DMA_DONE | RX_RESET_DONE))) {
> +		complete(&gi3c->done);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
[ ... ]
> +static int _i3c_geni_execute_command(struct geni_i3c_dev *gi3c, struct geni_i3c_xfer_params *xfer)
> +{
[ ... ]
> +	if (xfer->mode == GENI_SE_DMA) {
> +		if (gi3c->err) {
> +			if (is_write)
> +				writel_relaxed(1, gi3c->se.base + SE_DMA_TX_FSM_RST);
> +			else
> +				writel_relaxed(1, gi3c->se.base + SE_DMA_RX_FSM_RST);
> +
> +			time_remaining = wait_for_completion_timeout(&gi3c->done, XFER_TIMEOUT);
> +			dev_dbg(gi3c->se.dev, "Timeout completing DMA transfer\n");
> +		}
> +
> +		if (is_write)
> +			geni_se_tx_dma_unprep(&gi3c->se, tx_dma, len);
> +		else
> +			geni_se_rx_dma_unprep(&gi3c->se, rx_dma, len);
> +	}

[Severity: High]
In _i3c_geni_execute_command() and geni_i3c_irq(), how is gi3c->done
synchronized across multi-command loops and DMA FSM resets?

1. Callers only call reinit_completion(&gi3c->done) once before their
   transfer loops rather than in _i3c_geni_execute_command() before each
   command and before waiting on SE_DMA_TX_FSM_RST / SE_DMA_RX_FSM_RST.
2. In geni_i3c_irq(), the DMA completion check handles RX_RESET_DONE in
   dm_rx_st, but omits TX_RESET_DONE in dm_tx_st, so waiting for
   SE_DMA_TX_FSM_RST in _i3c_geni_execute_command() will always time out
   before geni_se_tx_dma_unprep() unmaps the buffer.
3. Because m_stat is not masked with SE_GENI_M_IRQ_EN, if M_CMD_DONE_EN is
   set in m_stat while dma_mode is active, geni_i3c_irq() can complete
   gi3c->done before TX_DMA_DONE or RX_DMA_DONE fires, causing
   _i3c_geni_execute_command() to unmap the DMA buffer while DMA is still in
   flight and leaving an extra completion in gi3c->done for the next command.

[ ... ]
> +static int geni_i3c_master_priv_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *xfers,
> +				      int nxfers, enum i3c_xfer_mode mode)
> +{
> +	struct i3c_master_controller *m = i3c_dev_get_master(dev);
> +	struct geni_i3c_dev *gi3c = to_geni_i3c_master(m);
> +	bool use_7e = false;
> +	int ret;
> +
> +	scoped_guard(mutex, &gi3c->lock) {
[ ... ]
> +		qcom_geni_i3c_conf(gi3c, PUSH_PULL_MODE);
> +
> +		for (int i = 0; i < nxfers; i++) {
> +			bool stall = (i < (nxfers - 1));
> +			struct geni_i3c_xfer_params xfer = { GENI_SE_FIFO };
> +
> +			xfer.m_param  = (stall ? STOP_STRETCH : 0);
> +			xfer.m_param |= FIELD_PREP(SLAVE_ADDR_MASK, dev->info.dyn_addr);
> +			xfer.m_param |= (use_7e) ? USE_7E : 0;
> +
> +			/* use_7e = true only for last transfer */
> +			use_7e = (i == nxfers - 1);

[Severity: High]
In geni_i3c_master_priv_xfers(), use_7e is initialized to false and only set
to (i == nxfers - 1) after xfer.m_param |= (use_7e) ? USE_7E : 0 has already
been evaluated.

On iterations i < nxfers - 1, use_7e is false; on the final iteration
i == nxfers - 1, use_7e is still false when xfer.m_param is built and only
becomes true right before the loop exits. As a result, USE_7E is never set on
any transfer (and shouldn't USE_7E be set on the first transfer i == 0, as in
geni_i3c_master_send_ccc_cmd() and geni_i3c_perform_daa()?).

> +
> +			if (xfers[i].rnw) {
[ ... ]
> +static int geni_i3c_master_i2c_xfers(struct i2c_dev_desc *dev, struct i2c_msg *msgs, int num)
> +{
[ ... ]
> +	for (int i = 0; i < num; i++) {
> +		struct geni_i3c_xfer_params xfer;
> +
> +		xfer.m_cmd    = (msgs[i].flags & I2C_M_RD) ? I2C_READ : I2C_WRITE;
> +		xfer.m_param  = (i < (num - 1)) ? STOP_STRETCH : 0;
> +		xfer.m_param |= FIELD_PREP(SLAVE_ADDR_MASK, msgs[i].addr);
> +		xfer.mode     = msgs[i].len > (gi3c->tx_depth / 2) ?
> +				GENI_SE_DMA : GENI_SE_FIFO;
> +		if (msgs[i].flags & I2C_M_RD)
> +			ret = i3c_geni_execute_read_command(gi3c, &xfer, msgs[i].buf,
> +							    msgs[i].len);
> +		else
> +			ret = i3c_geni_execute_write_command(gi3c, &xfer, msgs[i].buf,
> +							     msgs[i].len);
> +		if (ret)
> +			break;
> +	}

[Severity: High]
In geni_i3c_master_i2c_xfers(), when msgs[i].len > (gi3c->tx_depth / 2),
msgs[i].buf is passed directly to GENI_SE_DMA and mapped with
dma_map_single() in geni_se_tx_dma_prep() / geni_se_rx_dma_prep().

Since i2c_msg buffers are not guaranteed to be DMA-safe (as noted in the
struct i3c_master_controller_ops documentation in include/linux/i3c/master.h),
should this use i2c_get_dma_safe_msg_buf() and i2c_put_dma_safe_msg_buf()?

Also, for GENI_SE_DMA transfers:
- In _i3c_geni_execute_command(), geni_se_setup_m_cmd() starts the hardware
  command before geni_se_tx_dma_prep() or geni_se_rx_dma_prep() sets up the
  DMA transfer (or falls back to GENI_SE_FIFO).
- In i3c_geni_resources_init(), avg_bw is set on GENI_TO_CORE and
  CPU_TO_GENI, but gi3c->se.icc_paths[GENI_TO_DDR].avg_bw is left at 0.

[ ... ]
> +static int geni_i3c_hw_init(struct geni_i3c_dev *gi3c, struct platform_device *pdev)
> +{
> +	u32 proto, fifo_disable;
> +
> +	proto = geni_se_read_proto(&gi3c->se);
> +	if (proto != GENI_SE_I3C) {
> +		geni_se_resources_off(&gi3c->se);
> +		return dev_err_probe(gi3c->se.dev, -ENXIO, "Invalid proto %d\n", proto);
> +	}
> +
> +	fifo_disable = readl_relaxed(gi3c->se.base + GENI_IF_DISABLE_RO);
> +	if (fifo_disable) {
> +		geni_se_resources_off(&gi3c->se);
> +		return dev_err_probe(gi3c->se.dev, -ENXIO, "GPI DMA mode not supported\n");
> +	}
[ ... ]
> +static int geni_i3c_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = geni_se_resources_on(&gi3c->se);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "Error turning resources ON\n");
> +
> +	ret = geni_i3c_hw_init(gi3c, pdev);
> +	geni_se_resources_off(&gi3c->se);
> +	if (ret)
> +		return ret;
[ ... ]
> +static int geni_i3c_runtime_suspend(struct device *dev)
> +{
> +	struct geni_i3c_dev *gi3c = dev_get_drvdata(dev);
> +
> +	disable_irq(gi3c->irq);
> +	geni_se_resources_off(&gi3c->se);
> +
> +	return 0;
> +}

[Severity: Medium]
If geni_i3c_hw_init() fails, it calls geni_se_resources_off(&gi3c->se) before
returning, and then geni_i3c_probe() calls geni_se_resources_off(&gi3c->se) a
second time. Won't this underflow the enable/prepare counts on se->clk and
the shared QUP wrapper clocks?

Also, in geni_i3c_runtime_suspend(), if geni_se_resources_off(&gi3c->se)
fails (for instance if pinctrl_pm_select_sleep_state() returns an error
before clocks are disabled), should the error be propagated after re-enabling
gi3c->irq?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104111610.3739095-1-mukesh.savaliya@oss.qualcomm.com?part=2

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c