Re: [PATCH] reset: sky1: propagate regmap errors and validate signals

[Philipp Zabel]

On Fr, 2026-07-24 at 10:12 +0530, kr494167@xxxxxxxxx wrote:
> From: Surendra Singh Chouhan <kr494167@xxxxxxxxx>
> 
> sky1_reset_assert(), sky1_reset_deassert(), sky1_reset(), and
> sky1_reset_status() ignored the return values of sky1_reset_set() and
> regmap_read(). If regmap operations failed (e.g. due to bus I/O errors),

Which bus I/O errors cause error return values?
The regmap behind this is MMIO.

> error codes were swallowed and functions returned success (0) or incorrect
> status (1).
> 
> In addition, sky1_src_signals is a sparse array containing unmapped entries
> (offset == 0 && bit == 0).

This is not true. Please point out the alleged unmapped entries.

> Out-of-bounds id lookups or references to
> unmapped entries were not validated before performing regmap operations.
> 
> Fix these by capturing and propagating regmap read/write errors across
> all ops, and adding bounds and unmapped signal validation in
> sky1_reset_set() and sky1_reset_status().
> 
> Fixes: 7cd3ca90d921 ("reset: add Sky1 soc reset support")
> Signed-off-by: Surendra Singh Chouhan <kr494167@xxxxxxxxx>
> ---
>  drivers/reset/reset-sky1.c | 54 +++++++++++++++++++++++++++++++-------
>  1 file changed, 44 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/reset/reset-sky1.c b/drivers/reset/reset-sky1.c
> index 78e80a533c39..885eb9563ef8 100644
> --- a/drivers/reset/reset-sky1.c
> +++ b/drivers/reset/reset-sky1.c
> @@ -267,8 +267,17 @@ static int sky1_reset_set(struct reset_controller_dev *rcdev,
>  			  unsigned long id, bool assert)
>  {
>  	struct sky1_src *sky1src = to_sky1_src(rcdev);
> -	const struct sky1_src_signal *signal = &sky1src->signals[id];
> -	unsigned int value = assert ? 0 : signal->bit;
> +	const struct sky1_src_signal *signal;
> +	unsigned int value;
> +
> +	if (id >= sky1src->rcdev.nr_resets)
> +		return -EINVAL;

This is unnecessary. The id field of struct reset_control, where this
id comes from, is set up after checking exactly this in
fwnode_reset_simple_xlate(). 

> +
> +	signal = &sky1src->signals[id];
> +	if (!signal->offset && !signal->bit)
> +		return -EINVAL;

This is unnecessary. All array entries should contain a bit value.

> +
> +	value = assert ? 0 : signal->bit;
>  
>  	return regmap_update_bits(sky1src->regmap,
>  				  signal->offset, signal->bit, value);
> 
[...]
>  static int sky1_reset_status(struct reset_controller_dev *rcdev,
>  			     unsigned long id)
>  {
> -	unsigned int value = 0;
>  	struct sky1_src *sky1src = to_sky1_src(rcdev);
> -	const struct sky1_src_signal *signal = &sky1src->signals[id];
> +	const struct sky1_src_signal *signal;
> +	unsigned int value = 0;
> +	int ret;
> +
> +	if (id >= sky1src->rcdev.nr_resets)
> +		return -EINVAL;
> +
> +	signal = &sky1src->signals[id];
> +	if (!signal->offset && !signal->bit)
> +		return -EINVAL;

Both unnecessary, see above.

> +
> +	ret = regmap_read(sky1src->regmap, signal->offset, &value);
> +	if (ret)
> +		return ret;
>  
> -	regmap_read(sky1src->regmap, signal->offset, &value);
>  	return !(value & signal->bit);
>  }
>  

regards
Philipp