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