Re: [PATCH v2 2/2] i3c: master: dw: Drop redundant core reset name

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

Pre-existing issues:
- [Medium] Missing device wakeup in the remove path causes unbalanced devres cleanup (double clock disable and double reset assert) if the device is in runtime suspend.
--

--- Patch [2]: [PATCH v2 2/2] i3c: master: dw: Drop redundant core reset name ---
commit 6e023481fe0a3988544e2173709b9f994efee394
Author: Shubham Patil <[email protected]>

i3c: master: dw: Drop redundant core reset name

The DesignWare I3C master has a single reset line, so a dedicated
reset name is redundant. Look up the reset by index by passing NULL
instead of the "core_rst" name.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 2f8c0c4683e06..488fb455d3cc8 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -1584,7 +1584,7 @@ int dw_i3c_common_probe(struct dw_i3c_master *master,
>  		return PTR_ERR(master->pclk);
>  
>  	master->core_rst = devm_reset_control_get_optional_exclusive_deasserted(&pdev->dev,
> -										"core_rst");
> +										NULL);

[Severity: Medium]
This is a pre-existing issue, but touching this devm allocation raises a
question about how devres cleans up during driver unbind.

Since devm_reset_control_get_optional_exclusive_deasserted() and the earlier
devm_clk_get_enabled() register devres actions to assert resets and disable
clocks, does dw_i3c_common_remove() need to wake the device before teardown?

If the device is runtime suspended during driver unbind,
dw_i3c_master_runtime_suspend() has already disabled clocks and asserted the
reset. Because dw_i3c_common_remove() does not call pm_runtime_get_sync() to
resume the device:

void dw_i3c_common_remove(struct dw_i3c_master *master)
{
	i3c_master_unregister(&master->base);

	/* Balance pm_runtime_get_noresume() from probe() */
	if (master->quirks & DW_I3C_DISABLE_RUNTIME_PM_QUIRK)
		pm_runtime_put_noidle(master->dev);

	pm_runtime_disable(master->dev);
	pm_runtime_set_suspended(master->dev);
	pm_runtime_dont_use_autosuspend(master->dev);
}

The automatic devres cleanup will attempt to disable already-disabled clocks
and assert an already-asserted reset, which could trigger a noisy WARN from
the Common Clock Framework.

Could this double-cleanup be avoided by resuming the device before disabling
runtime PM in dw_i3c_common_remove()?

>  	if (IS_ERR(master->core_rst))
>  		return PTR_ERR(master->core_rst);
>

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

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