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

Public

Hi Tze Yee Ng,

We will check this on our end and get back to you shortly with an update.

Thanks,
Manikanta


> -----Original Message-----
> From: NG, TZE YEE <[email protected]>
> Sent: Friday, June 19, 2026 1:49 PM
> To: Guntupalli, Manikanta <[email protected]>
> Cc: [email protected]; [email protected]; Frank Li
> <[email protected]>
> Subject: 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
> >>> master->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%2Febddb8b62eae92de0a
> 7
> >>> eeda93cb18213c677ae96.1781077653.git.tze.yee.ng%40altera.com%3Fpart%
> >>>
> 3D1&data=05%7C02%7Ctze.yee.ng%40altera.com%7C8e2758995cb645fd357608
> d
> >>>
> ec7d58fc1%7Cfbd72e03d4a54110adce614d51f2077a%7C0%7C0%7C6391679172
> 502
> >>>
> 16401%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLj
> AuMD
> >>>
> AwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%
> 7C&
> >>>
> sdata=by5%2FiOLcg8kdN%2FkGUwg3B988Zkk3%2FjQJA%2FOqMllYOY0%3D&r
> eserve
> >>> d=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%7C8e2758995cb645fd357608d
> >>>
> ec7d58fc1%7Cfbd72e03d4a54110adce614d51f2077a%7C0%7C0%7C6391679172
> 502
> >>>
> 61515%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLj
> AuMD
> >>>
> AwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%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