On Mon, Nov 19, 2007 at 04:38:02PM -0800, Andrew Morton wrote:
> On Fri, 16 Nov 2007 14:39:57 -0800
> mark gross <[EMAIL PROTECTED]> wrote:
>
> > -#define MAX_FAULT_REASON_IDX ARRAY_SIZE(fault_reason_strings) - 1
> > +#define MAX_FAULT_REASON_IDX (ARRAY_SIZE(fault_reason_strings) - 1)
>
> hm. The logic in there looks screwy.
I didn't like it when they added the -1 to the above, but after looking
at it sideways for a while I convinced myself that it was ok. so I
thought it be good to add the parentheses.
>
>
> static char *fault_reason_strings[] =
> {
> "Software",
> "Present bit in root entry is clear",
> "Present bit in context entry is clear",
> "Invalid context entry",
> "Access beyond MGAW",
> "PTE Write access is not set",
> "PTE Read access is not set",
> "Next page table ptr is invalid",
> "Root table address invalid",
> "Context table ptr is invalid",
> "non-zero reserved fields in RTP",
> "non-zero reserved fields in CTP",
> "non-zero reserved fields in PTE",
> "Unknown"
> };
> #define MAX_FAULT_REASON_IDX (ARRAY_SIZE(fault_reason_strings) - 1)
>
> char *dmar_get_fault_reason(u8 fault_reason)
> {
> if (fault_reason >= MAX_FAULT_REASON_IDX)
> return fault_reason_strings[MAX_FAULT_REASON_IDX - 1];
> else
> return fault_reason_strings[fault_reason];
> }
>
>
> so all invalid fault_reasons will cause us to display "non-zero reserved
> fields in PTE".
GAH! Thats wrong.
I also don't like the use of >= here, its harder for me to parse in my
head than just >.
>
> Why not just do
>
> if (fault_reason >= ARRAY_SIZE(fault_reason_strings))
> return "Unknown";
> return fault_reason_strings[fault_reason];
>
> ?
Why not indeed. :)
>
> (might as well make fault_reason_strings[] const, too).
Thats a good thing too.
I'll put together a patch this morning to address these think-ohs.
--mgross
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
