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

On Mon, Jul 27, 2026 at 02:41:12PM +0000, Akhil R wrote:
> On Tue, 21 Jul 2026 04:26:15 +0000, Sashiko AI review <[email protected]> wrote:
> > commit f3333de342eb1c266eb94909516ea27afe8378d5
> > Author: Akhil R <[email protected]>
> >
> > 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.

Yes, needn't double check if probe already checked.


Frank

>
> > > +
> > > +	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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c