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

[Akhil R <akhilrajeev@nvidia.com>]

On Tue, 21 Jul 2026 04:22:59 +0000, Sashiko AI review <sashiko-bot@kernel.org> wrote:
> commit bf82e9c156c66763c855e5549bd1a91bafc5fdf3
> Author: Akhil R <akhilrajeev@nvidia.com>
>
> 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(). 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?

Both are pre-existing behaviours not introduced by adding the Tegra410
ACPI ID; they are better handled separately.

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

Best Regards,
Akhil

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