linux-kernel - Re: [PATCH v5 1/3] clocksource/drivers/timer-tegra186: add WDIOC

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] [day] [month] [year] [list]

Message-ID: <aBJcxnOhKLUGA5lx@mai.linaro.org>
Date: Wed, 30 Apr 2025 19:24:22 +0200
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Thierry Reding <thierry.reding@...il.com>
Cc: Jon Hunter <jonathanh@...dia.com>, Robert Lin <robelin@...dia.com>,
	tglx@...utronix.de, pohsuns@...dia.com,
	linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
	sumitg@...dia.com
Subject: Re: [PATCH v5 1/3] clocksource/drivers/timer-tegra186: add
 WDIOC_GETTIMELEFT support

On Tue, Apr 29, 2025 at 04:23:22PM +0200, Thierry Reding wrote:
> On Tue, Apr 29, 2025 at 03:19:25PM +0200, Daniel Lezcano wrote:
> > On 29/04/2025 11:15, Jon Hunter wrote:
> > > Hi Daniel,
> > > 
> > > On 29/04/2025 09:59, Daniel Lezcano wrote:
> > > > On Mon, Apr 21, 2025 at 06:08:19PM +0800, Robert Lin wrote:
> > > > > From: Pohsun Su <pohsuns@...dia.com>
> > > > > 
> > > > > This change adds support for WDIOC_GETTIMELEFT so userspace
> > > > > programs can get the number of seconds before system reset by
> > > > > the watchdog timer via ioctl.
> > > > > 
> > > > > Signed-off-by: Pohsun Su <pohsuns@...dia.com>
> > > > > Signed-off-by: Robert Lin <robelin@...dia.com>
> > > > > ---
> > > > 
> > > > Hi Robert,
> > > > 
> > > > I realize that this driver should be split in two and the watchdog
> > > > part go
> > > > under drivers/watchdog.
> > > 
> > > Are there any other examples you know of where the timer is split in
> > > this way? It is not clear to me how you propose we do this?
> > 
> > Just keep the clocksource and move the watchdog code (everything related to
> > the watchdog_ops) to a new driver under drivers/watchdog
> 
> That's a bad idea. This is all a single register space, so we can't have
> "proper" drivers (i.e. ones that exclusively request I/O memory regions)
> if we split them up.
> 
> I understand that it's nice and easy to have things split up along
> subsystem boundaries, but sometimes hardware designs just aren't that
> cleanly separated.

Yes, that's true.

The driver has a lot of watchdog code inside and I think it is
possible to move most part of it under drivers/watchdog. Perhaps by
exporting tegra186_wdt_disable() / tegra186_wdt_enable().

Anyway, I understand that is an important change and I don't want to
block this series for this reason. At the first glance, these changes
seem to be fine for me, I'll just do a last review and comment/pick
them.

> > BTW, there are three clocksources with the same rating, what is the point of
> > having them supported ?
> > 
> > Is it not the architected clocksource enough ?
> 
> The TSC clock source that this driver exposes is different from the
> architected timer. It's a SoC-wide clock that is routed to various IP
> blocks and used for timestamping events. This clocksource allows these
> events to be properly compared, etc.

I see, thanks for clarifying

  -- Daniel


-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog