Re: [PATCH V2] iommu/amd: Add logic to decode AMD IOMMU event flag

[Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>]

On Tue, Apr 02, 2013 at 02:05:14PM -0500, Suthikulpanit, Suravee wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit-5C7GfCeVMHo@public.gmane.org>
> +static const char * const _type_field_encodings[] = {
> +	/* 00 */"Reserved",
> +	/* 01 */"Master Abort",
> +	/* 10 */"Target Abort",
> +	/* 11 */"Data Error",

In these arrays, please put the comments at the end of the line and
align them nicely like this:

	static const char * const _type_field_encodings[] = {
		"Reserved",		/* 00 */
		"Master Abort",		/* 01 */
		"Target Abort",		/* 10 */
		"Data Error",		/* 11 */
	};

This improves readability a lot.

> +static void dump_flags(int flags, int ev_type)
> +{
> +	struct _event_log_flags *p = (struct _event_log_flags *) &flags;
> +	u32 err_type = p->type;
> +
> +	pr_err("AMD-Vi: Flags details: %s, NX:%u, %s, %s, %s, %s, %s, %s, %s\n",
> +		(p->gn ? "Use guest address" : "Use nested address"),
> +		(p->nx),
> +		(p->us ? "User privileges" : "Supervisor privileges"),
> +		(p->i ? "Interrupt request" : "memory request"),
> +		(p->pr ? "Page present or Interrupt remapped" :
> +			"Page not present or Interrupt blocked"),
> +		(p->rw ? "Write transaction" : "Read transaction"),
> +		(p->pe ? "Does not have permission" : "Has permission"),
> +		(p->rz ? "Reserv bit not zero" : "Illegal level encoding"),
> +		(p->tr ? "Translation request" : "Transaction request"));

These strings are too long, please just print useful shortcuts for them,
like
	"US" : "SV" instead of
	"User privileges" : "Supervisor privileges"

Also the commas between the strings are not needed then.

As a general rule, in the default configuration an event-log entry
should not take more than a single line in dmesg (which is also not too
long). If you want to print more information in some cases you can
enable that by a command line parameter like 'amd_iommu=verbose' or
something like that.

	Joerg