[PATCH] rtc: spacemit: handle regmap_test_bits() error return

[PATCH] rtc: spacemit: handle regmap_test_bits() error return

[kr494167]

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")
Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>
---
 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..c1820c920ba3 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 ? ret : -EINVAL;	/* RTC is disabled or error */
 
 	ret = regmap_bulk_read(regmap, RTC_TIME, time, sizeof(time));
 	if (ret)
-- 
2.55.0

Re: [PATCH] rtc: spacemit: handle regmap_test_bits() error return

[sashiko-bot]

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

Pre-existing issues:
- [High] The anti-tearing retry loop in `p1_rtc_read_time()` is flawed and can silently accept corrupted time readings because it only compares the seconds byte between reads.
--

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

rtc: spacemit: handle regmap_test_bits() error return

This commit fixes the error handling when reading the RTC control register.
It ensures that if regmap_test_bits() returns a negative error code, that
error is propagated instead of being incorrectly treated as boolean false.

> diff --git a/drivers/rtc/rtc-spacemit-p1.c b/drivers/rtc/rtc-spacemit-p1.c
> index 43ab62494bb4b..c1820c920ba3d 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 ? ret : -EINVAL;	/* RTC is disabled or error */
>  
>  	ret = regmap_bulk_read(regmap, RTC_TIME, time, sizeof(time));
>  	if (ret)

[Severity: High]
This is a pre-existing issue, but does the anti-tearing retry loop in
p1_rtc_read_time() fully protect against corrupted time readings?

Looking just below this diff in p1_rtc_read_time(), the loop checks for time
tearing by only comparing the seconds byte:

	do {
		seconds = time[0];
		ret = regmap_bulk_read(regmap, RTC_TIME, time, sizeof(time));
		if (ret)
			return ret;
	} while (time[0] != seconds && --count);

If a hardware time rollover occurs exactly after the seconds byte (time[0])
is read during the second regmap_bulk_read(), the read could return the
pre-rollover seconds but post-rollover minutes, hours, or days. 

Since the seconds byte matches the previous read, the loop would incorrectly
terminate and accept the torn time array. Can this lead to corrupted time
being returned to the RTC subsystem?

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

Re: [PATCH] rtc: spacemit: handle regmap_test_bits() error return

[Alex Elder]

On 7/23/26 9:42 PM, kr494167@gmail.com wrote:
> 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")
> Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>

This is an I2C-based regmap, so yes, we do need to test
for errors when reading or writing registers.

I think this is a good fix.  I have a minor suggestion or
two below but either way:

Reviewed-by: Alex Elder <elder@riscstar.com>

> ---
>   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..c1820c920ba3 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 ? ret : -EINVAL;	/* RTC is disabled or error */

Since regmap_test_bits() can return -EINVAL, I wonder if a
different error code would be better to indicate "disabled".
It can return -EBUSY and -EIO too, as well as anything any
regmap->reg_read() can return.  Scanning through the list of
basic error codes I don't have a good suggestion, unfortunately.

Otherwise, perhaps just:

		return ret ? : -EINVAL;  /* comment if you like */

>   
>   	ret = regmap_bulk_read(regmap, RTC_TIME, time, sizeof(time));
>   	if (ret)

Re: [PATCH] rtc: spacemit: handle regmap_test_bits() error return

[Alexandre Belloni]

On 24/07/2026 08:32:13-0500, Alex Elder wrote:
> On 7/23/26 9:42 PM, kr494167@gmail.com wrote:
> > 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")
> > Signed-off-by: Surendra Singh Chouhan <kr494167@gmail.com>
> 
> This is an I2C-based regmap, so yes, we do need to test
> for errors when reading or writing registers.
> 
> I think this is a good fix.  I have a minor suggestion or
> two below but either way:
> 
> Reviewed-by: Alex Elder <elder@riscstar.com>
> 
> > ---
> >   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..c1820c920ba3 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 ? ret : -EINVAL;	/* RTC is disabled or error */
> 
> Since regmap_test_bits() can return -EINVAL, I wonder if a
> different error code would be better to indicate "disabled".
> It can return -EBUSY and -EIO too, as well as anything any
> regmap->reg_read() can return.  Scanning through the list of
> basic error codes I don't have a good suggestion, unfortunately.
> 

No, EINVAL is standard across all the drivers. I considered changing
this to ETIME or ENODATA but that would break userspace too much.

> Otherwise, perhaps just:
> 
> 		return ret ? : -EINVAL;  /* comment if you like */
> 
> >   	ret = regmap_bulk_read(regmap, RTC_TIME, time, sizeof(time));
> >   	if (ret)
> 

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