[RGC PATCH 0/2] split file2alias using elf sections

[RGC PATCH 0/2] split file2alias using elf sections

[Alessandro Rubini]

When adding a new bus type with autoloading of modules, people should
always change the global scripts/mod/file2alias.c source file, whereas
most of the new code is just new files and individual Makefile/Kconfig
lines.

The first patch turns all the "normal" alias generation in a
table-driven loop. The second patch moves a few alias types out of the
main file2alias.c source, as a demonstration that the thing works.

I didn't move all bus/alias types out of the main file, as it's a huge
work. But if the approach is going to be accepted I can do that (or
happily leave the task to who volunteers).

/alessandro

Alessandro Rubini (2):
  modpost: use table-lookup to build module aliases
  modpost: use config and ELF sections to build file2alias

 scripts/mod/Makefile        |   13 ++-
 scripts/mod/alias_acpi.c    |   14 ++
 scripts/mod/alias_bcma.c    |   25 ++++
 scripts/mod/alias_pci.c     |   49 +++++++
 scripts/mod/alias_spi.c     |   15 +++
 scripts/mod/device_switch.h |   47 +++++++
 scripts/mod/file2alias.c    |  294 ++++++++-----------------------------------
 scripts/mod/modpost.h       |   18 +++
 scripts/mod/modpost.lds     |    9 ++
 9 files changed, 243 insertions(+), 241 deletions(-)
 create mode 100644 scripts/mod/alias_acpi.c
 create mode 100644 scripts/mod/alias_bcma.c
 create mode 100644 scripts/mod/alias_pci.c
 create mode 100644 scripts/mod/alias_spi.c
 create mode 100644 scripts/mod/device_switch.h
 create mode 100644 scripts/mod/modpost.lds

[RFC PATCH 1/2] modpost: use table-lookup to build module aliases

[Alessandro Rubini]

The function handle_moddevtable was a huge "else if" thing. This
patch places the lookup in a table to reduce code and increase data.

Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
Acked-by: Samuel I. Gonsalvez <siglesia@cern.ch>
Acked-by: Manohar Vanga <manohar.vanga@cern.ch>
---
 scripts/mod/file2alias.c |  166 +++++++++++++++++-----------------------------
 1 files changed, 60 insertions(+), 106 deletions(-)

diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index f936d1f..1247bc9 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -913,6 +913,51 @@ static void do_table(void *symval, unsigned long size,
 	}
 }
 
+/* This array collects all instances that use the generic do_table above */
+struct devtable_switch {
+	const char *device_id;
+	const char *symname;
+	unsigned long id_size;
+	void *function;
+};
+#define E(id, name, structname, f) {id, name, sizeof(struct structname), f}
+
+static struct devtable_switch devtable_switch[] = {
+	E("pci", "__mod_pci_device_table", pci_device_id, do_pci_entry),
+	E("hid", "__mod_hid_device_table", hid_device_id, do_hid_entry),
+	E("ieee1394", "__mod_ieee1394_device_table",
+	  ieee1394_device_id, do_ieee1394_entry),
+	E("ccw", "__mod_ccw_device_table", ccw_device_id, do_ccw_entry),
+	E("ap", "__mod_ap_device_table", ap_device_id, do_ap_entry),
+	E("css", "__mod_css_device_table", css_device_id, do_css_entry),
+	E("seio", "__mod_serio_device_table", serio_device_id, do_serio_entry),
+	E("acpi", "__mod_acpi_device_table", acpi_device_id, do_acpi_entry),
+	E("pcmcia", "__mod_pcmcia_device_table",
+	  pcmcia_device_id, do_pcmcia_entry),
+	E("of", "__mod_of_device_table", of_device_id, do_of_entry),
+	E("vio", "__mod_vio_device_table", vio_device_id, do_vio_entry),
+	E("input", "__mod_input_device_table", input_device_id, do_input_entry),
+	E("eisa", "__mod_eisa_device_table", eisa_device_id, do_eisa_entry),
+	E("parisc", "__mod_parisc_device_table",
+	  parisc_device_id, do_parisc_entry),
+	E("sdio", "__mod_sdio_device_table", sdio_device_id, do_sdio_entry),
+	E("ssb", "__mod_ssb_device_table", ssb_device_id, do_ssb_entry),
+	E("bcma", "__mod_bcma_device_table", bcma_device_id, do_bcma_entry),
+	E("virtio", "__mod_virtio_device_table",
+	  virtio_device_id, do_virtio_entry),
+	E("vmbus", "__mod_vmbus_device_table",
+	  hv_vmbus_device_id, do_vmbus_entry),
+	E("i2c", "__mod_i2c_device_table", i2c_device_id, do_i2c_entry),
+	E("spi", "__mod_spi_device_table", spi_device_id, do_spi_entry),
+	E("dmi", "__mod_dmi_device_table", dmi_system_id, do_dmi_entry),
+	E("platform", "__mod_platform_device_table",
+	  platform_device_id, do_platform_entry),
+	E("mdio", "__mod_mdio_device_table", mdio_device_id, do_mdio_entry),
+	E("zorro", "__mod_zorro_device_table", zorro_device_id, do_zorro_entry),
+	E("isa", "__mod_isapnp_device_table",
+	  isapnp_device_id, do_isapnp_entry)
+};
+
 /* Create MODULE_ALIAS() statements.
  * At this time, we cannot write the actual output C source yet,
  * so we write into the mod->dev_table_buf buffer. */
@@ -936,117 +981,26 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
 			+ sym->st_value;
 	}
 
-	if (sym_is(symname, "__mod_pci_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct pci_device_id), "pci",
-			 do_pci_entry, mod);
-	else if (sym_is(symname, "__mod_usb_device_table"))
-		/* special case to handle bcdDevice ranges */
+	/* First handle the "special" cases */
+	if (sym_is(symname, "__mod_usb_device_table"))
 		do_usb_table(symval, sym->st_size, mod);
-	else if (sym_is(symname, "__mod_hid_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct hid_device_id), "hid",
-			 do_hid_entry, mod);
-	else if (sym_is(symname, "__mod_ieee1394_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct ieee1394_device_id), "ieee1394",
-			 do_ieee1394_entry, mod);
-	else if (sym_is(symname, "__mod_ccw_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct ccw_device_id), "ccw",
-			 do_ccw_entry, mod);
-	else if (sym_is(symname, "__mod_ap_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct ap_device_id), "ap",
-			 do_ap_entry, mod);
-	else if (sym_is(symname, "__mod_css_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct css_device_id), "css",
-			 do_css_entry, mod);
-	else if (sym_is(symname, "__mod_serio_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct serio_device_id), "serio",
-			 do_serio_entry, mod);
-	else if (sym_is(symname, "__mod_acpi_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct acpi_device_id), "acpi",
-			 do_acpi_entry, mod);
 	else if (sym_is(symname, "__mod_pnp_device_table"))
 		do_pnp_device_entry(symval, sym->st_size, mod);
 	else if (sym_is(symname, "__mod_pnp_card_device_table"))
 		do_pnp_card_entries(symval, sym->st_size, mod);
-	else if (sym_is(symname, "__mod_pcmcia_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct pcmcia_device_id), "pcmcia",
-			 do_pcmcia_entry, mod);
-        else if (sym_is(symname, "__mod_of_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct of_device_id), "of",
-			 do_of_entry, mod);
-        else if (sym_is(symname, "__mod_vio_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct vio_device_id), "vio",
-			 do_vio_entry, mod);
-	else if (sym_is(symname, "__mod_input_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct input_device_id), "input",
-			 do_input_entry, mod);
-	else if (sym_is(symname, "__mod_eisa_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct eisa_device_id), "eisa",
-			 do_eisa_entry, mod);
-	else if (sym_is(symname, "__mod_parisc_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct parisc_device_id), "parisc",
-			 do_parisc_entry, mod);
-	else if (sym_is(symname, "__mod_sdio_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct sdio_device_id), "sdio",
-			 do_sdio_entry, mod);
-	else if (sym_is(symname, "__mod_ssb_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct ssb_device_id), "ssb",
-			 do_ssb_entry, mod);
-	else if (sym_is(symname, "__mod_bcma_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct bcma_device_id), "bcma",
-			 do_bcma_entry, mod);
-	else if (sym_is(symname, "__mod_virtio_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct virtio_device_id), "virtio",
-			 do_virtio_entry, mod);
-	else if (sym_is(symname, "__mod_vmbus_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct hv_vmbus_device_id), "vmbus",
-			 do_vmbus_entry, mod);
-	else if (sym_is(symname, "__mod_i2c_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct i2c_device_id), "i2c",
-			 do_i2c_entry, mod);
-	else if (sym_is(symname, "__mod_spi_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct spi_device_id), "spi",
-			 do_spi_entry, mod);
-	else if (sym_is(symname, "__mod_dmi_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct dmi_system_id), "dmi",
-			 do_dmi_entry, mod);
-	else if (sym_is(symname, "__mod_platform_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct platform_device_id), "platform",
-			 do_platform_entry, mod);
-	else if (sym_is(symname, "__mod_mdio_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct mdio_device_id), "mdio",
-			 do_mdio_entry, mod);
-	else if (sym_is(symname, "__mod_zorro_device_table"))
-		do_table(symval, sym->st_size,
-			 sizeof(struct zorro_device_id), "zorro",
-			 do_zorro_entry, mod);
-	else if (sym_is(symname, "__mod_isapnp_device_table"))
-		do_table(symval, sym->st_size,
-			sizeof(struct isapnp_device_id), "isa",
-			do_isapnp_entry, mod);
+	else {
+		struct devtable_switch *p = devtable_switch;
+		int i;
+
+		/* scan the array */
+		for (i = 0; i < ARRAY_SIZE(devtable_switch); i++, p++) {
+			if (sym_is(symname, p->symname)) {
+				do_table(symval, sym->st_size, p->id_size,
+					 p->device_id, p->function, mod);
+				break;
+			}
+		}
+	}
 	free(zeros);
 }
 
-- 
1.6.0.2

[RFC PATCH 2/2] modpost: use config and ELF sections to build file2alias

[Alessandro Rubini]

The patch adds a temporary ELF section, then folded into .data, in
order to be able to split bus types to their own individual files.
Since modpost is built for each specific kernel, we can leave off
device types that are not relevant.

Even better, people adding new bus types can just drop in their own
file without patching file2alias over and over, with high chances to
get a collision.

The patch only pulls 4 bus types off file2alias, as a working example
that the approach works and the result is maintainable.

Signed-off-by: Alessandro Rubini <rubini@gnudd.com>
Acked-by: Samuel I. Gonsalvez <siglesia@cern.ch>
Acked-by: Manohar Vanga <manohar.vanga@cern.ch>
---
 scripts/mod/Makefile        |   13 +++-
 scripts/mod/alias_acpi.c    |   14 ++++
 scripts/mod/alias_bcma.c    |   25 +++++++
 scripts/mod/alias_pci.c     |   49 +++++++++++++
 scripts/mod/alias_spi.c     |   15 ++++
 scripts/mod/device_switch.h |   47 +++++++++++++
 scripts/mod/file2alias.c    |  162 +++----------------------------------------
 scripts/mod/modpost.h       |   18 +++++
 scripts/mod/modpost.lds     |    9 +++
 9 files changed, 200 insertions(+), 152 deletions(-)
 create mode 100644 scripts/mod/alias_acpi.c
 create mode 100644 scripts/mod/alias_bcma.c
 create mode 100644 scripts/mod/alias_pci.c
 create mode 100644 scripts/mod/alias_spi.c
 create mode 100644 scripts/mod/device_switch.h
 create mode 100644 scripts/mod/modpost.lds

diff --git a/scripts/mod/Makefile b/scripts/mod/Makefile
index ff954f8..96c3b58 100644
--- a/scripts/mod/Makefile
+++ b/scripts/mod/Makefile
@@ -1,7 +1,18 @@
 hostprogs-y	:= modpost mk_elfconfig
 always		:= $(hostprogs-y) empty.o
 
-modpost-objs	:= modpost.o file2alias.o sumversion.o
+# we can use bool or tristate options here
+modpost-y	:= modpost.o file2alias.o sumversion.o
+modpost-$(CONFIG_ACPI)		+= alias_acpi.o
+modpost-$(CONFIG_BCMA)		+= alias_bcma.o
+modpost-$(CONFIG_PCI)		+= alias_pci.o
+modpost-$(CONFIG_SPI)		+= alias_spi.o
+modpost-y	+= $(modpost-m)
+
+
+modpost-objs	+= $(modpost-y)
+
+HOSTLDFLAGS := $(obj)/modpost.lds
 
 # dependencies on generated files need to be listed explicitly
 
diff --git a/scripts/mod/alias_acpi.c b/scripts/mod/alias_acpi.c
new file mode 100644
index 0000000..acb6842
--- /dev/null
+++ b/scripts/mod/alias_acpi.c
@@ -0,0 +1,14 @@
+#include "modpost.h"
+#include "device_switch.h"
+
+/* looks like: "acpi:ACPI0003 or acpi:PNP0C0B" or "acpi:LNXVIDEO" */
+static int do_acpi_entry(const char *filename,
+			struct acpi_device_id *id, char *alias)
+{
+	sprintf(alias, "acpi*:%s:*", id->id);
+	return 1;
+}
+
+static struct devtable_switch dt_sw_acpi[] __DEVTABLE_SWITCH = {
+	E("acpi", "__mod_acpi_device_table", acpi_device_id, do_acpi_entry),
+};
diff --git a/scripts/mod/alias_bcma.c b/scripts/mod/alias_bcma.c
new file mode 100644
index 0000000..87fa860
--- /dev/null
+++ b/scripts/mod/alias_bcma.c
@@ -0,0 +1,25 @@
+
+#include "modpost.h"
+#include "device_switch.h"
+
+/* Looks like: bcma:mNidNrevNclN. */
+static int do_bcma_entry(const char *filename,
+			 struct bcma_device_id *id, char *alias)
+{
+	id->manuf = TO_NATIVE(id->manuf);
+	id->id = TO_NATIVE(id->id);
+	id->rev = TO_NATIVE(id->rev);
+	id->class = TO_NATIVE(id->class);
+
+	strcpy(alias, "bcma:");
+	ADD(alias, "m", id->manuf != BCMA_ANY_MANUF, id->manuf);
+	ADD(alias, "id", id->id != BCMA_ANY_ID, id->id);
+	ADD(alias, "rev", id->rev != BCMA_ANY_REV, id->rev);
+	ADD(alias, "cl", id->class != BCMA_ANY_CLASS, id->class);
+	add_wildcard(alias);
+	return 1;
+}
+
+static struct devtable_switch dt_sw_bcma[] __DEVTABLE_SWITCH = {
+	E("bcma", "__mod_bcma_device_table", bcma_device_id, do_bcma_entry),
+};
diff --git a/scripts/mod/alias_pci.c b/scripts/mod/alias_pci.c
new file mode 100644
index 0000000..90f3a9a
--- /dev/null
+++ b/scripts/mod/alias_pci.c
@@ -0,0 +1,49 @@
+#include "modpost.h"
+#include "device_switch.h"
+
+/* Looks like: pci:vNdNsvNsdNbcNscNiN. */
+static int do_pci_entry(const char *filename,
+			struct pci_device_id *id, char *alias)
+{
+	/* Class field can be divided into these three. */
+	unsigned char baseclass, subclass, interface,
+		baseclass_mask, subclass_mask, interface_mask;
+
+	id->vendor = TO_NATIVE(id->vendor);
+	id->device = TO_NATIVE(id->device);
+	id->subvendor = TO_NATIVE(id->subvendor);
+	id->subdevice = TO_NATIVE(id->subdevice);
+	id->class = TO_NATIVE(id->class);
+	id->class_mask = TO_NATIVE(id->class_mask);
+
+	strcpy(alias, "pci:");
+	ADD(alias, "v", id->vendor != PCI_ANY_ID, id->vendor);
+	ADD(alias, "d", id->device != PCI_ANY_ID, id->device);
+	ADD(alias, "sv", id->subvendor != PCI_ANY_ID, id->subvendor);
+	ADD(alias, "sd", id->subdevice != PCI_ANY_ID, id->subdevice);
+
+	baseclass = (id->class) >> 16;
+	baseclass_mask = (id->class_mask) >> 16;
+	subclass = (id->class) >> 8;
+	subclass_mask = (id->class_mask) >> 8;
+	interface = id->class;
+	interface_mask = id->class_mask;
+
+	if ((baseclass_mask != 0 && baseclass_mask != 0xFF)
+	    || (subclass_mask != 0 && subclass_mask != 0xFF)
+	    || (interface_mask != 0 && interface_mask != 0xFF)) {
+		warn("Can't handle masks in %s:%04X\n",
+		     filename, id->class_mask);
+		return 0;
+	}
+
+	ADD(alias, "bc", baseclass_mask == 0xFF, baseclass);
+	ADD(alias, "sc", subclass_mask == 0xFF, subclass);
+	ADD(alias, "i", interface_mask == 0xFF, interface);
+	add_wildcard(alias);
+	return 1;
+}
+
+static struct devtable_switch dt_sw_pci[] __DEVTABLE_SWITCH = {
+	E("pci", "__mod_pci_device_table", pci_device_id, do_pci_entry),
+};
diff --git a/scripts/mod/alias_spi.c b/scripts/mod/alias_spi.c
new file mode 100644
index 0000000..d509820
--- /dev/null
+++ b/scripts/mod/alias_spi.c
@@ -0,0 +1,15 @@
+#include "modpost.h"
+#include "device_switch.h"
+
+/* Looks like: spi:S */
+static int do_spi_entry(const char *filename, struct spi_device_id *id,
+			char *alias)
+{
+	sprintf(alias, SPI_MODULE_PREFIX "%s", id->name);
+
+	return 1;
+}
+
+static struct devtable_switch dt_sw_spi[] __DEVTABLE_SWITCH = {
+	E("spi", "__mod_spi_device_table", spi_device_id, do_spi_entry),
+};
diff --git a/scripts/mod/device_switch.h b/scripts/mod/device_switch.h
new file mode 100644
index 0000000..9443cd5
--- /dev/null
+++ b/scripts/mod/device_switch.h
@@ -0,0 +1,47 @@
+/* We use the ELF typedefs for kernel_ulong_t but bite the bullet and
+ * use either stdint.h or inttypes.h for the rest. */
+#if KERNEL_ELFCLASS == ELFCLASS32
+typedef Elf32_Addr	kernel_ulong_t;
+#define BITS_PER_LONG 32
+#else
+typedef Elf64_Addr	kernel_ulong_t;
+#define BITS_PER_LONG 64
+#endif
+#ifdef __sun__
+#include <inttypes.h>
+#else
+#include <stdint.h>
+#endif
+
+#include <ctype.h>
+
+typedef uint32_t	__u32;
+typedef uint16_t	__u16;
+typedef unsigned char	__u8;
+
+/* Big exception to the "don't include kernel headers into userspace, which
+ * even potentially has different endianness and word sizes, since
+ * we handle those differences explicitly below */
+#include "../../include/linux/mod_devicetable.h"
+
+#define ADD(str, sep, cond, field)                              \
+do {                                                            \
+        strcat(str, sep);                                       \
+        if (cond)                                               \
+                sprintf(str + strlen(str),                      \
+                        sizeof(field) == 1 ? "%02X" :           \
+                        sizeof(field) == 2 ? "%04X" :           \
+                        sizeof(field) == 4 ? "%08X" : "",       \
+                        field);                                 \
+        else                                                    \
+                sprintf(str + strlen(str), "*");                \
+} while(0)
+
+/* Always end in a wildcard, for future extension */
+static inline void add_wildcard(char *str)
+{
+	int len = strlen(str);
+
+	if (str[len - 1] != '*')
+		strcat(str + len, "*");
+}
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 1247bc9..8e91fe4 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -11,54 +11,7 @@
  */
 
 #include "modpost.h"
-
-/* We use the ELF typedefs for kernel_ulong_t but bite the bullet and
- * use either stdint.h or inttypes.h for the rest. */
-#if KERNEL_ELFCLASS == ELFCLASS32
-typedef Elf32_Addr	kernel_ulong_t;
-#define BITS_PER_LONG 32
-#else
-typedef Elf64_Addr	kernel_ulong_t;
-#define BITS_PER_LONG 64
-#endif
-#ifdef __sun__
-#include <inttypes.h>
-#else
-#include <stdint.h>
-#endif
-
-#include <ctype.h>
-
-typedef uint32_t	__u32;
-typedef uint16_t	__u16;
-typedef unsigned char	__u8;
-
-/* Big exception to the "don't include kernel headers into userspace, which
- * even potentially has different endianness and word sizes, since
- * we handle those differences explicitly below */
-#include "../../include/linux/mod_devicetable.h"
-
-#define ADD(str, sep, cond, field)                              \
-do {                                                            \
-        strcat(str, sep);                                       \
-        if (cond)                                               \
-                sprintf(str + strlen(str),                      \
-                        sizeof(field) == 1 ? "%02X" :           \
-                        sizeof(field) == 2 ? "%04X" :           \
-                        sizeof(field) == 4 ? "%08X" : "",       \
-                        field);                                 \
-        else                                                    \
-                sprintf(str + strlen(str), "*");                \
-} while(0)
-
-/* Always end in a wildcard, for future extension */
-static inline void add_wildcard(char *str)
-{
-	int len = strlen(str);
-
-	if (str[len - 1] != '*')
-		strcat(str + len, "*");
-}
+#include "device_switch.h"
 
 unsigned int cross_build = 0;
 /**
@@ -314,49 +267,6 @@ static int do_ieee1394_entry(const char *filename,
 	return 1;
 }
 
-/* Looks like: pci:vNdNsvNsdNbcNscNiN. */
-static int do_pci_entry(const char *filename,
-			struct pci_device_id *id, char *alias)
-{
-	/* Class field can be divided into these three. */
-	unsigned char baseclass, subclass, interface,
-		baseclass_mask, subclass_mask, interface_mask;
-
-	id->vendor = TO_NATIVE(id->vendor);
-	id->device = TO_NATIVE(id->device);
-	id->subvendor = TO_NATIVE(id->subvendor);
-	id->subdevice = TO_NATIVE(id->subdevice);
-	id->class = TO_NATIVE(id->class);
-	id->class_mask = TO_NATIVE(id->class_mask);
-
-	strcpy(alias, "pci:");
-	ADD(alias, "v", id->vendor != PCI_ANY_ID, id->vendor);
-	ADD(alias, "d", id->device != PCI_ANY_ID, id->device);
-	ADD(alias, "sv", id->subvendor != PCI_ANY_ID, id->subvendor);
-	ADD(alias, "sd", id->subdevice != PCI_ANY_ID, id->subdevice);
-
-	baseclass = (id->class) >> 16;
-	baseclass_mask = (id->class_mask) >> 16;
-	subclass = (id->class) >> 8;
-	subclass_mask = (id->class_mask) >> 8;
-	interface = id->class;
-	interface_mask = id->class_mask;
-
-	if ((baseclass_mask != 0 && baseclass_mask != 0xFF)
-	    || (subclass_mask != 0 && subclass_mask != 0xFF)
-	    || (interface_mask != 0 && interface_mask != 0xFF)) {
-		warn("Can't handle masks in %s:%04X\n",
-		     filename, id->class_mask);
-		return 0;
-	}
-
-	ADD(alias, "bc", baseclass_mask == 0xFF, baseclass);
-	ADD(alias, "sc", subclass_mask == 0xFF, subclass);
-	ADD(alias, "i", interface_mask == 0xFF, interface);
-	add_wildcard(alias);
-	return 1;
-}
-
 /* looks like: "ccw:tNmNdtNdmN" */
 static int do_ccw_entry(const char *filename,
 			struct ccw_device_id *id, char *alias)
@@ -415,14 +325,6 @@ static int do_serio_entry(const char *filename,
 	return 1;
 }
 
-/* looks like: "acpi:ACPI0003 or acpi:PNP0C0B" or "acpi:LNXVIDEO" */
-static int do_acpi_entry(const char *filename,
-			struct acpi_device_id *id, char *alias)
-{
-	sprintf(alias, "acpi*:%s:*", id->id);
-	return 1;
-}
-
 /* looks like: "pnp:dD" */
 static void do_pnp_device_entry(void *symval, unsigned long size,
 				struct module *mod)
@@ -702,24 +604,6 @@ static int do_ssb_entry(const char *filename,
 	return 1;
 }
 
-/* Looks like: bcma:mNidNrevNclN. */
-static int do_bcma_entry(const char *filename,
-			 struct bcma_device_id *id, char *alias)
-{
-	id->manuf = TO_NATIVE(id->manuf);
-	id->id = TO_NATIVE(id->id);
-	id->rev = TO_NATIVE(id->rev);
-	id->class = TO_NATIVE(id->class);
-
-	strcpy(alias, "bcma:");
-	ADD(alias, "m", id->manuf != BCMA_ANY_MANUF, id->manuf);
-	ADD(alias, "id", id->id != BCMA_ANY_ID, id->id);
-	ADD(alias, "rev", id->rev != BCMA_ANY_REV, id->rev);
-	ADD(alias, "cl", id->class != BCMA_ANY_CLASS, id->class);
-	add_wildcard(alias);
-	return 1;
-}
-
 /* Looks like: virtio:dNvN */
 static int do_virtio_entry(const char *filename, struct virtio_device_id *id,
 			   char *alias)
@@ -765,13 +649,14 @@ static int do_i2c_entry(const char *filename, struct i2c_device_id *id,
 	return 1;
 }
 
-/* Looks like: spi:S */
-static int do_spi_entry(const char *filename, struct spi_device_id *id,
-			char *alias)
+static void dmi_ascii_filter(char *d, const char *s)
 {
-	sprintf(alias, SPI_MODULE_PREFIX "%s", id->name);
+	/* Filter out characters we don't want to see in the modalias string */
+	for (; *s; s++)
+		if (*s > ' ' && *s < 127 && *s != ':')
+			*(d++) = *s;
 
-	return 1;
+	*d = 0;
 }
 
 static const struct dmifield {
@@ -793,17 +678,6 @@ static const struct dmifield {
 	{ NULL,  DMI_NONE }
 };
 
-static void dmi_ascii_filter(char *d, const char *s)
-{
-	/* Filter out characters we don't want to see in the modalias string */
-	for (; *s; s++)
-		if (*s > ' ' && *s < 127 && *s != ':')
-			*(d++) = *s;
-
-	*d = 0;
-}
-
-
 static int do_dmi_entry(const char *filename, struct dmi_system_id *id,
 			char *alias)
 {
@@ -891,7 +765,7 @@ static inline int sym_is(const char *symbol, const char *name)
 	return match[strlen(name)] == '\0';
 }
 
-static void do_table(void *symval, unsigned long size,
+void do_table(void *symval, unsigned long size,
 		     unsigned long id_size,
 		     const char *device_id,
 		     void *function,
@@ -913,17 +787,7 @@ static void do_table(void *symval, unsigned long size,
 	}
 }
 
-/* This array collects all instances that use the generic do_table above */
-struct devtable_switch {
-	const char *device_id;
-	const char *symname;
-	unsigned long id_size;
-	void *function;
-};
-#define E(id, name, structname, f) {id, name, sizeof(struct structname), f}
-
-static struct devtable_switch devtable_switch[] = {
-	E("pci", "__mod_pci_device_table", pci_device_id, do_pci_entry),
+static struct devtable_switch devtable_switch[] __DEVTABLE_SWITCH = {
 	E("hid", "__mod_hid_device_table", hid_device_id, do_hid_entry),
 	E("ieee1394", "__mod_ieee1394_device_table",
 	  ieee1394_device_id, do_ieee1394_entry),
@@ -931,7 +795,6 @@ static struct devtable_switch devtable_switch[] = {
 	E("ap", "__mod_ap_device_table", ap_device_id, do_ap_entry),
 	E("css", "__mod_css_device_table", css_device_id, do_css_entry),
 	E("seio", "__mod_serio_device_table", serio_device_id, do_serio_entry),
-	E("acpi", "__mod_acpi_device_table", acpi_device_id, do_acpi_entry),
 	E("pcmcia", "__mod_pcmcia_device_table",
 	  pcmcia_device_id, do_pcmcia_entry),
 	E("of", "__mod_of_device_table", of_device_id, do_of_entry),
@@ -942,13 +805,11 @@ static struct devtable_switch devtable_switch[] = {
 	  parisc_device_id, do_parisc_entry),
 	E("sdio", "__mod_sdio_device_table", sdio_device_id, do_sdio_entry),
 	E("ssb", "__mod_ssb_device_table", ssb_device_id, do_ssb_entry),
-	E("bcma", "__mod_bcma_device_table", bcma_device_id, do_bcma_entry),
 	E("virtio", "__mod_virtio_device_table",
 	  virtio_device_id, do_virtio_entry),
 	E("vmbus", "__mod_vmbus_device_table",
 	  hv_vmbus_device_id, do_vmbus_entry),
 	E("i2c", "__mod_i2c_device_table", i2c_device_id, do_i2c_entry),
-	E("spi", "__mod_spi_device_table", spi_device_id, do_spi_entry),
 	E("dmi", "__mod_dmi_device_table", dmi_system_id, do_dmi_entry),
 	E("platform", "__mod_platform_device_table",
 	  platform_device_id, do_platform_entry),
@@ -989,11 +850,10 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
 	else if (sym_is(symname, "__mod_pnp_card_device_table"))
 		do_pnp_card_entries(symval, sym->st_size, mod);
 	else {
-		struct devtable_switch *p = devtable_switch;
-		int i;
+		struct devtable_switch *p;
 
 		/* scan the array */
-		for (i = 0; i < ARRAY_SIZE(devtable_switch); i++, p++) {
+		for (p = __dt_sw_first; p < __dt_sw_last; p++) {
 			if (sym_is(symname, p->symname)) {
 				do_table(symval, sym->st_size, p->id_size,
 					 p->device_id, p->function, mod);
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 2031119..ea98de3 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -169,6 +169,24 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
 			Elf_Sym *sym, const char *symname);
 void add_moddevtable(struct buffer *buf, struct module *mod);
 
+void do_table(void *symval, unsigned long size,
+	      unsigned long id_size,
+	      const char *device_id,
+	      void *function,
+	      struct module *mod);
+
+/* An array of these is assembled at compile time */
+struct devtable_switch {
+	const char *device_id;
+	const char *symname;
+	unsigned long id_size;
+	void *function;
+};
+#define E(id, name, structname, f) {id, name, sizeof(struct structname), f}
+#define __DEVTABLE_SWITCH __attribute__((section(".devtable_switch"),used))
+
+extern struct devtable_switch __dt_sw_first[], __dt_sw_last[];
+
 /* sumversion.c */
 void maybe_frob_rcs_version(const char *modfilename,
 			    char *version,
diff --git a/scripts/mod/modpost.lds b/scripts/mod/modpost.lds
new file mode 100644
index 0000000..0642326
--- /dev/null
+++ b/scripts/mod/modpost.lds
@@ -0,0 +1,9 @@
+SECTIONS
+{
+	.data : {
+		. = ALIGN(0x40);
+		__dt_sw_first = .;
+		*(.devtable_switch);
+		__dt_sw_last = .;
+		}
+}
-- 
1.6.0.2

Re: [RFC PATCH 2/2] modpost: use config and ELF sections to build file2alias

[Dave Martin]

Taking a look at your original series...

On Tue, Nov 22, 2011 at 12:56:32PM +0100, Alessandro Rubini wrote:

[...]

> lkml.org/lkml/2011/11/4/125 (0/2: description of the work)                                                                                                                                                  
> lkml.org/lkml/2011/11/4/126 (1/2: turn the if plethora to table lookup)                                                                                                                                     
> lkml.org/lkml/2011/11/4/127 (2/2: partial split to type-specific files).                                                                                                                                    

I'm not the expert here, but I have a few comments that might be
useful.

On Fri, 4 Nov 2011 13:23:21 +0100, Alessandro Rubini <rubini@gnudd.com> wrote:
> diff --git a/scripts/mod/Makefile b/scripts/mod/Makefile
> index ff954f8..96c3b58 100644
> --- a/scripts/mod/Makefile
> +++ b/scripts/mod/Makefile
> @@ -1,7 +1,18 @@
>  hostprogs-y	:= modpost mk_elfconfig
>  always		:= $(hostprogs-y) empty.o
>  
> -modpost-objs	:= modpost.o file2alias.o sumversion.o
> +# we can use bool or tristate options here
> +modpost-y	:= modpost.o file2alias.o sumversion.o
> +modpost-$(CONFIG_ACPI)		+= alias_acpi.o
> +modpost-$(CONFIG_BCMA)		+= alias_bcma.o
> +modpost-$(CONFIG_PCI)		+= alias_pci.o
> +modpost-$(CONFIG_SPI)		+= alias_spi.o
> +modpost-y	+= $(modpost-m)
> +
> +
> +modpost-objs	+= $(modpost-y)
> +
> +HOSTLDFLAGS := $(obj)/modpost.lds
>  
>  # dependencies on generated files need to be listed explicitly
>  
> diff --git a/scripts/mod/alias_acpi.c b/scripts/mod/alias_acpi.c
> new file mode 100644
> index 0000000..acb6842
> --- /dev/null
> +++ b/scripts/mod/alias_acpi.c
> @@ -0,0 +1,14 @@
> +#include "modpost.h"
> +#include "device_switch.h"
> +
> +/* looks like: "acpi:ACPI0003 or acpi:PNP0C0B" or "acpi:LNXVIDEO" */
> +static int do_acpi_entry(const char *filename,
> +			struct acpi_device_id *id, char *alias)
> +{
> +	sprintf(alias, "acpi*:%s:*", id->id);
> +	return 1;
> +}
> +
> +static struct devtable_switch dt_sw_acpi[] __DEVTABLE_SWITCH = {
> +	E("acpi", "__mod_acpi_device_table", acpi_device_id, do_acpi_entry),
> +};

In files which define multiple bus types, there's no reason to put them
all in the same array, so we can avoid the explicit boilerplate.

Assuming this technique is used at all (see comments below), a modified
approach like the following would be possible:

device_switch.h:

#include <linux/compiler.h>

#define DEVTABLE_SWITCH(device_id, symname, structname, function) \
        __DEVTABLE_SWITCH(__LINE__, device_id, symname, structname, function)

#define __DEVTABLE_SWITCH(line_num, device_id, symname, structname, function) \
        static const struct devtable_swith __dt_sw_##device_id##_##line_num \
                __section(.devtable.switch) __used = {          \
                        .device_id = #device_id,                \
                        .symname = symname,                     \
                        .id_size = sizeof(struct structname)    \
                        .function = function                    \
                }

__LINE__ is used simply to generate unique local symbol names in case a
file wants to define multiple entries.  If we don't think that will ever
happen, that trick isn't needed.

At time of use, this just becomes:

#incluce "device_switch.h"

DEVTABLE_SWITCH(acpi, "__mod_acpi_device_table", acpi_device_id, do_acpi_entry);


Notwithstanding this, your version does look like it should work.

> diff --git a/scripts/mod/alias_bcma.c b/scripts/mod/alias_bcma.c
> new file mode 100644
> index 0000000..87fa860
> --- /dev/null
> +++ b/scripts/mod/alias_bcma.c
> @@ -0,0 +1,25 @@
> +
> +#include "modpost.h"
> +#include "device_switch.h"
> +
> +/* Looks like: bcma:mNidNrevNclN. */
> +static int do_bcma_entry(const char *filename,
> +			 struct bcma_device_id *id, char *alias)
> +{
> +	id->manuf = TO_NATIVE(id->manuf);
> +	id->id = TO_NATIVE(id->id);
> +	id->rev = TO_NATIVE(id->rev);
> +	id->class = TO_NATIVE(id->class);
> +
> +	strcpy(alias, "bcma:");
> +	ADD(alias, "m", id->manuf != BCMA_ANY_MANUF, id->manuf);
> +	ADD(alias, "id", id->id != BCMA_ANY_ID, id->id);
> +	ADD(alias, "rev", id->rev != BCMA_ANY_REV, id->rev);
> +	ADD(alias, "cl", id->class != BCMA_ANY_CLASS, id->class);
> +	add_wildcard(alias);
> +	return 1;
> +}
> +
> +static struct devtable_switch dt_sw_bcma[] __DEVTABLE_SWITCH = {
> +	E("bcma", "__mod_bcma_device_table", bcma_device_id, do_bcma_entry),
> +};
> diff --git a/scripts/mod/alias_pci.c b/scripts/mod/alias_pci.c
> new file mode 100644
> index 0000000..90f3a9a
> --- /dev/null
> +++ b/scripts/mod/alias_pci.c
> @@ -0,0 +1,49 @@
> +#include "modpost.h"
> +#include "device_switch.h"
> +
> +/* Looks like: pci:vNdNsvNsdNbcNscNiN. */
> +static int do_pci_entry(const char *filename,
> +			struct pci_device_id *id, char *alias)
> +{
> +	/* Class field can be divided into these three. */
> +	unsigned char baseclass, subclass, interface,
> +		baseclass_mask, subclass_mask, interface_mask;
> +
> +	id->vendor = TO_NATIVE(id->vendor);
> +	id->device = TO_NATIVE(id->device);
> +	id->subvendor = TO_NATIVE(id->subvendor);
> +	id->subdevice = TO_NATIVE(id->subdevice);
> +	id->class = TO_NATIVE(id->class);
> +	id->class_mask = TO_NATIVE(id->class_mask);
> +
> +	strcpy(alias, "pci:");
> +	ADD(alias, "v", id->vendor != PCI_ANY_ID, id->vendor);
> +	ADD(alias, "d", id->device != PCI_ANY_ID, id->device);
> +	ADD(alias, "sv", id->subvendor != PCI_ANY_ID, id->subvendor);
> +	ADD(alias, "sd", id->subdevice != PCI_ANY_ID, id->subdevice);
> +
> +	baseclass = (id->class) >> 16;
> +	baseclass_mask = (id->class_mask) >> 16;
> +	subclass = (id->class) >> 8;
> +	subclass_mask = (id->class_mask) >> 8;
> +	interface = id->class;
> +	interface_mask = id->class_mask;
> +
> +	if ((baseclass_mask != 0 && baseclass_mask != 0xFF)
> +	    || (subclass_mask != 0 && subclass_mask != 0xFF)
> +	    || (interface_mask != 0 && interface_mask != 0xFF)) {
> +		warn("Can't handle masks in %s:%04X\n",
> +		     filename, id->class_mask);
> +		return 0;
> +	}
> +
> +	ADD(alias, "bc", baseclass_mask == 0xFF, baseclass);
> +	ADD(alias, "sc", subclass_mask == 0xFF, subclass);
> +	ADD(alias, "i", interface_mask == 0xFF, interface);
> +	add_wildcard(alias);
> +	return 1;
> +}
> +
> +static struct devtable_switch dt_sw_pci[] __DEVTABLE_SWITCH = {
> +	E("pci", "__mod_pci_device_table", pci_device_id, do_pci_entry),
> +};
> diff --git a/scripts/mod/alias_spi.c b/scripts/mod/alias_spi.c
> new file mode 100644
> index 0000000..d509820
> --- /dev/null
> +++ b/scripts/mod/alias_spi.c
> @@ -0,0 +1,15 @@
> +#include "modpost.h"
> +#include "device_switch.h"
> +
> +/* Looks like: spi:S */
> +static int do_spi_entry(const char *filename, struct spi_device_id *id,
> +			char *alias)
> +{
> +	sprintf(alias, SPI_MODULE_PREFIX "%s", id->name);
> +
> +	return 1;
> +}
> +
> +static struct devtable_switch dt_sw_spi[] __DEVTABLE_SWITCH = {
> +	E("spi", "__mod_spi_device_table", spi_device_id, do_spi_entry),
> +};
> diff --git a/scripts/mod/device_switch.h b/scripts/mod/device_switch.h
> new file mode 100644
> index 0000000..9443cd5
> --- /dev/null
> +++ b/scripts/mod/device_switch.h
> @@ -0,0 +1,47 @@
> +/* We use the ELF typedefs for kernel_ulong_t but bite the bullet and
> + * use either stdint.h or inttypes.h for the rest. */
> +#if KERNEL_ELFCLASS == ELFCLASS32
> +typedef Elf32_Addr	kernel_ulong_t;
> +#define BITS_PER_LONG 32
> +#else
> +typedef Elf64_Addr	kernel_ulong_t;
> +#define BITS_PER_LONG 64
> +#endif
> +#ifdef __sun__
> +#include <inttypes.h>
> +#else
> +#include <stdint.h>
> +#endif
> +
> +#include <ctype.h>
> +
> +typedef uint32_t	__u32;
> +typedef uint16_t	__u16;
> +typedef unsigned char	__u8;
> +
> +/* Big exception to the "don't include kernel headers into userspace, which
> + * even potentially has different endianness and word sizes, since
> + * we handle those differences explicitly below */
> +#include "../../include/linux/mod_devicetable.h"
> +
> +#define ADD(str, sep, cond, field)                              \
> +do {                                                            \
> +        strcat(str, sep);                                       \
> +        if (cond)                                               \
> +                sprintf(str + strlen(str),                      \
> +                        sizeof(field) == 1 ? "%02X" :           \
> +                        sizeof(field) == 2 ? "%04X" :           \
> +                        sizeof(field) == 4 ? "%08X" : "",       \
> +                        field);                                 \
> +        else                                                    \
> +                sprintf(str + strlen(str), "*");                \
> +} while(0)
> +
> +/* Always end in a wildcard, for future extension */
> +static inline void add_wildcard(char *str)
> +{
> +	int len = strlen(str);
> +
> +	if (str[len - 1] != '*')
> +		strcat(str + len, "*");
> +}
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 1247bc9..8e91fe4 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -11,54 +11,7 @@
>   */
>  
>  #include "modpost.h"
> -
> -/* We use the ELF typedefs for kernel_ulong_t but bite the bullet and
> - * use either stdint.h or inttypes.h for the rest. */
> -#if KERNEL_ELFCLASS == ELFCLASS32
> -typedef Elf32_Addr	kernel_ulong_t;
> -#define BITS_PER_LONG 32
> -#else
> -typedef Elf64_Addr	kernel_ulong_t;
> -#define BITS_PER_LONG 64
> -#endif
> -#ifdef __sun__
> -#include <inttypes.h>
> -#else
> -#include <stdint.h>
> -#endif
> -
> -#include <ctype.h>
> -
> -typedef uint32_t	__u32;
> -typedef uint16_t	__u16;
> -typedef unsigned char	__u8;
> -
> -/* Big exception to the "don't include kernel headers into userspace, which
> - * even potentially has different endianness and word sizes, since
> - * we handle those differences explicitly below */
> -#include "../../include/linux/mod_devicetable.h"
> -
> -#define ADD(str, sep, cond, field)                              \
> -do {                                                            \
> -        strcat(str, sep);                                       \
> -        if (cond)                                               \
> -                sprintf(str + strlen(str),                      \
> -                        sizeof(field) == 1 ? "%02X" :           \
> -                        sizeof(field) == 2 ? "%04X" :           \
> -                        sizeof(field) == 4 ? "%08X" : "",       \
> -                        field);                                 \
> -        else                                                    \
> -                sprintf(str + strlen(str), "*");                \
> -} while(0)
> -
> -/* Always end in a wildcard, for future extension */
> -static inline void add_wildcard(char *str)
> -{
> -	int len = strlen(str);
> -
> -	if (str[len - 1] != '*')
> -		strcat(str + len, "*");
> -}
> +#include "device_switch.h"
>  
>  unsigned int cross_build = 0;
>  /**
> @@ -314,49 +267,6 @@ static int do_ieee1394_entry(const char *filename,
>  	return 1;
>  }
>  
> -/* Looks like: pci:vNdNsvNsdNbcNscNiN. */
> -static int do_pci_entry(const char *filename,
> -			struct pci_device_id *id, char *alias)
> -{
> -	/* Class field can be divided into these three. */
> -	unsigned char baseclass, subclass, interface,
> -		baseclass_mask, subclass_mask, interface_mask;
> -
> -	id->vendor = TO_NATIVE(id->vendor);
> -	id->device = TO_NATIVE(id->device);
> -	id->subvendor = TO_NATIVE(id->subvendor);
> -	id->subdevice = TO_NATIVE(id->subdevice);
> -	id->class = TO_NATIVE(id->class);
> -	id->class_mask = TO_NATIVE(id->class_mask);
> -
> -	strcpy(alias, "pci:");
> -	ADD(alias, "v", id->vendor != PCI_ANY_ID, id->vendor);
> -	ADD(alias, "d", id->device != PCI_ANY_ID, id->device);
> -	ADD(alias, "sv", id->subvendor != PCI_ANY_ID, id->subvendor);
> -	ADD(alias, "sd", id->subdevice != PCI_ANY_ID, id->subdevice);
> -
> -	baseclass = (id->class) >> 16;
> -	baseclass_mask = (id->class_mask) >> 16;
> -	subclass = (id->class) >> 8;
> -	subclass_mask = (id->class_mask) >> 8;
> -	interface = id->class;
> -	interface_mask = id->class_mask;
> -
> -	if ((baseclass_mask != 0 && baseclass_mask != 0xFF)
> -	    || (subclass_mask != 0 && subclass_mask != 0xFF)
> -	    || (interface_mask != 0 && interface_mask != 0xFF)) {
> -		warn("Can't handle masks in %s:%04X\n",
> -		     filename, id->class_mask);
> -		return 0;
> -	}
> -
> -	ADD(alias, "bc", baseclass_mask == 0xFF, baseclass);
> -	ADD(alias, "sc", subclass_mask == 0xFF, subclass);
> -	ADD(alias, "i", interface_mask == 0xFF, interface);
> -	add_wildcard(alias);
> -	return 1;
> -}
> -
>  /* looks like: "ccw:tNmNdtNdmN" */
>  static int do_ccw_entry(const char *filename,
>  			struct ccw_device_id *id, char *alias)
> @@ -415,14 +325,6 @@ static int do_serio_entry(const char *filename,
>  	return 1;
>  }
>  
> -/* looks like: "acpi:ACPI0003 or acpi:PNP0C0B" or "acpi:LNXVIDEO" */
> -static int do_acpi_entry(const char *filename,
> -			struct acpi_device_id *id, char *alias)
> -{
> -	sprintf(alias, "acpi*:%s:*", id->id);
> -	return 1;
> -}
> -
>  /* looks like: "pnp:dD" */
>  static void do_pnp_device_entry(void *symval, unsigned long size,
>  				struct module *mod)
> @@ -702,24 +604,6 @@ static int do_ssb_entry(const char *filename,
>  	return 1;
>  }
>  
> -/* Looks like: bcma:mNidNrevNclN. */
> -static int do_bcma_entry(const char *filename,
> -			 struct bcma_device_id *id, char *alias)
> -{
> -	id->manuf = TO_NATIVE(id->manuf);
> -	id->id = TO_NATIVE(id->id);
> -	id->rev = TO_NATIVE(id->rev);
> -	id->class = TO_NATIVE(id->class);
> -
> -	strcpy(alias, "bcma:");
> -	ADD(alias, "m", id->manuf != BCMA_ANY_MANUF, id->manuf);
> -	ADD(alias, "id", id->id != BCMA_ANY_ID, id->id);
> -	ADD(alias, "rev", id->rev != BCMA_ANY_REV, id->rev);
> -	ADD(alias, "cl", id->class != BCMA_ANY_CLASS, id->class);
> -	add_wildcard(alias);
> -	return 1;
> -}
> -
>  /* Looks like: virtio:dNvN */
>  static int do_virtio_entry(const char *filename, struct virtio_device_id *id,
>  			   char *alias)
> @@ -765,13 +649,14 @@ static int do_i2c_entry(const char *filename, struct i2c_device_id *id,
>  	return 1;
>  }
>  
> -/* Looks like: spi:S */
> -static int do_spi_entry(const char *filename, struct spi_device_id *id,
> -			char *alias)
> +static void dmi_ascii_filter(char *d, const char *s)
>  {
> -	sprintf(alias, SPI_MODULE_PREFIX "%s", id->name);
> +	/* Filter out characters we don't want to see in the modalias string */
> +	for (; *s; s++)
> +		if (*s > ' ' && *s < 127 && *s != ':')
> +			*(d++) = *s;
>  
> -	return 1;
> +	*d = 0;
>  }
>  
>  static const struct dmifield {
> @@ -793,17 +678,6 @@ static const struct dmifield {
>  	{ NULL,  DMI_NONE }
>  };
>  
> -static void dmi_ascii_filter(char *d, const char *s)
> -{
> -	/* Filter out characters we don't want to see in the modalias string */
> -	for (; *s; s++)
> -		if (*s > ' ' && *s < 127 && *s != ':')
> -			*(d++) = *s;
> -
> -	*d = 0;
> -}
> -
> -
>  static int do_dmi_entry(const char *filename, struct dmi_system_id *id,
>  			char *alias)
>  {
> @@ -891,7 +765,7 @@ static inline int sym_is(const char *symbol, const char *name)
>  	return match[strlen(name)] == '\0';
>  }
>  
> -static void do_table(void *symval, unsigned long size,
> +void do_table(void *symval, unsigned long size,
>  		     unsigned long id_size,
>  		     const char *device_id,
>  		     void *function,
> @@ -913,17 +787,7 @@ static void do_table(void *symval, unsigned long size,
>  	}
>  }
>  
> -/* This array collects all instances that use the generic do_table above */
> -struct devtable_switch {
> -	const char *device_id;
> -	const char *symname;
> -	unsigned long id_size;
> -	void *function;
> -};
> -#define E(id, name, structname, f) {id, name, sizeof(struct structname), f}
> -
> -static struct devtable_switch devtable_switch[] = {
> -	E("pci", "__mod_pci_device_table", pci_device_id, do_pci_entry),
> +static struct devtable_switch devtable_switch[] __DEVTABLE_SWITCH = {
>  	E("hid", "__mod_hid_device_table", hid_device_id, do_hid_entry),
>  	E("ieee1394", "__mod_ieee1394_device_table",
>  	  ieee1394_device_id, do_ieee1394_entry),
> @@ -931,7 +795,6 @@ static struct devtable_switch devtable_switch[] = {
>  	E("ap", "__mod_ap_device_table", ap_device_id, do_ap_entry),
>  	E("css", "__mod_css_device_table", css_device_id, do_css_entry),
>  	E("seio", "__mod_serio_device_table", serio_device_id, do_serio_entry),
> -	E("acpi", "__mod_acpi_device_table", acpi_device_id, do_acpi_entry),
>  	E("pcmcia", "__mod_pcmcia_device_table",
>  	  pcmcia_device_id, do_pcmcia_entry),
>  	E("of", "__mod_of_device_table", of_device_id, do_of_entry),
> @@ -942,13 +805,11 @@ static struct devtable_switch devtable_switch[] = {
>  	  parisc_device_id, do_parisc_entry),
>  	E("sdio", "__mod_sdio_device_table", sdio_device_id, do_sdio_entry),
>  	E("ssb", "__mod_ssb_device_table", ssb_device_id, do_ssb_entry),
> -	E("bcma", "__mod_bcma_device_table", bcma_device_id, do_bcma_entry),
>  	E("virtio", "__mod_virtio_device_table",
>  	  virtio_device_id, do_virtio_entry),
>  	E("vmbus", "__mod_vmbus_device_table",
>  	  hv_vmbus_device_id, do_vmbus_entry),
>  	E("i2c", "__mod_i2c_device_table", i2c_device_id, do_i2c_entry),
> -	E("spi", "__mod_spi_device_table", spi_device_id, do_spi_entry),
>  	E("dmi", "__mod_dmi_device_table", dmi_system_id, do_dmi_entry),
>  	E("platform", "__mod_platform_device_table",
>  	  platform_device_id, do_platform_entry),
> @@ -989,11 +850,10 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
>  	else if (sym_is(symname, "__mod_pnp_card_device_table"))
>  		do_pnp_card_entries(symval, sym->st_size, mod);
>  	else {
> -		struct devtable_switch *p = devtable_switch;
> -		int i;
> +		struct devtable_switch *p;
>  
>  		/* scan the array */
> -		for (i = 0; i < ARRAY_SIZE(devtable_switch); i++, p++) {
> +		for (p = __dt_sw_first; p < __dt_sw_last; p++) {
>  			if (sym_is(symname, p->symname)) {
>  				do_table(symval, sym->st_size, p->id_size,
>  					 p->device_id, p->function, mod);
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index 2031119..ea98de3 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -169,6 +169,24 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
>  			Elf_Sym *sym, const char *symname);
>  void add_moddevtable(struct buffer *buf, struct module *mod);
>  
> +void do_table(void *symval, unsigned long size,
> +	      unsigned long id_size,
> +	      const char *device_id,
> +	      void *function,
> +	      struct module *mod);
> +
> +/* An array of these is assembled at compile time */
> +struct devtable_switch {
> +	const char *device_id;
> +	const char *symname;
> +	unsigned long id_size;
> +	void *function;
> +};
> +#define E(id, name, structname, f) {id, name, sizeof(struct structname), f}
> +#define __DEVTABLE_SWITCH __attribute__((section(".devtable_switch"),used))

I have some misgivings about using this kind of toolchain trick where it
is not needed -- though this is partly a matter of taste.

(To clarify, I believe this technique isn't needed except when the
resulting tables need to be inspectable from a binary only
environment, such as the running kernel image or tools like modinfo.)

All we really need here is to include the required set of headers at
compile-time, rather than doing these link-time tricks.  This is
essentially just a source transofmration and can probably be done
straightforwardly with an autogenerated include file, a couple of
Makefile rules and some scripting or preprocessor tricks to paste
the relevant entries into a common array.

If you think that's worth a try, we can discuss it further.

> +
> +extern struct devtable_switch __dt_sw_first[], __dt_sw_last[];
> +
>  /* sumversion.c */
>  void maybe_frob_rcs_version(const char *modfilename,
>                           char *version,
> diff --git a/scripts/mod/modpost.lds b/scripts/mod/modpost.lds
> new file mode 100644
> index 0000000..0642326
> --- /dev/null
> +++ b/scripts/mod/modpost.lds
> @@ -0,0 +1,9 @@
> +SECTIONS
> +{
> +     .data : {
> +             . = ALIGN(0x40);
> +             __dt_sw_first = .;
> +             *(.devtable_switch);
> +             __dt_sw_last = .;
> +             }
> +}
> --
> 1.6.0.2
>
>

For myself, I have some misgivings about using this kind of toolchain
trick where it is not needed -- though this is partly a matter of taste.

(To clarify, I think this stuff is only needed where the resulting
tables need to be inspectable from a binary-only environment, such
as in the running kernel, or when using kernel image analysis tools
which are not supposed to depend in having a configured kernel source
tree handy, such as modinfo.)

The problem to be solved here is essentially a source transofmration
and should be possible to do straightforwardly with an autogenerated
include file, a couple of Makefile rules and some scripting or
preprocessor tricks to paste the relevant entries into a common
array.

If you think that's worth a try, we can discuss it further.

> +
> +extern struct devtable_switch __dt_sw_first[], __dt_sw_last[];
> +
>  /* sumversion.c */
>  void maybe_frob_rcs_version(const char *modfilename,
>  			    char *version,
> diff --git a/scripts/mod/modpost.lds b/scripts/mod/modpost.lds
> new file mode 100644
> index 0000000..0642326
> --- /dev/null
> +++ b/scripts/mod/modpost.lds
> @@ -0,0 +1,9 @@
> +SECTIONS
> +{
> +	.data : {
> +		. = ALIGN(0x40);
> +		__dt_sw_first = .;
> +		*(.devtable_switch);
> +		__dt_sw_last = .;
> +		}
> +}
> -- 
> 1.6.0.2
> 
> 

Otherwise, this series looks like a sensible evolution to me.

Cheers
---Dave

Re: [RFC PATCH 2/2] modpost: use config and ELF sections to build file2alias

[Alessandro Rubini]

> I'm not the expert here, but I have a few comments that might be
> useful.

Yes, thanks.

> In files which define multiple bus types, there's no reason to put them
> all in the same array, so we can avoid the explicit boilerplate.

Nice catch, I'll do as you suggest.

> For myself, I have some misgivings about using this kind of toolchain
> trick where it is not needed -- though this is partly a matter of taste.
> 
> (To clarify, I think this stuff is only needed where the resulting
> [...]

This is needed because the bus abstraction and module autoloading
is so useful that we have a lot of uses, and they are ever increasing.
As said, in my current workgroup we have two new buses in the works.

> The problem to be solved here is essentially a source transofmration
> and should be possible to do straightforwardly with an autogenerated
> include file, a couple of Makefile rules and some scripting or
> preprocessor tricks to paste the relevant entries into a common
> array.

But the result is more hackish than this. ELF sections are well
understood and widely used in this environment, so the solution is
proved working. Scripting otherwise is an unneeded complication, as
every user will have to know a  new technique, which may break in
the future (e.g. gmake 3.82 broke some makefiles: each new trick is
a new risk).

> Otherwise, this series looks like a sensible evolution to me.

thanks
/alessandro

Re: [RFC PATCH 2/2] modpost: use config and ELF sections to build file2alias

[Dave Martin]

On Wed, Nov 23, 2011 at 05:54:52PM +0100, Alessandro Rubini wrote:
> > I'm not the expert here, but I have a few comments that might be
> > useful.
> 
> Yes, thanks.
> 
> > In files which define multiple bus types, there's no reason to put them
> > all in the same array, so we can avoid the explicit boilerplate.
> 
> Nice catch, I'll do as you suggest.
> 
> > For myself, I have some misgivings about using this kind of toolchain
> > trick where it is not needed -- though this is partly a matter of taste.
> > 
> > (To clarify, I think this stuff is only needed where the resulting
> > [...]
> 
> This is needed because the bus abstraction and module autoloading
> is so useful that we have a lot of uses, and they are ever increasing.
> As said, in my current workgroup we have two new buses in the works.

You may have misunderstood -- putting tables in special sections and
doing a link-time merge is the thing that we don't necessarily to do
here.  We do need the final merged table for use by file2alias.c; we
just don't necessarily have to construct it in that way.

Orthogonalising the addition of new buses to the module framework is
clearly a good idea though -- I'm not disupting that.

> > The problem to be solved here is essentially a source transofmration
> > and should be possible to do straightforwardly with an autogenerated
> > include file, a couple of Makefile rules and some scripting or
> > preprocessor tricks to paste the relevant entries into a common
> > array.
> 
> But the result is more hackish than this. ELF sections are well

I'm not entirely convinced that it _can't_ be done in a less hack-ish
way... but I confess that my own attempts did end up in a bit of a
mess.  So I can't really argue my point there.

> understood and widely used in this environment, so the solution is
> proved working. Scripting otherwise is an unneeded complication, as
> every user will have to know a  new technique, which may break in
> the future (e.g. gmake 3.82 broke some makefiles: each new trick is
> a new risk).

As I said, it's partly a matter of personal taste.  I'm happy to go with
other people's judgement.  Sticking with something people are used to
certainly has value.

Cheers
---Dave

Re: [RGC PATCH 0/2] split file2alias using elf sections

[Greg KH]

On Fri, Nov 04, 2011 at 01:23:00PM +0100, Alessandro Rubini wrote:
> When adding a new bus type with autoloading of modules, people should
> always change the global scripts/mod/file2alias.c source file, whereas
> most of the new code is just new files and individual Makefile/Kconfig
> lines.
> 
> The first patch turns all the "normal" alias generation in a
> table-driven loop. The second patch moves a few alias types out of the
> main file2alias.c source, as a demonstration that the thing works.
> 
> I didn't move all bus/alias types out of the main file, as it's a huge
> work. But if the approach is going to be accepted I can do that (or
> happily leave the task to who volunteers).

Yes!!!!

I've been wanting this for years, and so have others (Linus, Andrew,
etc.)

I'll gladly take this through my driver-core tree, as it can handle
cross-bus issues like this.

Do you feel this series is good enough to accept, or do you want to
respin it again?

thanks,

greg k-h

Re: [RGC PATCH 0/2] split file2alias using elf sections

[Alessandro Rubini]

Thanks Greg for your interest.

> Do you feel this series is good enough to accept, or do you want to
> respin it again?

With the new AMBA stuff, I need to respin it.

/alessandro

Re: [RGC PATCH 0/2] split file2alias using elf sections

[Rusty Russell]

On Tue, 22 Nov 2011 11:23:00 -0800, Greg KH <greg@kroah.com> wrote:
> On Fri, Nov 04, 2011 at 01:23:00PM +0100, Alessandro Rubini wrote:
> > When adding a new bus type with autoloading of modules, people should
> > always change the global scripts/mod/file2alias.c source file, whereas
> > most of the new code is just new files and individual Makefile/Kconfig
> > lines.
> > 
> > The first patch turns all the "normal" alias generation in a
> > table-driven loop. The second patch moves a few alias types out of the
> > main file2alias.c source, as a demonstration that the thing works.
> > 
> > I didn't move all bus/alias types out of the main file, as it's a huge
> > work. But if the approach is going to be accepted I can do that (or
> > happily leave the task to who volunteers).
> 
> Yes!!!!
> 
> I've been wanting this for years, and so have others (Linus, Andrew,
> etc.)
> 
> I'll gladly take this through my driver-core tree, as it can handle
> cross-bus issues like this.
> 
> Do you feel this series is good enough to accept, or do you want to
> respin it again?

The table driven loop is nice and simple, though the E() macro is a bit
contrived.

The separation into separate files just to avoid merge issues is
overkill and useless churn.  The conflicts are simple, and you've just
moved themto conflicts in the Makefile.

We'd be better off putting the table in alphabetical order and stopping
there.  Easy code, no tricks.

Cheers,
Rusty.

Re: [RGC PATCH 0/2] split file2alias using elf sections

[Alessandro Rubini]

Hello Rusty, thankyou for your feedback.
I plan to make V2 tomorrow (it was planned much earlier).

> The table driven loop is nice and simple, though the E() macro is a bit
> contrived.

Yes, but we have long names and I wanted to avoid splitting lines.
I can make it ENTRY().
 
> The separation into separate files just to avoid merge issues is
> overkill and useless churn.  The conflicts are simple, and you've just
> moved themto conflicts in the Makefile.

Yes and no. Actually, I expect several new buses to appear over time,
and being separate files they can even be conditionally compiled based
on config. 

> We'd be better off putting the table in alphabetical order and stopping
> there.  Easy code, no tricks.

I see. I'm submitting two patches anyways, but you can pick one only.

Thanks
/alessandro