Re: [PATCH v5 08/12] i3c: dw-i3c-master: Add ACPI core clock frequency quirk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Changing `core_clk` from mandatory to optional delays missing clock validation, leading to unclocked hardware register accesses that can trigger a synchronous external abort (system crash) on misconfigured platforms.

Pre-existing issues:
- [High] The workqueue `master->wq` is destroyed during unregistration, but the devres-managed hardware IRQ remains active without being masked, allowing a late interrupt to queue work on the destroyed workqueue.
- [High] The driver unconditionally casts the pointer returned by `device_get_match_data()` to an `unsigned long` when binding an ACPI companion, leading to garbage quirk flags if the device is bound via the PRP0001 ACPI-OF fallback.
--

commit d5fbc1910ed540d2be9a4cb57886399f9c4f9b04
Author: Akhil R <akhilrajeev@nvidia.com>

i3c: dw-i3c-master: Add ACPI core clock frequency quirk

Some ACPI-enumerated devices like Tegra410 do not expose the controller
core clock through the clk framework. Unlike device tree, ACPI on Arm does
not model clock providers. The hardware is expected to have its clocks
enabled by firmware before the OS takes over.

Make the core clock optional and allow selected ACPI devices to provide the
core clock rate through the "clock-frequency" _DSD property when the core
clock is absent.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 29030fd9594aa..8e40d178d500a 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c

[ ... ]

> @@ -1577,7 +1598,7 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>  	if (IS_ERR(master->regs))
>  		return PTR_ERR(master->regs);
>  
> -	master->core_clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	master->core_clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);

[Severity: High]
If the device tree is missing the core clock, this will now return NULL
instead of an error. Could this lead to unclocked MMIO accesses?

In dw_i3c_common_probe():

    master->core_clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
    ...
    writel(INTR_ALL, master->regs + INTR_STATUS);

If the clock is missing, the MMIO access to INTR_STATUS will happen
unconditionally, which can trigger a synchronous external abort on some
systems.

The missing clock is checked later in dw_i3c_master_get_core_rate():

    if (!(master->quirks & DW_I3C_ACPI_SKIP_CLK_RST)) {
        dev_err(master->dev, "missing core clock\n");
        return 0;
    }

Is it possible to validate the missing clock earlier to prevent the kernel
panic during the probe sequence?

>  	if (IS_ERR(master->core_clk))
>  		return PTR_ERR(master->core_clk);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does the removal path
destroy the workqueue while hardware interrupts are still active?

In drivers/i3c/master/dw-i3c-master.c:dw_i3c_common_remove():

    i3c_master_unregister(&master->base);

This unregisters the device and eventually triggers:

drivers/i3c/master.c:i3c_masterdev_release():
    if (master->wq)
        destroy_workqueue(master->wq);

Since INTR_STATUS_EN is never cleared to mask interrupts and the devres
managed IRQ is not yet freed, could a delayed IBI or Hot-Join interrupt
still fire?

If it does, the still-active IRQ handler will attempt to queue work on
the destroyed workqueue:

drivers/i3c/master/dw-i3c-master.c:dw_i3c_master_irq_handle_ibis():
    } else if (IBI_TYPE_HJ(reg)) {
        i3c_master_queue_hotjoin(&master->base);
    }

Can this result in a use-after-free?

[Severity: High]
This is a pre-existing issue, but does this unconditionally cast the match
data pointer to an integer?

In drivers/i3c/master/dw-i3c-master.c:dw_i3c_common_probe():

    if (has_acpi_companion(&pdev->dev)) {
        quirks = (unsigned long)device_get_match_data(&pdev->dev);
    } else if (pdev->dev.of_node) {
        drvdata = device_get_match_data(&pdev->dev);
        if (drvdata)
            quirks = drvdata->flags;
    }

If the device uses the PRP0001 ACPI-OF fallback, has_acpi_companion() will
be true, and device_get_match_data() will return a pointer to the OF table
struct dw_i3c_drvdata.

Does this cast the pointer address directly into the quirks bitmask, causing
unpredictable feature toggles?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624102153.1770072-1-akhilrajeev@nvidia.com?part=8

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c