[v10,03/18] rtc: sun6i: Fix time overflow handling

Using "unsigned long" for UNIX timestamps is never a good idea, and
comparing the value of such a variable against U32_MAX does not do
anything useful on 32-bit systems.

Use the proper time64_t type when dealing with timestamps, and avoid
cutting down the time range unnecessarily. This also fixes the flawed
check for the alarm time being too far into the future.

The check for this condition is actually somewhat theoretical, as the
RTC counts till 2033 only anyways, and 2^32 seconds from now is not
before the year 2157 - at which point I hope nobody will be using this
hardware anymore.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/rtc/rtc-sun6i.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

On Fri, 11 Feb 2022 12:26:28 +0000
Andre Przywara <andre.przywara@arm.com> wrote:

Hi Alessandro, Alexandre,

I was wondering if you would consider taking this (as a fix)?
This (time_gap > U32_MAX) comparison looks flawed by design, and we should
use time_t these days anyway.

Also, do you have an opinion on the other RTC patches? The linear day
patch (v10 04/18)[1] and the broken-down alarm registers (v10 05/18)[2]
were on the list for a while now and are needed by other SoCs as well
(R329[3] and the RISC-V D1).

Cheers,
Andre
[1] https://lore.kernel.org/linux-arm-kernel/20220211122643.1343315-5-andre.przywara@arm.com/
[2] https://lore.kernel.org/linux-arm-kernel/20220211122643.1343315-6-andre.przywara@arm.com/
[3] https://lore.kernel.org/linux-arm-kernel/20210802062212.73220-3-icenowy@sipeed.com/

> Using "unsigned long" for UNIX timestamps is never a good idea, and
> comparing the value of such a variable against U32_MAX does not do
> anything useful on 32-bit systems.
> 
> Use the proper time64_t type when dealing with timestamps, and avoid
> cutting down the time range unnecessarily. This also fixes the flawed
> check for the alarm time being too far into the future.
> 
> The check for this condition is actually somewhat theoretical, as the
> RTC counts till 2033 only anyways, and 2^32 seconds from now is not
> before the year 2157 - at which point I hope nobody will be using this
> hardware anymore.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/rtc/rtc-sun6i.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-sun6i.c b/drivers/rtc/rtc-sun6i.c
> index 35b34d14a1db..dc3ae851841c 100644
> --- a/drivers/rtc/rtc-sun6i.c
> +++ b/drivers/rtc/rtc-sun6i.c
> @@ -139,7 +139,7 @@ struct sun6i_rtc_dev {
>  	const struct sun6i_rtc_clk_data *data;
>  	void __iomem *base;
>  	int irq;
> -	unsigned long alarm;
> +	time64_t alarm;
>  
>  	struct clk_hw hw;
>  	struct clk_hw *int_osc;
> @@ -511,10 +511,8 @@ static int sun6i_rtc_setalarm(struct device *dev,
> struct rtc_wkalrm *wkalrm) struct sun6i_rtc_dev *chip =
> dev_get_drvdata(dev); struct rtc_time *alrm_tm = &wkalrm->time;
>  	struct rtc_time tm_now;
> -	unsigned long time_now = 0;
> -	unsigned long time_set = 0;
> -	unsigned long time_gap = 0;
> -	int ret = 0;
> +	time64_t time_now, time_set;
> +	int ret;
>  
>  	ret = sun6i_rtc_gettime(dev, &tm_now);
>  	if (ret < 0) {
> @@ -529,9 +527,7 @@ static int sun6i_rtc_setalarm(struct device *dev,
> struct rtc_wkalrm *wkalrm) return -EINVAL;
>  	}
>  
> -	time_gap = time_set - time_now;
> -
> -	if (time_gap > U32_MAX) {
> +	if ((time_set - time_now) > U32_MAX) {
>  		dev_err(dev, "Date too far in the future\n");
>  		return -EINVAL;
>  	}
> @@ -540,7 +536,7 @@ static int sun6i_rtc_setalarm(struct device *dev,
> struct rtc_wkalrm *wkalrm) writel(0, chip->base + SUN6I_ALRM_COUNTER);
>  	usleep_range(100, 300);
>  
> -	writel(time_gap, chip->base + SUN6I_ALRM_COUNTER);
> +	writel(time_set - time_now, chip->base + SUN6I_ALRM_COUNTER);
>  	chip->alarm = time_set;
>  
>  	sun6i_rtc_setaie(wkalrm->enabled, chip);

On Fri, 11 Feb 2022 12:26:28 +0000, Andre Przywara wrote:
> Using "unsigned long" for UNIX timestamps is never a good idea, and
> comparing the value of such a variable against U32_MAX does not do
> anything useful on 32-bit systems.
> 
> Use the proper time64_t type when dealing with timestamps, and avoid
> cutting down the time range unnecessarily. This also fixes the flawed
> check for the alarm time being too far into the future.
> 
> [...]

Applied, thanks!

[03/18] rtc: sun6i: Fix time overflow handling
        commit: 25c9815569cefd4f719c6c1266fe897e57642278

Best regards,