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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] The reset controller operations access hardware registers via regmap without ensuring the reset controller device is active, leading to potential system crashes.
- [High] The reset controller is registered before its underlying hardware (NOC reset) is fully initialized and deasserted, creating a race condition that can cause a bus hang.
- [High] The return value of `reset_control_deassert()` is unchecked, which can lead to silent hardware failures and subsequent bus hangs.
- [Medium] Using asynchronous `pm_runtime_put()` on the probe error path races with `pm_runtime_disable()`, leaving the hardware active and the reset improperly deasserted.
- [Low] The `dev_pm_ops` structure is assigned unconditionally without the `pm_ptr()` wrapper, generating dead code when PM support is disabled.
--

commit 44fbafef81df1536ade6d0fdca4767f4c7fc1234
Author: Joakim Zhang <joakim.zhang@cixtech.com>
Subject: reset: cix: add audss support to sky1 reset driver

This commit extends the Sky1 reset controller driver for the AUDSS CRU syscon.
It adds support for sixteen active-low software reset bits for audio subsystem
peripherals, reusing the existing regmap-based reset operations.

> diff --git a/drivers/reset/reset-sky1.c b/drivers/reset/reset-sky1.c
> --- a/drivers/reset/reset-sky1.c
> +++ b/drivers/reset/reset-sky1.c
[ ... ]
> @@ -343,21 +379,65 @@ 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);

[Severity: High]
Is it safe to register the reset controller globally before the AUDSS NoC
reset is deasserted? 

If a consumer driver attempting to probe or wake up concurrently tries to
assert or deassert a reset line, could the regmap MMIO access stall the bus
and trigger a synchronous external abort because the AUDSS NoC is still held
in reset?

> +	if (ret)
> +		return ret;
> +
> +	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]
Can reset_control_deassert() fail and return an error code? 

If it fails and leaves the NoC in reset, will devm_of_platform_populate()
attempting to probe children over a blocked NoC cause a bus stall or
system crash? Should the return value be checked and propagated here?

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

[Severity: Medium]
Does using asynchronous pm_runtime_put() on the error path risk a race with
pm_runtime_disable() which is called during devres cleanup? 

If devm_of_platform_populate() fails, the probe function returns an error and
devres will immediately disable PM runtime. This could cancel the pending
async suspend work, meaning sky1_reset_runtime_suspend() might be skipped,
leaking rst_noc in a deasserted state. 

Would using pm_runtime_put_sync() ensure the hardware is properly suspended
before cleanup continues?

> +		return ret;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static const struct dev_pm_ops sky1_reset_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: Critical]
Now that PM runtime support is added and the AUDSS variant's NoC reset can be
asserted during runtime suspend, do the existing reset controller operations
(sky1_reset_assert, sky1_reset_deassert, and sky1_reset_status) need to call
pm_runtime_resume_and_get() before performing regmap accesses?

If a consumer driver toggles a reset line while this controller is runtime
suspended, will the raw MMIO write to a block held in NOC reset cause a
synchronous external abort?

> +
>  static struct platform_driver sky1_reset_driver = {
>  	.probe	= sky1_reset_probe,
>  	.driver = {
>  		.name		= "cix,sky1-rst",
>  		.of_match_table = sky1_sysreg_of_match,
> +		.pm		= &sky1_reset_pm_ops,

[Severity: Low]
If CONFIG_PM is disabled, will this unconditional assignment cause
sky1_reset_pm_ops and its callbacks to be compiled but unreachable,
generating dead code? 

Should this assignment be wrapped with pm_ptr() to align with standard
Power Management subsystem guidelines?

>  	},
>  };

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