On Fri, Jun 14, 2019 at 06:10:18PM +0200, Thomas Gleixner wrote: > On Thu, 13 Jun 2019, Ricardo Neri wrote: > > > On Tue, Jun 11, 2019 at 09:54:25PM +0200, Thomas Gleixner wrote: > > > On Thu, 23 May 2019, Ricardo Neri wrote: > > > > > > > HPET timer 2 will be used to drive the HPET-based hardlockup detector. > > > > Reserve such timer to ensure it cannot be used by user space programs or > > > > for clock events. > > > > > > > > When looking for MSI-capable timers for clock events, skip timer 2 if > > > > the HPET hardlockup detector is selected. > > > > > > Why? Both the changelog and the code change lack an explanation why this > > > timer is actually touched after it got reserved for the platform. The > > > reservation should make it inaccessible for other things. > > > > hpet_reserve_platform_timers() will give the HPET char driver a data > > structure which specifies which drivers are reserved. In this manner, > > they cannot be used by applications via file opens. The timer used by > > the hardlockup detector should be marked as reserved. > > > > Also, hpet_msi_capability_lookup() populates another data structure > > which is used when obtaining an unused timer for a HPET clock event. > > The timer used by the hardlockup detector should not be included in such > > data structure. > > > > Is this the explanation you would like to see? If yes, I will include it > > in the changelog. > > Yes, the explanation makes sense. The code still sucks. Not really your > fault, but this is not making it any better. > > What bothers me most is the fact that CONFIG_X86_HARDLOCKUP_DETECTOR_HPET > removes one HPET timer unconditionally. It neither checks whether the hpet > watchdog is actually enabled on the command line, nor does it validate > upfront whether the HPET supports FSB delivery. > > That wastes an HPET timer unconditionally for no value. Not that I > personally care much about /dev/hpet, but some older laptops depend on HPET > per cpu timers as the local APIC timer stops in C2/3. So this unconditional > reservation will cause regressions for no reason. > > The proper approach here is to: > > 1) Evaluate the command line _before_ hpet_enable() is invoked > > 2) Check the availability of FSB delivery in hpet_enable() > > Reserve an HPET channel for the watchdog only when #1 and #2 are true.
Sure. I will add the explanation in the message commit and only reserve the timer if both of the conditions above are met. Thanks and BR, Ricardo
