Re: [PATCH]intel-iommu-PMEN support

mark gross
2007-11-20T17:59:16Z

On Mon, Nov 19, 2007 at 04:38:02PM -0800, Andrew Morton wrote:
> On Fri, 16 Nov 2007 14:39:57 -0800
> mark gross <mgross@linux.intel.com> 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