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

On 12/6/2026 12:21 am, Frank Li wrote:
> [You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> 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

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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c