Re: [PATCH v7 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:
- [Medium] Dead Code / Missing Feature Implementation for DW_I3C_ACPI_SKIP_CLK_RST

Pre-existing issues:
- [High] Runtime PM and Resource Cleanup Bug during driver unbind
- [High] Type Confusion and Pointer Truncation for PRP0001 ACPI Devices
--

--- Patch [8]: [PATCH v7 08/12] i3c: dw-i3c-master: Add ACPI core clock frequency quirk ---
commit 7469594594011ab29fdf220e06f76b074bff94d1
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..273d7a0020993 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
[ ... ]
> @@ -1573,14 +1589,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);

[Severity: High]
This isn't a bug introduced by this patch, but does this cast cause type
confusion for PRP0001 ACPI devices?

When an ACPI device binds to the driver using the PRP0001 OF-compatibility
mechanism, device_get_match_data() delegates to the OF match table and
returns a pointer to struct dw_i3c_drvdata.

Because has_acpi_companion() returns true, this branch will cast that
pointer directly to an unsigned long, treating the memory address as a
bitmask of quirks. This could corrupt the driver state.

> +	} else if (pdev->dev.of_node) {
> +		drvdata = device_get_match_data(&pdev->dev);
> +		if (drvdata)
> +			quirks = drvdata->flags;
> +	}
> +	master->quirks = quirks;
> +
>  	master->regs = devm_platform_ioremap_resource(pdev, 0);
>  	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);
>  	if (IS_ERR(master->core_clk))
>  		return PTR_ERR(master->core_clk);
>  
> +	if (!master->core_clk && !(master->quirks & DW_I3C_ACPI_SKIP_CLK_RST)) {

[Severity: Medium]
Will this driver still fail to probe on ACPI platforms without a core clock?

The code checks if DW_I3C_ACPI_SKIP_CLK_RST is set in master->quirks.
However, it appears this quirk is never included in the driver's ACPI
match table (amd_i3c_device_match). Without being populated by the ACPI
matching framework, the feature seems to act as dead code.

> +		dev_err(&pdev->dev, "missing core clock\n");
> +		return -EINVAL;
> +	}
> +

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but is there a runtime PM and
resource cleanup issue during driver unbind?

When the device is removed (e.g., driver unbound via sysfs) while in a
runtime-suspended state, or an autosuspend timer fires concurrently with the
remove callback, the driver core calls pm_runtime_put_sync() before remove.

This means the device may be suspended when dw_i3c_common_remove() executes.
The callback unregisters the I3C master:

dw_i3c_common_remove()
    i3c_master_unregister(&master->base)
        dw_i3c_master_bus_cleanup()
            dw_i3c_master_disable()

This access to the DEVICE_CTRL MMIO register happens while the controller
might be unclocked, which could cause a synchronous external abort
(kernel panic).

Would it be safer to wake the device via pm_runtime_get_sync() before
unregistering the bus?

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