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

[Akhil R <akhilrajeev@nvidia.com>]

On Tue, 21 Jul 2026 04:26:15 +0000, Sashiko AI review <sashiko-bot@kernel.org> wrote:
> commit f3333de342eb1c266eb94909516ea27afe8378d5
> 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.
>
> > diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> > index 29030fd9594aa..3ec3ab1c13b4c 100644
> > --- a/drivers/i3c/master/dw-i3c-master.c
> > +++ b/drivers/i3c/master/dw-i3c-master.c
>
> [ ... ]
>
> > @@ -556,13 +557,33 @@ static void dw_i3c_master_set_intr_regs(struct dw_i3c_master *master)
> > +static unsigned long dw_i3c_master_get_core_rate(struct dw_i3c_master *master)
> > +{
> > +	unsigned int core_rate_prop;
> > +
> > +	if (master->core_clk)
> > +		return clk_get_rate(master->core_clk);
> > +
> > +	if (!(master->quirks & DW_I3C_ACPI_SKIP_CLK_RST)) {
> > +		dev_err(master->dev, "missing core clock\n");
> > +		return 0;
> > +	}
>
> [Severity: Low]
> Is this check reachable?
>
> During initialization in dw_i3c_common_probe(), if the core clock is missing
> and DW_I3C_ACPI_SKIP_CLK_RST is not set, probe immediately aborts with -EINVAL.
>
> By the time dw_i3c_master_get_core_rate() is executed, it seems guaranteed
> that if core_clk is missing, the DW_I3C_ACPI_SKIP_CLK_RST quirk is already
> set, making this fallback check dead code.

The check is defensive and is harmless. I believe it adds to readability,
but can be cleaned up if preferred.

> > +
> > +	if (device_property_read_u32(master->dev, "clock-frequency", &core_rate_prop)) {
> > +		dev_err(master->dev, "missing clock-frequency property\n");
> > +		return 0;
> > +	}
> > +
> > +	return core_rate_prop;
> > +}
>
> [ ... ]
>
> > @@ -1573,14 +1594,28 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
> > +	if (has_acpi_companion(&pdev->dev)) {
> > +		quirks = (unsigned long)device_get_match_data(&pdev->dev);
> > +	} else if (pdev->dev.of_node) {
>
> [Severity: High]
> This isn't a bug introduced by this patch, but since this block was
> relocated, I noticed a potential issue for devices using ACPI PRP0001.
>
> If the device is instantiated via ACPI using the PRP0001 HID,
> device_get_match_data() correctly falls back to the OF match table and
> returns a pointer to struct dw_i3c_drvdata.
>
> Does casting this pointer to an unsigned long and assigning it to
> master->quirks cause random kernel pointers to be used as a quirk bitmask?
> Depending on the kernel layout, this might cause erratic driver behavior such
> as missing PM operations.

> > +		drvdata = device_get_match_data(&pdev->dev);
> > +		if (drvdata)
> > +			quirks = drvdata->flags;
> > +	}
> > +	master->quirks = quirks;
>
> [ ... ]
>
> > @@ -1636,15 +1671,6 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
> > -	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;
> > -	}
> > -	master->quirks = quirks;
> > -
>
> [Severity: High]
> This is a pre-existing issue, but I noticed a potential problem in the device
> removal path regarding runtime PM state.
>
> When the driver is unbound, dw_i3c_common_remove() calls
> i3c_master_unregister(), which internally calls dw_i3c_master_bus_cleanup().
> The dw_i3c_master_bus_cleanup() function accesses the DEVICE_CTRL MMIO
> register. Because the driver does not wake the device via
> pm_runtime_get_sync() before unregistration, the device may be in a
> runtime-suspended state with its core clocks disabled.
>
> Could this cause a fatal bus fault on platforms like ARM due to unclocked MMIO
> accesses? Additionally, if the device remains suspended, will the subsequent
> devres cleanup attempt to disable the already-disabled clocks, causing a clock
> enable counter underflow warning?

These are existing issues in the driver. It would be better to address them
separately.

Best Regards,
Akhil

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