Re: [PATCH v4 08/12] i3c: dw-i3c-master: Add a quirk to skip clock and reset

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

Pre-existing issues:
- [High] Type confusion when an ACPI device binds via PRP0001, causing memory addresses to be cast to hardware quirk flags.
- [High] Double disable of clocks during driver unbind if the device is runtime suspended.
--

commit 40ac6ebf3eb6f03540ce6b0b57bfd00730536f5d
Author: Akhil R <[email protected]>

i3c: dw-i3c-master: Add a quirk to skip clock and reset

Some ACPI-enumerated devices like Tegra410 do not have clock and reset
resources exposed via the clk/reset frameworks. Unlike device tree,
ACPI on Arm does not model such provider functions.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 3e510fddf06c5..a2a4b88c20177 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c

[ ... ]

> @@ -1577,18 +1593,33 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>  	if (IS_ERR(master->regs))
>  		return PTR_ERR(master->regs);
>  
> -	master->core_clk = devm_clk_get_enabled(&pdev->dev, NULL);
> -	if (IS_ERR(master->core_clk))
> -		return PTR_ERR(master->core_clk);
> -
> -	master->pclk = devm_clk_get_optional_enabled(&pdev->dev, "pclk");
> -	if (IS_ERR(master->pclk))
> -		return PTR_ERR(master->pclk);
> +	if (has_acpi_companion(&pdev->dev)) {
> +		quirks = (unsigned long)device_get_match_data(&pdev->dev);

[Severity: High]
This is a pre-existing issue, but can this cast cause a type confusion if an
ACPI device uses the PRP0001 HID to bind via OF-style matching?

In that scenario, has_acpi_companion() evaluates to true, but
device_get_match_data() returns a pointer to the OF match data (like struct
dw_i3c_drvdata). Casting this pointer directly to an unsigned long would
interpret the memory address as a quirk bitmask, potentially enabling quirks
randomly depending on the kernel's memory layout.

> +	} else if (pdev->dev.of_node) {
> +		drvdata = device_get_match_data(&pdev->dev);
> +		if (drvdata)
> +			quirks = drvdata->flags;
> +	}
> +	master->quirks = quirks;
>  
> -	master->core_rst = devm_reset_control_get_optional_exclusive_deasserted(&pdev->dev,
> -										"core_rst");
> -	if (IS_ERR(master->core_rst))
> -		return PTR_ERR(master->core_rst);
> +	if (master->quirks & DW_I3C_ACPI_SKIP_CLK_RST) {
> +		master->core_clk = NULL;
> +		master->pclk = NULL;
> +		master->core_rst = NULL;
> +	} else {
> +		master->core_clk = devm_clk_get_enabled(&pdev->dev, NULL);

[Severity: High]
This is a pre-existing issue, but does using devm_clk_get_enabled() risk a
double-disable during driver unbind if the device is runtime suspended?

If the device is in the RPM_SUSPENDED state when dw_i3c_common_remove() runs,
the clock has already been disabled by the runtime_suspend callback. Because
the remove function calls pm_runtime_disable() without first waking the
device up via pm_runtime_get_sync(), the subsequent automatic devres cleanup
will call clk_disable_unprepare() a second time on the already disabled clock.

> +		if (IS_ERR(master->core_clk))
> +			return PTR_ERR(master->core_clk);
> +
> +		master->pclk = devm_clk_get_optional_enabled(&pdev->dev, "pclk");
> +		if (IS_ERR(master->pclk))
> +			return PTR_ERR(master->pclk);
> +
> +		master->core_rst = devm_reset_control_get_optional_exclusive_deasserted(&pdev->dev,
> +											"core_rst");
> +		if (IS_ERR(master->core_rst))
> +			return PTR_ERR(master->core_rst);
> +	}
>  
>  	spin_lock_init(&master->xferqueue.lock);
>  	INIT_LIST_HEAD(&master->xferqueue.list);

-- 
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