linux-kernel - Re: [PATCH] clockevents/drivers/i8253: Do not zero timer counter in shutdown

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: <e9a9fb03a4fd47ebddc3bf984726c0f789d94489.camel@infradead.org>
Date: Thu, 01 Aug 2024 18:49:10 +0100
From: David Woodhouse <dwmw2@...radead.org>
To: Thomas Gleixner <tglx@...utronix.de>, lirongqing@...du.com, 
 seanjc@...gle.com, kys@...rosoft.com, haiyangz@...rosoft.com,
 wei.liu@...nel.org,  decui@...rosoft.com, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com,  x86@...nel.org, linux-hyperv@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clockevents/drivers/i8253: Do not zero timer counter in
 shutdown

On Thu, 2024-08-01 at 16:21 +0200, Thomas Gleixner wrote:
> On Tue, Feb 07 2023 at 09:14, lirongqing@...du.com wrote:
> > @@ -117,11 +110,6 @@ static int pit_shutdown(struct clock_event_device *evt)
> >  
> >         outb_p(0x30, PIT_MODE);
> >  
> > -       if (i8253_clear_counter_on_shutdown) {
> > -               outb_p(0, PIT_CH0);
> > -               outb_p(0, PIT_CH0);
> > -       }
> > -
> 
> The stop sequence is wrong:
> 
>     When there is a count in progress, writing a new LSB before the
>     counter has counted down to 0 and rolled over to FFFFh, WILL stop
>     the counter.  However, if the LSB is loaded AFTER the counter has
>     rolled over to FFFFh, so that an MSB now exists in the counter, then
>     the counter WILL NOT stop.
> 
> The original i8253 datasheet says:
> 
>     1) Write 1st byte stops the current counting
>     2) Write 2nd byte starts the new count

It says that for mode zero ("Interrupt on Terminal Count"), yes. But in
that mode, shouldn't the IRQ only fire *one* more time anyway, rather
than repeatedly? That should be OK, shouldn't it?

"When terminal count is reached, the output will go high and remain
high until the selected count register is reloaded wityh the mode or a
new count is loaded".

It's OK for it to keep *counting* as long as it stops firing
interrupts, isn't it? 

Either way, this is somewhat orthogonal to the patch I posted in 
https://lore.kernel.org/kvm/6cd62b5058e11a6262cb2e798cc85cc5daead3b1.camel@infradead.org/T/#u
for the fact that we don't shut down the PIT at *all* if we aren't ever
going to use it.

I'm glad I decided to export a function from the clocksource driver and
just *call* it from pit_timer_init() though. Means we can bikeshed the
shutdown sequence in *one* place and it isn't duplicated.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)