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

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

[kr494167]

From: Surendra Singh Chouhan <kr494167@gmail.com>

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),
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). 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@gmail.com>
---
 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;
+
+	signal = &sky1src->signals[id];
+	if (!signal->offset && !signal->bit)
+		return -EINVAL;
+
+	value = assert ? 0 : signal->bit;
 
 	return regmap_update_bits(sky1src->regmap,
 				  signal->offset, signal->bit, value);
@@ -277,7 +286,12 @@ static int sky1_reset_set(struct reset_controller_dev *rcdev,
 static int sky1_reset_assert(struct reset_controller_dev *rcdev,
 			     unsigned long id)
 {
-	sky1_reset_set(rcdev, id, true);
+	int ret;
+
+	ret = sky1_reset_set(rcdev, id, true);
+	if (ret)
+		return ret;
+
 	usleep_range(SKY1_RESET_SLEEP_MIN_US,
 		     SKY1_RESET_SLEEP_MAX_US);
 	return 0;
@@ -286,7 +300,12 @@ static int sky1_reset_assert(struct reset_controller_dev *rcdev,
 static int sky1_reset_deassert(struct reset_controller_dev *rcdev,
 			       unsigned long id)
 {
-	sky1_reset_set(rcdev, id, false);
+	int ret;
+
+	ret = sky1_reset_set(rcdev, id, false);
+	if (ret)
+		return ret;
+
 	usleep_range(SKY1_RESET_SLEEP_MIN_US,
 		     SKY1_RESET_SLEEP_MAX_US);
 	return 0;
@@ -295,19 +314,34 @@ static int sky1_reset_deassert(struct reset_controller_dev *rcdev,
 static int sky1_reset(struct reset_controller_dev *rcdev,
 		      unsigned long id)
 {
-	sky1_reset_assert(rcdev, id);
-	sky1_reset_deassert(rcdev, id);
-	return 0;
+	int ret;
+
+	ret = sky1_reset_assert(rcdev, id);
+	if (ret)
+		return ret;
+
+	return sky1_reset_deassert(rcdev, id);
 }
 
 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;
+
+	ret = regmap_read(sky1src->regmap, signal->offset, &value);
+	if (ret)
+		return ret;
 
-	regmap_read(sky1src->regmap, signal->offset, &value);
 	return !(value & signal->bit);
 }
 
-- 
2.55.0

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

[Philipp Zabel]

On Fr, 2026-07-24 at 10:12 +0530, kr494167@gmail.com wrote:
> From: Surendra Singh Chouhan <kr494167@gmail.com>
> 
> 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@gmail.com>
> ---
>  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