Re: [PATCH v4 3/3] i3c: dw: Add apb reset support

[sashiko-bot@kernel.org]

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

New issues:
- [High] Asymmetric reset assertion/deassertion order for APB and Core resets violates hardware sequencing rules.

Pre-existing issues:
- [High] Missing error handling for clock enablement and reset deassertion in runtime resume leads to potential kernel panics.
--

commit f0e43b6549a854c56bb22e329a26e4ad0bcd7e1c
Author: Jisheng Zhang <jszhang@kernel.org>
Subject: i3c: dw: Add apb reset support

This commit adds support for an APB interface reset in the DesignWare I3C
master driver. It fetches the optional reset during probe and updates the
runtime power management callbacks to assert and deassert it alongside the
core reset.

> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index a7593d6efac5a..03efd46db8d0c 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
[ ... ]
> @@ -1784,6 +1789,7 @@ static int __maybe_unused dw_i3c_master_runtime_suspend(struct device *dev)
>  	dw_i3c_master_disable(master);
>  
>  	reset_control_assert(master->core_rst);
> +	reset_control_assert(master->apb_rst);
>  	dw_i3c_master_disable_clks(master);
>  	pinctrl_pm_select_sleep_state(dev);
>  	return 0;
> @@ -1796,6 +1802,7 @@ static int __maybe_unused dw_i3c_master_runtime_resume(struct device *dev)
>  	pinctrl_pm_select_default_state(dev);
>  	dw_i3c_master_enable_clks(master);
>  	reset_control_deassert(master->core_rst);
> +	reset_control_deassert(master->apb_rst);

[Severity: High]
Should the reset deassertion order in resume be reversed to follow standard
hardware sequencing?

Currently, the resets are asserted in the suspend path as core_rst followed
by apb_rst, and they are deasserted in the resume path in the exact same
order. Standard practice usually requires a last-in, first-out sequence.

Additionally, dw_i3c_common_probe() requests these resets in core_rst then
apb_rst order. This means the automatic devres teardown will assert them as
apb_rst then core_rst, which is the opposite of the manual runtime suspend
logic introduced here.

Could this asymmetric sequencing cause bus hangs or IP initialization
failures if a core is released from reset while its APB bus interface
remains in reset?

[Severity: High]
This is a pre-existing issue, but this patch extends the pattern to the newly
added apb_rst. Are we missing error handling for the clock enablement and
reset deassertion?

The return values from dw_i3c_master_enable_clks() and both calls to
reset_control_deassert() are being discarded. If any of these operations
fail, the driver will proceed to configure the hardware by calling
dw_i3c_master_set_intr_regs().

Would it be safer to check these return values and return an error code to
the PM framework to avoid potential synchronous external aborts when
accessing inactive IP registers?

>  
>  	dw_i3c_master_set_intr_regs(master);
>  	dw_i3c_master_restore_timing_regs(master);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525140018.19598-1-jszhang@kernel.org?part=3