[PATCH] i3c: master: dw-i3c-master: fix OD timing for first broadcast
NG, TZE YEE
tze.yee.ng at altera.com
Fri Jun 19 01:18:39 PDT 2026
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 at 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 at 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 at 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 at 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
More information about the linux-i3c
mailing list