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

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

New issues:
- [Low] Structurally unreachable error condition in `dw_i3c_master_get_core_rate()`.

Pre-existing issues:
- [High] Missing pm_runtime_get_sync() in device removal path causes unclocked MMIO access bus faults and devres clock refcount underflows.
- [High] Incorrect casting of device_get_match_data() result for ACPI PRP0001 devices causes random kernel pointers to be used as quirk bitmasks.
--

--- Patch [8]: [PATCH v6 08/12] i3c: dw-i3c-master: Add ACPI core clock frequency quirk ---
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)
>  	writel(IBI_REQ_REJECT_ALL, master->regs + IBI_MR_REQ_REJECT);
>  }
>  
> +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.

> +
> +	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,
>  
>  	master->dev = &pdev->dev;
>  
> +	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;
> +
>  	master->regs = devm_platform_ioremap_resource(pdev, 0);

[ ... ]

> @@ -1636,15 +1671,6 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>  		master->has_ibi_data = true;
>  	writel(thld_ctrl, master->regs + QUEUE_THLD_CTRL);
>  
> -	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;
> -
>  	/* Keep controller enabled by preventing runtime suspend */
>  	if (master->quirks & DW_I3C_DISABLE_RUNTIME_PM_QUIRK)
>  		pm_runtime_get_noresume(&pdev->dev);

[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():

dw_i3c_common_remove()
    i3c_master_unregister()
        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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8

-- 
linux-i3c mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-i3c