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

Public

Hi,

Adding the below contact:
+@Datta, Shubhrajyoti

Thanks,
Manikanta

> -----Original Message-----
> From: NG, TZE YEE <[email protected]>
> Sent: Wednesday, July 1, 2026 11:16 AM
> To: Guntupalli, Manikanta <[email protected]>
> Cc: [email protected]; [email protected]; Frank Li
> <[email protected]>
> Subject: Re: RE: [PATCH] i3c: master: dw-i3c-master: fix OD timing for first
> broadcast
>
> On 24/6/2026 2:44 pm, Guntupalli, Manikanta wrote:
> > Public
> >
> > Hi Tze Yee Ng,
> >
> > We will check this on our end and get back to you shortly with an update.
> >
> > Thanks,
> > Manikanta
> >
> >
>
> Hi Manikanta,
>
> Gentle follow-up on the AMD quirk question below. It has been about a week since
> your 24 June reply, and I have not seen an update yet.
>
> For AMDI0015 with AMD_I3C_OD_PP_TIMING, I still need your guidance on:
> - what OD timing ->set_speed(NORMAL) should restore,
> - how ->set_speed(SLOW) should be programmed, and
> - whether send_ccc_cmd() should keep forcing AMD_I3C_OD_TIMING before every
> CCC.
>
> Once I have your input, I can send an AMD-specific follow-up patch.
>
> Thanks,
> Tze Yee Ng
>
> >> -----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%3Fpar
> >>>>> t%
> >>>>>
> >>
> 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