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

On Wed, Jun 10, 2026 at 08:36:55AM +0000, [email protected] 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 <[email protected]>
>
> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-i3c

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c