Message-ID: <3f1a065b0801051208y7e832e8ctda61f28e6328dec3@mail.gmail.com>
Date:	Sat, 5 Jan 2008 12:08:07 -0800
From:	"Russell Leidich" <rml@...gle.com>
To:	"Andi Kleen" <andi@...stfloor.org>
Cc:	"Torsten Kaiser" <just.for.lkml@...glemail.com>,
	"Andrew Morton" <akpm@...ux-foundation.org>,
	linux-kernel@...r.kernel.org,
	"Thomas Gleixner" <tglx@...utronix.de>,
	"Ingo Molnar" <mingo@...e.hu>, valdis.kletnieks@...edu,
	thockin@...gle.com
Subject: Re: [PATCH] AMD Thermal Interrupt Support

On Jan 5, 2008 5:24 AM, Andi Kleen <andi@...stfloor.org> wrote:
> >
> > 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
>
> They disable interrupts

Got it.

>
> > locks would be required at all, if everyone just saved 0xCF8 and
> > 0xCFC, but I digress.)
>
> Not sure what you mean? Since it is two non atomic accesses even
> saving restoring the registers would not make the accesses safe for lockless.
> If someone changes 0xcf8 before you can access 0xcfc you always have
> a problem.

No, it would be safe, because the interrupting process doesn't
actually need to save 0xCFC (since it's a function of 0xCF8).  SMI
handlers use this technique to ensure they don't destroy OS config
space accesses.  Anyway, it doesn't matter.  As long as everyone CLIs
across the whole business, there is no possibility of races.

>
> In fact we had (or still have) races with some older user land accessing these
> ports directly.

Yuck.  I'm going to assume that I can ignore this fact, as they're
already broken with respect to lots of other kernel code.

>
> The only access method that is lockless is mmconfig, which will work
> on most (but not all) Fam10h systems.
>
> > 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.
>
> Well at least as written the comment is not quite correct.

OK, I buy that now.  I'll fix the comment and resend.

> >
> >
> > Agreed.  I had it at the top of the function, but now I've worked it
> > into both places.
> >
> > >
> > > 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
>
> k8_northbridges[] is used by various subsystems, most of them
> do not care at all about thermal throttling.
>
> > 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.
>
> ? you already got a blacklist ?

Yes, technically.  It's a single "if" statement which tests for 0x1103
:).  What I was saying was that if lots more thermally-broken CPUs
show up in the wild, then we can expand this into a blacklist array.

>
> >  But I expect that most future IDs will function correctly, hence my
> > reliance on the whitelist.
>
> ? but you don't have a whitelist, you have a black list.

I'm using k8_northbridges[] as my de-facto whitelist, because I
believe that future CPUs will probably function properly.  (It's hard
to imagine AMD removing or breaking a perfectly functional system,
apart from maybe adding new features.  Worst case, as I said, we  can
create a blacklist.  But right now, that's unnecessary.)

Any other concerns?

-- 
Russell Leidich
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists