Re: [PATCH v4 2/5] reset: cix: add audss support to sky1 reset driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Using asynchronous `pm_runtime_put()` on the probe error path cancels the pending suspend, leaking the active power state.
- [High] Premature registration of the reset controller creates a race condition that can lead to bus faults or use-after-free.
- [High] The return value of `reset_control_deassert()` is ignored.
- [Low] Unconditional assignment of the `.pm` ops pointer generates dead code when PM is disabled.
--

commit 22eea5c604734dfc2f4c075cfefb3354c7756088
Author: Joakim Zhang <joakim.zhang@cixtech.com>

reset: cix: add audss support to sky1 reset driver

Extend the Sky1 reset controller driver for the AUDSS CRU syscon. The
AUDSS block provides sixteen active-low software reset bits in one
register for audio subsystem peripherals.

> diff --git a/drivers/reset/reset-sky1.c b/drivers/reset/reset-sky1.c
> index 78e80a533c399..462501c2ea066 100644
> --- a/drivers/reset/reset-sky1.c
> +++ b/drivers/reset/reset-sky1.c
[ ... ]
> @@ -343,12 +393,36 @@ static int sky1_reset_probe(struct platform_device *pdev)
>  	sky1src->rcdev.of_node   = dev->of_node;
>  	sky1src->rcdev.dev       = dev;
>  
> -	return devm_reset_controller_register(dev, &sky1src->rcdev);
> +	ret = devm_reset_controller_register(dev, &sky1src->rcdev);
> +	if (ret)
> +		return ret;
> +

[Severity: High]
Does registering the reset controller this early in sky1_reset_probe()
expose it to the system before hardware and power management are fully
initialized?

If a concurrent consumer device attempts to trigger a reset before the
NOC reset is acquired and power management is enabled, could this
execute register operations against unpowered or uninitialized hardware?

If devm_reset_control_get_exclusive() later returns -EPROBE_DEFER, the
controller would be abruptly unregistered via devres while potentially
in use.

Could devm_reset_controller_register() be called as the final step in
probe() after all initialization is complete?

> +	platform_set_drvdata(pdev, sky1src);
> +
> +	if (of_device_is_compatible(dev->of_node, "cix,sky1-audss-system-control")) {
> +		sky1src->rst_noc = devm_reset_control_get_exclusive(dev, NULL);
> +		if (IS_ERR(sky1src->rst_noc))
> +			return dev_err_probe(dev, PTR_ERR(sky1src->rst_noc),
> +					     "failed to get audss noc reset");
> +
> +		pm_runtime_get_noresume(dev);
> +		pm_runtime_set_active(dev);
> +		devm_pm_runtime_enable(dev);
> +
> +		reset_control_deassert(sky1src->rst_noc);

[Severity: High]
Does this ignore the integer return value from reset_control_deassert()?

If deassertion fails, the driver proceeds to call
devm_of_platform_populate(). Child devices could probe and attempt to
access their registers while the hardware is still stuck in reset.

Could we validate the return value of reset_control_deassert() before
proceeding?

> +
> +		ret = devm_of_platform_populate(dev);
> +		pm_runtime_put(dev);
> +		return ret;

[Severity: High]
If devm_of_platform_populate() fails, this uses the asynchronous
pm_runtime_put() on the error path.

Since devm_pm_runtime_enable() was used, returning an error triggers
devres cleanup which executes pm_runtime_disable(). This cancels any
pending asynchronous idle/suspend work.

Because of this cancellation, sky1_reset_runtime_suspend() would never
execute, leaving the NOC reset permanently leaked in a deasserted state.

Should this use pm_runtime_put_sync() instead to guarantee synchronous
suspend before PM is disabled?

> +	}
> +
> +	return 0;
>  }
[ ... ]
> @@ -358,6 +432,12 @@ static struct platform_driver sky1_reset_driver = {
>  	.driver = {
>  		.name		= "cix,sky1-rst",
>  		.of_match_table = sky1_sysreg_of_match,
> +		.pm		= &(const struct dev_pm_ops){
> +			SET_RUNTIME_PM_OPS(sky1_reset_runtime_suspend,
> +					   sky1_reset_runtime_resume, NULL)
> +			SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +						pm_runtime_force_resume)
> +		},

[Severity: Low]
Does this unconditional assignment of the .pm pointer generate dead code
when CONFIG_PM is disabled?

The structure and statically referenced callbacks get compiled into the
kernel even though they can never execute.

Could this use the pm_ptr() wrapper macro to allow the compiler to
eliminate the unused structure?

>  	},
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617060437.1474816-1-joakim.zhang@cixtech.com?part=2