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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The reset status callback returns inverted logic compared to the active-low hardware state.
- [High] The `sky1_audss_reset_status()` callback ignores the return value of `regmap_read()`, leading to the use of an uninitialized variable on read failures.
- [High] Hardware update failures are silently ignored in the `assert`, `deassert`, and `reset` callbacks.
- [High] Reset operations access hardware registers without holding a PM runtime reference to the parent device.
--

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

reset: cix: add sky1 audss auxiliary reset driver
    
Add an auxiliary reset controller driver for the AUDSS CRU. Sixteen
software reset lines for audio subsystem peripherals are controlled
through one 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..f29fe3554ec17
> --- /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]
Does this regmap access need to be protected by holding a runtime PM
reference on the parent device? If the parent device implements runtime PM,
can it suspend and disable clocks while a consumer calls this reset function,
potentially causing a bus hang?

> +
> +static int sky1_audss_reset_assert(struct reset_controller_dev *rcdev,
> +				   unsigned long id)
> +{
> +	sky1_audss_reset_set(rcdev, id, true);

[Severity: High]
Is it intentional to discard the error code returned by sky1_audss_reset_set()?
If the underlying regmap_update_bits() fails, this will unconditionally report
success.

> +	usleep_range(SKY1_RESET_SLEEP_MIN_US, SKY1_RESET_SLEEP_MAX_US);
> +	return 0;
> +}
> +
> +static int sky1_audss_reset_deassert(struct reset_controller_dev *rcdev,
> +				     unsigned long id)
> +{
> +	sky1_audss_reset_set(rcdev, id, false);

[Severity: High]
Should this also propagate the return value of sky1_audss_reset_set() instead
of ignoring potential hardware update failures?

> +	usleep_range(SKY1_RESET_SLEEP_MIN_US, SKY1_RESET_SLEEP_MAX_US);
> +	return 0;
> +}
> +
> +static int sky1_audss_reset(struct reset_controller_dev *rcdev,
> +			    unsigned long id)
> +{
> +	sky1_audss_reset_assert(rcdev, id);
> +	sky1_audss_reset_deassert(rcdev, id);

[Severity: High]
Does this code need to capture and propagate errors from the assert and
deassert steps? It seems hardware faults might be masked from caller drivers.

> +	return 0;
> +}
> +
> +static int sky1_audss_reset_status(struct reset_controller_dev *rcdev,
> +				   unsigned long id)
> +{
> +	struct sky1_audss_reset *priv = to_sky1_audss_reset(rcdev);
> +	const struct sky1_audss_reset_map *signal = &priv->map[id];
> +	unsigned int value;
> +
> +	regmap_read(priv->regmap, signal->offset, &value);

[Severity: High]
If regmap_read() fails, it leaves value uninitialized. Should this check
the return value of regmap_read() and return the error code to avoid using
uninitialized stack memory?

Also, similar to sky1_audss_reset_set(), does this register access need
pm_runtime_get_sync(dev->parent) to prevent reading from a suspended hardware
block?

> +	return !!(value & signal->mask);

[Severity: High]
Since sky1_audss_reset_set() writes 0 to assert the reset line, the hardware
appears to use an active-low scheme. However, this returns 0 when the bit is
cleared. The reset controller framework requires the status callback to return
a positive value when asserted. Will this report inverted states to consumer
drivers?

> +}

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