[PATCH 0/4] dmi: Rework to get IPMI autoloading from DMI tables

[PATCH 0/4] dmi: Rework to get IPMI autoloading from DMI tables

[minyard]

The IPMI driver would not auto-load from DMI tables.  So these patches
creates a platform device from an IPMI DMI table entry, and then
modify the IPMI driver to handle all this.

I followed how ACPI works mostly, with a fwnode and such.  But greatly
simplified, of course :).

I'm no sure if patch 4 is done in the right place.  Maybe it would
be better in the IPMI driver directory?  But it's pretty clean where
it is.

Also, I wasn't quite sure about the name of the device.  If the device
was named ipmi_si or ipmi_ssif, the driver override would not be
required, but then you really couldn't tell it came from DMI.

You could also create a firmware_node like ACPI does, but that might
be overkill.

-corey

[PATCH 1/4] dmi: remove const from return of dmi_find_device

[minyard]

From: Corey Minyard <cminyard@mvista.com>

A fwnode_handle is being added to dmi_device, and that will need to
be updated.  So remove the const.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Jean Delvare <jdelvare@suse.de>
Cc: Andy Lutomirski <luto@kernel.org>
---
 drivers/firmware/dmi_scan.c | 11 +++++------
 include/linux/dmi.h         | 10 +++++-----
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 88bebe1..da471b2 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -896,14 +896,13 @@ EXPORT_SYMBOL(dmi_name_in_vendors);
  *	A new search is initiated by passing %NULL as the @from argument.
  *	If @from is not %NULL, searches continue from next device.
  */
-const struct dmi_device *dmi_find_device(int type, const char *name,
-				    const struct dmi_device *from)
+struct dmi_device *dmi_find_device(int type, const char *name,
+				   const struct dmi_device *from)
 {
-	const struct list_head *head = from ? &from->list : &dmi_devices;
-	struct list_head *d;
+	struct list_head *d = from ? from->list.next : dmi_devices.next;
 
-	for (d = head->next; d != &dmi_devices; d = d->next) {
-		const struct dmi_device *dev =
+	for (; d != &dmi_devices; d = d->next) {
+		struct dmi_device *dev =
 			list_entry(d, struct dmi_device, list);
 
 		if (((type == DMI_DEV_TYPE_ANY) || (dev->type == type)) &&
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index 5e9c74c..a930a4d 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -98,9 +98,9 @@ struct dmi_dev_onboard {
 extern struct kobject *dmi_kobj;
 extern int dmi_check_system(const struct dmi_system_id *list);
 const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
-extern const char * dmi_get_system_info(int field);
-extern const struct dmi_device * dmi_find_device(int type, const char *name,
-	const struct dmi_device *from);
+extern const char *dmi_get_system_info(int field);
+extern struct dmi_device *dmi_find_device(int type, const char *name,
+	 const struct dmi_device *from);
 extern void dmi_scan_machine(void);
 extern void dmi_memdev_walk(void);
 extern void dmi_set_dump_stack_arch_desc(void);
@@ -116,8 +116,8 @@ extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
 #else
 
 static inline int dmi_check_system(const struct dmi_system_id *list) { return 0; }
-static inline const char * dmi_get_system_info(int field) { return NULL; }
-static inline const struct dmi_device * dmi_find_device(int type, const char *name,
+static inline const char *dmi_get_system_info(int field) { return NULL; }
+static inline struct dmi_device *dmi_find_device(int type, const char *name,
 	const struct dmi_device *from) { return NULL; }
 static inline void dmi_scan_machine(void) { return; }
 static inline void dmi_memdev_walk(void) { }
-- 
2.5.0

[PATCH 2/4] dmi: Add a DMI firmware node and handling

[minyard]

From: Corey Minyard <cminyard@mvista.com>

This is so that an IPMI platform device can be created from a
DMI firmware entry.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Jean Delvare <jdelvare@suse.de>
Cc: Andy Lutomirski <luto@kernel.org>
---
 drivers/firmware/dmi_scan.c | 34 ++++++++++++++++++++++++----------
 include/linux/dmi.h         | 24 ++++++++++++++++++++++++
 include/linux/fwnode.h      |  1 +
 3 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index da471b2..13d9bca 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -41,6 +41,16 @@ static struct dmi_memdev_info {
 } *dmi_memdev;
 static int dmi_memdev_nr;
 
+static void *dmi_zalloc(unsigned len)
+{
+	void *ret = dmi_alloc(len);
+
+	if (ret)
+		memset(ret, 0, len);
+
+	return ret;
+}
+
 static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
 {
 	const u8 *bp = ((u8 *) dm) + dm->length;
@@ -242,6 +252,12 @@ static void __init dmi_save_type(const struct dmi_header *dm, int slot,
 	dmi_ident[slot] = s;
 }
 
+static void __init dmi_devices_list_add(struct dmi_device *dev)
+{
+	dev->fwnode.type = FWNODE_DMI;
+	list_add(&dev->list, &dmi_devices);
+}
+
 static void __init dmi_save_one_device(int type, const char *name)
 {
 	struct dmi_device *dev;
@@ -250,15 +266,14 @@ static void __init dmi_save_one_device(int type, const char *name)
 	if (dmi_find_device(type, name, NULL))
 		return;
 
-	dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
+	dev = dmi_zalloc(sizeof(*dev) + strlen(name) + 1);
 	if (!dev)
 		return;
 
 	dev->type = type;
 	strcpy((char *)(dev + 1), name);
 	dev->name = (char *)(dev + 1);
-	dev->device_data = NULL;
-	list_add(&dev->list, &dmi_devices);
+	dmi_devices_list_add(dev);
 }
 
 static void __init dmi_save_devices(const struct dmi_header *dm)
@@ -287,15 +302,14 @@ static void __init dmi_save_oem_strings_devices(const struct dmi_header *dm)
 		if (devname == dmi_empty_string)
 			continue;
 
-		dev = dmi_alloc(sizeof(*dev));
+		dev = dmi_zalloc(sizeof(*dev));
 		if (!dev)
 			break;
 
 		dev->type = DMI_DEV_TYPE_OEM_STRING;
 		dev->name = devname;
-		dev->device_data = NULL;
 
-		list_add(&dev->list, &dmi_devices);
+		dmi_devices_list_add(dev);
 	}
 }
 
@@ -310,7 +324,7 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
 
 	memcpy(data, dm, dm->length);
 
-	dev = dmi_alloc(sizeof(*dev));
+	dev = dmi_zalloc(sizeof(*dev));
 	if (!dev)
 		return;
 
@@ -318,7 +332,7 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
 	dev->name = "IPMI controller";
 	dev->device_data = data;
 
-	list_add_tail(&dev->list, &dmi_devices);
+	dmi_devices_list_add(dev);
 }
 
 static void __init dmi_save_dev_pciaddr(int instance, int segment, int bus,
@@ -331,7 +345,7 @@ static void __init dmi_save_dev_pciaddr(int instance, int segment, int bus,
 	    segment == 0xFFFF && bus == 0xFF && devfn == 0xFF)
 		return;
 
-	dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
+	dev = dmi_zalloc(sizeof(*dev) + strlen(name) + 1);
 	if (!dev)
 		return;
 
@@ -345,7 +359,7 @@ static void __init dmi_save_dev_pciaddr(int instance, int segment, int bus,
 	dev->dev.name = (char *)&dev[1];
 	dev->dev.device_data = dev;
 
-	list_add(&dev->dev.list, &dmi_devices);
+	dmi_devices_list_add(&dev->dev);
 }
 
 static void __init dmi_save_extended_devices(const struct dmi_header *dm)
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index a930a4d..e130c38 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -4,6 +4,7 @@
 #include <linux/list.h>
 #include <linux/kobject.h>
 #include <linux/mod_devicetable.h>
+#include <linux/fwnode.h>
 
 /* enum dmi_field is in mod_devicetable.h */
 
@@ -80,6 +81,7 @@ struct dmi_header {
 
 struct dmi_device {
 	struct list_head list;
+	struct fwnode_handle fwnode;
 	int type;
 	const char *name;
 	void *device_data;	/* Type specific data */
@@ -113,6 +115,18 @@ extern int dmi_walk(void (*decode)(const struct dmi_header *, void *),
 extern bool dmi_match(enum dmi_field f, const char *str);
 extern void dmi_memdev_name(u16 handle, const char **bank, const char **device);
 
+static inline bool is_dmi_fwnode(struct fwnode_handle *fwnode)
+{
+	return fwnode && fwnode->type == FWNODE_DMI;
+}
+
+static inline struct dmi_device *to_dmi_device(struct fwnode_handle *fwnode)
+{
+	if (is_dmi_fwnode(fwnode))
+		return container_of(fwnode, struct dmi_device, fwnode);
+	return NULL;
+}
+
 #else
 
 static inline int dmi_check_system(const struct dmi_system_id *list) { return 0; }
@@ -144,6 +158,16 @@ static inline void dmi_memdev_name(u16 handle, const char **bank,
 static inline const struct dmi_system_id *
 	dmi_first_match(const struct dmi_system_id *list) { return NULL; }
 
+static inline bool is_dmi_fwnode(struct fwnode_handle *fwnode)
+{
+	return false
+}
+
+static inline struct dmi_fwnode *to_dmi_device(struct fwnode_handle *fwnode)
+{
+	return NULL;
+}
+
 #endif
 
 #endif	/* __DMI_H__ */
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 8516717..8690de2 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -19,6 +19,7 @@ enum fwnode_type {
 	FWNODE_ACPI_DATA,
 	FWNODE_PDATA,
 	FWNODE_IRQCHIP,
+	FWNODE_DMI,
 };
 
 struct fwnode_handle {
-- 
2.5.0

[PATCH 3/4] dmi: Move IPMI DMI scanning to the DMI code

[minyard]

From: Corey Minyard <cminyard@mvista.com>

It makes more sense to be there, and it's cleaner with the
upcoming conversion of IPMI DMI to a platform device.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Jean Delvare <jdelvare@suse.de>
Cc: Andy Lutomirski <luto@kernel.org>
---
 drivers/char/ipmi/ipmi_si_intf.c | 119 +++++++--------------------------------
 drivers/char/ipmi/ipmi_ssif.c    |  40 +++++--------
 drivers/firmware/dmi_scan.c      | 108 +++++++++++++++++++++++++++++++++--
 include/linux/dmi.h              |  26 +++++++++
 4 files changed, 162 insertions(+), 131 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 9fda22e..22292e1 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -2272,98 +2272,36 @@ static void spmi_find_bmc(void)
 #endif
 
 #ifdef CONFIG_DMI
-struct dmi_ipmi_data {
-	u8   		type;
-	u8   		addr_space;
-	unsigned long	base_addr;
-	u8   		irq;
-	u8              offset;
-	u8              slave_addr;
-};
-
-static int decode_dmi(const struct dmi_header *dm,
-				struct dmi_ipmi_data *dmi)
+static void try_init_dmi(struct dmi_device *dmi_dev)
 {
-	const u8	*data = (const u8 *)dm;
-	unsigned long  	base_addr;
-	u8		reg_spacing;
-	u8              len = dm->length;
+	struct dmi_dev_ipmi *ipmi_data = to_dmi_dev_ipmi(dmi_dev);
+	struct smi_info *info;
 
-	dmi->type = data[4];
+	if (!ipmi_data)
+		return;
 
-	memcpy(&base_addr, data+8, sizeof(unsigned long));
-	if (len >= 0x11) {
-		if (base_addr & 1) {
-			/* I/O */
-			base_addr &= 0xFFFE;
-			dmi->addr_space = IPMI_IO_ADDR_SPACE;
-		} else
-			/* Memory */
-			dmi->addr_space = IPMI_MEM_ADDR_SPACE;
-
-		/* If bit 4 of byte 0x10 is set, then the lsb for the address
-		   is odd. */
-		dmi->base_addr = base_addr | ((data[0x10] & 0x10) >> 4);
-
-		dmi->irq = data[0x11];
-
-		/* The top two bits of byte 0x10 hold the register spacing. */
-		reg_spacing = (data[0x10] & 0xC0) >> 6;
-		switch (reg_spacing) {
-		case 0x00: /* Byte boundaries */
-		    dmi->offset = 1;
-		    break;
-		case 0x01: /* 32-bit boundaries */
-		    dmi->offset = 4;
-		    break;
-		case 0x02: /* 16-byte boundaries */
-		    dmi->offset = 16;
-		    break;
-		default:
-		    /* Some other interface, just ignore it. */
-		    return -EIO;
-		}
-	} else {
-		/* Old DMI spec. */
-		/*
-		 * Note that technically, the lower bit of the base
-		 * address should be 1 if the address is I/O and 0 if
-		 * the address is in memory.  So many systems get that
-		 * wrong (and all that I have seen are I/O) so we just
-		 * ignore that bit and assume I/O.  Systems that use
-		 * memory should use the newer spec, anyway.
-		 */
-		dmi->base_addr = base_addr & 0xfffe;
-		dmi->addr_space = IPMI_IO_ADDR_SPACE;
-		dmi->offset = 1;
+	if (!ipmi_data->good_data) {
+		pr_err(PFX "DMI data for this device was invalid.\n");
+		return;
 	}
 
-	dmi->slave_addr = data[6];
-
-	return 0;
-}
-
-static void try_init_dmi(struct dmi_ipmi_data *ipmi_data)
-{
-	struct smi_info *info;
-
 	info = smi_info_alloc();
 	if (!info) {
-		printk(KERN_ERR PFX "Could not allocate SI data\n");
+		pr_err(PFX "Could not allocate SI data\n");
 		return;
 	}
 
 	info->addr_source = SI_SMBIOS;
-	printk(KERN_INFO PFX "probing via SMBIOS\n");
+	pr_info(PFX "probing via SMBIOS\n");
 
 	switch (ipmi_data->type) {
-	case 0x01: /* KCS */
+	case IPMI_DMI_TYPE_KCS:
 		info->si_type = SI_KCS;
 		break;
-	case 0x02: /* SMIC */
+	case IPMI_DMI_TYPE_SMIC:
 		info->si_type = SI_SMIC;
 		break;
-	case 0x03: /* BT */
+	case IPMI_DMI_TYPE_BT:
 		info->si_type = SI_BT;
 		break;
 	default:
@@ -2371,22 +2309,12 @@ static void try_init_dmi(struct dmi_ipmi_data *ipmi_data)
 		return;
 	}
 
-	switch (ipmi_data->addr_space) {
-	case IPMI_MEM_ADDR_SPACE:
-		info->io_setup = mem_setup;
-		info->io.addr_type = IPMI_MEM_ADDR_SPACE;
-		break;
-
-	case IPMI_IO_ADDR_SPACE:
+	if (ipmi_data->is_io_space) {
 		info->io_setup = port_setup;
 		info->io.addr_type = IPMI_IO_ADDR_SPACE;
-		break;
-
-	default:
-		kfree(info);
-		printk(KERN_WARNING PFX "Unknown SMBIOS I/O Address type: %d\n",
-		       ipmi_data->addr_space);
-		return;
+	} else {
+		info->io_setup = mem_setup;
+		info->io.addr_type = IPMI_MEM_ADDR_SPACE;
 	}
 	info->io.addr_data = ipmi_data->base_addr;
 
@@ -2413,17 +2341,10 @@ static void try_init_dmi(struct dmi_ipmi_data *ipmi_data)
 
 static void dmi_find_bmc(void)
 {
-	const struct dmi_device *dev = NULL;
-	struct dmi_ipmi_data data;
-	int                  rv;
+	struct dmi_device *dev = NULL;
 
-	while ((dev = dmi_find_device(DMI_DEV_TYPE_IPMI, NULL, dev))) {
-		memset(&data, 0, sizeof(data));
-		rv = decode_dmi((const struct dmi_header *) dev->device_data,
-				&data);
-		if (!rv)
-			try_init_dmi(&data);
-	}
+	while ((dev = dmi_find_device(DMI_DEV_TYPE_IPMI, NULL, dev)))
+		try_init_dmi(dev);
 }
 #endif /* CONFIG_DMI */
 
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 5f1c3d0..7be0109 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -1906,42 +1906,28 @@ static void spmi_find_bmc(void) { }
 #endif
 
 #ifdef CONFIG_DMI
-static int decode_dmi(const struct dmi_device *dmi_dev)
+static int decode_dmi(struct dmi_device *dmi_dev)
 {
-	struct dmi_header *dm = dmi_dev->device_data;
-	u8             *data = (u8 *) dm;
-	u8             len = dm->length;
-	unsigned short myaddr;
-	int            slave_addr;
-
-	if (num_addrs >= MAX_SSIF_BMCS)
-		return -1;
+	struct dmi_dev_ipmi *ipmi_data = to_dmi_dev_ipmi(dmi_dev);
 
-	if (len < 9)
-		return -1;
-
-	if (data[0x04] != 4) /* Not SSIF */
-		return -1;
+	if (!ipmi_data)
+		return -ENODEV;
 
-	if ((data[8] >> 1) == 0) {
-		/*
-		 * Some broken systems put the I2C address in
-		 * the slave address field.  We try to
-		 * accommodate them here.
-		 */
-		myaddr = data[6] >> 1;
-		slave_addr = 0;
-	} else {
-		myaddr = data[8] >> 1;
-		slave_addr = data[6];
+	if (!ipmi_data->good_data) {
+		pr_err(PFX "DMI data for this device was invalid.\n");
+		return -EINVAL;
 	}
 
-	return new_ssif_client(myaddr, NULL, 0, 0, SI_SMBIOS);
+	if (ipmi_data->type != IPMI_DMI_TYPE_SSIF)
+		return -ENODEV;
+
+	return new_ssif_client(ipmi_data->base_addr, NULL, 0,
+			       ipmi_data->slave_addr, SI_SMBIOS);
 }
 
 static void dmi_iterator(void)
 {
-	const struct dmi_device *dev = NULL;
+	struct dmi_device *dev = NULL;
 
 	while ((dev = dmi_find_device(DMI_DEV_TYPE_IPMI, NULL, dev)))
 		decode_dmi(dev);
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 13d9bca..6e4859d 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -313,9 +313,105 @@ static void __init dmi_save_oem_strings_devices(const struct dmi_header *dm)
 	}
 }
 
+#define DMI_IPMI_MIN_LENGTH	0x10
+#define DMI_IPMI_VER2_LENGTH	0x12
+#define DMI_IPMI_TYPE		4
+#define DMI_IPMI_SLAVEADDR	6
+#define DMI_IPMI_ADDR		8
+#define DMI_IPMI_ACCESS		0x10
+#define DMI_IPMI_IRQ		0x11
+#define DMI_IPMI_IO_MASK	0xfffe
+
+static void __init dmi_decode_ipmi(struct dmi_dev_ipmi *dev,
+				   const struct dmi_header *dm)
+{
+	const u8	*data = (const u8 *) dm;
+	unsigned long	base_addr;
+	u8              len = dm->length;
+	bool            slave_addr_set;
+
+	if (len < DMI_IPMI_MIN_LENGTH)
+		return;
+
+	dev->type = data[DMI_IPMI_TYPE];
+
+	memcpy(&base_addr, data + DMI_IPMI_ADDR, sizeof(unsigned long));
+	if (len >= DMI_IPMI_VER2_LENGTH) {
+		if (dev->type == IPMI_DMI_TYPE_SSIF) {
+			if ((data[DMI_IPMI_ADDR] >> 1) == 0) {
+				/*
+				 * Some broken systems put the I2C address in
+				 * the slave address field.  We try to
+				 * accommodate them here.
+				 */
+				dev->base_addr = data[DMI_IPMI_SLAVEADDR] >> 1;
+				dev->slave_addr = 0;
+				slave_addr_set = true;
+			} else {
+				dev->base_addr = data[DMI_IPMI_ADDR] >> 1;
+			}
+		} else {
+			if (base_addr & 1) {
+				/* I/O */
+				base_addr &= DMI_IPMI_IO_MASK;
+				dev->is_io_space = 1;
+			} else {
+				/* Memory */
+				dev->is_io_space = 0;
+			}
+
+			/*
+			 * If bit 4 of byte 0x10 is set, then the lsb
+			 * for the address is odd.
+			 */
+			base_addr |= (data[DMI_IPMI_ACCESS] >> 4) & 1;
+
+			dev->base_addr = base_addr;
+			dev->irq = data[DMI_IPMI_IRQ];
+
+			/*
+			 * The top two bits of byte 0x10 hold the
+			 * register spacing.
+			 */
+			switch ((data[DMI_IPMI_ACCESS] >> 6) & 3) {
+			case 0: /* Byte boundaries */
+				dev->offset = 1;
+				break;
+			case 1: /* 32-bit boundaries */
+				dev->offset = 4;
+				break;
+			case 2: /* 16-byte boundaries */
+				dev->offset = 16;
+				break;
+			default:
+				/* Leave 0 for undefined. */
+				return;
+			}
+		}
+	} else {
+		/* Old DMI spec. */
+		/*
+		 * Note that technically, the lower bit of the base
+		 * address should be 1 if the address is I/O and 0 if
+		 * the address is in memory.  So many systems get that
+		 * wrong (and all that I have seen are I/O) so we just
+		 * ignore that bit and assume I/O.  Systems that use
+		 * memory should use the newer spec, anyway.
+		 */
+		dev->base_addr = base_addr & DMI_IPMI_IO_MASK;
+		dev->is_io_space = 1;
+		dev->offset = 1;
+	}
+
+	if (!slave_addr_set)
+		dev->slave_addr = data[DMI_IPMI_SLAVEADDR];
+
+	dev->good_data = 1;
+}
+
 static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
 {
-	struct dmi_device *dev;
+	struct dmi_dev_ipmi *dev;
 	void *data;
 
 	data = dmi_alloc(dm->length);
@@ -328,11 +424,13 @@ static void __init dmi_save_ipmi_device(const struct dmi_header *dm)
 	if (!dev)
 		return;
 
-	dev->type = DMI_DEV_TYPE_IPMI;
-	dev->name = "IPMI controller";
-	dev->device_data = data;
+	dev->dev.type = DMI_DEV_TYPE_IPMI;
+	dev->dev.name = "IPMI controller";
+	dev->dev.device_data = data;
 
-	dmi_devices_list_add(dev);
+	dmi_decode_ipmi(dev, dm);
+
+	dmi_devices_list_add(&dev->dev);
 }
 
 static void __init dmi_save_dev_pciaddr(int instance, int segment, int bus,
diff --git a/include/linux/dmi.h b/include/linux/dmi.h
index e130c38..ca37ba5 100644
--- a/include/linux/dmi.h
+++ b/include/linux/dmi.h
@@ -97,6 +97,24 @@ struct dmi_dev_onboard {
 	int devfn;
 };
 
+/* Device data for an IPMI entry. */
+struct dmi_dev_ipmi {
+	struct dmi_device dev;
+	u8		good_data;
+	u8		type;
+	u8		is_io_space;
+	u8		irq;
+	u8		offset;
+	u8		slave_addr;
+	unsigned long	base_addr;
+};
+
+/* dmi_ipmi_data type field */
+#define IPMI_DMI_TYPE_KCS	0x01
+#define IPMI_DMI_TYPE_SMIC	0x02
+#define IPMI_DMI_TYPE_BT	0x03
+#define IPMI_DMI_TYPE_SSIF	0x04
+
 extern struct kobject *dmi_kobj;
 extern int dmi_check_system(const struct dmi_system_id *list);
 const struct dmi_system_id *dmi_first_match(const struct dmi_system_id *list);
@@ -127,6 +145,14 @@ static inline struct dmi_device *to_dmi_device(struct fwnode_handle *fwnode)
 	return NULL;
 }
 
+static inline struct dmi_dev_ipmi *to_dmi_dev_ipmi(struct dmi_device *dev)
+{
+	if (!dev || dev->type != DMI_DEV_TYPE_IPMI)
+		return NULL;
+
+	return container_of(dev, struct dmi_dev_ipmi, dev);
+}
+
 #else
 
 static inline int dmi_check_system(const struct dmi_system_id *list) { return 0; }
-- 
2.5.0

[PATCH 4/4] dmi/ipmi: Add IPMI DMI devices as platform devices

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Have DMI create a platform device for every IPMI device it
finds.

This requires some modification of the IPMI driver to find the
IPMI devices as platform devices.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Jean Delvare <jdelvare@suse.de>
Cc: Andy Lutomirski <luto@kernel.org>
---
 drivers/char/ipmi/ipmi_si_intf.c | 42 +++++++++----------
 drivers/char/ipmi/ipmi_ssif.c    | 87 ++++++++++++++++++++++++++++++++--------
 drivers/firmware/dmi_scan.c      | 55 ++++++++++++++++++++++++-
 3 files changed, 142 insertions(+), 42 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 22292e1..4984c86 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -2272,23 +2272,19 @@ static void spmi_find_bmc(void)
 #endif
 
 #ifdef CONFIG_DMI
-static void try_init_dmi(struct dmi_device *dmi_dev)
+static int dmi_ipmi_probe(struct platform_device *pdev)
 {
+	struct dmi_device *dmi_dev = to_dmi_device(pdev->dev.fwnode);
 	struct dmi_dev_ipmi *ipmi_data = to_dmi_dev_ipmi(dmi_dev);
 	struct smi_info *info;
 
-	if (!ipmi_data)
-		return;
-
-	if (!ipmi_data->good_data) {
-		pr_err(PFX "DMI data for this device was invalid.\n");
-		return;
-	}
+	if (!si_trydmi || !ipmi_data)
+		return -ENODEV;
 
 	info = smi_info_alloc();
 	if (!info) {
 		pr_err(PFX "Could not allocate SI data\n");
-		return;
+		return -ENOMEM;
 	}
 
 	info->addr_source = SI_SMBIOS;
@@ -2306,7 +2302,7 @@ static void try_init_dmi(struct dmi_device *dmi_dev)
 		break;
 	default:
 		kfree(info);
-		return;
+		return -EINVAL;
 	}
 
 	if (ipmi_data->is_io_space) {
@@ -2330,6 +2326,8 @@ static void try_init_dmi(struct dmi_device *dmi_dev)
 	if (info->irq)
 		info->irq_setup = std_irq_setup;
 
+	info->dev = &pdev->dev;
+
 	pr_info("ipmi_si: SMBIOS: %s %#lx regsize %d spacing %d irq %d\n",
 		 (info->io.addr_type == IPMI_IO_ADDR_SPACE) ? "io" : "mem",
 		 info->io.addr_data, info->io.regsize, info->io.regspacing,
@@ -2337,14 +2335,13 @@ static void try_init_dmi(struct dmi_device *dmi_dev)
 
 	if (add_smi(info))
 		kfree(info);
-}
 
-static void dmi_find_bmc(void)
+	return 0;
+}
+#else
+static int dmi_ipmi_probe(struct platform_device *pdev)
 {
-	struct dmi_device *dev = NULL;
-
-	while ((dev = dmi_find_device(DMI_DEV_TYPE_IPMI, NULL, dev)))
-		try_init_dmi(dev);
+	return -ENODEV;
 }
 #endif /* CONFIG_DMI */
 
@@ -2729,7 +2726,10 @@ static int ipmi_probe(struct platform_device *dev)
 	if (of_ipmi_probe(dev) == 0)
 		return 0;
 
-	return acpi_ipmi_probe(dev);
+	if (acpi_ipmi_probe(dev) == 0)
+		return 0;
+
+	return dmi_ipmi_probe(dev);
 }
 
 static int ipmi_remove(struct platform_device *dev)
@@ -3446,7 +3446,7 @@ static int add_smi(struct smi_info *new_smi)
 	       si_to_str[new_smi->si_type]);
 	mutex_lock(&smi_infos_lock);
 	if (!is_new_interface(new_smi)) {
-		printk(KERN_CONT " duplicate interface\n");
+		printk(KERN_CONT ": duplicate interface\n");
 		rv = -EBUSY;
 		goto out_err;
 	}
@@ -3737,11 +3737,6 @@ static int init_ipmi_si(void)
 	}
 #endif
 
-#ifdef CONFIG_DMI
-	if (si_trydmi)
-		dmi_find_bmc();
-#endif
-
 #ifdef CONFIG_ACPI
 	if (si_tryacpi)
 		spmi_find_bmc();
@@ -3902,6 +3897,7 @@ static void cleanup_ipmi_si(void)
 }
 module_exit(cleanup_ipmi_si);
 
+MODULE_ALIAS("platform:dmi-ipmi-si");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Corey Minyard <minyard@mvista.com>");
 MODULE_DESCRIPTION("Interface to the IPMI driver for the KCS, SMIC, and BT"
diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index 7be0109..4579e59 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -181,6 +181,8 @@ struct ssif_addr_info {
 	int slave_addr;
 	enum ipmi_addr_src addr_src;
 	union ipmi_smi_info_union addr_info;
+	struct device *dev;
+	struct i2c_client *client;
 
 	struct mutex clients_mutex;
 	struct list_head clients;
@@ -1444,6 +1446,7 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
 			ssif_info->addr_source = addr_info->addr_src;
 			ssif_info->ssif_debug = addr_info->debug;
 			ssif_info->addr_info = addr_info->addr_info;
+			addr_info->client = client;
 			slave_addr = addr_info->slave_addr;
 		}
 	}
@@ -1680,8 +1683,19 @@ static int ssif_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	}
 
  out:
-	if (rv)
+	if (rv) {
+		/*
+		 * Note that if addr_info->client is assigned, we
+		 * leave it.  The i2c client hangs around even if we
+		 * return a failure here, and the failure here is not
+		 * propagated back to the i2c code.  This seems to be
+		 * design intent, strange as it may be.  But if we
+		 * don't leave it, ssif_platform_remove will not remove
+		 * the client like it should.
+		 */
+		dev_err(&client->dev, "Unable to start IPMI SSIF: %d\n", rv);
 		kfree(ssif_info);
+	}
 	kfree(resp);
 	return rv;
 
@@ -1706,7 +1720,8 @@ static int ssif_adapter_handler(struct device *adev, void *opaque)
 
 static int new_ssif_client(int addr, char *adapter_name,
 			   int debug, int slave_addr,
-			   enum ipmi_addr_src addr_src)
+			   enum ipmi_addr_src addr_src,
+			   struct device *dev)
 {
 	struct ssif_addr_info *addr_info;
 	int rv = 0;
@@ -1736,9 +1751,14 @@ static int new_ssif_client(int addr, char *adapter_name,
 		sizeof(addr_info->binfo.type));
 	addr_info->binfo.addr = addr;
 	addr_info->binfo.platform_data = addr_info;
+	if (dev)
+		addr_info->binfo.fwnode = dev->fwnode;
 	addr_info->debug = debug;
 	addr_info->slave_addr = slave_addr;
 	addr_info->addr_src = addr_src;
+	addr_info->dev = dev;
+
+	dev_set_drvdata(dev, addr_info);
 
 	list_add_tail(&addr_info->link, &ssif_infos);
 
@@ -1877,7 +1897,7 @@ static int try_init_spmi(struct SPMITable *spmi)
 
 	myaddr = spmi->addr.address >> 1;
 
-	return new_ssif_client(myaddr, NULL, 0, 0, SI_SPMI);
+	return new_ssif_client(myaddr, NULL, 0, 0, SI_SPMI, NULL);
 }
 
 static void spmi_find_bmc(void)
@@ -1906,11 +1926,12 @@ static void spmi_find_bmc(void) { }
 #endif
 
 #ifdef CONFIG_DMI
-static int decode_dmi(struct dmi_device *dmi_dev)
+static int dmi_ipmi_probe(struct platform_device *pdev)
 {
+	struct dmi_device *dmi_dev = to_dmi_device(pdev->dev.fwnode);
 	struct dmi_dev_ipmi *ipmi_data = to_dmi_dev_ipmi(dmi_dev);
 
-	if (!ipmi_data)
+	if (!ssif_trydmi || !ipmi_data)
 		return -ENODEV;
 
 	if (!ipmi_data->good_data) {
@@ -1922,18 +1943,13 @@ static int decode_dmi(struct dmi_device *dmi_dev)
 		return -ENODEV;
 
 	return new_ssif_client(ipmi_data->base_addr, NULL, 0,
-			       ipmi_data->slave_addr, SI_SMBIOS);
+			       ipmi_data->slave_addr, SI_SMBIOS, &pdev->dev);
 }
-
-static void dmi_iterator(void)
+#else
+static int dmi_ipmi_probe(struct platform_device *pdev)
 {
-	struct dmi_device *dev = NULL;
-
-	while ((dev = dmi_find_device(DMI_DEV_TYPE_IPMI, NULL, dev)))
-		decode_dmi(dev);
+	return -ENODEV;
 }
-#else
-static void dmi_iterator(void) { }
 #endif
 
 static const struct i2c_device_id ssif_id[] = {
@@ -1954,6 +1970,36 @@ static struct i2c_driver ssif_i2c_driver = {
 	.detect		= ssif_detect
 };
 
+static int ssif_platform_probe(struct platform_device *dev)
+{
+	return dmi_ipmi_probe(dev);
+}
+
+static int ssif_platform_remove(struct platform_device *dev)
+{
+	struct ssif_addr_info *addr_info = dev_get_drvdata(&dev->dev);
+
+	if (!addr_info)
+		return 0;
+
+	mutex_lock(&ssif_infos_mutex);
+	if (addr_info->client)
+		i2c_unregister_device(addr_info->client);
+
+	list_del(&addr_info->link);
+	kfree(addr_info);
+	mutex_unlock(&ssif_infos_mutex);
+	return 0;
+}
+
+static struct platform_driver ipmi_driver = {
+	.driver = {
+		.name = DEVICE_NAME,
+	},
+	.probe		= ssif_platform_probe,
+	.remove		= ssif_platform_remove,
+};
+
 static int init_ipmi_ssif(void)
 {
 	int i;
@@ -1968,7 +2014,7 @@ static int init_ipmi_ssif(void)
 	for (i = 0; i < num_addrs; i++) {
 		rv = new_ssif_client(addr[i], adapter_name[i],
 				     dbg[i], slave_addrs[i],
-				     SI_HARDCODED);
+				     SI_HARDCODED, NULL);
 		if (rv)
 			pr_err(PFX
 			       "Couldn't add hardcoded device at addr 0x%x\n",
@@ -1978,8 +2024,7 @@ static int init_ipmi_ssif(void)
 	if (ssif_tryacpi)
 		ssif_i2c_driver.driver.acpi_match_table	=
 			ACPI_PTR(ssif_acpi_match);
-	if (ssif_trydmi)
-		dmi_iterator();
+
 	if (ssif_tryacpi)
 		spmi_find_bmc();
 
@@ -1989,6 +2034,11 @@ static int init_ipmi_ssif(void)
 	if (!rv)
 		initialized = true;
 
+	/* Wait until here so ACPI devices will start first. */
+	rv = platform_driver_register(&ipmi_driver);
+	if (rv)
+		pr_err(PFX "Unable to register driver: %d\n", rv);
+
 	return rv;
 }
 module_init(init_ipmi_ssif);
@@ -2000,12 +2050,15 @@ static void cleanup_ipmi_ssif(void)
 
 	initialized = false;
 
+	platform_driver_unregister(&ipmi_driver);
+
 	i2c_del_driver(&ssif_i2c_driver);
 
 	free_ssif_clients();
 }
 module_exit(cleanup_ipmi_ssif);
 
+MODULE_ALIAS("platform:dmi-ipmi-ssif");
 MODULE_AUTHOR("Todd C Davis <todd.c.davis@intel.com>, Corey Minyard <minyard@acm.org>");
 MODULE_DESCRIPTION("IPMI driver for management controllers on a SMBus");
 MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
index 6e4859d..7fda3ce 100644
--- a/drivers/firmware/dmi_scan.c
+++ b/drivers/firmware/dmi_scan.c
@@ -7,6 +7,7 @@
 #include <linux/efi.h>
 #include <linux/bootmem.h>
 #include <linux/random.h>
+#include <linux/platform_device.h>
 #include <asm/dmi.h>
 #include <asm/unaligned.h>
 
@@ -793,6 +794,52 @@ static ssize_t raw_table_read(struct file *file, struct kobject *kobj,
 	return count;
 }
 
+static void __init dmi_add_platform_ipmi(struct dmi_device *dev, int *nr)
+{
+	struct platform_device *pdev;
+	struct dmi_dev_ipmi *ipmi_dev = to_dmi_dev_ipmi(dev);
+	int rv;
+
+	if (!ipmi_dev->good_data) {
+		pr_err("dmi: Invalid IPMI data, not creating platform device");
+		return;
+	}
+
+	if (ipmi_dev->type == IPMI_DMI_TYPE_SSIF)
+		pdev = platform_device_alloc("dmi-ipmi-ssif", *nr);
+	else
+		pdev = platform_device_alloc("dmi-ipmi-si", *nr);
+	if (!pdev) {
+		pr_err("dmi: Error allocation IPMI platform device");
+		return;
+	}
+	if (ipmi_dev->type == IPMI_DMI_TYPE_SSIF)
+		pdev->driver_override = "ipmi_ssif";
+	else
+		pdev->driver_override = "ipmi_si";
+
+	pdev->dev.fwnode = &dev->fwnode;
+	rv = platform_device_add(pdev);
+	if (rv) {
+		dev_err(&pdev->dev, "dmi: Unable to add device: %d\n", rv);
+		platform_device_del(pdev);
+		return;
+	}
+
+	(*nr)++;
+}
+
+static void __init dmi_add_platform_devices(void)
+{
+	struct dmi_device *dev;
+	int nr_ipmi = 0;
+
+	list_for_each_entry(dev, &dmi_devices, list) {
+		if (dev->type == DMI_DEV_TYPE_IPMI)
+			dmi_add_platform_ipmi(dev, &nr_ipmi);
+	}
+}
+
 static BIN_ATTR(smbios_entry_point, S_IRUSR, raw_table_read, NULL, 0);
 static BIN_ATTR(DMI, S_IRUSR, raw_table_read, NULL, 0);
 
@@ -833,9 +880,13 @@ static int __init dmi_init(void)
 	bin_attr_DMI.size = dmi_len;
 	bin_attr_DMI.private = dmi_table;
 	ret = sysfs_create_bin_file(tables_kobj, &bin_attr_DMI);
-	if (!ret)
-		return 0;
+	if (ret)
+		goto out_remove_bin_file;
+
+	dmi_add_platform_devices();
+	return 0;
 
+ out_remove_bin_file:
 	sysfs_remove_bin_file(tables_kobj,
 			      &bin_attr_smbios_entry_point);
  err_unmap:
-- 
2.5.0

Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling

[kbuild test robot]

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

Hi Corey,

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on v4.5-rc1 next-20160129]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/minyard-acm-org/dmi-Rework-to-get-IPMI-autoloading-from-DMI-tables/20160130-074830
config: i386-tinyconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from arch/x86/kernel/setup.c:46:0:
   include/linux/dmi.h: In function 'is_dmi_fwnode':
>> include/linux/dmi.h:164:1: error: expected ';' before '}' token
    }
    ^

vim +164 include/linux/dmi.h

   158	static inline const struct dmi_system_id *
   159		dmi_first_match(const struct dmi_system_id *list) { return NULL; }
   160	
   161	static inline bool is_dmi_fwnode(struct fwnode_handle *fwnode)
   162	{
   163		return false
 > 164	}
   165	
   166	static inline struct dmi_fwnode *to_dmi_device(struct fwnode_handle *fwnode)
   167	{

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6216 bytes --]

Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling

[Corey Minyard]

Dang, I've been doing too much Python. I've fixed this, but I guess I'll 
wait for more comments.

-corey

On 01/29/2016 05:59 PM, kbuild test robot wrote:
> Hi Corey,
>
> [auto build test ERROR on char-misc/char-misc-testing]
> [also build test ERROR on v4.5-rc1 next-20160129]
> [if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
>
> url:    https://github.com/0day-ci/linux/commits/minyard-acm-org/dmi-Rework-to-get-IPMI-autoloading-from-DMI-tables/20160130-074830
> config: i386-tinyconfig (attached as .config)
> reproduce:
>          # save the attached .config to linux build tree
>          make ARCH=i386
>
> All errors (new ones prefixed by >>):
>
>     In file included from arch/x86/kernel/setup.c:46:0:
>     include/linux/dmi.h: In function 'is_dmi_fwnode':
>>> include/linux/dmi.h:164:1: error: expected ';' before '}' token
>      }
>      ^
>
> vim +164 include/linux/dmi.h
>
>     158	static inline const struct dmi_system_id *
>     159		dmi_first_match(const struct dmi_system_id *list) { return NULL; }
>     160	
>     161	static inline bool is_dmi_fwnode(struct fwnode_handle *fwnode)
>     162	{
>     163		return false
>   > 164	}
>     165	
>     166	static inline struct dmi_fwnode *to_dmi_device(struct fwnode_handle *fwnode)
>     167	{
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling

[kbuild test robot]

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

Hi Corey,

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on v4.5-rc1 next-20160129]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/minyard-acm-org/dmi-Rework-to-get-IPMI-autoloading-from-DMI-tables/20160130-074830
config: i386-randconfig-s0-201604 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

>> WARNING: vmlinux.o(.text.unlikely+0xe634): Section mismatch in reference from the function dmi_zalloc() to the function .init.text:extend_brk()
   The function dmi_zalloc() references
   the function __init extend_brk().
   This is often because dmi_zalloc lacks a __init
   annotation or the annotation of extend_brk is wrong.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24189 bytes --]

Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling

[Andy Lutomirski]

On Fri, Jan 29, 2016 at 2:43 PM,  <minyard@acm.org> wrote:
> From: Corey Minyard <cminyard@mvista.com>
>
> This is so that an IPMI platform device can be created from a
> DMI firmware entry.

This doesn't apply for me.  What's it based on?

--Andy

Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling

[Corey Minyard]

On 01/31/2016 04:46 PM, Andy Lutomirski wrote:
> On Fri, Jan 29, 2016 at 2:43 PM,  <minyard@acm.org> wrote:
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> This is so that an IPMI platform device can be created from a
>> DMI firmware entry.
> This doesn't apply for me.  What's it based on?
>
> --Andy

It was off of Linus' master a couple of days ago.  I just rebased to the 
latest and it applied cleanly.

I can resend, if you like.

Thanks,

-corey

Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling

[Jean Delvare]

Hi Corey,

I won't comment on the IPMI side of this as this isn't my area. However
I have a comment on the DMI part:

Le Friday 29 January 2016 à 16:43 -0600, minyard@acm.org a écrit :
> From: Corey Minyard <cminyard@mvista.com>
> 
> This is so that an IPMI platform device can be created from a
> DMI firmware entry.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Jean Delvare <jdelvare@suse.de>
> Cc: Andy Lutomirski <luto@kernel.org>
> ---
>  drivers/firmware/dmi_scan.c | 34 ++++++++++++++++++++++++----------
>  include/linux/dmi.h         | 24 ++++++++++++++++++++++++
>  include/linux/fwnode.h      |  1 +
>  3 files changed, 49 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
> index da471b2..13d9bca 100644
> --- a/drivers/firmware/dmi_scan.c
> +++ b/drivers/firmware/dmi_scan.c
> @@ -41,6 +41,16 @@ static struct dmi_memdev_info {
>  } *dmi_memdev;
>  static int dmi_memdev_nr;
>  
> +static void *dmi_zalloc(unsigned len)
> +{
> +	void *ret = dmi_alloc(len);
> +
> +	if (ret)
> +		memset(ret, 0, len);
> +
> +	return ret;
> +}
> +
>  static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
>  {
>  	const u8 *bp = ((u8 *) dm) + dm->length;
> @@ -242,6 +252,12 @@ static void __init dmi_save_type(const struct dmi_header *dm, int slot,
> (...)
> @@ -250,15 +266,14 @@ static void __init dmi_save_one_device(int type, const char *name)
>  	if (dmi_find_device(type, name, NULL))
>  		return;
>  
> -	dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
> +	dev = dmi_zalloc(sizeof(*dev) + strlen(name) + 1);
>  	if (!dev)
>  		return;
>  
>  	dev->type = type;
>  	strcpy((char *)(dev + 1), name);
>  	dev->name = (char *)(dev + 1);
> -	dev->device_data = NULL;

This change seems rather unrelated, and I'm not sure what purpose it
serves. On ia64 and arm64 it is clearly redundant as dmi_alloc calls
kzalloc directly. On x86_64, extend_brk is called instead (don't ask me
why, I have no clue) but looking at the code I see that it does
memset(ret, 0, size) as well so memory is also zeroed there. Which makes
dmi_alloc the same as dmi_zalloc on all 3 architectures.

So please revert this change. This will make your patch easier to
review, too.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling

[Corey Minyard]

On 02/01/2016 03:25 AM, Jean Delvare wrote:
> Hi Corey,
>
> I won't comment on the IPMI side of this as this isn't my area. However
> I have a comment on the DMI part:
>
> Le Friday 29 January 2016 à 16:43 -0600, minyard@acm.org a écrit :
>> From: Corey Minyard <cminyard@mvista.com>
>>
>> This is so that an IPMI platform device can be created from a
>> DMI firmware entry.
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Cc: Jean Delvare <jdelvare@suse.de>
>> Cc: Andy Lutomirski <luto@kernel.org>
>> ---
>>   drivers/firmware/dmi_scan.c | 34 ++++++++++++++++++++++++----------
>>   include/linux/dmi.h         | 24 ++++++++++++++++++++++++
>>   include/linux/fwnode.h      |  1 +
>>   3 files changed, 49 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>> index da471b2..13d9bca 100644
>> --- a/drivers/firmware/dmi_scan.c
>> +++ b/drivers/firmware/dmi_scan.c
>> @@ -41,6 +41,16 @@ static struct dmi_memdev_info {
>>   } *dmi_memdev;
>>   static int dmi_memdev_nr;
>>   
>> +static void *dmi_zalloc(unsigned len)
>> +{
>> +	void *ret = dmi_alloc(len);
>> +
>> +	if (ret)
>> +		memset(ret, 0, len);
>> +
>> +	return ret;
>> +}
>> +
>>   static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
>>   {
>>   	const u8 *bp = ((u8 *) dm) + dm->length;
>> @@ -242,6 +252,12 @@ static void __init dmi_save_type(const struct dmi_header *dm, int slot,
>> (...)
>> @@ -250,15 +266,14 @@ static void __init dmi_save_one_device(int type, const char *name)
>>   	if (dmi_find_device(type, name, NULL))
>>   		return;
>>   
>> -	dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
>> +	dev = dmi_zalloc(sizeof(*dev) + strlen(name) + 1);
>>   	if (!dev)
>>   		return;
>>   
>>   	dev->type = type;
>>   	strcpy((char *)(dev + 1), name);
>>   	dev->name = (char *)(dev + 1);
>> -	dev->device_data = NULL;
> This change seems rather unrelated, and I'm not sure what purpose it
> serves. On ia64 and arm64 it is clearly redundant as dmi_alloc calls
> kzalloc directly. On x86_64, extend_brk is called instead (don't ask me
> why, I have no clue) but looking at the code I see that it does
> memset(ret, 0, size) as well so memory is also zeroed there. Which makes
> dmi_alloc the same as dmi_zalloc on all 3 architectures.
>
> So please revert this change. This will make your patch easier to
> review, too.
>
Ok.  I had assumed extend_break wasn't zeroing since there were all the 
NULL assignments,
I should have looked.

I was thinking about this, and the fwnode could just be added to the 
IPMI device.  I'm not
sure if you would prefer that over adding it to dmi_device.  The fwnode 
is in acpi_device,
and I was modelling these changes after that, but maybe that's not 
required here.

Thanks,

-corey

Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling

[Andy Lutomirski]

On Feb 2, 2016 5:37 AM, "Corey Minyard" <minyard@acm.org> wrote:
>
> On 02/01/2016 03:25 AM, Jean Delvare wrote:
>>
>> Hi Corey,
>>
>> I won't comment on the IPMI side of this as this isn't my area. However
>> I have a comment on the DMI part:
>>
>> Le Friday 29 January 2016 à 16:43 -0600, minyard@acm.org a écrit :
>>>
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> This is so that an IPMI platform device can be created from a
>>> DMI firmware entry.
>>>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> Cc: Jean Delvare <jdelvare@suse.de>
>>> Cc: Andy Lutomirski <luto@kernel.org>
>>> ---
>>>   drivers/firmware/dmi_scan.c | 34 ++++++++++++++++++++++++----------
>>>   include/linux/dmi.h         | 24 ++++++++++++++++++++++++
>>>   include/linux/fwnode.h      |  1 +
>>>   3 files changed, 49 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>>> index da471b2..13d9bca 100644
>>> --- a/drivers/firmware/dmi_scan.c
>>> +++ b/drivers/firmware/dmi_scan.c
>>> @@ -41,6 +41,16 @@ static struct dmi_memdev_info {
>>>   } *dmi_memdev;
>>>   static int dmi_memdev_nr;
>>>   +static void *dmi_zalloc(unsigned len)
>>> +{
>>> +       void *ret = dmi_alloc(len);
>>> +
>>> +       if (ret)
>>> +               memset(ret, 0, len);
>>> +
>>> +       return ret;
>>> +}
>>> +
>>>   static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
>>>   {
>>>         const u8 *bp = ((u8 *) dm) + dm->length;
>>> @@ -242,6 +252,12 @@ static void __init dmi_save_type(const struct dmi_header *dm, int slot,
>>> (...)
>>> @@ -250,15 +266,14 @@ static void __init dmi_save_one_device(int type, const char *name)
>>>         if (dmi_find_device(type, name, NULL))
>>>                 return;
>>>   -     dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
>>> +       dev = dmi_zalloc(sizeof(*dev) + strlen(name) + 1);
>>>         if (!dev)
>>>                 return;
>>>         dev->type = type;
>>>         strcpy((char *)(dev + 1), name);
>>>         dev->name = (char *)(dev + 1);
>>> -       dev->device_data = NULL;
>>
>> This change seems rather unrelated, and I'm not sure what purpose it
>> serves. On ia64 and arm64 it is clearly redundant as dmi_alloc calls
>> kzalloc directly. On x86_64, extend_brk is called instead (don't ask me
>> why, I have no clue) but looking at the code I see that it does
>> memset(ret, 0, size) as well so memory is also zeroed there. Which makes
>> dmi_alloc the same as dmi_zalloc on all 3 architectures.
>>
>> So please revert this change. This will make your patch easier to
>> review, too.
>>
> Ok.  I had assumed extend_break wasn't zeroing since there were all the NULL assignments,
> I should have looked.
>
> I was thinking about this, and the fwnode could just be added to the IPMI device.  I'm not
> sure if you would prefer that over adding it to dmi_device.  The fwnode is in acpi_device,
> and I was modelling these changes after that, but maybe that's not required here.

I think dmi_device is right, especially if your patches result in a
firmware_node sysfs link being created.  That way the link will point
to the right place.


--Andy

Re: [PATCH 2/4] dmi: Add a DMI firmware node and handling

[Corey Minyard]

On 02/02/2016 12:25 PM, Andy Lutomirski wrote:
> On Feb 2, 2016 5:37 AM, "Corey Minyard" <minyard@acm.org> wrote:
>> On 02/01/2016 03:25 AM, Jean Delvare wrote:
>>> Hi Corey,
>>>
>>> I won't comment on the IPMI side of this as this isn't my area. However
>>> I have a comment on the DMI part:
>>>
>>> Le Friday 29 January 2016 à 16:43 -0600, minyard@acm.org a écrit :
>>>> From: Corey Minyard <cminyard@mvista.com>
>>>>
>>>> This is so that an IPMI platform device can be created from a
>>>> DMI firmware entry.
>>>>
>>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>>> Cc: Jean Delvare <jdelvare@suse.de>
>>>> Cc: Andy Lutomirski <luto@kernel.org>
>>>> ---
>>>>    drivers/firmware/dmi_scan.c | 34 ++++++++++++++++++++++++----------
>>>>    include/linux/dmi.h         | 24 ++++++++++++++++++++++++
>>>>    include/linux/fwnode.h      |  1 +
>>>>    3 files changed, 49 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/dmi_scan.c b/drivers/firmware/dmi_scan.c
>>>> index da471b2..13d9bca 100644
>>>> --- a/drivers/firmware/dmi_scan.c
>>>> +++ b/drivers/firmware/dmi_scan.c
>>>> @@ -41,6 +41,16 @@ static struct dmi_memdev_info {
>>>>    } *dmi_memdev;
>>>>    static int dmi_memdev_nr;
>>>>    +static void *dmi_zalloc(unsigned len)
>>>> +{
>>>> +       void *ret = dmi_alloc(len);
>>>> +
>>>> +       if (ret)
>>>> +               memset(ret, 0, len);
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>>    static const char * __init dmi_string_nosave(const struct dmi_header *dm, u8 s)
>>>>    {
>>>>          const u8 *bp = ((u8 *) dm) + dm->length;
>>>> @@ -242,6 +252,12 @@ static void __init dmi_save_type(const struct dmi_header *dm, int slot,
>>>> (...)
>>>> @@ -250,15 +266,14 @@ static void __init dmi_save_one_device(int type, const char *name)
>>>>          if (dmi_find_device(type, name, NULL))
>>>>                  return;
>>>>    -     dev = dmi_alloc(sizeof(*dev) + strlen(name) + 1);
>>>> +       dev = dmi_zalloc(sizeof(*dev) + strlen(name) + 1);
>>>>          if (!dev)
>>>>                  return;
>>>>          dev->type = type;
>>>>          strcpy((char *)(dev + 1), name);
>>>>          dev->name = (char *)(dev + 1);
>>>> -       dev->device_data = NULL;
>>> This change seems rather unrelated, and I'm not sure what purpose it
>>> serves. On ia64 and arm64 it is clearly redundant as dmi_alloc calls
>>> kzalloc directly. On x86_64, extend_brk is called instead (don't ask me
>>> why, I have no clue) but looking at the code I see that it does
>>> memset(ret, 0, size) as well so memory is also zeroed there. Which makes
>>> dmi_alloc the same as dmi_zalloc on all 3 architectures.
>>>
>>> So please revert this change. This will make your patch easier to
>>> review, too.
>>>
>> Ok.  I had assumed extend_break wasn't zeroing since there were all the NULL assignments,
>> I should have looked.
>>
>> I was thinking about this, and the fwnode could just be added to the IPMI device.  I'm not
>> sure if you would prefer that over adding it to dmi_device.  The fwnode is in acpi_device,
>> and I was modelling these changes after that, but maybe that's not required here.
> I think dmi_device is right, especially if your patches result in a
> firmware_node sysfs link being created.  That way the link will point
> to the right place.

Yeah, that's the conclusion I had come to, I think.  It doesn't 
currently add the
firmware_node to sysfs, but that's easily added and probably a next logical
step.

I'll have a new set of patches out today after I compile test at each step.

Thanks,

-corey