linux-kernel - Re: [PATCH v6 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

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: <20241210230750dc03a1d5@mail.local>
Date: Wed, 11 Dec 2024 00:07:50 +0100
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: Arnd Bergmann <arnd@...db.de>
Cc: Ciprian Costea <ciprianmarian.costea@....nxp.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will@...nel.org>, linux-rtc@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	NXP S32 Linux Team <s32@....com>, imx@...ts.linux.dev,
	Christophe Lizzi <clizzi@...hat.com>,
	Alberto Ruiz <aruizrui@...hat.com>,
	Enric Balletbo <eballetb@...hat.com>,
	Bogdan Hamciuc <bogdan.hamciuc@....com>,
	Ghennadi Procopciuc <Ghennadi.Procopciuc@....com>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v6 2/4] rtc: s32g: add NXP S32G2/S32G3 SoC support

On 10/12/2024 09:22:51+0100, Arnd Bergmann wrote:
> On Mon, Dec 9, 2024, at 18:17, Ciprian Marian Costea wrote:
> > On 12/6/2024 2:41 PM, Arnd Bergmann wrote:
> 
> >> I think storing 'rtc_hz' as a u32 variable and adding a range
> >> check when filling it would help, mainly to save the next reader
> >> from having to understand what is going on.
> >> 
> >
> > The confusion on my end is that I cannot see where 'div_u64() implicitly 
> > casts the dividend 'hz' from 64-bit to 32-bit' by following the method's 
> > implementation [1]
> 
> I mean passing a 64-bit variable into a function that takes a
> 32-bit argument truncates the range.
> 
> > But I agree that 'rtc_hz' can be stored into a 32-bit variable with a 
> > range check added when it is taken from the Linux clock API to avoid any
> > unneeded abstractions.
> 
> ok
> 
> >> 
> >> This is the same as just removing the error handling and
> >> relying on unsigned integer overflow semantics.
> >> 
> >> The usual check we do in time_before()/time_after instead
> >> checks if the elapsed time is less than half the available
> >> range:
> >> 
> >> #define time_after(a,b)         \
> >>          (typecheck(unsigned long, a) && \
> >>           typecheck(unsigned long, b) && \
> >>           ((long)((b) - (a)) < 0))
> >> 
> >
> > Ok. Thanks for the suggestion. I will look into using 
> > 'time_before()/time_after()' API instead of directly checking via 
> > comparison operators.
> 
> To be clear: you can't directly use time_before() here because
> that takes an 'unsigned long' argument, so you want the
> same logic, but for u32 values. I have not found an existing
> helper for that, but it's possible I missed it.
> 
> >> Who sets that alarm though? Are you relying on custom userspace
> >> for this, or is that something that the kernel already does
> >> that I'm missing?
> >
> > The test usage is via 'rtcwake' [2] userspace tool.
> > I've detailed a bit the testing scenario in the cover letter for this 
> > patchset [3]:
> >
> > "
> > Following is an example of Suspend to RAM trigger on S32G2/S32G3 SoCs,
> > using userspace tools such as rtcwake:
> > # rtcwake -s 2 -m mem
> > # rtcwake: assuming RTC uses UTC ...
> > # rtcwake: wakeup from "mem" using /dev/rtc0 at Wed Feb  6 06:28:36 2036
> 
> Got it. I feel this also needs either some documentation in
> the source code, or some infrastructure in the rtc layer if
> this is a common problem in other drivers as well. If there
> is a maximum time that the system can be suspended for without
> a wakeup, why not just set an earlier wakeup in the kernel
> when you have all the information for it?
> 
> Or maybe this should not actually be an 'rtc' driver at all?
> In the old days, we used drivers like
> arch/arm/mach-omap1/timer32k.c to register a handler
> for read_persistent_clock64(), which completely bypasses
> the RTC layer and provides both automatic wakeup and more
> accurate accounting of sleep time. 
> 
> Another example was the tegra clocksource driver, which used
> to use read_persistent_clock64() but changed to being
> a CLOCK_SOURCE_SUSPEND_NONSTOP source in 95170f0708f2
> ("clocksource/drivers/tegra: Rework for compensation of
> suspend time"). The same seems true for timer-ti-32k.c and
> timer-sprd.c.
> 
> Alexandre, Daniel, any recommendations here?
>

We have a few driver for timers that are not actual RTCs but need the
common alarm and wakeup API so they can use alarmtimers for example.
The limitation is not super common as RTCs will generally have the same
range support for alarm as what they have for time and date.

However, I'm still super confused by how complex this driver is. There
are only 3 interesting registers in this IP, one of those being read
only...

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