Re: [PATCH v7 3/4] reset: cix: add sky1 audss auxiliary reset driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing PM runtime management causes system hangs or failures when accessing reset registers while the parent CRU is suspended.
--

commit 9e37ffe1b0a7e10201a5f5d87d450b05d374b858
Author: Joakim Zhang <joakim.zhang@cixtech.com>

reset: cix: add sky1 audss auxiliary reset driver

This commit adds an auxiliary reset controller driver for the AUDSS CRU. It
manages sixteen software reset lines for audio subsystem peripherals via a
single register in the CRU register map.

> diff --git a/drivers/reset/reset-sky1-audss.c b/drivers/reset/reset-sky1-audss.c
> new file mode 100644
> index 0000000000000..d31d80e1251a8
> --- /dev/null
> +++ b/drivers/reset/reset-sky1-audss.c

[ ... ]

> +static int sky1_audss_reset_set(struct reset_controller_dev *rcdev,
> +				unsigned long id, bool assert)
> +{
> +	struct sky1_audss_reset *priv = to_sky1_audss_reset(rcdev);
> +	const struct sky1_audss_reset_map *signal = &priv->map[id];
> +	unsigned int value = assert ? 0 : signal->mask;
> +
> +	return regmap_update_bits(priv->regmap, signal->offset, signal->mask, value);

[Severity: High]
Can this cause a synchronous external abort or bus hang if the parent CRU is
currently suspended?

It appears sky1_audss_reset_set() accesses the parent's regmap without
acquiring a PM runtime reference to ensure the CRU is awake. If a consumer
device calls reset_control_assert() or reset_control_deassert() while the
CRU is in runtime suspend, this MMIO access will hit a powered-down block.

> +}

[ ... ]

> +static int sky1_audss_reset_deassert(struct reset_controller_dev *rcdev,
> +				     unsigned long id)
> +{
> +	int ret;
> +
> +	ret = sky1_audss_reset_set(rcdev, id, false);

[Severity: High]
Because the driver does not keep a PM runtime reference while the line is
deasserted, could the parent CRU suspend and lose the deasserted state?

If a consumer deasserts its reset and then allows the subsystem to idle, the
CRU could suspend. This would reset the block and silently lose the state of
the AUDSS_SW_RST register when the CRU wakes back up.

> +	if (ret)
> +		return ret;
> +
> +	usleep_range(SKY1_RESET_SLEEP_MIN_US, SKY1_RESET_SLEEP_MAX_US);
> +	return 0;
> +}

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