[PATCH 0/3] Bring SFI support to out-of-tree driver modules on Intel Mid

[PATCH 0/3] Bring SFI support to out-of-tree driver modules on Intel Mid

[David Cohen]

Hi,

This patchset extends sfi_device() macro support to driver modules.
The main use case is to allow external driver modules to be enumerated by SFI
on Intel Mid platforms.

Br, David Cohen

---
David Cohen (3):
  sfi: add private data to sfi_parse_table()
  x86: intel-mid: struct devs_id.name should have 'SFI_NAME_LEN' length
  x86: intel-mid: allow sfi_device() to be used by modules

 arch/x86/include/asm/intel-mid.h             | 19 ++++++--
 arch/x86/platform/intel-mid/intel-mid.c      |  2 +-
 arch/x86/platform/intel-mid/intel_mid_vrtc.c |  2 +-
 arch/x86/platform/intel-mid/sfi.c            | 70 +++++++++++++++++++++-------
 arch/x86/platform/sfi/sfi.c                  |  8 ++--
 drivers/sfi/sfi_acpi.c                       |  5 +-
 drivers/sfi/sfi_core.c                       |  4 +-
 include/linux/sfi.h                          |  8 ++--
 8 files changed, 84 insertions(+), 34 deletions(-)

-- 
1.8.4.2

[PATCH 1/3] sfi: add private data to sfi_parse_table()

[David Cohen]

sfi_parse_table() receives a callback as argument to call during its
execution. This patch adds private data argument to send to such
callback.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
Cc: Len Brown <lenb@kernel.org>
---
 arch/x86/include/asm/intel-mid.h             |  4 ++--
 arch/x86/platform/intel-mid/intel-mid.c      |  2 +-
 arch/x86/platform/intel-mid/intel_mid_vrtc.c |  2 +-
 arch/x86/platform/intel-mid/sfi.c            | 13 +++++++------
 arch/x86/platform/sfi/sfi.c                  |  8 ++++----
 drivers/sfi/sfi_acpi.c                       |  5 +++--
 drivers/sfi/sfi_core.c                       |  4 ++--
 include/linux/sfi.h                          |  8 +++++---
 8 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h
index e34e097b6f9d..c810abd5178c 100644
--- a/arch/x86/include/asm/intel-mid.h
+++ b/arch/x86/include/asm/intel-mid.h
@@ -17,8 +17,8 @@
 extern int intel_mid_pci_init(void);
 extern int get_gpio_by_name(const char *name);
 extern void intel_scu_device_register(struct platform_device *pdev);
-extern int __init sfi_parse_mrtc(struct sfi_table_header *table);
-extern int __init sfi_parse_mtmr(struct sfi_table_header *table);
+extern int __init sfi_parse_mrtc(struct sfi_table_header *table, void *const priv);
+extern int __init sfi_parse_mtmr(struct sfi_table_header *table, void *const priv);
 extern int sfi_mrtc_num;
 extern struct sfi_rtc_table_entry sfi_mrtc_array[];
 
diff --git a/arch/x86/platform/intel-mid/intel-mid.c b/arch/x86/platform/intel-mid/intel-mid.c
index 1bbedc4b0f88..68124ed102c3 100644
--- a/arch/x86/platform/intel-mid/intel-mid.c
+++ b/arch/x86/platform/intel-mid/intel-mid.c
@@ -83,7 +83,7 @@ static unsigned long __init intel_mid_calibrate_tsc(void)
 
 static void __init intel_mid_time_init(void)
 {
-	sfi_table_parse(SFI_SIG_MTMR, NULL, NULL, sfi_parse_mtmr);
+	sfi_table_parse(SFI_SIG_MTMR, NULL, NULL, sfi_parse_mtmr, NULL);
 	switch (intel_mid_timer_options) {
 	case INTEL_MID_TIMER_APBT_ONLY:
 		break;
diff --git a/arch/x86/platform/intel-mid/intel_mid_vrtc.c b/arch/x86/platform/intel-mid/intel_mid_vrtc.c
index 4762cff7facd..3afb51ad745b 100644
--- a/arch/x86/platform/intel-mid/intel_mid_vrtc.c
+++ b/arch/x86/platform/intel-mid/intel_mid_vrtc.c
@@ -120,7 +120,7 @@ void __init intel_mid_rtc_init(void)
 {
 	unsigned long vrtc_paddr;
 
-	sfi_table_parse(SFI_SIG_MRTC, NULL, NULL, sfi_parse_mrtc);
+	sfi_table_parse(SFI_SIG_MRTC, NULL, NULL, sfi_parse_mrtc, NULL);
 
 	vrtc_paddr = sfi_mrtc_array[0].phys_addr;
 	if (!sfi_mrtc_num || !vrtc_paddr)
diff --git a/arch/x86/platform/intel-mid/sfi.c b/arch/x86/platform/intel-mid/sfi.c
index 438306ebed05..2c66024b7067 100644
--- a/arch/x86/platform/intel-mid/sfi.c
+++ b/arch/x86/platform/intel-mid/sfi.c
@@ -73,7 +73,7 @@ EXPORT_SYMBOL_GPL(intel_scu_notifier);
 	((dev)->get_platform_data ? (dev)->get_platform_data(priv) : NULL)
 
 /* parse all the mtimer info to a static mtimer array */
-int __init sfi_parse_mtmr(struct sfi_table_header *table)
+int __init sfi_parse_mtmr(struct sfi_table_header *table, void *const priv)
 {
 	struct sfi_table_simple *sb;
 	struct sfi_timer_table_entry *pentry;
@@ -146,7 +146,7 @@ void sfi_free_mtmr(struct sfi_timer_table_entry *mtmr)
 }
 
 /* parse all the mrtc info to a global mrtc array */
-int __init sfi_parse_mrtc(struct sfi_table_header *table)
+int __init sfi_parse_mrtc(struct sfi_table_header *table, void *const priv)
 {
 	struct sfi_table_simple *sb;
 	struct sfi_rtc_table_entry *pentry;
@@ -185,7 +185,8 @@ int __init sfi_parse_mrtc(struct sfi_table_header *table)
  * Parsing GPIO table first, since the DEVS table will need this table
  * to map the pin name to the actual pin.
  */
-static int __init sfi_parse_gpio(struct sfi_table_header *table)
+static int __init sfi_parse_gpio(struct sfi_table_header *table,
+				 void *const priv)
 {
 	struct sfi_table_simple *sb;
 	struct sfi_gpio_table_entry *pentry;
@@ -427,7 +428,7 @@ static struct devs_id __init *get_device_id(u8 type, char *name)
 	return NULL;
 }
 
-static int __init sfi_parse_devs(struct sfi_table_header *table)
+static int __init sfi_parse_devs(struct sfi_table_header *table, void *const priv)
 {
 	struct sfi_table_simple *sb;
 	struct sfi_device_table_entry *pentry;
@@ -509,8 +510,8 @@ static int __init sfi_parse_devs(struct sfi_table_header *table)
 
 static int __init intel_mid_platform_init(void)
 {
-	sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio);
-	sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs);
+	sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio, NULL);
+	sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_devs, NULL);
 	return 0;
 }
 arch_initcall(intel_mid_platform_init);
diff --git a/arch/x86/platform/sfi/sfi.c b/arch/x86/platform/sfi/sfi.c
index bcd1a703e3e6..8b022f5e30e8 100644
--- a/arch/x86/platform/sfi/sfi.c
+++ b/arch/x86/platform/sfi/sfi.c
@@ -48,7 +48,7 @@ static void __init mp_sfi_register_lapic(u8 id)
 	generic_processor_info(id, GET_APIC_VERSION(apic_read(APIC_LVR)));
 }
 
-static int __init sfi_parse_cpus(struct sfi_table_header *table)
+static int __init sfi_parse_cpus(struct sfi_table_header *table, void *priv)
 {
 	struct sfi_table_simple *sb;
 	struct sfi_cpu_table_entry *pentry;
@@ -71,7 +71,7 @@ static int __init sfi_parse_cpus(struct sfi_table_header *table)
 
 #ifdef CONFIG_X86_IO_APIC
 
-static int __init sfi_parse_ioapic(struct sfi_table_header *table)
+static int __init sfi_parse_ioapic(struct sfi_table_header *table, void *priv)
 {
 	struct sfi_table_simple *sb;
 	struct sfi_apic_table_entry *pentry;
@@ -100,10 +100,10 @@ int __init sfi_platform_init(void)
 {
 #ifdef CONFIG_X86_LOCAL_APIC
 	register_lapic_address(sfi_lapic_addr);
-	sfi_table_parse(SFI_SIG_CPUS, NULL, NULL, sfi_parse_cpus);
+	sfi_table_parse(SFI_SIG_CPUS, NULL, NULL, sfi_parse_cpus, NULL);
 #endif
 #ifdef CONFIG_X86_IO_APIC
-	sfi_table_parse(SFI_SIG_APIC, NULL, NULL, sfi_parse_ioapic);
+	sfi_table_parse(SFI_SIG_APIC, NULL, NULL, sfi_parse_ioapic, NULL);
 #endif
 	return 0;
 }
diff --git a/drivers/sfi/sfi_acpi.c b/drivers/sfi/sfi_acpi.c
index f5b4ca581541..be71be084a5f 100644
--- a/drivers/sfi/sfi_acpi.c
+++ b/drivers/sfi/sfi_acpi.c
@@ -96,7 +96,8 @@ static inline struct acpi_table_header *sfi_to_acpi_th(
  *
  * Parse the ACPI XSDT for later access by sfi_acpi_table_parse().
  */
-static int __init sfi_acpi_parse_xsdt(struct sfi_table_header *th)
+static int __init
+sfi_acpi_parse_xsdt(struct sfi_table_header *th, void *const priv)
 {
 	struct sfi_table_key key = SFI_ANY_KEY;
 	int tbl_cnt, i;
@@ -119,7 +120,7 @@ int __init sfi_acpi_init(void)
 {
 	struct sfi_table_key xsdt_key = { .sig = SFI_SIG_XSDT };
 
-	sfi_table_parse(SFI_SIG_XSDT, NULL, NULL, sfi_acpi_parse_xsdt);
+	sfi_table_parse(SFI_SIG_XSDT, NULL, NULL, sfi_acpi_parse_xsdt, NULL);
 
 	/* Only call the get_table to keep the table mapped */
 	xsdt_va = (struct acpi_table_xsdt *)sfi_get_table(&xsdt_key);
diff --git a/drivers/sfi/sfi_core.c b/drivers/sfi/sfi_core.c
index 1e824fb1649b..8289d95be04a 100644
--- a/drivers/sfi/sfi_core.c
+++ b/drivers/sfi/sfi_core.c
@@ -278,7 +278,7 @@ void sfi_put_table(struct sfi_table_header *th)
 
 /* Find table with signature, run handler on it */
 int sfi_table_parse(char *signature, char *oem_id, char *oem_table_id,
-			sfi_table_handler handler)
+			sfi_table_handler handler, void *const priv)
 {
 	struct sfi_table_header *table = NULL;
 	struct sfi_table_key key;
@@ -295,7 +295,7 @@ int sfi_table_parse(char *signature, char *oem_id, char *oem_table_id,
 	if (!table)
 		goto exit;
 
-	ret = handler(table);
+	ret = handler(table, priv);
 	sfi_put_table(table);
 exit:
 	return ret;
diff --git a/include/linux/sfi.h b/include/linux/sfi.h
index d9b436f09925..3d2be5c02256 100644
--- a/include/linux/sfi.h
+++ b/include/linux/sfi.h
@@ -170,14 +170,15 @@ struct sfi_gpio_table_entry {
 	char	pin_name[SFI_NAME_LEN];
 } __packed;
 
-typedef int (*sfi_table_handler) (struct sfi_table_header *table);
+typedef int (*sfi_table_handler) (struct sfi_table_header *table,
+				  void *const priv);
 
 #ifdef CONFIG_SFI
 extern void __init sfi_init(void);
 extern int __init sfi_platform_init(void);
 extern void __init sfi_init_late(void);
 extern int sfi_table_parse(char *signature, char *oem_id, char *oem_table_id,
-				sfi_table_handler handler);
+				sfi_table_handler handler, void *const priv);
 
 extern int sfi_disabled;
 static inline void disable_sfi(void)
@@ -199,7 +200,8 @@ static inline void sfi_init_late(void)
 
 static inline int sfi_table_parse(char *signature, char *oem_id,
 					char *oem_table_id,
-					sfi_table_handler handler)
+					sfi_table_handler handler,
+					void *const priv)
 {
 	return -1;
 }
-- 
1.8.4.2

[PATCH 2/3] x86: intel-mid: struct devs_id.name should have 'SFI_NAME_LEN' length

[David Cohen]

This patch fixes struct devs_id.name to match size of
struct sfi_device_table_entry.size.
Correct size if SFI_NAME_LEN instead of SFI_NAME_LEN + 1.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---
 arch/x86/include/asm/intel-mid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h
index c810abd5178c..971cbb1abab3 100644
--- a/arch/x86/include/asm/intel-mid.h
+++ b/arch/x86/include/asm/intel-mid.h
@@ -28,7 +28,7 @@ extern struct sfi_rtc_table_entry sfi_mrtc_array[];
  * its platform data.
  */
 struct devs_id {
-	char name[SFI_NAME_LEN + 1];
+	char name[SFI_NAME_LEN];
 	u8 type;
 	u8 delay;
 	void *(*get_platform_data)(void *info);
-- 
1.8.4.2

[PATCH 3/3] x86: intel-mid: allow sfi_device() to be used by modules

[David Cohen]

This patch implements support for sfi_device() to be called from
driver modules. This is useful to bring SFI support to out-of-tree
modules.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---
 arch/x86/include/asm/intel-mid.h  | 13 +++++++++
 arch/x86/platform/intel-mid/sfi.c | 59 ++++++++++++++++++++++++++++++---------
 2 files changed, 59 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/intel-mid.h b/arch/x86/include/asm/intel-mid.h
index 971cbb1abab3..92b6dc333197 100644
--- a/arch/x86/include/asm/intel-mid.h
+++ b/arch/x86/include/asm/intel-mid.h
@@ -11,6 +11,7 @@
 #ifndef _ASM_X86_INTEL_MID_H
 #define _ASM_X86_INTEL_MID_H
 
+#include <linux/module.h>
 #include <linux/sfi.h>
 #include <linux/platform_device.h>
 
@@ -21,6 +22,7 @@ extern int __init sfi_parse_mrtc(struct sfi_table_header *table, void *const pri
 extern int __init sfi_parse_mtmr(struct sfi_table_header *table, void *const priv);
 extern int sfi_mrtc_num;
 extern struct sfi_rtc_table_entry sfi_mrtc_array[];
+extern int sfi_parse_one_dev(struct sfi_table_header *table, void *const priv);
 
 /*
  * Here defines the array of devices platform data that IAFW would export
@@ -37,9 +39,20 @@ struct devs_id {
 				struct devs_id *dev);
 };
 
+#ifndef MODULE
 #define sfi_device(i)   \
 	static const struct devs_id *const __intel_mid_sfi_##i##_dev __used \
 	__attribute__((__section__(".x86_intel_mid_dev.init"))) = &i
+#else
+#define sfi_device(i)	\
+	static int __init i##_as_module(void)		\
+	{						\
+		sfi_table_parse(SFI_SIG_DEVS, NULL, NULL, sfi_parse_one_dev, \
+							   (void *const)&i); \
+		return 0;				\
+	}						\
+	module_init(i##_as_module);
+#endif
 
 /*
  * Medfield is the follow-up of Moorestown, it combines two chip solution into
diff --git a/arch/x86/platform/intel-mid/sfi.c b/arch/x86/platform/intel-mid/sfi.c
index 2c66024b7067..fb62fad61392 100644
--- a/arch/x86/platform/intel-mid/sfi.c
+++ b/arch/x86/platform/intel-mid/sfi.c
@@ -227,6 +227,7 @@ int get_gpio_by_name(const char *name)
 	}
 	return -EINVAL;
 }
+EXPORT_SYMBOL_GPL(get_gpio_by_name);
 
 void __init intel_scu_device_register(struct platform_device *pdev)
 {
@@ -318,10 +319,10 @@ void intel_scu_devices_destroy(void)
 }
 EXPORT_SYMBOL_GPL(intel_scu_devices_destroy);
 
-static void __init install_irq_resource(struct platform_device *pdev, int irq)
+static void install_irq_resource(struct platform_device *pdev, int irq)
 {
 	/* Single threaded */
-	static struct resource res __initdata = {
+	static struct resource res = {
 		.name = "IRQ",
 		.flags = IORESOURCE_IRQ,
 	};
@@ -329,8 +330,8 @@ static void __init install_irq_resource(struct platform_device *pdev, int irq)
 	platform_device_add_resources(pdev, &res, 1);
 }
 
-static void __init sfi_handle_ipc_dev(struct sfi_device_table_entry *pentry,
-					struct devs_id *dev)
+static void sfi_handle_ipc_dev(struct sfi_device_table_entry *pentry,
+			       struct devs_id *dev)
 {
 	struct platform_device *pdev;
 	void *pdata = NULL;
@@ -353,8 +354,8 @@ static void __init sfi_handle_ipc_dev(struct sfi_device_table_entry *pentry,
 	platform_device_add(pdev);
 }
 
-static void __init sfi_handle_spi_dev(struct sfi_device_table_entry *pentry,
-					struct devs_id *dev)
+static void sfi_handle_spi_dev(struct sfi_device_table_entry *pentry,
+				struct devs_id *dev)
 {
 	struct spi_board_info spi_info;
 	void *pdata = NULL;
@@ -383,8 +384,8 @@ static void __init sfi_handle_spi_dev(struct sfi_device_table_entry *pentry,
 		spi_register_board_info(&spi_info, 1);
 }
 
-static void __init sfi_handle_i2c_dev(struct sfi_device_table_entry *pentry,
-					struct devs_id *dev)
+static void sfi_handle_i2c_dev(struct sfi_device_table_entry *pentry,
+				struct devs_id *dev, int from_module)
 {
 	struct i2c_board_info i2c_info;
 	void *pdata = NULL;
@@ -405,14 +406,20 @@ static void __init sfi_handle_i2c_dev(struct sfi_device_table_entry *pentry,
 
 	if (dev->delay)
 		intel_scu_i2c_device_register(pentry->host_num, &i2c_info);
-	else
+	else if (!from_module)
+		/*
+		 * Despite i2c_register_board_info() is __init, we have a
+		 * false-positive section mismatch here: this function is not
+		 * allowed to be called from non-__init source with
+		 * @from_module == 0.
+		 */
 		i2c_register_board_info(pentry->host_num, &i2c_info, 1);
 }
 
 extern struct devs_id *const __x86_intel_mid_dev_start[],
 		      *const __x86_intel_mid_dev_end[];
 
-static struct devs_id __init *get_device_id(u8 type, char *name)
+static struct devs_id *get_device_id(u8 type, char *name)
 {
 	struct devs_id *const *dev_table;
 
@@ -428,8 +435,9 @@ static struct devs_id __init *get_device_id(u8 type, char *name)
 	return NULL;
 }
 
-static int __init sfi_parse_devs(struct sfi_table_header *table, void *const priv)
+static int __sfi_parse_devs(struct sfi_table_header *table, void *const priv)
 {
+	struct devs_id *const devid = priv;
 	struct sfi_table_simple *sb;
 	struct sfi_device_table_entry *pentry;
 	struct devs_id *dev = NULL;
@@ -480,7 +488,16 @@ static int __init sfi_parse_devs(struct sfi_table_header *table, void *const pri
 			irq = 0; /* No irq */
 		}
 
-		dev = get_device_id(pentry->type, pentry->name);
+		/*
+		 * If @devid is NULL we want to go through the whole static
+		 * dev_ids list by calling get_device_id()
+		 */
+		if (likely(!devid))
+			dev = get_device_id(pentry->type, pentry->name);
+		else if (devid->type == pentry->type &&
+			 !strncmp(devid->name, pentry->name,
+				  sizeof(devid->name)))
+			dev = devid;
 
 		if (!dev)
 			continue;
@@ -496,7 +513,7 @@ static int __init sfi_parse_devs(struct sfi_table_header *table, void *const pri
 				sfi_handle_spi_dev(pentry, dev);
 				break;
 			case SFI_DEV_TYPE_I2C:
-				sfi_handle_i2c_dev(pentry, dev);
+				sfi_handle_i2c_dev(pentry, dev, !!devid);
 				break;
 			case SFI_DEV_TYPE_UART:
 			case SFI_DEV_TYPE_HSI:
@@ -504,10 +521,26 @@ static int __init sfi_parse_devs(struct sfi_table_header *table, void *const pri
 				break;
 			}
 		}
+		if (unlikely(devid))
+			/* We found the one we're looking for */
+			return 0;
 	}
+
 	return 0;
 }
 
+int sfi_parse_one_dev(struct sfi_table_header *table, void *const priv)
+{
+	return priv ? __sfi_parse_devs(table, priv) : -EINVAL;
+}
+EXPORT_SYMBOL_GPL(sfi_parse_one_dev);
+
+static int __init
+sfi_parse_devs(struct sfi_table_header *table, void *const priv)
+{
+	return __sfi_parse_devs(table, NULL);
+}
+
 static int __init intel_mid_platform_init(void)
 {
 	sfi_table_parse(SFI_SIG_GPIO, NULL, NULL, sfi_parse_gpio, NULL);
-- 
1.8.4.2

Re: [PATCH 0/3] Bring SFI support to out-of-tree driver modules on Intel Mid

[Christoph Hellwig]

On Tue, Nov 12, 2013 at 02:13:37PM -0800, David Cohen wrote:
> Hi,
> 
> This patchset extends sfi_device() macro support to driver modules.
> The main use case is to allow external driver modules to be enumerated by SFI
> on Intel Mid platforms.

How about you merge those module again?  Remember code added to the
kernel without users isn't testable, and out of tree modules do not
bring us any value add.

Re: [PATCH 0/3] Bring SFI support to out-of-tree driver modules on Intel Mid

[Ingo Molnar]

* Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, Nov 12, 2013 at 02:13:37PM -0800, David Cohen wrote:
>
> > Hi,
> > 
> > This patchset extends sfi_device() macro support to driver modules. 
> > The main use case is to allow external driver modules to be enumerated 
> > by SFI on Intel Mid platforms.
> 
> How about you merge those module again?  Remember code added to the 
> kernel without users isn't testable, and out of tree modules do not 
> bring us any value add.

I wanted to make the exact same point.

I recently had to revert a similarly misguided attempt which added bloat 
for out of tree modules without merging it in tree, see commit 
b5dfcb09debc ("Revert "x86/UV: Add uvtrace support").

Thanks,

	Ingo

Re: [PATCH 0/3] Bring SFI support to out-of-tree driver modules on Intel Mid

[David Cohen]

On 11/13/2013 03:19 AM, Ingo Molnar wrote:
>
> * Christoph Hellwig <hch@infradead.org> wrote:
>
>> On Tue, Nov 12, 2013 at 02:13:37PM -0800, David Cohen wrote:
>>
>>> Hi,
>>>
>>> This patchset extends sfi_device() macro support to driver modules.
>>> The main use case is to allow external driver modules to be enumerated
>>> by SFI on Intel Mid platforms.
>>
>> How about you merge those module again?  Remember code added to the
>> kernel without users isn't testable, and out of tree modules do not
>> bring us any value add.
>
> I wanted to make the exact same point.
>
> I recently had to revert a similarly misguided attempt which added bloat
> for out of tree modules without merging it in tree, see commit
> b5dfcb09debc ("Revert "x86/UV: Add uvtrace support").

I was a bit reluctant in sending these patches, but I can tell my
background to explain why:

I work most of my time with embedded platforms and I got the duty to
maintain and sync Intel Mid codes from our internal tree to upstream
(if you check out there Android trees supporting Intel platform you'll
see it's completely different to what we have officially on Linux).

Unfortunately some drivers depend on Intel Mid to support a legacy
device enumeration with SFI + platform codes (these platform codes are
placed into arch/x86/platform/intel-mid/device_libs/). It means I need
to support in my internal tree files that belong to drivers I don't
own. Most of times it's OK, but regardless my will, we've drivers that
can't be upstreamed right away.

In order to keep sanity in Intel Mid codes between internal tree and
upstream, I'm trying to make non-upstreamable drivers to not mix
platform codes to Intel Mid until they are ready to get merged to Linux
kernel officially. This change makes Linux more friendly for drivers to
reach time-to-market and do upstream work at same time.
One other reason is to keep my internal tree closer to upstream and
make things easier for upstreaming of Intel Mid patches themselves
(which is my main interest).

But this code is testable. I used the word "external" because all the
platform code merged into Linux are compiled as builtin. But my tests
prior to send these patches were done by converting some of those codes
to be compiled as module (I am glad to share the patches for tests).

Br, David Cohen

>
> Thanks,
>
> 	Ingo
>

[PATCH] x86: intel-mid: add test module for sfi_device()

[David Cohen]

This patch adds a test module to validate sfi_device() when used from a
driver module.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---

Hi Ingo and Christoph,

Does this test module make sense to you?
This could be the 4th patch of this series.

Br, David

---
 arch/x86/Kconfig                                   |  9 +++++
 arch/x86/platform/intel-mid/device_libs/Makefile   |  2 ++
 .../intel-mid/device_libs/platform_sfi_test.c      | 42 ++++++++++++++++++++++
 3 files changed, 53 insertions(+)
 create mode 100644 arch/x86/platform/intel-mid/device_libs/platform_sfi_test.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index c5f3414e453d..0b55a7742b65 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -457,6 +457,15 @@ config X86_INTEL_MID
 	  systems which do not have the PCI legacy interfaces (Moorestown,
 	  Medfield). If you are building for a PC class system say N here.
 
+config SFI_TEST_MODULE
+	tristate "Intel MID sfi test module"
+	depends on SFI && X86_INTEL_MID && m
+	---help---
+	  Select to build a test module to validate SFI devices on Intel MID
+	  platforms from driver modules.
+
+	  This module will be called platform_sfi_test.ko
+
 config X86_INTEL_LPSS
 	bool "Intel Low Power Subsystem Support"
 	depends on ACPI
diff --git a/arch/x86/platform/intel-mid/device_libs/Makefile b/arch/x86/platform/intel-mid/device_libs/Makefile
index 097e7a7940d8..b4e858e5c6e9 100644
--- a/arch/x86/platform/intel-mid/device_libs/Makefile
+++ b/arch/x86/platform/intel-mid/device_libs/Makefile
@@ -20,3 +20,5 @@ obj-$(subst m,y,$(CONFIG_DRM_MEDFIELD)) += platform_tc35876x.o
 obj-$(subst m,y,$(CONFIG_SERIAL_MRST_MAX3110)) += platform_max3111.o
 # MISC Devices
 obj-$(subst m,y,$(CONFIG_KEYBOARD_GPIO)) += platform_gpio_keys.o
+# Test
+obj-$(CONFIG_SFI_TEST_MODULE) += platform_sfi_test.o
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_sfi_test.c b/arch/x86/platform/intel-mid/device_libs/platform_sfi_test.c
new file mode 100644
index 000000000000..79cc18461eba
--- /dev/null
+++ b/arch/x86/platform/intel-mid/device_libs/platform_sfi_test.c
@@ -0,0 +1,42 @@
+/*
+ * platform_sfi_test.c: test module for sfi_device()
+ *
+ * (C) Copyright 2013 Intel Corporation
+ * Author: David Cohen <david.a.cohen@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+
+#include <linux/module.h>
+#include <asm/intel-mid.h>
+
+/*
+ * Even though we're implementing this device_handler callback, it should
+ * never be called when loading the module because the sfi device name
+ * "I'm_not_here" won't match to any entry on sfi table.
+ *
+ * This test validates compilation of sfi_device() in modules and validates
+ * that the attempt to enumerate an sfi device as module doesn't goes bad
+ * (despite the sfi module won't be found, the code to do the match it will be
+ * used).
+ */
+static void
+sfi_test_dev_handler(struct sfi_device_table_entry *pentry, struct devs_id *dev)
+{
+	pr_info("%s: This is a test for sfi_device() as module\n", __func__);
+}
+
+static const struct devs_id sfi_test_dev_id __initconst = {
+	.name = "I'm_not_here",
+	.type = SFI_DEV_TYPE_UART,
+	.device_handler = sfi_test_dev_handler,
+};
+
+sfi_device(sfi_test_dev_id);
+
+MODULE_AUTHOR("David Cohen <david.a.cohen@linux.intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("This module provides test for sfi_device() macro");
-- 
1.8.4.2

Re: [PATCH] x86: intel-mid: add test module for sfi_device()

[David Cohen]

On 11/13/2013 11:29 AM, David Cohen wrote:
> This patch adds a test module to validate sfi_device() when used from a
> driver module.
>
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ---
>
> Hi Ingo and Christoph,
>
> Does this test module make sense to you?
> This could be the 4th patch of this series.
>
> Br, David
>
> ---
>   arch/x86/Kconfig                                   |  9 +++++
>   arch/x86/platform/intel-mid/device_libs/Makefile   |  2 ++
>   .../intel-mid/device_libs/platform_sfi_test.c      | 42 ++++++++++++++++++++++
>   3 files changed, 53 insertions(+)
>   create mode 100644 arch/x86/platform/intel-mid/device_libs/platform_sfi_test.c
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index c5f3414e453d..0b55a7742b65 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -457,6 +457,15 @@ config X86_INTEL_MID
>   	  systems which do not have the PCI legacy interfaces (Moorestown,
>   	  Medfield). If you are building for a PC class system say N here.
>
> +config SFI_TEST_MODULE
> +	tristate "Intel MID sfi test module"
> +	depends on SFI && X86_INTEL_MID && m
> +	---help---
> +	  Select to build a test module to validate SFI devices on Intel MID
> +	  platforms from driver modules.
> +
> +	  This module will be called platform_sfi_test.ko
> +
>   config X86_INTEL_LPSS
>   	bool "Intel Low Power Subsystem Support"
>   	depends on ACPI
> diff --git a/arch/x86/platform/intel-mid/device_libs/Makefile b/arch/x86/platform/intel-mid/device_libs/Makefile
> index 097e7a7940d8..b4e858e5c6e9 100644
> --- a/arch/x86/platform/intel-mid/device_libs/Makefile
> +++ b/arch/x86/platform/intel-mid/device_libs/Makefile
> @@ -20,3 +20,5 @@ obj-$(subst m,y,$(CONFIG_DRM_MEDFIELD)) += platform_tc35876x.o
>   obj-$(subst m,y,$(CONFIG_SERIAL_MRST_MAX3110)) += platform_max3111.o
>   # MISC Devices
>   obj-$(subst m,y,$(CONFIG_KEYBOARD_GPIO)) += platform_gpio_keys.o
> +# Test
> +obj-$(CONFIG_SFI_TEST_MODULE) += platform_sfi_test.o
> diff --git a/arch/x86/platform/intel-mid/device_libs/platform_sfi_test.c b/arch/x86/platform/intel-mid/device_libs/platform_sfi_test.c
> new file mode 100644
> index 000000000000..79cc18461eba
> --- /dev/null
> +++ b/arch/x86/platform/intel-mid/device_libs/platform_sfi_test.c
> @@ -0,0 +1,42 @@
> +/*
> + * platform_sfi_test.c: test module for sfi_device()
> + *
> + * (C) Copyright 2013 Intel Corporation
> + * Author: David Cohen <david.a.cohen@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +
> +#include <linux/module.h>
> +#include <asm/intel-mid.h>
> +
> +/*
> + * Even though we're implementing this device_handler callback, it should
> + * never be called when loading the module because the sfi device name
> + * "I'm_not_here" won't match to any entry on sfi table.
> + *
> + * This test validates compilation of sfi_device() in modules and validates
> + * that the attempt to enumerate an sfi device as module doesn't goes bad
> + * (despite the sfi module won't be found, the code to do the match it will be
> + * used).

I missed few typos on this comment but I can fix it if this code is
fine.

Br, David

> + */
> +static void
> +sfi_test_dev_handler(struct sfi_device_table_entry *pentry, struct devs_id *dev)
> +{
> +	pr_info("%s: This is a test for sfi_device() as module\n", __func__);
> +}
> +
> +static const struct devs_id sfi_test_dev_id __initconst = {
> +	.name = "I'm_not_here",
> +	.type = SFI_DEV_TYPE_UART,
> +	.device_handler = sfi_test_dev_handler,
> +};
> +
> +sfi_device(sfi_test_dev_id);
> +
> +MODULE_AUTHOR("David Cohen <david.a.cohen@linux.intel.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("This module provides test for sfi_device() macro");
>

Re: [PATCH 0/3] Bring SFI support to out-of-tree driver modules on Intel Mid

[David Cohen]

On 11/13/2013 12:14 AM, Christoph Hellwig wrote:
> On Tue, Nov 12, 2013 at 02:13:37PM -0800, David Cohen wrote:
>> Hi,

Hi Christoph,

>>
>> This patchset extends sfi_device() macro support to driver modules.
>> The main use case is to allow external driver modules to be enumerated by SFI
>> on Intel Mid platforms.
>
> How about you merge those module again?  Remember code added to the
> kernel without users isn't testable, and out of tree modules do not
> bring us any value add.

I got your point.
Here's the testable user:
https://patchwork.kernel.org/patch/3179381/
By simply 'make ARCH=i386 allyesconfig' it will be selected.

But I think there is a misunderstanding here :)
My main target is to sync Intel MID codes from our internal tree to
upstream. These patches I am sending are useful to protect Intel MID
from drivers I don't own and not ready for upstreaming.
My main intention is to upstream more Intel MID codes in a faster and
more reliably way (if internal tree == upstream my tests will be
better).

Since Intel MID users are too sensitive to time to market (sorry to
bring this scenario but we can't run from it on embedded world) I can't
prevent drivers not ready yet for upstreaming to use sfi_device() macro.

So, given my intention is to increase upstreamed codes, do you think it
makes sense to have this sfi_device() modularization support?

Br, David Cohen

[PATCH v1.1] x86: intel-mid: add test module for sfi_device()

[David Cohen]

This patch adds a test module to validate sfi_device() when used from a
driver module.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---

v1 to v1.1:
 - Fixed typos on comment.
 - No functional changes

Br, David

---
 arch/x86/Kconfig                                   |  9 +++++
 arch/x86/platform/intel-mid/device_libs/Makefile   |  2 ++
 .../intel-mid/device_libs/platform_sfi_test.c      | 41 ++++++++++++++++++++++
 3 files changed, 52 insertions(+)
 create mode 100644 arch/x86/platform/intel-mid/device_libs/platform_sfi_test.c

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 00fa3b7d2abe..140e1de64d81 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -456,6 +456,15 @@ config X86_INTEL_MID
 	  systems which do not have the PCI legacy interfaces (Moorestown,
 	  Medfield). If you are building for a PC class system say N here.
 
+config SFI_TEST_MODULE
+	tristate "Intel MID sfi test module"
+	depends on SFI && X86_INTEL_MID && m
+	---help---
+	  Select to build a test module to validate SFI devices on Intel MID
+	  platforms from driver modules.
+
+	  This module will be called platform_sfi_test.ko
+
 config X86_INTEL_LPSS
 	bool "Intel Low Power Subsystem Support"
 	depends on ACPI
diff --git a/arch/x86/platform/intel-mid/device_libs/Makefile b/arch/x86/platform/intel-mid/device_libs/Makefile
index 097e7a7940d8..b4e858e5c6e9 100644
--- a/arch/x86/platform/intel-mid/device_libs/Makefile
+++ b/arch/x86/platform/intel-mid/device_libs/Makefile
@@ -20,3 +20,5 @@ obj-$(subst m,y,$(CONFIG_DRM_MEDFIELD)) += platform_tc35876x.o
 obj-$(subst m,y,$(CONFIG_SERIAL_MRST_MAX3110)) += platform_max3111.o
 # MISC Devices
 obj-$(subst m,y,$(CONFIG_KEYBOARD_GPIO)) += platform_gpio_keys.o
+# Test
+obj-$(CONFIG_SFI_TEST_MODULE) += platform_sfi_test.o
diff --git a/arch/x86/platform/intel-mid/device_libs/platform_sfi_test.c b/arch/x86/platform/intel-mid/device_libs/platform_sfi_test.c
new file mode 100644
index 000000000000..dc146f4a2c18
--- /dev/null
+++ b/arch/x86/platform/intel-mid/device_libs/platform_sfi_test.c
@@ -0,0 +1,41 @@
+/*
+ * platform_sfi_test.c: test module for sfi_device()
+ *
+ * (C) Copyright 2013 Intel Corporation
+ * Author: David Cohen <david.a.cohen@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+
+#include <linux/module.h>
+#include <asm/intel-mid.h>
+
+/*
+ * Even though we're implementing this device_handler callback, it should
+ * never be called when loading the module because the sfi device name
+ * "I'm_not_here" won't match to any entry on sfi table.
+ *
+ * This test validates sfi_device() compilation and usage in modules.
+ * Despite this sfi device test isn't found in the end, the module tests Intel
+ * MID sfi codes when trying to match this device with current sfi table.
+ */
+static void
+sfi_test_dev_handler(struct sfi_device_table_entry *pentry, struct devs_id *dev)
+{
+	pr_info("%s: This is a test for sfi_device() as module\n", __func__);
+}
+
+static const struct devs_id sfi_test_dev_id __initconst = {
+	.name = "I'm_not_here",
+	.type = SFI_DEV_TYPE_UART,
+	.device_handler = sfi_test_dev_handler,
+};
+
+sfi_device(sfi_test_dev_id);
+
+MODULE_AUTHOR("David Cohen <david.a.cohen@linux.intel.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("This module provides test for sfi_device() macro");
-- 
1.8.4.2

Re: [PATCH v1.1] x86: intel-mid: add test module for sfi_device()

[Christoph Hellwig]

On Fri, Nov 15, 2013 at 04:09:18PM -0800, David Cohen wrote:
> This patch adds a test module to validate sfi_device() when used from a
> driver module.

I don't think this is all that useful.  How about you prepeare a few
of the more useful drivers from your tree for submission instead?

Re: [PATCH v1.1] x86: intel-mid: add test module for sfi_device()

[David Cohen]

On 11/18/2013 07:28 AM, Christoph Hellwig wrote:
> On Fri, Nov 15, 2013 at 04:09:18PM -0800, David Cohen wrote:
>> This patch adds a test module to validate sfi_device() when used from a
>> driver module.
> 
> I don't think this is all that useful.  How about you prepeare a few
> of the more useful drivers from your tree for submission instead?

One of these drivers you can track here:
https://patchwork.kernel.org/patch/3109791/
This is necessary to enable serial console on Saltbay (Merrifield based
platform). But the driver is still being reworked to be upstreamed.

Anyway, upstream those drivers won't work to validate this patch set
we're discussion here. All platform codes are bool (can't be module).
The real purpose of these patches is to make my internal tree to be
equal to upstream. My intention is to upstream *all* internal patches
of Intel MID and temporarily move away from arch/x86/platform/intel-
mid/device_libs/ the platform code from still-on-staging-state drivers.

So, we need a dummy module on upstream to make this code testable.

In case this code is not accepted, I'll will have to maintain 2
official public branches: one with these patches and one without them.

Br, David

Re: [PATCH v1.1] x86: intel-mid: add test module for sfi_device()

[David Cohen]

On 11/18/2013 09:35 AM, David Cohen wrote:
> On 11/18/2013 07:28 AM, Christoph Hellwig wrote:
>> On Fri, Nov 15, 2013 at 04:09:18PM -0800, David Cohen wrote:
>>> This patch adds a test module to validate sfi_device() when used from a
>>> driver module.
>>
>> I don't think this is all that useful.  How about you prepeare a few
>> of the more useful drivers from your tree for submission instead?
> 
> One of these drivers you can track here:
> https://patchwork.kernel.org/patch/3109791/
> This is necessary to enable serial console on Saltbay (Merrifield based
> platform). But the driver is still being reworked to be upstreamed.
> 
> Anyway, upstream those drivers won't work to validate this patch set
> we're discussion here. All platform codes are bool (can't be module).

I meant: all upstreamed platform codes are bool.

> The real purpose of these patches is to make my internal tree to be
> equal to upstream. My intention is to upstream *all* internal patches
> of Intel MID and temporarily move away from arch/x86/platform/intel-
> mid/device_libs/ the platform code from still-on-staging-state drivers.
> 
> So, we need a dummy module on upstream to make this code testable.
> 
> In case this code is not accepted, I'll will have to maintain 2
> official public branches: one with these patches and one without them.
> 
> Br, David
> 

Re: [PATCH v1.1] x86: intel-mid: add test module for sfi_device()

[David Cohen]

Any comments on this?

Br, David

On 11/18/2013 09:37 AM, David Cohen wrote:
> On 11/18/2013 09:35 AM, David Cohen wrote:
>> On 11/18/2013 07:28 AM, Christoph Hellwig wrote:
>>> On Fri, Nov 15, 2013 at 04:09:18PM -0800, David Cohen wrote:
>>>> This patch adds a test module to validate sfi_device() when used from a
>>>> driver module.
>>>
>>> I don't think this is all that useful.  How about you prepeare a few
>>> of the more useful drivers from your tree for submission instead?
>>
>> One of these drivers you can track here:
>> https://patchwork.kernel.org/patch/3109791/
>> This is necessary to enable serial console on Saltbay (Merrifield based
>> platform). But the driver is still being reworked to be upstreamed.
>>
>> Anyway, upstream those drivers won't work to validate this patch set
>> we're discussion here. All platform codes are bool (can't be module).
> 
> I meant: all upstreamed platform codes are bool.
> 
>> The real purpose of these patches is to make my internal tree to be
>> equal to upstream. My intention is to upstream *all* internal patches
>> of Intel MID and temporarily move away from arch/x86/platform/intel-
>> mid/device_libs/ the platform code from still-on-staging-state drivers.
>>
>> So, we need a dummy module on upstream to make this code testable.
>>
>> In case this code is not accepted, I'll will have to maintain 2
>> official public branches: one with these patches and one without them.
>>
>> Br, David
>>
>