On Jan 4, 2008 2:26 PM, Andi Kleen <andi@firstfloor.org> wrote:
quoted text
> > There are 2 pending issues, to which I have received insufficient feedback:
> >
> > 1. Andi would like to eliminate the trampoline in mce_thermal.c, but
> > no one has responded to my proposed disgusting hack on entry_64.S in
> > order to do so.
>
> There are two ways to do it:
>
> either duplicate the entry point in entry_64.S and set different
> vectors or let the asm glue jump through a call vector (use
> apicinterrupt ...,*thermal_vector(%rip) ) Later is probably better.

OK, I did it the latter way.  Your thermal_vector is my
smp_thermal_interrupt, which I have converted from a function to a
pointer to the CPU-specific function.

quoted text
>
> >
> > 2. Ingo pointed out that a given config file did not build.  But when
>
> The patch that was in git-x86 didn't build on 32bit. Perhaps it was a
> 32bit config file? Run it under "linux32" or equivalent on your distro.

I _think_ it's fixed now.  I'll let him rerun it at his end for verification.

quoted text
>
> > +      */
> > +     if (therm_throt_process(1))
> > +             mce_log_therm_throt_event(cpu, 1);
> > +     /*
> > +      * We'll still get subsequent interrupts even if we don't clear the
> > +      * status bit in THERM_CTL_F3X64.  Take advantage of this fact to avoid
> > +      * touching PCI space.  (If this assumption fails at some point, we'll
> > +      * need to schedule_work() in order to enter a process context, so that
> > +      * PCI locks can be asserted for proper access.  This requirement, in
>
> PCI locks are spinlocks so they don't need process context. In PREEMPT-RT
> kernels they can sleep, but there the handler will be likely already
> a thread. Touching config space from interrupt context is legal, although
> not very popular because it tends to be slow (but a few drivers do it)

But if PCI locks are spinlocks, then how can one access config space
in an interrupt handler, as it might be locked by the foreground?  (No
locks would be required at all, if everyone just saved 0xCF8 and
0xCFC, but I digress.)  And it's one thing to be "likely" already in a
thread, and another to be sure.  If you can address these issues, I'll
change or remove the comment.  I just want to prevent a
reasonable-looking but bad coding change from happening in the future.

quoted text
>
> > +     /*
> > +      * If any of the northbridges has PCI ID 0x1103, then its thermal
> > +      * hardware suffers from an erratum which prevents this code from
> > +      * working, so abort.
>
> Please add
>
>  * This implies it only works on Family 10h aka AMD Quad Core.
>
> Otherwise I can just see the support questions of people asking why this
> doesn't work for them.

Agreed.  I had it at the top of the function, but now I've worked it
into both places.

quoted text
>
> Anyways I'm unsure about the blacklist here -- white list would
> probably have been better. k8_northbridges[] will certainly include
> Griffin northtbridges and who knows if TT will work there or not.
> [sorry for mentioning that not earlier]

Ideally, every ID in the k8_northbridges[] whitelist would have
functional thermal throttling.  If more IDs than 0x1103 turn out to
have this feature broken, then we may need to add a blacklist as well.
 But I expect that most future IDs will function correctly, hence my
reliance on the whitelist.  In my view, anyone who adds an ID to a
whitelist (or just a list, for that matter) is obligated to perform a
static analysis (i.e. grep for "k8_northbridges") of the implications
of such act; therefore, I'm not too concerned about Griffin.

I've attached an updated patch.  In the unlikely event that you want
to check this in, let me know so I can give it a final test.

-- 
Russell Leidich