[bug report] rtc: rzn1: Add oscillator offset support

[bug report] rtc: rzn1: Add oscillator offset support

[Dan Carpenter]

Hello Miquel Raynal,

The patch be4a11cf98af: "rtc: rzn1: Add oscillator offset support"
from May 16, 2022, leads to the following Smatch static checker
warning:

	drivers/rtc/rtc-rzn1.c:302 rzn1_rtc_set_offset()
	warn: 'steps' 'true' implies 'steps > 0' is 'true'

drivers/rtc/rtc-rzn1.c
    270 static int rzn1_rtc_set_offset(struct device *dev, long offset)
    271 {
    272         struct rzn1_rtc *rtc = dev_get_drvdata(dev);
    273         unsigned int steps;
                ^^^^^^^^^^^^^^^^^^
steps is unsigned

    274         int stepsh, stepsl;
    275         u32 val = 0;
    276         int ret;
    277 
    278         /*
    279          * Check which resolution mode (every 20 or 60s) can be used.
    280          * Between 2 and 124 clock pulses can be added or substracted.
    281          *
    282          * In 20s mode, the minimum resolution is 2 / (32768 * 20) which is
    283          * close to 3051 ppb. In 60s mode, the resolution is closer to 1017.
    284          */
    285         stepsh = DIV_ROUND_CLOSEST(offset, 1017);
    286         stepsl = DIV_ROUND_CLOSEST(offset, 3051);
    287 
    288         if (stepsh >= -0x3E && stepsh <= 0x3E) {
    289                 /* 1017 ppb per step */
    290                 steps = stepsh;
    291                 val |= RZN1_RTC_SUBU_DEV;
    292         } else if (stepsl >= -0x3E && stepsl <= 0x3E) {
    293                 /* 3051 ppb per step */
    294                 steps = stepsl;
    295         } else {
    296                 return -ERANGE;
    297         }
    298 
    299         if (!steps)

if steps is zero return

    300                 return 0;
    301 
--> 302         if (steps > 0) {

This is true.  No need to check.

    303                 val |= steps + 1;
    304         } else {
    305                 val |= RZN1_RTC_SUBU_DECR;
    306                 val |= (~(-steps - 1)) & 0x3F;
    307         }
    308 
    309         ret = readl_poll_timeout(rtc->base + RZN1_RTC_CTL2, val,
    310                                  !(val & RZN1_RTC_CTL2_WUST), 100, 2000000);
    311         if (ret)
    312                 return ret;
    313 
    314         writel(val, rtc->base + RZN1_RTC_SUBU);
    315 
    316         return 0;
    317 }

regards,
dan carpenter

Re: [bug report] rtc: rzn1: Add oscillator offset support

[Miquel Raynal]

Hi Dan,

dan.carpenter@oracle.com wrote on Thu, 19 May 2022 16:56:31 +0300:

> Hello Miquel Raynal,
> 
> The patch be4a11cf98af: "rtc: rzn1: Add oscillator offset support"
> from May 16, 2022, leads to the following Smatch static checker
> warning:
> 
> 	drivers/rtc/rtc-rzn1.c:302 rzn1_rtc_set_offset()
> 	warn: 'steps' 'true' implies 'steps > 0' is 'true'
> 
> drivers/rtc/rtc-rzn1.c
>     270 static int rzn1_rtc_set_offset(struct device *dev, long offset)
>     271 {
>     272         struct rzn1_rtc *rtc = dev_get_drvdata(dev);
>     273         unsigned int steps;
>                 ^^^^^^^^^^^^^^^^^^
> steps is unsigned
> 
>     274         int stepsh, stepsl;
>     275         u32 val = 0;
>     276         int ret;
>     277 
>     278         /*
>     279          * Check which resolution mode (every 20 or 60s) can be used.
>     280          * Between 2 and 124 clock pulses can be added or substracted.
>     281          *
>     282          * In 20s mode, the minimum resolution is 2 / (32768 * 20) which is
>     283          * close to 3051 ppb. In 60s mode, the resolution is closer to 1017.
>     284          */
>     285         stepsh = DIV_ROUND_CLOSEST(offset, 1017);
>     286         stepsl = DIV_ROUND_CLOSEST(offset, 3051);
>     287 
>     288         if (stepsh >= -0x3E && stepsh <= 0x3E) {
>     289                 /* 1017 ppb per step */
>     290                 steps = stepsh;
>     291                 val |= RZN1_RTC_SUBU_DEV;
>     292         } else if (stepsl >= -0x3E && stepsl <= 0x3E) {
>     293                 /* 3051 ppb per step */
>     294                 steps = stepsl;
>     295         } else {
>     296                 return -ERANGE;
>     297         }
>     298 
>     299         if (!steps)
> 
> if steps is zero return
> 
>     300                 return 0;
>     301 
> --> 302         if (steps > 0) {  
> 
> This is true.  No need to check.

'steps' should not be unsigned in order to properly carry the offset.

Do you plan to send a patch?

> 
>     303                 val |= steps + 1;
>     304         } else {
>     305                 val |= RZN1_RTC_SUBU_DECR;
>     306                 val |= (~(-steps - 1)) & 0x3F;
>     307         }
>     308 
>     309         ret = readl_poll_timeout(rtc->base + RZN1_RTC_CTL2, val,
>     310                                  !(val & RZN1_RTC_CTL2_WUST), 100, 2000000);
>     311         if (ret)
>     312                 return ret;
>     313 
>     314         writel(val, rtc->base + RZN1_RTC_SUBU);
>     315 
>     316         return 0;
>     317 }
> 
> regards,
> dan carpenter


Thanks,
Miquèl