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

[PATCH v3 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.

Please note that series need to be applied on top of [1] since
patch "i3c: renesas: Perform Dynamic Address Assignment on resume"
uses i3c_master_reattach_i3c_dev_locked().

Thank you,
Claudiu

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

[1] https://lore.kernel.org/linux-i3c/20260525064209.2263045-1-lakshay.piplani@nxp.com/T/#m74ddb24e1e3a8006d933bd6106cc0c4508a11d2c

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: Do not attach devices if xfer failed
  i3c: renesas: Clean DATBAS register on detach
  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 | 540 +++++++++++++++++++++++--------
 1 file changed, 414 insertions(+), 126 deletions(-)

-- 
2.43.0


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

[PATCH v3 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.

Please note that series need to be applied on top of [1] since
patch "i3c: renesas: Perform Dynamic Address Assignment on resume"
uses i3c_master_reattach_i3c_dev_locked().

Thank you,
Claudiu

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

[1] https://lore.kernel.org/linux-i3c/20260525064209.2263045-1-lakshay.piplani@nxp.com/T/#m74ddb24e1e3a8006d933bd6106cc0c4508a11d2c

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: Do not attach devices if xfer failed
  i3c: renesas: Clean DATBAS register on detach
  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 | 540 +++++++++++++++++++++++--------
 1 file changed, 414 insertions(+), 126 deletions(-)

-- 
2.43.0

[PATCH v3 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. Along with it, clear any status flag to avoid
triggering the same interrupts again.

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 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 | 44 +++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index f39c449922ca..6e7ece2e0b4e 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1014,6 +1014,12 @@ static irqreturn_t renesas_i3c_tx_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			/* Clear the Transmit Buffer Empty status flag. */
+			renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
@@ -1053,11 +1059,18 @@ static irqreturn_t renesas_i3c_resp_isr(int irq, void *data)
 	int ret = 0;
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
+		/* Clear the Respone Queue Full status flag */
+		renesas_clear_bit(i3c->regs, NTST, NTST_RSPQFF);
+
 		xfer = i3c->xferqueue.cur;
-		cmd = xfer->cmds;
+		if (!xfer) {
+			/* Clear any error flags. */
+			renesas_clear_bit(i3c->regs, BCTL, BCTL_ABT);
+			renesas_clear_bit(i3c->regs, NTST, NTST_TEF | NTST_TABTF);
+			return IRQ_HANDLED;
+		}
 
-		/* Clear the Respone Queue Full status flag*/
-		renesas_clear_bit(i3c->regs, NTST, NTST_RSPQFF);
+		cmd = xfer->cmds;
 
 		data_len = NRSPQP_DATA_LEN(resp_descriptor);
 
@@ -1138,6 +1151,12 @@ static irqreturn_t renesas_i3c_tend_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			/* Clear any status flag. */
+			renesas_clear_bit(i3c->regs, BST, BST_NACKDF | BST_TENDF);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
@@ -1184,6 +1203,14 @@ static irqreturn_t renesas_i3c_rx_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			/* Clear any status registers. */
+			renesas_clear_bit(i3c->regs, BST, BST_SPCNDDF);
+			/* Clear the Read Buffer Full status flag. */
+			renesas_clear_bit(i3c->regs, NTST, NTST_RDBFF0);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
@@ -1235,6 +1262,11 @@ static irqreturn_t renesas_i3c_stop_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			/* Clear the RX/TX Data Buffer Full status flag. */
+			renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0);
+			return IRQ_HANDLED;
+		}
 
 		/* read back registers to confirm writes have fully propagated */
 		renesas_writel(i3c->regs, BST, 0);
@@ -1259,6 +1291,12 @@ static irqreturn_t renesas_i3c_start_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			/* Clear any status registers. */
+			renesas_clear_bit(i3c->regs, BST, BST_STCNDDF);
+			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 v3 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. Along with it, clear any status flag to avoid
triggering the same interrupts again.

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 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 | 44 +++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index f39c449922ca..6e7ece2e0b4e 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1014,6 +1014,12 @@ static irqreturn_t renesas_i3c_tx_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			/* Clear the Transmit Buffer Empty status flag. */
+			renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
@@ -1053,11 +1059,18 @@ static irqreturn_t renesas_i3c_resp_isr(int irq, void *data)
 	int ret = 0;
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
+		/* Clear the Respone Queue Full status flag */
+		renesas_clear_bit(i3c->regs, NTST, NTST_RSPQFF);
+
 		xfer = i3c->xferqueue.cur;
-		cmd = xfer->cmds;
+		if (!xfer) {
+			/* Clear any error flags. */
+			renesas_clear_bit(i3c->regs, BCTL, BCTL_ABT);
+			renesas_clear_bit(i3c->regs, NTST, NTST_TEF | NTST_TABTF);
+			return IRQ_HANDLED;
+		}
 
-		/* Clear the Respone Queue Full status flag*/
-		renesas_clear_bit(i3c->regs, NTST, NTST_RSPQFF);
+		cmd = xfer->cmds;
 
 		data_len = NRSPQP_DATA_LEN(resp_descriptor);
 
@@ -1138,6 +1151,12 @@ static irqreturn_t renesas_i3c_tend_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			/* Clear any status flag. */
+			renesas_clear_bit(i3c->regs, BST, BST_NACKDF | BST_TENDF);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
@@ -1184,6 +1203,14 @@ static irqreturn_t renesas_i3c_rx_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			/* Clear any status registers. */
+			renesas_clear_bit(i3c->regs, BST, BST_SPCNDDF);
+			/* Clear the Read Buffer Full status flag. */
+			renesas_clear_bit(i3c->regs, NTST, NTST_RDBFF0);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
@@ -1235,6 +1262,11 @@ static irqreturn_t renesas_i3c_stop_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			/* Clear the RX/TX Data Buffer Full status flag. */
+			renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0);
+			return IRQ_HANDLED;
+		}
 
 		/* read back registers to confirm writes have fully propagated */
 		renesas_writel(i3c->regs, BST, 0);
@@ -1259,6 +1291,12 @@ static irqreturn_t renesas_i3c_start_isr(int irq, void *data)
 
 	scoped_guard(spinlock, &i3c->xferqueue.lock) {
 		xfer = i3c->xferqueue.cur;
+		if (!xfer) {
+			/* Clear any status registers. */
+			renesas_clear_bit(i3c->regs, BST, BST_STCNDDF);
+			return IRQ_HANDLED;
+		}
+
 		cmd = xfer->cmds;
 
 		if (xfer->is_i2c_xfer) {
-- 
2.43.0

Re: [PATCH v3 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:

New issues:
- [High] Returning early from ISRs on the `!xfer` path without disabling the hardware interrupt enables or correctly resolving the interrupt conditions causes an interrupt storm (hard lockup).

Pre-existing issues:
- [High] Missing loop breaks on transfer timeouts lead to hardware state machine corruption, data races, and silent failures.
- [High] Read-Modify-Write (RMW) operations on status registers unintentionally clear active hardware interrupts, leading to lost interrupts and hangs.
--

commit 5e4d8b30b6f49a760746b92be5b09e02f9393a8d
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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

The patch checks if the transfer is valid before accessing it in the interrupt
handlers and clears status flags to avoid triggering the same interrupts again.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index f39c449922ca2..6e7ece2e0b4ee 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1014,6 +1014,12 @@ static irqreturn_t renesas_i3c_tx_isr(int irq, void *data)
>  
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			/* Clear the Transmit Buffer Empty status flag. */
> +			renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0);

[Severity: High]
Does clearing the status bit without satisfying the hardware condition or
disabling the interrupt mask leave the hardware condition active?

For level-sensitive interrupts like an empty TX FIFO (NTST_TDBEF0), clearing
the status bit without filling the FIFO or disabling the interrupt mask
(NTIE_TDBEIE0) might leave the hardware condition active.

Could the hardware immediately re-trigger the ISR upon return, causing an
infinite interrupt loop and CPU lockup?

[Severity: High]
This is a pre-existing issue, but does using renesas_clear_bit() to clear
hardware interrupts risk losing interrupts?

The renesas_clear_bit() helper performs a read-modify-write operation on
status registers. If a new hardware interrupt asserts between the read and
the write, writing back the read value of 0 could unintentionally clear the
newly asserted interrupt.

Could this cause the driver to lose interrupts and hang while waiting for
events that were accidentally cleared?

> +			return IRQ_HANDLED;
> +		}

[ ... ]

> @@ -1235,6 +1262,11 @@ static irqreturn_t renesas_i3c_stop_isr(int irq, void *data)
>  
>  	scoped_guard(spinlock, &i3c->xferqueue.lock) {
>  		xfer = i3c->xferqueue.cur;
> +		if (!xfer) {
> +			/* Clear the RX/TX Data Buffer Full status flag. */
> +			renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0);
> +			return IRQ_HANDLED;

[Severity: High]
Does this early return miss clearing the actual Stop Condition flag
(BST_SPCNDDF)?

If the Stop Condition flag is not cleared, could the interrupt remain
asserted and lead to an interrupt storm?

> +		}

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the loop in renesas_i3c_i3c_xfers()
fail to break on transfer timeouts?

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i3c_xfers() {
        ...
		time_left = renesas_i3c_wait_xfer(i3c, xfer);
		if (!time_left)
			abort_xfer = true;
	}

	if (abort_xfer)
		renesas_i3c_abort_xfer(i3c);

	return 0;
}

When renesas_i3c_wait_xfer() times out, abort_xfer is set to true but the
loop continues. The code proceeds to modify xfer->cmds for the next message
and re-enqueues the xfer structure while the hardware controller is still
actively processing the timed-out transfer.

Could this cause a data race on cmd and corrupt the hardware state machine?

Additionally, renesas_i3c_i3c_xfers() unconditionally returns 0, which seems
to falsely report success to the subsystem even if all transfers timed out.

A similar issue appears to exist in renesas_i3c_i2c_xfers():

drivers/i3c/master/renesas-i3c.c:renesas_i3c_i2c_xfers() {
        ...
		time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
		if (!time_left)
			abort_xfer = true;

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

Because the timeout does not set cmd->err, the loop fails to break and
continues to process subsequent messages.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=1

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

[PATCH v3 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 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 6e7ece2e0b4e..88a16efe096d 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;
@@ -607,10 +608,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

[PATCH v3 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 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 6e7ece2e0b4e..88a16efe096d 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;
@@ -607,10 +608,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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] The EXTBR configuration truncates `od_low_ticks` and `od_high_ticks` if they exceed 255, resulting in incorrect open-drain timing settings being cached and applied.
--

commit 2eb55ab7a61832c11e45d7cd3e0bd459e8db5dfc
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Restore STDBR and EXTBR registers on resume

This patch stores the Extended Bit Rate (EXTBR) settings during bus
initialization and explicitly restores both STDBR and EXTBR on resume
to preserve controller state across a power-loss suspend.

Fixes: e7218986319b ("i3c: renesas: Add suspend/resume support")
Cc: stable@vger.kernel.org

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 6e7ece2e0b4ee..88a16efe096de 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> @@ -607,10 +608,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);

[Severity: High]
This is a pre-existing issue, but does this calculation silently truncate
the open-drain ticks?

Looking earlier in renesas_i3c_bus_init(), the while-loop termination
condition allows od_low_ticks and od_high_ticks to exceed 255:

        if ((od_low_ticks / 2) <= 0xFF && pp_low_ticks < 0x3F)
            break;

The Standard Bit Rate (STDBR) configuration safely handles these larger
values by right-shifting them when double_SBR is set. However, the EXTBR
macros use FIELD_PREP(GENMASK(7, 0), x), which silently truncates any
values over 255.

Since this patch now explicitly caches the truncated EXTBR calculation to
persist it across system resumes, would it be better to address this
truncation so we don't restore broken timing configurations?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=2

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

[PATCH v3 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 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 88a16efe096d..4c86e7257804 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

[PATCH v3 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 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 88a16efe096d..4c86e7257804 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 v3 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 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 4c86e7257804..76a4831098c9 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -892,10 +892,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 v3 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 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 4c86e7257804..76a4831098c9 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -892,10 +892,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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The reattach_i3c_dev function unconditionally moves devices to a different free index on every call, causing endless index bouncing, array fragmentation, and unnecessary DATBAS register rewrites, which likely breaks Dynamic Address Assignment (DAA).
--

commit 15d5e07352154039ed134b971df0d63e73d607c0
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Reconfigure the DATBAS register on re-attach

This commit reconfigures the DATBAS register during device re-attach when a
device changes its position in the i3c->addrs[] array. It also updates the
software caches.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 4c86e72578043..76a4831098c94 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -892,10 +892,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) {

[Severity: High]
Does this logic unconditionally move devices to a different index on every
reattach call?

Because the device currently occupies data->index, that index's bit is cleared
in i3c->free_pos (meaning it is not free). Therefore, the lowest free slot
(pos) returned by renesas_i3c_get_free_pos() can never equal the currently
occupied slot (data->index).

As a result, this condition unconditionally evaluates to true on every
reattach call as long as a free slot exists. This forces the device to vacate
its current hardware index, move to the new position, and rewrite its DATBAS
registers, resulting in perpetual ping-ponging of slots instead of compacting
the array.

Furthermore, this fragmentation leaves lower indices free while upper ones are
occupied. During renesas_i3c_daa(), the controller calculates the number of
expected DAA devices as maxdevs - ret (where ret is the lowest free index).
If there are fragmented active devices at higher indices, will the hardware's
sequential DAA assignment overwrite their DATBAS configuration and lead to
functional failures?

> +		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);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=4

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

[PATCH v3 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 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 76a4831098c9..7ef317b2ba39 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -480,8 +480,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

[PATCH v3 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 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 76a4831098c9..7ef317b2ba39 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -480,8 +480,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

Re: [PATCH v3 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 resets (`tresetn` and `presetn`) are deasserted before clocks are enabled during resume, violating standard power-on sequences and contradicting the probe logic.
--

commit e6c25e39ab123d0505dce6bd4d588e973c083ab8
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Reset the controller on resume

This commit reorders the software reset (renesas_i3c_reset) during
resume to happen after the clocks are enabled. It also updates the
reset function to use an atomic polling mechanism since the resume callback
runs during the noirq phase.

Cc: stable@vger.kernel.org

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 76a4831098c94..7ef317b2ba39a 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 is a pre-existing issue, but the commit message states it aims to
"follow the same sequence as in probe", where clocks are enabled before
resets are deasserted.

While this patch moves the software reset to happen after the clocks are
enabled, the hardware resets (tresetn and presetn) are still deasserted
before clk_bulk_enable() is called in renesas_i3c_resume_noirq():

drivers/i3c/master/renesas-i3c.c:renesas_i3c_resume_noirq() {
    ...
	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);
    ...
}

Should the reset_control_deassert() calls also be moved to happen after
clk_bulk_enable() to fully align with the probe sequence and avoid releasing
synchronous hardware resets while the peripheral clock is disabled?

>  	/* 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/20260608201543.804902-1-claudiu.beznea@kernel.org?part=5

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

[PATCH v3 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().

To cover the case where the controller had already attached all the
i3c->maxdevs devices before a suspend/resume cycle and i3c->free_pos is
zero, struct renesas_i3c::resuming flag was introduced.

The flag is set in renesas_i3c_resume() before calling
i3c_master_do_daa_ext() and checked in renesas_i3c_daa().

When resuming from suspend, renesas_i3c_daa() re-runs DAA for all slots
except those used by I2C devices. Since I2C devices may be attached both
during probe (at bus initialization time) and at runtime, the device
slots maintained by the driver may become interleaved with both I2C and
I3C devices.

However, the controller performs dynamic address assignment for I3C
devices starting from a specified DATBAS register index and for a
specified number of I3C devices. This becomes a problem when the bus is
fully occupied by I2C and I3C devices before suspend.

To address this, I2C and I3C devices are grouped, if resuming, in the
i3c->addrs[] array such that I2C devices occupy the first slots and I3C
devices occupy the remaining ones. In this scenario, I3C devices are
re-attached using i3c_master_reattach_i3c_dev_locked(), since the bus is
fully occupied and renesas_i3c_get_free_pos() would otherwise return a
negative error code.

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 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 | 209 ++++++++++++++++++++++++-------
 1 file changed, 166 insertions(+), 43 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 7ef317b2ba39..c475d30a84c7 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -252,6 +252,15 @@ struct renesas_i3c_xferqueue {
 	spinlock_t lock;
 };
 
+struct renesas_i3c_addr {
+	union {
+		struct i2c_dev_desc *i2c_dev;
+		struct i3c_dev_desc *i3c_dev;
+	};
+	bool is_i2c;
+	u8 addr;
+};
+
 struct renesas_i3c {
 	struct i3c_master_controller base;
 	enum i3c_internal_state internal_state;
@@ -262,13 +271,13 @@ struct renesas_i3c {
 	u32 i3c_STDBR;
 	u32 extbr;
 	unsigned long rate;
-	u8 addrs[RENESAS_I3C_MAX_DEVS];
+	struct renesas_i3c_addr 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;
+	bool resuming;
 	u8 num_clks;
 	u8 refclk_div;
 };
@@ -335,7 +344,7 @@ static int renesas_i3c_get_addr_pos(struct renesas_i3c *i3c, u8 addr)
 	int pos;
 
 	for (pos = 0; pos < i3c->maxdevs; pos++) {
-		if (addr == i3c->addrs[pos])
+		if (addr == i3c->addrs[pos].addr)
 			return pos;
 	}
 
@@ -480,8 +489,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)
@@ -637,12 +646,80 @@ static void renesas_i3c_bus_cleanup(struct i3c_master_controller *m)
 	renesas_i3c_reset(i3c);
 }
 
+static u8 renesas_i3c_group_devs_in_slots(struct renesas_i3c *i3c)
+{
+	struct renesas_i3c_i2c_dev_data *new_i2c_data, *new_i3c_data;
+	struct renesas_i3c_addr i2c_addr, i3c_addr;
+	struct i3c_dev_desc *i3c_dev;
+	struct i2c_dev_desc *i2c_dev;
+	u8 i2c_pos = 0;
+
+	/*
+	 * The controller cannot handle interleaved I2C and I3C devices in the
+	 * slots. It expects to be configured with a starting DATBAS register
+	 * index and the number of I3C devices for which to run dynamic address
+	 * assignment.
+	 *
+	 * Since I2C devices may be added after bus initialization, group the
+	 * devices in slots so that the I2C devices occupy the first slots
+	 * and I3C devices the remaining ones.
+	 *
+	 * This allows the controller to run ENTDAA by specifying the starting
+	 * DATBAS register index and the number of I3C devices. As a result,
+	 * ENTDAA can be re-run for all I3C devices and they can be re-attached
+	 * after a suspend/resume cycle in which power to both the controller
+	 * and the I3C devices was lost.
+	 */
+	for (u8 pos = 0; pos < i3c->maxdevs; pos++) {
+		if (i3c->free_pos & BIT(pos))
+			continue;
+
+		if (!i3c->addrs[pos].is_i2c)
+			continue;
+
+		if (pos == i2c_pos) {
+			i2c_pos++;
+			continue;
+		}
+
+		/*
+		 * Swap the I3C device on i2c_pos slot with the I2C dev
+		 * on pos slot.
+		 */
+		i3c_addr = i3c->addrs[i2c_pos];
+		i3c_dev = i3c_addr.i3c_dev;
+
+		i2c_addr = i3c->addrs[pos];
+		i2c_dev = i2c_addr.i2c_dev;
+
+		new_i2c_data = i3c_dev_get_master_data(i3c_dev->dev->desc);
+		new_i3c_data = i2c_dev_get_master_data(i2c_dev);
+
+		i3c->addrs[i2c_pos] = i3c->addrs[pos];
+		i3c->addrs[i2c_pos].is_i2c = true;
+		i3c->addrs[i2c_pos].i2c_dev = i2c_dev;
+
+		i3c->addrs[pos] = i2c_addr;
+		i3c->addrs[pos].is_i2c = false;
+		i3c->addrs[pos].i3c_dev = i3c_dev;
+
+		new_i2c_data->index = i2c_pos++;
+		new_i3c_data->index = pos;
+
+		i2c_dev_set_master_data(i2c_dev, new_i2c_data);
+		i3c_dev_set_master_data(i3c_dev, new_i3c_data);
+	}
+
+	return i2c_pos;
+}
+
 static int renesas_i3c_daa(struct i3c_master_controller *m)
 {
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_cmd *cmd;
 	u32 olddevs, newdevs;
 	u8 last_addr = 0, pos;
+	u8 first_i3c_pos = 0;
 	int ret;
 
 	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
@@ -655,16 +732,25 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 	olddevs = ~(i3c->free_pos);
 	i3c->internal_state = I3C_INTERNAL_STATE_CONTROLLER_ENTDAA;
 
+	if (i3c->resuming)
+		first_i3c_pos = renesas_i3c_group_devs_in_slots(i3c);
+
 	/* Setting DATBASn registers for target devices. */
 	for (pos = 0; pos < i3c->maxdevs; pos++) {
-		if (olddevs & BIT(pos))
-			continue;
+		if (olddevs & BIT(pos)) {
+			if (i3c->resuming) {
+				if (pos < first_i3c_pos)
+					continue;
+			} else {
+				continue;
+			}
+		}
 
 		ret = i3c_master_get_free_addr(m, last_addr + 1);
 		if (ret < 0)
 			return -ENOSPC;
 
-		i3c->addrs[pos] = ret;
+		i3c->addrs[pos].addr = ret;
 		last_addr = ret;
 
 		renesas_writel(i3c->regs, DATBAS(pos), datbas_dvdyad_with_parity(ret));
@@ -674,9 +760,14 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 	cmd = xfer->cmds;
 	cmd->rx_count = 0;
 
-	ret = renesas_i3c_get_free_pos(i3c);
-	if (ret < 0)
-		return ret;
+	if (i3c->resuming) {
+		/* Run ENTDAA for all the I3C devices on the bus, if resuming. */
+		ret = first_i3c_pos;
+	} else {
+		ret = renesas_i3c_get_free_pos(i3c);
+		if (ret < 0)
+			return ret;
+	}
 
 	/*
 	 * Setup the command descriptor to start the ENTDAA command
@@ -689,12 +780,38 @@ 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);
-	newdevs &= ~olddevs;
+	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
+	/* Re-attach all the I3C devices on resume. */
+	if (!i3c->resuming)
+		newdevs &= ~olddevs;
 
 	for (pos = 0; pos < i3c->maxdevs; pos++) {
-		if (newdevs & BIT(pos))
-			i3c_master_add_i3c_dev_locked(m, i3c->addrs[pos]);
+		if (!(newdevs & BIT(pos)))
+			continue;
+
+		/*
+		 * If the bus was fully occupied before suspend, re-attach the
+		 * devices and update the addresses tracked by the subsystem.
+		 * Without this, i3c_master_add_i3c_dev_locked() returns errors
+		 * due to a lack of free slots (as reported by
+		 * renesas_i3c_get_free_pos()), causing the attachment to fail.
+		 *
+		 * Otherwise, follow the normal
+		 * i3c_master_add_i3c_dev_locked() path, which will re-attach the
+		 * devices, keep the subsystem state and driver addresses in sync,
+		 * and perform all the required internal bookkeeping.
+		 */
+		if (!i3c->free_pos && i3c->resuming) {
+			struct i3c_dev_desc *dev = i3c->addrs[pos].i3c_dev->dev->desc;
+			u8 old_dyn_addr;
+
+			old_dyn_addr = dev->info.dyn_addr;
+			dev->info.dyn_addr = i3c->addrs[pos].addr;
+
+			i3c_master_reattach_i3c_dev_locked(dev, old_dyn_addr);
+		} else {
+			i3c_master_add_i3c_dev_locked(m, i3c->addrs[pos].addr);
+		}
 	}
 
 	return 0;
@@ -876,11 +993,12 @@ static int renesas_i3c_attach_i3c_dev(struct i3c_dev_desc *dev)
 		return -ENOMEM;
 
 	data->index = pos;
-	i3c->addrs[pos] = dev->info.dyn_addr ? : dev->info.static_addr;
+	i3c->addrs[pos].addr = dev->info.dyn_addr ? : dev->info.static_addr;
+	i3c->addrs[pos].i3c_dev = dev;
 	i3c->free_pos &= ~BIT(pos);
 
 	renesas_writel(i3c->regs, DATBAS(pos), DATBAS_DVSTAD(dev->info.static_addr) |
-				    datbas_dvdyad_with_parity(i3c->addrs[pos]));
+				    datbas_dvdyad_with_parity(i3c->addrs[pos].addr));
 	i3c_dev_set_master_data(dev, data);
 
 	return 0;
@@ -892,25 +1010,28 @@ 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);
+	struct i3c_dev_desc *tmp_dev = i3c->addrs[data->index].i3c_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->addrs[data->index].addr = 0;
+		i3c->addrs[data->index].i3c_dev = NULL;
 		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 :
+	i3c->addrs[data->index].addr = dev->info.dyn_addr ? dev->info.dyn_addr :
 							dev->info.static_addr;
+	i3c->addrs[data->index].i3c_dev = tmp_dev;
 
 	renesas_writel(i3c->regs, DATBAS(data->index),
 		       DATBAS_DVSTAD(dev->info.static_addr) |
-		       datbas_dvdyad_with_parity(i3c->addrs[data->index]));
+		       datbas_dvdyad_with_parity(i3c->addrs[data->index].addr));
 
 	return 0;
 }
@@ -922,7 +1043,8 @@ static void renesas_i3c_detach_i3c_dev(struct i3c_dev_desc *dev)
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 
 	i3c_dev_set_master_data(dev, NULL);
-	i3c->addrs[data->index] = 0;
+	i3c->addrs[data->index].addr = 0;
+	i3c->addrs[data->index].i3c_dev = NULL;
 	i3c->free_pos |= BIT(data->index);
 	kfree(data);
 }
@@ -1002,7 +1124,9 @@ static int renesas_i3c_attach_i2c_dev(struct i2c_dev_desc *dev)
 		return -ENOMEM;
 
 	data->index = pos;
-	i3c->addrs[pos] = dev->addr;
+	i3c->addrs[pos].addr = dev->addr;
+	i3c->addrs[pos].is_i2c = true;
+	i3c->addrs[pos].i2c_dev = dev;
 	i3c->free_pos &= ~BIT(pos);
 	i2c_dev_set_master_data(dev, data);
 
@@ -1016,7 +1140,9 @@ static void renesas_i3c_detach_i2c_dev(struct i2c_dev_desc *dev)
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 
 	i2c_dev_set_master_data(dev, NULL);
-	i3c->addrs[data->index] = 0;
+	i3c->addrs[data->index].addr = 0;
+	i3c->addrs[data->index].is_i2c = false;
+	i3c->addrs[data->index].i2c_dev = NULL;
 	i3c->free_pos |= BIT(data->index);
 	kfree(data);
 }
@@ -1419,12 +1545,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 +1555,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 +1582,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 +1611,23 @@ 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);
 
+	i3c->resuming = true;
+
+	ret = i3c_master_do_daa_ext(&i3c->base, true);
+	if (ret)
+		dev_err(dev, "DAA failed on resume, ret=%d", ret);
+
+	i3c->resuming = false;
+
 	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 +1640,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

[PATCH v3 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().

To cover the case where the controller had already attached all the
i3c->maxdevs devices before a suspend/resume cycle and i3c->free_pos is
zero, struct renesas_i3c::resuming flag was introduced.

The flag is set in renesas_i3c_resume() before calling
i3c_master_do_daa_ext() and checked in renesas_i3c_daa().

When resuming from suspend, renesas_i3c_daa() re-runs DAA for all slots
except those used by I2C devices. Since I2C devices may be attached both
during probe (at bus initialization time) and at runtime, the device
slots maintained by the driver may become interleaved with both I2C and
I3C devices.

However, the controller performs dynamic address assignment for I3C
devices starting from a specified DATBAS register index and for a
specified number of I3C devices. This becomes a problem when the bus is
fully occupied by I2C and I3C devices before suspend.

To address this, I2C and I3C devices are grouped, if resuming, in the
i3c->addrs[] array such that I2C devices occupy the first slots and I3C
devices occupy the remaining ones. In this scenario, I3C devices are
re-attached using i3c_master_reattach_i3c_dev_locked(), since the bus is
fully occupied and renesas_i3c_get_free_pos() would otherwise return a
negative error code.

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 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 | 209 ++++++++++++++++++++++++-------
 1 file changed, 166 insertions(+), 43 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 7ef317b2ba39..c475d30a84c7 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -252,6 +252,15 @@ struct renesas_i3c_xferqueue {
 	spinlock_t lock;
 };
 
+struct renesas_i3c_addr {
+	union {
+		struct i2c_dev_desc *i2c_dev;
+		struct i3c_dev_desc *i3c_dev;
+	};
+	bool is_i2c;
+	u8 addr;
+};
+
 struct renesas_i3c {
 	struct i3c_master_controller base;
 	enum i3c_internal_state internal_state;
@@ -262,13 +271,13 @@ struct renesas_i3c {
 	u32 i3c_STDBR;
 	u32 extbr;
 	unsigned long rate;
-	u8 addrs[RENESAS_I3C_MAX_DEVS];
+	struct renesas_i3c_addr 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;
+	bool resuming;
 	u8 num_clks;
 	u8 refclk_div;
 };
@@ -335,7 +344,7 @@ static int renesas_i3c_get_addr_pos(struct renesas_i3c *i3c, u8 addr)
 	int pos;
 
 	for (pos = 0; pos < i3c->maxdevs; pos++) {
-		if (addr == i3c->addrs[pos])
+		if (addr == i3c->addrs[pos].addr)
 			return pos;
 	}
 
@@ -480,8 +489,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)
@@ -637,12 +646,80 @@ static void renesas_i3c_bus_cleanup(struct i3c_master_controller *m)
 	renesas_i3c_reset(i3c);
 }
 
+static u8 renesas_i3c_group_devs_in_slots(struct renesas_i3c *i3c)
+{
+	struct renesas_i3c_i2c_dev_data *new_i2c_data, *new_i3c_data;
+	struct renesas_i3c_addr i2c_addr, i3c_addr;
+	struct i3c_dev_desc *i3c_dev;
+	struct i2c_dev_desc *i2c_dev;
+	u8 i2c_pos = 0;
+
+	/*
+	 * The controller cannot handle interleaved I2C and I3C devices in the
+	 * slots. It expects to be configured with a starting DATBAS register
+	 * index and the number of I3C devices for which to run dynamic address
+	 * assignment.
+	 *
+	 * Since I2C devices may be added after bus initialization, group the
+	 * devices in slots so that the I2C devices occupy the first slots
+	 * and I3C devices the remaining ones.
+	 *
+	 * This allows the controller to run ENTDAA by specifying the starting
+	 * DATBAS register index and the number of I3C devices. As a result,
+	 * ENTDAA can be re-run for all I3C devices and they can be re-attached
+	 * after a suspend/resume cycle in which power to both the controller
+	 * and the I3C devices was lost.
+	 */
+	for (u8 pos = 0; pos < i3c->maxdevs; pos++) {
+		if (i3c->free_pos & BIT(pos))
+			continue;
+
+		if (!i3c->addrs[pos].is_i2c)
+			continue;
+
+		if (pos == i2c_pos) {
+			i2c_pos++;
+			continue;
+		}
+
+		/*
+		 * Swap the I3C device on i2c_pos slot with the I2C dev
+		 * on pos slot.
+		 */
+		i3c_addr = i3c->addrs[i2c_pos];
+		i3c_dev = i3c_addr.i3c_dev;
+
+		i2c_addr = i3c->addrs[pos];
+		i2c_dev = i2c_addr.i2c_dev;
+
+		new_i2c_data = i3c_dev_get_master_data(i3c_dev->dev->desc);
+		new_i3c_data = i2c_dev_get_master_data(i2c_dev);
+
+		i3c->addrs[i2c_pos] = i3c->addrs[pos];
+		i3c->addrs[i2c_pos].is_i2c = true;
+		i3c->addrs[i2c_pos].i2c_dev = i2c_dev;
+
+		i3c->addrs[pos] = i2c_addr;
+		i3c->addrs[pos].is_i2c = false;
+		i3c->addrs[pos].i3c_dev = i3c_dev;
+
+		new_i2c_data->index = i2c_pos++;
+		new_i3c_data->index = pos;
+
+		i2c_dev_set_master_data(i2c_dev, new_i2c_data);
+		i3c_dev_set_master_data(i3c_dev, new_i3c_data);
+	}
+
+	return i2c_pos;
+}
+
 static int renesas_i3c_daa(struct i3c_master_controller *m)
 {
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_cmd *cmd;
 	u32 olddevs, newdevs;
 	u8 last_addr = 0, pos;
+	u8 first_i3c_pos = 0;
 	int ret;
 
 	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
@@ -655,16 +732,25 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 	olddevs = ~(i3c->free_pos);
 	i3c->internal_state = I3C_INTERNAL_STATE_CONTROLLER_ENTDAA;
 
+	if (i3c->resuming)
+		first_i3c_pos = renesas_i3c_group_devs_in_slots(i3c);
+
 	/* Setting DATBASn registers for target devices. */
 	for (pos = 0; pos < i3c->maxdevs; pos++) {
-		if (olddevs & BIT(pos))
-			continue;
+		if (olddevs & BIT(pos)) {
+			if (i3c->resuming) {
+				if (pos < first_i3c_pos)
+					continue;
+			} else {
+				continue;
+			}
+		}
 
 		ret = i3c_master_get_free_addr(m, last_addr + 1);
 		if (ret < 0)
 			return -ENOSPC;
 
-		i3c->addrs[pos] = ret;
+		i3c->addrs[pos].addr = ret;
 		last_addr = ret;
 
 		renesas_writel(i3c->regs, DATBAS(pos), datbas_dvdyad_with_parity(ret));
@@ -674,9 +760,14 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 	cmd = xfer->cmds;
 	cmd->rx_count = 0;
 
-	ret = renesas_i3c_get_free_pos(i3c);
-	if (ret < 0)
-		return ret;
+	if (i3c->resuming) {
+		/* Run ENTDAA for all the I3C devices on the bus, if resuming. */
+		ret = first_i3c_pos;
+	} else {
+		ret = renesas_i3c_get_free_pos(i3c);
+		if (ret < 0)
+			return ret;
+	}
 
 	/*
 	 * Setup the command descriptor to start the ENTDAA command
@@ -689,12 +780,38 @@ 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);
-	newdevs &= ~olddevs;
+	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
+	/* Re-attach all the I3C devices on resume. */
+	if (!i3c->resuming)
+		newdevs &= ~olddevs;
 
 	for (pos = 0; pos < i3c->maxdevs; pos++) {
-		if (newdevs & BIT(pos))
-			i3c_master_add_i3c_dev_locked(m, i3c->addrs[pos]);
+		if (!(newdevs & BIT(pos)))
+			continue;
+
+		/*
+		 * If the bus was fully occupied before suspend, re-attach the
+		 * devices and update the addresses tracked by the subsystem.
+		 * Without this, i3c_master_add_i3c_dev_locked() returns errors
+		 * due to a lack of free slots (as reported by
+		 * renesas_i3c_get_free_pos()), causing the attachment to fail.
+		 *
+		 * Otherwise, follow the normal
+		 * i3c_master_add_i3c_dev_locked() path, which will re-attach the
+		 * devices, keep the subsystem state and driver addresses in sync,
+		 * and perform all the required internal bookkeeping.
+		 */
+		if (!i3c->free_pos && i3c->resuming) {
+			struct i3c_dev_desc *dev = i3c->addrs[pos].i3c_dev->dev->desc;
+			u8 old_dyn_addr;
+
+			old_dyn_addr = dev->info.dyn_addr;
+			dev->info.dyn_addr = i3c->addrs[pos].addr;
+
+			i3c_master_reattach_i3c_dev_locked(dev, old_dyn_addr);
+		} else {
+			i3c_master_add_i3c_dev_locked(m, i3c->addrs[pos].addr);
+		}
 	}
 
 	return 0;
@@ -876,11 +993,12 @@ static int renesas_i3c_attach_i3c_dev(struct i3c_dev_desc *dev)
 		return -ENOMEM;
 
 	data->index = pos;
-	i3c->addrs[pos] = dev->info.dyn_addr ? : dev->info.static_addr;
+	i3c->addrs[pos].addr = dev->info.dyn_addr ? : dev->info.static_addr;
+	i3c->addrs[pos].i3c_dev = dev;
 	i3c->free_pos &= ~BIT(pos);
 
 	renesas_writel(i3c->regs, DATBAS(pos), DATBAS_DVSTAD(dev->info.static_addr) |
-				    datbas_dvdyad_with_parity(i3c->addrs[pos]));
+				    datbas_dvdyad_with_parity(i3c->addrs[pos].addr));
 	i3c_dev_set_master_data(dev, data);
 
 	return 0;
@@ -892,25 +1010,28 @@ 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);
+	struct i3c_dev_desc *tmp_dev = i3c->addrs[data->index].i3c_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->addrs[data->index].addr = 0;
+		i3c->addrs[data->index].i3c_dev = NULL;
 		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 :
+	i3c->addrs[data->index].addr = dev->info.dyn_addr ? dev->info.dyn_addr :
 							dev->info.static_addr;
+	i3c->addrs[data->index].i3c_dev = tmp_dev;
 
 	renesas_writel(i3c->regs, DATBAS(data->index),
 		       DATBAS_DVSTAD(dev->info.static_addr) |
-		       datbas_dvdyad_with_parity(i3c->addrs[data->index]));
+		       datbas_dvdyad_with_parity(i3c->addrs[data->index].addr));
 
 	return 0;
 }
@@ -922,7 +1043,8 @@ static void renesas_i3c_detach_i3c_dev(struct i3c_dev_desc *dev)
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 
 	i3c_dev_set_master_data(dev, NULL);
-	i3c->addrs[data->index] = 0;
+	i3c->addrs[data->index].addr = 0;
+	i3c->addrs[data->index].i3c_dev = NULL;
 	i3c->free_pos |= BIT(data->index);
 	kfree(data);
 }
@@ -1002,7 +1124,9 @@ static int renesas_i3c_attach_i2c_dev(struct i2c_dev_desc *dev)
 		return -ENOMEM;
 
 	data->index = pos;
-	i3c->addrs[pos] = dev->addr;
+	i3c->addrs[pos].addr = dev->addr;
+	i3c->addrs[pos].is_i2c = true;
+	i3c->addrs[pos].i2c_dev = dev;
 	i3c->free_pos &= ~BIT(pos);
 	i2c_dev_set_master_data(dev, data);
 
@@ -1016,7 +1140,9 @@ static void renesas_i3c_detach_i2c_dev(struct i2c_dev_desc *dev)
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 
 	i2c_dev_set_master_data(dev, NULL);
-	i3c->addrs[data->index] = 0;
+	i3c->addrs[data->index].addr = 0;
+	i3c->addrs[data->index].is_i2c = false;
+	i3c->addrs[data->index].i2c_dev = NULL;
 	i3c->free_pos |= BIT(data->index);
 	kfree(data);
 }
@@ -1419,12 +1545,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 +1555,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 +1582,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 +1611,23 @@ 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);
 
+	i3c->resuming = true;
+
+	ret = i3c_master_do_daa_ext(&i3c->base, true);
+	if (ret)
+		dev_err(dev, "DAA failed on resume, ret=%d", ret);
+
+	i3c->resuming = false;
+
 	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 +1640,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

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

[sashiko-bot]

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

New issues:
- [Critical] `renesas_i3c_group_devs_in_slots()` crashes on free slots and fails to update the `free_pos` bitmask.
- [Critical] Typo in array swapping logic duplicates I2C devices and corrupts driver state.
- [High] Call to undefined function `i3c_master_reattach_i3c_dev_locked` and logically invalid usage of the reattach API.
- [High] Bypassing `i3c_master_add_i3c_dev_locked()` on resume ignores UID arbitration order, breaking device isolation.
- [High] Redundant and unsafe dereference of `->dev->desc` when retrieving master data.
- [High] Stale `olddevs` mask corrupts DATBAS registers for relocated I2C devices.

Pre-existing issues:
- [High] Integer underflow in `GENMASK` calculation in `renesas_i3c_daa()` causes undefined behavior.
--

commit 3b3dbe672a58e6c80b4a3c7aad942c61076f9cd2
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Perform Dynamic Address Assignment on resume

The patch modifies the Renesas I3C driver to run Dynamic Address Assignment
(DAA) in the controller resume path to restore communication with I3C devices
that lose power during suspend. It groups I2C and I3C devices during resume
and re-runs ENTDAA for I3C devices.

> 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

[ ... ]

> +static u8 renesas_i3c_group_devs_in_slots(struct renesas_i3c *i3c)
> +{
> +	struct renesas_i3c_i2c_dev_data *new_i2c_data, *new_i3c_data;
> +	struct renesas_i3c_addr i2c_addr, i3c_addr;
> +	struct i3c_dev_desc *i3c_dev;
> +	struct i2c_dev_desc *i2c_dev;
> +	u8 i2c_pos = 0;

[ ... ]

> +	for (u8 pos = 0; pos < i3c->maxdevs; pos++) {
> +		if (i3c->free_pos & BIT(pos))
> +			continue;
> +
> +		if (!i3c->addrs[pos].is_i2c)
> +			continue;
> +
> +		if (pos == i2c_pos) {
> +			i2c_pos++;
> +			continue;
> +		}
> +
> +		/*
> +		 * Swap the I3C device on i2c_pos slot with the I2C dev
> +		 * on pos slot.
> +		 */
> +		i3c_addr = i3c->addrs[i2c_pos];
> +		i3c_dev = i3c_addr.i3c_dev;
> +
> +		i2c_addr = i3c->addrs[pos];

[Severity: Critical]
If i2c_pos points to a free slot because earlier iterations skipped free
slots without advancing i2c_pos, won't i3c_dev be NULL?

Furthermore, does this function need to update i3c->free_pos when swapping
slots to ensure the bitmask stays synchronized with the array layout?

> +		i2c_dev = i2c_addr.i2c_dev;
> +
> +		new_i2c_data = i3c_dev_get_master_data(i3c_dev->dev->desc);

[Severity: High]
Is the ->dev->desc dereference here safe?

Since i3c_dev is already a struct i3c_dev_desc *, the dev field might
be NULL if the device was only partially attached or memory allocation failed,
causing a crash. i3c_dev_get_master_data() can accept i3c_dev directly.

> +		new_i3c_data = i2c_dev_get_master_data(i2c_dev);
> +
> +		i3c->addrs[i2c_pos] = i3c->addrs[pos];
> +		i3c->addrs[i2c_pos].is_i2c = true;
> +		i3c->addrs[i2c_pos].i2c_dev = i2c_dev;
> +
> +		i3c->addrs[pos] = i2c_addr;

[Severity: Critical]
Since i2c_addr was initialized to i3c->addrs[pos] above, doesn't this
assign the entry to itself?

This would duplicate the I2C device into both slots, destroy the original I3C
device's dynamic address state, and cross-assign private data structures.
Should i3c_addr be used instead?

> +		i3c->addrs[pos].is_i2c = false;
> +		i3c->addrs[pos].i3c_dev = i3c_dev;
> +
> +		new_i2c_data->index = i2c_pos++;
> +		new_i3c_data->index = pos;
> +
> +		i2c_dev_set_master_data(i2c_dev, new_i2c_data);
> +		i3c_dev_set_master_data(i3c_dev, new_i3c_data);

[ ... ]

>  static int renesas_i3c_daa(struct i3c_master_controller *m)
>  {
>  	struct renesas_i3c *i3c = to_renesas_i3c(m);
>  	struct renesas_i3c_cmd *cmd;
>  	u32 olddevs, newdevs;
>  	u8 last_addr = 0, pos;
> +	u8 first_i3c_pos = 0;
>  	int ret;

[ ... ]

> @@ -655,16 +732,25 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
>  	olddevs = ~(i3c->free_pos);
>  	i3c->internal_state = I3C_INTERNAL_STATE_CONTROLLER_ENTDAA;
>  
> +	if (i3c->resuming)
> +		first_i3c_pos = renesas_i3c_group_devs_in_slots(i3c);

[Severity: High]
Since olddevs is computed from i3c->free_pos before calling
renesas_i3c_group_devs_in_slots(), which shuffles the array but doesn't
update i3c->free_pos, is it possible for olddevs to be stale?

If an I2C device is relocated into an empty slot where the stale mask is 0,
the subsequent DAA loop might fall through and overwrite the I2C device's slot
with a new I3C dynamic address.

> +
>  	/* Setting DATBASn registers for target devices. */
>  	for (pos = 0; pos < i3c->maxdevs; pos++) {

[ ... ]

> @@ -689,12 +780,38 @@ 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);
> -	newdevs &= ~olddevs;
> +	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);

[Severity: High]
This is a pre-existing issue, but if cmd->rx_count equals i3c->maxdevs
(e.g., when no devices are assigned), the first argument to GENMASK becomes
-1.

Doesn't this invoke undefined behavior via an invalid shift count in the
GENMASK macro, returning garbage values?

> +	/* Re-attach all the I3C devices on resume. */
> +	if (!i3c->resuming)
> +		newdevs &= ~olddevs;
>  
>  	for (pos = 0; pos < i3c->maxdevs; pos++) {
> -		if (newdevs & BIT(pos))
> -			i3c_master_add_i3c_dev_locked(m, i3c->addrs[pos]);
> +		if (!(newdevs & BIT(pos)))
> +			continue;
> +
> +		/*
> +		 * If the bus was fully occupied before suspend, re-attach the
> +		 * devices and update the addresses tracked by the subsystem.
> +		 * Without this, i3c_master_add_i3c_dev_locked() returns errors
> +		 * due to a lack of free slots (as reported by
> +		 * renesas_i3c_get_free_pos()), causing the attachment to fail.
> +		 *
> +		 * Otherwise, follow the normal
> +		 * i3c_master_add_i3c_dev_locked() path, which will re-attach the
> +		 * devices, keep the subsystem state and driver addresses in sync,
> +		 * and perform all the required internal bookkeeping.
> +		 */
> +		if (!i3c->free_pos && i3c->resuming) {
> +			struct i3c_dev_desc *dev = i3c->addrs[pos].i3c_dev->dev->desc;

[Severity: High]
Is it safe to dereference i3c_dev->dev->desc here?

If dev is NULL due to partial attachment or memory allocation failure prior
to suspend, this will cause a kernel crash.

> +			u8 old_dyn_addr;
> +
> +			old_dyn_addr = dev->info.dyn_addr;
> +			dev->info.dyn_addr = i3c->addrs[pos].addr;
> +
> +			i3c_master_reattach_i3c_dev_locked(dev, old_dyn_addr);

[Severity: High]
Does i3c_master_reattach_i3c_dev_locked() exist in the kernel?

Calling a non-existent function will result in a build failure. Additionally,
even if this was intended to be i3c_master_reattach_i3c_dev(), that API
sends a SETNEWDA CCC to the device's old dynamic address. Since ENTDAA just
assigned a new dynamic address via hardware, won't the device NACK the
command?

Furthermore, by bypassing i3c_master_add_i3c_dev_locked(), does this ignore
the PID arbitration order? ENTDAA assigns addresses based on PID arbitration,
so the device that won arbitration might not correspond to the descriptor at
i3c->addrs[pos], breaking device isolation.

> +		} else {
> +			i3c_master_add_i3c_dev_locked(m, i3c->addrs[pos].addr);
> +		}
>  	}
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=6

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

[PATCH v3 07/17] i3c: renesas: Do not attach devices if xfer failed

[Claudiu Beznea]

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

The controller may return an NRSPQP_ERROR_* error code while still
providing a valid cmd->rx_count. It has been observed that when the
transfer fails with NRSPQP_ERROR_ADDRESS_NACK, calling
i3c_master_add_i3c_dev_locked() may lead to crashes. Set newdevs to zero
if the transfer failed.

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 v3:
- none

Changes in v2:
- none, this patch is new

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

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index c475d30a84c7..f96848022c45 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -780,10 +780,15 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 
 	renesas_i3c_wait_xfer(i3c, xfer);
 
-	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
-	/* Re-attach all the I3C devices on resume. */
-	if (!i3c->resuming)
-		newdevs &= ~olddevs;
+	/* Skip attaching if there are failures on the xfer. */
+	if (xfer->ret) {
+		newdevs = 0;
+	} else {
+		newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
+		/* Re-attach all the I3C devices on resume. */
+		if (!i3c->resuming)
+			newdevs &= ~olddevs;
+	}
 
 	for (pos = 0; pos < i3c->maxdevs; pos++) {
 		if (!(newdevs & BIT(pos)))
-- 
2.43.0


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

[PATCH v3 07/17] i3c: renesas: Do not attach devices if xfer failed

[Claudiu Beznea]

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

The controller may return an NRSPQP_ERROR_* error code while still
providing a valid cmd->rx_count. It has been observed that when the
transfer fails with NRSPQP_ERROR_ADDRESS_NACK, calling
i3c_master_add_i3c_dev_locked() may lead to crashes. Set newdevs to zero
if the transfer failed.

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 v3:
- none

Changes in v2:
- none, this patch is new

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

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index c475d30a84c7..f96848022c45 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -780,10 +780,15 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
 
 	renesas_i3c_wait_xfer(i3c, xfer);
 
-	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
-	/* Re-attach all the I3C devices on resume. */
-	if (!i3c->resuming)
-		newdevs &= ~olddevs;
+	/* Skip attaching if there are failures on the xfer. */
+	if (xfer->ret) {
+		newdevs = 0;
+	} else {
+		newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
+		/* Re-attach all the I3C devices on resume. */
+		if (!i3c->resuming)
+			newdevs &= ~olddevs;
+	}
 
 	for (pos = 0; pos < i3c->maxdevs; pos++) {
 		if (!(newdevs & BIT(pos)))
-- 
2.43.0

Re: [PATCH v3 07/17] i3c: renesas: Do not attach devices if xfer failed

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch prevents discovery of I3C devices by unconditionally discarding successfully attached devices when a DAA transfer terminates with an expected Address NACK, leading to permanently orphaned devices and address collisions.
--

commit 03ecc946e72f3aecbac0cad85676f818d253cd2c
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Do not attach devices if xfer failed

This commit attempts to fix a crash during device attachment when a transfer
fails. It modifies the logic to zero out the new devices mask if the
transfer reports an error, bypassing the attachment loop for those devices.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index c475d30a84c7c..f96848022c45d 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -780,10 +780,15 @@ static int renesas_i3c_daa(struct i3c_master_controller *m)
>  
>  	renesas_i3c_wait_xfer(i3c, xfer);
>  
> -	newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
> -	/* Re-attach all the I3C devices on resume. */
> -	if (!i3c->resuming)
> -		newdevs &= ~olddevs;
> +	/* Skip attaching if there are failures on the xfer. */
> +	if (xfer->ret) {
> +		newdevs = 0;
> +	} else {

[Severity: High]
Does this incorrectly discard successfully attached devices during a partial
device discovery?

During the ENTDAA procedure, the controller assigns addresses sequentially.
When there are fewer unassigned devices on the bus than the requested slots,
the controller eventually receives an address NACK, which is the standard
termination condition for the loop.

This expected NACK translates to a non-zero xfer->ret, but cmd->rx_count
still holds the valid count of unassigned slots. If three devices attach
successfully and the fourth attempt NACKs, xfer->ret is set, and this
logic will set newdevs to 0. 

Could this result in discarding the three successfully attached devices and
leaving them orphaned on the bus with active dynamic addresses, breaking
discovery for any bus that is not fully populated?

> +		newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
> +		/* Re-attach all the I3C devices on resume. */
> +		if (!i3c->resuming)
> +			newdevs &= ~olddevs;
> +	}
>  
>  	for (pos = 0; pos < i3c->maxdevs; pos++) {
>  		if (!(newdevs & BIT(pos)))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=7

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

[PATCH v3 08/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 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 f96848022c45..195c40956148 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1047,6 +1047,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].addr = 0;
 	i3c->addrs[data->index].i3c_dev = NULL;
-- 
2.43.0

[PATCH v3 08/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 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 f96848022c45..195c40956148 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1047,6 +1047,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].addr = 0;
 	i3c->addrs[data->index].i3c_dev = NULL;
-- 
2.43.0


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

Re: [PATCH v3 08/17] i3c: renesas: Clean DATBAS register on detach

[sashiko-bot]

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

Pre-existing issues:
- [High] NULL pointer dereferences in slot grouping and unnecessary indirection during resume.
- [High] Undefined behavior from GENMASK when no devices attach during ENTDAA.
- [High] Hardware state corruption due to missing cleanup of DATBAS registers in DAA.
--

commit 0d20150d70b84df8abaeebdb009b4fee9f1d4c9b
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 f96848022c45d..195c40956148b 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1047,6 +1047,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);
> +

[Severity: High]
This isn't a bug introduced by this patch, but since we are clearing DATBAS
registers here to avoid hardware issues, should we also clean them up in the
error and skip paths of renesas_i3c_daa()?

In renesas_i3c_daa(), we eagerly program DATBAS for all free slots before
initiating ENTDAA:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_daa() {
    ...
		ret = i3c_master_get_free_addr(m, last_addr + 1);
		if (ret < 0)
			return -ENOSPC;

		i3c->addrs[pos].addr = ret;
		last_addr = ret;

		renesas_writel(i3c->regs, DATBAS(pos), datbas_dvdyad_with_parity(ret));
    ...
}

If i3c_master_get_free_addr() fails midway, it returns -ENOSPC without clearing
the previously programmed DATBAS registers. 

Additionally, if ENTDAA assigns fewer devices than available slots, the
remaining unassigned slots are skipped without reverting their DATBAS registers
to 0:

drivers/i3c/master/renesas-i3c.c:renesas_i3c_daa() {
    ...
	for (pos = 0; pos < i3c->maxdevs; pos++) {
		if (!(newdevs & BIT(pos)))
			continue;
    ...
}

Could this leave stale pre-assigned addresses in DATBAS and cause similar bus
collisions or false ACKs?


[Severity: High]
This is a pre-existing issue, but does renesas_i3c_group_devs_in_slots()
blindly dereference a NULL pointer if an I2C device is being swapped into an
empty slot?

drivers/i3c/master/renesas-i3c.c:renesas_i3c_group_devs_in_slots() {
    ...
		i3c_addr = i3c->addrs[i2c_pos];
		i3c_dev = i3c_addr.i3c_dev;
    ...
		new_i2c_data = i3c_dev_get_master_data(i3c_dev->dev->desc);
    ...
}

If i2c_pos points to a free slot (e.g., if a device was detached earlier),
i3c_addr.i3c_dev is NULL, and dereferencing i3c_dev->dev will cause an
immediate kernel panic.

Additionally, since i3c_dev is already a struct i3c_dev_desc *, is it necessary
to do the redundant indirection i3c_dev->dev->desc? A similar redundant
indirection happens during resume in renesas_i3c_daa():

drivers/i3c/master/renesas-i3c.c:renesas_i3c_daa() {
    ...
		if (!i3c->free_pos && i3c->resuming) {
			struct i3c_dev_desc *dev = i3c->addrs[pos].i3c_dev->dev->desc;
    ...
}

Will this oops if the Linux device wrapper (dev) failed to allocate or was
unassigned?


[Severity: High]
This is also a pre-existing issue, but can GENMASK invoke undefined behavior
in renesas_i3c_daa() if no devices attach during ENTDAA?

drivers/i3c/master/renesas-i3c.c:renesas_i3c_daa() {
    ...
	/* Skip attaching if there are failures on the xfer. */
	if (xfer->ret) {
		newdevs = 0;
	} else {
		newdevs = GENMASK(i3c->maxdevs - cmd->rx_count - 1, first_i3c_pos);
    ...
}

If the hardware executes an ENTDAA command but no new devices are found or
assigned, the response queue returns the remaining unassigned count in data_len.
This sets cmd->rx_count to the requested device count
(i3c->maxdevs - first_i3c_pos).

When this happens, the GENMASK macro is evaluated with high as first_i3c_pos - 1
and low as first_i3c_pos. Since high < low, doesn't this trigger an
out-of-bounds bitwise shift and yield a garbage mask, corrupting the newdevs
mask?

>  	i3c_dev_set_master_data(dev, NULL);
>  	i3c->addrs[data->index].addr = 0;
>  	i3c->addrs[data->index].i3c_dev = NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=8

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

[PATCH v3 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 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 195c40956148..4a055c9f4ffe 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1565,24 +1565,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);
 
@@ -1592,19 +1590,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)
@@ -1639,10 +1637,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 v3 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 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 195c40956148..4a055c9f4ffe 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1565,24 +1565,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);
 
@@ -1592,19 +1590,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)
@@ -1639,10 +1637,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

[PATCH v3 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 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 4a055c9f4ffe..1fd39dd57847 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1066,13 +1066,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 v3 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 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 4a055c9f4ffe..1fd39dd57847 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1066,13 +1066,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 v3 10/17] i3c: renesas: Return immediately if there is no transfer

[sashiko-bot]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the return value of `wait_for_completion_timeout()` is ignored in `renesas_i3c_i2c_xfers()`, leading to falsely reporting success on timeouts and continuing to issue subsequent messages to a hung bus.
- [High] This problem wasn't introduced by this patch, but transfer completion status (`xfer->ret`) is completely ignored in `renesas_i3c_i3c_xfers()`, causing all I3C private transfers to unconditionally report success even if they fail or time out.
--

commit c9a5059d97052e39bcba94fdb82d462a309a98d2
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

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

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 4a055c9f4ffea..1fd39dd578474 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -1066,13 +1066,13 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
>  	u8 start_bit = CNDCTL_STCND;
>  	int i;
>  
> +	if (!i2c_nxfers)
> +		return 0;
> +

[Severity: High]
This isn't a bug introduced by this patch, but the return value of
wait_for_completion_timeout() is ignored later in renesas_i3c_i2c_xfers():

		wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);

		if (cmd->err)
			break;

When wait_for_completion_timeout() returns 0 (indicating a timeout), there
is no check to set cmd->err. Since cmd->err remains 0, the loop incorrectly
continues to issue start conditions for remaining messages on the timed-out
bus.

Finally, the function returns cmd->err (0), falsely reporting success to
the I2C subsystem. Does this silently hide I2C timeouts from caller drivers?

[Severity: High]
This isn't a bug introduced by this patch, but transfer completion status
is completely ignored in renesas_i3c_i3c_xfers() in the same file:

		renesas_i3c_wait_xfer(i3c, xfer);
	}

	return 0;

The function renesas_i3c_wait_xfer() records completion status (e.g., -EIO,
-ENOSPC, -ETIMEDOUT) in xfer->ret. However, the loop never inspects
xfer->ret and unconditionally returns 0 at the end.

Could this silently hide NACKs, parity errors, and timeouts from I3C target
drivers, breaking the subsystem's error handling and leading to false
assumptions about data validity?

>  	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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=10

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

[PATCH v3 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 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 1fd39dd57847..4504c6e6db31 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -726,6 +726,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);
 
@@ -756,10 +760,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;
-
 	if (i3c->resuming) {
 		/* Run ENTDAA for all the I3C devices on the bus, if resuming. */
 		ret = first_i3c_pos;
@@ -883,13 +883,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:
@@ -939,15 +939,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;
 
@@ -1073,12 +1073,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 v3 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 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 1fd39dd57847..4504c6e6db31 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -726,6 +726,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);
 
@@ -756,10 +760,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;
-
 	if (i3c->resuming) {
 		/* Run ENTDAA for all the I3C devices on the bus, if resuming. */
 		ret = first_i3c_pos;
@@ -883,13 +883,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:
@@ -939,15 +939,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;
 
@@ -1073,12 +1073,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 v3 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:
- [Critical] Missing bounds checks and integer underflow in RX interrupt handlers lead to massive out-of-bounds heap writes.
- [High] I3C and I2C transfer loops ignore hardware errors and timeouts, silently returning success and risking state corruption.
--

commit 1be8528941940333e90705770f0a977f95d9b3d5
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
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -939,15 +939,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 while reviewing the transfer
initialization here, I noticed an issue in the loop below.

Does the loop ignore hardware errors and timeouts? It looks like the hardware
error status in xfer->ret is not checked, and the function unconditionally
returns 0:

    time_left = renesas_i3c_wait_xfer(i3c, xfer);
    if (!time_left)
        abort_xfer = true;
}

if (abort_xfer)
    renesas_i3c_abort_xfer(i3c);

return 0;

Also, on timeout, it sets abort_xfer = true but doesn't break out of the
loop, which means it reuses the active xfer struct for the next transfer
while the hardware hasn't been aborted yet. Can this lead to state
corruption or information leaks?

[ ... ]
> @@ -1073,12 +1073,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 also a pre-existing issue, but I see a similar pattern in
renesas_i3c_i2c_xfers() where it might fail to break out of the loop
when a timeout occurs:

    time_left = wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
    if (!time_left)
        abort_xfer = true;

    if (cmd->err)
        break;

Since cmd->err remains 0 on timeout, the loop continues to the next message
and eventually returns 0 instead of propagating the timeout. Should the loop
break when abort_xfer is set to true?

[ ... ]

[Severity: Critical]
This is a pre-existing issue not introduced by this patch, but while
reviewing the driver I noticed potential out-of-bounds writes in the
RX interrupt handlers.

In renesas_i3c_rx_isr(), is there a missing bounds check against cmd->len?
The data seems to be read directly into cmd->rx_buf based on the FIFO fill
level without validating it against the allocated length:

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

It looks like this also overwrites the start of cmd->rx_buf and overwrites
cmd->rx_count instead of appending or incrementing.

Additionally, in renesas_i3c_resp_isr(), does the subtraction risk integer
underflow?

    if (NDBSTLV0_RDBLV(renesas_readl(i3c->regs, NDBSTLV0)) && !cmd->err)
        bytes_remaining = data_len - cmd->rx_count;

    i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, bytes_remaining);

If data_len from the response descriptor is smaller than cmd->rx_count, this
subtraction could underflow. When passed to i3c_readl_fifo(), the value
could become a negative integer, which is implicitly cast to a massive
positive size in readsl(). Could this cause a remote heap overflow?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=11

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

[PATCH v3 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 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 4504c6e6db31..dab643cd8859 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -634,7 +634,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 v3 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 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 4504c6e6db31..dab643cd8859 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -634,7 +634,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 v3 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 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 dab643cd8859..38ce722881da 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -560,10 +560,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);
 
@@ -614,27 +610,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 v3 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 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 dab643cd8859..38ce722881da 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -560,10 +560,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);
 
@@ -614,27 +610,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 v3 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] Division by zero when calculating i2c_total_ticks in I3C_BUS_MODE_PURE if the I2C SCL rate is not provided.
- [High] Silent truncation of EXTBR register fields causes invalid timing configurations when tick counts exceed 255.
--

commit 379eeb8380aac5b75810596989edff2cb2084d3b
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Update HW registers after SW computations are done

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.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index dab643cd88598..38ce722881da4 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
> @@ -560,10 +560,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 could this cause a division by zero panic
in renesas_i3c_bus_init()?

If the I3C bus operates in I3C_BUS_MODE_PURE and the i2c-scl-hz property
is missing from the device tree, bus->scl_rate.i2c remains 0 because the core
function i3c_bus_set_mode() does not apply a fallback value in pure mode.
Will this lead to a division by zero during the probe phase?

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

[ ... ]

> @@ -614,27 +610,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) |

[Severity: High]
This isn't a bug introduced by this patch, but could this silently truncate
the EXTBR register fields?

The driver supports od_low_ticks and od_high_ticks up to 510 by checking
(od_low_ticks / 2) > 0xFF. For the STDBR registers, double_SBR is set and
used to right-shift the values. However, EXTBR_EBRLO and EXTBR_EBRHO directly
apply an 8-bit mask (e.g. FIELD_PREP(GENMASK(7, 0), x)) without shifting.

If the adjusted ticks fall between 256 and 510, does this misconfigure the
hardware with drastically shortened or zero-length Extended Bit Rate periods?

>  		     EXTBR_EBRLP(pp_low_ticks) | EXTBR_EBRHP(pp_high_ticks);
> -	renesas_writel(i3c->regs, EXTBR, i3c->extbr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=13

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

[PATCH v3 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 v3:
- none

Changes in v2:
- collected tags

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

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 38ce722881da..103c938d1455 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 {
@@ -262,22 +262,22 @@ struct renesas_i3c_addr {
 };
 
 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;
+	struct renesas_i3c_addr 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;
-	struct renesas_i3c_addr 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;
-	bool resuming;
+	u16 maxdevs;
 	u8 num_clks;
 	u8 refclk_div;
 };
-- 
2.43.0

[PATCH v3 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 v3:
- none

Changes in v2:
- collected tags

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

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index 38ce722881da..103c938d1455 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 {
@@ -262,22 +262,22 @@ struct renesas_i3c_addr {
 };
 
 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;
+	struct renesas_i3c_addr 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;
-	struct renesas_i3c_addr 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;
-	bool resuming;
+	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

[PATCH v3 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 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 103c938d1455..6572934a1837 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1536,12 +1536,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 v3 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 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 103c938d1455..6572934a1837 100644
--- a/drivers/i3c/master/renesas-i3c.c
+++ b/drivers/i3c/master/renesas-i3c.c
@@ -1536,12 +1536,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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Hardware transfers are not aborted on timeout, leading to late-interrupt races that prematurely complete subsequent transfers, and the driver unconditionally returns success (0) on timeout.
- [High] In `renesas_i3c_i2c_xfers()`, the return value of `wait_for_completion_timeout()` is ignored, causing the driver to blindly proceed to the next message when a timeout occurs.
--

commit 6bee3adf98ffbd95bda0355a68d579fc811fc62e
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Use the "dev_name:irq_name" format for the interrupt name

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.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index 103c938d14553..6572934a1837e 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c

[ ... ]

> static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> {
> 	unsigned long time_left;
> 
> 	renesas_i3c_enqueue_xfer(i3c, 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 is a pre-existing issue, but hardware transfers do not appear to be
aborted on timeout here.

The timeout dequeues the software object but does not abort the hardware,
setting up a late-interrupt race condition.

When looking at renesas_i3c_i3c_xfers():

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

	return 0;
}

Can this cause the loop to continue without breaking after a timeout?

It looks like the function proceeds to enqueue the next message in the
transfer sequence using the same xfer object and sends it to the hardware,
even though the hardware hasn't been properly aborted or reset for the
previous command.

If the hardware eventually completes the original timed-out message, could
it fire an interrupt where the ISR (renesas_i3c_resp_isr) sees the xfer
object and calls complete(&xfer->comp), causing the
wait_for_completion_timeout() for the second message to return prematurely?

Could this false success also cause the caller to use uninitialized read
buffers or assume writes succeeded? Does this same omission of timeout error
propagation also apply to renesas_i3c_send_ccc_cmd() and renesas_i3c_daa()?

[ ... ]

> static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
> 				 struct i2c_msg *i2c_xfers,
> 				 int i2c_nxfers)
> {
[ ... ]
> 		wait_for_completion_timeout(&xfer->comp, m->i2c.timeout);
> 
> 		if (cmd->err)
> 			break;
> 
> 		start_bit = CNDCTL_SRCND;
> 	}
> 
> 	return cmd->err;
> }

[Severity: High]
This is a pre-existing issue, but does this loop fail to break on a timeout?

The return value of wait_for_completion_timeout() is ignored here. If a
timeout occurs, cmd->err is never updated and remains 0. The loop condition
if (cmd->err) evaluates to false, so the driver continues to the next I2C
message.

Does this cause the driver to issue a new start condition to the hardware
while it is still hung processing the timed-out message, reusing the
un-reinitialized completion object?

Since it eventually returns cmd->err (0), could this falsely report success
to the I2C subsystem for a transfer sequence that failed or timed out?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=15

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

[PATCH v3 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 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 6572934a1837..b6c282c53a54 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 v3 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 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 6572934a1837..b6c282c53a54 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 v3 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 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 | 150 +++++++++++++++++++++++++------
 1 file changed, 122 insertions(+), 28 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index b6c282c53a54..d6121ca86990 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,
@@ -263,9 +262,10 @@ struct renesas_i3c_addr {
 
 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;
 	struct renesas_i3c_addr addrs[RENESAS_I3C_MAX_DEVS];
@@ -278,7 +278,6 @@ struct renesas_i3c {
 	u32 i3c_STDBR;
 	u32 extbr;
 	u16 maxdevs;
-	u8 num_clks;
 	u8 refclk_div;
 };
 
@@ -443,7 +442,24 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
 	}
 }
 
-static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
+static void renesas_i3c_abort_xfer(struct renesas_i3c *i3c)
+{
+	guard(spinlock_irqsave)(&i3c->xferqueue.lock);
+
+	/* Disable all the interrupts */
+	renesas_writel(i3c->regs, BIE, 0);
+	renesas_writel(i3c->regs, NTIE, 0);
+
+	/* Clear normal transfer status flags. */
+	renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0 | NTST_RSPQFF |
+					   NTST_TEF | NTST_TABTF);
+	/* Clear bus status flags. */
+	renesas_clear_bit(i3c->regs, BST, BST_NACKDF | BST_TENDF | BST_SPCNDDF);
+	/* Clear error flags. */
+	renesas_clear_bit(i3c->regs, BCTL, BCTL_ABT);
+}
+
+static unsigned long renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
 {
 	unsigned long time_left;
 
@@ -452,6 +468,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)
@@ -485,6 +503,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);
@@ -556,7 +580,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;
 
@@ -627,6 +651,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));
@@ -716,6 +745,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;
 	u8 first_i3c_pos = 0;
@@ -729,6 +759,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);
 
@@ -777,7 +812,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_abort_xfer(i3c);
 
 	/* Skip attaching if there are failures on the xfer. */
 	if (xfer->ret) {
@@ -870,6 +907,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) {
@@ -887,6 +925,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. */
@@ -921,7 +964,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_abort_xfer(i3c);
 
 	ret = xfer->ret;
 	if (ret)
@@ -936,7 +981,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 abort_xfer = false;
+	int i, ret;
 
 	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
 	if (!xfer)
@@ -944,6 +991,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);
 
@@ -975,9 +1027,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)
+			abort_xfer = true;
 	}
 
+	if (abort_xfer)
+		renesas_i3c_abort_xfer(i3c);
+
 	return 0;
 }
 
@@ -986,12 +1043,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;
@@ -1015,7 +1077,12 @@ static int renesas_i3c_reattach_i3c_dev(struct i3c_dev_desc *dev,
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
 	struct i3c_dev_desc *tmp_dev = i3c->addrs[data->index].i3c_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);
 
@@ -1045,8 +1112,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].addr = 0;
@@ -1063,7 +1134,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 abort_xfer = false;
+	int i, ret;
 
 	if (!i2c_nxfers)
 		return 0;
@@ -1076,6 +1149,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)) {
@@ -1102,7 +1180,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)
+			abort_xfer = true;
 
 		if (cmd->err)
 			break;
@@ -1111,6 +1191,10 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 	}
 
 	renesas_i3c_dequeue_xfer(i3c, xfer);
+
+	if (abort_xfer)
+		renesas_i3c_abort_xfer(i3c);
+
 	return cmd->err;
 }
 
@@ -1511,12 +1595,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))
@@ -1583,8 +1671,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:
@@ -1606,13 +1692,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);
@@ -1635,15 +1721,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 v3 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 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 | 150 +++++++++++++++++++++++++------
 1 file changed, 122 insertions(+), 28 deletions(-)

diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
index b6c282c53a54..d6121ca86990 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,
@@ -263,9 +262,10 @@ struct renesas_i3c_addr {
 
 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;
 	struct renesas_i3c_addr addrs[RENESAS_I3C_MAX_DEVS];
@@ -278,7 +278,6 @@ struct renesas_i3c {
 	u32 i3c_STDBR;
 	u32 extbr;
 	u16 maxdevs;
-	u8 num_clks;
 	u8 refclk_div;
 };
 
@@ -443,7 +442,24 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
 	}
 }
 
-static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
+static void renesas_i3c_abort_xfer(struct renesas_i3c *i3c)
+{
+	guard(spinlock_irqsave)(&i3c->xferqueue.lock);
+
+	/* Disable all the interrupts */
+	renesas_writel(i3c->regs, BIE, 0);
+	renesas_writel(i3c->regs, NTIE, 0);
+
+	/* Clear normal transfer status flags. */
+	renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0 | NTST_RSPQFF |
+					   NTST_TEF | NTST_TABTF);
+	/* Clear bus status flags. */
+	renesas_clear_bit(i3c->regs, BST, BST_NACKDF | BST_TENDF | BST_SPCNDDF);
+	/* Clear error flags. */
+	renesas_clear_bit(i3c->regs, BCTL, BCTL_ABT);
+}
+
+static unsigned long renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
 {
 	unsigned long time_left;
 
@@ -452,6 +468,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)
@@ -485,6 +503,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);
@@ -556,7 +580,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;
 
@@ -627,6 +651,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));
@@ -716,6 +745,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;
 	u8 first_i3c_pos = 0;
@@ -729,6 +759,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);
 
@@ -777,7 +812,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_abort_xfer(i3c);
 
 	/* Skip attaching if there are failures on the xfer. */
 	if (xfer->ret) {
@@ -870,6 +907,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) {
@@ -887,6 +925,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. */
@@ -921,7 +964,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_abort_xfer(i3c);
 
 	ret = xfer->ret;
 	if (ret)
@@ -936,7 +981,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 abort_xfer = false;
+	int i, ret;
 
 	struct renesas_i3c_xfer *xfer __free(kfree) = renesas_i3c_alloc_xfer(i3c, 1);
 	if (!xfer)
@@ -944,6 +991,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);
 
@@ -975,9 +1027,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)
+			abort_xfer = true;
 	}
 
+	if (abort_xfer)
+		renesas_i3c_abort_xfer(i3c);
+
 	return 0;
 }
 
@@ -986,12 +1043,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;
@@ -1015,7 +1077,12 @@ static int renesas_i3c_reattach_i3c_dev(struct i3c_dev_desc *dev,
 	struct renesas_i3c *i3c = to_renesas_i3c(m);
 	struct renesas_i3c_i2c_dev_data *data = i3c_dev_get_master_data(dev);
 	struct i3c_dev_desc *tmp_dev = i3c->addrs[data->index].i3c_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);
 
@@ -1045,8 +1112,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].addr = 0;
@@ -1063,7 +1134,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 abort_xfer = false;
+	int i, ret;
 
 	if (!i2c_nxfers)
 		return 0;
@@ -1076,6 +1149,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)) {
@@ -1102,7 +1180,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)
+			abort_xfer = true;
 
 		if (cmd->err)
 			break;
@@ -1111,6 +1191,10 @@ static int renesas_i3c_i2c_xfers(struct i2c_dev_desc *dev,
 	}
 
 	renesas_i3c_dequeue_xfer(i3c, xfer);
+
+	if (abort_xfer)
+		renesas_i3c_abort_xfer(i3c);
+
 	return cmd->err;
 }
 
@@ -1511,12 +1595,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))
@@ -1583,8 +1671,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:
@@ -1606,13 +1692,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);
@@ -1635,15 +1721,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 v3 17/17] i3c: renesas: Add runtime PM support

[sashiko-bot]

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

New issues:
- [High] Async Runtime PM put during driver unbind leaves the clock enabled.
- [High] Newly added renesas_i3c_abort_xfer clears the abort bit instead of setting it, leaving hardware stuck.

Pre-existing issues:
- [High] Timeout handling in transfer loops fails to break the loop, ignores the timeout, and incorrectly returns success.
- [High] I3C RX buffer is not advanced during FIFO reads, leaving buffer tails uninitialized.
--

commit 810cc3873a9dc52a932c03376330719f06d17de0
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

i3c: renesas: Add runtime PM support

This commit adds runtime PM support to the Renesas I3C driver, replacing
explicit clock management with runtime PM APIs. It updates controller
operations with PM guards and introduces a transfer abort mechanism to handle
timeouts asynchronously.

> diff --git a/drivers/i3c/master/renesas-i3c.c b/drivers/i3c/master/renesas-i3c.c
> index b6c282c53a541..d6121ca869908 100644
> --- a/drivers/i3c/master/renesas-i3c.c
> +++ b/drivers/i3c/master/renesas-i3c.c
[ ... ]
> @@ -443,7 +442,24 @@ static void renesas_i3c_enqueue_xfer(struct renesas_i3c *i3c, struct renesas_i3c
>  	}
>  }
>  
> -static void renesas_i3c_wait_xfer(struct renesas_i3c *i3c, struct renesas_i3c_xfer *xfer)
> +static void renesas_i3c_abort_xfer(struct renesas_i3c *i3c)
> +{
> +	guard(spinlock_irqsave)(&i3c->xferqueue.lock);
> +
> +	/* Disable all the interrupts */
> +	renesas_writel(i3c->regs, BIE, 0);
> +	renesas_writel(i3c->regs, NTIE, 0);
> +
> +	/* Clear normal transfer status flags. */
> +	renesas_clear_bit(i3c->regs, NTST, NTST_TDBEF0 | NTST_RDBFF0 | NTST_RSPQFF |
> +					   NTST_TEF | NTST_TABTF);
> +	/* Clear bus status flags. */
> +	renesas_clear_bit(i3c->regs, BST, BST_NACKDF | BST_TENDF | BST_SPCNDDF);
> +	/* Clear error flags. */
> +	renesas_clear_bit(i3c->regs, BCTL, BCTL_ABT);

[Severity: High]
Does this clear the abort bit instead of setting it?

Hardware abort bits like BCTL_ABT typically must be written as 1 to trigger
an abort. If we only clear the bit, will the hardware controller be left
running the timed-out transfer in the background and get stuck?

> +}
> +
[ ... ]
> @@ -975,9 +1027,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)
> +			abort_xfer = true;

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of a break here
cause the loop to continue iterating?

If the transfer times out, it sets abort_xfer = true but continues to the
next command, overwriting the shared cmd descriptor.

Also, since renesas_i3c_i3c_xfers() unconditionally returns 0 at the end,
will this hide the timeout from the subsystem and potentially cause it to
read uninitialized RX buffers?

>  	}
>  
> +	if (abort_xfer)
> +		renesas_i3c_abort_xfer(i3c);
> +
>  	return 0;
>  }
[ ... ]
> @@ -1045,8 +1112,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].addr = 0;

[Severity: High]
Does this asynchronously put the runtime PM usage counter without a
synchronous suspend before driver teardown completes?

During driver unbind, renesas_i3c_remove() unregisters the I3C master, which
detaches devices. The PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND macro releases
the device asynchronously via pm_runtime_put_autosuspend().

Right after remove returns, devres calls pm_runtime_disable(), which cancels
the pending autosuspend work. Could this leave the hardware permanently in
the RPM_ACTIVE state and leak resources by never disabling the clocks?

[ ... ]
> @@ -1102,7 +1180,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)
> +			abort_xfer = true;
>  
>  		if (cmd->err)
>  			break;

[Severity: High]
This is a pre-existing issue, but similar to I3C transfers above, does this
fail to break the loop when wait_for_completion_timeout() times out?

Since cmd->err remains 0 on timeout, the loop continues and eventually returns
0, hiding the timeout from the subsystem.


Additionally, I noticed a pre-existing issue outside the immediate scope of
this patch:

[Severity: High]
This is a pre-existing issue, but in renesas_i3c_rx_isr() and
renesas_i3c_resp_isr(), do the I3C FIFO reads copy data into cmd->rx_buf
without advancing the buffer pointer?

renesas_i3c_rx_isr() does:

    i3c_readl_fifo(i3c->regs + NTDTBP0, cmd->rx_buf, read_bytes);
    cmd->rx_count = read_bytes;

Since cmd->rx_buf is not incremented and rx_count is overwritten rather than
accumulated, will multiple interrupts overwrite the beginning of the buffer?
Could this leave the tail of the buffer uninitialized, leading to information
leaks or data corruption when the subsystem reads it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608201543.804902-1-claudiu.beznea@kernel.org?part=17

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