Messages in this thread Patch in this message | | | Date | Wed, 18 Jun 2008 15:38:57 +0200 | From | Andreas Herrmann <> | Subject | Re: [PATCH 5/5 v2] x86: PAT: make pat_x_mtrr_type() more readable |
| |
On Mon, Jun 16, 2008 at 06:42:43PM +0100, Hugh Dickins wrote: > On Mon, 16 Jun 2008, Andreas Herrmann wrote: > > I've found it inconvenient to review pat_x_mtrr_type(). > > Thus I slightly changed it and added some comment to make > > it more readable. > > I found it hard to read too; but it's amusing how utterly different > are our ideas to make it more readable. Most of what it is doing > seems to me confusingly left over from earlier implementations.
;-)
> I've appended my version at the bottom: my involvement in pat.c is > not very strong, so would you like to take over my patch and combine > best of both into one? I don't particularly want to stroll along > after, undoing what you did.
Why combining both patches? I've checked your patch and found it more straightforward than mine. And the attached patch makes it even shorter. (patch against tip/x86/pat -- where your patch is already residing)
> > > > I've also added BUG statements for (some) unused/unhandled PAT/MTRR > > types. > > I suspect your pat_type BUG is uninteresting (given _PAGE_CACHE_MASK
It's just interesting if someone would change that mask and forget to handle potential new pat_types. I admit that is rather unlikely.
> only has two bits to play with), and your mtrr_type BUG dangerous -
Well, the former code had this snippet in pat_x_mtrr_type():
- mtrr_type = mtrr_type_lookup(start, end); - if (mtrr_type == 0xFF) { /* MTRR not enabled */ - *ret_prot = prot; - return 0; - } - if (mtrr_type == 0xFE) { /* MTRR match error */ - *ret_prot = _PAGE_CACHE_UC; - return -1; - } - if (mtrr_type != MTRR_TYPE_UNCACHABLE && - mtrr_type != MTRR_TYPE_WRBACK && - mtrr_type != MTRR_TYPE_WRCOMB) { /* MTRR type unhandled */ - *ret_prot = _PAGE_CACHE_UC; - return -1; - } -
But now it seems that the function intentional handles MTRR_TYPE_WRTHROUGH and the 0xFE/0xFF cases and thus the BUG statement is wrong.
Thanks,
Andreas
--- [PATCH] x86: shrink pat_x_mtrr_type to its essentials
Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com> --- arch/x86/mm/pat.c | 30 +++++++++++------------------- 1 files changed, 11 insertions(+), 19 deletions(-)
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c index ac3a2b1..227df3c 100644 --- a/arch/x86/mm/pat.c +++ b/arch/x86/mm/pat.c @@ -161,29 +161,21 @@ static DEFINE_SPINLOCK(memtype_lock); /* protects memtype list */ */ static unsigned long pat_x_mtrr_type(u64 start, u64 end, unsigned long req_type) { - u8 mtrr_type; - - /* - * We return the PAT request directly for types where PAT takes - * precedence with respect to MTRR and for UC_MINUS. - * Consistency checks with other PAT requests is done later - * while going through memtype list. - */ - if (req_type == _PAGE_CACHE_WC || - req_type == _PAGE_CACHE_UC_MINUS || - req_type == _PAGE_CACHE_UC) - return req_type; - /* * Look for MTRR hint to get the effective type in case where PAT * request is for WB. */ - mtrr_type = mtrr_type_lookup(start, end); - if (mtrr_type == MTRR_TYPE_UNCACHABLE) - return _PAGE_CACHE_UC; - if (mtrr_type == MTRR_TYPE_WRCOMB) - return _PAGE_CACHE_WC; - return _PAGE_CACHE_WB; + if (req_type == _PAGE_CACHE_WB) { + u8 mtrr_type; + + mtrr_type = mtrr_type_lookup(start, end); + if (mtrr_type == MTRR_TYPE_UNCACHABLE) + return _PAGE_CACHE_UC; + if (mtrr_type == MTRR_TYPE_WRCOMB) + return _PAGE_CACHE_WC; + } + + return req_type; } /* -- 1.5.5.4
| |