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

On 24/07/2026 14:06:33+0000, [email protected] 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 <[email protected]>
> 
> 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