All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] rtc: spacemit: handle regmap_test_bits() error return
@ 2026-07-24 13:58 ` kr494167
  0 siblings, 0 replies; 4+ messages in thread
From: kr494167 @ 2026-07-24 13:58 UTC (permalink / raw)
  To: alexandre.belloni, dlan, elder
  Cc: linux-rtc, linux-riscv, spacemit, linux-kernel,
	Surendra Singh Chouhan

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

p1_rtc_read_time() called if (!regmap_test_bits(regmap, RTC_CTRL, RTC_EN))
to check if the RTC was enabled.

regmap_test_bits() returns 1 if the bit is set, 0 if not set, and a
negative error code (e.g. -EIO) if reading the control register fails.
Using !regmap_test_bits(...) evaluates a negative error code as boolean
false, causing I2C/regmap read failures to be ignored and incorrectly
proceeding to read time registers from a failing device.

Fix this by capturing the return value of regmap_test_bits() and returning
the error code if negative, or -EINVAL if the RTC is disabled.

Fixes: a6de182daa2b ("rtc: spacemit: support the SpacemiT P1 RTC")
Reviewed-by: Alex Elder <elder@riscstar.com>
Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>
---
v2:
 - Use ternary shorthand (ret ?: -EINVAL) as suggested by Alex Elder.
 - Collect Reviewed-by tag from Alex Elder.

 drivers/rtc/rtc-spacemit-p1.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-spacemit-p1.c b/drivers/rtc/rtc-spacemit-p1.c
index 43ab62494bb4..1de7bd995d29 100644
--- a/drivers/rtc/rtc-spacemit-p1.c
+++ b/drivers/rtc/rtc-spacemit-p1.c
@@ -57,8 +57,9 @@ static int p1_rtc_read_time(struct device *dev, struct rtc_time *t)
 	u8 time[6];
 	int ret;
 
-	if (!regmap_test_bits(regmap, RTC_CTRL, RTC_EN))
-		return -EINVAL;		/* RTC is disabled */
+	ret = regmap_test_bits(regmap, RTC_CTRL, RTC_EN);
+	if (ret <= 0)
+		return ret ?: -EINVAL;	/* RTC is disabled or error */
 
 	ret = regmap_bulk_read(regmap, RTC_TIME, time, sizeof(time));
 	if (ret)
-- 
2.55.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* [PATCH v2] rtc: spacemit: handle regmap_test_bits() error return
@ 2026-07-24 13:58 ` kr494167
  0 siblings, 0 replies; 4+ messages in thread
From: kr494167 @ 2026-07-24 13:58 UTC (permalink / raw)
  To: alexandre.belloni, dlan, elder
  Cc: linux-rtc, linux-riscv, spacemit, linux-kernel,
	Surendra Singh Chouhan

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

p1_rtc_read_time() called if (!regmap_test_bits(regmap, RTC_CTRL, RTC_EN))
to check if the RTC was enabled.

regmap_test_bits() returns 1 if the bit is set, 0 if not set, and a
negative error code (e.g. -EIO) if reading the control register fails.
Using !regmap_test_bits(...) evaluates a negative error code as boolean
false, causing I2C/regmap read failures to be ignored and incorrectly
proceeding to read time registers from a failing device.

Fix this by capturing the return value of regmap_test_bits() and returning
the error code if negative, or -EINVAL if the RTC is disabled.

Fixes: a6de182daa2b ("rtc: spacemit: support the SpacemiT P1 RTC")
Reviewed-by: Alex Elder <elder@riscstar.com>
Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>
---
v2:
 - Use ternary shorthand (ret ?: -EINVAL) as suggested by Alex Elder.
 - Collect Reviewed-by tag from Alex Elder.

 drivers/rtc/rtc-spacemit-p1.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-spacemit-p1.c b/drivers/rtc/rtc-spacemit-p1.c
index 43ab62494bb4..1de7bd995d29 100644
--- a/drivers/rtc/rtc-spacemit-p1.c
+++ b/drivers/rtc/rtc-spacemit-p1.c
@@ -57,8 +57,9 @@ static int p1_rtc_read_time(struct device *dev, struct rtc_time *t)
 	u8 time[6];
 	int ret;
 
-	if (!regmap_test_bits(regmap, RTC_CTRL, RTC_EN))
-		return -EINVAL;		/* RTC is disabled */
+	ret = regmap_test_bits(regmap, RTC_CTRL, RTC_EN);
+	if (ret <= 0)
+		return ret ?: -EINVAL;	/* RTC is disabled or error */
 
 	ret = regmap_bulk_read(regmap, RTC_TIME, time, sizeof(time));
 	if (ret)
-- 
2.55.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] rtc: spacemit: handle regmap_test_bits() error return
  2026-07-24 13:58 ` kr494167
  (?)
@ 2026-07-24 14:06 ` sashiko-bot
  2026-07-24 14:17   ` Alexandre Belloni
  -1 siblings, 1 reply; 4+ messages in thread
From: sashiko-bot @ 2026-07-24 14:06 UTC (permalink / raw)
  To: kr494167; +Cc: Alexandre Belloni, linux-rtc

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The RTC time read retry loop in `p1_rtc_read_time` is vulnerable to a torn read (time corruption) if a time roll-over occurs during the register read.
--

--- Patch [1]: [PATCH v2] rtc: spacemit: handle regmap_test_bits() error return ---
commit a044092d776fab1e96ae2f8b48ace64aa0ab2325
Author: Surendra Singh Chouhan <kr494167@gmail.com>

rtc: spacemit: handle regmap_test_bits() error return

p1_rtc_read_time() called if (!regmap_test_bits(regmap, RTC_CTRL, RTC_EN))
to check if the RTC was enabled. This evaluates negative error codes as
boolean false, causing read failures to be ignored. Fix this by capturing
the return value of regmap_test_bits() and returning the error code.

> diff --git a/drivers/rtc/rtc-spacemit-p1.c b/drivers/rtc/rtc-spacemit-p1.c
> index 43ab62494bb4b..1de7bd995d296 100644
> --- a/drivers/rtc/rtc-spacemit-p1.c
> +++ b/drivers/rtc/rtc-spacemit-p1.c
> @@ -57,8 +57,9 @@ static int p1_rtc_read_time(struct device *dev, struct rtc_time *t)
>  	u8 time[6];
>  	int ret;
>  
> -	if (!regmap_test_bits(regmap, RTC_CTRL, RTC_EN))
> -		return -EINVAL;		/* RTC is disabled */
> +	ret = regmap_test_bits(regmap, RTC_CTRL, RTC_EN);
> +	if (ret <= 0)
> +		return ret ?: -EINVAL;	/* RTC is disabled or error */
>  
>  	ret = regmap_bulk_read(regmap, RTC_TIME, time, sizeof(time));
>  	if (ret)
>  		return ret;
>  
>  	do {
>  		seconds = time[0];
>  		ret = regmap_bulk_read(regmap, RTC_TIME, time, sizeof(time));
>  		if (ret)
>  			return ret;
>  	} while (time[0] != seconds && --count);

[Severity: High]
This is a pre-existing issue, but does this loop correctly detect time
roll-overs that happen during the read?

If a time roll-over occurs during the second regmap_bulk_read() in
p1_rtc_read_time(), specifically after reading time[0] (seconds) but before
reading higher units, time[0] will match the seconds from the previous read.

This would cause the loop to terminate early and accept the newly read time
array.

Could this return a torn time from the RTC (for example, 14:00:59 instead of
13:59:59 or 14:00:00)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724135803.81223-1-kr494167@gmail.com?part=1

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] rtc: spacemit: handle regmap_test_bits() error return
  2026-07-24 14:06 ` sashiko-bot
@ 2026-07-24 14:17   ` Alexandre Belloni
  0 siblings, 0 replies; 4+ messages in thread
From: Alexandre Belloni @ 2026-07-24 14:17 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: kr494167, linux-rtc

On 24/07/2026 14:06:33+0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] The RTC time read retry loop in `p1_rtc_read_time` is vulnerable to a torn read (time corruption) if a time roll-over occurs during the register read.
> --
> 
> --- Patch [1]: [PATCH v2] rtc: spacemit: handle regmap_test_bits() error return ---
> commit a044092d776fab1e96ae2f8b48ace64aa0ab2325
> Author: Surendra Singh Chouhan <kr494167@gmail.com>
> 
> rtc: spacemit: handle regmap_test_bits() error return
> 
> p1_rtc_read_time() called if (!regmap_test_bits(regmap, RTC_CTRL, RTC_EN))
> to check if the RTC was enabled. This evaluates negative error codes as
> boolean false, causing read failures to be ignored. Fix this by capturing
> the return value of regmap_test_bits() and returning the error code.
> 
> > diff --git a/drivers/rtc/rtc-spacemit-p1.c b/drivers/rtc/rtc-spacemit-p1.c
> > index 43ab62494bb4b..1de7bd995d296 100644
> > --- a/drivers/rtc/rtc-spacemit-p1.c
> > +++ b/drivers/rtc/rtc-spacemit-p1.c
> > @@ -57,8 +57,9 @@ static int p1_rtc_read_time(struct device *dev, struct rtc_time *t)
> >  	u8 time[6];
> >  	int ret;
> >  
> > -	if (!regmap_test_bits(regmap, RTC_CTRL, RTC_EN))
> > -		return -EINVAL;		/* RTC is disabled */
> > +	ret = regmap_test_bits(regmap, RTC_CTRL, RTC_EN);
> > +	if (ret <= 0)
> > +		return ret ?: -EINVAL;	/* RTC is disabled or error */
> >  
> >  	ret = regmap_bulk_read(regmap, RTC_TIME, time, sizeof(time));
> >  	if (ret)
> >  		return ret;
> >  
> >  	do {
> >  		seconds = time[0];
> >  		ret = regmap_bulk_read(regmap, RTC_TIME, time, sizeof(time));
> >  		if (ret)
> >  			return ret;
> >  	} while (time[0] != seconds && --count);
> 
> [Severity: High]
> This is a pre-existing issue, but does this loop correctly detect time
> roll-overs that happen during the read?
> 
> If a time roll-over occurs during the second regmap_bulk_read() in
> p1_rtc_read_time(), specifically after reading time[0] (seconds) but before
> reading higher units, time[0] will match the seconds from the previous read.
> 
> This would cause the loop to terminate early and accept the newly read time
> array.
> 
> Could this return a torn time from the RTC (for example, 14:00:59 instead of
> 13:59:59 or 14:00:00)?
> 

While this is a valid concern, I guess this RTC latches the registers


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-07-24 14:17 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-07-24 13:58 [PATCH v2] rtc: spacemit: handle regmap_test_bits() error return kr494167
2026-07-24 13:58 ` kr494167
2026-07-24 14:06 ` sashiko-bot
2026-07-24 14:17   ` Alexandre Belloni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.