linux-kernel - RE: [PATCH 3/4] rtc: zynqmp: rework set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <LV5PR12MB98045BF8A85640F924BCFD4492A3A@LV5PR12MB9804.namprd12.prod.outlook.com>
Date: Tue, 9 Dec 2025 19:03:02 +0000
From: "T, Harini" <Harini.T@....com>
To: Tomas Melin <tomas.melin@...sala.com>, Alexandre Belloni
	<alexandre.belloni@...tlin.com>, "Simek, Michal" <michal.simek@....com>
CC: "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 3/4] rtc: zynqmp: rework set_offset

[Public]

Hi,

> -----Original Message-----
> From: Tomas Melin <tomas.melin@...sala.com>
> Sent: Monday, December 1, 2025 6:20 PM
> To: Alexandre Belloni <alexandre.belloni@...tlin.com>; Simek, Michal
> <michal.simek@....com>
> Cc: linux-rtc@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; Tomas Melin <tomas.melin@...sala.com>
> Subject: [PATCH 3/4] rtc: zynqmp: rework set_offset
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> set_offset was using remainder of do_div as tick_mult which resulted in
> wrong offset. Calibration value also assumed builtin calibration default.
> Update fract_offset to correctly calculate the value for negative offset and
> replace the for loop with division.
>
> Signed-off-by: Tomas Melin <tomas.melin@...sala.com>
> ---
>  drivers/rtc/rtc-zynqmp.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/rtc/rtc-zynqmp.c b/drivers/rtc/rtc-zynqmp.c index
> 7af5f6f99538f961a53ff56bfc656c907611b900..3bc8831ba2c4c4c701a49506b6
> 7ae6174f3ade3d 100644
> --- a/drivers/rtc/rtc-zynqmp.c
> +++ b/drivers/rtc/rtc-zynqmp.c
> @@ -208,13 +208,13 @@ static int xlnx_rtc_read_offset(struct device *dev,
> long *offset)  static int xlnx_rtc_set_offset(struct device *dev, long offset)  {
>         struct xlnx_rtc_dev *xrtcdev = dev_get_drvdata(dev);
> -       unsigned long long rtc_ppb = RTC_PPB;
> -       unsigned int tick_mult = do_div(rtc_ppb, xrtcdev->freq);
> +       unsigned int calibval, tick_mult, fract_part;
tick_mult is mentioned as int in previous patch and unsigned here. Justify the type in commit description.
>         unsigned char fract_tick = 0;
> -       unsigned int calibval;
> -       short int  max_tick;
> -       int fract_offset;
> +       int freq = xrtcdev->freq;
> +       int max_tick, fract_offset;
Please follow reverse xmas tree variable ordering.
Also keep the frac_* variables uniform in both set and read offset functions.
>
> +       /* ticks to reach RTC_PPB */
The comment is misleading. Its tick_mult is nanoseconds per tick, not a tick count.
> +       tick_mult = DIV_ROUND_CLOSEST(RTC_PPB, xrtcdev->freq);
We can first validate offset and then calculate tick_mult to reduce CPU instructions incase of invalid inputs
>         if (offset < RTC_MIN_OFFSET || offset > RTC_MAX_OFFSET)
>                 return -ERANGE;
>
> @@ -223,29 +223,22 @@ static int xlnx_rtc_set_offset(struct device *dev,
> long offset)
>
>         /* Number fractional ticks for given offset */
>         if (fract_offset) {
> +               /* round up here so we stay below a full tick */
> +               fract_part = DIV_ROUND_UP(tick_mult, RTC_FR_MAX_TICKS);
>                 if (fract_offset < 0) {
> -                       fract_offset = fract_offset + tick_mult;
> +                       fract_offset += (fract_part * RTC_FR_MAX_TICKS);
It would be better to add comment to explain on the negative offset borrowing logic
>                         max_tick--;
>                 }
> -               if (fract_offset > (tick_mult / RTC_FR_MAX_TICKS)) {
> -                       for (fract_tick = 1; fract_tick < 16; fract_tick++) {
> -                               if (fract_offset <=
> -                                   (fract_tick *
> -                                    (tick_mult / RTC_FR_MAX_TICKS)))
> -                                       break;
> -                       }
> -               }
> +               fract_tick = fract_offset / fract_part;
Its better to use DIV_ROUND_UP()
>         }
>
>         /* Zynqmp RTC uses second and fractional tick
>          * counters for compensation
>          */
> -       calibval = max_tick + RTC_CALIB_DEF;
> +       calibval = max_tick + freq;
>
>         if (fract_tick)
> -               calibval |= RTC_FR_EN;
> -
> -       calibval |= (fract_tick << RTC_FR_DATSHIFT);
> +               calibval |= (RTC_FR_EN | (fract_tick <<
> + RTC_FR_DATSHIFT));
>
>         writel(calibval, (xrtcdev->reg_base + RTC_CALIB_WR));
>
>
> --
> 2.47.3
>

Regards,
Harini T