drivers/acpi: sizeof/sizeof array size calculations

PATCH In review Andi Drebes 7 participants 13 messages May 26, 2007 – Jun 15, 2007

Subsystems (2)

Kernel janitors ACPI

lore

Share & tools

lore

Andi Drebes

author

2007-05-26 10:39 UTC #0 lore raw

PATCH 1/1 drivers/acpi: sizeof/sizeof array size calculations

This patch replaces lines in the acpi subsystem that use sizeof/sizeof to determine the size of an array with the ARRAY_SIZE(x) macro.

Signed-off-by Andi Drebes <[email protected]>

---
Compile tested with allyesconfig on i386.
Diffed against Linus' git-tree.

drivers/acpi/resources/rsdump.c +1 -1

index 46da116..7b8e12d 100644
@@ -76,7 +76,7 @@ acpi_rs_dump_descriptor(void *resource, struct acpi_rsdump_info *table);
 
 #define ACPI_RSD_OFFSET(f)          (u8) ACPI_OFFSET (union acpi_resource_data,f)
 #define ACPI_PRT_OFFSET(f)          (u8) ACPI_OFFSET (struct acpi_pci_routing_table,f)
-#define ACPI_RSD_TABLE_SIZE(name)   (sizeof(name) / sizeof (struct acpi_rsdump_info))
+#define ACPI_RSD_TABLE_SIZE(name)   (ARRAY_SIZE(name))
 
 /*******************************************************************************
  *

drivers/acpi/tables/tbfadt.c +1 -1

index 1285e91..4d59de2 100644
@@ -104,7 +104,7 @@ static struct acpi_fadt_info fadt_info_table[] = {
 	 ACPI_FADT_OFFSET(gpe1_block_length), ACPI_FADT_SEPARATE_LENGTH}
 };
 
-#define ACPI_FADT_INFO_ENTRIES        (sizeof (fadt_info_table) / sizeof (struct acpi_fadt_info))
+#define ACPI_FADT_INFO_ENTRIES        (ARRAY_SIZE(fadt_info_table))
 
 /*******************************************************************************
  *

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Robert P. J. Day

2007-05-26 11:28 UTC #1 lore raw

Quoted 15 lines · Andi Drebes

This patch replaces lines in the acpi subsystem that use sizeof/sizeof to determine the size of an array with the ARRAY_SIZE(x) macro.
Signed-off-by Andi Drebes <[email protected]>
---
Compile tested with allyesconfig on i386.
Diffed against Linus' git-tree.
drivers/acpi/resources/rsdump.c +0 -0
index 46da116..7b8e12d 100644

drivers/acpi/resources/rsdump.c acpi_rs_dump_descriptor(void *resource, struct acpi_rsdump_info *table); · line 82

Quoted 8 lines

drivers/acpi/resources/rsdump.c +1 -1

@@ -76,7 +76,7 @@ acpi_rs_dump_descriptor(void *resource, struct acpi_rsdump_info *table);
 
 #define ACPI_RSD_OFFSET(f)          (u8) ACPI_OFFSET (union acpi_resource_data,f)
 #define ACPI_PRT_OFFSET(f)          (u8) ACPI_OFFSET (struct acpi_pci_routing_table,f)
-#define ACPI_RSD_TABLE_SIZE(name)   (sizeof(name) / sizeof (struct acpi_rsdump_info))
+#define ACPI_RSD_TABLE_SIZE(name)   (ARRAY_SIZE(name))
 
 /*******************************************************************************
  *

normally, what i would do in a case like the above is delete the
macro ACPI_RSD_TABLE_SIZE entirely, and replace all invocations of it with a direct invocation of ARRAY_SIZE. there seems to be little value in defining a whole new macro whose only purpose in life is to do what an already-existing macro does just as well and with more clarity.

but there's something else about that file that's just a bit weird.
note the form of most of those array definitions:

struct acpi_rsdump_info acpi_rs_dump_irq[6] = {
        {ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_irq), "IRQ", NULL},
        {ACPI_RSD_1BITFLAG, ACPI_RSD_OFFSET(irq.triggering), "Triggering",
         acpi_gbl_he_decode},
	...

what's the point of **hard-coding** the array size as "6", only to
use a macro in the very next line to calculate that same value? and that's done throughout those array definitions. why not just leave the hard-coded array size out, and let the macro take care of things?

rday

Signature · 11 lines

====================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA

http://fsdev.net/wiki/index.php?title=Main_Page
====================================
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Andi Drebes

author

2007-05-26 13:53 UTC #2 lore raw

<snip>

Quoted 11 lines

drivers/acpi/resources/rsdump.c +1 -1

index 46da116..7b8e12d 100644
@@ -76,7 +76,7 @@ acpi_rs_dump_descriptor(void *resource, struct acpi_rsdump_info *table);
 
 #define ACPI_RSD_OFFSET(f)          (u8) ACPI_OFFSET (union acpi_resource_data,f)
 #define ACPI_PRT_OFFSET(f)          (u8) ACPI_OFFSET (struct acpi_pci_routing_table,f)
-#define ACPI_RSD_TABLE_SIZE(name)   (sizeof(name) / sizeof (struct acpi_rsdump_info))
+#define ACPI_RSD_TABLE_SIZE(name)   (ARRAY_SIZE(name))

Quoted 9 lines

drivers/acpi/resources/rsdump.c acpi_rs_dump_descriptor(void *resource, struct acpi_rsdump_info *table); · line 82
Quoted 2 lines
drivers/acpi/resources/rsdump.c +0 -0
 /*******************************************************************************
  *
normally, what i would do in a case like the above is delete the
macro ACPI_RSD_TABLE_SIZE entirely, and replace all invocations of it with a direct invocation of ARRAY_SIZE. there seems to be little value in defining a whole new macro whose only purpose in life is to do what an already-existing macro does just as well and with more clarity.

OK. I just replaced all invocations of ACPI_RSD_TABLE_SIZE with ARRAY_SIZE and removed the definition.

Quoted 1 line

but there's something else about that file that's just a bit weird.

<snip>

Quoted 4 lines

what's the point of **hard-coding** the array size as "6", only to
use a macro in the very next line to calculate that same value? and that's done throughout those array definitions. why not just leave the hard-coded array size out, and let the macro take care of things?

I tried to figure out why the value is hardcoded in the _first entry_. Actually, the acpi_rs_dump_* array definitions are used in drivers/acpi/resources/rsinfo.c in another array called acpi_gbl_dump_resource_dispatch (Line 124). They are also defined
as externel variables in include/acpi/acresrc.h. But I think the point of the definitions in the header file is just to make them accessible fro rsinfo.c (because the only references in drivers/acpi/ are in drivers/acpi/resources/rsinfo.c and drivers/acpi/resources/rsdump.c (where they are defined)). acpi_gbl_dump_resource_dispatch seems o be referenced only once. That is in drivers/acpi/resources/rsdump.c in the function called acpi_rs_dump_resource_list:

acpi_rs_dump_descriptor(&resource_list->data,

					acpi_gbl_dump_resource_dispatch[type]);

acpi_rs_dump_descriptor is defined in the same file and finally uses the table. The hardcoded sizes of the arrays are used to walk through the array. There's also a comment about that:

/* First table entry must contain the table length (# of table entries) */

As acpi_rs_dump_descriptor only gets a pointer to an array like that, it's impossible for it to determine the size (or am I wrong?). So finally, I think the hardcoded values are really needed. What I dislike is that an ordinary acpi_rsdump_info entry in the array is used to store the size. So acpi_rsdump_info.offset is used in two ways. For the first entry this is the number of entries in the array. For all the following entries it is a real offset. Perhaps one should use a struct that contains an array for the following entries instead of using an array directly (or is this a common practice?). To get back to robert's comment: I think also that the hardcoded values *in brackets* should be removed. I couldn't find any code that relies on that. To determine the size, the first entry is used (see above).

However, there's another thing that confuses me. I tried to figure out how drivers/acpi/resources/rsdump.c is actually compiled. I inspected the Makefile which says:

obj-y := rsaddr.o rscreate.o rsinfo.o rsio.o rslist.o rsmisc.o rsxface.o \
rscalc.o rsirq.o rsmemory.o rsutils.o

obj-$(ACPI_FUTURE_USAGE) += rsdump.o

I couldn't find ACPI_FUTURE_USAGE in the configuration. I tried to add the object file diretly to obj-y which causes a lot of errors. I found this post when I searched the web for more information about the symbol:

http://mail.nl.linux.org/kernelnewbies/2006-07/msg00250.html

The message is pretty old (July 2006), but it endorses my assumption that this code isn't actually used. This is somewhat weird to me. Why is there code in the kernel tree that is reserved for future use. And why does it live there for already a year? I would really like to test my changes (even only by compilation).

Here's a new patch that totally removes the ACPI_RSD_TABLE_SIZE macro, aswell as the hardcoded array sizes in brackets at the beginning of a definition. In contrast to the original patch, this one only affects rsdump.c (I split this, because there's another issue about tbfadt.c pointed out by Richard Knutsson. The other changes will be in a reply to his message)

Cheers,
Andi

The changes haven't been tested yet (see above)

Signed-off-by Andi Drebes <[email protected]>

drivers/acpi/resources/rsdump.c +42 ~~-43~~

index 46da116..67a7511 100644
@@ -76,7 +76,6 @@ acpi_rs_dump_descriptor(void *resource, struct acpi_rsdump_info *table);
 
 #define ACPI_RSD_OFFSET(f)          (u8) ACPI_OFFSET (union acpi_resource_data,f)
 #define ACPI_PRT_OFFSET(f)          (u8) ACPI_OFFSET (struct acpi_pci_routing_table,f)
-#define ACPI_RSD_TABLE_SIZE(name)   (sizeof(name) / sizeof (struct acpi_rsdump_info))
 
 /*******************************************************************************
  *

@@ -87,8 +86,8 @@ acpi_rs_dump_descriptor(void *resource, struct acpi_rsdump_info *table);
  *
  ******************************************************************************/
 
-struct acpi_rsdump_info acpi_rs_dump_irq[6] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_irq), "IRQ", NULL},
+struct acpi_rsdump_info acpi_rs_dump_irq[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_irq), "IRQ", NULL},
 	{ACPI_RSD_1BITFLAG, ACPI_RSD_OFFSET(irq.triggering), "Triggering",
 	 acpi_gbl_he_decode},
 	{ACPI_RSD_1BITFLAG, ACPI_RSD_OFFSET(irq.polarity), "Polarity",

@@ -101,8 +100,8 @@ struct acpi_rsdump_info acpi_rs_dump_irq[6] = {
 	 "Interrupt List", NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_dma[6] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_dma), "DMA", NULL},
+struct acpi_rsdump_info acpi_rs_dump_dma[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_dma), "DMA", NULL},
 	{ACPI_RSD_2BITFLAG, ACPI_RSD_OFFSET(dma.type), "Speed",
 	 acpi_gbl_typ_decode},
 	{ACPI_RSD_1BITFLAG, ACPI_RSD_OFFSET(dma.bus_master), "Mastering",

@@ -115,8 +114,8 @@ struct acpi_rsdump_info acpi_rs_dump_dma[6] = {
 	 NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_start_dpf[3] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_start_dpf),
+struct acpi_rsdump_info acpi_rs_dump_start_dpf[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_start_dpf),
 	 "Start-Dependent-Functions", NULL},
 	{ACPI_RSD_2BITFLAG, ACPI_RSD_OFFSET(start_dpf.compatibility_priority),
 	 "Compatibility Priority", acpi_gbl_config_decode},

@@ -124,13 +123,13 @@ struct acpi_rsdump_info acpi_rs_dump_start_dpf[3] = {
 	 "Performance/Robustness", acpi_gbl_config_decode}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_end_dpf[1] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_end_dpf),
+struct acpi_rsdump_info acpi_rs_dump_end_dpf[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_end_dpf),
 	 "End-Dependent-Functions", NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_io[6] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_io), "I/O", NULL},
+struct acpi_rsdump_info acpi_rs_dump_io[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_io), "I/O", NULL},
 	{ACPI_RSD_1BITFLAG, ACPI_RSD_OFFSET(io.io_decode), "Address Decoding",
 	 acpi_gbl_io_decode},
 	{ACPI_RSD_UINT16, ACPI_RSD_OFFSET(io.minimum), "Address Minimum", NULL},

@@ -140,29 +139,29 @@ struct acpi_rsdump_info acpi_rs_dump_io[6] = {
 	 NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_fixed_io[3] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_fixed_io),
+struct acpi_rsdump_info acpi_rs_dump_fixed_io[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_fixed_io),
 	 "Fixed I/O", NULL},
 	{ACPI_RSD_UINT16, ACPI_RSD_OFFSET(fixed_io.address), "Address", NULL},
 	{ACPI_RSD_UINT8, ACPI_RSD_OFFSET(fixed_io.address_length),
 	 "Address Length", NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_vendor[3] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_vendor),
+struct acpi_rsdump_info acpi_rs_dump_vendor[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_vendor),
 	 "Vendor Specific", NULL},
 	{ACPI_RSD_UINT16, ACPI_RSD_OFFSET(vendor.byte_length), "Length", NULL},
 	{ACPI_RSD_LONGLIST, ACPI_RSD_OFFSET(vendor.byte_data[0]), "Vendor Data",
 	 NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_end_tag[1] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_end_tag), "EndTag",
+struct acpi_rsdump_info acpi_rs_dump_end_tag[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_end_tag), "EndTag",
 	 NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_memory24[6] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_memory24),
+struct acpi_rsdump_info acpi_rs_dump_memory24[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_memory24),
 	 "24-Bit Memory Range", NULL},
 	{ACPI_RSD_1BITFLAG, ACPI_RSD_OFFSET(memory24.write_protect),
 	 "Write Protect", acpi_gbl_rw_decode},

@@ -176,8 +175,8 @@ struct acpi_rsdump_info acpi_rs_dump_memory24[6] = {
 	 "Address Length", NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_memory32[6] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_memory32),
+struct acpi_rsdump_info acpi_rs_dump_memory32[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_memory32),
 	 "32-Bit Memory Range", NULL},
 	{ACPI_RSD_1BITFLAG, ACPI_RSD_OFFSET(memory32.write_protect),
 	 "Write Protect", acpi_gbl_rw_decode},

@@ -191,8 +190,8 @@ struct acpi_rsdump_info acpi_rs_dump_memory32[6] = {
 	 "Address Length", NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_fixed_memory32[4] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_fixed_memory32),
+struct acpi_rsdump_info acpi_rs_dump_fixed_memory32[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_fixed_memory32),
 	 "32-Bit Fixed Memory Range", NULL},
 	{ACPI_RSD_1BITFLAG, ACPI_RSD_OFFSET(fixed_memory32.write_protect),
 	 "Write Protect", acpi_gbl_rw_decode},

@@ -202,8 +201,8 @@ struct acpi_rsdump_info acpi_rs_dump_fixed_memory32[4] = {
 	 "Address Length", NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_address16[8] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_address16),
+struct acpi_rsdump_info acpi_rs_dump_address16[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_address16),
 	 "16-Bit WORD Address Space", NULL},
 	{ACPI_RSD_ADDRESS, 0, NULL, NULL},
 	{ACPI_RSD_UINT16, ACPI_RSD_OFFSET(address16.granularity), "Granularity",

@@ -219,8 +218,8 @@ struct acpi_rsdump_info acpi_rs_dump_address16[8] = {
 	{ACPI_RSD_SOURCE, ACPI_RSD_OFFSET(address16.resource_source), NULL, NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_address32[8] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_address32),
+struct acpi_rsdump_info acpi_rs_dump_address32[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_address32),
 	 "32-Bit DWORD Address Space", NULL},
 	{ACPI_RSD_ADDRESS, 0, NULL, NULL},
 	{ACPI_RSD_UINT32, ACPI_RSD_OFFSET(address32.granularity), "Granularity",

@@ -236,8 +235,8 @@ struct acpi_rsdump_info acpi_rs_dump_address32[8] = {
 	{ACPI_RSD_SOURCE, ACPI_RSD_OFFSET(address32.resource_source), NULL, NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_address64[8] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_address64),
+struct acpi_rsdump_info acpi_rs_dump_address64[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_address64),
 	 "64-Bit QWORD Address Space", NULL},
 	{ACPI_RSD_ADDRESS, 0, NULL, NULL},
 	{ACPI_RSD_UINT64, ACPI_RSD_OFFSET(address64.granularity), "Granularity",

@@ -253,8 +252,8 @@ struct acpi_rsdump_info acpi_rs_dump_address64[8] = {
 	{ACPI_RSD_SOURCE, ACPI_RSD_OFFSET(address64.resource_source), NULL, NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_ext_address64[8] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_ext_address64),
+struct acpi_rsdump_info acpi_rs_dump_ext_address64[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_ext_address64),
 	 "64-Bit Extended Address Space", NULL},
 	{ACPI_RSD_ADDRESS, 0, NULL, NULL},
 	{ACPI_RSD_UINT64, ACPI_RSD_OFFSET(ext_address64.granularity),

@@ -271,8 +270,8 @@ struct acpi_rsdump_info acpi_rs_dump_ext_address64[8] = {
 	 "Type-Specific Attribute", NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_ext_irq[8] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_ext_irq),
+struct acpi_rsdump_info acpi_rs_dump_ext_irq[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_ext_irq),
 	 "Extended IRQ", NULL},
 	{ACPI_RSD_1BITFLAG, ACPI_RSD_OFFSET(extended_irq.producer_consumer),
 	 "Type", acpi_gbl_consume_decode},

@@ -290,8 +289,8 @@ struct acpi_rsdump_info acpi_rs_dump_ext_irq[8] = {
 	 "Interrupt List", NULL}
 };
 
-struct acpi_rsdump_info acpi_rs_dump_generic_reg[6] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_generic_reg),
+struct acpi_rsdump_info acpi_rs_dump_generic_reg[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_generic_reg),
 	 "Generic Register", NULL},
 	{ACPI_RSD_UINT8, ACPI_RSD_OFFSET(generic_reg.space_id), "Space ID",
 	 NULL},

@@ -307,8 +306,8 @@ struct acpi_rsdump_info acpi_rs_dump_generic_reg[6] = {
 /*
  * Tables used for common address descriptor flag fields
  */
-static struct acpi_rsdump_info acpi_rs_dump_general_flags[5] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_general_flags), NULL,
+static struct acpi_rsdump_info acpi_rs_dump_general_flags[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_general_flags), NULL,
 	 NULL},
 	{ACPI_RSD_1BITFLAG, ACPI_RSD_OFFSET(address.producer_consumer),
 	 "Consumer/Producer", acpi_gbl_consume_decode},

@@ -320,8 +319,8 @@ static struct acpi_rsdump_info acpi_rs_dump_general_flags[5] = {
 	 "Max Relocatability", acpi_gbl_max_decode}
 };
 
-static struct acpi_rsdump_info acpi_rs_dump_memory_flags[5] = {
-	{ACPI_RSD_LITERAL, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_memory_flags),
+static struct acpi_rsdump_info acpi_rs_dump_memory_flags[] = {
+	{ACPI_RSD_LITERAL, ARRAY_SIZE(acpi_rs_dump_memory_flags),
 	 "Resource Type", (void *)"Memory Range"},
 	{ACPI_RSD_1BITFLAG, ACPI_RSD_OFFSET(address.info.mem.write_protect),
 	 "Write Protect", acpi_gbl_rw_decode},

@@ -333,8 +332,8 @@ static struct acpi_rsdump_info acpi_rs_dump_memory_flags[5] = {
 	 "Translation", acpi_gbl_ttp_decode}
 };
 
-static struct acpi_rsdump_info acpi_rs_dump_io_flags[4] = {
-	{ACPI_RSD_LITERAL, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_io_flags),
+static struct acpi_rsdump_info acpi_rs_dump_io_flags[] = {
+	{ACPI_RSD_LITERAL, ARRAY_SIZE(acpi_rs_dump_io_flags),
 	 "Resource Type", (void *)"I/O Range"},
 	{ACPI_RSD_2BITFLAG, ACPI_RSD_OFFSET(address.info.io.range_type),
 	 "Range Type", acpi_gbl_rng_decode},

@@ -347,8 +346,8 @@ static struct acpi_rsdump_info acpi_rs_dump_io_flags[4] = {
 /*
  * Table used to dump _PRT contents
  */
-static struct acpi_rsdump_info acpi_rs_dump_prt[5] = {
-	{ACPI_RSD_TITLE, ACPI_RSD_TABLE_SIZE(acpi_rs_dump_prt), NULL, NULL},
+static struct acpi_rsdump_info acpi_rs_dump_prt[] = {
+	{ACPI_RSD_TITLE, ARRAY_SIZE(acpi_rs_dump_prt), NULL, NULL},
 	{ACPI_RSD_UINT64, ACPI_PRT_OFFSET(address), "Address", NULL},
 	{ACPI_RSD_UINT32, ACPI_PRT_OFFSET(pin), "Pin", NULL},
 	{ACPI_RSD_STRING, ACPI_PRT_OFFSET(source[0]), "Source", NULL},

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Richard Knutsson

2007-05-26 11:35 UTC #3 lore raw

In reply to Andi Drebes

Quoted 15 lines · Andi Drebes

This patch replaces lines in the acpi subsystem that use sizeof/sizeof to determine the size of an array with the ARRAY_SIZE(x) macro.
Signed-off-by Andi Drebes <[email protected]>
---
Compile tested with allyesconfig on i386.
Diffed against Linus' git-tree.
drivers/acpi/resources/rsdump.c +0 -0
index 46da116..7b8e12d 100644

drivers/acpi/resources/rsdump.c acpi_rs_dump_descriptor(void *resource, struct acpi_rsdump_info *table); · line 80

Quoted 6 lines

drivers/acpi/resources/rsdump.c +1 -1

@@ -76,7 +76,7 @@ acpi_rs_dump_descriptor(void *resource, struct acpi_rsdump_info *table);
 
 #define ACPI_RSD_OFFSET(f)          (u8) ACPI_OFFSET (union acpi_resource_data,f)
 #define ACPI_PRT_OFFSET(f)          (u8) ACPI_OFFSET (struct acpi_pci_routing_table,f)
-#define ACPI_RSD_TABLE_SIZE(name)   (sizeof(name) / sizeof (struct acpi_rsdump_info))
+#define ACPI_RSD_TABLE_SIZE(name)   (ARRAY_SIZE(name))

Any reason to not just replace ACPI_RSD_TABLE_SIZE with ARRAY_SIZE? Got just 21 instances of it in the file (+ the define) and no more in the rest of the tree.

Quoted 8 lines

drivers/acpi/resources/rsdump.c +0 -0
 /*******************************************************************************
  *
drivers/acpi/tables/tbfadt.c +0 -0
index 1285e91..4d59de2 100644

drivers/acpi/tables/tbfadt.c static struct acpi_fadt_info fadt_info_table[] = { · line 108

Quoted 6 lines

drivers/acpi/tables/tbfadt.c +1 -1

@@ -104,7 +104,7 @@ static struct acpi_fadt_info fadt_info_table[] = {
 	 ACPI_FADT_OFFSET(gpe1_block_length), ACPI_FADT_SEPARATE_LENGTH}
 };
 
-#define ACPI_FADT_INFO_ENTRIES        (sizeof (fadt_info_table) / sizeof (struct acpi_fadt_info))
+#define ACPI_FADT_INFO_ENTRIES        (ARRAY_SIZE(fadt_info_table))

Normally I think this is ok when it is just a "constant", since the name of it may be more descriptive, but it is just used twice and I find (imho) ARRAY_SIZE(fadt_info_table) easier to understand.

drivers/acpi/tables/tbfadt.c static struct acpi_fadt_info fadt_info_table[] = { · line 110

Quoted 4 lines

drivers/acpi/tables/tbfadt.c +0 -0

 /*******************************************************************************
  *

cu
Richard Knutsson

Andi Drebes

author

2007-05-26 13:58 UTC #4 lore raw

<snip>

Quoted 6 lines

drivers/acpi/resources/rsdump.c +0 -0

index 46da116..7b8e12d 100644
@@ -76,7 +76,7 @@ acpi_rs_dump_descriptor(void *resource, struct acpi_rsdump_info *table);

Quoted 8 lines

drivers/acpi/resources/rsdump.c acpi_rs_dump_descriptor(void *resource, struct acpi_rsdump_info *table); · line 80
Quoted 5 lines
drivers/acpi/resources/rsdump.c +1 -1
 #define ACPI_RSD_OFFSET(f)          (u8) ACPI_OFFSET (union acpi_resource_data,f)
 #define ACPI_PRT_OFFSET(f)          (u8) ACPI_OFFSET (struct acpi_pci_routing_table,f)
-#define ACPI_RSD_TABLE_SIZE(name)   (sizeof(name) / sizeof (struct acpi_rsdump_info))
+#define ACPI_RSD_TABLE_SIZE(name)   (ARRAY_SIZE(name))
 
Any reason to not just replace ACPI_RSD_TABLE_SIZE with ARRAY_SIZE? Got just 21 instances of it in the file (+ the define) and no more in the rest of the tree.

I agree with that. Robert P. J. Day pointed out something similiar. For a more verbose answer to that see the reply to his message.

Quoted 10 lines

drivers/acpi/resources/rsdump.c +0 -0

 /*******************************************************************************
  *

drivers/acpi/tables/tbfadt.c +0 -0

index 1285e91..4d59de2 100644
@@ -104,7 +104,7 @@ static struct acpi_fadt_info fadt_info_table[] = {
 	 ACPI_FADT_OFFSET(gpe1_block_length), ACPI_FADT_SEPARATE_LENGTH}
 };

Quoted 6 lines

drivers/acpi/tables/tbfadt.c static struct acpi_fadt_info fadt_info_table[] = { · line 108
Quoted 3 lines
drivers/acpi/tables/tbfadt.c +1 -1
-#define ACPI_FADT_INFO_ENTRIES        (sizeof (fadt_info_table) / sizeof (struct acpi_fadt_info))
+#define ACPI_FADT_INFO_ENTRIES        (ARRAY_SIZE(fadt_info_table))
 
Normally I think this is ok when it is just a "constant", since the name of it may be more descriptive, but it is just used twice and I find (imho) ARRAY_SIZE(fadt_info_table) easier to understand.

I totally agree with that. Here's a new patch that totally removes the ACPI_FADT_INFO_ENTRIES macro. In contrast to the original patch, this one only affects tbfadt.c (I split this, because there's another issue about rsdump.c pointed out by Robert P. J. Day. The other changes will be in a reply to his message)

Tested by compilation.

Signed-off-by Andi Drebes <[email protected]>

drivers/acpi/tables/tbfadt.c +2 -4

index 1285e91..ba6d4a1 100644
@@ -104,8 +104,6 @@ static struct acpi_fadt_info fadt_info_table[] = {
 	 ACPI_FADT_OFFSET(gpe1_block_length), ACPI_FADT_SEPARATE_LENGTH}
 };
 
-#define ACPI_FADT_INFO_ENTRIES        (sizeof (fadt_info_table) / sizeof (struct acpi_fadt_info))
-
 /*******************************************************************************
  *
  * FUNCTION:    acpi_tb_init_generic_address

@@ -295,7 +293,7 @@ static void acpi_tb_convert_fadt(void)
 	 * Expand the 32-bit V1.0 addresses to the 64-bit "X" generic address
 	 * structures as necessary.
 	 */
-	for (i = 0; i < ACPI_FADT_INFO_ENTRIES; i++) {
+	for (i = 0; i < ARRAY_SIZE(fadt_info_table); i++) {
 		target  		    ACPI_ADD_PTR(struct acpi_generic_address, &acpi_gbl_FADT,
 				 fadt_info_table[i].target);

@@ -392,7 +390,7 @@ static void acpi_tb_validate_fadt(void)
 
 	/* Examine all of the 64-bit extended address fields (X fields) */
 
-	for (i = 0; i < ACPI_FADT_INFO_ENTRIES; i++) {
+	for (i = 0; i < ARRAY_SIZE(fadt_info_table); i++) {
 
 		/* Generate pointers to the 32-bit and 64-bit addresses and get the length */

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Len Brown

reviewer

2007-05-30 19:25 UTC #5 lore raw

Quoted 2 lines

Quoted 2 lines
Any reason to not just replace ACPI_RSD_TABLE_SIZE with ARRAY_SIZE?

Probably because ARRAY_SIZE doesn't exist in ACPICA, which is where this code comes from...

When we change syntax in ACPICA files in Linux to make it more "beautiful", then it creates more work for me -- as forever on, that syntax difference must be manually compared to upstream ACPICA and Linux -- and that syntax difference causes upstream patches to no longer apply and require hand merging.

If you can license your patches to ACPICA files (drivers/acpi/*/*) back to Intel to re-distribute under _both_ licenses at the top of the file, then I'll be happy to apply them. Otherwise, I'm really not excited about creating work for no functional benefit.

thanks,
-Len
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Christoph Hellwig

2007-05-31 09:56 UTC #6 lore raw

Re: [KJ] [PATCH] drivers/acpi: sizeof/sizeof array size calculations replaced with ARRAY_SIZE

Quoted 11 lines · Len Brown

Quoted 2 lines
Quoted 2 lines
Any reason to not just replace ACPI_RSD_TABLE_SIZE with ARRAY_SIZE?
Probably because ARRAY_SIZE doesn't exist in ACPICA, which is where this code comes from...
When we change syntax in ACPICA files in Linux to make it more "beautiful", then it creates more work for me -- as forever on, that syntax difference must be manually compared to upstream ACPICA and Linux -- and that syntax difference causes upstream patches to no longer apply and require hand merging.

Or we could stop that ACPCICA crap ASAP. The acpi code not only looks like crap because of that but it's buggy as hell now.

Intel is claiming how great linux is and how supportive is to it, but can't even write proper code for their abomination of a firmware standard. This is really more than dishonest. Please take patches to get acpi into shape and stop this complaining.

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Pavel Machek

2007-06-10 10:57 UTC #7 lore raw

Re: [KJ] [PATCH] drivers/acpi: sizeof/sizeof array size calculations replaced with ARRAY_SIZE

Hi!

Quoted 5 lines

Quoted 5 lines
Quoted 1 line
Quoted 1 line
Any reason to not just replace ACPI_RSD_TABLE_SIZE with ARRAY_SIZE?
Probably because ARRAY_SIZE doesn't exist in ACPICA, which is where this code comes from...

Quoted 8 lines

Quoted 5 lines
When we change syntax in ACPICA files in Linux to make it more "beautiful", then it creates more work for me -- as forever on, that syntax difference must be manually compared to upstream ACPICA and Linux -- and that syntax difference causes upstream patches to no longer apply and require hand merging.
Or we could stop that ACPCICA crap ASAP. The acpi code not only looks like crap because of that but it's buggy as hell now.

+1.

Len, acpi subsystem is old enough to live by kernel standards, and important enough that it should look&feel like a kernel code. It also does not seem to change quickly, so merging patches should not be a big deal.
Pavel

Signature · 6 lines

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Bjorn Helgaas

2007-06-10 21:44 UTC #8 lore raw

Re: [KJ] [PATCH] drivers/acpi: sizeof/sizeof array size calculations replaced with ARRAY_SIZE

Quoted 14 lines · Pavel Machek

Quoted 5 lines
Quoted 5 lines
Quoted 1 line
Quoted 1 line
Any reason to not just replace ACPI_RSD_TABLE_SIZE with ARRAY_SIZE?
Probably because ARRAY_SIZE doesn't exist in ACPICA, which is where this code comes from...
Quoted 8 lines
Quoted 5 lines
When we change syntax in ACPICA files in Linux to make it more "beautiful", then it creates more work for me -- as forever on, that syntax difference must be manually compared to upstream ACPICA and Linux -- and that syntax difference causes upstream patches to no longer apply and require hand merging.
Or we could stop that ACPCICA crap ASAP. The acpi code not only looks like crap because of that but it's buggy as hell now.

Quoted 6 lines

+1.
Len, acpi subsystem is old enough to live by kernel standards, and important enough that it should look&feel like a kernel code. It also does not seem to change quickly, so merging patches should not be a big deal.

I agree the ACPI CA is a nuisance. But in this case, we're making a mountain out of a molehill. I suspect that if somebody spent the 15 minutes to make the ARRAY_SIZE patch work in both the Linux ACPI CA and the generic Intel one and license it appropriately, Len would happily apply the patch.
_______________________________________________
Kernel-janitors mailing list [email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Andi Drebes

author

2007-06-12 18:41 UTC #9 lore raw

Re: [KJ] [PATCH] drivers/acpi: sizeof/sizeof array size calculations replaced with ARRAY_SIZE

First off: sorry for my late answer.

Quoted 5 lines

I agree the ACPI CA is a nuisance. But in this case, we're making a mountain out of a molehill. I suspect that if somebody spent the 15 minutes to make the ARRAY_SIZE patch work in both the Linux ACPI CA and the generic Intel one and license it appropriately, Len would happily apply the patch.

I hope I got everything right. Here's what I understood so far:

The ACPI Subsystem in the kernel is derived from intel sources. I searched the web for information about that and finally found

http://www.intel.com/technology/iapc/acpi/downloads.htm

Is that the right one? So what I would have to do in order to "make the ARRAY_SIZE patch work in both the Linux ACPI CA and the generic Intel one" is to modify those sources aswell, send a patch to Intel and another one back to the lkml.

Is that right? Or am I totally wrong?

Cheers,
Andi
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Bjorn Helgaas

2007-06-12 18:53 UTC #10 lore raw

Re: [KJ] [PATCH] drivers/acpi: sizeof/sizeof array size calculations replaced with ARRAY_SIZE

Quoted 12 lines · Andi Drebes

First off: sorry for my late answer.
Quoted 5 lines
I agree the ACPI CA is a nuisance. But in this case, we're making a mountain out of a molehill. I suspect that if somebody spent the 15 minutes to make the ARRAY_SIZE patch work in both the Linux ACPI CA and the generic Intel one and license it appropriately, Len would happily apply the patch.
I hope I got everything right. Here's what I understood so far:
The ACPI Subsystem in the kernel is derived from intel sources. I searched the web for information about that and finally found

Quoted 7 lines

http://www.intel.com/technology/iapc/acpi/downloads.htm
Is that the right one? So what I would have to do in order to "make the ARRAY_SIZE patch work in both the Linux ACPI CA and the generic Intel one" is to modify those sources aswell, send a patch to Intel and another one back to the lkml.

You found the right one. The ACPI CA in Linux is really the same as the one from Intel. Len integrates the Intel one into Linux periodically. He uses scripts or something to format it so it looks more like Linux.

I think Len sometimes takes patches against the "Linux-ized" ACPI CA. He probably has to apply those by hand to the Intel one, which is the real "upstream" in this case. So it might be a bit easier for him if you generated a diff against the Intel version. Then it would show up in Linux, too, in the fullness of time.

In any event, you want to end up with one patch, not two.

You probably would have to add something like:

  #ifndef ARRAY_SIZE
  #define ARRAY_SIZE(x) ...
  #endif

somewhere in the ACPI CA header files. That way it will work in non-Linux OSes as well.

Bjorn
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Andi Drebes

author

2007-06-13 21:21 UTC #11 lore raw

Re: [KJ] [PATCH] drivers/acpi: sizeof/sizeof array size calculations replaced with ARRAY_SIZE

<snip>

Quoted 4 lines

You found the right one. The ACPI CA in Linux is really the same as the one from Intel. Len integrates the Intel one into Linux periodically. He uses scripts or something to format it so it looks more like Linux.

Ok. That's what I thought so far.

Quoted 4 lines

I think Len sometimes takes patches against the "Linux-ized" ACPI CA. He probably has to apply those by hand to the Intel one, which is the real "upstream" in this case. So it might be a bit easier for him if you generated a diff against the Intel version.

OK. I will do this in the next time.

Quoted 1 line

In any event, you want to end up with one patch, not two.

Ok. I thought of two patches because I didn't know how strong the sources from the ACPI CA are modified.

Quoted 8 lines

You probably would have to add something like:
  #ifndef ARRAY_SIZE
  #define ARRAY_SIZE(x) ...
  #endif
somewhere in the ACPI CA header files. That way it will work in non-Linux OSes as well.

Thanks so far,
Andi
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Andi Drebes

author

2007-06-15 17:56 UTC #12 lore raw

Re: [KJ] [PATCH] drivers/acpi: sizeof/sizeof array size calculations replaced with ARRAY_SIZE

In reply to Bjorn Helgaas

<snip>

Quoted 2 lines

You found the right one. The ACPI CA in Linux is really the same as the one from Intel.

Hmmm. The file that I downloaded recently is acpica-unix-20061109. It seems to be quite old. There are some differences between the contents
of that archive and the acpi files in the kernel. I hope I really got the right version.

Quoted 3 lines

So it might be a bit easier for
him if you generated a diff against the Intel version. Then it would show up in Linux, too, in the fullness of time.

Ok. The new patch can be found in this message.
I tried to figure out where to put the definition of the ARRAY_SIZE macro. I searched a little bit in the include directory, but I couldn't find a place that satisfied me to a hundred percent. I chose acpi.h, because this seems to be file that is most general. The problem is, that the file only consists of lines that include other header files and the newly added definition for ARRAY_SIZE breaks this consistency in some way. Perhaps Len could give me a hint where to put it.

Ok, here's the new patch:

Diffed against the intel acpica implementation "acpica-unix-20061109". Tested by compilation.

Signed-off-by Andi Drebes <[email protected]>

---

include/acpi.h +6 -0

index ddaeec4..9de744c 100644
@@ -139,5 +139,11 @@
 #include "achware.h"            /* Hardware defines and interfaces */
 #include "acutils.h"            /* Utility interfaces */
 
+#ifndef ARRAY_SIZE
+/*
+ * ARRAY_SIZE calculates the size of a statically initialized array
+ */
+#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]))
+#endif
 
 #endif /* __ACPI_H__ */

resources/rsdump.c +21 ~~-23~~

136 lines not on this page · raw message

tables/tbfadt.c +2 -5

index a1afd6d..04f2fac 100644
@@ -189,9 +189,6 @@ static ACPI_FADT_INFO     FadtInfoTable[]                              ACPI_FADT_OFFSET (Gpe1BlockLength),     ACPI_FADT_SEPARATE_LENGTH}
 };
 
-#define ACPI_FADT_INFO_ENTRIES        (sizeof (FadtInfoTable) / sizeof (ACPI_FADT_INFO))
-
-
 /*******************************************************************************
  *
  * FUNCTION:    AcpiTbInitGenericAddress

@@ -398,7 +395,7 @@ AcpiTbConvertFadt (
      * Expand the 32-bit V1.0 addresses to the 64-bit "X" generic address
      * structures as necessary.
      */
-    for (i = 0; i < ACPI_FADT_INFO_ENTRIES; i++)
+    for (i = 0; i < ARRAY_SIZE(FadtInfoTable); i++)
     {
         Target = ACPI_ADD_PTR (
             ACPI_GENERIC_ADDRESS, &AcpiGbl_FADT, FadtInfoTable[i].Target);

@@ -471,7 +468,7 @@ AcpiTbValidateFadt (
 
     /* Examine all of the 64-bit extended address fields (X fields) */
 
-    for (i = 0; i < ACPI_FADT_INFO_ENTRIES; i++)
+    for (i = 0; i < ARRAY_SIZE(FadtInfoTable); i++)
     {
         /* Generate pointers to the 32-bit and 64-bit addresses and get the length */

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors