[PATCH v5 00/17] i3c: renesas: Suspend to RAM with power loss and runtime PM

[PATCH v5 00/17] i3c: renesas: Suspend to RAM with power loss and runtime PM

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

This series adjusts the suspend to RAM code to handle cases where power
to the connected devices is lost during suspend to RAM. The fixes
included in this series are required for that support.

Along with suspend to RAM support, runtime PM support is also added.
Cleanup patches were included to prepare for clean runtime PM support.

Thank you,
Claudiu

Changes in v5:
- in patch 1 introduced renesas_i3c_irqs_mask_and_clear_locked()
  similar to what was present in v4 in patch
  "i3c: renesas: Add runtime PM support" but without locking; use the
  same function to mask all the interrupts and cleanup the status
  flag in case interrupts are triggered after the transfer completion
  timed out
- introduce patch "i3c: renesas: Fix out-of-bounds access for newdevs mask"
  to avoid failures when there are no I3C devices connected on the
  bus at probe
- patch "i3c: renesas: Perform Dynamic Address Assignment on resume"
  was restored to the v1 variant; kept it simple to fix the
  suspend/resume for the moment

Changes in v4:
- dropped patch "i3c: renesas: Do not attach devices if xfer failed"
- fixed the swap in renesas_i3c_group_devs_in_slots() for i3c->addr[]
  renesas_i3c_irqs_mask_and_clear();
- in patch 16/16 renamed renesas_i3c_abort_xfer() from v3 to
	
Changes in v3:
- re-based on top of series [1] to be able to use
  i3c_master_reattach_i3c_dev_locked()
- used i3c_master_reattach_i3c_dev_locked() on patch
  "i3c: renesas: Perform Dynamic Address Assignment on resume" along with
  grouping the I2C and I3C devices in the driver slot
- collected tags

Changes in v2:
- dropped patch "i3c: renesas: Use the divider 128"
- adjusted the patches title and description where requested in the
  review process
- adjusted the DAA procedure after resume to still properly re-configure
  the controller in case the bus was full before a suspend
- added patch "i3c: renesas: Do not attach devices if xfer failed"
- collected tags

Claudiu Beznea (17):
  i3c: renesas: Check that the transfer is valid before accessing it
  i3c: renesas: Restore STDBR and EXTBR registers on resume
  i3c: renesas: Follow the reset deassert order used in probe
  i3c: renesas: Reconfigure the DATBAS register on re-attach
  i3c: renesas: Reset the controller on resume
  i3c: renesas: Perform Dynamic Address Assignment on resume
  i3c: renesas: Clean DATBAS register on detach
  i3c: renesas: Fix out-of-bounds access for newdevs mask
  i3c: renesas: Use reset_control_bulk_{assert, deassert}()
  i3c: renesas: Return immediately if there is no transfer
  i3c: renesas: Follow a unified pattern for transfer and command
    initialization
  i3c: renesas: Drop the explicit memset() call
  i3c: renesas: Update HW registers after SW computations are done
  i3c: renesas: Organize structures to avoid unnecessary padding
  i3c: renesas: Use the "dev_name:irq_name" format for the interrupt
    name
  i3c: renesas: Drop unnecessary tab
  i3c: renesas: Add runtime PM support

 drivers/i3c/master/renesas-i3c.c | 375 +++++++++++++++++++++----------
 1 file changed, 261 insertions(+), 114 deletions(-)

-- 
2.43.0

[PATCH v5 00/17] i3c: renesas: Suspend to RAM with power loss and runtime PM

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

This series adjusts the suspend to RAM code to handle cases where power
to the connected devices is lost during suspend to RAM. The fixes
included in this series are required for that support.

Along with suspend to RAM support, runtime PM support is also added.
Cleanup patches were included to prepare for clean runtime PM support.

Thank you,
Claudiu

Changes in v5:
- in patch 1 introduced renesas_i3c_irqs_mask_and_clear_locked()
  similar to what was present in v4 in patch
  "i3c: renesas: Add runtime PM support" but without locking; use the
  same function to mask all the interrupts and cleanup the status
  flag in case interrupts are triggered after the transfer completion
  timed out
- introduce patch "i3c: renesas: Fix out-of-bounds access for newdevs mask"
  to avoid failures when there are no I3C devices connected on the
  bus at probe
- patch "i3c: renesas: Perform Dynamic Address Assignment on resume"
  was restored to the v1 variant; kept it simple to fix the
  suspend/resume for the moment

Changes in v4:
- dropped patch "i3c: renesas: Do not attach devices if xfer failed"
- fixed the swap in renesas_i3c_group_devs_in_slots() for i3c->addr[]
  renesas_i3c_irqs_mask_and_clear();
- in patch 16/16 renamed renesas_i3c_abort_xfer() from v3 to
	
Changes in v3:
- re-based on top of series [1] to be able to use
  i3c_master_reattach_i3c_dev_locked()
- used i3c_master_reattach_i3c_dev_locked() on patch
  "i3c: renesas: Perform Dynamic Address Assignment on resume" along with
  grouping the I2C and I3C devices in the driver slot
- collected tags

Changes in v2:
- dropped patch "i3c: renesas: Use the divider 128"
- adjusted the patches title and description where requested in the
  review process
- adjusted the DAA procedure after resume to still properly re-configure
  the controller in case the bus was full before a suspend
- added patch "i3c: renesas: Do not attach devices if xfer failed"
- collected tags

Claudiu Beznea (17):
  i3c: renesas: Check that the transfer is valid before accessing it
  i3c: renesas: Restore STDBR and EXTBR registers on resume
  i3c: renesas: Follow the reset deassert order used in probe
  i3c: renesas: Reconfigure the DATBAS register on re-attach
  i3c: renesas: Reset the controller on resume
  i3c: renesas: Perform Dynamic Address Assignment on resume
  i3c: renesas: Clean DATBAS register on detach
  i3c: renesas: Fix out-of-bounds access for newdevs mask
  i3c: renesas: Use reset_control_bulk_{assert, deassert}()
  i3c: renesas: Return immediately if there is no transfer
  i3c: renesas: Follow a unified pattern for transfer and command
    initialization
  i3c: renesas: Drop the explicit memset() call
  i3c: renesas: Update HW registers after SW computations are done
  i3c: renesas: Organize structures to avoid unnecessary padding
  i3c: renesas: Use the "dev_name:irq_name" format for the interrupt
    name
  i3c: renesas: Drop unnecessary tab
  i3c: renesas: Add runtime PM support

 drivers/i3c/master/renesas-i3c.c | 375 +++++++++++++++++++++----------
 1 file changed, 261 insertions(+), 114 deletions(-)

-- 
2.43.0


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

[PATCH v5 01/17] i3c: renesas: Check that the transfer is valid before accessing it

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The Renesas I3C driver uses an asynchronous model to transfer data. It
prepares a struct renesas_i3c_xfer, enqueues it, and waits for completion.
The interrupt handler dequeues the transfer, updates/uses it, and signals
the waiting thread.

If the completion times out, the waiting thread dequeues the transfer and
free it. If an interrupt fires after that, the handler may access freed
memory, leading to crashes.

Check that the transfer is still valid before accessing it in the
interrupt handler. With it clear any status flags and disable all
the interrupts to avoid triggering the same interrupts again.

Fixes: d028219a9f14 ("i3c: master: Add basic driver for the Renesas I3C controller")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- introduced renesas_i3c_irqs_mask_and_clear_locked() that keeps
  unified the IRQ mask and clean path
- updated the patch description

Changes in v4:
- disable also the interrupts
- dropped the Rb tag

Changes in v3:
- none

Changes in v2:
- clean the IRQ status bits before returning IRQ_HANDLED and adjusted the
  patch description to reflect this change
- collected Frank's tag. Frank, please let me know if you consider
  I should drop your tag. Thanks!

 drivers/i3c/master/renesas-i3c.c | 52 +++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 7 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index f39c449922ca..38b8428f464c 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -433,6 +433,21 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
 	}
 }
 
+static void renesas_i3c_irqs_mask_and_clear_locked(struct renesas_i3c *i3c)
+{
+	/* Disable all the interrupts. */
+	renesas_writel(i3c->regs, BIE, 0);
+	renesas_writel(i3c->regs, NTIE, 0);
+
+	/* Clear normal transfer status flags. */
+	renesas_writel(i3c->regs, NTST, 0);
+
+	/* Clear bus status flags. */
+	renesas_writel(i3c->regs, BST, 0);
+	/* Read back registers to confirm writes have fully propagated. */
+	renesas_readl(i3c->regs, BST);
+}
+
 static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
 {
 	unsigned long time_left;
@@ -1014,6 +1029,11 @@ static irqreturn_t renesas_i3c_tx_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			renesas_i3c_irqs_mask_and_clear_locked(i3c);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
@@ -1054,6 +1074,11 @@ static irqreturn_t renesas_i3c_resp_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			renesas_i3c_irqs_mask_and_clear_locked(i3c);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		/* Clear the Respone Queue Full status flag*/
@@ -1138,6 +1163,11 @@ static irqreturn_t renesas_i3c_tend_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			renesas_i3c_irqs_mask_and_clear_locked(i3c);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
@@ -1184,6 +1214,11 @@ static irqreturn_t renesas_i3c_rx_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			renesas_i3c_irqs_mask_and_clear_locked(i3c);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
@@ -1234,15 +1269,13 @@ static irqreturn_t renesas_i3c_stop_isr(int irq, void *data)
 	struct renesas_i3c_xfer *xfer;
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
-		xfer = i3c->xferqueue.cur;
-
-		/* read back registers to confirm writes have fully propagated */
-		renesas_writel(i3c->regs, BST, 0);
-		renesas_readl(i3c->regs, BST);
-		renesas_writel(i3c->regs, BIE, 0);
-		renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0);
+		renesas_i3c_irqs_mask_and_clear_locked(i3c);
 		renesas_clear_bit(i3c->regs, SCSTRCTL, SCSTRCTL_RWE);
 
+		xfer = i3c->xferqueue.cur;
+		if (!xfer)
+			return IRQ_HANDLED;
+
 		xfer->ret = 0;
 		complete(&xfer->comp);
 	}
@@ -1259,6 +1292,11 @@ static irqreturn_t renesas_i3c_start_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			renesas_i3c_irqs_mask_and_clear_locked(i3c);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
-- 
2.43.0

[PATCH v5 01/17] i3c: renesas: Check that the transfer is valid before accessing it

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The Renesas I3C driver uses an asynchronous model to transfer data. It
prepares a struct renesas_i3c_xfer, enqueues it, and waits for completion.
The interrupt handler dequeues the transfer, updates/uses it, and signals
the waiting thread.

If the completion times out, the waiting thread dequeues the transfer and
free it. If an interrupt fires after that, the handler may access freed
memory, leading to crashes.

Check that the transfer is still valid before accessing it in the
interrupt handler. With it clear any status flags and disable all
the interrupts to avoid triggering the same interrupts again.

Fixes: d028219a9f14 ("i3c: master: Add basic driver for the Renesas I3C controller")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- introduced renesas_i3c_irqs_mask_and_clear_locked() that keeps
  unified the IRQ mask and clean path
- updated the patch description

Changes in v4:
- disable also the interrupts
- dropped the Rb tag

Changes in v3:
- none

Changes in v2:
- clean the IRQ status bits before returning IRQ_HANDLED and adjusted the
  patch description to reflect this change
- collected Frank's tag. Frank, please let me know if you consider
  I should drop your tag. Thanks!

 drivers/i3c/master/renesas-i3c.c | 52 +++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 7 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index f39c449922ca..38b8428f464c 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -433,6 +433,21 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
 	}
 }
 
+static void renesas_i3c_irqs_mask_and_clear_locked(struct renesas_i3c *i3c)
+{
+	/* Disable all the interrupts. */
+	renesas_writel(i3c->regs, BIE, 0);
+	renesas_writel(i3c->regs, NTIE, 0);
+
+	/* Clear normal transfer status flags. */
+	renesas_writel(i3c->regs, NTST, 0);
+
+	/* Clear bus status flags. */
+	renesas_writel(i3c->regs, BST, 0);
+	/* Read back registers to confirm writes have fully propagated. */
+	renesas_readl(i3c->regs, BST);
+}
+
 static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
 {
 	unsigned long time_left;
@@ -1014,6 +1029,11 @@ static irqreturn_t renesas_i3c_tx_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			renesas_i3c_irqs_mask_and_clear_locked(i3c);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
@@ -1054,6 +1074,11 @@ static irqreturn_t renesas_i3c_resp_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			renesas_i3c_irqs_mask_and_clear_locked(i3c);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		/* Clear the Respone Queue Full status flag*/
@@ -1138,6 +1163,11 @@ static irqreturn_t renesas_i3c_tend_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			renesas_i3c_irqs_mask_and_clear_locked(i3c);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
@@ -1184,6 +1214,11 @@ static irqreturn_t renesas_i3c_rx_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			renesas_i3c_irqs_mask_and_clear_locked(i3c);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
@@ -1234,15 +1269,13 @@ static irqreturn_t renesas_i3c_stop_isr(int irq, void *data)
 	struct renesas_i3c_xfer *xfer;
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
-		xfer = i3c->xferqueue.cur;
-
-		/* read back registers to confirm writes have fully propagated */
-		renesas_writel(i3c->regs, BST, 0);
-		renesas_readl(i3c->regs, BST);
-		renesas_writel(i3c->regs, BIE, 0);
-		renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0);
+		renesas_i3c_irqs_mask_and_clear_locked(i3c);
 		renesas_clear_bit(i3c->regs, SCSTRCTL, SCSTRCTL_RWE);
 
+		xfer = i3c->xferqueue.cur;
+		if (!xfer)
+			return IRQ_HANDLED;
+
 		xfer->ret = 0;
 		complete(&xfer->comp);
 	}
@@ -1259,6 +1292,11 @@ static irqreturn_t renesas_i3c_start_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			renesas_i3c_irqs_mask_and_clear_locked(i3c);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
-- 
2.43.0


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

Re: [PATCH v5 01/17] i3c: renesas: Check that the transfer is valid before accessing it

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The I3C and I2C transfer functions ignore hardware timeouts and errors, erroneously returning success (0) and continuing operations.
- [High] DAA transfer timeouts are treated as successful assignments, registering phantom devices on the bus.
- [High] The patch's NULL-deref fix is incomplete: failing to abort the hardware on timeout allows delayed interrupts to corrupt subsequent enqueued transfers.
--

--- Patch [1]: [PATCH v5 01/17] i3c: renesas: Check that the transfer is valid before accessing it ---
commit 34ed2a5d1d43de932d3b546c4674b92c1ec5bc03
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Check that the transfer is valid before accessing it

The Renesas I3C driver uses an asynchronous model to transfer data. It
prepares a struct renesas_i3c_xfer, enqueues it, and waits for completion.
The interrupt handler dequeues the transfer, updates/uses it, and signals
the waiting thread.

If the completion times out, the waiting thread dequeues the transfer and
free it. If an interrupt fires after that, the handler may access freed
memory, leading to crashes.

Check that the transfer is still valid before accessing it in the
interrupt handler. With it clear any status flags and disable all
the interrupts to avoid triggering the same interrupts again.

Fixes: d028219a9f14 ("i3c: master: Add basic driver for the Renesas I3C controller")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f39c449922ca2..38b8428f464c3 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1014,6 +1029,11 @@ static irqreturn_t renesas_i3c_tx_isr(int irq, void *data)
>  
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			renesas_i3c_irqs_mask_and_clear_locked(i3c);
> +			return IRQ_HANDLED;
> +		}
> +
>  		cmd = xfer->cmds;

[Severity: High]
This is a pre-existing issue, but does this fix leave the hardware in a state
where delayed interrupts can corrupt subsequent transfers?

If a transfer times out in renesas_i3c_wait_xfer(), it is dequeued but the
hardware is not aborted:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_wait_xfer() {
    ...
	time_left = wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000));
	if (!time_left)
		renesas_i3c_dequeue_xfer(i3c, xfer);
}

If a new transfer B is enqueued and becomes i3c->xferqueue.cur, and then the
delayed interrupt for the timed-out transfer A fires, xfer will not be NULL.

The ISR would then process transfer A's hardware events against transfer B's
commands, which could lead to cross-transfer data corruption.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing I noticed that
hardware timeouts and errors appear to be ignored in the I3C and I2C transfer
functions, which could lead to data corruption or protocol failures.

In renesas_i3c_i3c_xfers(), the error state in xfer->ret is ignored entirely
and the function unconditionally returns 0:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers() {
    ...
		renesas_i3c_wait_xfer(i3c, xfer);
	}

	return 0;
}

Does this code hide failures by falsely assuming operations succeeded?

Similarly, in renesas_i3c_i2c_xfers(), wait_for_completion_timeout() returns 0
on timeout, but cmd->err remains 0 (its zero-initialized value):

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
    ...
		wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);

		if (cmd->err)
			break;
    ...
}

Is it possible for this loop to continue operations and eventually return 0
despite a hardware timeout?

[Severity: High]
This is another pre-existing issue, but I also noticed that DAA transfer
timeouts might be treated as successful assignments.

In renesas_i3c_daa(), if the DAA command times out, cmd->rx_count remains 0:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_daa() {
    ...
	renesas_i3c_wait_xfer(i3c, xfer);

	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
	newdevs &= ~olddevs;

	for (pos = 0; pos < i3c->maxdevs; pos++) {
		if (newdevs & BIT(pos))
			i3c_master_add_i3c_dev_locked(m, i3c->addrs[pos]);
	}
    ...
}

Does this generate a full mask of new devices and incorrectly register them on
the bus when the bus hangs or a device fails DAA?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=1

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

Re: [PATCH v5 01/17] i3c: renesas: Check that the transfer is valid before accessing it

[Frank Li]

On Mon, Jul 13, 2026 at 04:05:29PM +0300, Claudiu Beznea wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The Renesas I3C driver uses an asynchronous model to transfer data. It
> prepares a struct renesas_i3c_xfer, enqueues it, and waits for completion.
> The interrupt handler dequeues the transfer, updates/uses it, and signals
> the waiting thread.
>
> If the completion times out, the waiting thread dequeues the transfer and
> free it. If an interrupt fires after that, the handler may access freed
> memory, leading to crashes.
>
> Check that the transfer is still valid before accessing it in the
> interrupt handler. With it clear any status flags and disable all
> the interrupts to avoid triggering the same interrupts again.
>
> Fixes: d028219a9f14 ("i3c: master: Add basic driver for the Renesas I3C controller")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Changes in v5:
> - introduced renesas_i3c_irqs_mask_and_clear_locked() that keeps
>   unified the IRQ mask and clean path
> - updated the patch description
>
> Changes in v4:
> - disable also the interrupts
> - dropped the Rb tag
>
> Changes in v3:
> - none
>
> Changes in v2:
> - clean the IRQ status bits before returning IRQ_HANDLED and adjusted the
>   patch description to reflect this change
> - collected Frank's tag. Frank, please let me know if you consider
>   I should drop your tag. Thanks!
>
>  drivers/i3c/master/renesas-i3c.c | 52 +++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f39c449922ca..38b8428f464c 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -433,6 +433,21 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
>  	}
>  }
>
> +static void renesas_i3c_irqs_mask_and_clear_locked(struct renesas_i3c *i3c)
> +{
> +	/* Disable all the interrupts. */
> +	renesas_writel(i3c->regs, BIE, 0);
> +	renesas_writel(i3c->regs, NTIE, 0);
> +
> +	/* Clear normal transfer status flags. */
> +	renesas_writel(i3c->regs, NTST, 0);
> +
> +	/* Clear bus status flags. */
> +	renesas_writel(i3c->regs, BST, 0);
> +	/* Read back registers to confirm writes have fully propagated. */
> +	renesas_readl(i3c->regs, BST);
> +}
> +
>  static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
>  {
>  	unsigned long time_left;
> @@ -1014,6 +1029,11 @@ static irqreturn_t renesas_i3c_tx_isr(int irq, void *data)
>
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			renesas_i3c_irqs_mask_and_clear_locked(i3c);
> +			return IRQ_HANDLED;
> +		}
> +
>  		cmd = xfer->cmds;
>
>  		if (xfer->is_i2c_xfer) {
> @@ -1054,6 +1074,11 @@ static irqreturn_t renesas_i3c_resp_isr(int irq, void *data)
>
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			renesas_i3c_irqs_mask_and_clear_locked(i3c);
> +			return IRQ_HANDLED;
> +		}
> +
>  		cmd = xfer->cmds;
>
>  		/* Clear the Respone Queue Full status flag*/
> @@ -1138,6 +1163,11 @@ static irqreturn_t renesas_i3c_tend_isr(int irq, void *data)
>
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			renesas_i3c_irqs_mask_and_clear_locked(i3c);
> +			return IRQ_HANDLED;
> +		}
> +
>  		cmd = xfer->cmds;
>
>  		if (xfer->is_i2c_xfer) {
> @@ -1184,6 +1214,11 @@ static irqreturn_t renesas_i3c_rx_isr(int irq, void *data)
>
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			renesas_i3c_irqs_mask_and_clear_locked(i3c);
> +			return IRQ_HANDLED;
> +		}
> +
>  		cmd = xfer->cmds;
>
>  		if (xfer->is_i2c_xfer) {
> @@ -1234,15 +1269,13 @@ static irqreturn_t renesas_i3c_stop_isr(int irq, void *data)
>  	struct renesas_i3c_xfer *xfer;
>
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
> -		xfer = i3c->xferqueue.cur;
> -
> -		/* read back registers to confirm writes have fully propagated */
> -		renesas_writel(i3c->regs, BST, 0);
> -		renesas_readl(i3c->regs, BST);
> -		renesas_writel(i3c->regs, BIE, 0);
> -		renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0);
> +		renesas_i3c_irqs_mask_and_clear_locked(i3c);
>  		renesas_clear_bit(i3c->regs, SCSTRCTL, SCSTRCTL_RWE);
>
> +		xfer = i3c->xferqueue.cur;
> +		if (!xfer)
> +			return IRQ_HANDLED;
> +
>  		xfer->ret = 0;
>  		complete(&xfer->comp);
>  	}
> @@ -1259,6 +1292,11 @@ static irqreturn_t renesas_i3c_start_isr(int irq, void *data)
>
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			renesas_i3c_irqs_mask_and_clear_locked(i3c);
> +			return IRQ_HANDLED;
> +		}
> +
>  		cmd = xfer->cmds;
>
>  		if (xfer->is_i2c_xfer) {
> --
> 2.43.0
>

Re: [PATCH v5 01/17] i3c: renesas: Check that the transfer is valid before accessing it

[Frank Li]

On Mon, Jul 13, 2026 at 04:05:29PM +0300, Claudiu Beznea wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The Renesas I3C driver uses an asynchronous model to transfer data. It
> prepares a struct renesas_i3c_xfer, enqueues it, and waits for completion.
> The interrupt handler dequeues the transfer, updates/uses it, and signals
> the waiting thread.
>
> If the completion times out, the waiting thread dequeues the transfer and
> free it. If an interrupt fires after that, the handler may access freed
> memory, leading to crashes.
>
> Check that the transfer is still valid before accessing it in the
> interrupt handler. With it clear any status flags and disable all
> the interrupts to avoid triggering the same interrupts again.
>
> Fixes: d028219a9f14 ("i3c: master: Add basic driver for the Renesas I3C controller")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Changes in v5:
> - introduced renesas_i3c_irqs_mask_and_clear_locked() that keeps
>   unified the IRQ mask and clean path
> - updated the patch description
>
> Changes in v4:
> - disable also the interrupts
> - dropped the Rb tag
>
> Changes in v3:
> - none
>
> Changes in v2:
> - clean the IRQ status bits before returning IRQ_HANDLED and adjusted the
>   patch description to reflect this change
> - collected Frank's tag. Frank, please let me know if you consider
>   I should drop your tag. Thanks!
>
>  drivers/i3c/master/renesas-i3c.c | 52 +++++++++++++++++++++++++++-----
>  1 file changed, 45 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f39c449922ca..38b8428f464c 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -433,6 +433,21 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
>  	}
>  }
>
> +static void renesas_i3c_irqs_mask_and_clear_locked(struct renesas_i3c *i3c)
> +{
> +	/* Disable all the interrupts. */
> +	renesas_writel(i3c->regs, BIE, 0);
> +	renesas_writel(i3c->regs, NTIE, 0);
> +
> +	/* Clear normal transfer status flags. */
> +	renesas_writel(i3c->regs, NTST, 0);
> +
> +	/* Clear bus status flags. */
> +	renesas_writel(i3c->regs, BST, 0);
> +	/* Read back registers to confirm writes have fully propagated. */
> +	renesas_readl(i3c->regs, BST);
> +}
> +
>  static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
>  {
>  	unsigned long time_left;
> @@ -1014,6 +1029,11 @@ static irqreturn_t renesas_i3c_tx_isr(int irq, void *data)
>
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			renesas_i3c_irqs_mask_and_clear_locked(i3c);
> +			return IRQ_HANDLED;
> +		}
> +
>  		cmd = xfer->cmds;
>
>  		if (xfer->is_i2c_xfer) {
> @@ -1054,6 +1074,11 @@ static irqreturn_t renesas_i3c_resp_isr(int irq, void *data)
>
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			renesas_i3c_irqs_mask_and_clear_locked(i3c);
> +			return IRQ_HANDLED;
> +		}
> +
>  		cmd = xfer->cmds;
>
>  		/* Clear the Respone Queue Full status flag*/
> @@ -1138,6 +1163,11 @@ static irqreturn_t renesas_i3c_tend_isr(int irq, void *data)
>
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			renesas_i3c_irqs_mask_and_clear_locked(i3c);
> +			return IRQ_HANDLED;
> +		}
> +
>  		cmd = xfer->cmds;
>
>  		if (xfer->is_i2c_xfer) {
> @@ -1184,6 +1214,11 @@ static irqreturn_t renesas_i3c_rx_isr(int irq, void *data)
>
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			renesas_i3c_irqs_mask_and_clear_locked(i3c);
> +			return IRQ_HANDLED;
> +		}
> +
>  		cmd = xfer->cmds;
>
>  		if (xfer->is_i2c_xfer) {
> @@ -1234,15 +1269,13 @@ static irqreturn_t renesas_i3c_stop_isr(int irq, void *data)
>  	struct renesas_i3c_xfer *xfer;
>
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
> -		xfer = i3c->xferqueue.cur;
> -
> -		/* read back registers to confirm writes have fully propagated */
> -		renesas_writel(i3c->regs, BST, 0);
> -		renesas_readl(i3c->regs, BST);
> -		renesas_writel(i3c->regs, BIE, 0);
> -		renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0);
> +		renesas_i3c_irqs_mask_and_clear_locked(i3c);
>  		renesas_clear_bit(i3c->regs, SCSTRCTL, SCSTRCTL_RWE);
>
> +		xfer = i3c->xferqueue.cur;
> +		if (!xfer)
> +			return IRQ_HANDLED;
> +
>  		xfer->ret = 0;
>  		complete(&xfer->comp);
>  	}
> @@ -1259,6 +1292,11 @@ static irqreturn_t renesas_i3c_start_isr(int irq, void *data)
>
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			renesas_i3c_irqs_mask_and_clear_locked(i3c);
> +			return IRQ_HANDLED;
> +		}
> +
>  		cmd = xfer->cmds;
>
>  		if (xfer->is_i2c_xfer) {
> --
> 2.43.0
>

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

[PATCH v5 02/17] i3c: renesas: Restore STDBR and EXTBR registers on resume

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The Renesas RZ/G3S supports a power saving state where power to the most
SoC componentes (including I3C) is lost.

The STDBR and EXTBR are configured in initialization phase though the
struct i3c_master_controller_ops::bus_init. Set them on resume function
as well to keep the same state of the controller after a suspend with
power loss and a similar initialization sequence as in bus_init.

Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 38b8428f464c..cd9928649c7f 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -260,6 +260,7 @@ struct renesas_i3c {
 	u32 dyn_addr;
 	u32 i2c_STDBR;
 	u32 i3c_STDBR;
+	u32 extbr;
 	unsigned long rate;
 	u8 addrs[RENESAS_I3C_MAX_DEVS];
 	struct renesas_i3c_xferqueue xferqueue;
@@ -622,10 +623,9 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
 	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
 
 	/* Extended Bit Rate setting */
-	renesas_writel(i3c->regs, EXTBR, EXTBR_EBRLO(od_low_ticks) |
-					   EXTBR_EBRHO(od_high_ticks) |
-					   EXTBR_EBRLP(pp_low_ticks) |
-					   EXTBR_EBRHP(pp_high_ticks));
+	i3c->extbr = EXTBR_EBRLO(od_low_ticks) | EXTBR_EBRHO(od_high_ticks) |
+		     EXTBR_EBRLP(pp_low_ticks) | EXTBR_EBRHP(pp_high_ticks);
+	renesas_writel(i3c->regs, EXTBR, i3c->extbr);
 
 	renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
 	i3c->refclk_div = cks;
@@ -1468,6 +1468,8 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 		goto err_tresetn;
 
 	/* Re-store I3C registers value. */
+	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
+	renesas_writel(i3c->regs, EXTBR, i3c->extbr);
 	renesas_writel(i3c->regs, REFCKCTL,
 		       REFCKCTL_IREFCKS(i3c->refclk_div));
 	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYADV |
-- 
2.43.0

[PATCH v5 02/17] i3c: renesas: Restore STDBR and EXTBR registers on resume

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The Renesas RZ/G3S supports a power saving state where power to the most
SoC componentes (including I3C) is lost.

The STDBR and EXTBR are configured in initialization phase though the
struct i3c_master_controller_ops::bus_init. Set them on resume function
as well to keep the same state of the controller after a suspend with
power loss and a similar initialization sequence as in bus_init.

Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 38b8428f464c..cd9928649c7f 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -260,6 +260,7 @@ struct renesas_i3c {
 	u32 dyn_addr;
 	u32 i2c_STDBR;
 	u32 i3c_STDBR;
+	u32 extbr;
 	unsigned long rate;
 	u8 addrs[RENESAS_I3C_MAX_DEVS];
 	struct renesas_i3c_xferqueue xferqueue;
@@ -622,10 +623,9 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
 	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
 
 	/* Extended Bit Rate setting */
-	renesas_writel(i3c->regs, EXTBR, EXTBR_EBRLO(od_low_ticks) |
-					   EXTBR_EBRHO(od_high_ticks) |
-					   EXTBR_EBRLP(pp_low_ticks) |
-					   EXTBR_EBRHP(pp_high_ticks));
+	i3c->extbr = EXTBR_EBRLO(od_low_ticks) | EXTBR_EBRHO(od_high_ticks) |
+		     EXTBR_EBRLP(pp_low_ticks) | EXTBR_EBRHP(pp_high_ticks);
+	renesas_writel(i3c->regs, EXTBR, i3c->extbr);
 
 	renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
 	i3c->refclk_div = cks;
@@ -1468,6 +1468,8 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 		goto err_tresetn;
 
 	/* Re-store I3C registers value. */
+	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
+	renesas_writel(i3c->regs, EXTBR, i3c->extbr);
 	renesas_writel(i3c->regs, REFCKCTL,
 		       REFCKCTL_IREFCKS(i3c->refclk_div));
 	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYADV |
-- 
2.43.0


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

Re: [PATCH v5 02/17] i3c: renesas: Restore STDBR and EXTBR registers on resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] EXTBR timing registers are silently truncated when SCL low/high ticks exceed 255.
- [High] Hardware resets are unconditionally deasserted before clocks are enabled during system resume.
--

--- Patch [2]: [PATCH v5 02/17] i3c: renesas: Restore STDBR and EXTBR registers on resume ---
commit 33917325ffb6281761a5f99c8b41bb972ad950bd
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Restore STDBR and EXTBR registers on resume

This commit adds an extbr field to the renesas_i3c struct to cache the
Extended Bit Rate configuration during initialization. It then restores
this value, along with the Standard Bit Rate register, during the device
resume sequence to retain the controller's state after power loss.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 38b8428f464c3..cd9928649c7f7 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> @@ -622,10 +623,9 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
>  	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
>  
>  	/* Extended Bit Rate setting */
> -	renesas_writel(i3c->regs, EXTBR, EXTBR_EBRLO(od_low_ticks) |
> -					   EXTBR_EBRHO(od_high_ticks) |
> -					   EXTBR_EBRLP(pp_low_ticks) |
> -					   EXTBR_EBRHP(pp_high_ticks));
> +	i3c->extbr = EXTBR_EBRLO(od_low_ticks) | EXTBR_EBRHO(od_high_ticks) |
> +		     EXTBR_EBRLP(pp_low_ticks) | EXTBR_EBRHP(pp_high_ticks);
> +	renesas_writel(i3c->regs, EXTBR, i3c->extbr);

[Severity: High]
This isn't a bug introduced by this patch, but caching this value here means
we are storing a corrupted register value to restore on system resume.

When the calculated od_low_ticks or od_high_ticks exceed 255 (which sets
the double_SBR bit for STDBR), does EXTBR_EBRLO silently truncate the
upper bits?

Unlike STDBR_SBRLO which shifts the value down by double_SBR before
masking, EXTBR_EBRLO passes the value directly to
FIELD_PREP(GENMASK(7, 0), x).

Since od_low_ticks can be up to 510, this would drop the high bit and
result in malformed Extended Bit Rate (EXTBR) timings.

>  
>  	renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
>  	i3c->refclk_div = cks;

[ ... ]

> @@ -1468,6 +1468,8 @@ static int renesas_i3c_resume_noirq(struct device *dev)
>  		goto err_tresetn;
>  
>  	/* Re-store I3C registers value. */
> +	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
> +	renesas_writel(i3c->regs, EXTBR, i3c->extbr);

[Severity: High]
This is a pre-existing issue, but since it is modifying register restores
during system resume, are we doing this in the correct order with respect
to clocks?

Looking slightly earlier in renesas_i3c_resume_noirq():

renesas_i3c_resume_noirq() {
        ...
        ret = reset_control_deassert(i3c->presetn);
        if (ret)
                return ret;

        ret = reset_control_deassert(i3c->tresetn);
        if (ret)
                goto err_presetn;

        ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
        ...
}

If the hardware resets are synchronous, could deasserting them while
clocks are disabled leave the controller in an undefined state or prevent
proper recovery during resume?

>  	renesas_writel(i3c->regs, REFCKCTL,
>  		       REFCKCTL_IREFCKS(i3c->refclk_div));
>  	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYADV |

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=2

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

[PATCH v5 03/17] i3c: renesas: Follow the reset deassert order used in probe

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Use the same reset deassert order in the resume and probe paths to avoid
potential failures due to ordering differences.

Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index cd9928649c7f..ccf55afcdedc 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1455,17 +1455,17 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 	struct renesas_i3c *i3c = dev_get_drvdata(dev);
 	int i, ret;
 
-	ret = reset_control_deassert(i3c->presetn);
+	ret = reset_control_deassert(i3c->tresetn);
 	if (ret)
 		return ret;
 
-	ret = reset_control_deassert(i3c->tresetn);
+	ret = reset_control_deassert(i3c->presetn);
 	if (ret)
-		goto err_presetn;
+		goto err_tresetn;
 
 	ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
 	if (ret)
-		goto err_tresetn;
+		goto err_presetn;
 
 	/* Re-store I3C registers value. */
 	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
@@ -1486,10 +1486,10 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 
 	return 0;
 
-err_tresetn:
-	reset_control_assert(i3c->tresetn);
 err_presetn:
 	reset_control_assert(i3c->presetn);
+err_tresetn:
+	reset_control_assert(i3c->tresetn);
 	return ret;
 }
 
-- 
2.43.0

[PATCH v5 03/17] i3c: renesas: Follow the reset deassert order used in probe

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Use the same reset deassert order in the resume and probe paths to avoid
potential failures due to ordering differences.

Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index cd9928649c7f..ccf55afcdedc 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1455,17 +1455,17 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 	struct renesas_i3c *i3c = dev_get_drvdata(dev);
 	int i, ret;
 
-	ret = reset_control_deassert(i3c->presetn);
+	ret = reset_control_deassert(i3c->tresetn);
 	if (ret)
 		return ret;
 
-	ret = reset_control_deassert(i3c->tresetn);
+	ret = reset_control_deassert(i3c->presetn);
 	if (ret)
-		goto err_presetn;
+		goto err_tresetn;
 
 	ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
 	if (ret)
-		goto err_tresetn;
+		goto err_presetn;
 
 	/* Re-store I3C registers value. */
 	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
@@ -1486,10 +1486,10 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 
 	return 0;
 
-err_tresetn:
-	reset_control_assert(i3c->tresetn);
 err_presetn:
 	reset_control_assert(i3c->presetn);
+err_tresetn:
+	reset_control_assert(i3c->tresetn);
 	return ret;
 }
 
-- 
2.43.0


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

Re: [PATCH v5 03/17] i3c: renesas: Follow the reset deassert order used in probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The resume sequence deasserts resets before enabling clocks, which contradicts the probe sequence and violates standard hardware initialization ordering.
--

--- Patch [3]: [PATCH v5 03/17] i3c: renesas: Follow the reset deassert order used in probe ---
commit 1f6a035090d6207c3bc14c55b1513d8d3cf0afcd
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Follow the reset deassert order used in probe

This commit reorders the deassertion of the presetn and tresetn resets
during resume to match the order used in the probe path. This prevents
potential failures caused by ordering differences.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index cd9928649c7f7..ccf55afcdedc7 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> @@ -1455,17 +1455,17 @@ static int renesas_i3c_resume_noirq(struct device *dev)
>  	struct renesas_i3c *i3c = dev_get_drvdata(dev);
>  	int i, ret;
>  
> -	ret = reset_control_deassert(i3c->presetn);
> +	ret = reset_control_deassert(i3c->tresetn);
>  	if (ret)
>  		return ret;
>  
> -	ret = reset_control_deassert(i3c->tresetn);
> +	ret = reset_control_deassert(i3c->presetn);
>  	if (ret)
> -		goto err_presetn;
> +		goto err_tresetn;
>  
>  	ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
>  	if (ret)
> -		goto err_tresetn;
> +		goto err_presetn;

[Severity: High]
This is a pre-existing issue, but is it safe to deassert the synchronous 
resets before the clocks are enabled in renesas_i3c_resume_noirq()?

Deasserting synchronous resets while the peripheral's clock is disabled can
prevent the internal state machine from correctly exiting the reset state.

Subsequent register accesses to a peripheral that has failed to initialize 
or is unclocked can cause an asynchronous external abort (system hang) or 
leave the I3C bus non-functional.

Could the clock enablement step be moved before the reset deassertion to
fully mirror the correct initialization order?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=3

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

[PATCH v5 04/17] i3c: renesas: Reconfigure the DATBAS register on re-attach

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

During re-attach, the device may change its position in the i3c->addrs[]
array. As a result, it may use a different Device Address Table Basic
Register (DATBAS), which needs to be reconfigured.

Reconfigure the DATBAS register on re-attach. Along with it update
software caches.

Fixes: d028219a9f14 ("i3c: master: Add basic driver for the Renesas I3C controller")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- use "data->index > pos" condition

Changes in v3:
- collected tags

Changes in v2:
- dropped the "if (pos < 0)" check in renesas_i3c_reattach_i3c_dev() to allow
  re-attaching in case of a full bus; along with it the condition to update
  the DATBAS register and software caches was updated to
  if (data->index != pos && pos >= 0)
- adjusted the patch title

 drivers/i3c/master/renesas-i3c.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index ccf55afcdedc..517ac2df9bd4 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -907,10 +907,26 @@ static int renesas_i3c_reattach_i3c_dev(struct i3c_dev_desc *dev,
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
+	int pos;
+
+	pos = renesas_i3c_get_free_pos(i3c);
+
+	if (data->index > pos && pos >= 0) {
+		renesas_writel(i3c->regs, DATBAS(data->index), 0);
+		i3c->addrs[data->index] = 0;
+		i3c->free_pos |= BIT(data->index);
+
+		data->index = pos;
+		i3c->free_pos &= ~BIT(data->index);
+	}
 
 	i3c->addrs[data->index] = dev->info.dyn_addr ? dev->info.dyn_addr :
 							dev->info.static_addr;
 
+	renesas_writel(i3c->regs, DATBAS(data->index),
+		       DATBAS_DVSTAD(dev->info.static_addr) |
+		       datbas_dvdyad_with_parity(i3c->addrs[data->index]));
+
 	return 0;
 }
 
-- 
2.43.0

[PATCH v5 04/17] i3c: renesas: Reconfigure the DATBAS register on re-attach

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

During re-attach, the device may change its position in the i3c->addrs[]
array. As a result, it may use a different Device Address Table Basic
Register (DATBAS), which needs to be reconfigured.

Reconfigure the DATBAS register on re-attach. Along with it update
software caches.

Fixes: d028219a9f14 ("i3c: master: Add basic driver for the Renesas I3C controller")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- use "data->index > pos" condition

Changes in v3:
- collected tags

Changes in v2:
- dropped the "if (pos < 0)" check in renesas_i3c_reattach_i3c_dev() to allow
  re-attaching in case of a full bus; along with it the condition to update
  the DATBAS register and software caches was updated to
  if (data->index != pos && pos >= 0)
- adjusted the patch title

 drivers/i3c/master/renesas-i3c.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index ccf55afcdedc..517ac2df9bd4 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -907,10 +907,26 @@ static int renesas_i3c_reattach_i3c_dev(struct i3c_dev_desc *dev,
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
+	int pos;
+
+	pos = renesas_i3c_get_free_pos(i3c);
+
+	if (data->index > pos && pos >= 0) {
+		renesas_writel(i3c->regs, DATBAS(data->index), 0);
+		i3c->addrs[data->index] = 0;
+		i3c->free_pos |= BIT(data->index);
+
+		data->index = pos;
+		i3c->free_pos &= ~BIT(data->index);
+	}
 
 	i3c->addrs[data->index] = dev->info.dyn_addr ? dev->info.dyn_addr :
 							dev->info.static_addr;
 
+	renesas_writel(i3c->regs, DATBAS(data->index),
+		       DATBAS_DVSTAD(dev->info.static_addr) |
+		       datbas_dvdyad_with_parity(i3c->addrs[data->index]));
+
 	return 0;
 }
 
-- 
2.43.0


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

[PATCH v5 05/17] i3c: renesas: Reset the controller on resume

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reset the controller on resume after enabling the clocks to follow the
same sequence as in probe and avoid potential ordering related failures.

With it, renesas_i3c_reset() was updated to use read_poll_timeout_atomic(),
as the driver's resume callback is executed during the noirq phase of
resume, where interrupts are disabled.

Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- collected tags

Changes in v2:
- replaced the read_poll_timeout() in renesas_i3c_reset() with
  read_poll_timeout_atomic() as the renesas_i3c_reset() is called
  in noirq phase of the suspend/resume; updated the patch description
  to reflect that
- collected Frank's tag. Frank, please let me know if this should be
  dropped. Thanks!

 drivers/i3c/master/renesas-i3c.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 517ac2df9bd4..6590da962592 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -495,8 +495,8 @@ static int renesas_i3c_reset(struct renesas_i3c *i3c)
 	renesas_writel(i3c->regs, BCTL, 0);
 	renesas_set_bit(i3c->regs, RSTCTL, RSTCTL_RI3CRST);
 
-	return read_poll_timeout(renesas_readl, val, !(val & RSTCTL_RI3CRST),
-				 0, 1000, false, i3c->regs, RSTCTL);
+	return read_poll_timeout_atomic(renesas_readl, val, !(val & RSTCTL_RI3CRST),
+					0, 1000, false, i3c->regs, RSTCTL);
 }
 
 static void renesas_i3c_hw_init(struct renesas_i3c *i3c)
@@ -1483,6 +1483,10 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 	if (ret)
 		goto err_presetn;
 
+	ret = renesas_i3c_reset(i3c);
+	if (ret)
+		goto err_clks_disable;
+
 	/* Re-store I3C registers value. */
 	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
 	renesas_writel(i3c->regs, EXTBR, i3c->extbr);
@@ -1502,6 +1506,8 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 
 	return 0;
 
+err_clks_disable:
+	clk_bulk_disable(i3c->num_clks, i3c->clks);
 err_presetn:
 	reset_control_assert(i3c->presetn);
 err_tresetn:
-- 
2.43.0

[PATCH v5 05/17] i3c: renesas: Reset the controller on resume

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reset the controller on resume after enabling the clocks to follow the
same sequence as in probe and avoid potential ordering related failures.

With it, renesas_i3c_reset() was updated to use read_poll_timeout_atomic(),
as the driver's resume callback is executed during the noirq phase of
resume, where interrupts are disabled.

Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- collected tags

Changes in v2:
- replaced the read_poll_timeout() in renesas_i3c_reset() with
  read_poll_timeout_atomic() as the renesas_i3c_reset() is called
  in noirq phase of the suspend/resume; updated the patch description
  to reflect that
- collected Frank's tag. Frank, please let me know if this should be
  dropped. Thanks!

 drivers/i3c/master/renesas-i3c.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 517ac2df9bd4..6590da962592 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -495,8 +495,8 @@ static int renesas_i3c_reset(struct renesas_i3c *i3c)
 	renesas_writel(i3c->regs, BCTL, 0);
 	renesas_set_bit(i3c->regs, RSTCTL, RSTCTL_RI3CRST);
 
-	return read_poll_timeout(renesas_readl, val, !(val & RSTCTL_RI3CRST),
-				 0, 1000, false, i3c->regs, RSTCTL);
+	return read_poll_timeout_atomic(renesas_readl, val, !(val & RSTCTL_RI3CRST),
+					0, 1000, false, i3c->regs, RSTCTL);
 }
 
 static void renesas_i3c_hw_init(struct renesas_i3c *i3c)
@@ -1483,6 +1483,10 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 	if (ret)
 		goto err_presetn;
 
+	ret = renesas_i3c_reset(i3c);
+	if (ret)
+		goto err_clks_disable;
+
 	/* Re-store I3C registers value. */
 	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
 	renesas_writel(i3c->regs, EXTBR, i3c->extbr);
@@ -1502,6 +1506,8 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 
 	return 0;
 
+err_clks_disable:
+	clk_bulk_disable(i3c->num_clks, i3c->clks);
 err_presetn:
 	reset_control_assert(i3c->presetn);
 err_tresetn:
-- 
2.43.0


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

Re: [PATCH v5 05/17] i3c: renesas: Reset the controller on resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Hardware reset lines are deasserted before controller clocks are enabled during system resume.
--

--- Patch [5]: [PATCH v5 05/17] i3c: renesas: Reset the controller on resume ---
commit 9230535f3beec14a9429fad84d54b26dbaff5182
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Reset the controller on resume

This commit updates the resume handler to reset the controller after enabling
the clocks to match the probe sequence. It also switches renesas_i3c_reset()
to use read_poll_timeout_atomic() since the resume callback runs in the noirq
phase.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 517ac2df9bd40..6590da962592a 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -1483,6 +1483,10 @@ static int renesas_i3c_resume_noirq(struct device *dev)
>  	if (ret)
>  		goto err_presetn;
>  
> +	ret = renesas_i3c_reset(i3c);
> +	if (ret)
> +		goto err_clks_disable;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the sequence in
renesas_i3c_resume_noirq() correctly handle hardware constraints?

Looking at the code just above this hunk, the resets are deasserted
before the clocks are enabled:

	ret = reset_control_deassert(i3c->tresetn);
	if (ret)
		return ret;

	ret = reset_control_deassert(i3c->presetn);
	if (ret)
		goto err_tresetn;

	ret = clk_bulk_enable(i3c->num_clks, i3c->clks);

Deasserting peripheral resets while its clocks are disabled can cause the
hardware to remain stuck in reset or enter an undefined, non-functional
state when the clocks are subsequently enabled. Could this leave the I3C
master unresponsive after resume?

>  	/* Re-store I3C registers value. */
>  	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
>  	renesas_writel(i3c->regs, EXTBR, i3c->extbr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=5

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

[PATCH v5 06/17] i3c: renesas: Perform Dynamic Address Assignment on resume

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The Renesas RZ/G3S SoC supports a power saving mode where power to most
SoC components, including I3C, is turned off.

On systems where the I3C devices also loses power during suspend (e.g. NXP
P3T1085UK-ARD connected to the PMOD1_6A connector of the RZ SMARC Carrier
2 + Renesas RZ/G3S SMARC SOM), the devices becomes unreachable after
resume.

Running DAA in the controller resume path restores communication. However,
DAA relies on interrupts for TX/RX, which are not available in the noirq
suspend/resume phase (unless they are wakeup interrupts). For this, the
suspend/resume callbacks were moved out of the noirq phase. Currently,
there is no identified use case on either the Renesas RZ/G3S or Renesas
RZ/G3E SoCs that requires the controller suspend/resume hooks to be part of
the noirq suspend/resume phase.

Since renesas_i3c_reset() is not called anymore in atomic context
update it to use read_poll_timeout().

Along with this, struct renesas_i3c::DATBASn and its usage were removed,
as they are no longer needed.

Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- restore it to the level of v1; the other scenarios updated by
  sashiko were already present w/ and w/o this patch and could be
  addressed incrementally
- updated the patch description

Changes in v4:
- used directly i3c_dev instead of i3c_dev->dev->desc
- fixed the swap in renesas_i3c_group_devs_in_slots() for i3c->addr[]

Changes in v3:
- added renesas_i3c_group_devs_in_slots(); along with it, the
  struct renesas_i3c_addr was updated with i3c_dev and i3c_dev
  and the attach/detach/re-attach APIs were adjusted accordingly
- dropped DATBASn member of struct renesas_i3c
- used i3c_master_reattach_i3c_dev_locked() to re-attach devices
  on a fully occupied bus
- in resume, moved i2c_mark_adapter_resumed() after i3c_master_do_daa_ext()
  since it can update the internal driver data structure i2c specific

Changes in v2:
- adjusted the code to still work in case the full bus was occupied before
  a suspend/resume cycle; for that:
-- introduced struct renesas_i3c_addr
-- preserved i3c->DATBASn[] which is saved in suspend and used in resume,
   in renesas_i3c_daa()
- updated the patch description to reflect the new updates

 drivers/i3c/master/renesas-i3c.c | 38 +++++++++++++-------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 6590da962592..acc30ed615ab 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -265,7 +265,6 @@ struct renesas_i3c {
 	u8 addrs[RENESAS_I3C_MAX_DEVS];
 	struct renesas_i3c_xferqueue xferqueue;
 	void __iomem *regs;
-	u32 *DATBASn;
 	struct clk_bulk_data *clks;
 	struct reset_control *presetn;
 	struct reset_control *tresetn;
@@ -495,8 +494,8 @@ static int renesas_i3c_reset(struct renesas_i3c *i3c)
 	renesas_writel(i3c->regs, BCTL, 0);
 	renesas_set_bit(i3c->regs, RSTCTL, RSTCTL_RI3CRST);
 
-	return read_poll_timeout_atomic(renesas_readl, val, !(val & RSTCTL_RI3CRST),
-					0, 1000, false, i3c->regs, RSTCTL);
+	return read_poll_timeout(renesas_readl, val, !(val & RSTCTL_RI3CRST),
+				 0, 1000, false, i3c->regs, RSTCTL);
 }
 
 static void renesas_i3c_hw_init(struct renesas_i3c *i3c)
@@ -1419,12 +1418,6 @@ static int renesas_i3c_probe(struct platform_device *pdev)
 	i3c->maxdevs = RENESAS_I3C_MAX_DEVS;
 	i3c->free_pos = GENMASK(i3c->maxdevs - 1, 0);
 
-	/* Allocate dynamic Device Address Table backup. */
-	i3c->DATBASn = devm_kzalloc(&pdev->dev, sizeof(u32) * i3c->maxdevs,
-				    GFP_KERNEL);
-	if (!i3c->DATBASn)
-		return -ENOMEM;
-
 	return i3c_master_register(&i3c->base, &pdev->dev, &renesas_i3c_ops, false);
 }
 
@@ -1435,17 +1428,13 @@ static void renesas_i3c_remove(struct platform_device *pdev)
 	i3c_master_unregister(&i3c->base);
 }
 
-static int renesas_i3c_suspend_noirq(struct device *dev)
+static int renesas_i3c_suspend(struct device *dev)
 {
 	struct renesas_i3c *i3c = dev_get_drvdata(dev);
-	int i, ret;
+	int ret;
 
 	i2c_mark_adapter_suspended(&i3c->base.i2c);
 
-	/* Store Device Address Table values. */
-	for (i = 0; i < i3c->maxdevs; i++)
-		i3c->DATBASn[i] = renesas_readl(i3c->regs, DATBAS(i));
-
 	ret = reset_control_assert(i3c->presetn);
 	if (ret)
 		goto err_mark_resumed;
@@ -1466,10 +1455,10 @@ static int renesas_i3c_suspend_noirq(struct device *dev)
 	return ret;
 }
 
-static int renesas_i3c_resume_noirq(struct device *dev)
+static int renesas_i3c_resume(struct device *dev)
 {
 	struct renesas_i3c *i3c = dev_get_drvdata(dev);
-	int i, ret;
+	int ret;
 
 	ret = reset_control_deassert(i3c->tresetn);
 	if (ret)
@@ -1495,15 +1484,19 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYADV |
 		       MSDVAD_MDYAD(i3c->dyn_addr));
 
-	/* Restore Device Address Table values. */
-	for (i = 0; i < i3c->maxdevs; i++)
-		renesas_writel(i3c->regs, DATBAS(i), i3c->DATBASn[i]);
-
 	/* I3C hw init. */
 	renesas_i3c_hw_init(i3c);
 
+	ret = i3c_master_do_daa_ext(&i3c->base, true);
+	if (ret)
+		dev_err(dev, "DAA failed on resume, ret=%d", ret);
+
 	i2c_mark_adapter_resumed(&i3c->base.i2c);
 
+	/*
+	 * I3C devices may have retained their dynamic address anyway. Do not
+	 * fail the resume because of DAA error.
+	 */
 	return 0;
 
 err_clks_disable:
@@ -1516,8 +1509,7 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 }
 
 static const struct dev_pm_ops renesas_i3c_pm_ops = {
-	NOIRQ_SYSTEM_SLEEP_PM_OPS(renesas_i3c_suspend_noirq,
-				  renesas_i3c_resume_noirq)
+	SYSTEM_SLEEP_PM_OPS(renesas_i3c_suspend, renesas_i3c_resume)
 };
 
 static const struct of_device_id renesas_i3c_of_ids[] = {
-- 
2.43.0

[PATCH v5 06/17] i3c: renesas: Perform Dynamic Address Assignment on resume

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The Renesas RZ/G3S SoC supports a power saving mode where power to most
SoC components, including I3C, is turned off.

On systems where the I3C devices also loses power during suspend (e.g. NXP
P3T1085UK-ARD connected to the PMOD1_6A connector of the RZ SMARC Carrier
2 + Renesas RZ/G3S SMARC SOM), the devices becomes unreachable after
resume.

Running DAA in the controller resume path restores communication. However,
DAA relies on interrupts for TX/RX, which are not available in the noirq
suspend/resume phase (unless they are wakeup interrupts). For this, the
suspend/resume callbacks were moved out of the noirq phase. Currently,
there is no identified use case on either the Renesas RZ/G3S or Renesas
RZ/G3E SoCs that requires the controller suspend/resume hooks to be part of
the noirq suspend/resume phase.

Since renesas_i3c_reset() is not called anymore in atomic context
update it to use read_poll_timeout().

Along with this, struct renesas_i3c::DATBASn and its usage were removed,
as they are no longer needed.

Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- restore it to the level of v1; the other scenarios updated by
  sashiko were already present w/ and w/o this patch and could be
  addressed incrementally
- updated the patch description

Changes in v4:
- used directly i3c_dev instead of i3c_dev->dev->desc
- fixed the swap in renesas_i3c_group_devs_in_slots() for i3c->addr[]

Changes in v3:
- added renesas_i3c_group_devs_in_slots(); along with it, the
  struct renesas_i3c_addr was updated with i3c_dev and i3c_dev
  and the attach/detach/re-attach APIs were adjusted accordingly
- dropped DATBASn member of struct renesas_i3c
- used i3c_master_reattach_i3c_dev_locked() to re-attach devices
  on a fully occupied bus
- in resume, moved i2c_mark_adapter_resumed() after i3c_master_do_daa_ext()
  since it can update the internal driver data structure i2c specific

Changes in v2:
- adjusted the code to still work in case the full bus was occupied before
  a suspend/resume cycle; for that:
-- introduced struct renesas_i3c_addr
-- preserved i3c->DATBASn[] which is saved in suspend and used in resume,
   in renesas_i3c_daa()
- updated the patch description to reflect the new updates

 drivers/i3c/master/renesas-i3c.c | 38 +++++++++++++-------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 6590da962592..acc30ed615ab 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -265,7 +265,6 @@ struct renesas_i3c {
 	u8 addrs[RENESAS_I3C_MAX_DEVS];
 	struct renesas_i3c_xferqueue xferqueue;
 	void __iomem *regs;
-	u32 *DATBASn;
 	struct clk_bulk_data *clks;
 	struct reset_control *presetn;
 	struct reset_control *tresetn;
@@ -495,8 +494,8 @@ static int renesas_i3c_reset(struct renesas_i3c *i3c)
 	renesas_writel(i3c->regs, BCTL, 0);
 	renesas_set_bit(i3c->regs, RSTCTL, RSTCTL_RI3CRST);
 
-	return read_poll_timeout_atomic(renesas_readl, val, !(val & RSTCTL_RI3CRST),
-					0, 1000, false, i3c->regs, RSTCTL);
+	return read_poll_timeout(renesas_readl, val, !(val & RSTCTL_RI3CRST),
+				 0, 1000, false, i3c->regs, RSTCTL);
 }
 
 static void renesas_i3c_hw_init(struct renesas_i3c *i3c)
@@ -1419,12 +1418,6 @@ static int renesas_i3c_probe(struct platform_device *pdev)
 	i3c->maxdevs = RENESAS_I3C_MAX_DEVS;
 	i3c->free_pos = GENMASK(i3c->maxdevs - 1, 0);
 
-	/* Allocate dynamic Device Address Table backup. */
-	i3c->DATBASn = devm_kzalloc(&pdev->dev, sizeof(u32) * i3c->maxdevs,
-				    GFP_KERNEL);
-	if (!i3c->DATBASn)
-		return -ENOMEM;
-
 	return i3c_master_register(&i3c->base, &pdev->dev, &renesas_i3c_ops, false);
 }
 
@@ -1435,17 +1428,13 @@ static void renesas_i3c_remove(struct platform_device *pdev)
 	i3c_master_unregister(&i3c->base);
 }
 
-static int renesas_i3c_suspend_noirq(struct device *dev)
+static int renesas_i3c_suspend(struct device *dev)
 {
 	struct renesas_i3c *i3c = dev_get_drvdata(dev);
-	int i, ret;
+	int ret;
 
 	i2c_mark_adapter_suspended(&i3c->base.i2c);
 
-	/* Store Device Address Table values. */
-	for (i = 0; i < i3c->maxdevs; i++)
-		i3c->DATBASn[i] = renesas_readl(i3c->regs, DATBAS(i));
-
 	ret = reset_control_assert(i3c->presetn);
 	if (ret)
 		goto err_mark_resumed;
@@ -1466,10 +1455,10 @@ static int renesas_i3c_suspend_noirq(struct device *dev)
 	return ret;
 }
 
-static int renesas_i3c_resume_noirq(struct device *dev)
+static int renesas_i3c_resume(struct device *dev)
 {
 	struct renesas_i3c *i3c = dev_get_drvdata(dev);
-	int i, ret;
+	int ret;
 
 	ret = reset_control_deassert(i3c->tresetn);
 	if (ret)
@@ -1495,15 +1484,19 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYADV |
 		       MSDVAD_MDYAD(i3c->dyn_addr));
 
-	/* Restore Device Address Table values. */
-	for (i = 0; i < i3c->maxdevs; i++)
-		renesas_writel(i3c->regs, DATBAS(i), i3c->DATBASn[i]);
-
 	/* I3C hw init. */
 	renesas_i3c_hw_init(i3c);
 
+	ret = i3c_master_do_daa_ext(&i3c->base, true);
+	if (ret)
+		dev_err(dev, "DAA failed on resume, ret=%d", ret);
+
 	i2c_mark_adapter_resumed(&i3c->base.i2c);
 
+	/*
+	 * I3C devices may have retained their dynamic address anyway. Do not
+	 * fail the resume because of DAA error.
+	 */
 	return 0;
 
 err_clks_disable:
@@ -1516,8 +1509,7 @@ static int renesas_i3c_resume_noirq(struct device *dev)
 }
 
 static const struct dev_pm_ops renesas_i3c_pm_ops = {
-	NOIRQ_SYSTEM_SLEEP_PM_OPS(renesas_i3c_suspend_noirq,
-				  renesas_i3c_resume_noirq)
+	SYSTEM_SLEEP_PM_OPS(renesas_i3c_suspend, renesas_i3c_resume)
 };
 
 static const struct of_device_id renesas_i3c_of_ids[] = {
-- 
2.43.0


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

Re: [PATCH v5 06/17] i3c: renesas: Perform Dynamic Address Assignment on resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removal of the DATBASn save/restore mechanism completely breaks the I3C bus upon resume for previously attached devices, and fails to resume if the bus was fully occupied.
--

--- Patch [6]: [PATCH v5 06/17] i3c: renesas: Perform Dynamic Address Assignment on resume ---
commit e4ac838c055dc33beb2ff4e8968a032da48a2a47
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Perform Dynamic Address Assignment on resume

This commit updates the suspend/resume operations by moving them out of
the noirq phase and attempting to perform Dynamic Address Assignment on
resume. It also removes the save and restore of the DATBASn registers.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> @@ -1495,15 +1484,19 @@ static int renesas_i3c_resume_noirq(struct device *dev)
>  	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYADV |
>  		       MSDVAD_MDYAD(i3c->dyn_addr));
>  
> -	/* Restore Device Address Table values. */
> -	for (i = 0; i < i3c->maxdevs; i++)
> -		renesas_writel(i3c->regs, DATBAS(i), i3c->DATBASn[i]);
> -
>  	/* I3C hw init. */
>  	renesas_i3c_hw_init(i3c);
>  
> +	ret = i3c_master_do_daa_ext(&i3c->base, true);

[Severity: High]
Does removing the DATBASn save and restore mechanism break communication
with previously attached devices upon resume?

Looking at renesas_i3c_daa(), it explicitly skips hardware register
initialization for previously attached devices:

renesas_i3c_daa() {
        ...
        if (olddevs & BIT(pos))
                continue;
        ...
}

Since the controller was reset during suspend, won't these hardware
registers remain uninitialized (0) if DATBASn isn't manually restored here?

Additionally, if the bus was fully occupied before suspend, will this cause
the DAA process to abort entirely?

renesas_i3c_daa() {
        ...
        ret = renesas_i3c_get_free_pos(i3c);
        if (ret < 0)
                return ret;
        ...
}

If the bus is full, free_pos would be 0, causing this to return -ENOSPC
immediately. This aborts before the ENTDAA command is sent, which would
leave the previously attached devices permanently stranded without dynamic
addresses.

> +	if (ret)
> +		dev_err(dev, "DAA failed on resume, ret=%d", ret);
> +
>  	i2c_mark_adapter_resumed(&i3c->base.i2c);
>  
> +	/*
> +	 * I3C devices may have retained their dynamic address anyway. Do not
> +	 * fail the resume because of DAA error.
> +	 */
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=6

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

Re: [PATCH v5 06/17] i3c: renesas: Perform Dynamic Address Assignment on resume

[Frank Li]

On Mon, Jul 13, 2026 at 01:50:32PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Removal of the DATBASn save/restore mechanism completely breaks the I3C bus upon resume for previously attached devices, and fails to resume if the bus was fully occupied.
> --
>
> --- Patch [6]: [PATCH v5 06/17] i3c: renesas: Perform Dynamic Address Assignment on resume ---
> commit e4ac838c055dc33beb2ff4e8968a032da48a2a47
> Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> i3c: renesas: Perform Dynamic Address Assignment on resume
>
> This commit updates the suspend/resume operations by moving them out of
> the noirq phase and attempting to perform Dynamic Address Assignment on
> resume. It also removes the save and restore of the DATBASn registers.
>
> > diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> > --- a/drivers/i3c/master/renesas-i3c.c
> > +++ b/drivers/i3c/master/renesas-i3c.c
>
> [ ... ]
>
> > @@ -1495,15 +1484,19 @@ static int renesas_i3c_resume_noirq(struct device *dev)
> >  	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYADV |
> >  		       MSDVAD_MDYAD(i3c->dyn_addr));
> >
> > -	/* Restore Device Address Table values. */
> > -	for (i = 0; i < i3c->maxdevs; i++)
> > -		renesas_writel(i3c->regs, DATBAS(i), i3c->DATBASn[i]);
> > -
> >  	/* I3C hw init. */
> >  	renesas_i3c_hw_init(i3c);
> >
> > +	ret = i3c_master_do_daa_ext(&i3c->base, true);
>
> [Severity: High]
> Does removing the DATBASn save and restore mechanism break communication
> with previously attached devices upon resume?
>
> Looking at renesas_i3c_daa(), it explicitly skips hardware register
> initialization for previously attached devices:
>
> renesas_i3c_daa() {
>         ...
>         if (olddevs & BIT(pos))
>                 continue;
>         ...
> }
>
> Since the controller was reset during suspend, won't these hardware
> registers remain uninitialized (0) if DATBASn isn't manually restored here?
>
> Additionally, if the bus was fully occupied before suspend, will this cause
> the DAA process to abort entirely?
>
> renesas_i3c_daa() {
>         ...
>         ret = renesas_i3c_get_free_pos(i3c);
>         if (ret < 0)
>                 return ret;
>         ...
> }
>
> If the bus is full, free_pos would be 0, causing this to return -ENOSPC
> immediately. This aborts before the ENTDAA command is sent, which would
> leave the previously attached devices permanently stranded without dynamic
> addresses.

Please check this sashiko review result?

Frank

>
> > +	if (ret)
> > +		dev_err(dev, "DAA failed on resume, ret=%d", ret);
> > +
> >  	i2c_mark_adapter_resumed(&i3c->base.i2c);
> >
> > +	/*
> > +	 * I3C devices may have retained their dynamic address anyway. Do not
> > +	 * fail the resume because of DAA error.
> > +	 */
> >  	return 0;
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=6
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

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

Re: [PATCH v5 06/17] i3c: renesas: Perform Dynamic Address Assignment on resume

[Claudiu Beznea]

On 7/13/26 19:59, Frank Li wrote:
> On Mon, Jul 13, 2026 at 01:50:32PM +0000,sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [High] Removal of the DATBASn save/restore mechanism completely breaks the I3C bus upon resume for previously attached devices, and fails to resume if the bus was fully occupied.
>> --
>>
>> --- Patch [6]: [PATCH v5 06/17] i3c: renesas: Perform Dynamic Address Assignment on resume ---
>> commit e4ac838c055dc33beb2ff4e8968a032da48a2a47
>> Author: Claudiu Beznea<claudiu.beznea.uj@bp.renesas.com>
>>
>> i3c: renesas: Perform Dynamic Address Assignment on resume
>>
>> This commit updates the suspend/resume operations by moving them out of
>> the noirq phase and attempting to perform Dynamic Address Assignment on
>> resume. It also removes the save and restore of the DATBASn registers.
>>
>>> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
>>> --- a/drivers/i3c/master/renesas-i3c.c
>>> +++ b/drivers/i3c/master/renesas-i3c.c
>> [ ... ]
>>
>>> @@ -1495,15 +1484,19 @@ static int renesas_i3c_resume_noirq(struct device *dev)
>>>   	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYADV |
>>>   		       MSDVAD_MDYAD(i3c->dyn_addr));
>>>
>>> -	/* Restore Device Address Table values. */
>>> -	for (i = 0; i < i3c->maxdevs; i++)
>>> -		renesas_writel(i3c->regs, DATBAS(i), i3c->DATBASn[i]);
>>> -
>>>   	/* I3C hw init. */
>>>   	renesas_i3c_hw_init(i3c);
>>>
>>> +	ret = i3c_master_do_daa_ext(&i3c->base, true);
>> [Severity: High]
>> Does removing the DATBASn save and restore mechanism break communication
>> with previously attached devices upon resume?
>>
>> Looking at renesas_i3c_daa(), it explicitly skips hardware register
>> initialization for previously attached devices:
>>
>> renesas_i3c_daa() {
>>          ...
>>          if (olddevs & BIT(pos))
>>                  continue;
>>          ...
>> }
>>
>> Since the controller was reset during suspend, won't these hardware
>> registers remain uninitialized (0) if DATBASn isn't manually restored here?
>>
>> Additionally, if the bus was fully occupied before suspend, will this cause
>> the DAA process to abort entirely?
>>
>> renesas_i3c_daa() {
>>          ...
>>          ret = renesas_i3c_get_free_pos(i3c);
>>          if (ret < 0)
>>                  return ret;
>>          ...
>> }
>>
>> If the bus is full, free_pos would be 0, causing this to return -ENOSPC
>> immediately. This aborts before the ENTDAA command is sent, which would
>> leave the previously attached devices permanently stranded without dynamic
>> addresses.
> Please check this sashiko review result?

I tried to address this though the versions v2-v4 variant of this patch that 
I've posted previously but after more investigation and thinking it looked to me 
that the solution to this is not that simple and related only to this driver 
(see below).

Last time I've asked for some guidance [1] but got no input on it.

Apart from v4, I tried (before posting v5) to use separate free_pos list to be 
used while resuming, and still use only i3c_master_add_i3c_dev_locked() in 
renesas_i3c_daa() but that didn't work either, because the devices (present 
before suspend and) discovered at resume, ended up to be added to the new list 
then deleted by the code of i3c_master_add_i3c_dev_locked().

I have also thought about increasing the free_pos bitmask with one extra entry 
while resuming to allow attach, detach, reattach work (for the devices connected 
before suspend), but I don't think that is going to work either, and looks 
hackish to me.

I also tried calling i3c_master_detach_free_devs() (only the code for I3C 
devices) before running DAA on resume but that wasn't enough for the solution to 
work.

I think this issue reported by sashiko can be encountered (at some point) on all 
the drivers that track and limit the number of attached devices with a bitmask 
(or other mechanism) and check that bitmask in the ->attach_i3c_dev.

E.g., on the Renesas RZ/G3S I can end up to that point even w/o the code from 
this patch doing the following:
1/ connect 2 I3C devices to the bus (I'm using the NXP P3T1085UK-ARD with 2
    temperature sensors)
2/ after the 2 I3C devices are attached, simulate the I3C bus is
    full (no entries in the free_pos mask) (with the diff patch below)
3/ cut the power to the NXP P3T1085UK-ARD board
4/ connect the power to the NXP P3T1085UK-ARD
5/ at this point if I run do_daa the devices are not going to attach anymore

I can simulate this on my side. I think it is the case for any other driver 
following the above mentioned pattern for attaching.

That was the reason I have currently dropped the attempt to solve the problem 
highlighted by sashiko as its solution looks to me to be more complicated and 
involved more testing to be done, not only related to this driver.

Since the issue is not only related to this patch but more on how the attach 
procedure works, and can be reproduced w/o this patch, I considered only the 
approach in this patch was be enough for suspend/resume fix and later fixes 
should be added to cover this complicated scenario.

Please let me know if you have any suggestions on how to go forward with this 
problem.

This is the patch I used for simulating full bus:

 > git show
commit 01f99391ab3e (HEAD)
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Date:   Tue Jul 14 11:49:14 2026 +0300

     test

     Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index f1be38a640ca..4cb98328c8eb 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -810,6 +810,29 @@ static ssize_t do_daa_store(struct device *dev,

  static DEVICE_ATTR_WO(do_daa);

+bool i3c_full_bus = false;
+
+static ssize_t full_bus_store(struct device *dev,
+                           struct device_attribute *attr,
+                           const char *buf, size_t count)
+{
+       struct i3c_master_controller *master = dev_to_i3cmaster(dev);
+       bool val;
+       int ret;
+
+       i3c_full_bus = true;
+
+       return count;
+}
+
+static DEVICE_ATTR_WO(full_bus);
+
+bool i3c_get_full_bus(void)
+{
+       return i3c_full_bus;
+}
+EXPORT_SYMBOL_GPL(i3c_get_full_bus);
+
  static struct attribute *i3c_masterdev_attrs[] = {
         &dev_attr_mode.attr,
         &dev_attr_current_master.attr,
@@ -822,6 +845,7 @@ static struct attribute *i3c_masterdev_attrs[] = {
         &dev_attr_hdrcap.attr,
         &dev_attr_hotjoin.attr,
         &dev_attr_do_daa.attr,
+       &dev_attr_full_bus.attr,
         NULL,
  };
  ATTRIBUTE_GROUPS(i3c_masterdev);
diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 65151991b4ec..8e78ef602c51 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -324,6 +324,9 @@ static inline u32 datbas_dvdyad_with_parity(u8 addr)

  static int renesas_i3c_get_free_pos(struct renesas_i3c *i3c)
  {
+       if (i3c_get_full_bus())
+               return -ENOSPC;
+
         if (!(i3c->free_pos & GENMASK(i3c->maxdevs - 1, 0)))
                 return -ENOSPC;

@@ -689,6 +692,11 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)

         renesas_i3c_wait_xfer(i3c, xfer);

+       if (cmd->rx_count >= i3c->maxdevs)
+               newdevs = 0;
+       else
+               newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
+
         newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
         newdevs &= ~olddevs;

@@ -867,6 +875,7 @@ static int renesas_i3c_attach_i3c_dev(struct i3c_dev_desc *dev)
         struct renesas_i3c_i2c_dev_data *data;
         int pos;

+       pr_err("%s(): in\n", __func__);
         pos = renesas_i3c_get_free_pos(i3c);
         if (pos < 0)
                 return pos;
@@ -883,6 +892,7 @@ static int renesas_i3c_attach_i3c_dev(struct i3c_dev_desc *dev)
                                     datbas_dvdyad_with_parity(i3c->addrs[pos]));
         i3c_dev_set_master_data(dev, data);

+       pr_err("%s(): out\n", __func__);
         return 0;
  }

@@ -894,6 +904,8 @@ static int renesas_i3c_reattach_i3c_dev(struct i3c_dev_desc 
*dev,
         struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
         int pos;

+       pr_err("%s(): in\n", __func__);
+
         pos = renesas_i3c_get_free_pos(i3c);

         if (data->index > pos && pos >= 0) {
@@ -911,7 +923,7 @@ static int renesas_i3c_reattach_i3c_dev(struct i3c_dev_desc 
*dev,
         renesas_writel(i3c->regs, DATBAS(data->index),
                        DATBAS_DVSTAD(dev->info.static_addr) |
                        datbas_dvdyad_with_parity(i3c->addrs[data->index]));
-
+       pr_err("%s(): out\n", __func__);
         return 0;
  }

@@ -922,11 +934,13 @@ static void renesas_i3c_detach_i3c_dev(struct i3c_dev_desc 
*dev)
         struct renesas_i3c *i3c = to_renesas_i3c(m);

         renesas_writel(i3c->regs, DATBAS(data->index), 0);
+       pr_err("%s(): in\n", __func__);

         i3c_dev_set_master_data(dev, NULL);
         i3c->addrs[data->index] = 0;
         i3c->free_pos |= BIT(data->index);
         kfree(data);
+       pr_err("%s(): out\n", __func__);
  }

  static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 4d2a68793324..3ab31d85bff6 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -22,6 +22,8 @@
  #define I3C_BROADCAST_ADDR             0x7e
  #define I3C_MAX_ADDR                   GENMASK(6, 0)

+bool i3c_get_full_bus(void);
+
  struct i2c_client;

  /* notifier actions. notifier call data is the struct i3c_bus */


Applied on top of the fixes patches from this series except the one in this thread:

01f99391ab3e (HEAD) test
6d3e526d34a3 Revert "i3c: renesas: Perform Dynamic Address Assignment on resume"
7256e1a32469 i3c: renesas: Clean DATBAS register on detach
1c9e940709a2 i3c: renesas: Perform Dynamic Address Assignment on resume
305787a2f797 i3c: renesas: Reset the controller on resume
d6edb5c5d46a i3c: renesas: Reconfigure the DATBAS register on re-attach
88ab25e78d95 i3c: renesas: Follow the reset deassert order used in probe
80908175920f i3c: renesas: Restore STDBR and EXTBR registers on resume
455ff70955fc i3c: renesas: Check that the transfer is valid before accessing it
27bc7a253560 arm64: dts: renesas: rzg3s-smarc-som: Enable I3C
fd146e7c97f8 pinctrl: renesas: rzg2l: Add RZ/G3S support for selecting the I3C 
power source
277b0db8d168 dt-bindings: pinctrl: renesas,rzg2l-pinctrl: Document the missing 
I3C power source option
de297915b104 pinctrl: renesas: rzg2l: Drop defines present in struct rzg2l_hwcfg
e9a915ca8cc7 pinctrl: renesas: rzg2l: Generalize the power source code
34cf6dafc474 (tag: next-20260709, linux-next/master) Add linux-next specific 
files for 20260709

The steps I followed to reproduce the problem even w/o the patch in this series 
is at [2].

Thank you,
Claudiu

[1] https://lore.kernel.org/all/559b8b0c-412a-4d0f-9872-a4c92f3b9611@kernel.org/
[2] 
https://github.com/claudiubeznea/logs/blob/cc6a6e80936b8368a2187c72dd7e23a6a88b5a84/logs

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

[PATCH v5 07/17] i3c: renesas: Clean DATBAS register on detach

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The controller uses DATBAS registers on TX/RX logic. Clean the DATBAS
register for the detached I3C device to avoid issues.

Fixes: d028219a9f14 ("i3c: master: Add basic driver for the Renesas I3C controller")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index acc30ed615ab..b9784d238f61 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -935,6 +935,8 @@ static void renesas_i3c_detach_i3c_dev(struct i3c_dev_desc *dev)
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 
+	renesas_writel(i3c->regs, DATBAS(data->index), 0);
+
 	i3c_dev_set_master_data(dev, NULL);
 	i3c->addrs[data->index] = 0;
 	i3c->free_pos |= BIT(data->index);
-- 
2.43.0

[PATCH v5 07/17] i3c: renesas: Clean DATBAS register on detach

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The controller uses DATBAS registers on TX/RX logic. Clean the DATBAS
register for the detached I3C device to avoid issues.

Fixes: d028219a9f14 ("i3c: master: Add basic driver for the Renesas I3C controller")
Cc: stable@vger.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index acc30ed615ab..b9784d238f61 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -935,6 +935,8 @@ static void renesas_i3c_detach_i3c_dev(struct i3c_dev_desc *dev)
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 
+	renesas_writel(i3c->regs, DATBAS(data->index), 0);
+
 	i3c_dev_set_master_data(dev, NULL);
 	i3c->addrs[data->index] = 0;
 	i3c->free_pos |= BIT(data->index);
-- 
2.43.0


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

Re: [PATCH v5 07/17] i3c: renesas: Clean DATBAS register on detach

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The ENTDAA procedure in `renesas_i3c_daa` is incompatible with fragmented device indices, miscalculates masks, and leaks speculative `DATBAS` state.
- [High] Hardware transfer is not aborted on timeout and queued transfers are never started, leading to cross-transfer corruption and use-after-free.
--

--- Patch [7]: [PATCH v5 07/17] i3c: renesas: Clean DATBAS register on detach ---
commit c5598a36339bc89f9663008d7b023828e4ec62e5
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Clean DATBAS register on detach

The controller uses DATBAS registers on TX/RX logic. Clean the DATBAS
register for the detached I3C device to avoid issues.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index acc30ed615ab9..b9784d238f61c 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -935,6 +935,8 @@ static void renesas_i3c_detach_i3c_dev(struct i3c_dev_desc *dev)
>  	struct i3c_master_controller *m = i3c_dev_get_master(dev);
>  	struct renesas_i3c *i3c = to_renesas_i3c(m);
>  
> +	renesas_writel(i3c->regs, DATBAS(data->index), 0);
> +
>  	i3c_dev_set_master_data(dev, NULL);
>  	i3c->addrs[data->index] = 0;
>  	i3c->free_pos |= BIT(data->index);

While reviewing this patch, I noticed a couple of pre-existing issues in
drivers/i3c/master/renesas-i3c.c that might need attention.

In renesas_i3c_daa():

>         newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
>         newdevs &= ~olddevs;

[Severity: High]
This is a pre-existing issue, but does this miscalculate the mask if zero
devices join during DAA?

If zero devices join, cmd->rx_count equals i3c->maxdevs, which would make
GENMASK(maxdevs - rx_count - 1, 0) evaluate to GENMASK(-1, 0). Could this
underflow to ~0UL and falsely add phantom devices for every free position?

Also in renesas_i3c_daa():

>         NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
>         NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;

If free indices are fragmented (for example, after a detach like the one this
patch addresses), does this instruct the hardware to assign a sequential block
of devices? 

It appears this might override the DATBAS configuration of already-occupied
higher indices, resulting in duplicate I3C addresses. Furthermore, do
speculative DATBAS writes for indices that do not join get cleaned up, or
could they permanently pollute the controller's state?

In renesas_i3c_wait_xfer():

>         time_left = wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000));
>         if (!time_left)
>                 renesas_i3c_dequeue_xfer(i3c, xfer);

[Severity: High]
This isn't a bug introduced by this patch, but does this timeout path leave
the hardware actively processing the transfer?

The timeout removes the transfer from the software queue and allows its
memory to be freed, but if an abort isn't sent to the hardware, could this
create a use-after-free race?

In renesas_i3c_resp_isr():

>         xfer = list_first_entry_or_null(&i3c->xferqueue.list,
>                                         struct renesas_i3c_xfer, node);
>         if (xfer)
>                 list_del_init(&xfer->node);
> 
>         i3c->xferqueue.cur = xfer;

If the next transfer is popped from the queue and assigned to cur, but never
actually started in the hardware, will it permanently stall and guarantee a
timeout?

If a new transfer is later started and becomes cur, could the old delayed
hardware interrupt fire and process old events against the new cur transfer,
writing PIO data into the new buffer (or previously freed stack space)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=7

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

[PATCH v5 08/17] i3c: renesas: Fix out-of-bounds access for newdevs mask

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

When software initiates DAA (Dynamic Address Assignment), the controller
reports the result via the NRSPQP (Normal Response Queue Port Register).
The data length field of the response descriptor, which is accessible
through the NRSPQP register, indicates the number of devices remaining
after DAA. Consequently, when the bus is empty, this field contains the
maximum number of devices supported by the controller (8 for the Renesas
I3C controller).

Adjust the condition that computes the newly discovered devices bitmask
to prevent an out-of-bounds when the I3C bus is empty.

Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none; this patch is new

 drivers/i3c/master/renesas-i3c.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index b9784d238f61..c459e40fd5ff 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -703,7 +703,11 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 
 	renesas_i3c_wait_xfer(i3c, xfer);
 
-	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
+	if (cmd->rx_count >= i3c->maxdevs)
+		newdevs = 0;
+	else
+		newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
+
 	newdevs &= ~olddevs;
 
 	for (pos = 0; pos < i3c->maxdevs; pos++) {
-- 
2.43.0

[PATCH v5 08/17] i3c: renesas: Fix out-of-bounds access for newdevs mask

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

When software initiates DAA (Dynamic Address Assignment), the controller
reports the result via the NRSPQP (Normal Response Queue Port Register).
The data length field of the response descriptor, which is accessible
through the NRSPQP register, indicates the number of devices remaining
after DAA. Consequently, when the bus is empty, this field contains the
maximum number of devices supported by the controller (8 for the Renesas
I3C controller).

Adjust the condition that computes the newly discovered devices bitmask
to prevent an out-of-bounds when the I3C bus is empty.

Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none; this patch is new

 drivers/i3c/master/renesas-i3c.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index b9784d238f61..c459e40fd5ff 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -703,7 +703,11 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 
 	renesas_i3c_wait_xfer(i3c, xfer);
 
-	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
+	if (cmd->rx_count >= i3c->maxdevs)
+		newdevs = 0;
+	else
+		newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
+
 	newdevs &= ~olddevs;
 
 	for (pos = 0; pos < i3c->maxdevs; pos++) {
-- 
2.43.0


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

Re: [PATCH v5 08/17] i3c: renesas: Fix out-of-bounds access for newdevs mask

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver completely ignores transfer completion status and timeouts in critical I/O functions (`renesas_i3c_daa`, `renesas_i3c_i3c_xfers`, `renesas_i3c_i2c_xfers`), leading to silent failures, uninitialized kernel memory leaks, and race conditions.
- [High] The Dynamic Address Assignment (DAA) logic incorrectly calculates the `newdevs` bitmask using `GENMASK(..., 0)`, failing to account for non-contiguous (fragmented) index allocations.
- [High] Missing hardware reset on transfer timeout causes command/response FIFOs to desynchronize, corrupting subsequent transfers.
--

--- Patch [8]: [PATCH v5 08/17] i3c: renesas: Fix out-of-bounds access for newdevs mask ---
commit 141d263a8a62dfaec861ec3e856a94b76c38fa01
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Fix out-of-bounds access for newdevs mask

When software initiates DAA (Dynamic Address Assignment), the controller
reports the result via the NRSPQP (Normal Response Queue Port Register).
This patch adjusts the condition that computes the newly discovered devices
bitmask to prevent an out-of-bounds access when the I3C bus is empty.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index b9784d238f61c..c459e40fd5ffc 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -703,7 +703,11 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
>  
>  	renesas_i3c_wait_xfer(i3c, xfer);

[Severity: High]
This is a pre-existing issue, but does the driver need to check the
completion status of renesas_i3c_wait_xfer()?

If this transfer times out, cmd->rx_count remains 0, which could cause the
subsequent code to process phantom devices.

Similar unhandled timeouts appear in renesas_i3c_i3c_xfers(), where returning
0 unconditionally could leak uninitialized read buffers, and in
renesas_i3c_i2c_xfers(), where ignoring the timeout allows the loop to modify
cmd fields concurrently with pending hardware operations.

Additionally, what happens to the hardware state if a timeout occurs?

drivers/i3c/master/renesas-i3c.c:renesas_i3c_wait_xfer() {
    ...
    time_left = wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000));
    if (!time_left)
        renesas_i3c_dequeue_xfer(i3c, xfer);
    ...
}

If the transfer is dequeued from software on timeout without triggering a
hardware abort or reset, could the controller remain active? The next
enqueued software transfer might then be corrupted by the hardware completion
interrupt of the previous timed-out transfer.

> -	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
> +	if (cmd->rx_count >= i3c->maxdevs)
> +		newdevs = 0;
> +	else
> +		newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
> +
>  	newdevs &= ~olddevs;

[Severity: High]
This isn't a bug introduced by this patch, but does the calculation of
newdevs properly account for non-contiguous index allocations?

The Dynamic Address Assignment logic calculates the newdevs bitmask using
GENMASK(..., 0), which computes a bitmask strictly starting from index 0.

If a device is detached leaving a gap in free_pos (e.g., at index 0) while
higher indices are occupied, DAA sets DEV_INDEX to the first gap. Since
GENMASK(..., 0) ignores the DEV_INDEX offset, applying newdevs &= ~olddevs
could clear the incorrectly shifted bits corresponding to occupied slots.
Could this cause newly attached devices to be permanently dropped?

>  
>  	for (pos = 0; pos < i3c->maxdevs; pos++) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=8

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

Re: [PATCH v5 08/17] i3c: renesas: Fix out-of-bounds access for newdevs mask

[Frank Li]

On Mon, Jul 13, 2026 at 04:05:36PM +0300, Claudiu Beznea wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> When software initiates DAA (Dynamic Address Assignment), the controller
> reports the result via the NRSPQP (Normal Response Queue Port Register).
> The data length field of the response descriptor, which is accessible
> through the NRSPQP register, indicates the number of devices remaining
> after DAA. Consequently, when the bus is empty, this field contains the
> maximum number of devices supported by the controller (8 for the Renesas
> I3C controller).
>
> Adjust the condition that computes the newly discovered devices bitmask
> to prevent an out-of-bounds when the I3C bus is empty.
>
> Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Changes in v5:
> - none; this patch is new
>
>  drivers/i3c/master/renesas-i3c.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index b9784d238f61..c459e40fd5ff 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -703,7 +703,11 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
>
>  	renesas_i3c_wait_xfer(i3c, xfer);
>
> -	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
> +	if (cmd->rx_count >= i3c->maxdevs)
> +		newdevs = 0;
> +	else
> +		newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
> +
>  	newdevs &= ~olddevs;
>
>  	for (pos = 0; pos < i3c->maxdevs; pos++) {
> --
> 2.43.0
>

Re: [PATCH v5 08/17] i3c: renesas: Fix out-of-bounds access for newdevs mask

[Frank Li]

On Mon, Jul 13, 2026 at 04:05:36PM +0300, Claudiu Beznea wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> When software initiates DAA (Dynamic Address Assignment), the controller
> reports the result via the NRSPQP (Normal Response Queue Port Register).
> The data length field of the response descriptor, which is accessible
> through the NRSPQP register, indicates the number of devices remaining
> after DAA. Consequently, when the bus is empty, this field contains the
> maximum number of devices supported by the controller (8 for the Renesas
> I3C controller).
>
> Adjust the condition that computes the newly discovered devices bitmask
> to prevent an out-of-bounds when the I3C bus is empty.
>
> Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Changes in v5:
> - none; this patch is new
>
>  drivers/i3c/master/renesas-i3c.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index b9784d238f61..c459e40fd5ff 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -703,7 +703,11 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
>
>  	renesas_i3c_wait_xfer(i3c, xfer);
>
> -	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
> +	if (cmd->rx_count >= i3c->maxdevs)
> +		newdevs = 0;
> +	else
> +		newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);
> +
>  	newdevs &= ~olddevs;
>
>  	for (pos = 0; pos < i3c->maxdevs; pos++) {
> --
> 2.43.0
>

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

[PATCH v5 09/17] i3c: renesas: Use reset_control_bulk_{assert, deassert}()

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Use reset_control_bulk_assert() and reset_control_bulk_deassert() in the
suspend and resume paths to simplify the code.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- none

 drivers/i3c/master/renesas-i3c.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index c459e40fd5ff..915090d0ad37 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1437,24 +1437,22 @@ static void renesas_i3c_remove(struct platform_device *pdev)
 static int renesas_i3c_suspend(struct device *dev)
 {
 	struct renesas_i3c *i3c = dev_get_drvdata(dev);
+	struct reset_control_bulk_data resets[] = {
+		{ .rstc = i3c->presetn },
+		{ .rstc = i3c->tresetn },
+	};
 	int ret;
 
 	i2c_mark_adapter_suspended(&i3c->base.i2c);
 
-	ret = reset_control_assert(i3c->presetn);
+	ret = reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
 	if (ret)
 		goto err_mark_resumed;
 
-	ret = reset_control_assert(i3c->tresetn);
-	if (ret)
-		goto err_presetn;
-
 	clk_bulk_disable(i3c->num_clks, i3c->clks);
 
 	return 0;
 
-err_presetn:
-	reset_control_deassert(i3c->presetn);
 err_mark_resumed:
 	i2c_mark_adapter_resumed(&i3c->base.i2c);
 
@@ -1464,19 +1462,19 @@ static int renesas_i3c_suspend(struct device *dev)
 static int renesas_i3c_resume(struct device *dev)
 {
 	struct renesas_i3c *i3c = dev_get_drvdata(dev);
+	struct reset_control_bulk_data resets[] = {
+		{ .rstc = i3c->presetn },
+		{ .rstc = i3c->tresetn },
+	};
 	int ret;
 
-	ret = reset_control_deassert(i3c->tresetn);
+	ret = reset_control_bulk_deassert(ARRAY_SIZE(resets), resets);
 	if (ret)
 		return ret;
 
-	ret = reset_control_deassert(i3c->presetn);
-	if (ret)
-		goto err_tresetn;
-
 	ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
 	if (ret)
-		goto err_presetn;
+		goto err_resets_asserted;
 
 	ret = renesas_i3c_reset(i3c);
 	if (ret)
@@ -1507,10 +1505,8 @@ static int renesas_i3c_resume(struct device *dev)
 
 err_clks_disable:
 	clk_bulk_disable(i3c->num_clks, i3c->clks);
-err_presetn:
-	reset_control_assert(i3c->presetn);
-err_tresetn:
-	reset_control_assert(i3c->tresetn);
+err_resets_asserted:
+	reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
 	return ret;
 }
 
-- 
2.43.0

[PATCH v5 09/17] i3c: renesas: Use reset_control_bulk_{assert, deassert}()

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Use reset_control_bulk_assert() and reset_control_bulk_deassert() in the
suspend and resume paths to simplify the code.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- none

 drivers/i3c/master/renesas-i3c.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index c459e40fd5ff..915090d0ad37 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1437,24 +1437,22 @@ static void renesas_i3c_remove(struct platform_device *pdev)
 static int renesas_i3c_suspend(struct device *dev)
 {
 	struct renesas_i3c *i3c = dev_get_drvdata(dev);
+	struct reset_control_bulk_data resets[] = {
+		{ .rstc = i3c->presetn },
+		{ .rstc = i3c->tresetn },
+	};
 	int ret;
 
 	i2c_mark_adapter_suspended(&i3c->base.i2c);
 
-	ret = reset_control_assert(i3c->presetn);
+	ret = reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
 	if (ret)
 		goto err_mark_resumed;
 
-	ret = reset_control_assert(i3c->tresetn);
-	if (ret)
-		goto err_presetn;
-
 	clk_bulk_disable(i3c->num_clks, i3c->clks);
 
 	return 0;
 
-err_presetn:
-	reset_control_deassert(i3c->presetn);
 err_mark_resumed:
 	i2c_mark_adapter_resumed(&i3c->base.i2c);
 
@@ -1464,19 +1462,19 @@ static int renesas_i3c_suspend(struct device *dev)
 static int renesas_i3c_resume(struct device *dev)
 {
 	struct renesas_i3c *i3c = dev_get_drvdata(dev);
+	struct reset_control_bulk_data resets[] = {
+		{ .rstc = i3c->presetn },
+		{ .rstc = i3c->tresetn },
+	};
 	int ret;
 
-	ret = reset_control_deassert(i3c->tresetn);
+	ret = reset_control_bulk_deassert(ARRAY_SIZE(resets), resets);
 	if (ret)
 		return ret;
 
-	ret = reset_control_deassert(i3c->presetn);
-	if (ret)
-		goto err_tresetn;
-
 	ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
 	if (ret)
-		goto err_presetn;
+		goto err_resets_asserted;
 
 	ret = renesas_i3c_reset(i3c);
 	if (ret)
@@ -1507,10 +1505,8 @@ static int renesas_i3c_resume(struct device *dev)
 
 err_clks_disable:
 	clk_bulk_disable(i3c->num_clks, i3c->clks);
-err_presetn:
-	reset_control_assert(i3c->presetn);
-err_tresetn:
-	reset_control_assert(i3c->tresetn);
+err_resets_asserted:
+	reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
 	return ret;
 }
 
-- 
2.43.0


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

Re: [PATCH v5 09/17] i3c: renesas: Use reset_control_bulk_{assert, deassert}()

[Frank Li]

On Mon, Jul 13, 2026 at 04:05:37PM +0300, Claudiu Beznea wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Use reset_control_bulk_assert() and reset_control_bulk_deassert() in the
> suspend and resume paths to simplify the code.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Changes in v5:
> - none
>
> Changes in v4:
> - none
>
> Changes in v3:
> - none
>
> Changes in v2:
> - none
>
>  drivers/i3c/master/renesas-i3c.c | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index c459e40fd5ff..915090d0ad37 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1437,24 +1437,22 @@ static void renesas_i3c_remove(struct platform_device *pdev)
>  static int renesas_i3c_suspend(struct device *dev)
>  {
>  	struct renesas_i3c *i3c = dev_get_drvdata(dev);
> +	struct reset_control_bulk_data resets[] = {
> +		{ .rstc = i3c->presetn },
> +		{ .rstc = i3c->tresetn },
> +	};
>  	int ret;
>
>  	i2c_mark_adapter_suspended(&i3c->base.i2c);
>
> -	ret = reset_control_assert(i3c->presetn);
> +	ret = reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
>  	if (ret)
>  		goto err_mark_resumed;
>
> -	ret = reset_control_assert(i3c->tresetn);
> -	if (ret)
> -		goto err_presetn;
> -
>  	clk_bulk_disable(i3c->num_clks, i3c->clks);
>
>  	return 0;
>
> -err_presetn:
> -	reset_control_deassert(i3c->presetn);
>  err_mark_resumed:
>  	i2c_mark_adapter_resumed(&i3c->base.i2c);
>
> @@ -1464,19 +1462,19 @@ static int renesas_i3c_suspend(struct device *dev)
>  static int renesas_i3c_resume(struct device *dev)
>  {
>  	struct renesas_i3c *i3c = dev_get_drvdata(dev);
> +	struct reset_control_bulk_data resets[] = {
> +		{ .rstc = i3c->presetn },
> +		{ .rstc = i3c->tresetn },
> +	};
>  	int ret;
>
> -	ret = reset_control_deassert(i3c->tresetn);
> +	ret = reset_control_bulk_deassert(ARRAY_SIZE(resets), resets);
>  	if (ret)
>  		return ret;
>
> -	ret = reset_control_deassert(i3c->presetn);
> -	if (ret)
> -		goto err_tresetn;
> -
>  	ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
>  	if (ret)
> -		goto err_presetn;
> +		goto err_resets_asserted;
>
>  	ret = renesas_i3c_reset(i3c);
>  	if (ret)
> @@ -1507,10 +1505,8 @@ static int renesas_i3c_resume(struct device *dev)
>
>  err_clks_disable:
>  	clk_bulk_disable(i3c->num_clks, i3c->clks);
> -err_presetn:
> -	reset_control_assert(i3c->presetn);
> -err_tresetn:
> -	reset_control_assert(i3c->tresetn);
> +err_resets_asserted:
> +	reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
>  	return ret;
>  }
>
> --
> 2.43.0
>

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

Re: [PATCH v5 09/17] i3c: renesas: Use reset_control_bulk_{assert, deassert}()

[Frank Li]

On Mon, Jul 13, 2026 at 04:05:37PM +0300, Claudiu Beznea wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> Use reset_control_bulk_assert() and reset_control_bulk_deassert() in the
> suspend and resume paths to simplify the code.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>
> Changes in v5:
> - none
>
> Changes in v4:
> - none
>
> Changes in v3:
> - none
>
> Changes in v2:
> - none
>
>  drivers/i3c/master/renesas-i3c.c | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index c459e40fd5ff..915090d0ad37 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1437,24 +1437,22 @@ static void renesas_i3c_remove(struct platform_device *pdev)
>  static int renesas_i3c_suspend(struct device *dev)
>  {
>  	struct renesas_i3c *i3c = dev_get_drvdata(dev);
> +	struct reset_control_bulk_data resets[] = {
> +		{ .rstc = i3c->presetn },
> +		{ .rstc = i3c->tresetn },
> +	};
>  	int ret;
>
>  	i2c_mark_adapter_suspended(&i3c->base.i2c);
>
> -	ret = reset_control_assert(i3c->presetn);
> +	ret = reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
>  	if (ret)
>  		goto err_mark_resumed;
>
> -	ret = reset_control_assert(i3c->tresetn);
> -	if (ret)
> -		goto err_presetn;
> -
>  	clk_bulk_disable(i3c->num_clks, i3c->clks);
>
>  	return 0;
>
> -err_presetn:
> -	reset_control_deassert(i3c->presetn);
>  err_mark_resumed:
>  	i2c_mark_adapter_resumed(&i3c->base.i2c);
>
> @@ -1464,19 +1462,19 @@ static int renesas_i3c_suspend(struct device *dev)
>  static int renesas_i3c_resume(struct device *dev)
>  {
>  	struct renesas_i3c *i3c = dev_get_drvdata(dev);
> +	struct reset_control_bulk_data resets[] = {
> +		{ .rstc = i3c->presetn },
> +		{ .rstc = i3c->tresetn },
> +	};
>  	int ret;
>
> -	ret = reset_control_deassert(i3c->tresetn);
> +	ret = reset_control_bulk_deassert(ARRAY_SIZE(resets), resets);
>  	if (ret)
>  		return ret;
>
> -	ret = reset_control_deassert(i3c->presetn);
> -	if (ret)
> -		goto err_tresetn;
> -
>  	ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
>  	if (ret)
> -		goto err_presetn;
> +		goto err_resets_asserted;
>
>  	ret = renesas_i3c_reset(i3c);
>  	if (ret)
> @@ -1507,10 +1505,8 @@ static int renesas_i3c_resume(struct device *dev)
>
>  err_clks_disable:
>  	clk_bulk_disable(i3c->num_clks, i3c->clks);
> -err_presetn:
> -	reset_control_assert(i3c->presetn);
> -err_tresetn:
> -	reset_control_assert(i3c->tresetn);
> +err_resets_asserted:
> +	reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
>  	return ret;
>  }
>
> --
> 2.43.0
>

[PATCH v5 10/17] i3c: renesas: Return immediately if there is no transfer

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

There is no need to allocate a transfer structure when i2c_nxfers is zero.
Return immediately instead of unnecessarily allocating memory.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- updated patch title

 drivers/i3c/master/renesas-i3c.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 915090d0ad37..9a3613220034 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -957,13 +957,13 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 	u8 start_bit = CNDCTL_STCND;
 	int i;
 
+	if (!i2c_nxfers)
+		return 0;
+
 	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
 	if (!xfer)
 		return -ENOMEM;
 
-	if (!i2c_nxfers)
-		return 0;
-
 	renesas_i3c_bus_enable(m, false);
 
 	init_completion(&xfer->comp);
-- 
2.43.0

[PATCH v5 10/17] i3c: renesas: Return immediately if there is no transfer

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

There is no need to allocate a transfer structure when i2c_nxfers is zero.
Return immediately instead of unnecessarily allocating memory.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- updated patch title

 drivers/i3c/master/renesas-i3c.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 915090d0ad37..9a3613220034 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -957,13 +957,13 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 	u8 start_bit = CNDCTL_STCND;
 	int i;
 
+	if (!i2c_nxfers)
+		return 0;
+
 	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
 	if (!xfer)
 		return -ENOMEM;
 
-	if (!i2c_nxfers)
-		return 0;
-
 	renesas_i3c_bus_enable(m, false);
 
 	init_completion(&xfer->comp);
-- 
2.43.0


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

Re: [PATCH v5 10/17] i3c: renesas: Return immediately if there is no transfer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver ignores the return value of wait_for_completion_timeout() during I2C transfers, leading to data races and hardware state corruption.
- [High] The driver unconditionally returns success in I3C transfers regardless of hardware errors or timeouts.
- [High] A timeout during Dynamic Address Assignment (DAA) causes the driver to incorrectly register phantom devices.
- [High] Concurrent I3C and I2C transfers can clobber hardware registers due to missing driver-level serialization.
--

--- Patch [10]: [PATCH v5 10/17] i3c: renesas: Return immediately if there is no transfer ---
commit 10f5d1cd5ccdca9a32d38cbec0d2a0b8846661ac
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Return immediately if there is no transfer

This commit moves the check for zero transfers before allocating the transfer
structure. This avoids an unnecessary memory allocation and cleanup when there
are no transfers to process.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 915090d0ad37e..9a36132200342 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -957,13 +957,13 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
>  	u8 start_bit = CNDCTL_STCND;
>  	int i;
>  
> +	if (!i2c_nxfers)
> +		return 0;
> +
>  	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
>  	if (!xfer)
>  		return -ENOMEM;
>  
> -	if (!i2c_nxfers)
> -		return 0;
> -
>  	renesas_i3c_bus_enable(m, false);
>  
>  	init_completion(&xfer->comp);

[Severity: High]
This is a pre-existing issue, but since you are modifying renesas_i3c_i2c_xfers(),
does this function fail to handle timeouts correctly? 

If wait_for_completion_timeout() times out, its return value is discarded, and
cmd->err is likely 0:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
        ...
		wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);

		if (cmd->err)
			break;
        ...
}

Will this allow the loop to proceed to the next I2C message and immediately
clobber hardware registers for a new command while the hardware might still be
processing the timed-out command?


[Severity: High]
This is a pre-existing issue, but does renesas_i3c_i2c_xfers() also lack proper
driver-level serialization?

The function enqueues the transfer but immediately starts modifying hardware
registers (like CNDCTL) without waiting for its turn in the queue:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
        ...
		renesas_i3c_enqueue_xfer(i3c, xfer);

	for (i = 0; i < i2c_nxfers; i++) {
		...
		/* Issue Start condition */
		renesas_set_bit(i3c->regs, CNDCTL, start_bit);
        ...
}

Because the I3C core relies on i3c_bus_normaluse_lock() (a read lock), multiple
transfers can be requested concurrently. Can concurrent I2C and I3C transfers
clobber hardware registers?


[Severity: High]
This is a pre-existing issue, but the same lack of serialization appears to
exist in renesas_i3c_i3c_xfers():

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers() {
        ...
		if (!i3c_xfers[i].rnw && i3c_xfers[i].len > 4) {
			i3c_writel_fifo(i3c->regs + NTDTBP0, cmd->tx_buf, cmd->len);
			if (cmd->len > NTDTBP0_DEPTH * sizeof(u32))
				renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
		}

		renesas_i3c_wait_xfer(i3c, xfer);
        ...
}

Since the function unconditionally writes to the hardware FIFO before calling
enqueue/wait, won't this destroy the active transfer if another thread is
currently transmitting?


[Severity: High]
This is a pre-existing issue, but does renesas_i3c_i3c_xfers() unconditionally
return success?

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers() {
        ...
		renesas_i3c_wait_xfer(i3c, xfer);
	}

	return 0;
}

The xfer->ret value populated by renesas_i3c_wait_xfer() is never checked. Will
this lead to silent failures where hardware errors (like NACK or timeouts) are
ignored?


[Severity: High]
This is a pre-existing issue, but does renesas_i3c_daa() incorrectly register
phantom devices if a timeout occurs during Dynamic Address Assignment?

If the ENTDAA command times out, cmd->rx_count remains 0 and the timeout error
in xfer->ret is ignored:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_daa() {
        ...
		newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, 0);

	newdevs &= ~olddevs;

	for (pos = 0; pos < i3c->maxdevs; pos++) {
		if (newdevs & BIT(pos))
			i3c_master_add_i3c_dev_locked(m, i3c->addrs[pos]);
        ...
}

Because cmd->rx_count is 0, GENMASK evaluates to all bits. Will this cause the
driver to loop over this mask and register phantom I3C devices on the bus for
every unused address slot?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=10

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

[PATCH v5 11/17] i3c: renesas: Follow a unified pattern for transfer and command initialization

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Follow a unified pattern for transfer and command initialization across
the driver. This keeps the code cleaner and easier to follow. Also, in
some cases the I3C device was enabled before the transfer data structure
was even allocated.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 9a3613220034..c1396c49f45e 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -663,6 +663,10 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 	if (!xfer)
 		return -ENOMEM;
 
+	init_completion(&xfer->comp);
+	cmd = xfer->cmds;
+	cmd->rx_count = 0;
+
 	/* Enable I3C bus. */
 	renesas_i3c_bus_enable(m, true);
 
@@ -684,10 +688,6 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 		renesas_writel(i3c->regs, DATBAS(pos), datbas_dvdyad_with_parity(ret));
 	}
 
-	init_completion(&xfer->comp);
-	cmd = xfer->cmds;
-	cmd->rx_count = 0;
-
 	ret = renesas_i3c_get_free_pos(i3c);
 	if (ret < 0)
 		return ret;
@@ -779,13 +779,13 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
 	if (!xfer)
 		return -ENOMEM;
 
-	renesas_i3c_bus_enable(m, true);
-
 	init_completion(&xfer->comp);
 	cmd = xfer->cmds;
 	cmd->rnw = ccc->rnw;
 	cmd->cmd0 = 0;
 
+	renesas_i3c_bus_enable(m, true);
+
 	/* Calculate the command descriptor. */
 	switch (ccc->id) {
 	case I3C_CCC_SETDASA:
@@ -835,15 +835,15 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
 	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
 	int i;
 
-	/* Enable I3C bus. */
-	renesas_i3c_bus_enable(m, true);
-
 	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
 	if (!xfer)
 		return -ENOMEM;
 
 	init_completion(&xfer->comp);
 
+	/* Enable I3C bus. */
+	renesas_i3c_bus_enable(m, true);
+
 	for (i = 0; i < i3c_nxfers; i++) {
 		struct renesas_i3c_cmd *cmd = xfer->cmds;
 
@@ -964,12 +964,12 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 	if (!xfer)
 		return -ENOMEM;
 
-	renesas_i3c_bus_enable(m, false);
-
 	init_completion(&xfer->comp);
 	xfer->is_i2c_xfer = true;
 	cmd = xfer->cmds;
 
+	renesas_i3c_bus_enable(m, false);
+
 	if (!(renesas_readl(i3c->regs, BCST) & BCST_BFREF)) {
 		cmd->err = -EBUSY;
 		return cmd->err;
-- 
2.43.0

[PATCH v5 11/17] i3c: renesas: Follow a unified pattern for transfer and command initialization

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Follow a unified pattern for transfer and command initialization across
the driver. This keeps the code cleaner and easier to follow. Also, in
some cases the I3C device was enabled before the transfer data structure
was even allocated.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 9a3613220034..c1396c49f45e 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -663,6 +663,10 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 	if (!xfer)
 		return -ENOMEM;
 
+	init_completion(&xfer->comp);
+	cmd = xfer->cmds;
+	cmd->rx_count = 0;
+
 	/* Enable I3C bus. */
 	renesas_i3c_bus_enable(m, true);
 
@@ -684,10 +688,6 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 		renesas_writel(i3c->regs, DATBAS(pos), datbas_dvdyad_with_parity(ret));
 	}
 
-	init_completion(&xfer->comp);
-	cmd = xfer->cmds;
-	cmd->rx_count = 0;
-
 	ret = renesas_i3c_get_free_pos(i3c);
 	if (ret < 0)
 		return ret;
@@ -779,13 +779,13 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
 	if (!xfer)
 		return -ENOMEM;
 
-	renesas_i3c_bus_enable(m, true);
-
 	init_completion(&xfer->comp);
 	cmd = xfer->cmds;
 	cmd->rnw = ccc->rnw;
 	cmd->cmd0 = 0;
 
+	renesas_i3c_bus_enable(m, true);
+
 	/* Calculate the command descriptor. */
 	switch (ccc->id) {
 	case I3C_CCC_SETDASA:
@@ -835,15 +835,15 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
 	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
 	int i;
 
-	/* Enable I3C bus. */
-	renesas_i3c_bus_enable(m, true);
-
 	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
 	if (!xfer)
 		return -ENOMEM;
 
 	init_completion(&xfer->comp);
 
+	/* Enable I3C bus. */
+	renesas_i3c_bus_enable(m, true);
+
 	for (i = 0; i < i3c_nxfers; i++) {
 		struct renesas_i3c_cmd *cmd = xfer->cmds;
 
@@ -964,12 +964,12 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 	if (!xfer)
 		return -ENOMEM;
 
-	renesas_i3c_bus_enable(m, false);
-
 	init_completion(&xfer->comp);
 	xfer->is_i2c_xfer = true;
 	cmd = xfer->cmds;
 
+	renesas_i3c_bus_enable(m, false);
+
 	if (!(renesas_readl(i3c->regs, BCST) & BCST_BFREF)) {
 		cmd->err = -EBUSY;
 		return cmd->err;
-- 
2.43.0


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

Re: [PATCH v5 11/17] i3c: renesas: Follow a unified pattern for transfer and command initialization

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The `renesas_i3c_i3c_xfers` function completely ignores the return status of individual I3C transfers, unconditionally returning success even on hardware failures or timeouts.
- [High] The `renesas_i3c_i2c_xfers` function ignores timeouts from `wait_for_completion_timeout()`, leading to hardware desynchronization, silent failures, and a race condition.
--

--- Patch [11]: [PATCH v5 11/17] i3c: renesas: Follow a unified pattern for transfer and command initialization ---
commit 0fa956e2c2bc3ba154fb16507f16f40d18d2d2db
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

    i3c: renesas: Follow a unified pattern for transfer and command initialization
    
    Follow a unified pattern for transfer and command initialization across
    the driver. This keeps the code cleaner and easier to follow. Also, in
    some cases the I3C device was enabled before the transfer data structure
    was even allocated.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 9a36132200342..c1396c49f45e5 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -835,15 +835,15 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
>  	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
>  	int i;
>  
> -	/* Enable I3C bus. */
> -	renesas_i3c_bus_enable(m, true);
> -
>  	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
>  	if (!xfer)
>  		return -ENOMEM;
>  
>  	init_completion(&xfer->comp);
>  
> +	/* Enable I3C bus. */
> +	renesas_i3c_bus_enable(m, true);
> +
>  	for (i = 0; i < i3c_nxfers; i++) {
>  		struct renesas_i3c_cmd *cmd = xfer->cmds;

[Severity: High]
This isn't a bug introduced by this patch, but does this function properly
propagate hardware errors?

Looking further down in renesas_i3c_i3c_xfers(), the transfer status is
captured when calling renesas_i3c_wait_xfer():

    renesas_i3c_wait_xfer(i3c, xfer);

However, xfer->ret is completely ignored, and the function unconditionally
returns 0. Hardware I3C bus errors (like NACK, parity errors, CRC failures,
or timeouts) appear to be completely masked from the I3C subsystem and client
drivers.

Can this lead to upper layers assuming write commands succeeded when they
failed, or read commands returning valid data when the buffer actually
contains uninitialized memory?

[ ... ]
> @@ -964,12 +964,12 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
>  	if (!xfer)
>  		return -ENOMEM;
>  
> -	renesas_i3c_bus_enable(m, false);
> -
>  	init_completion(&xfer->comp);
>  	xfer->is_i2c_xfer = true;
>  	cmd = xfer->cmds;
>  
> +	renesas_i3c_bus_enable(m, false);
> +
>  	if (!(renesas_readl(i3c->regs, BCST) & BCST_BFREF)) {
>  		cmd->err = -EBUSY;
>  		return cmd->err;

[Severity: High]
This is a pre-existing issue, but are timeouts handled safely here?

Further down in renesas_i3c_i2c_xfers(), the function waits for completion:

    wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
    
    if (cmd->err)
        break;

The return value of wait_for_completion_timeout() (which is 0 on timeout) is
not checked, and cmd->err is typically 0 on a timeout. This causes the driver
to proceed with the next transfer erroneously.

If the driver immediately starts the next transfer by modifying the shared cmd
structure in-place while the previous transfer is still enqueued, could a
delayed interrupt from the timed-out transfer access the repurposed cmd
structure?

This seems like it could cause memory corruption or an unpredictable hardware
state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=11

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

[PATCH v5 12/17] i3c: renesas: Drop the explicit memset() call

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Drop the explicit memset() call on struct i3c_device_info object, as it is
already initialized at declaration through compiler initialization.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index c1396c49f45e..52f09d966651 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -639,7 +639,6 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
 	i3c->dyn_addr = ret;
 	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYAD(ret) | MSDVAD_MDYADV);
 
-	memset(&info, 0, sizeof(info));
 	info.dyn_addr = ret;
 	return i3c_master_set_info(&i3c->base, &info);
 }
-- 
2.43.0

[PATCH v5 12/17] i3c: renesas: Drop the explicit memset() call

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Drop the explicit memset() call on struct i3c_device_info object, as it is
already initialized at declaration through compiler initialization.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index c1396c49f45e..52f09d966651 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -639,7 +639,6 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
 	i3c->dyn_addr = ret;
 	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYAD(ret) | MSDVAD_MDYADV);
 
-	memset(&info, 0, sizeof(info));
 	info.dyn_addr = ret;
 	return i3c_master_set_info(&i3c->base, &info);
 }
-- 
2.43.0


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

[PATCH v5 13/17] i3c: renesas: Update HW registers after SW computations are done

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

renesas_i3c_bus_init() performs a number of computations and software
cache updates, interleaving them with hardware register writes. While
this works today, it makes it harder to minimize the time the controller
must remain powered when runtime PM is introduced.

Perform all software computations and cache updates first, then update
the hardware registers. This prepares for future runtime PM support.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 52f09d966651..2cff7c6ae369 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -565,10 +565,6 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
 	if (!i3c->rate)
 		return -EINVAL;
 
-	ret = renesas_i3c_reset(i3c);
-	if (ret)
-		return ret;
-
 	i2c_total_ticks = DIV_ROUND_UP(i3c->rate, bus->scl_rate.i2c);
 	i3c_total_ticks = DIV_ROUND_UP(i3c->rate, bus->scl_rate.i3c);
 
@@ -619,27 +615,31 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
 			STDBR_SBRHO(double_SBR, od_high_ticks) |
 			STDBR_SBRLP(pp_low_ticks) |
 			STDBR_SBRHP(pp_high_ticks);
-	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
 
 	/* Extended Bit Rate setting */
 	i3c->extbr = EXTBR_EBRLO(od_low_ticks) | EXTBR_EBRHO(od_high_ticks) |
 		     EXTBR_EBRLP(pp_low_ticks) | EXTBR_EBRHP(pp_high_ticks);
-	renesas_writel(i3c->regs, EXTBR, i3c->extbr);
-
-	renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
-	i3c->refclk_div = cks;
-
-	/* I3C hw init*/
-	renesas_i3c_hw_init(i3c);
 
 	ret = i3c_master_get_free_addr(m, 0);
 	if (ret < 0)
 		return ret;
 
+	info.dyn_addr = ret;
 	i3c->dyn_addr = ret;
-	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYAD(ret) | MSDVAD_MDYADV);
+	i3c->refclk_div = cks;
+
+	ret = renesas_i3c_reset(i3c);
+	if (ret)
+		return ret;
+
+	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
+	renesas_writel(i3c->regs, EXTBR, i3c->extbr);
+	renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
+	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYAD(i3c->dyn_addr) | MSDVAD_MDYADV);
+
+	/* I3C hw init*/
+	renesas_i3c_hw_init(i3c);
 
-	info.dyn_addr = ret;
 	return i3c_master_set_info(&i3c->base, &info);
 }
 
-- 
2.43.0

[PATCH v5 13/17] i3c: renesas: Update HW registers after SW computations are done

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

renesas_i3c_bus_init() performs a number of computations and software
cache updates, interleaving them with hardware register writes. While
this works today, it makes it harder to minimize the time the controller
must remain powered when runtime PM is introduced.

Perform all software computations and cache updates first, then update
the hardware registers. This prepares for future runtime PM support.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 52f09d966651..2cff7c6ae369 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -565,10 +565,6 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
 	if (!i3c->rate)
 		return -EINVAL;
 
-	ret = renesas_i3c_reset(i3c);
-	if (ret)
-		return ret;
-
 	i2c_total_ticks = DIV_ROUND_UP(i3c->rate, bus->scl_rate.i2c);
 	i3c_total_ticks = DIV_ROUND_UP(i3c->rate, bus->scl_rate.i3c);
 
@@ -619,27 +615,31 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
 			STDBR_SBRHO(double_SBR, od_high_ticks) |
 			STDBR_SBRLP(pp_low_ticks) |
 			STDBR_SBRHP(pp_high_ticks);
-	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
 
 	/* Extended Bit Rate setting */
 	i3c->extbr = EXTBR_EBRLO(od_low_ticks) | EXTBR_EBRHO(od_high_ticks) |
 		     EXTBR_EBRLP(pp_low_ticks) | EXTBR_EBRHP(pp_high_ticks);
-	renesas_writel(i3c->regs, EXTBR, i3c->extbr);
-
-	renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
-	i3c->refclk_div = cks;
-
-	/* I3C hw init*/
-	renesas_i3c_hw_init(i3c);
 
 	ret = i3c_master_get_free_addr(m, 0);
 	if (ret < 0)
 		return ret;
 
+	info.dyn_addr = ret;
 	i3c->dyn_addr = ret;
-	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYAD(ret) | MSDVAD_MDYADV);
+	i3c->refclk_div = cks;
+
+	ret = renesas_i3c_reset(i3c);
+	if (ret)
+		return ret;
+
+	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
+	renesas_writel(i3c->regs, EXTBR, i3c->extbr);
+	renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
+	renesas_writel(i3c->regs, MSDVAD, MSDVAD_MDYAD(i3c->dyn_addr) | MSDVAD_MDYADV);
+
+	/* I3C hw init*/
+	renesas_i3c_hw_init(i3c);
 
-	info.dyn_addr = ret;
 	return i3c_master_set_info(&i3c->base, &info);
 }
 
-- 
2.43.0


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

Re: [PATCH v5 13/17] i3c: renesas: Update HW registers after SW computations are done

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Potential division by zero when calculating timing adjustments if i3c->rate > 1 GHz.
- [High] Kernel panic due to division by zero if the I3C bus operates in pure mode and the device tree lacks the 'i2c-scl-hz' property.
--

--- Patch [13]: [PATCH v5 13/17] i3c: renesas: Update HW registers after SW computations are done ---
commit c4f913d9658906f3cc459cae7e1d70cd5318dc3c
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Subject: i3c: renesas: Update HW registers after SW computations are done

This patch modifies renesas_i3c_bus_init() to perform software computations
and cache updates before writing to hardware registers. This prepares the
driver for future runtime PM support by minimizing the time the controller
must remain powered.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 52f09d9666512..2cff7c6ae3692 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -565,10 +565,6 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
>  	if (!i3c->rate)
>  		return -EINVAL;
>  
> -	ret = renesas_i3c_reset(i3c);
> -	if (ret)
> -		return ret;
> -
>  	i2c_total_ticks = DIV_ROUND_UP(i3c->rate, bus->scl_rate.i2c);

[Severity: High]
This is a pre-existing issue, but does this code cause a divide by zero if
the bus operates in pure mode without an explicit I2C rate limit?

If the I3C bus has no I2C children (falling back to I3C_BUS_MODE_PURE) and
the device tree lacks the 'i2c-scl-hz' property, bus->scl_rate.i2c is never
populated and remains 0. 

When DIV_ROUND_UP() executes here in renesas_i3c_bus_init(), will it
trigger a kernel panic?

>  	i3c_total_ticks = DIV_ROUND_UP(i3c->rate, bus->scl_rate.i3c);
>  

[ ... ]

> @@ -619,27 +615,31 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
>  			STDBR_SBRHO(double_SBR, od_high_ticks) |
>  			STDBR_SBRLP(pp_low_ticks) |
>  			STDBR_SBRHP(pp_high_ticks);

[Severity: High]
This isn't a bug introduced by this patch, but can we hit a divide by zero
exception during these timing calculations in renesas_i3c_bus_init() and
renesas_i3c_hw_init() if the I3C controller clock (tclk) exceeds 1 GHz?

Looking at the code just before this hunk in renesas_i3c_bus_init():

    od_low_ticks -= t.scl_fall_ns / (NSEC_PER_SEC / i3c->rate) + 1;
    od_high_ticks -= t.scl_rise_ns / (NSEC_PER_SEC / i3c->rate) + 1;

And similarly in renesas_i3c_hw_init() in drivers/i3c/master/renesas-i3c.c:

    val = DIV_ROUND_UP(I3C_BUS_TBUF_MIXED_FM_MIN_NS,
                       NSEC_PER_SEC / i3c->rate);

If i3c->rate is greater than 1,000,000,000, integer division
(NSEC_PER_SEC / i3c->rate) evaluates to 0. 

Will this truncated zero value subsequently be used as a divisor, causing a
kernel panic during the driver's probe sequence?

> -	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
>  
>  	/* Extended Bit Rate setting */
>  	i3c->extbr = EXTBR_EBRLO(od_low_ticks) | EXTBR_EBRHO(od_high_ticks) |
>  		     EXTBR_EBRLP(pp_low_ticks) | EXTBR_EBRHP(pp_high_ticks);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=13

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

[PATCH v5 14/17] i3c: renesas: Organize structures to avoid unnecessary padding

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reorder structure members to reduce padding and improve memory layout.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 2cff7c6ae369..a54c20ef5f6e 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -221,19 +221,19 @@ enum renesas_i3c_event {
 };
 
 struct renesas_i3c_cmd {
+	const void *tx_buf;
+	void *rx_buf;
+	/* i2c xfer */
+	u8 *i2c_buf;
+	const struct i2c_msg *msg;
+	int i2c_bytes_left;
+	int i2c_is_last;
 	u32 cmd0;
 	u32 len;
-	const void *tx_buf;
 	u32 tx_count;
-	void *rx_buf;
 	u32 rx_count;
 	u32 err;
 	u8 rnw;
-	/* i2c xfer */
-	int i2c_bytes_left;
-	int i2c_is_last;
-	u8 *i2c_buf;
-	const struct i2c_msg *msg;
 };
 
 struct renesas_i3c_xfer {
@@ -253,21 +253,22 @@ struct renesas_i3c_xferqueue {
 };
 
 struct renesas_i3c {
+	void __iomem *regs;
+	struct clk_bulk_data *clks;
+	struct reset_control *presetn;
+	struct reset_control *tresetn;
+	struct renesas_i3c_xferqueue xferqueue;
 	struct i3c_master_controller base;
+	u8 addrs[RENESAS_I3C_MAX_DEVS];
+	unsigned long rate;
 	enum i3c_internal_state internal_state;
-	u16 maxdevs;
+	bool resuming;
 	u32 free_pos;
 	u32 dyn_addr;
 	u32 i2c_STDBR;
 	u32 i3c_STDBR;
 	u32 extbr;
-	unsigned long rate;
-	u8 addrs[RENESAS_I3C_MAX_DEVS];
-	struct renesas_i3c_xferqueue xferqueue;
-	void __iomem *regs;
-	struct clk_bulk_data *clks;
-	struct reset_control *presetn;
-	struct reset_control *tresetn;
+	u16 maxdevs;
 	u8 num_clks;
 	u8 refclk_div;
 };
-- 
2.43.0

[PATCH v5 14/17] i3c: renesas: Organize structures to avoid unnecessary padding

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Reorder structure members to reduce padding and improve memory layout.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 2cff7c6ae369..a54c20ef5f6e 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -221,19 +221,19 @@ enum renesas_i3c_event {
 };
 
 struct renesas_i3c_cmd {
+	const void *tx_buf;
+	void *rx_buf;
+	/* i2c xfer */
+	u8 *i2c_buf;
+	const struct i2c_msg *msg;
+	int i2c_bytes_left;
+	int i2c_is_last;
 	u32 cmd0;
 	u32 len;
-	const void *tx_buf;
 	u32 tx_count;
-	void *rx_buf;
 	u32 rx_count;
 	u32 err;
 	u8 rnw;
-	/* i2c xfer */
-	int i2c_bytes_left;
-	int i2c_is_last;
-	u8 *i2c_buf;
-	const struct i2c_msg *msg;
 };
 
 struct renesas_i3c_xfer {
@@ -253,21 +253,22 @@ struct renesas_i3c_xferqueue {
 };
 
 struct renesas_i3c {
+	void __iomem *regs;
+	struct clk_bulk_data *clks;
+	struct reset_control *presetn;
+	struct reset_control *tresetn;
+	struct renesas_i3c_xferqueue xferqueue;
 	struct i3c_master_controller base;
+	u8 addrs[RENESAS_I3C_MAX_DEVS];
+	unsigned long rate;
 	enum i3c_internal_state internal_state;
-	u16 maxdevs;
+	bool resuming;
 	u32 free_pos;
 	u32 dyn_addr;
 	u32 i2c_STDBR;
 	u32 i3c_STDBR;
 	u32 extbr;
-	unsigned long rate;
-	u8 addrs[RENESAS_I3C_MAX_DEVS];
-	struct renesas_i3c_xferqueue xferqueue;
-	void __iomem *regs;
-	struct clk_bulk_data *clks;
-	struct reset_control *presetn;
-	struct reset_control *tresetn;
+	u16 maxdevs;
 	u8 num_clks;
 	u8 refclk_div;
 };
-- 
2.43.0


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

Re: [PATCH v5 14/17] i3c: renesas: Organize structures to avoid unnecessary padding

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch implicitly adds a new, completely unused field `bool resuming;` to `struct renesas_i3c` which also introduces unnecessary padding.
--

--- Patch [14]: [PATCH v5 14/17] i3c: renesas: Organize structures to avoid unnecessary padding ---
commit c33f65cddae91fbf173c604e3698103c59da9466
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
i3c: renesas: Organize structures to avoid unnecessary padding

This commit reorders the members of the renesas_i3c_cmd and renesas_i3c
structures. The stated goal is to reduce padding and improve memory layout.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 2cff7c6ae3692..a54c20ef5f6ea 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> @@ -253,21 +253,22 @@ struct renesas_i3c_xferqueue {
>  };
>  
>  struct renesas_i3c {
> +	void __iomem *regs;
> +	struct clk_bulk_data *clks;
> +	struct reset_control *presetn;
> +	struct reset_control *tresetn;
> +	struct renesas_i3c_xferqueue xferqueue;
>  	struct i3c_master_controller base;
> +	u8 addrs[RENESAS_I3C_MAX_DEVS];
> +	unsigned long rate;
>  	enum i3c_internal_state internal_state;
> -	u16 maxdevs;
> +	bool resuming;

[Severity: Medium]
Does the addition of this boolean field contradict the commit message?

The commit message states the goal is to reduce padding and improve memory 
layout, but this adds a completely unused field, resuming, to struct 
renesas_i3c that was not present before. Adding this 1-byte field between a 
4-byte enum (internal_state) and a 4-byte u32 (free_pos) appears to 
introduce 3 bytes of internal padding to satisfy alignment requirements.

Is it possible this field was accidentally included from another patch?

>  	u32 free_pos;
>  	u32 dyn_addr;
>  	u32 i2c_STDBR;
>  	u32 i3c_STDBR;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=14

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

[PATCH v5 15/17] i3c: renesas: Use the "dev_name:irq_name" format for the interrupt name

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Use the "dev_name:irq_name" format for the interrupt names. This makes it
easier to identify interrupts in systems where multiple devices may request
interrupts with the same name.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index a54c20ef5f6e..f28e260dd166 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1409,12 +1409,19 @@ static int renesas_i3c_probe(struct platform_device *pdev)
 		return ret;
 
 	for (i = 0; i < ARRAY_SIZE(renesas_i3c_irqs); i++) {
+		const char *irqname;
+
 		ret = platform_get_irq_byname(pdev, renesas_i3c_irqs[i].name);
 		if (ret < 0)
 			return ret;
 
+		irqname = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s:%s", dev_name(&pdev->dev),
+					 renesas_i3c_irqs[i].desc);
+		if (!irqname)
+			return -ENOMEM;
+
 		ret = devm_request_irq(&pdev->dev, ret, renesas_i3c_irqs[i].isr,
-				       0, renesas_i3c_irqs[i].desc, i3c);
+				       0, irqname, i3c);
 		if (ret)
 			return ret;
 	}
-- 
2.43.0

[PATCH v5 15/17] i3c: renesas: Use the "dev_name:irq_name" format for the interrupt name

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Use the "dev_name:irq_name" format for the interrupt names. This makes it
easier to identify interrupts in systems where multiple devices may request
interrupts with the same name.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index a54c20ef5f6e..f28e260dd166 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1409,12 +1409,19 @@ static int renesas_i3c_probe(struct platform_device *pdev)
 		return ret;
 
 	for (i = 0; i < ARRAY_SIZE(renesas_i3c_irqs); i++) {
+		const char *irqname;
+
 		ret = platform_get_irq_byname(pdev, renesas_i3c_irqs[i].name);
 		if (ret < 0)
 			return ret;
 
+		irqname = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s:%s", dev_name(&pdev->dev),
+					 renesas_i3c_irqs[i].desc);
+		if (!irqname)
+			return -ENOMEM;
+
 		ret = devm_request_irq(&pdev->dev, ret, renesas_i3c_irqs[i].isr,
-				       0, renesas_i3c_irqs[i].desc, i3c);
+				       0, irqname, i3c);
 		if (ret)
 			return ret;
 	}
-- 
2.43.0


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

[PATCH v5 16/17] i3c: renesas: Drop unnecessary tab

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Remove an unnecessary tab to make the code cleaner.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index f28e260dd166..378baac71aef 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -109,7 +109,7 @@
 #define  NCMDQP_DATA_LENGTH(x)	FIELD_PREP(GENMASK(31, 16), x)
 
 #define NRSPQP			0x154 /* Normal Respone Queue */
-#define  NRSPQP_NO_ERROR			0
+#define  NRSPQP_NO_ERROR		0
 #define  NRSPQP_ERROR_CRC		1
 #define  NRSPQP_ERROR_PARITY		2
 #define  NRSPQP_ERROR_FRAME		3
-- 
2.43.0

[PATCH v5 16/17] i3c: renesas: Drop unnecessary tab

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Remove an unnecessary tab to make the code cleaner.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- none

Changes in v4:
- none

Changes in v3:
- none

Changes in v2:
- collected tags

 drivers/i3c/master/renesas-i3c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index f28e260dd166..378baac71aef 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -109,7 +109,7 @@
 #define  NCMDQP_DATA_LENGTH(x)	FIELD_PREP(GENMASK(31, 16), x)
 
 #define NRSPQP			0x154 /* Normal Respone Queue */
-#define  NRSPQP_NO_ERROR			0
+#define  NRSPQP_NO_ERROR		0
 #define  NRSPQP_ERROR_CRC		1
 #define  NRSPQP_ERROR_PARITY		2
 #define  NRSPQP_ERROR_FRAME		3
-- 
2.43.0


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

[PATCH v5 17/17] i3c: renesas: Add runtime PM support

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

On the SoCs where the Renesas I3C driver is enabled (RZ/G3S and RZ/G3E),
the clocks of the IP are managed through a clock PM domain. To keep the
I3C code simpler, the explicit clock handling was dropped along with the
addition of runtime PM support, in favor of the runtime PM APIs. Only the
code for getting tclk was preserved, as it is necessary to compute the
I3C clock rate.

All the APIs provided to the I3C subsystem through struct
i3c_master_controller_ops are guarded with runtime PM APIs to
enable/disable the controller at runtime.

As the Renesas I3C driver implements an asynchronous transmit model by
preparing a transfer and waiting for its completion through the ISR,
renesas_i3c_abort_xfer() was added to disable interrupts and clear any
pending IRQ status bits when there is no completion in the defined
timeout. Along with this, renesas_i3c_wait_xfer() return type was changed
to unsigned long.

Add runtime PM support for the Renesas I3C driver.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- updated renesas_i3c_irqs_mask_and_clear() to use the
  renesas_i3c_irqs_mask_and_clear_locked() introduced in patch 1 from
  this series

Changes in v4:
- renamed renesas_i3c_abort_xfer() from v3 to
  renesas_i3c_irqs_mask_and_clear(); along with it renamed local varibles
  abort_xfer to xfer_failed

Changes in v3:
- dropped the RPM resume/suspend in renesas_i3c_suspend() to read the
  DATBASn registers as the DATBASn register are not used on suspend/resume
  path anymore in this version

Changes in v2:
- dropped the runtime suspend/resume functions as for now, there will be
  no pinctrl sleep state described in DT
- do not synchronize the IRQs in renesas_i3c_abort_xfer() as some handlers
  may re-enable interrupts; instead just disable the interrupts and clean
  any status bits that the IRQ handlers are touching; with this the
  struct renesas_i3c::{irqs, num_irqs} and the associated code was removed
- dropped the renesas_i3c_dont_use_autosuspend() along with the
  devm_add_action_or_reset() call to set it since the same operation is
  done by the devres cleanup helper of devm_pm_runtime_enable()
- adjusted the renesas_i3c_suspend() with RPM calls to save the DATBAS
  registers
- use pm_runtime_resume_and_get() in renesas_i3c_resume() to avoid
  mixing gotos with cleanup helpers
- adjusted the patch description to reflect these changes

 drivers/i3c/master/renesas-i3c.c | 140 ++++++++++++++++++++++++-------
 1 file changed, 112 insertions(+), 28 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 378baac71aef..1975c599ca43 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -22,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
 #include "../internals.h"
@@ -199,8 +200,6 @@
 #define RENESAS_I3C_MAX_DEVS	8
 #define I2C_INIT_MSG		-1
 
-#define RENESAS_I3C_TCLK_IDX	1
-
 enum i3c_internal_state {
 	I3C_INTERNAL_STATE_DISABLED,
 	I3C_INTERNAL_STATE_CONTROLLER_IDLE,
@@ -254,9 +253,10 @@ struct renesas_i3c_xferqueue {
 
 struct renesas_i3c {
 	void __iomem *regs;
-	struct clk_bulk_data *clks;
+	struct clk *tclk;
 	struct reset_control *presetn;
 	struct reset_control *tresetn;
+	struct device *dev;
 	struct renesas_i3c_xferqueue xferqueue;
 	struct i3c_master_controller base;
 	u8 addrs[RENESAS_I3C_MAX_DEVS];
@@ -269,7 +269,6 @@ struct renesas_i3c {
 	u32 i3c_STDBR;
 	u32 extbr;
 	u16 maxdevs;
-	u8 num_clks;
 	u8 refclk_div;
 };
 
@@ -449,7 +448,14 @@ static void renesas_i3c_irqs_mask_and_clear_locked(struct renesas_i3c *i3c)
 	renesas_readl(i3c->regs, BST);
 }
 
-static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
+static void renesas_i3c_irqs_mask_and_clear(struct renesas_i3c *i3c)
+{
+	guard(spinlock_irqsave)(&i3c->xferqueue.lock);
+
+	renesas_i3c_irqs_mask_and_clear_locked(i3c);
+}
+
+static unsigned long renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
 {
 	unsigned long time_left;
 
@@ -458,6 +464,8 @@ static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xf
 	time_left = wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000));
 	if (!time_left)
 		renesas_i3c_dequeue_xfer(i3c, xfer);
+
+	return time_left;
 }
 
 static void renesas_i3c_set_prts(struct renesas_i3c *i3c, u32 val)
@@ -491,6 +499,12 @@ static void renesas_i3c_bus_enable(struct i3c_master_controller *m, bool i3c_mod
 static int renesas_i3c_reset(struct renesas_i3c *i3c)
 {
 	u32 val;
+	int ret;
+
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
 
 	renesas_writel(i3c->regs, BCTL, 0);
 	renesas_set_bit(i3c->regs, RSTCTL, RSTCTL_RI3CRST);
@@ -562,7 +576,7 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
 	int od_high_ticks, od_low_ticks, i2c_total_ticks;
 	int ret;
 
-	i3c->rate = clk_get_rate(i3c->clks[RENESAS_I3C_TCLK_IDX].clk);
+	i3c->rate = clk_get_rate(i3c->tclk);
 	if (!i3c->rate)
 		return -EINVAL;
 
@@ -633,6 +647,11 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
 	if (ret)
 		return ret;
 
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
 	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
 	renesas_writel(i3c->regs, EXTBR, i3c->extbr);
 	renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
@@ -655,6 +674,7 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 {
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_cmd *cmd;
+	unsigned long time_left;
 	u32 olddevs, newdevs;
 	u8 last_addr = 0, pos;
 	int ret;
@@ -667,6 +687,11 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 	cmd = xfer->cmds;
 	cmd->rx_count = 0;
 
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
 	/* Enable I3C bus. */
 	renesas_i3c_bus_enable(m, true);
 
@@ -701,7 +726,9 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 		    NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
 		    NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;
 
-	renesas_i3c_wait_xfer(i3c, xfer);
+	time_left = renesas_i3c_wait_xfer(i3c, xfer);
+	if (!time_left)
+		renesas_i3c_irqs_mask_and_clear(i3c);
 
 	if (cmd->rx_count >= i3c->maxdevs)
 		newdevs = 0;
@@ -767,6 +794,7 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
 {
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_cmd *cmd;
+	unsigned long time_left;
 	int ret, pos = 0;
 
 	if (ccc->id & I3C_CCC_DIRECT) {
@@ -784,6 +812,11 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
 	cmd->rnw = ccc->rnw;
 	cmd->cmd0 = 0;
 
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
 	renesas_i3c_bus_enable(m, true);
 
 	/* Calculate the command descriptor. */
@@ -818,7 +851,9 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
 		}
 	}
 
-	renesas_i3c_wait_xfer(i3c, xfer);
+	time_left = renesas_i3c_wait_xfer(i3c, xfer);
+	if (!time_left)
+		renesas_i3c_irqs_mask_and_clear(i3c);
 
 	ret = xfer->ret;
 	if (ret)
@@ -833,7 +868,9 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
-	int i;
+	unsigned long time_left;
+	bool xfer_failed = false;
+	int i, ret;
 
 	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
 	if (!xfer)
@@ -841,6 +878,11 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
 
 	init_completion(&xfer->comp);
 
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
 	/* Enable I3C bus. */
 	renesas_i3c_bus_enable(m, true);
 
@@ -872,9 +914,14 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
 				renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
 		}
 
-		renesas_i3c_wait_xfer(i3c, xfer);
+		time_left = renesas_i3c_wait_xfer(i3c, xfer);
+		if (!time_left)
+			xfer_failed = true;
 	}
 
+	if (xfer_failed)
+		renesas_i3c_irqs_mask_and_clear(i3c);
+
 	return 0;
 }
 
@@ -883,12 +930,17 @@ static int renesas_i3c_attach_i3c_dev(struct i3c_dev_desc *dev)
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_i2c_dev_data *data;
-	int pos;
+	int pos, ret;
 
 	pos = renesas_i3c_get_free_pos(i3c);
 	if (pos < 0)
 		return pos;
 
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
 	data = kzalloc_obj(*data);
 	if (!data)
 		return -ENOMEM;
@@ -910,7 +962,12 @@ static int renesas_i3c_reattach_i3c_dev(struct i3c_dev_desc *dev,
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
-	int pos;
+	int pos, ret;
+
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
 
 	pos = renesas_i3c_get_free_pos(i3c);
 
@@ -938,8 +995,12 @@ static void renesas_i3c_detach_i3c_dev(struct i3c_dev_desc *dev)
 	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
+	int ret;
 
-	renesas_writel(i3c->regs, DATBAS(data->index), 0);
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (!ret)
+		renesas_writel(i3c->regs, DATBAS(data->index), 0);
 
 	i3c_dev_set_master_data(dev, NULL);
 	i3c->addrs[data->index] = 0;
@@ -955,7 +1016,9 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_cmd *cmd;
 	u8 start_bit = CNDCTL_STCND;
-	int i;
+	unsigned long time_left;
+	bool xfer_failed = false;
+	int i, ret;
 
 	if (!i2c_nxfers)
 		return 0;
@@ -968,6 +1031,11 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 	xfer->is_i2c_xfer = true;
 	cmd = xfer->cmds;
 
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
 	renesas_i3c_bus_enable(m, false);
 
 	if (!(renesas_readl(i3c->regs, BCST) & BCST_BFREF)) {
@@ -994,7 +1062,9 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 
 		renesas_set_bit(i3c->regs, NTSTE, NTSTE_TDBEE0);
 
-		wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
+		time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
+		if (!time_left)
+			xfer_failed = true;
 
 		if (cmd->err)
 			break;
@@ -1003,6 +1073,10 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 	}
 
 	renesas_i3c_dequeue_xfer(i3c, xfer);
+
+	if (xfer_failed)
+		renesas_i3c_irqs_mask_and_clear(i3c);
+
 	return cmd->err;
 }
 
@@ -1384,12 +1458,16 @@ static int renesas_i3c_probe(struct platform_device *pdev)
 	if (IS_ERR(i3c->regs))
 		return PTR_ERR(i3c->regs);
 
-	ret = devm_clk_bulk_get_all_enabled(&pdev->dev, &i3c->clks);
-	if (ret <= RENESAS_I3C_TCLK_IDX)
-		return dev_err_probe(&pdev->dev, ret < 0 ? ret : -EINVAL,
-				     "Failed to get clocks (need > %d, got %d)\n",
-				     RENESAS_I3C_TCLK_IDX, ret);
-	i3c->num_clks = ret;
+	i3c->tclk = devm_clk_get(&pdev->dev, "tclk");
+	if (IS_ERR(i3c->tclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(i3c->tclk), "Failed to get tclk");
+
+	i3c->dev = &pdev->dev;
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 300);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
 
 	i3c->tresetn = devm_reset_control_get_optional_exclusive_deasserted(&pdev->dev, "tresetn");
 	if (IS_ERR(i3c->tresetn))
@@ -1456,8 +1534,6 @@ static int renesas_i3c_suspend(struct device *dev)
 	if (ret)
 		goto err_mark_resumed;
 
-	clk_bulk_disable(i3c->num_clks, i3c->clks);
-
 	return 0;
 
 err_mark_resumed:
@@ -1479,13 +1555,13 @@ static int renesas_i3c_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
+	ret = renesas_i3c_reset(i3c);
 	if (ret)
 		goto err_resets_asserted;
 
-	ret = renesas_i3c_reset(i3c);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
-		goto err_clks_disable;
+		goto err_resets_asserted;
 
 	/* Re-store I3C registers value. */
 	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
@@ -1504,15 +1580,23 @@ static int renesas_i3c_resume(struct device *dev)
 
 	i2c_mark_adapter_resumed(&i3c->base.i2c);
 
+	pm_runtime_put_autosuspend(dev);
+
 	/*
 	 * I3C devices may have retained their dynamic address anyway. Do not
 	 * fail the resume because of DAA error.
 	 */
 	return 0;
 
-err_clks_disable:
-	clk_bulk_disable(i3c->num_clks, i3c->clks);
 err_resets_asserted:
+	/*
+	 * If this happens, there is no way to recover from this state without
+	 * reloading the driver. We want to avoid keeping the reset line
+	 * deasserted unnecessarily. The runtime paths will still work correctly
+	 * even if the IP registers are accessed while reset is asserted (e.g.
+	 * if a runtime path is triggered after a failed resume). Checked on
+	 * RZ/G3S.
+	 */
 	reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
 	return ret;
 }
-- 
2.43.0

[PATCH v5 17/17] i3c: renesas: Add runtime PM support

[Claudiu Beznea]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

On the SoCs where the Renesas I3C driver is enabled (RZ/G3S and RZ/G3E),
the clocks of the IP are managed through a clock PM domain. To keep the
I3C code simpler, the explicit clock handling was dropped along with the
addition of runtime PM support, in favor of the runtime PM APIs. Only the
code for getting tclk was preserved, as it is necessary to compute the
I3C clock rate.

All the APIs provided to the I3C subsystem through struct
i3c_master_controller_ops are guarded with runtime PM APIs to
enable/disable the controller at runtime.

As the Renesas I3C driver implements an asynchronous transmit model by
preparing a transfer and waiting for its completion through the ISR,
renesas_i3c_abort_xfer() was added to disable interrupts and clear any
pending IRQ status bits when there is no completion in the defined
timeout. Along with this, renesas_i3c_wait_xfer() return type was changed
to unsigned long.

Add runtime PM support for the Renesas I3C driver.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v5:
- updated renesas_i3c_irqs_mask_and_clear() to use the
  renesas_i3c_irqs_mask_and_clear_locked() introduced in patch 1 from
  this series

Changes in v4:
- renamed renesas_i3c_abort_xfer() from v3 to
  renesas_i3c_irqs_mask_and_clear(); along with it renamed local varibles
  abort_xfer to xfer_failed

Changes in v3:
- dropped the RPM resume/suspend in renesas_i3c_suspend() to read the
  DATBASn registers as the DATBASn register are not used on suspend/resume
  path anymore in this version

Changes in v2:
- dropped the runtime suspend/resume functions as for now, there will be
  no pinctrl sleep state described in DT
- do not synchronize the IRQs in renesas_i3c_abort_xfer() as some handlers
  may re-enable interrupts; instead just disable the interrupts and clean
  any status bits that the IRQ handlers are touching; with this the
  struct renesas_i3c::{irqs, num_irqs} and the associated code was removed
- dropped the renesas_i3c_dont_use_autosuspend() along with the
  devm_add_action_or_reset() call to set it since the same operation is
  done by the devres cleanup helper of devm_pm_runtime_enable()
- adjusted the renesas_i3c_suspend() with RPM calls to save the DATBAS
  registers
- use pm_runtime_resume_and_get() in renesas_i3c_resume() to avoid
  mixing gotos with cleanup helpers
- adjusted the patch description to reflect these changes

 drivers/i3c/master/renesas-i3c.c | 140 ++++++++++++++++++++++++-------
 1 file changed, 112 insertions(+), 28 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 378baac71aef..1975c599ca43 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -22,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
 #include "../internals.h"
@@ -199,8 +200,6 @@
 #define RENESAS_I3C_MAX_DEVS	8
 #define I2C_INIT_MSG		-1
 
-#define RENESAS_I3C_TCLK_IDX	1
-
 enum i3c_internal_state {
 	I3C_INTERNAL_STATE_DISABLED,
 	I3C_INTERNAL_STATE_CONTROLLER_IDLE,
@@ -254,9 +253,10 @@ struct renesas_i3c_xferqueue {
 
 struct renesas_i3c {
 	void __iomem *regs;
-	struct clk_bulk_data *clks;
+	struct clk *tclk;
 	struct reset_control *presetn;
 	struct reset_control *tresetn;
+	struct device *dev;
 	struct renesas_i3c_xferqueue xferqueue;
 	struct i3c_master_controller base;
 	u8 addrs[RENESAS_I3C_MAX_DEVS];
@@ -269,7 +269,6 @@ struct renesas_i3c {
 	u32 i3c_STDBR;
 	u32 extbr;
 	u16 maxdevs;
-	u8 num_clks;
 	u8 refclk_div;
 };
 
@@ -449,7 +448,14 @@ static void renesas_i3c_irqs_mask_and_clear_locked(struct renesas_i3c *i3c)
 	renesas_readl(i3c->regs, BST);
 }
 
-static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
+static void renesas_i3c_irqs_mask_and_clear(struct renesas_i3c *i3c)
+{
+	guard(spinlock_irqsave)(&i3c->xferqueue.lock);
+
+	renesas_i3c_irqs_mask_and_clear_locked(i3c);
+}
+
+static unsigned long renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
 {
 	unsigned long time_left;
 
@@ -458,6 +464,8 @@ static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xf
 	time_left = wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000));
 	if (!time_left)
 		renesas_i3c_dequeue_xfer(i3c, xfer);
+
+	return time_left;
 }
 
 static void renesas_i3c_set_prts(struct renesas_i3c *i3c, u32 val)
@@ -491,6 +499,12 @@ static void renesas_i3c_bus_enable(struct i3c_master_controller *m, bool i3c_mod
 static int renesas_i3c_reset(struct renesas_i3c *i3c)
 {
 	u32 val;
+	int ret;
+
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
 
 	renesas_writel(i3c->regs, BCTL, 0);
 	renesas_set_bit(i3c->regs, RSTCTL, RSTCTL_RI3CRST);
@@ -562,7 +576,7 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
 	int od_high_ticks, od_low_ticks, i2c_total_ticks;
 	int ret;
 
-	i3c->rate = clk_get_rate(i3c->clks[RENESAS_I3C_TCLK_IDX].clk);
+	i3c->rate = clk_get_rate(i3c->tclk);
 	if (!i3c->rate)
 		return -EINVAL;
 
@@ -633,6 +647,11 @@ static int renesas_i3c_bus_init(struct i3c_master_controller *m)
 	if (ret)
 		return ret;
 
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
 	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
 	renesas_writel(i3c->regs, EXTBR, i3c->extbr);
 	renesas_writel(i3c->regs, REFCKCTL, REFCKCTL_IREFCKS(cks));
@@ -655,6 +674,7 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 {
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_cmd *cmd;
+	unsigned long time_left;
 	u32 olddevs, newdevs;
 	u8 last_addr = 0, pos;
 	int ret;
@@ -667,6 +687,11 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 	cmd = xfer->cmds;
 	cmd->rx_count = 0;
 
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
 	/* Enable I3C bus. */
 	renesas_i3c_bus_enable(m, true);
 
@@ -701,7 +726,9 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 		    NCMDQP_CMD(I3C_CCC_ENTDAA) | NCMDQP_DEV_INDEX(ret) |
 		    NCMDQP_DEV_COUNT(i3c->maxdevs - ret) | NCMDQP_TOC;
 
-	renesas_i3c_wait_xfer(i3c, xfer);
+	time_left = renesas_i3c_wait_xfer(i3c, xfer);
+	if (!time_left)
+		renesas_i3c_irqs_mask_and_clear(i3c);
 
 	if (cmd->rx_count >= i3c->maxdevs)
 		newdevs = 0;
@@ -767,6 +794,7 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
 {
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_cmd *cmd;
+	unsigned long time_left;
 	int ret, pos = 0;
 
 	if (ccc->id & I3C_CCC_DIRECT) {
@@ -784,6 +812,11 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
 	cmd->rnw = ccc->rnw;
 	cmd->cmd0 = 0;
 
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
 	renesas_i3c_bus_enable(m, true);
 
 	/* Calculate the command descriptor. */
@@ -818,7 +851,9 @@ static int renesas_i3c_send_ccc_cmd(struct i3c_master_controller *m,
 		}
 	}
 
-	renesas_i3c_wait_xfer(i3c, xfer);
+	time_left = renesas_i3c_wait_xfer(i3c, xfer);
+	if (!time_left)
+		renesas_i3c_irqs_mask_and_clear(i3c);
 
 	ret = xfer->ret;
 	if (ret)
@@ -833,7 +868,9 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
-	int i;
+	unsigned long time_left;
+	bool xfer_failed = false;
+	int i, ret;
 
 	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
 	if (!xfer)
@@ -841,6 +878,11 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
 
 	init_completion(&xfer->comp);
 
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
 	/* Enable I3C bus. */
 	renesas_i3c_bus_enable(m, true);
 
@@ -872,9 +914,14 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
 				renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
 		}
 
-		renesas_i3c_wait_xfer(i3c, xfer);
+		time_left = renesas_i3c_wait_xfer(i3c, xfer);
+		if (!time_left)
+			xfer_failed = true;
 	}
 
+	if (xfer_failed)
+		renesas_i3c_irqs_mask_and_clear(i3c);
+
 	return 0;
 }
 
@@ -883,12 +930,17 @@ static int renesas_i3c_attach_i3c_dev(struct i3c_dev_desc *dev)
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_i2c_dev_data *data;
-	int pos;
+	int pos, ret;
 
 	pos = renesas_i3c_get_free_pos(i3c);
 	if (pos < 0)
 		return pos;
 
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
 	data = kzalloc_obj(*data);
 	if (!data)
 		return -ENOMEM;
@@ -910,7 +962,12 @@ static int renesas_i3c_reattach_i3c_dev(struct i3c_dev_desc *dev,
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
-	int pos;
+	int pos, ret;
+
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
 
 	pos = renesas_i3c_get_free_pos(i3c);
 
@@ -938,8 +995,12 @@ static void renesas_i3c_detach_i3c_dev(struct i3c_dev_desc *dev)
 	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
 	struct i3c_master_controller *m = i3c_dev_get_master(dev);
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
+	int ret;
 
-	renesas_writel(i3c->regs, DATBAS(data->index), 0);
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (!ret)
+		renesas_writel(i3c->regs, DATBAS(data->index), 0);
 
 	i3c_dev_set_master_data(dev, NULL);
 	i3c->addrs[data->index] = 0;
@@ -955,7 +1016,9 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_cmd *cmd;
 	u8 start_bit = CNDCTL_STCND;
-	int i;
+	unsigned long time_left;
+	bool xfer_failed = false;
+	int i, ret;
 
 	if (!i2c_nxfers)
 		return 0;
@@ -968,6 +1031,11 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 	xfer->is_i2c_xfer = true;
 	cmd = xfer->cmds;
 
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
 	renesas_i3c_bus_enable(m, false);
 
 	if (!(renesas_readl(i3c->regs, BCST) & BCST_BFREF)) {
@@ -994,7 +1062,9 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 
 		renesas_set_bit(i3c->regs, NTSTE, NTSTE_TDBEE0);
 
-		wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
+		time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
+		if (!time_left)
+			xfer_failed = true;
 
 		if (cmd->err)
 			break;
@@ -1003,6 +1073,10 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 	}
 
 	renesas_i3c_dequeue_xfer(i3c, xfer);
+
+	if (xfer_failed)
+		renesas_i3c_irqs_mask_and_clear(i3c);
+
 	return cmd->err;
 }
 
@@ -1384,12 +1458,16 @@ static int renesas_i3c_probe(struct platform_device *pdev)
 	if (IS_ERR(i3c->regs))
 		return PTR_ERR(i3c->regs);
 
-	ret = devm_clk_bulk_get_all_enabled(&pdev->dev, &i3c->clks);
-	if (ret <= RENESAS_I3C_TCLK_IDX)
-		return dev_err_probe(&pdev->dev, ret < 0 ? ret : -EINVAL,
-				     "Failed to get clocks (need > %d, got %d)\n",
-				     RENESAS_I3C_TCLK_IDX, ret);
-	i3c->num_clks = ret;
+	i3c->tclk = devm_clk_get(&pdev->dev, "tclk");
+	if (IS_ERR(i3c->tclk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(i3c->tclk), "Failed to get tclk");
+
+	i3c->dev = &pdev->dev;
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 300);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
 
 	i3c->tresetn = devm_reset_control_get_optional_exclusive_deasserted(&pdev->dev, "tresetn");
 	if (IS_ERR(i3c->tresetn))
@@ -1456,8 +1534,6 @@ static int renesas_i3c_suspend(struct device *dev)
 	if (ret)
 		goto err_mark_resumed;
 
-	clk_bulk_disable(i3c->num_clks, i3c->clks);
-
 	return 0;
 
 err_mark_resumed:
@@ -1479,13 +1555,13 @@ static int renesas_i3c_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
+	ret = renesas_i3c_reset(i3c);
 	if (ret)
 		goto err_resets_asserted;
 
-	ret = renesas_i3c_reset(i3c);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
-		goto err_clks_disable;
+		goto err_resets_asserted;
 
 	/* Re-store I3C registers value. */
 	renesas_writel(i3c->regs, STDBR, i3c->i3c_STDBR);
@@ -1504,15 +1580,23 @@ static int renesas_i3c_resume(struct device *dev)
 
 	i2c_mark_adapter_resumed(&i3c->base.i2c);
 
+	pm_runtime_put_autosuspend(dev);
+
 	/*
 	 * I3C devices may have retained their dynamic address anyway. Do not
 	 * fail the resume because of DAA error.
 	 */
 	return 0;
 
-err_clks_disable:
-	clk_bulk_disable(i3c->num_clks, i3c->clks);
 err_resets_asserted:
+	/*
+	 * If this happens, there is no way to recover from this state without
+	 * reloading the driver. We want to avoid keeping the reset line
+	 * deasserted unnecessarily. The runtime paths will still work correctly
+	 * even if the IP registers are accessed while reset is asserted (e.g.
+	 * if a runtime path is triggered after a failed resume). Checked on
+	 * RZ/G3S.
+	 */
 	reset_control_bulk_assert(ARRAY_SIZE(resets), resets);
 	return ret;
 }
-- 
2.43.0


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

Re: [PATCH v5 17/17] i3c: renesas: Add runtime PM support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Removal of DATBASn save/restore logic permanently bricks the I3C bus after system suspend/resume.
- [Medium] Pending asynchronous suspend is cancelled during driver removal, leaking power.

Pre-existing issues:
- [High] Transfer timeouts are ignored and loops are not aborted, falsely returning success and risking heap buffer overflows on hung hardware.
--

--- Patch [17]: [PATCH v5 17/17] i3c: renesas: Add runtime PM support ---
commit fd9b8f0562f27e4a6338d1856950821776c595b5
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Add runtime PM support

On the SoCs where the Renesas I3C driver is enabled (RZ/G3S and RZ/G3E),
the clocks of the IP are managed through a clock PM domain. To keep the
I3C code simpler, the explicit clock handling was dropped along with the
addition of runtime PM support, in favor of the runtime PM APIs.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 378baac71aefa..1975c599ca438 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> @@ -491,6 +499,12 @@ static void renesas_i3c_bus_enable(struct i3c_master_controller *m, bool i3c_mod
>  static int renesas_i3c_reset(struct renesas_i3c *i3c)
>  {
>  	u32 val;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(i3c->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;

[Severity: Medium]
Will this leak power when the driver is unbound or the module is unloaded?

During driver removal, the I3C core calls renesas_i3c_bus_cleanup() which in
turn calls renesas_i3c_reset(). This acquires the PM synchronously but releases
it asynchronously with an autosuspend delay.

Because devm_pm_runtime_enable() was used in probe, the devres cleanup
immediately executes pm_runtime_disable(). This cancels any pending
asynchronous autosuspend work, leaving the device and its PM domain clocks
permanently enabled.

[ ... ]

> @@ -872,9 +914,14 @@ static int renesas_i3c_i3c_xfers(struct i3c_dev_desc *dev, struct i3c_xfer *i3c_
>  				renesas_set_bit(i3c->regs, NTIE, NTIE_TDBEIE0);
>  		}
>  
> -		renesas_i3c_wait_xfer(i3c, xfer);
> +		time_left = renesas_i3c_wait_xfer(i3c, xfer);
> +		if (!time_left)
> +			xfer_failed = true;
>  	}

[Severity: High]
This is a pre-existing issue, but does this missing timeout handling risk a
heap buffer overflow?

If a hardware transfer is delayed and the timeout expires, the loop continues
to the next message and reuses the shared cmd descriptor for the next transfer.

If the delayed hardware eventually completes the first transfer while the
second is set up, renesas_i3c_rx_isr() will read the data:

    read_bytes = NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) * sizeof(u32);
    i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);

Since it reads based on the hardware FIFO length without validating against
cmd->len, won't this overwrite the new, potentially smaller, buffer?

Also, won't this incorrectly return 0 (success) on a timeout since the loop is
not aborted?

[ ... ]

> @@ -1479,13 +1555,13 @@ static int renesas_i3c_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_bulk_enable(i3c->num_clks, i3c->clks);
> +	ret = renesas_i3c_reset(i3c);
>  	if (ret)
>  		goto err_resets_asserted;

[Severity: High]
Does dropping the DATBASn register save/restore logic break the I3C bus after
suspend/resume?

The renesas_i3c_reset() function clears all hardware registers, including
DATBAS, to 0. Later in the resume process, i3c_master_do_daa_ext() calls
renesas_i3c_daa() to rediscover devices.

However, renesas_i3c_daa() explicitly skips DATBAS programming for
already-attached devices:

    for (pos = 0; pos < i3c->maxdevs; pos++) {
        if (olddevs & BIT(pos))
            continue;
        ...
        renesas_writel(i3c->regs, DATBAS(pos), datbas_dvdyad_with_parity(ret));
    }

Because the previously attached devices never dropped their dynamic addresses,
they will not participate in DAA. This leaves their DATBAS registers at 0.

Won't all subsequent communication fail since the controller targets address 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713130545.568657-1-claudiu.beznea+renesas@tuxon.dev?part=17

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

Re: [PATCH v5 00/17] i3c: renesas: Suspend to RAM with power loss and runtime PM

[Tommaso Merciai]

Hi Claudiu,
Thanks for your patch.

On Mon, Jul 13, 2026 at 04:05:28PM +0300, Claudiu Beznea wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Hi,
> 
> This series adjusts the suspend to RAM code to handle cases where power
> to the connected devices is lost during suspend to RAM. The fixes
> included in this series are required for that support.
> 
> Along with suspend to RAM support, runtime PM support is also added.
> Cleanup patches were included to prepare for clean runtime PM support.
> 
> Thank you,
> Claudiu
> 
> Changes in v5:
> - in patch 1 introduced renesas_i3c_irqs_mask_and_clear_locked()
>   similar to what was present in v4 in patch
>   "i3c: renesas: Add runtime PM support" but without locking; use the
>   same function to mask all the interrupts and cleanup the status
>   flag in case interrupts are triggered after the transfer completion
>   timed out
> - introduce patch "i3c: renesas: Fix out-of-bounds access for newdevs mask"
>   to avoid failures when there are no I3C devices connected on the
>   bus at probe
> - patch "i3c: renesas: Perform Dynamic Address Assignment on resume"
>   was restored to the v1 variant; kept it simple to fix the
>   suspend/resume for the moment

Tested on RZ/G3E SMARC CARRIER II + RZ SMARC BREAKOUT board with one
P3T1085UK-ARD board connected.

Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>


Kind Regards,
Tommaso


> 
> Changes in v4:
> - dropped patch "i3c: renesas: Do not attach devices if xfer failed"
> - fixed the swap in renesas_i3c_group_devs_in_slots() for i3c->addr[]
>   renesas_i3c_irqs_mask_and_clear();
> - in patch 16/16 renamed renesas_i3c_abort_xfer() from v3 to
> 	
> Changes in v3:
> - re-based on top of series [1] to be able to use
>   i3c_master_reattach_i3c_dev_locked()
> - used i3c_master_reattach_i3c_dev_locked() on patch
>   "i3c: renesas: Perform Dynamic Address Assignment on resume" along with
>   grouping the I2C and I3C devices in the driver slot
> - collected tags
> 
> Changes in v2:
> - dropped patch "i3c: renesas: Use the divider 128"
> - adjusted the patches title and description where requested in the
>   review process
> - adjusted the DAA procedure after resume to still properly re-configure
>   the controller in case the bus was full before a suspend
> - added patch "i3c: renesas: Do not attach devices if xfer failed"
> - collected tags
> 
> Claudiu Beznea (17):
>   i3c: renesas: Check that the transfer is valid before accessing it
>   i3c: renesas: Restore STDBR and EXTBR registers on resume
>   i3c: renesas: Follow the reset deassert order used in probe
>   i3c: renesas: Reconfigure the DATBAS register on re-attach
>   i3c: renesas: Reset the controller on resume
>   i3c: renesas: Perform Dynamic Address Assignment on resume
>   i3c: renesas: Clean DATBAS register on detach
>   i3c: renesas: Fix out-of-bounds access for newdevs mask
>   i3c: renesas: Use reset_control_bulk_{assert, deassert}()
>   i3c: renesas: Return immediately if there is no transfer
>   i3c: renesas: Follow a unified pattern for transfer and command
>     initialization
>   i3c: renesas: Drop the explicit memset() call
>   i3c: renesas: Update HW registers after SW computations are done
>   i3c: renesas: Organize structures to avoid unnecessary padding
>   i3c: renesas: Use the "dev_name:irq_name" format for the interrupt
>     name
>   i3c: renesas: Drop unnecessary tab
>   i3c: renesas: Add runtime PM support
> 
>  drivers/i3c/master/renesas-i3c.c | 375 +++++++++++++++++++++----------
>  1 file changed, 261 insertions(+), 114 deletions(-)
> 
> -- 
> 2.43.0
> 

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

Re: [PATCH v5 00/17] i3c: renesas: Suspend to RAM with power loss and runtime PM

[Tommaso Merciai]

Hi Claudiu,
Thanks for your patch.

On Mon, Jul 13, 2026 at 04:05:28PM +0300, Claudiu Beznea wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Hi,
> 
> This series adjusts the suspend to RAM code to handle cases where power
> to the connected devices is lost during suspend to RAM. The fixes
> included in this series are required for that support.
> 
> Along with suspend to RAM support, runtime PM support is also added.
> Cleanup patches were included to prepare for clean runtime PM support.
> 
> Thank you,
> Claudiu
> 
> Changes in v5:
> - in patch 1 introduced renesas_i3c_irqs_mask_and_clear_locked()
>   similar to what was present in v4 in patch
>   "i3c: renesas: Add runtime PM support" but without locking; use the
>   same function to mask all the interrupts and cleanup the status
>   flag in case interrupts are triggered after the transfer completion
>   timed out
> - introduce patch "i3c: renesas: Fix out-of-bounds access for newdevs mask"
>   to avoid failures when there are no I3C devices connected on the
>   bus at probe
> - patch "i3c: renesas: Perform Dynamic Address Assignment on resume"
>   was restored to the v1 variant; kept it simple to fix the
>   suspend/resume for the moment

Tested on RZ/G3E SMARC CARRIER II + RZ SMARC BREAKOUT board with one
P3T1085UK-ARD board connected.

Tested-by: Tommaso Merciai <tommaso.merciai.xr@bp.renesas.com>


Kind Regards,
Tommaso


> 
> Changes in v4:
> - dropped patch "i3c: renesas: Do not attach devices if xfer failed"
> - fixed the swap in renesas_i3c_group_devs_in_slots() for i3c->addr[]
>   renesas_i3c_irqs_mask_and_clear();
> - in patch 16/16 renamed renesas_i3c_abort_xfer() from v3 to
> 	
> Changes in v3:
> - re-based on top of series [1] to be able to use
>   i3c_master_reattach_i3c_dev_locked()
> - used i3c_master_reattach_i3c_dev_locked() on patch
>   "i3c: renesas: Perform Dynamic Address Assignment on resume" along with
>   grouping the I2C and I3C devices in the driver slot
> - collected tags
> 
> Changes in v2:
> - dropped patch "i3c: renesas: Use the divider 128"
> - adjusted the patches title and description where requested in the
>   review process
> - adjusted the DAA procedure after resume to still properly re-configure
>   the controller in case the bus was full before a suspend
> - added patch "i3c: renesas: Do not attach devices if xfer failed"
> - collected tags
> 
> Claudiu Beznea (17):
>   i3c: renesas: Check that the transfer is valid before accessing it
>   i3c: renesas: Restore STDBR and EXTBR registers on resume
>   i3c: renesas: Follow the reset deassert order used in probe
>   i3c: renesas: Reconfigure the DATBAS register on re-attach
>   i3c: renesas: Reset the controller on resume
>   i3c: renesas: Perform Dynamic Address Assignment on resume
>   i3c: renesas: Clean DATBAS register on detach
>   i3c: renesas: Fix out-of-bounds access for newdevs mask
>   i3c: renesas: Use reset_control_bulk_{assert, deassert}()
>   i3c: renesas: Return immediately if there is no transfer
>   i3c: renesas: Follow a unified pattern for transfer and command
>     initialization
>   i3c: renesas: Drop the explicit memset() call
>   i3c: renesas: Update HW registers after SW computations are done
>   i3c: renesas: Organize structures to avoid unnecessary padding
>   i3c: renesas: Use the "dev_name:irq_name" format for the interrupt
>     name
>   i3c: renesas: Drop unnecessary tab
>   i3c: renesas: Add runtime PM support
> 
>  drivers/i3c/master/renesas-i3c.c | 375 +++++++++++++++++++++----------
>  1 file changed, 261 insertions(+), 114 deletions(-)
> 
> -- 
> 2.43.0
>