[PATCH] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[PATCH] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[tze.yee.ng]

From: Tze Yee Ng <tze.yee.ng@altera.com>

Implement ->set_speed() so the I3C core can switch open-drain timing for
the first broadcast address per spec: I3C_OPEN_DRAIN_SLOW_SPEED programs
tHIGH_INIT (200 ns) before RSTDAA, and I3C_OPEN_DRAIN_NORMAL_SPEED restores
normal OD timing afterward. Cache the normal OD register value during bus
init and use a separate od_hcnt for the slow path so SDR extended timing
remains derived from the normal PP hcnt.

Fixes I2C devices with spike filters not being detected on mixed buses.

Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
---
 drivers/i3c/master/dw-i3c-master.c | 53 ++++++++++++++++++++++++++++++
 drivers/i3c/master/dw-i3c-master.h |  1 +
 include/linux/i3c/master.h         |  1 +
 3 files changed, 55 insertions(+)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 655693a2187e..dbb801910b3d 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -592,6 +592,7 @@ static int dw_i3c_clk_cfg(struct dw_i3c_master *master)
 	scl_timing = SCL_I3C_TIMING_HCNT(hcnt) | SCL_I3C_TIMING_LCNT(lcnt);
 	writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
 	master->i3c_od_timing = scl_timing;
+	master->i3c_od_timing_normal = scl_timing;
 
 	lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR1_SCL_RATE) - hcnt;
 	scl_timing = SCL_EXT_LCNT_1(lcnt);
@@ -1480,6 +1481,57 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int dw_i3c_master_set_speed(struct i3c_master_controller *m,
+				   enum i3c_open_drain_speed speed)
+{
+	struct dw_i3c_master *master = to_dw_i3c_master(m);
+	unsigned long core_rate, core_period;
+	u32 scl_timing;
+	u8 od_hcnt, lcnt;
+	int ret;
+
+	ret = pm_runtime_resume_and_get(master->dev);
+	if (ret < 0) {
+		dev_err(master->dev,
+			"<%s> cannot resume i3c bus master, err: %d\n",
+			__func__, ret);
+		return ret;
+	}
+
+	switch (speed) {
+	case I3C_OPEN_DRAIN_SLOW_SPEED:
+		core_rate = clk_get_rate(master->core_clk);
+		if (!core_rate) {
+			ret = -EINVAL;
+			break;
+		}
+
+		core_period = DIV_ROUND_UP(1000000000, core_rate);
+		lcnt = SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
+		od_hcnt = max_t(u8, SCL_I3C_TIMING_CNT_MIN,
+				DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS,
+					     core_period) - 1);
+		scl_timing = SCL_I3C_TIMING_HCNT(od_hcnt) |
+			     SCL_I3C_TIMING_LCNT(lcnt);
+		writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
+		master->i3c_od_timing = scl_timing;
+		break;
+
+	case I3C_OPEN_DRAIN_NORMAL_SPEED:
+		writel(master->i3c_od_timing_normal,
+		       master->regs + SCL_I3C_OD_TIMING);
+		master->i3c_od_timing = master->i3c_od_timing_normal;
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	pm_runtime_put_autosuspend(master->dev);
+
+	return ret;
+}
+
 static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
 					    unsigned long dev_nack_retry_cnt)
 {
@@ -1534,6 +1586,7 @@ static const struct i3c_master_controller_ops dw_mipi_i3c_ops = {
 	.recycle_ibi_slot = dw_i3c_master_recycle_ibi_slot,
 	.enable_hotjoin = dw_i3c_master_enable_hotjoin,
 	.disable_hotjoin = dw_i3c_master_disable_hotjoin,
+	.set_speed = dw_i3c_master_set_speed,
 	.set_dev_nack_retry = dw_i3c_master_set_dev_nack_retry,
 };
 
diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
index c5cb695c16ab..c814087a6f1f 100644
--- a/drivers/i3c/master/dw-i3c-master.h
+++ b/drivers/i3c/master/dw-i3c-master.h
@@ -46,6 +46,7 @@ struct dw_i3c_master {
 	u32 dev_addr;
 	u32 i3c_pp_timing;
 	u32 i3c_od_timing;
+	u32 i3c_od_timing_normal;
 	u32 ext_lcnt_timing;
 	u32 bus_free_timing;
 	u32 i2c_fm_timing;
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 592b646f6134..c7d7448c4cd7 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -259,6 +259,7 @@ struct i3c_device {
 #define I3C_BUS_THIGH_MIXED_MAX_NS	41
 #define I3C_BUS_TIDLE_MIN_NS		200000
 #define I3C_BUS_TLOW_OD_MIN_NS		200
+#define I3C_BUS_THIGH_INIT_OD_MIN_NS	200
 
 /**
  * enum i3c_bus_mode - I3C bus mode
-- 
2.43.7


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

Re: [PATCH] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[sashiko-bot]

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

New issues:
- [High] The newly introduced `dw_i3c_master_set_speed()` permanently overwrites hardware-specific AMD timings with generic timings.
- [High] The `od_hcnt` calculation for `I3C_OPEN_DRAIN_SLOW_SPEED` violates the `tHIGH_INIT` minimum requirement.

Pre-existing issues:
- [High] The IRQ handler accesses hardware registers without Runtime PM protection.
- [High] Use-After-Free and invalid `queue_work()` due to improper teardown ordering in `dw_i3c_common_remove()`.
- [High] `dw_i3c_master_set_dev_nack_retry` modifies hardware registers without resuming the device via Runtime PM.
--

commit bd5322e3d6d6cd3a6a8155f24cc89a12fe61be00
Author: Tze Yee Ng <tze.yee.ng@altera.com>

i3c: master: dw-i3c-master: fix OD timing for first broadcast

This commit implements the set_speed() callback to switch open-drain timing
for the first broadcast address to slow speed. It aims to fix I2C devices
with spike filters not being detected on mixed buses by dynamically
switching between slow and normal open drain speeds.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index a7593d6efac5a..6cf7071d3d06b 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
[ ... ]
> @@ -1484,6 +1485,57 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This is a pre-existing issue, but does the IRQ handler access hardware
registers without Runtime PM protection?

The interrupt handler directly reads hardware registers before checking the
device's PM active state:

drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_irq_handler() {
    ...
    status = readl(master->regs + INTR_STATUS);
    ...
}

Since the driver does not synchronize IRQs in its runtime_suspend callback,
if an interrupt arrives right as the device enters runtime suspend (or if a
spurious interrupt occurs), will this raw register read access powered-down
hardware and cause a synchronous external abort?

> +static int dw_i3c_master_set_speed(struct i3c_master_controller *m,
> +				   enum i3c_open_drain_speed speed)
> +{
[ ... ]
> +	switch (speed) {
> +	case I3C_OPEN_DRAIN_SLOW_SPEED:
[ ... ]
> +		core_period = DIV_ROUND_UP(1000000000, core_rate);
> +		lcnt = SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
> +		od_hcnt = max_t(u8, SCL_I3C_TIMING_CNT_MIN,
> +				DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS,
> +					     core_period) - 1);

[Severity: High]
Does this precision loss guarantee the resulting pulse is shorter than the 
specification minimum for certain clock frequencies?

If core_period is calculated by rounding up 1000000000 / core_rate, using
this overestimated integer in the division 
DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS, core_period) - 1 underestimates
the necessary cycle count.

For example, at a 133.3 MHz core rate, the exact period is 7.5 ns but
core_period evaluates to 8 ns. The calculated cycle count evaluates to 25
cycles, which produces a 187.5 ns pulse.

Does this violate the strict 200 ns minimum for tHIGH_INIT required by the 
I3C specification?

> +		scl_timing = SCL_I3C_TIMING_HCNT(od_hcnt) |
> +			     SCL_I3C_TIMING_LCNT(lcnt);
> +		writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
> +		master->i3c_od_timing = scl_timing;
> +		break;
> +
> +	case I3C_OPEN_DRAIN_NORMAL_SPEED:
> +		writel(master->i3c_od_timing_normal,
> +		       master->regs + SCL_I3C_OD_TIMING);

[Severity: High]
Does unconditionally restoring master->i3c_od_timing_normal here permanently 
overwrite hardware-specific AMD timings with generic timings?

If amd_configure_od_pp_quirk() is not updated to modify 
master->i3c_od_timing_normal, restoring it after a broadcast destroys the 
required AMD-specific quirk timings.

Additionally, does dw_i3c_master_send_ccc_cmd() unconditionally overwrite
the OD timing with AMD normal timing right before sending a broadcast CCC, 
bypassing the I3C_OPEN_DRAIN_SLOW_SPEED state expected by the I3C core?

> +		master->i3c_od_timing = master->i3c_od_timing_normal;
> +		break;
[ ... ]
> +static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
> +					    unsigned long dev_nack_retry_cnt)
> +{
[ ... ]
> +	for (i = 0; i < master->maxdevs; i++) {
> +		/* Skip free/empty slots */
> +		if (master->free_pos & BIT(i))
> +			continue;
> +
> +		reg = readl(master->regs +
> +				DEV_ADDR_TABLE_LOC(master->datstartaddr, i));

[Severity: High]
This is a pre-existing issue, but does this function modify hardware
registers without resuming the device via Runtime PM?

The sysfs attribute dev_nack_retry_count directly invokes 
master->ops->set_dev_nack_retry(). Since sysfs callbacks can be triggered
from userspace at any time, executing this while the device is runtime
suspended will access powered-down hardware.

On ARM SoCs, can this trigger a kernel panic (external abort) and allow 
userspace to trivially crash the system?

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a use-after-free or invalid
queue_work() due to improper teardown ordering in dw_i3c_common_remove()?

dw_i3c_common_remove() calls cancel_work_sync(&master->hj_work) before the
hardware interrupt is disabled and freed by devres:

drivers/i3c/master/dw-i3c-master.c:dw_i3c_common_remove() {
    ...
    cancel_work_sync(&master->hj_work);
    i3c_master_unregister(&master->base);
    ...
}

Because the IRQ remains active throughout the function and during early
devres teardown, can an incoming Hot Join interrupt trigger the IRQ handler,
which will call queue_work() to re-queue hj_work?

Does this either cause queue_work() to execute on a destroyed workqueue or
result in a use-after-free when the workqueue subsequently processes the
work using the master object that has already been freed by devres?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/ebddb8b62eae92de0a7eeda93cb18213c677ae96.1781077653.git.tze.yee.ng@altera.com?part=1

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

Re: [PATCH] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[Frank Li]

On Wed, Jun 10, 2026 at 08:36:55AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>
> New issues:
> - [High] The newly introduced `dw_i3c_master_set_speed()` permanently overwrites hardware-specific AMD timings with generic timings.
> - [High] The `od_hcnt` calculation for `I3C_OPEN_DRAIN_SLOW_SPEED` violates the `tHIGH_INIT` minimum requirement.
>
> Pre-existing issues:
> - [High] The IRQ handler accesses hardware registers without Runtime PM protection.
> - [High] Use-After-Free and invalid `queue_work()` due to improper teardown ordering in `dw_i3c_common_remove()`.
> - [High] `dw_i3c_master_set_dev_nack_retry` modifies hardware registers without resuming the device via Runtime PM.
> --
>
> commit bd5322e3d6d6cd3a6a8155f24cc89a12fe61be00
> Author: Tze Yee Ng <tze.yee.ng@altera.com>
>
> i3c: master: dw-i3c-master: fix OD timing for first broadcast
>
> This commit implements the set_speed() callback to switch open-drain timing
> for the first broadcast address to slow speed. It aims to fix I2C devices
> with spike filters not being detected on mixed buses by dynamically
> switching between slow and normal open drain speeds.
>
> > diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> > index a7593d6efac5a..6cf7071d3d06b 100644
> > --- a/drivers/i3c/master/dw-i3c-master.c
> > +++ b/drivers/i3c/master/dw-i3c-master.c
> [ ... ]
> > @@ -1484,6 +1485,57 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
> >  	return IRQ_HANDLED;
> >  }
>
> [Severity: High]
> This is a pre-existing issue, but does the IRQ handler access hardware
> registers without Runtime PM protection?
>
> The interrupt handler directly reads hardware registers before checking the
> device's PM active state:
>
> drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_irq_handler() {
>     ...
>     status = readl(master->regs + INTR_STATUS);
>     ...
> }
>
> Since the driver does not synchronize IRQs in its runtime_suspend callback,
> if an interrupt arrives right as the device enters runtime suspend (or if a
> spurious interrupt occurs), will this raw register read access powered-down
> hardware and cause a synchronous external abort?
>
> > +static int dw_i3c_master_set_speed(struct i3c_master_controller *m,
> > +				   enum i3c_open_drain_speed speed)
> > +{
> [ ... ]
> > +	switch (speed) {
> > +	case I3C_OPEN_DRAIN_SLOW_SPEED:
> [ ... ]
> > +		core_period = DIV_ROUND_UP(1000000000, core_rate);
> > +		lcnt = SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
> > +		od_hcnt = max_t(u8, SCL_I3C_TIMING_CNT_MIN,
> > +				DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS,
> > +					     core_period) - 1);
>
> [Severity: High]
> Does this precision loss guarantee the resulting pulse is shorter than the
> specification minimum for certain clock frequencies?
>
> If core_period is calculated by rounding up 1000000000 / core_rate, using
> this overestimated integer in the division
> DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS, core_period) - 1 underestimates
> the necessary cycle count.
>
> For example, at a 133.3 MHz core rate, the exact period is 7.5 ns but
> core_period evaluates to 8 ns. The calculated cycle count evaluates to 25
> cycles, which produces a 187.5 ns pulse.
>
> Does this violate the strict 200 ns minimum for tHIGH_INIT required by the
> I3C specification?

Is it true?

>
> > +		scl_timing = SCL_I3C_TIMING_HCNT(od_hcnt) |
> > +			     SCL_I3C_TIMING_LCNT(lcnt);
> > +		writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
> > +		master->i3c_od_timing = scl_timing;
> > +		break;
> > +
> > +	case I3C_OPEN_DRAIN_NORMAL_SPEED:
> > +		writel(master->i3c_od_timing_normal,
> > +		       master->regs + SCL_I3C_OD_TIMING);
>
> [Severity: High]
> Does unconditionally restoring master->i3c_od_timing_normal here permanently
> overwrite hardware-specific AMD timings with generic timings?
>
> If amd_configure_od_pp_quirk() is not updated to modify
> master->i3c_od_timing_normal, restoring it after a broadcast destroys the
> required AMD-specific quirk timings.
>
> Additionally, does dw_i3c_master_send_ccc_cmd() unconditionally overwrite
> the OD timing with AMD normal timing right before sending a broadcast CCC,
> bypassing the I3C_OPEN_DRAIN_SLOW_SPEED state expected by the I3C core?
>
> > +		master->i3c_od_timing = master->i3c_od_timing_normal;
> > +		break;

Please check this also?

Frank

> [ ... ]
> > +static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
> > +					    unsigned long dev_nack_retry_cnt)
> > +{
> [ ... ]
> > +	for (i = 0; i < master->maxdevs; i++) {
> > +		/* Skip free/empty slots */
> > +		if (master->free_pos & BIT(i))
> > +			continue;
> > +
> > +		reg = readl(master->regs +
> > +				DEV_ADDR_TABLE_LOC(master->datstartaddr, i));
>
> [Severity: High]
> This is a pre-existing issue, but does this function modify hardware
> registers without resuming the device via Runtime PM?
>
> The sysfs attribute dev_nack_retry_count directly invokes
> master->ops->set_dev_nack_retry(). Since sysfs callbacks can be triggered
> from userspace at any time, executing this while the device is runtime
> suspended will access powered-down hardware.
>
> On ARM SoCs, can this trigger a kernel panic (external abort) and allow
> userspace to trivially crash the system?
>
> [ ... ]
>
> [Severity: High]
> This is a pre-existing issue, but is there a use-after-free or invalid
> queue_work() due to improper teardown ordering in dw_i3c_common_remove()?
>
> dw_i3c_common_remove() calls cancel_work_sync(&master->hj_work) before the
> hardware interrupt is disabled and freed by devres:
>
> drivers/i3c/master/dw-i3c-master.c:dw_i3c_common_remove() {
>     ...
>     cancel_work_sync(&master->hj_work);
>     i3c_master_unregister(&master->base);
>     ...
> }
>
> Because the IRQ remains active throughout the function and during early
> devres teardown, can an incoming Hot Join interrupt trigger the IRQ handler,
> which will call queue_work() to re-queue hj_work?
>
> Does this either cause queue_work() to execute on a destroyed workqueue or
> result in a use-after-free when the workqueue subsequently processes the
> work using the master object that has already been freed by devres?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/ebddb8b62eae92de0a7eeda93cb18213c677ae96.1781077653.git.tze.yee.ng@altera.com?part=1
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

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

Re: [PATCH] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[NG, TZE YEE]

On 12/6/2026 12:21 am, Frank Li wrote:
> [You don't often get email from frank.li@oss.nxp.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> On Wed, Jun 10, 2026 at 08:36:55AM +0000, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
>>
>> New issues:
>> - [High] The newly introduced `dw_i3c_master_set_speed()` permanently overwrites hardware-specific AMD timings with generic timings.
>> - [High] The `od_hcnt` calculation for `I3C_OPEN_DRAIN_SLOW_SPEED` violates the `tHIGH_INIT` minimum requirement.
>>
>> Pre-existing issues:
>> - [High] The IRQ handler accesses hardware registers without Runtime PM protection.
>> - [High] Use-After-Free and invalid `queue_work()` due to improper teardown ordering in `dw_i3c_common_remove()`.
>> - [High] `dw_i3c_master_set_dev_nack_retry` modifies hardware registers without resuming the device via Runtime PM.
>> --
>>
>> commit bd5322e3d6d6cd3a6a8155f24cc89a12fe61be00
>> Author: Tze Yee Ng <tze.yee.ng@altera.com>
>>
>> i3c: master: dw-i3c-master: fix OD timing for first broadcast
>>
>> This commit implements the set_speed() callback to switch open-drain timing
>> for the first broadcast address to slow speed. It aims to fix I2C devices
>> with spike filters not being detected on mixed buses by dynamically
>> switching between slow and normal open drain speeds.
>>
>>> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
>>> index a7593d6efac5a..6cf7071d3d06b 100644
>>> --- a/drivers/i3c/master/dw-i3c-master.c
>>> +++ b/drivers/i3c/master/dw-i3c-master.c
>> [ ... ]
>>> @@ -1484,6 +1485,57 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
>>>      return IRQ_HANDLED;
>>>   }
>>
>> [Severity: High]
>> This is a pre-existing issue, but does the IRQ handler access hardware
>> registers without Runtime PM protection?
>>
>> The interrupt handler directly reads hardware registers before checking the
>> device's PM active state:
>>
>> drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_irq_handler() {
>>      ...
>>      status = readl(master->regs + INTR_STATUS);
>>      ...
>> }
>>
>> Since the driver does not synchronize IRQs in its runtime_suspend callback,
>> if an interrupt arrives right as the device enters runtime suspend (or if a
>> spurious interrupt occurs), will this raw register read access powered-down
>> hardware and cause a synchronous external abort?
>>
>>> +static int dw_i3c_master_set_speed(struct i3c_master_controller *m,
>>> +                              enum i3c_open_drain_speed speed)
>>> +{
>> [ ... ]
>>> +   switch (speed) {
>>> +   case I3C_OPEN_DRAIN_SLOW_SPEED:
>> [ ... ]
>>> +           core_period = DIV_ROUND_UP(1000000000, core_rate);
>>> +           lcnt = SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
>>> +           od_hcnt = max_t(u8, SCL_I3C_TIMING_CNT_MIN,
>>> +                           DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS,
>>> +                                        core_period) - 1);
>>
>> [Severity: High]
>> Does this precision loss guarantee the resulting pulse is shorter than the
>> specification minimum for certain clock frequencies?
>>
>> If core_period is calculated by rounding up 1000000000 / core_rate, using
>> this overestimated integer in the division
>> DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS, core_period) - 1 underestimates
>> the necessary cycle count.
>>
>> For example, at a 133.3 MHz core rate, the exact period is 7.5 ns but
>> core_period evaluates to 8 ns. The calculated cycle count evaluates to 25
>> cycles, which produces a 187.5 ns pulse.
>>
>> Does this violate the strict 200 ns minimum for tHIGH_INIT required by the
>> I3C specification?
>
> Is it true?
>
>>
>>> +           scl_timing = SCL_I3C_TIMING_HCNT(od_hcnt) |
>>> +                        SCL_I3C_TIMING_LCNT(lcnt);
>>> +           writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
>>> +           master->i3c_od_timing = scl_timing;
>>> +           break;
>>> +
>>> +   case I3C_OPEN_DRAIN_NORMAL_SPEED:
>>> +           writel(master->i3c_od_timing_normal,
>>> +                  master->regs + SCL_I3C_OD_TIMING);
>>
>> [Severity: High]
>> Does unconditionally restoring master->i3c_od_timing_normal here permanently
>> overwrite hardware-specific AMD timings with generic timings?
>>
>> If amd_configure_od_pp_quirk() is not updated to modify
>> master->i3c_od_timing_normal, restoring it after a broadcast destroys the
>> required AMD-specific quirk timings.
>>
>> Additionally, does dw_i3c_master_send_ccc_cmd() unconditionally overwrite
>> the OD timing with AMD normal timing right before sending a broadcast CCC,
>> bypassing the I3C_OPEN_DRAIN_SLOW_SPEED state expected by the I3C core?
>>
>>> +           master->i3c_od_timing = master->i3c_od_timing_normal;
>>> +           break;
>
> Please check this also?
>
> Frank
>
>> [ ... ]
>>> +static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
>>> +                                       unsigned long dev_nack_retry_cnt)
>>> +{
>> [ ... ]
>>> +   for (i = 0; i < master->maxdevs; i++) {
>>> +           /* Skip free/empty slots */
>>> +           if (master->free_pos & BIT(i))
>>> +                   continue;
>>> +
>>> +           reg = readl(master->regs +
>>> +                           DEV_ADDR_TABLE_LOC(master->datstartaddr, i));
>>
>> [Severity: High]
>> This is a pre-existing issue, but does this function modify hardware
>> registers without resuming the device via Runtime PM?
>>
>> The sysfs attribute dev_nack_retry_count directly invokes
>> master->ops->set_dev_nack_retry(). Since sysfs callbacks can be triggered
>> from userspace at any time, executing this while the device is runtime
>> suspended will access powered-down hardware.
>>
>> On ARM SoCs, can this trigger a kernel panic (external abort) and allow
>> userspace to trivially crash the system?
>>
>> [ ... ]
>>
>> [Severity: High]
>> This is a pre-existing issue, but is there a use-after-free or invalid
>> queue_work() due to improper teardown ordering in dw_i3c_common_remove()?
>>
>> dw_i3c_common_remove() calls cancel_work_sync(&master->hj_work) before the
>> hardware interrupt is disabled and freed by devres:
>>
>> drivers/i3c/master/dw-i3c-master.c:dw_i3c_common_remove() {
>>      ...
>>      cancel_work_sync(&master->hj_work);
>>      i3c_master_unregister(&master->base);
>>      ...
>> }
>>
>> Because the IRQ remains active throughout the function and during early
>> devres teardown, can an incoming Hot Join interrupt trigger the IRQ handler,
>> which will call queue_work() to re-queue hj_work?
>>
>> Does this either cause queue_work() to execute on a destroyed workqueue or
>> result in a use-after-free when the workqueue subsequently processes the
>> work using the master object that has already been freed by devres?
>>
>> --
>> Sashiko AI review · https://sashiko.dev/#/patchset/ebddb8b62eae92de0a7eeda93cb18213c677ae96.1781077653.git.tze.yee.ng@altera.com?part=1
>>
>> --
>> linux-i3c mailing list
>> linux-i3c@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-i3c

Hi Frank,

Sashiko is correct that the initial core_period-based calculation could
undershoot the 200 ns tHIGH_INIT minimum at some core clock rates. I
checked the Zephyr DesignWare I3C driver, which uses a core_rate-based
calculation aligned with the DWC timing model. v2 will adopt the same
approach:

od_hcnt = DIV_ROUND_UP((u64)I3C_BUS_THIGH_INIT_OD_MIN_NS * core_rate,
                                       1000000000ULL) - 1;

still using the existing - 1 and SCL_I3C_TIMING_CNT_MIN convention from
dw_i3c_clk_cfg().

Reference:
https://github.com/zephyrproject-rtos/zephyr/commit/4d6673bc693920bdacf96d7cd3cfeb71d421e0ae
  (drivers/i3c/i3c_dw.c, dw_i3c_init_scl_timing())

For AMD specific timings,  I left existing AMD behavior unchanged in v1.
@Manikanta Guntupalli, for AMDI0015, should ->set_speed() use
AMD_I3C_OD_TIMING or generic timing as the normal OD baseline at
slow/normal speed? I can add AMD-specific handling in v2 once that is
clarified.

Thanks,
Tze Yee
-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[NG, TZE YEE]

On 12/6/2026 3:58 pm, Ng, Tze Yee wrote:
> On 12/6/2026 12:21 am, Frank Li wrote:
>> [You don't often get email from frank.li@oss.nxp.com. Learn why this 
>> is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> On Wed, Jun 10, 2026 at 08:36:55AM +0000, sashiko-bot@kernel.org wrote:
>>> Thank you for your contribution! Sashiko AI review found 5 potential 
>>> issue(s) to consider:
>>>
>>> New issues:
>>> - [High] The newly introduced `dw_i3c_master_set_speed()` permanently 
>>> overwrites hardware-specific AMD timings with generic timings.
>>> - [High] The `od_hcnt` calculation for `I3C_OPEN_DRAIN_SLOW_SPEED` 
>>> violates the `tHIGH_INIT` minimum requirement.
>>>
>>> Pre-existing issues:
>>> - [High] The IRQ handler accesses hardware registers without Runtime 
>>> PM protection.
>>> - [High] Use-After-Free and invalid `queue_work()` due to improper 
>>> teardown ordering in `dw_i3c_common_remove()`.
>>> - [High] `dw_i3c_master_set_dev_nack_retry` modifies hardware 
>>> registers without resuming the device via Runtime PM.
>>> -- 
>>>
>>> commit bd5322e3d6d6cd3a6a8155f24cc89a12fe61be00
>>> Author: Tze Yee Ng <tze.yee.ng@altera.com>
>>>
>>> i3c: master: dw-i3c-master: fix OD timing for first broadcast
>>>
>>> This commit implements the set_speed() callback to switch open-drain 
>>> timing
>>> for the first broadcast address to slow speed. It aims to fix I2C 
>>> devices
>>> with spike filters not being detected on mixed buses by dynamically
>>> switching between slow and normal open drain speeds.
>>>
>>>> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/ 
>>>> master/dw-i3c-master.c
>>>> index a7593d6efac5a..6cf7071d3d06b 100644
>>>> --- a/drivers/i3c/master/dw-i3c-master.c
>>>> +++ b/drivers/i3c/master/dw-i3c-master.c
>>> [ ... ]
>>>> @@ -1484,6 +1485,57 @@ static irqreturn_t 
>>>> dw_i3c_master_irq_handler(int irq, void *dev_id)
>>>>      return IRQ_HANDLED;
>>>>   }
>>>
>>> [Severity: High]
>>> This is a pre-existing issue, but does the IRQ handler access hardware
>>> registers without Runtime PM protection?
>>>
>>> The interrupt handler directly reads hardware registers before 
>>> checking the
>>> device's PM active state:
>>>
>>> drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_irq_handler() {
>>>      ...
>>>      status = readl(master->regs + INTR_STATUS);
>>>      ...
>>> }
>>>
>>> Since the driver does not synchronize IRQs in its runtime_suspend 
>>> callback,
>>> if an interrupt arrives right as the device enters runtime suspend 
>>> (or if a
>>> spurious interrupt occurs), will this raw register read access 
>>> powered-down
>>> hardware and cause a synchronous external abort?
>>>
>>>> +static int dw_i3c_master_set_speed(struct i3c_master_controller *m,
>>>> +                              enum i3c_open_drain_speed speed)
>>>> +{
>>> [ ... ]
>>>> +   switch (speed) {
>>>> +   case I3C_OPEN_DRAIN_SLOW_SPEED:
>>> [ ... ]
>>>> +           core_period = DIV_ROUND_UP(1000000000, core_rate);
>>>> +           lcnt = SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
>>>> +           od_hcnt = max_t(u8, SCL_I3C_TIMING_CNT_MIN,
>>>> +                           DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS,
>>>> +                                        core_period) - 1);
>>>
>>> [Severity: High]
>>> Does this precision loss guarantee the resulting pulse is shorter 
>>> than the
>>> specification minimum for certain clock frequencies?
>>>
>>> If core_period is calculated by rounding up 1000000000 / core_rate, 
>>> using
>>> this overestimated integer in the division
>>> DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS, core_period) - 1 
>>> underestimates
>>> the necessary cycle count.
>>>
>>> For example, at a 133.3 MHz core rate, the exact period is 7.5 ns but
>>> core_period evaluates to 8 ns. The calculated cycle count evaluates 
>>> to 25
>>> cycles, which produces a 187.5 ns pulse.
>>>
>>> Does this violate the strict 200 ns minimum for tHIGH_INIT required 
>>> by the
>>> I3C specification?
>>
>> Is it true?
>>
>>>
>>>> +           scl_timing = SCL_I3C_TIMING_HCNT(od_hcnt) |
>>>> +                        SCL_I3C_TIMING_LCNT(lcnt);
>>>> +           writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
>>>> +           master->i3c_od_timing = scl_timing;
>>>> +           break;
>>>> +
>>>> +   case I3C_OPEN_DRAIN_NORMAL_SPEED:
>>>> +           writel(master->i3c_od_timing_normal,
>>>> +                  master->regs + SCL_I3C_OD_TIMING);
>>>
>>> [Severity: High]
>>> Does unconditionally restoring master->i3c_od_timing_normal here 
>>> permanently
>>> overwrite hardware-specific AMD timings with generic timings?
>>>
>>> If amd_configure_od_pp_quirk() is not updated to modify
>>> master->i3c_od_timing_normal, restoring it after a broadcast destroys 
>>> the
>>> required AMD-specific quirk timings.
>>>
>>> Additionally, does dw_i3c_master_send_ccc_cmd() unconditionally 
>>> overwrite
>>> the OD timing with AMD normal timing right before sending a broadcast 
>>> CCC,
>>> bypassing the I3C_OPEN_DRAIN_SLOW_SPEED state expected by the I3C core?
>>>
>>>> +           master->i3c_od_timing = master->i3c_od_timing_normal;
>>>> +           break;
>>
>> Please check this also?
>>
>> Frank
>>
>>> [ ... ]
>>>> +static int dw_i3c_master_set_dev_nack_retry(struct 
>>>> i3c_master_controller *m,
>>>> +                                       unsigned long 
>>>> dev_nack_retry_cnt)
>>>> +{
>>> [ ... ]
>>>> +   for (i = 0; i < master->maxdevs; i++) {
>>>> +           /* Skip free/empty slots */
>>>> +           if (master->free_pos & BIT(i))
>>>> +                   continue;
>>>> +
>>>> +           reg = readl(master->regs +
>>>> +                           DEV_ADDR_TABLE_LOC(master->datstartaddr, 
>>>> i));
>>>
>>> [Severity: High]
>>> This is a pre-existing issue, but does this function modify hardware
>>> registers without resuming the device via Runtime PM?
>>>
>>> The sysfs attribute dev_nack_retry_count directly invokes
>>> master->ops->set_dev_nack_retry(). Since sysfs callbacks can be 
>>> triggered
>>> from userspace at any time, executing this while the device is runtime
>>> suspended will access powered-down hardware.
>>>
>>> On ARM SoCs, can this trigger a kernel panic (external abort) and allow
>>> userspace to trivially crash the system?
>>>
>>> [ ... ]
>>>
>>> [Severity: High]
>>> This is a pre-existing issue, but is there a use-after-free or invalid
>>> queue_work() due to improper teardown ordering in 
>>> dw_i3c_common_remove()?
>>>
>>> dw_i3c_common_remove() calls cancel_work_sync(&master->hj_work) 
>>> before the
>>> hardware interrupt is disabled and freed by devres:
>>>
>>> drivers/i3c/master/dw-i3c-master.c:dw_i3c_common_remove() {
>>>      ...
>>>      cancel_work_sync(&master->hj_work);
>>>      i3c_master_unregister(&master->base);
>>>      ...
>>> }
>>>
>>> Because the IRQ remains active throughout the function and during early
>>> devres teardown, can an incoming Hot Join interrupt trigger the IRQ 
>>> handler,
>>> which will call queue_work() to re-queue hj_work?
>>>
>>> Does this either cause queue_work() to execute on a destroyed 
>>> workqueue or
>>> result in a use-after-free when the workqueue subsequently processes the
>>> work using the master object that has already been freed by devres?
>>>
>>> -- 
>>> Sashiko AI review · https://nam10.safelinks.protection.outlook.com/? 
>>> url=https%3A%2F%2Fsashiko.dev%2F%23%2Fpatchset%2Febddb8b62eae92de0a7eeda93cb18213c677ae96.1781077653.git.tze.yee.ng%40altera.com%3Fpart%3D1&data=05%7C02%7Ctze.yee.ng%40altera.com%7C8e2758995cb645fd357608dec7d58fc1%7Cfbd72e03d4a54110adce614d51f2077a%7C0%7C0%7C639167917250216401%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=by5%2FiOLcg8kdN%2FkGUwg3B988Zkk3%2FjQJA%2FOqMllYOY0%3D&reserved=0
>>>
>>> -- 
>>> linux-i3c mailing list
>>> linux-i3c@lists.infradead.org
>>> https://nam10.safelinks.protection.outlook.com/? 
>>> url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux- 
>>> i3c&data=05%7C02%7Ctze.yee.ng%40altera.com%7C8e2758995cb645fd357608dec7d58fc1%7Cfbd72e03d4a54110adce614d51f2077a%7C0%7C0%7C639167917250261515%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=yRfffqH10vlfqPSccrkt3XJkPqEDr7Ecoe08uXGamcQ%3D&reserved=0
> 
> Hi Frank,
> 
> Sashiko is correct that the initial core_period-based calculation could 
> undershoot the 200 ns tHIGH_INIT minimum at some core clock rates. I 
> checked the Zephyr DesignWare I3C driver, which uses a core_rate-based 
> calculation aligned with the DWC timing model. v2 will adopt the same 
> approach:
> 
> od_hcnt = DIV_ROUND_UP((u64)I3C_BUS_THIGH_INIT_OD_MIN_NS * core_rate,
>                         1000000000ULL) - 1;
> 
> still using the existing - 1 and SCL_I3C_TIMING_CNT_MIN convention from 
> dw_i3c_clk_cfg().
> 
> Reference: https://github.com/zephyrproject-rtos/zephyr/ 
> commit/4d6673bc693920bdacf96d7cd3cfeb71d421e0ae  (drivers/i3c/i3c_dw.c, 
> dw_i3c_init_scl_timing())
> 
> For AMD specific timings,  I left existing AMD behavior unchanged in v1. 
> @Manikanta Guntupalli, for AMDI0015, should ->set_speed() use 
> AMD_I3C_OD_TIMING or generic timing as the normal OD baseline at slow/ 
> normal speed? I can add AMD-specific handling in v2 once that is clarified.
> 
> Thanks,
> Tze Yee

Hi Manikanta,

Just checking whether you had a chance to look at the AMD dw-i3c-master 
question below.

For AMDI0015, I need to know whether ->set_speed() should use 
AMD_I3C_OD_TIMING or generic OD timing as the normal baseline, and 
whether send_ccc_cmd() should stop overwriting OD timing before RSTDAA.

Once I have your guidance, I can send an AMD-specific follow-up patch.

Thanks,
Tze Yee Ng

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

RE: [PATCH] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[Guntupalli, Manikanta]

Public

Hi Tze Yee Ng,

We will check this on our end and get back to you shortly with an update.

Thanks,
Manikanta


> -----Original Message-----
> From: NG, TZE YEE <tze.yee.ng@altera.com>
> Sent: Friday, June 19, 2026 1:49 PM
> To: Guntupalli, Manikanta <manikanta.guntupalli@amd.com>
> Cc: linux-i3c@lists.infradead.org; Frank.Li@kernel.org; Frank Li
> <Frank.li@oss.nxp.com>
> Subject: Re: [PATCH] i3c: master: dw-i3c-master: fix OD timing for first broadcast
>
> On 12/6/2026 3:58 pm, Ng, Tze Yee wrote:
> > On 12/6/2026 12:21 am, Frank Li wrote:
> >> [You don't often get email from frank.li@oss.nxp.com. Learn why this
> >> is important at https://aka.ms/LearnAboutSenderIdentification ]
> >>
> >> On Wed, Jun 10, 2026 at 08:36:55AM +0000, sashiko-bot@kernel.org wrote:
> >>> Thank you for your contribution! Sashiko AI review found 5 potential
> >>> issue(s) to consider:
> >>>
> >>> New issues:
> >>> - [High] The newly introduced `dw_i3c_master_set_speed()`
> >>> permanently overwrites hardware-specific AMD timings with generic timings.
> >>> - [High] The `od_hcnt` calculation for `I3C_OPEN_DRAIN_SLOW_SPEED`
> >>> violates the `tHIGH_INIT` minimum requirement.
> >>>
> >>> Pre-existing issues:
> >>> - [High] The IRQ handler accesses hardware registers without Runtime
> >>> PM protection.
> >>> - [High] Use-After-Free and invalid `queue_work()` due to improper
> >>> teardown ordering in `dw_i3c_common_remove()`.
> >>> - [High] `dw_i3c_master_set_dev_nack_retry` modifies hardware
> >>> registers without resuming the device via Runtime PM.
> >>> --
> >>>
> >>> commit bd5322e3d6d6cd3a6a8155f24cc89a12fe61be00
> >>> Author: Tze Yee Ng <tze.yee.ng@altera.com>
> >>>
> >>> i3c: master: dw-i3c-master: fix OD timing for first broadcast
> >>>
> >>> This commit implements the set_speed() callback to switch open-drain
> >>> timing for the first broadcast address to slow speed. It aims to fix
> >>> I2C devices with spike filters not being detected on mixed buses by
> >>> dynamically switching between slow and normal open drain speeds.
> >>>
> >>>> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/
> >>>> master/dw-i3c-master.c index a7593d6efac5a..6cf7071d3d06b 100644
> >>>> --- a/drivers/i3c/master/dw-i3c-master.c
> >>>> +++ b/drivers/i3c/master/dw-i3c-master.c
> >>> [ ... ]
> >>>> @@ -1484,6 +1485,57 @@ static irqreturn_t
> >>>> dw_i3c_master_irq_handler(int irq, void *dev_id)
> >>>>      return IRQ_HANDLED;
> >>>>   }
> >>>
> >>> [Severity: High]
> >>> This is a pre-existing issue, but does the IRQ handler access
> >>> hardware registers without Runtime PM protection?
> >>>
> >>> The interrupt handler directly reads hardware registers before
> >>> checking the device's PM active state:
> >>>
> >>> drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_irq_handler() {
> >>>      ...
> >>>      status = readl(master->regs + INTR_STATUS);
> >>>      ...
> >>> }
> >>>
> >>> Since the driver does not synchronize IRQs in its runtime_suspend
> >>> callback, if an interrupt arrives right as the device enters runtime
> >>> suspend (or if a spurious interrupt occurs), will this raw register
> >>> read access powered-down hardware and cause a synchronous external
> >>> abort?
> >>>
> >>>> +static int dw_i3c_master_set_speed(struct i3c_master_controller
> >>>> +*m,
> >>>> +                              enum i3c_open_drain_speed speed) {
> >>> [ ... ]
> >>>> +   switch (speed) {
> >>>> +   case I3C_OPEN_DRAIN_SLOW_SPEED:
> >>> [ ... ]
> >>>> +           core_period = DIV_ROUND_UP(1000000000, core_rate);
> >>>> +           lcnt =
> >>>> +SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
> >>>> +           od_hcnt = max_t(u8, SCL_I3C_TIMING_CNT_MIN,
> >>>> +
> >>>> +DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS,
> >>>> +                                        core_period) - 1);
> >>>
> >>> [Severity: High]
> >>> Does this precision loss guarantee the resulting pulse is shorter
> >>> than the specification minimum for certain clock frequencies?
> >>>
> >>> If core_period is calculated by rounding up 1000000000 / core_rate,
> >>> using this overestimated integer in the division
> >>> DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS, core_period) - 1
> >>> underestimates the necessary cycle count.
> >>>
> >>> For example, at a 133.3 MHz core rate, the exact period is 7.5 ns
> >>> but core_period evaluates to 8 ns. The calculated cycle count
> >>> evaluates to 25 cycles, which produces a 187.5 ns pulse.
> >>>
> >>> Does this violate the strict 200 ns minimum for tHIGH_INIT required
> >>> by the I3C specification?
> >>
> >> Is it true?
> >>
> >>>
> >>>> +           scl_timing = SCL_I3C_TIMING_HCNT(od_hcnt) |
> >>>> +                        SCL_I3C_TIMING_LCNT(lcnt);
> >>>> +           writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
> >>>> +           master->i3c_od_timing = scl_timing;
> >>>> +           break;
> >>>> +
> >>>> +   case I3C_OPEN_DRAIN_NORMAL_SPEED:
> >>>> +           writel(master->i3c_od_timing_normal,
> >>>> +                  master->regs + SCL_I3C_OD_TIMING);
> >>>
> >>> [Severity: High]
> >>> Does unconditionally restoring master->i3c_od_timing_normal here
> >>> permanently overwrite hardware-specific AMD timings with generic
> >>> timings?
> >>>
> >>> If amd_configure_od_pp_quirk() is not updated to modify
> >>> master->i3c_od_timing_normal, restoring it after a broadcast
> >>> master->destroys
> >>> the
> >>> required AMD-specific quirk timings.
> >>>
> >>> Additionally, does dw_i3c_master_send_ccc_cmd() unconditionally
> >>> overwrite the OD timing with AMD normal timing right before sending
> >>> a broadcast CCC, bypassing the I3C_OPEN_DRAIN_SLOW_SPEED state
> >>> expected by the I3C core?
> >>>
> >>>> +           master->i3c_od_timing = master->i3c_od_timing_normal;
> >>>> +           break;
> >>
> >> Please check this also?
> >>
> >> Frank
> >>
> >>> [ ... ]
> >>>> +static int dw_i3c_master_set_dev_nack_retry(struct
> >>>> i3c_master_controller *m,
> >>>> +                                       unsigned long
> >>>> dev_nack_retry_cnt)
> >>>> +{
> >>> [ ... ]
> >>>> +   for (i = 0; i < master->maxdevs; i++) {
> >>>> +           /* Skip free/empty slots */
> >>>> +           if (master->free_pos & BIT(i))
> >>>> +                   continue;
> >>>> +
> >>>> +           reg = readl(master->regs +
> >>>> +
> >>>> +DEV_ADDR_TABLE_LOC(master->datstartaddr,
> >>>> i));
> >>>
> >>> [Severity: High]
> >>> This is a pre-existing issue, but does this function modify hardware
> >>> registers without resuming the device via Runtime PM?
> >>>
> >>> The sysfs attribute dev_nack_retry_count directly invokes
> >>> master->ops->set_dev_nack_retry(). Since sysfs callbacks can be
> >>> triggered
> >>> from userspace at any time, executing this while the device is
> >>> runtime suspended will access powered-down hardware.
> >>>
> >>> On ARM SoCs, can this trigger a kernel panic (external abort) and
> >>> allow userspace to trivially crash the system?
> >>>
> >>> [ ... ]
> >>>
> >>> [Severity: High]
> >>> This is a pre-existing issue, but is there a use-after-free or
> >>> invalid
> >>> queue_work() due to improper teardown ordering in
> >>> dw_i3c_common_remove()?
> >>>
> >>> dw_i3c_common_remove() calls cancel_work_sync(&master->hj_work)
> >>> before the
> >>> hardware interrupt is disabled and freed by devres:
> >>>
> >>> drivers/i3c/master/dw-i3c-master.c:dw_i3c_common_remove() {
> >>>      ...
> >>>      cancel_work_sync(&master->hj_work);
> >>>      i3c_master_unregister(&master->base);
> >>>      ...
> >>> }
> >>>
> >>> Because the IRQ remains active throughout the function and during
> >>> early devres teardown, can an incoming Hot Join interrupt trigger
> >>> the IRQ handler, which will call queue_work() to re-queue hj_work?
> >>>
> >>> Does this either cause queue_work() to execute on a destroyed
> >>> workqueue or result in a use-after-free when the workqueue
> >>> subsequently processes the work using the master object that has
> >>> already been freed by devres?
> >>>
> >>> --
> >>> Sashiko AI review · https://nam10.safelinks.protection.outlook.com/?
> >>>
> url=https%3A%2F%2Fsashiko.dev%2F%23%2Fpatchset%2Febddb8b62eae92de0a
> 7
> >>> eeda93cb18213c677ae96.1781077653.git.tze.yee.ng%40altera.com%3Fpart%
> >>>
> 3D1&data=05%7C02%7Ctze.yee.ng%40altera.com%7C8e2758995cb645fd357608
> d
> >>>
> ec7d58fc1%7Cfbd72e03d4a54110adce614d51f2077a%7C0%7C0%7C6391679172
> 502
> >>>
> 16401%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLj
> AuMD
> >>>
> AwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%
> 7C&
> >>>
> sdata=by5%2FiOLcg8kdN%2FkGUwg3B988Zkk3%2FjQJA%2FOqMllYOY0%3D&r
> eserve
> >>> d=0
> >>>
> >>> --
> >>> linux-i3c mailing list
> >>> linux-i3c@lists.infradead.org
> >>> https://nam10.safelinks.protection.outlook.com/?
> >>> url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-
> >>>
> i3c&data=05%7C02%7Ctze.yee.ng%40altera.com%7C8e2758995cb645fd357608d
> >>>
> ec7d58fc1%7Cfbd72e03d4a54110adce614d51f2077a%7C0%7C0%7C6391679172
> 502
> >>>
> 61515%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLj
> AuMD
> >>>
> AwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%
> 7C&
> >>> sdata=yRfffqH10vlfqPSccrkt3XJkPqEDr7Ecoe08uXGamcQ%3D&reserved=0
> >
> > Hi Frank,
> >
> > Sashiko is correct that the initial core_period-based calculation
> > could undershoot the 200 ns tHIGH_INIT minimum at some core clock
> > rates. I checked the Zephyr DesignWare I3C driver, which uses a
> > core_rate-based calculation aligned with the DWC timing model. v2 will
> > adopt the same
> > approach:
> >
> > od_hcnt = DIV_ROUND_UP((u64)I3C_BUS_THIGH_INIT_OD_MIN_NS *
> core_rate,
> >                         1000000000ULL) - 1;
> >
> > still using the existing - 1 and SCL_I3C_TIMING_CNT_MIN convention
> > from dw_i3c_clk_cfg().
> >
> > Reference: https://github.com/zephyrproject-rtos/zephyr/
> > commit/4d6673bc693920bdacf96d7cd3cfeb71d421e0ae
> > (drivers/i3c/i3c_dw.c,
> > dw_i3c_init_scl_timing())
> >
> > For AMD specific timings,  I left existing AMD behavior unchanged in v1.
> > @Manikanta Guntupalli, for AMDI0015, should ->set_speed() use
> > AMD_I3C_OD_TIMING or generic timing as the normal OD baseline at slow/
> > normal speed? I can add AMD-specific handling in v2 once that is clarified.
> >
> > Thanks,
> > Tze Yee
>
> Hi Manikanta,
>
> Just checking whether you had a chance to look at the AMD dw-i3c-master question
> below.
>
> For AMDI0015, I need to know whether ->set_speed() should use
> AMD_I3C_OD_TIMING or generic OD timing as the normal baseline, and whether
> send_ccc_cmd() should stop overwriting OD timing before RSTDAA.
>
> Once I have your guidance, I can send an AMD-specific follow-up patch.
>
> Thanks,
> Tze Yee Ng

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

Re: [PATCH] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[Frank Li]

On Wed, Jun 10, 2026 at 01:14:26AM -0700, tze.yee.ng@altera.com wrote:
> From: Tze Yee Ng <tze.yee.ng@altera.com>
>
> Implement ->set_speed() so the I3C core can switch open-drain timing for
> the first broadcast address per spec: I3C_OPEN_DRAIN_SLOW_SPEED programs
> tHIGH_INIT (200 ns) before RSTDAA, and I3C_OPEN_DRAIN_NORMAL_SPEED restores
> normal OD timing afterward. Cache the normal OD register value during bus
> init and use a separate od_hcnt for the slow path so SDR extended timing
> remains derived from the normal PP hcnt.
>
> Fixes I2C devices with spike filters not being detected on mixed buses.
>
> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
> ---
>  drivers/i3c/master/dw-i3c-master.c | 53 ++++++++++++++++++++++++++++++
>  drivers/i3c/master/dw-i3c-master.h |  1 +
>  include/linux/i3c/master.h         |  1 +
>  3 files changed, 55 insertions(+)
>
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 655693a2187e..dbb801910b3d 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -592,6 +592,7 @@ static int dw_i3c_clk_cfg(struct dw_i3c_master *master)
>  	scl_timing = SCL_I3C_TIMING_HCNT(hcnt) | SCL_I3C_TIMING_LCNT(lcnt);
>  	writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
>  	master->i3c_od_timing = scl_timing;
> +	master->i3c_od_timing_normal = scl_timing;
>
>  	lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR1_SCL_RATE) - hcnt;
>  	scl_timing = SCL_EXT_LCNT_1(lcnt);
> @@ -1480,6 +1481,57 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>
> +static int dw_i3c_master_set_speed(struct i3c_master_controller *m,
> +				   enum i3c_open_drain_speed speed)
> +{
> +	struct dw_i3c_master *master = to_dw_i3c_master(m);
> +	unsigned long core_rate, core_period;
> +	u32 scl_timing;
> +	u8 od_hcnt, lcnt;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(master->dev);
> +	if (ret < 0) {
> +		dev_err(master->dev,
> +			"<%s> cannot resume i3c bus master, err: %d\n",
> +			__func__, ret);
> +		return ret;
> +	}

	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev->dev, pm);
	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
		return -ENXIO;

Use modem QCQUIRE macro

> +
> +	switch (speed) {
> +	case I3C_OPEN_DRAIN_SLOW_SPEED:
> +		core_rate = clk_get_rate(master->core_clk);
> +		if (!core_rate) {
> +			ret = -EINVAL;
> +			break;

you can return ret; here after use ACQUIRE macro.

Frank
> +		}
> +
> +		core_period = DIV_ROUND_UP(1000000000, core_rate);
> +		lcnt = SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
> +		od_hcnt = max_t(u8, SCL_I3C_TIMING_CNT_MIN,
> +				DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS,
> +					     core_period) - 1);
> +		scl_timing = SCL_I3C_TIMING_HCNT(od_hcnt) |
> +			     SCL_I3C_TIMING_LCNT(lcnt);
> +		writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
> +		master->i3c_od_timing = scl_timing;
> +		break;
> +
> +	case I3C_OPEN_DRAIN_NORMAL_SPEED:
> +		writel(master->i3c_od_timing_normal,
> +		       master->regs + SCL_I3C_OD_TIMING);
> +		master->i3c_od_timing = master->i3c_od_timing_normal;
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +	pm_runtime_put_autosuspend(master->dev);
> +
> +	return ret;
> +}
> +
>  static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
>  					    unsigned long dev_nack_retry_cnt)
>  {
> @@ -1534,6 +1586,7 @@ static const struct i3c_master_controller_ops dw_mipi_i3c_ops = {
>  	.recycle_ibi_slot = dw_i3c_master_recycle_ibi_slot,
>  	.enable_hotjoin = dw_i3c_master_enable_hotjoin,
>  	.disable_hotjoin = dw_i3c_master_disable_hotjoin,
> +	.set_speed = dw_i3c_master_set_speed,
>  	.set_dev_nack_retry = dw_i3c_master_set_dev_nack_retry,
>  };
>
> diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
> index c5cb695c16ab..c814087a6f1f 100644
> --- a/drivers/i3c/master/dw-i3c-master.h
> +++ b/drivers/i3c/master/dw-i3c-master.h
> @@ -46,6 +46,7 @@ struct dw_i3c_master {
>  	u32 dev_addr;
>  	u32 i3c_pp_timing;
>  	u32 i3c_od_timing;
> +	u32 i3c_od_timing_normal;
>  	u32 ext_lcnt_timing;
>  	u32 bus_free_timing;
>  	u32 i2c_fm_timing;
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 592b646f6134..c7d7448c4cd7 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -259,6 +259,7 @@ struct i3c_device {
>  #define I3C_BUS_THIGH_MIXED_MAX_NS	41
>  #define I3C_BUS_TIDLE_MIN_NS		200000
>  #define I3C_BUS_TLOW_OD_MIN_NS		200
> +#define I3C_BUS_THIGH_INIT_OD_MIN_NS	200
>
>  /**
>   * enum i3c_bus_mode - I3C bus mode
> --
> 2.43.7
>

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

Re: [PATCH] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[NG, TZE YEE]

On 12/6/2026 12:19 am, Frank Li wrote:
> [You don't often get email from frank.li@oss.nxp.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Wed, Jun 10, 2026 at 01:14:26AM -0700, tze.yee.ng@altera.com wrote:
>> From: Tze Yee Ng <tze.yee.ng@altera.com>
>>
>> Implement ->set_speed() so the I3C core can switch open-drain timing for
>> the first broadcast address per spec: I3C_OPEN_DRAIN_SLOW_SPEED programs
>> tHIGH_INIT (200 ns) before RSTDAA, and I3C_OPEN_DRAIN_NORMAL_SPEED restores
>> normal OD timing afterward. Cache the normal OD register value during bus
>> init and use a separate od_hcnt for the slow path so SDR extended timing
>> remains derived from the normal PP hcnt.
>>
>> Fixes I2C devices with spike filters not being detected on mixed buses.
>>
>> Signed-off-by: Tze Yee Ng <tze.yee.ng@altera.com>
>> ---
>>   drivers/i3c/master/dw-i3c-master.c | 53 ++++++++++++++++++++++++++++++
>>   drivers/i3c/master/dw-i3c-master.h |  1 +
>>   include/linux/i3c/master.h         |  1 +
>>   3 files changed, 55 insertions(+)
>>
>> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
>> index 655693a2187e..dbb801910b3d 100644
>> --- a/drivers/i3c/master/dw-i3c-master.c
>> +++ b/drivers/i3c/master/dw-i3c-master.c
>> @@ -592,6 +592,7 @@ static int dw_i3c_clk_cfg(struct dw_i3c_master *master)
>>        scl_timing = SCL_I3C_TIMING_HCNT(hcnt) | SCL_I3C_TIMING_LCNT(lcnt);
>>        writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
>>        master->i3c_od_timing = scl_timing;
>> +     master->i3c_od_timing_normal = scl_timing;
>>
>>        lcnt = DIV_ROUND_UP(core_rate, I3C_BUS_SDR1_SCL_RATE) - hcnt;
>>        scl_timing = SCL_EXT_LCNT_1(lcnt);
>> @@ -1480,6 +1481,57 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
>>        return IRQ_HANDLED;
>>   }
>>
>> +static int dw_i3c_master_set_speed(struct i3c_master_controller *m,
>> +                                enum i3c_open_drain_speed speed)
>> +{
>> +     struct dw_i3c_master *master = to_dw_i3c_master(m);
>> +     unsigned long core_rate, core_period;
>> +     u32 scl_timing;
>> +     u8 od_hcnt, lcnt;
>> +     int ret;
>> +
>> +     ret = pm_runtime_resume_and_get(master->dev);
>> +     if (ret < 0) {
>> +             dev_err(master->dev,
>> +                     "<%s> cannot resume i3c bus master, err: %d\n",
>> +                     __func__, ret);
>> +             return ret;
>> +     }
> 
>          PM_RUNTIME_ACQUIRE_AUTOSUSPEND(dev->dev, pm);
>          if (PM_RUNTIME_ACQUIRE_ERR(&pm))
>                  return -ENXIO;
> 
> Use modem QCQUIRE macro
> 
>> +
>> +     switch (speed) {
>> +     case I3C_OPEN_DRAIN_SLOW_SPEED:
>> +             core_rate = clk_get_rate(master->core_clk);
>> +             if (!core_rate) {
>> +                     ret = -EINVAL;
>> +                     break;
> 
> you can return ret; here after use ACQUIRE macro.
> 
> Frank
>> +             }
>> +
>> +             core_period = DIV_ROUND_UP(1000000000, core_rate);
>> +             lcnt = SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
>> +             od_hcnt = max_t(u8, SCL_I3C_TIMING_CNT_MIN,
>> +                             DIV_ROUND_UP(I3C_BUS_THIGH_INIT_OD_MIN_NS,
>> +                                          core_period) - 1);
>> +             scl_timing = SCL_I3C_TIMING_HCNT(od_hcnt) |
>> +                          SCL_I3C_TIMING_LCNT(lcnt);
>> +             writel(scl_timing, master->regs + SCL_I3C_OD_TIMING);
>> +             master->i3c_od_timing = scl_timing;
>> +             break;
>> +
>> +     case I3C_OPEN_DRAIN_NORMAL_SPEED:
>> +             writel(master->i3c_od_timing_normal,
>> +                    master->regs + SCL_I3C_OD_TIMING);
>> +             master->i3c_od_timing = master->i3c_od_timing_normal;
>> +             break;
>> +
>> +     default:
>> +             ret = -EINVAL;
>> +             break;
>> +     }
>> +     pm_runtime_put_autosuspend(master->dev);
>> +
>> +     return ret;
>> +}
>> +
>>   static int dw_i3c_master_set_dev_nack_retry(struct i3c_master_controller *m,
>>                                            unsigned long dev_nack_retry_cnt)
>>   {
>> @@ -1534,6 +1586,7 @@ static const struct i3c_master_controller_ops dw_mipi_i3c_ops = {
>>        .recycle_ibi_slot = dw_i3c_master_recycle_ibi_slot,
>>        .enable_hotjoin = dw_i3c_master_enable_hotjoin,
>>        .disable_hotjoin = dw_i3c_master_disable_hotjoin,
>> +     .set_speed = dw_i3c_master_set_speed,
>>        .set_dev_nack_retry = dw_i3c_master_set_dev_nack_retry,
>>   };
>>
>> diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
>> index c5cb695c16ab..c814087a6f1f 100644
>> --- a/drivers/i3c/master/dw-i3c-master.h
>> +++ b/drivers/i3c/master/dw-i3c-master.h
>> @@ -46,6 +46,7 @@ struct dw_i3c_master {
>>        u32 dev_addr;
>>        u32 i3c_pp_timing;
>>        u32 i3c_od_timing;
>> +     u32 i3c_od_timing_normal;
>>        u32 ext_lcnt_timing;
>>        u32 bus_free_timing;
>>        u32 i2c_fm_timing;
>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
>> index 592b646f6134..c7d7448c4cd7 100644
>> --- a/include/linux/i3c/master.h
>> +++ b/include/linux/i3c/master.h
>> @@ -259,6 +259,7 @@ struct i3c_device {
>>   #define I3C_BUS_THIGH_MIXED_MAX_NS   41
>>   #define I3C_BUS_TIDLE_MIN_NS         200000
>>   #define I3C_BUS_TLOW_OD_MIN_NS               200
>> +#define I3C_BUS_THIGH_INIT_OD_MIN_NS 200
>>
>>   /**
>>    * enum i3c_bus_mode - I3C bus mode
>> --
>> 2.43.7
>>
Hi Frank,

Thanks for review. I will change to use PM_RUNTIME_ACQUIRE in v2.

Thanks,
Tze Yee
-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c