Re: [PATCH] AMD Thermal Interrupt Support

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 27 Dec 2007 10:57:20 -0800 "Russell Leidich" <rml@google.com> wrote:

> Thanks Andrew.  I have a few questions on your comments...
> 
> On Dec 25, 2007 2:04 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> > > +     unsigned int apic_lv_therm;
> > > +
> > > +     /* Set up APIC_LVTTHMR to issue THERMAL_APIC_VECTOR. */
> > > +     apic_lv_therm = apic_read(APIC_LVTTHMR);
> > > +     /*
> > > +      * See if some agent other than this routine has already initialized
> > > +      * APIC_LVTTHMR, i.e. if it's unmasked, but not equal to the value that
> > > +      * we would have programmed, had we been here before on this core.
> > > +      */
> > > +     if ((!(apic_lv_therm & APIC_LVT_MASKED)) && ((apic_lv_therm &
> > > +             (APIC_MODE_MASK | APIC_VECTOR_MASK)) != (APIC_DM_FIXED |
> > > +             THERMAL_APIC_VECTOR))) {
> > > +             unsigned int cpu = smp_processor_id();
> >
> > afaict this function is called while the calling thread is running
> > preemptibly.  This smp_processor_id() call should have generated a runtime
> > warning if it was tested with all debug options enabled?
> 
> I thought that the whole point of on_each_cpu(&thermal_apic_init,
> NULL, 1, 0) was to ensure that thermal_apic_init() runs
> (nonpreemptibly) on each core.  No?

oh, OK.

> > > +static void default_smp_thermal_interrupt(void) {}
> >
> > static void default_smp_thermal_interrupt(void)
> > {
> > }
> >
> > please.
> >
> >
> > Can this function ever actually be called?
> 
> My colleauge was concerned that we have a do-nothing handler in case
> we get a spurious thermal interrupt.  In my view, there's no point
> programming for the possibility of broken hardware.  On the other
> hand, I do need some sort of indirection to bind the entry.S thermal
> handler from assembly language to either Intel or AMD C code.  Trouble
> is, at compiletime, we might have both Intel and AMD support
> installed, but only one of them should actually receive the interrupt
> at runtime.  So I think I need to do runtime binding, unless I want to
> do CPUID inside the ISR.  What do you think?

If it "can't happen" then we can just leave the vector pointing at 0 until
it gets set up properly.  But nobody ever got fired for mistrusting
hardware (and BIOS) so perhaps what we should do is to leave that there, but
make it emit a suitable diagnostic?  A printk_ratelimit()ed one, probably.