Re: [PATCH 5/5 v2] x86: PAT: make pat_x_mtrr_type() more readable

[Andreas Herrmann]

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@xxxxxxx>
---
 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



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