[PATCH] cleanup ACPI numa warnings

[PATCH] cleanup ACPI numa warnings

[Alex Williamson]

   The patch below removes these warnings:

  CC      drivers/acpi/numa.o
drivers/acpi/numa.c: In function `acpi_table_print_srat_entry':
drivers/acpi/numa.c:55: warning: unused variable `p'
drivers/acpi/numa.c:65: warning: unused variable `p'
drivers/acpi/numa.c: In function `acpi_numa_init':
drivers/acpi/numa.c:179: warning: passing arg 2 of `acpi_table_parse_srat' from incompatible pointer type
drivers/acpi/numa.c:182: warning: passing arg 2 of `acpi_table_parse_srat' from incompatible pointer type

And propagates the MADT error checking code into the SRAT code.

Signed-off-by: Alex Williamson <alex.williamson@hp.com>

===== drivers/acpi/numa.c 1.10 vs edited =====
--- 1.10/drivers/acpi/numa.c	2004-02-18 02:19:31 -07:00
+++ edited/drivers/acpi/numa.c	2004-08-05 14:37:19 -06:00
@@ -38,6 +38,33 @@
 
 extern int __init acpi_table_parse_madt_family (enum acpi_table_id id, unsigned long madt_size, int entry_id, acpi_madt_entry_handler handler, unsigned int max_entries);
 
+#ifdef ACPI_DEBUG_OUTPUT
+#define acpi_print_srat_processor_affinity(header) { \
+	struct acpi_table_processor_affinity *p = \
+	                      (struct acpi_table_processor_affinity*) header; \
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Processor (id[0x%02x] " \
+	                 "eid[0x%02x]) in proximity domain %d %s\n", \
+	                 p->apic_id, p->lsapic_eid, p->proximity_domain, \
+	                 p->flags.enabled?"enabled":"disabled")); }
+
+#define acpi_print_srat_memory_affinity(header) { \
+	struct acpi_table_memory_affinity *p = \
+	                         (struct acpi_table_memory_affinity*) header; \
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Memory (0x%08x%08x length " \
+	                 "0x%08x%08x type 0x%x) in proximity domain %d %s%s\n",\
+	                 p->base_addr_hi, p->base_addr_lo, p->length_hi, \
+	                 p->length_lo, p->memory_type, p->proximity_domain, \
+	                 p->flags.enabled ? "enabled" : "disabled", \
+	                 p->flags.hot_pluggable ? " hot-pluggable" : "")); }
+#else
+#define acpi_print_srat_processor_affinity(header)
+#define acpi_print_srat_memory_affinity(header)
+#endif
+
+#define BAD_SRAT_ENTRY(entry, end) ( \
+	(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
+	((acpi_table_entry_header *)entry)->length != sizeof(*entry))
+
 void __init
 acpi_table_print_srat_entry (
 	acpi_table_entry_header	*header)
@@ -51,27 +78,11 @@
 	switch (header->type) {
 
 	case ACPI_SRAT_PROCESSOR_AFFINITY:
-	{
-		struct acpi_table_processor_affinity *p =
-			(struct acpi_table_processor_affinity*) header;
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Processor (id[0x%02x] eid[0x%02x]) in proximity domain %d %s\n",
-		       p->apic_id, p->lsapic_eid, p->proximity_domain,
-		       p->flags.enabled?"enabled":"disabled"));
-	}
+		acpi_print_srat_processor_affinity(header);
 		break;
-
 	case ACPI_SRAT_MEMORY_AFFINITY:
-	{
-		struct acpi_table_memory_affinity *p =
-			(struct acpi_table_memory_affinity*) header;
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Memory (0x%08x%08x length 0x%08x%08x type 0x%x) in proximity domain %d %s%s\n",
-		       p->base_addr_hi, p->base_addr_lo, p->length_hi, p->length_lo,
-		       p->memory_type, p->proximity_domain,
-		       p->flags.enabled ? "enabled" : "disabled",
-		       p->flags.hot_pluggable ? " hot-pluggable" : ""));
-	}
+		acpi_print_srat_memory_affinity(header);
 		break;
-
 	default:
 		printk(KERN_WARNING PREFIX "Found unsupported SRAT entry (type = 0x%x)\n",
 			header->type);
@@ -103,12 +114,12 @@
 
 
 static int __init
-acpi_parse_processor_affinity (acpi_table_entry_header *header)
+acpi_parse_processor_affinity (acpi_table_entry_header *header, unsigned long size)
 {
 	struct acpi_table_processor_affinity *processor_affinity;
 
 	processor_affinity = (struct acpi_table_processor_affinity*) header;
-	if (!processor_affinity)
+	if (BAD_SRAT_ENTRY(processor_affinity, size))
 		return -EINVAL;
 
 	acpi_table_print_srat_entry(header);
@@ -121,12 +132,12 @@
 
 
 static int __init
-acpi_parse_memory_affinity (acpi_table_entry_header *header)
+acpi_parse_memory_affinity (acpi_table_entry_header *header, unsigned long size)
 {
 	struct acpi_table_memory_affinity *memory_affinity;
 
 	memory_affinity = (struct acpi_table_memory_affinity*) header;
-	if (!memory_affinity)
+	if (BAD_SRAT_ENTRY(memory_affinity, size))
 		return -EINVAL;
 
 	acpi_table_print_srat_entry(header);

Re: [PATCH] cleanup ACPI numa warnings

[Dave Hansen]

On Thu, 2004-08-05 at 13:46, Alex Williamson wrote:
> +#ifdef ACPI_DEBUG_OUTPUT
> +#define acpi_print_srat_processor_affinity(header) { \
> +	struct acpi_table_processor_affinity *p = \
> +	                      (struct acpi_table_processor_affinity*) header; \
> +	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Processor (id[0x%02x] " \
> +	                 "eid[0x%02x]) in proximity domain %d %s\n", \
> +	                 p->apic_id, p->lsapic_eid, p->proximity_domain, \
> +	                 p->flags.enabled?"enabled":"disabled")); }
> +
> +#define acpi_print_srat_memory_affinity(header) { \
> +	struct acpi_table_memory_affinity *p = \
> +	                         (struct acpi_table_memory_affinity*) header; \
> +	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Memory (0x%08x%08x length " \
> +	                 "0x%08x%08x type 0x%x) in proximity domain %d %s%s\n",\
> +	                 p->base_addr_hi, p->base_addr_lo, p->length_hi, \
> +	                 p->length_lo, p->memory_type, p->proximity_domain, \
> +	                 p->flags.enabled ? "enabled" : "disabled", \
> +	                 p->flags.hot_pluggable ? " hot-pluggable" : "")); }

Is there a reason that this can't be a normal function instead of a
9-line #define?

-- Dave



-------------------------------------------------------
This SF.Net email is sponsored by OSTG. Have you noticed the changes on
Linux.com, ITManagersJournal and NewsForge in the past few weeks? Now,
one more big change to announce. We are now OSTG- Open Source Technology
Group. Come see the changes on the new OSTG site. www.ostg.com

Re: [PATCH] cleanup ACPI numa warnings

[Alex Williamson]

On Thu, 2004-08-05 at 14:01 -0700, Dave Hansen wrote:
> On Thu, 2004-08-05 at 13:46, Alex Williamson wrote:
> > +#ifdef ACPI_DEBUG_OUTPUT
> > +#define acpi_print_srat_processor_affinity(header) { \
> > +	struct acpi_table_processor_affinity *p = \
> > +	                      (struct acpi_table_processor_affinity*) header; \
> > +	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Processor (id[0x%02x] " \
> > +	                 "eid[0x%02x]) in proximity domain %d %s\n", \
> > +	                 p->apic_id, p->lsapic_eid, p->proximity_domain, \
> > +	                 p->flags.enabled?"enabled":"disabled")); }
> > +
> > +#define acpi_print_srat_memory_affinity(header) { \
> > +	struct acpi_table_memory_affinity *p = \
> > +	                         (struct acpi_table_memory_affinity*) header; \
> > +	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Memory (0x%08x%08x length " \
> > +	                 "0x%08x%08x type 0x%x) in proximity domain %d %s%s\n",\
> > +	                 p->base_addr_hi, p->base_addr_lo, p->length_hi, \
> > +	                 p->length_lo, p->memory_type, p->proximity_domain, \
> > +	                 p->flags.enabled ? "enabled" : "disabled", \
> > +	                 p->flags.hot_pluggable ? " hot-pluggable" : "")); }
> 
> Is there a reason that this can't be a normal function instead of a
> 9-line #define?

   Well, it's 9 lines, but it boils down to one printk.  I'm not sure
putting it in a function would make it any more readable, long printks
are ugly by design.  Either way would work.

	Alex

-- 
Alex Williamson                             HP Linux & Open Source Lab



-------------------------------------------------------
This SF.Net email is sponsored by OSTG. Have you noticed the changes on
Linux.com, ITManagersJournal and NewsForge in the past few weeks? Now,
one more big change to announce. We are now OSTG- Open Source Technology
Group. Come see the changes on the new OSTG site. www.ostg.com

Re: [PATCH] cleanup ACPI numa warnings

[Martin J. Bligh]

--Alex Williamson <alex.williamson@hp.com> wrote (on Thursday, August 05, 2004 15:25:42 -0600):

> On Thu, 2004-08-05 at 14:01 -0700, Dave Hansen wrote:
>> On Thu, 2004-08-05 at 13:46, Alex Williamson wrote:
>> > +#ifdef ACPI_DEBUG_OUTPUT
>> > +#define acpi_print_srat_processor_affinity(header) { \
>> > +	struct acpi_table_processor_affinity *p = \
>> > +	                      (struct acpi_table_processor_affinity*) header; \
>> > +	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Processor (id[0x%02x] " \
>> > +	                 "eid[0x%02x]) in proximity domain %d %s\n", \
>> > +	                 p->apic_id, p->lsapic_eid, p->proximity_domain, \
>> > +	                 p->flags.enabled?"enabled":"disabled")); }
>> > +
>> > +#define acpi_print_srat_memory_affinity(header) { \
>> > +	struct acpi_table_memory_affinity *p = \
>> > +	                         (struct acpi_table_memory_affinity*) header; \
>> > +	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Memory (0x%08x%08x length " \
>> > +	                 "0x%08x%08x type 0x%x) in proximity domain %d %s%s\n",\
>> > +	                 p->base_addr_hi, p->base_addr_lo, p->length_hi, \
>> > +	                 p->length_lo, p->memory_type, p->proximity_domain, \
>> > +	                 p->flags.enabled ? "enabled" : "disabled", \
>> > +	                 p->flags.hot_pluggable ? " hot-pluggable" : "")); }
>> 
>> Is there a reason that this can't be a normal function instead of a
>> 9-line #define?
> 
>    Well, it's 9 lines, but it boils down to one printk.  I'm not sure
> putting it in a function would make it any more readable, long printks
> are ugly by design.  Either way would work.

Multi-line #defines are inherently eeeeevil ;-) I'd agree with Dave - static 
inlines are the normal way to do this.

M.

Re: [ACPI] Re: [PATCH] cleanup ACPI numa warnings

[Randy.Dunlap]

On Thu, 05 Aug 2004 20:35:10 -0700 Martin J. Bligh wrote:

| --Alex Williamson <alex.williamson@hp.com> wrote (on Thursday, August 05, 2004 15:25:42 -0600):
| 
| > On Thu, 2004-08-05 at 14:01 -0700, Dave Hansen wrote:
| >> On Thu, 2004-08-05 at 13:46, Alex Williamson wrote:
| >> > +#ifdef ACPI_DEBUG_OUTPUT
| >> > +#define acpi_print_srat_processor_affinity(header) { \
| >> > +	struct acpi_table_processor_affinity *p = \
| >> > +	                      (struct acpi_table_processor_affinity*) header; \
| >> > +	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Processor (id[0x%02x] " \
| >> > +	                 "eid[0x%02x]) in proximity domain %d %s\n", \
| >> > +	                 p->apic_id, p->lsapic_eid, p->proximity_domain, \
| >> > +	                 p->flags.enabled?"enabled":"disabled")); }
| >> > +
| >> > +#define acpi_print_srat_memory_affinity(header) { \
| >> > +	struct acpi_table_memory_affinity *p = \
| >> > +	                         (struct acpi_table_memory_affinity*) header; \
| >> > +	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Memory (0x%08x%08x length " \
| >> > +	                 "0x%08x%08x type 0x%x) in proximity domain %d %s%s\n",\
| >> > +	                 p->base_addr_hi, p->base_addr_lo, p->length_hi, \
| >> > +	                 p->length_lo, p->memory_type, p->proximity_domain, \
| >> > +	                 p->flags.enabled ? "enabled" : "disabled", \
| >> > +	                 p->flags.hot_pluggable ? " hot-pluggable" : "")); }
| >> 
| >> Is there a reason that this can't be a normal function instead of a
| >> 9-line #define?
| > 
| >    Well, it's 9 lines, but it boils down to one printk.  I'm not sure
| > putting it in a function would make it any more readable, long printks
| > are ugly by design.  Either way would work.
| 
| Multi-line #defines are inherently eeeeevil ;-) I'd agree with Dave - static 
| inlines are the normal way to do this.

That's surely just an opinion, right?  8;)

If they are more than a single function (like printk), I might agree,
but the Linux kernel has plenty of them already.

Using a macro for a big printk() seems to makes sense.
And there's nothing in CodingStyle that agrees with you that I could find.

--
~Randy

Re: Re: [PATCH] cleanup ACPI numa warnings

[Paul Jackson]

> And there's nothing in CodingStyle that agrees with you that I could find.

>From the file Documentation/SubmittingPatches:

        3) 'static inline' is better than a macro

        Static inline functions are greatly preferred over macros.
        They provide type safety, have no length limitations, no formatting
        limitations, and under gcc they are as cheap as macros.

        Macros should only be used for cases where a static inline is clearly
        suboptimal [there a few, isolated cases of this in fast paths],
        or where it is impossible to use a static inline function [such as
        string-izing].

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj-sJ/iWh9BUns@public.gmane.org> 1.650.933.1373


-------------------------------------------------------
This SF.Net email is sponsored by OSTG. Have you noticed the changes on
Linux.com, ITManagersJournal and NewsForge in the past few weeks? Now,
one more big change to announce. We are now OSTG- Open Source Technology
Group. Come see the changes on the new OSTG site. www.ostg.com

Re: [ACPI] Re: [PATCH] cleanup ACPI numa warnings

[Randy.Dunlap]

On Sat, 7 Aug 2004 10:57:29 -0700 Paul Jackson wrote:

| > And there's nothing in CodingStyle that agrees with you that I could find.
| 
| >From the file Documentation/SubmittingPatches:
| 
|         3) 'static inline' is better than a macro
| 
|         Static inline functions are greatly preferred over macros.
|         They provide type safety, have no length limitations, no formatting
|         limitations, and under gcc they are as cheap as macros.
| 
|         Macros should only be used for cases where a static inline is clearly
|         suboptimal [there a few, isolated cases of this in fast paths],
|         or where it is impossible to use a static inline function [such as
|         string-izing].

Oops.  Thanks, Paul.

I agree that the inline looks better than the macro (more readable,
possibly more maintainable), but not that the multi-line macro
is _evil_ (which is what Martin said).


--
~Randy

Re: Re: [PATCH] cleanup ACPI numa warnings

[Martin J. Bligh]

--"Randy.Dunlap" <rddunlap-3NddpPZAyC0@public.gmane.org> wrote (on Sunday, August 08, 2004 14:36:31 -0700):

> On Sat, 7 Aug 2004 10:57:29 -0700 Paul Jackson wrote:
> 
>| > And there's nothing in CodingStyle that agrees with you that I could find.
>| 
>| > From the file Documentation/SubmittingPatches:
>| 
>|         3) 'static inline' is better than a macro
>| 
>|         Static inline functions are greatly preferred over macros.
>|         They provide type safety, have no length limitations, no formatting
>|         limitations, and under gcc they are as cheap as macros.
>| 
>|         Macros should only be used for cases where a static inline is clearly
>|         suboptimal [there a few, isolated cases of this in fast paths],
>|         or where it is impossible to use a static inline function [such as
>|         string-izing].
> 
> Oops.  Thanks, Paul.
> 
> I agree that the inline looks better than the macro (more readable,
> possibly more maintainable), but not that the multi-line macro
> is _evil_ (which is what Martin said).

It's not that this multi-line macro was particularly offensive ... it's
that I've seen the most heinous crap in the past ... and the only sensible
place I can find to draw a line is ... no multiline macros ;-)

M.



-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285

Re: Re: [PATCH] cleanup ACPI numa warnings

[Alex Williamson]

   I'm not sure where we stand on this, sorry for the delay.  To recap,
the first patch I submitted cleaned up the original functions, but moved
the ugliness up into multi-line macros.  People didn't like the macros
and suggested static inlines.  However, static inlines don't work for
this application because the debug print needs state setup by the
ACPI_FUNCTION_NAME call.  IMHO, it's not worth setting up that state in
the static inline function for this little bit of cleanup.

   So, I think we left it at nobody liked the macros and static inlines
don't work.  General unhappiness.  Below is a patch that doesn't attempt
to cleanup the original code, it just adds the #ifdefs and range
checking w/ no macros.  Does this look better?  Below is the original
submit comment outlining the goal.  Thanks,

	Alex

   The patch below removes these warnings:

  CC      drivers/acpi/numa.o
drivers/acpi/numa.c: In function `acpi_table_print_srat_entry':
drivers/acpi/numa.c:55: warning: unused variable `p'
drivers/acpi/numa.c:65: warning: unused variable `p'
drivers/acpi/numa.c: In function `acpi_numa_init':
drivers/acpi/numa.c:179: warning: passing arg 2 of
`acpi_table_parse_srat' from incompatible pointer type
drivers/acpi/numa.c:182: warning: passing arg 2 of
`acpi_table_parse_srat' from incompatible pointer type

And propagates the MADT error checking code into the SRAT code.

Signed-off-by: Alex Williamson <alex.williamson-VXdhtT5mjnY@public.gmane.org>

===== drivers/acpi/numa.c 1.10 vs edited =====
--- 1.10/drivers/acpi/numa.c	2004-02-18 02:19:31 -07:00
+++ edited/drivers/acpi/numa.c	2004-08-10 16:58:37 -06:00
@@ -51,6 +51,7 @@
 	switch (header->type) {
 
 	case ACPI_SRAT_PROCESSOR_AFFINITY:
+#ifdef ACPI_DEBUG_OUTPUT
 	{
 		struct acpi_table_processor_affinity *p =
 			(struct acpi_table_processor_affinity*) header;
@@ -58,9 +59,11 @@
 		       p->apic_id, p->lsapic_eid, p->proximity_domain,
 		       p->flags.enabled?"enabled":"disabled"));
 	}
+#endif
 		break;
 
 	case ACPI_SRAT_MEMORY_AFFINITY:
+#ifdef ACPI_DEBUG_OUTPUT
 	{
 		struct acpi_table_memory_affinity *p =
 			(struct acpi_table_memory_affinity*) header;
@@ -70,6 +73,7 @@
 		       p->flags.enabled ? "enabled" : "disabled",
 		       p->flags.hot_pluggable ? " hot-pluggable" : ""));
 	}
+#endif
 		break;
 
 	default:
@@ -103,12 +107,14 @@
 
 
 static int __init
-acpi_parse_processor_affinity (acpi_table_entry_header *header)
+acpi_parse_processor_affinity (acpi_table_entry_header *header, unsigned long size)
 {
 	struct acpi_table_processor_affinity *processor_affinity;
 
 	processor_affinity = (struct acpi_table_processor_affinity*) header;
-	if (!processor_affinity)
+	if (!processor_affinity || (unsigned long)processor_affinity +
+	    sizeof(*processor_affinity) > size ||
+	    header->length != sizeof(*processor_affinity))
 		return -EINVAL;
 
 	acpi_table_print_srat_entry(header);
@@ -121,12 +127,14 @@
 
 
 static int __init
-acpi_parse_memory_affinity (acpi_table_entry_header *header)
+acpi_parse_memory_affinity (acpi_table_entry_header *header, unsigned long size)
 {
 	struct acpi_table_memory_affinity *memory_affinity;
 
 	memory_affinity = (struct acpi_table_memory_affinity*) header;
-	if (!memory_affinity)
+	if (!memory_affinity || (unsigned long)memory_affinity +
+	    sizeof(*memory_affinity) > size ||
+	    header->length != sizeof(*memory_affinity))
 		return -EINVAL;
 
 	acpi_table_print_srat_entry(header);





-------------------------------------------------------
SF.Net email is sponsored by Shop4tech.com-Lowest price on Blank Media
100pk Sonic DVD-R 4x for only $29 -100pk Sonic DVD+R for only $33
Save 50% off Retail on Ink & Toner - Free Shipping and Free Gift.
http://www.shop4tech.com/z/Inkjet_Cartridges/9_108_r285

Re: [ACPI] Re: [PATCH] cleanup ACPI numa warnings

[Jesse Barnes]

On Friday, August 20, 2004 2:55 pm, Alex Williamson wrote:
>    I'm not sure where we stand on this, sorry for the delay.  To recap,
> the first patch I submitted cleaned up the original functions, but moved
> the ugliness up into multi-line macros.  People didn't like the macros
> and suggested static inlines.  However, static inlines don't work for
> this application because the debug print needs state setup by the
> ACPI_FUNCTION_NAME call.  IMHO, it's not worth setting up that state in
> the static inline function for this little bit of cleanup.
>
>    So, I think we left it at nobody liked the macros and static inlines
> don't work.  General unhappiness.  Below is a patch that doesn't attempt
> to cleanup the original code, it just adds the #ifdefs and range
> checking w/ no macros.  Does this look better?  Below is the original
> submit comment outlining the goal.  Thanks,

Yes please.

Jesse

Re: [PATCH] cleanup ACPI numa warnings

[Alex Williamson]

On Sun, 2004-08-08 at 14:36 -0700, Randy.Dunlap wrote:
> On Sat, 7 Aug 2004 10:57:29 -0700 Paul Jackson wrote:
> 
> | > And there's nothing in CodingStyle that agrees with you that I could find.
> | 
> | >From the file Documentation/SubmittingPatches:
> | 
> |         3) 'static inline' is better than a macro
> |
> Oops.  Thanks, Paul.

   Ok, I was all set to switch to static inlines, but it doesn't work.
Compiling w/ debug on, _dbg is undefined, which is part of the
ACPI_DB_INFO macro, but it only gets setup by the ACPI_FUNCTION_NAME
macro.  Guess I got lucky by choosing to do it as a macro.  IMHO, it
doesn't really make sense to make the static inline functions more
complicated or hide where they're getting called to make this all work.
So, I think the choices are to stick with the ugly macros or put #ifdefs
around the code and essentially leave it the way it is.  Sorry I didn't
give it a more thorough look when originally questioned.  Better ideas?
Thanks,

	Alex

Re: [PATCH] cleanup ACPI numa warnings

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

On Sun, 2004-08-08 at 21:19, Alex Williamson wrote:
>    Ok, I was all set to switch to static inlines, but it doesn't work.
> Compiling w/ debug on, _dbg is undefined, which is part of the
> ACPI_DB_INFO macro, but it only gets setup by the ACPI_FUNCTION_NAME
> macro.  Guess I got lucky by choosing to do it as a macro.  IMHO, it
> doesn't really make sense to make the static inline functions more
> complicated or hide where they're getting called to make this all work.
> So, I think the choices are to stick with the ugly macros or put #ifdefs
> around the code and essentially leave it the way it is.  Sorry I didn't
> give it a more thorough look when originally questioned.  Better ideas?
> Thanks,

That code is already pretty hideous, so perhaps my original question
doesn't have that much impact.  The attached patch at least uses inline
functions.  It still has the #ifdefs, but what else do you expect for
debugging code?  Is this a feasible approach?

-- Dave

[-- Attachment #2: ACPI-warnings-1.patch --]
[-- Type: text/x-patch, Size: 3876 bytes --]

Only in acpi-warnings/drivers/acpi/: .Kconfig.swp
diff -ru clean/drivers/acpi/numa.c acpi-warnings/drivers/acpi/numa.c
--- clean/drivers/acpi/numa.c	2004-06-15 22:19:44.000000000 -0700
+++ acpi-warnings/drivers/acpi/numa.c	2004-08-08 21:46:58.000000000 -0700
@@ -38,6 +38,33 @@
 
 extern int __init acpi_table_parse_madt_family (enum acpi_table_id id, unsigned long madt_size, int entry_id, acpi_madt_entry_handler handler, unsigned int max_entries);
 
+#ifdef ACPI_DEBUG_OUTPUT
+static inline void acpi_print_srat_processor_affinity(struct acpi_table_processor_affinity *p)
+{
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Processor (id[0x%02x] "
+			"eid[0x%02x]) in proximity domain %d %s\n",
+	                 p->apic_id, p->lsapic_eid, p->proximity_domain,
+	                 p->flags.enabled?"enabled":"disabled")); }
+}
+
+static inline void acpi_print_srat_memory_affinity(struct acpi_table_memory_affinity *p)
+{
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Memory (0x%08x%08x length "
+	                 "0x%08x%08x type 0x%x) in proximity domain %d %s%s\n",
+	                 p->base_addr_hi, p->base_addr_lo, p->length_hi,
+	                 p->length_lo, p->memory_type, p->proximity_domain,
+	                 p->flags.enabled ? "enabled" : "disabled",
+	                 p->flags.hot_pluggable ? " hot-pluggable" : "")); }
+}
+#else
+static inline void acpi_print_srat_memory_affinity(acpi_table_entry_header *header) {}
+static inline void acpi_print_srat_processor_affinity(acpi_table_entry_header *header) {}
+#endif
+
+#define BAD_SRAT_ENTRY(entry, end) ( \
+	(!entry) || (unsigned long)entry + sizeof(*entry) > end ||  \
+	((acpi_table_entry_header *)entry)->length != sizeof(*entry))
+
 void __init
 acpi_table_print_srat_entry (
 	acpi_table_entry_header	*header)
@@ -51,27 +78,11 @@
 	switch (header->type) {
 
 	case ACPI_SRAT_PROCESSOR_AFFINITY:
-	{
-		struct acpi_table_processor_affinity *p =
-			(struct acpi_table_processor_affinity*) header;
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Processor (id[0x%02x] eid[0x%02x]) in proximity domain %d %s\n",
-		       p->apic_id, p->lsapic_eid, p->proximity_domain,
-		       p->flags.enabled?"enabled":"disabled"));
-	}
+		acpi_print_srat_processor_affinity((struct acpi_table_processor_affinity *)header);
 		break;
-
 	case ACPI_SRAT_MEMORY_AFFINITY:
-	{
-		struct acpi_table_memory_affinity *p =
-			(struct acpi_table_memory_affinity*) header;
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "SRAT Memory (0x%08x%08x length 0x%08x%08x type 0x%x) in proximity domain %d %s%s\n",
-		       p->base_addr_hi, p->base_addr_lo, p->length_hi, p->length_lo,
-		       p->memory_type, p->proximity_domain,
-		       p->flags.enabled ? "enabled" : "disabled",
-		       p->flags.hot_pluggable ? " hot-pluggable" : ""));
-	}
+		acpi_print_srat_memory_affinity((struct acpi_table_memory_affinity *)header);
 		break;
-
 	default:
 		printk(KERN_WARNING PREFIX "Found unsupported SRAT entry (type = 0x%x)\n",
 			header->type);
@@ -103,12 +114,12 @@
 
 
 static int __init
-acpi_parse_processor_affinity (acpi_table_entry_header *header)
+acpi_parse_processor_affinity (acpi_table_entry_header *header, unsigned long size)
 {
 	struct acpi_table_processor_affinity *processor_affinity;
 
 	processor_affinity = (struct acpi_table_processor_affinity*) header;
-	if (!processor_affinity)
+	if (BAD_SRAT_ENTRY(processor_affinity, size))
 		return -EINVAL;
 
 	acpi_table_print_srat_entry(header);
@@ -121,12 +132,12 @@
 
 
 static int __init
-acpi_parse_memory_affinity (acpi_table_entry_header *header)
+acpi_parse_memory_affinity (acpi_table_entry_header *header, unsigned long size)
 {
 	struct acpi_table_memory_affinity *memory_affinity;
 
 	memory_affinity = (struct acpi_table_memory_affinity*) header;
-	if (!memory_affinity)
+	if (BAD_SRAT_ENTRY(memory_affinity, size))
 		return -EINVAL;
 
 	acpi_table_print_srat_entry(header);

Re: [PATCH] cleanup ACPI numa warnings

[Alex Williamson]

On Sun, 2004-08-08 at 21:52 -0700, Dave Hansen wrote:
> On Sun, 2004-08-08 at 21:19, Alex Williamson wrote:
> >    Ok, I was all set to switch to static inlines, but it doesn't work.
> > Compiling w/ debug on, _dbg is undefined, which is part of the
> > ACPI_DB_INFO macro, but it only gets setup by the ACPI_FUNCTION_NAME
> > macro.  Guess I got lucky by choosing to do it as a macro.  IMHO, it
> > doesn't really make sense to make the static inline functions more
> > complicated or hide where they're getting called to make this all work.
> > So, I think the choices are to stick with the ugly macros or put #ifdefs
> > around the code and essentially leave it the way it is.  Sorry I didn't
> > give it a more thorough look when originally questioned.  Better ideas?
> > Thanks,
> 
> That code is already pretty hideous, so perhaps my original question
> doesn't have that much impact.  The attached patch at least uses inline
> functions.  It still has the #ifdefs, but what else do you expect for
> debugging code?  Is this a feasible approach?

   If you build with CONFIG_ACPI_DEBUG=y, you'll see the problem I was
trying to describe above with this approach.

drivers/acpi/numa.c: In function `acpi_print_srat_processor_affinity':
drivers/acpi/numa.c:44: error: `_dbg' undeclared (first use in this function)
drivers/acpi/numa.c:44: error: (Each undeclared identifier is reported only once
drivers/acpi/numa.c:44: error: for each function it appears in.)
drivers/acpi/numa.c: At top level:
drivers/acpi/numa.c:48: error: parse error before '}' token
drivers/acpi/numa.c: In function `acpi_print_srat_memory_affinity':
drivers/acpi/numa.c:52: error: `_dbg' undeclared (first use in this function)
drivers/acpi/numa.c: At top level:
drivers/acpi/numa.c:58: error: parse error before '}' token
make[2]: *** [drivers/acpi/numa.o] Error 1
make[1]: *** [drivers/acpi] Error 2
make: *** [drivers] Error 2

Re: [PATCH] cleanup ACPI numa warnings

[Dave Hansen]

On Sun, 2004-08-08 at 22:10, Alex Williamson wrote:
> On Sun, 2004-08-08 at 21:52 -0700, Dave Hansen wrote:
> > On Sun, 2004-08-08 at 21:19, Alex Williamson wrote:
> > >    Ok, I was all set to switch to static inlines, but it doesn't work.
> > > Compiling w/ debug on, _dbg is undefined, which is part of the
> > > ACPI_DB_INFO macro, but it only gets setup by the ACPI_FUNCTION_NAME
> > > macro.  Guess I got lucky by choosing to do it as a macro.  IMHO, it
> > > doesn't really make sense to make the static inline functions more
> > > complicated or hide where they're getting called to make this all work.
> > > So, I think the choices are to stick with the ugly macros or put #ifdefs
> > > around the code and essentially leave it the way it is.  Sorry I didn't
> > > give it a more thorough look when originally questioned.  Better ideas?
> > > Thanks,
> > 
> > That code is already pretty hideous, so perhaps my original question
> > doesn't have that much impact.  The attached patch at least uses inline
> > functions.  It still has the #ifdefs, but what else do you expect for
> > debugging code?  Is this a feasible approach?
> 
>    If you build with CONFIG_ACPI_DEBUG=y, you'll see the problem I was
> trying to describe above with this approach.
> 
> drivers/acpi/numa.c: In function `acpi_print_srat_processor_affinity':
> drivers/acpi/numa.c:44: error: `_dbg' undeclared (first use in this function)
> drivers/acpi/numa.c:44: error: (Each undeclared identifier is reported only once
> drivers/acpi/numa.c:44: error: for each function it appears in.)
> drivers/acpi/numa.c: At top level:
> drivers/acpi/numa.c:48: error: parse error before '}' token
> drivers/acpi/numa.c: In function `acpi_print_srat_memory_affinity':
> drivers/acpi/numa.c:52: error: `_dbg' undeclared (first use in this function)
> drivers/acpi/numa.c: At top level:
> drivers/acpi/numa.c:58: error: parse error before '}' token
> make[2]: *** [drivers/acpi/numa.o] Error 1
> make[1]: *** [drivers/acpi] Error 2
> make: *** [drivers] Error 2

OK, you win.  ACPI is so insanely disgusting already that you've proved
that you should go ahead and add as many multi-line #defines as you
can.  It can't possibly get any worse. :)

-- Dave



-------------------------------------------------------
This SF.Net email is sponsored by OSTG. Have you noticed the changes on
Linux.com, ITManagersJournal and NewsForge in the past few weeks? Now,
one more big change to announce. We are now OSTG- Open Source Technology
Group. Come see the changes on the new OSTG site. www.ostg.com