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

[sashiko-bot@kernel.org]

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