Re: [PATCH]intel-iommu-PMEN support

[mark gross <mgross@linux.intel.com>]

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