Re: [PATCH v6 09/12] i3c: dw-i3c-master: Add ACPI ID for Tegra410

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

Pre-existing issues:
- [High] Unsafe pointer cast of device_get_match_data() return value when binding via ACPI PRP0001.
- [High] Double-disable of core clocks during driver removal if the device is in runtime suspend.
--

--- Patch [9]: [PATCH v6 09/12] i3c: dw-i3c-master: Add ACPI ID for Tegra410 ---
commit bf82e9c156c66763c855e5549bd1a91bafc5fdf3
Author: Akhil R <[email protected]>

i3c: dw-i3c-master: Add ACPI ID for Tegra410
    
Update variable names to generic names and add Tegra410 ACPI ID to
support the I3C controller in Tegra410, which is a DesignWare I3C host
controller.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 3ec3ab1c13b4c..efe37f5224c71 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -1860,11 +1860,12 @@ static const struct of_device_id dw_i3c_master_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, dw_i3c_master_of_match);
>  
> -static const struct acpi_device_id amd_i3c_device_match[] = {
> +static const struct acpi_device_id dw_i3c_master_acpi_match[] = {
>  	{ "AMDI0015", AMD_I3C_OD_PP_TIMING },
> +	{ "NVDA2018", DW_I3C_ACPI_SKIP_CLK_RST },

[Severity: High]
This isn't a bug introduced by this patch, but adding a new ACPI ID brings
attention to a pre-existing issue when handling ACPI bindings.

Does this driver safely handle ACPI PRP0001 bindings?

In dw_i3c_common_probe(), if an ACPI companion is present, the match data is
cast to an integer bitmask:

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

When an ACPI device binds using the PRP0001 mechanism, it utilizes the OF
compatible strings and the OF match table. In this scenario,
device_get_match_data() returns the .data pointer from the of_device_id
table (e.g. &altr_agilex5_drvdata), rather than an integer quirk bitmask
from the acpi_device_id table.

Because of the unconditional cast, won't this store a kernel memory address
into master->quirks instead of a valid bitmask, thereby activating random
driver quirks?

[Severity: High]
This is also a pre-existing issue, but does dw_i3c_common_remove() correctly
handle devres clock cleanup when the device is in runtime suspend?

The probe function acquires and enables the core and pclk clocks using
devm_clk_get_optional_enabled():

	master->core_clk = devm_clk_get_optional_enabled(&pdev->dev, NULL);
...
	master->pclk = devm_clk_get_optional_enabled(&pdev->dev, "pclk");

These clocks are explicitly disabled in dw_i3c_master_runtime_suspend(). 

If dw_i3c_common_remove() is invoked while the device is in runtime suspend,
the clocks are already disabled. Since dw_i3c_common_remove() does not wake
the device (e.g., via pm_runtime_get_sync()), won't the devres cleanup
routine run clk_disable_unprepare() again on already disabled clocks,
leading to a negative clock enable count?

>  	{ }
>  };
> -MODULE_DEVICE_TABLE(acpi, amd_i3c_device_match);
> +MODULE_DEVICE_TABLE(acpi, dw_i3c_master_acpi_match);

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

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