[PATCH 0/4] ipmi: remove SPMI and update core driver with dev_printk

[PATCH 0/4] ipmi: remove SPMI and update core driver with dev_printk

[Myron Stowe]

These patches remove the SPMI based IPMI device discovery mechanism and
update the driver's core to use dev_printk() and its constructs.

As part of this patch series I wanted to remove the 'PFX' argument from
ipmi_of_probe()'s dev_printk constructs as I believe it produces redundant
output but I do not have a PPC platform to test against.

Ultimately, I would like to see if it is possible to also remove IPMI's
SMBIOS based device discovery mechanism.

---

Myron Stowe (4):
      ipmi: Raise precedence of PNP based discovery mechanisms (ACPI, PCI)
      ipmi: Remove SPMI table based device discovery method
      ipmi: Convert tracking of the ACPI device pointer to a PNP device
      ipmi: Update driver to use 'dev_printk()' and its constructs


 drivers/char/ipmi/ipmi_bt_sm.c      |    2 
 drivers/char/ipmi/ipmi_msghandler.c |   11 +
 drivers/char/ipmi/ipmi_si_intf.c    |  277 +++++++----------------------------
 3 files changed, 60 insertions(+), 230 deletions(-)

[PATCH 1/4] ipmi: Raise precedence of PNP based discovery mechanisms (ACPI, PCI)

[Myron Stowe]

This raises the precedence of PNP discovery mechanisms, ACPI and  PCI,
above SMBIOS and SPMI as indicated by Appendix C1 of the IPMI
specification.

Reference: IPMI - Intelligent Platform Management Interface Specification
           v2.0, Document Revision 1.0, 05.05.2005 Markup

  Appendix C1 - Locating IPMI System Interfaces via SMBIOS Tables
      "Note that the settings that this structure reports may be over-
      ridden by `Plug-and-Play' reassignment by the OS.  Therefore, this
      structure should be used only when the interface cannot be
      discovered via `Plug-and-Play' discovery mechanisms incorporated
      in interfaces such as PCI and ACPI."

Signed-off-by: Myron Stowe <myron.stowe@hp.com>
---

 drivers/char/ipmi/ipmi_si_intf.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 176f175..86a7dd7 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -3197,13 +3197,6 @@ static __devinit int init_ipmi_si(void)
 
 	hardcode_find_bmc();
 
-#ifdef CONFIG_DMI
-	dmi_find_bmc();
-#endif
-
-#ifdef CONFIG_ACPI
-	spmi_find_bmc();
-#endif
 #ifdef CONFIG_ACPI
 	pnp_register_driver(&ipmi_pnp_driver);
 #endif
@@ -3220,6 +3213,14 @@ static __devinit int init_ipmi_si(void)
 	of_register_platform_driver(&ipmi_of_platform_driver);
 #endif
 
+#ifdef CONFIG_DMI
+	dmi_find_bmc();
+#endif
+
+#ifdef CONFIG_ACPI
+	spmi_find_bmc();
+#endif
+
 	if (si_trydefaults) {
 		mutex_lock(&smi_infos_lock);
 		if (list_empty(&smi_infos)) {

[PATCH 2/4] ipmi: Remove SPMI table based device discovery method

[Myron Stowe]

Remove optional ACPI static Service Processor Management Interface
(SPMI) table based IPMI device discovery mechanism.

The SPMI table is available for use as an early boot process device
description mechanism (i.e. prior to the availability of the OS to
utilize ACPI, namespace based [IPI001], control methods) however,
‘Plug-and-Play’ enumeration mechanisms such as PCI and ACPI should
be used when possible [1].

Windows relies solely on ACPI/IPI0001 or PCI device based enumeration
for IPMI.  HP added the SPMI table for HP-UX to use before ACPI
namespace is available but Linux doesn't need such as its driver's
discovery processing takes place after both ACPI and PCI subsystems
have been set up.

[1] From Appendix C1 of the afore mentioned IPMI specification -
    "... this structure (referring to SPMI) should be used only
    when the interface cannot be discovered via 'Plug-and-Play'
    discovery mechanisms incorporated in interfaces such as PCI
    and ACPI."

Signed-off-by: Myron Stowe <myron.stowe@hp.com>
---

 drivers/char/ipmi/ipmi_bt_sm.c   |    2 
 drivers/char/ipmi/ipmi_si_intf.c |  165 --------------------------------------
 2 files changed, 2 insertions(+), 165 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_bt_sm.c b/drivers/char/ipmi/ipmi_bt_sm.c
index 7b98c06..d0e868e 100644
--- a/drivers/char/ipmi/ipmi_bt_sm.c
+++ b/drivers/char/ipmi/ipmi_bt_sm.c
@@ -204,7 +204,7 @@ static unsigned int bt_init_data(struct si_sm_data *bt, struct si_sm_io *io)
 	bt->BT_CAP_req2rsp = BT_NORMAL_TIMEOUT * 1000000;
 	bt->BT_CAP_retries = BT_NORMAL_RETRY_LIMIT;
 	/* BT_CAP_outreqs == zero is a flag to read BT Capabilities */
-	return 3; /* We claim 3 bytes of space; ought to check SPMI table */
+	return 3; /* We claim 3 bytes of space */
 }
 
 /* Jam a completion code (probably an error) into a response */
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 86a7dd7..806ae83 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -1118,7 +1118,7 @@ static struct ipmi_smi_handlers handlers = {
 
 /*
  * There can be 4 IO ports passed in (with or without IRQs), 4 addresses,
- * a default IO port, and 1 ACPI/SPMI address.  That sets SI_MAX_DRIVERS.
+ * a default IO port, and 1 ACPI address.  That sets SI_MAX_DRIVERS.
  */
 
 static LIST_HEAD(smi_infos);
@@ -1808,13 +1808,6 @@ static __devinit void hardcode_find_bmc(void)
 
 #include <linux/acpi.h>
 
-/*
- * Once we get an ACPI failure, we don't try any more, because we go
- * through the tables sequentially.  Once we don't find a table, there
- * are no more.
- */
-static int acpi_failure;
-
 /* For GPE-type interrupts. */
 static u32 ipmi_acpi_gpe(void *context)
 {
@@ -1873,158 +1866,6 @@ static int acpi_gpe_irq_setup(struct smi_info *info)
 	}
 }
 
-/*
- * Defined at
- * http://h21007.www2.hp.com/dspp/files/unprotected/devresource/
- * Docs/TechPapers/IA64/hpspmi.pdf
- */
-struct SPMITable {
-	s8	Signature[4];
-	u32	Length;
-	u8	Revision;
-	u8	Checksum;
-	s8	OEMID[6];
-	s8	OEMTableID[8];
-	s8	OEMRevision[4];
-	s8	CreatorID[4];
-	s8	CreatorRevision[4];
-	u8	InterfaceType;
-	u8	IPMIlegacy;
-	s16	SpecificationRevision;
-
-	/*
-	 * Bit 0 - SCI interrupt supported
-	 * Bit 1 - I/O APIC/SAPIC
-	 */
-	u8	InterruptType;
-
-	/*
-	 * If bit 0 of InterruptType is set, then this is the SCI
-	 * interrupt in the GPEx_STS register.
-	 */
-	u8	GPE;
-
-	s16	Reserved;
-
-	/*
-	 * If bit 1 of InterruptType is set, then this is the I/O
-	 * APIC/SAPIC interrupt.
-	 */
-	u32	GlobalSystemInterrupt;
-
-	/* The actual register address. */
-	struct acpi_generic_address addr;
-
-	u8	UID[4];
-
-	s8      spmi_id[1]; /* A '\0' terminated array starts here. */
-};
-
-static __devinit int try_init_spmi(struct SPMITable *spmi)
-{
-	struct smi_info  *info;
-	u8 		 addr_space;
-
-	if (spmi->IPMIlegacy != 1) {
-	    printk(KERN_INFO "IPMI: Bad SPMI legacy %d\n", spmi->IPMIlegacy);
-	    return -ENODEV;
-	}
-
-	if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY)
-		addr_space = IPMI_MEM_ADDR_SPACE;
-	else
-		addr_space = IPMI_IO_ADDR_SPACE;
-
-	info = kzalloc(sizeof(*info), GFP_KERNEL);
-	if (!info) {
-		printk(KERN_ERR "ipmi_si: Could not allocate SI data (3)\n");
-		return -ENOMEM;
-	}
-
-	info->addr_source = "SPMI";
-
-	/* Figure out the interface type. */
-	switch (spmi->InterfaceType) {
-	case 1:	/* KCS */
-		info->si_type = SI_KCS;
-		break;
-	case 2:	/* SMIC */
-		info->si_type = SI_SMIC;
-		break;
-	case 3:	/* BT */
-		info->si_type = SI_BT;
-		break;
-	default:
-		printk(KERN_INFO "ipmi_si: Unknown ACPI/SPMI SI type %d\n",
-			spmi->InterfaceType);
-		kfree(info);
-		return -EIO;
-	}
-
-	if (spmi->InterruptType & 1) {
-		/* We've got a GPE interrupt. */
-		info->irq = spmi->GPE;
-		info->irq_setup = acpi_gpe_irq_setup;
-	} else if (spmi->InterruptType & 2) {
-		/* We've got an APIC/SAPIC interrupt. */
-		info->irq = spmi->GlobalSystemInterrupt;
-		info->irq_setup = std_irq_setup;
-	} else {
-		/* Use the default interrupt setting. */
-		info->irq = 0;
-		info->irq_setup = NULL;
-	}
-
-	if (spmi->addr.bit_width) {
-		/* A (hopefully) properly formed register bit width. */
-		info->io.regspacing = spmi->addr.bit_width / 8;
-	} else {
-		info->io.regspacing = DEFAULT_REGSPACING;
-	}
-	info->io.regsize = info->io.regspacing;
-	info->io.regshift = spmi->addr.bit_offset;
-
-	if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
-		info->io_setup = mem_setup;
-		info->io.addr_type = IPMI_MEM_ADDR_SPACE;
-	} else if (spmi->addr.space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
-		info->io_setup = port_setup;
-		info->io.addr_type = IPMI_IO_ADDR_SPACE;
-	} else {
-		kfree(info);
-		printk(KERN_WARNING
-		       "ipmi_si: Unknown ACPI I/O Address type\n");
-		return -EIO;
-	}
-	info->io.addr_data = spmi->addr.address;
-
-	try_smi_init(info);
-
-	return 0;
-}
-
-static __devinit void spmi_find_bmc(void)
-{
-	acpi_status      status;
-	struct SPMITable *spmi;
-	int              i;
-
-	if (acpi_disabled)
-		return;
-
-	if (acpi_failure)
-		return;
-
-	for (i = 0; ; i++) {
-		status = acpi_get_table(ACPI_SIG_SPMI, i+1,
-					(struct acpi_table_header **)&spmi);
-		if (status != AE_OK)
-			return;
-
-		try_init_spmi(spmi);
-	}
-}
-
 static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 				    const struct pnp_device_id *dev_id)
 {
@@ -3217,10 +3058,6 @@ static __devinit int init_ipmi_si(void)
 	dmi_find_bmc();
 #endif
 
-#ifdef CONFIG_ACPI
-	spmi_find_bmc();
-#endif
-
 	if (si_trydefaults) {
 		mutex_lock(&smi_infos_lock);
 		if (list_empty(&smi_infos)) {

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] ipmi: Convert tracking of the ACPI device pointer to a PNP device

[Myron Stowe]

Convert PNP patch (git 9e368fa011d4e0aa050db348d69514900520e40b) to
maintain a pointer to a PNP device, 'pnp_dev', instead of the ACPI
device, 'acpi_dev', that is currently being tracked with PNP based
IPMI device discovery.

Signed-off-by: Myron Stowe <myron.stowe@hp.com>
---

 drivers/char/ipmi/ipmi_si_intf.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 806ae83..37c6912 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -1934,7 +1934,7 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 		info->irq_setup = std_irq_setup;
 	}
 
-	info->dev = &acpi_dev->dev;
+	info->dev = &dev->dev;
 	pnp_set_drvdata(dev, info);
 
 	return try_smi_init(info);

[PATCH 4/4] ipmi: Update driver to use 'dev_printk()' and its constructs

[Myron Stowe]

Update core IPMI driver printk()'s with dev_printk(), and its constructs,
to provide additional device topology information.


An example of the additional device topology for a PNP device -
  ipmi_si 00:02: probing via ACPI
  ipmi_si 00:02: [io  0x0ca2-0x0ca3] regsize 1 spacing 1 irq 0
  ipmi_si 00:02: Found new BMC (man_id: 0x00000b, prod_id: 0x0000, ...
  ipmi_si 00:02: IPMI kcs interface initialized

and for a PCI device -
  ipmi_si 0000:01:04.6: probing via PCI
  ipmi_si 0000:01:04.6: PCI INT A -> GSI 21 (level, low) -> IRQ 21
  ipmi_si 0000:01:04.6: [mem 0xf1ef0000-0xf1ef00ff] regsize 1 spaci...
  ipmi_si 0000:01:04.6: IPMI kcs interface initialized


Signed-off-by: Myron Stowe <myron.stowe@hp.com>
---

 drivers/char/ipmi/ipmi_msghandler.c |   11 ++--
 drivers/char/ipmi/ipmi_si_intf.c    |  103 ++++++++++++++++-------------------
 2 files changed, 53 insertions(+), 61 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index ec5e3f8..0e30114 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -2495,12 +2495,11 @@ static int ipmi_bmc_register(ipmi_smi_t intf, int ifnum,
 			return rv;
 		}
 
-		printk(KERN_INFO
-		       "ipmi: Found new BMC (man_id: 0x%6.6x, "
-		       " prod_id: 0x%4.4x, dev_id: 0x%2.2x)\n",
-		       bmc->id.manufacturer_id,
-		       bmc->id.product_id,
-		       bmc->id.device_id);
+		dev_info(intf->si_dev, "Found new BMC (man_id: 0x%6.6x, "
+			 "prod_id: 0x%4.4x, dev_id: 0x%2.2x)\n",
+			 bmc->id.manufacturer_id,
+			 bmc->id.product_id,
+			 bmc->id.device_id);
 	}
 
 	/*
diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 37c6912..e0966d2 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -573,9 +573,8 @@ static void handle_transaction_done(struct smi_info *smi_info)
 		smi_info->handlers->get_result(smi_info->si_sm, msg, 3);
 		if (msg[2] != 0) {
 			/* Error clearing flags */
-			printk(KERN_WARNING
-			       "ipmi_si: Error clearing flags: %2.2x\n",
-			       msg[2]);
+			dev_warn(smi_info->dev,
+				 "Error clearing flags: %2.2x\n", msg[2]);
 		}
 		if (smi_info->si_state == SI_CLEARING_FLAGS_THEN_SET_IRQ)
 			start_enable_irq(smi_info);
@@ -667,9 +666,8 @@ static void handle_transaction_done(struct smi_info *smi_info)
 		/* We got the flags from the SMI, now handle them. */
 		smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
 		if (msg[2] != 0) {
-			printk(KERN_WARNING
-			       "ipmi_si: Could not enable interrupts"
-			       ", failed get, using polled mode.\n");
+			dev_warn(smi_info->dev, "Could not enable interrupts"
+				 ", failed get, using polled mode.\n");
 			smi_info->si_state = SI_NORMAL;
 		} else {
 			msg[0] = (IPMI_NETFN_APP_REQUEST << 2);
@@ -691,9 +689,8 @@ static void handle_transaction_done(struct smi_info *smi_info)
 		/* We got the flags from the SMI, now handle them. */
 		smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
 		if (msg[2] != 0) {
-			printk(KERN_WARNING
-			       "ipmi_si: Could not enable interrupts"
-			       ", failed set, using polled mode.\n");
+			dev_warn(smi_info->dev, "Could not enable interrupts"
+				 ", failed set, using polled mode.\n");
 		}
 		smi_info->si_state = SI_NORMAL;
 		break;
@@ -706,9 +703,8 @@ static void handle_transaction_done(struct smi_info *smi_info)
 		/* We got the flags from the SMI, now handle them. */
 		smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
 		if (msg[2] != 0) {
-			printk(KERN_WARNING
-			       "ipmi_si: Could not disable interrupts"
-			       ", failed get.\n");
+			dev_warn(smi_info->dev, "Could not disable interrupts"
+				 ", failed get.\n");
 			smi_info->si_state = SI_NORMAL;
 		} else {
 			msg[0] = (IPMI_NETFN_APP_REQUEST << 2);
@@ -730,9 +726,8 @@ static void handle_transaction_done(struct smi_info *smi_info)
 		/* We got the flags from the SMI, now handle them. */
 		smi_info->handlers->get_result(smi_info->si_sm, msg, 4);
 		if (msg[2] != 0) {
-			printk(KERN_WARNING
-			       "ipmi_si: Could not disable interrupts"
-			       ", failed set.\n");
+			dev_warn(smi_info->dev, "Could not disable interrupts"
+				 ", failed set.\n");
 		}
 		smi_info->si_state = SI_NORMAL;
 		break;
@@ -1087,9 +1082,9 @@ static int smi_start_processing(void       *send_info,
 		new_smi->thread = kthread_run(ipmi_thread, new_smi,
 					      "kipmi%d", new_smi->intf_num);
 		if (IS_ERR(new_smi->thread)) {
-			printk(KERN_NOTICE "ipmi_si_intf: Could not start"
-			       " kernel thread due to error %ld, only using"
-			       " timers to drive the interface\n",
+			dev_notice(new_smi->dev, "Could not start"
+				   " kernel thread due to error %ld, only using"
+				   " timers to drive the interface\n",
 			       PTR_ERR(new_smi->thread));
 			new_smi->thread = NULL;
 		}
@@ -1752,6 +1747,8 @@ static __devinit void hardcode_find_bmc(void)
 			return;
 
 		info->addr_source = "hardcoded";
+		printk(KERN_INFO "ipmi_si: probing via %s\n",
+		       info->addr_source);
 
 		if (!si_type[i] || strcmp(si_type[i], "kcs") == 0) {
 			info->si_type = SI_KCS;
@@ -1871,6 +1868,7 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 {
 	struct acpi_device *acpi_dev;
 	struct smi_info *info;
+	struct resource *res;
 	acpi_handle handle;
 	acpi_status status;
 	unsigned long long tmp;
@@ -1884,6 +1882,7 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 		return -ENOMEM;
 
 	info->addr_source = "ACPI";
+	dev_info(&dev->dev, "probing via %s\n", info->addr_source);
 
 	handle = acpi_dev->handle;
 
@@ -1903,22 +1902,26 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 		info->si_type = SI_BT;
 		break;
 	default:
-		dev_info(&dev->dev, "unknown interface type %lld\n", tmp);
+		dev_info(&dev->dev, "unknown IPMI type %lld\n", tmp);
 		goto err_free;
 	}
 
-	if (pnp_port_valid(dev, 0)) {
+	res = pnp_get_resource(dev, IORESOURCE_IO, 0);
+	if (res) {
 		info->io_setup = port_setup;
 		info->io.addr_type = IPMI_IO_ADDR_SPACE;
-		info->io.addr_data = pnp_port_start(dev, 0);
-	} else if (pnp_mem_valid(dev, 0)) {
-		info->io_setup = mem_setup;
-		info->io.addr_type = IPMI_MEM_ADDR_SPACE;
-		info->io.addr_data = pnp_mem_start(dev, 0);
 	} else {
+		res = pnp_get_resource(dev, IORESOURCE_MEM, 0);
+		if (res) {
+			info->io_setup = mem_setup;
+			info->io.addr_type = IPMI_MEM_ADDR_SPACE;
+		}
+	}
+	if (!res) {
 		dev_err(&dev->dev, "no I/O or memory address\n");
 		goto err_free;
 	}
+	info->io.addr_data = res->start;
 
 	info->io.regspacing = DEFAULT_REGSPACING;
 	info->io.regsize = DEFAULT_REGSPACING;
@@ -1937,6 +1940,10 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 	info->dev = &dev->dev;
 	pnp_set_drvdata(dev, info);
 
+	dev_info(info->dev, "%pR regsize %d spacing %d irq %d\n",
+		res, info->io.regsize, info->io.regspacing,
+		info->irq);
+
 	return try_smi_init(info);
 
 err_free:
@@ -2048,6 +2055,7 @@ static __devinit void try_init_dmi(struct dmi_ipmi_data *ipmi_data)
 	}
 
 	info->addr_source = "SMBIOS";
+	printk(KERN_INFO "ipmi_si: probing via %s\n", info->addr_source);
 
 	switch (ipmi_data->type) {
 	case 0x01: /* KCS */
@@ -2147,6 +2155,7 @@ static int __devinit ipmi_pci_probe(struct pci_dev *pdev,
 		return -ENOMEM;
 
 	info->addr_source = "PCI";
+	dev_info(&pdev->dev, "probing via %s\n", info->addr_source);
 
 	switch (class_type) {
 	case PCI_ERMC_CLASSCODE_TYPE_SMIC:
@@ -2163,15 +2172,13 @@ static int __devinit ipmi_pci_probe(struct pci_dev *pdev,
 
 	default:
 		kfree(info);
-		printk(KERN_INFO "ipmi_si: %s: Unknown IPMI type: %d\n",
-		       pci_name(pdev), class_type);
+		dev_info(&pdev->dev, "Unknown IPMI type: %d\n", class_type);
 		return -ENOMEM;
 	}
 
 	rv = pci_enable_device(pdev);
 	if (rv) {
-		printk(KERN_ERR "ipmi_si: %s: couldn't enable PCI device\n",
-		       pci_name(pdev));
+		dev_err(&pdev->dev, "couldn't enable PCI device\n");
 		kfree(info);
 		return rv;
 	}
@@ -2199,6 +2206,10 @@ static int __devinit ipmi_pci_probe(struct pci_dev *pdev,
 	info->dev = &pdev->dev;
 	pci_set_drvdata(pdev, info);
 
+	dev_info(&pdev->dev, "%pR regsize %d spacing %d irq %d\n",
+		&pdev->resource[0], info->io.regsize, info->io.regspacing,
+		info->irq);
+
 	return try_smi_init(info);
 }
 
@@ -2306,7 +2317,7 @@ static int __devinit ipmi_of_probe(struct of_device *dev,
 	info->irq		= irq_of_parse_and_map(dev->node, 0);
 	info->dev		= &dev->dev;
 
-	dev_dbg(&dev->dev, "addr 0x%lx regsize %d spacing %d irq %x\n",
+	dev_dbg(&dev->dev, "addr 0x%lx regsize %d spacing %d irq %d\n",
 		info->io.addr_data, info->io.regsize, info->io.regspacing,
 		info->irq);
 
@@ -2769,17 +2780,6 @@ static int try_smi_init(struct smi_info *new_smi)
 	int rv;
 	int i;
 
-	if (new_smi->addr_source) {
-		printk(KERN_INFO "ipmi_si: Trying %s-specified %s state"
-		       " machine at %s address 0x%lx, slave address 0x%x,"
-		       " irq %d\n",
-		       new_smi->addr_source,
-		       si_to_str[new_smi->si_type],
-		       addr_space_to_str[new_smi->io.addr_type],
-		       new_smi->io.addr_data,
-		       new_smi->slave_addr, new_smi->irq);
-	}
-
 	mutex_lock(&smi_infos_lock);
 	if (!is_new_interface(new_smi)) {
 		printk(KERN_WARNING "ipmi_si: duplicate interface\n");
@@ -2916,9 +2916,8 @@ static int try_smi_init(struct smi_info *new_smi)
 			       "bmc",
 			       new_smi->slave_addr);
 	if (rv) {
-		printk(KERN_ERR
-		       "ipmi_si: Unable to register device: error %d\n",
-		       rv);
+		dev_err(new_smi->dev, "Unable to register device: error %d\n",
+			rv);
 		goto out_err_stop_timer;
 	}
 
@@ -2926,9 +2925,7 @@ static int try_smi_init(struct smi_info *new_smi)
 				     type_file_read_proc,
 				     new_smi);
 	if (rv) {
-		printk(KERN_ERR
-		       "ipmi_si: Unable to create proc entry: %d\n",
-		       rv);
+		dev_err(new_smi->dev, "Unable to create proc entry: %d\n", rv);
 		goto out_err_stop_timer;
 	}
 
@@ -2936,9 +2933,7 @@ static int try_smi_init(struct smi_info *new_smi)
 				     stat_file_read_proc,
 				     new_smi);
 	if (rv) {
-		printk(KERN_ERR
-		       "ipmi_si: Unable to create proc entry: %d\n",
-		       rv);
+		dev_err(new_smi->dev, "Unable to create proc entry: %d\n", rv);
 		goto out_err_stop_timer;
 	}
 
@@ -2946,9 +2941,7 @@ static int try_smi_init(struct smi_info *new_smi)
 				     param_read_proc,
 				     new_smi);
 	if (rv) {
-		printk(KERN_ERR
-		       "ipmi_si: Unable to create proc entry: %d\n",
-		       rv);
+		dev_err(new_smi->dev, "Unable to create proc entry: %d\n", rv);
 		goto out_err_stop_timer;
 	}
 
@@ -2956,8 +2949,8 @@ static int try_smi_init(struct smi_info *new_smi)
 
 	mutex_unlock(&smi_infos_lock);
 
-	printk(KERN_INFO "IPMI %s interface initialized\n",
-	       si_to_str[new_smi->si_type]);
+	dev_info(new_smi->dev, "IPMI %s interface initialized\n",
+		 si_to_str[new_smi->si_type]);
 
 	return 0;
 

RE: [Openipmi-developer] [PATCH 2/4] ipmi: Remove SPMI table based device discovery method

[Bela Lubkin]

Myron Stowe wrote:

> Remove optional ACPI static Service Processor Management Interface
> (SPMI) table based IPMI device discovery mechanism.

Myron,

The other patches look reasonable.  This one, I'm not so sure.

My concern is based on these thoughts:

During early hardware bring-up, both ACPI and IPMI subsystems
of a new system design tend to be a total mess.  The static
table parts of ACPI are likely to be usable much earlier than
the parts that require a fully functional ACPI subsystem.

For the people developing and testing the new system's IPMI
subsystem, losing the SPMI discovery method could be a
significant pain point.

A new IPMI subsystem may also be "bolted on" to an existing
system for early development before the new system is mature
enough to even power on.  In such a situation it may be easier
to supply a fake table than fake live ACPI information.  The
hardware at that point may not be in PCI space, yet may also
not be at static addresses that would be easy to supply as
module load parameters.

I would be more comfortable if you kept this code, perhaps
suppressed under a "CONFIG_IPMI_SPMI" config option.

>Bela<

Re: [PATCH 3/4] ipmi: Convert tracking of the ACPI device pointer to a PNP device

[ykzhao]

On Thu, 2010-03-04 at 11:44 +0800, Myron Stowe wrote:
> Convert PNP patch (git 9e368fa011d4e0aa050db348d69514900520e40b) to
> maintain a pointer to a PNP device, 'pnp_dev', instead of the ACPI
> device, 'acpi_dev', that is currently being tracked with PNP based
> IPMI device discovery.

Hi, Myron
	Now the IPMI interface defined in ACPI namespace is detected by using
PNP device driver. But it seems that this still belongs to ACPI
detection mechanism. At the same time IPMI device defined in ACPI
namespace is also used to enable the communication between ACPI and IPMI
device. In such case it will be better to know whether the IPMI
interface is discovered by using ACPI detection mechanism , which can be
realized by comparing the info->dev with acpi_dev->dev.
     Can we still use the acpi_dev->dev to track the IPMI device
discovery?

thanks.
     Yakui
        
> 
> Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> ---
> 
>  drivers/char/ipmi/ipmi_si_intf.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 806ae83..37c6912 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -1934,7 +1934,7 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
>  		info->irq_setup = std_irq_setup;
>  	}
>  
> -	info->dev = &acpi_dev->dev;
> +	info->dev = &dev->dev;
>  	pnp_set_drvdata(dev, info);
>  
>  	return try_smi_init(info);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] ipmi: remove SPMI and update core driver with dev_printk

[Corey Minyard]

Myron Stowe wrote:
> These patches remove the SPMI based IPMI device discovery mechanism and
> update the driver's core to use dev_printk() and its constructs.
>
> As part of this patch series I wanted to remove the 'PFX' argument from
> ipmi_of_probe()'s dev_printk constructs as I believe it produces redundant
> output but I do not have a PPC platform to test against.
>   
I like the dev_printk() stuff.  The order of discovery change is 
correct, too, I believe.  I don't have a PPC platform (with OF, anyway) 
with IPMI, so I can't test that, either.

> Ultimately, I would like to see if it is possible to also remove IPMI's
> SMBIOS based device discovery mechanism.
>   
Maybe in an ideal world, but I don't know where an ideal world is, so I 
have to live in the one I'm in.  There's plenty of systems that only 
document this in SMBIOS tables, there's plenty of systems with broken 
ACPI, etc.  So SMBIOS and SPMI are going to have to stay.


-corey

RE: [Openipmi-developer] [PATCH 2/4] ipmi: Remove SPMI table based device discovery method

[Myron Stowe]

On Wed, 2010-03-03 at 23:56 -0800, Bela Lubkin wrote:
> Myron Stowe wrote:
> 
> > Remove optional ACPI static Service Processor Management Interface
> > (SPMI) table based IPMI device discovery mechanism.
> 
> Myron,
> 
> The other patches look reasonable.  This one, I'm not so sure.
> 
> My concern is based on these thoughts:
> 
> During early hardware bring-up, both ACPI and IPMI subsystems
> of a new system design tend to be a total mess.  The static
> table parts of ACPI are likely to be usable much earlier than
> the parts that require a fully functional ACPI subsystem.

Hi Bela:

ACPI in new systems does tend to present a lot of challenges.  My job is
new platform enablement focused which more often than not translates to
"helping discover and fix BIOS/ACPI issues" so I definitely understand.

You also astutely point out that ACPI's static tables are available for
use by an OS - or OSPM in ACPI speak - much earlier than its namespace.
This is true but it is also our belief that no one uses it except HP.

> 
> For the people developing and testing the new system's IPMI
> subsystem, losing the SPMI discovery method could be a
> significant pain point.
> 
> A new IPMI subsystem may also be "bolted on" to an existing
> system for early development before the new system is mature
> enough to even power on.  In such a situation it may be easier
> to supply a fake table than fake live ACPI information.  The
> hardware at that point may not be in PCI space, yet may also
> not be at static addresses that would be easy to supply as
> module load parameters.

The point that in such a situation providing static addresses as module
load parameters is not a tenable approach seems to be a stretch but that
is tangential to the main discussion so lets not muddy the discussion
with such unless you feel there is more to say.

I see how one's life could be easier relying on the BIOS setting up
static tables such as either the SMBIOS or SPMI.  That way the BIOS is
taking the dynamic aspects into account relieving the OS, or OS/platform
bring up engineer, from such.  In such a case, what benefits does the
SPMI table provide as opposed to the SMBIOS table?
> 
> I would be more comfortable if you kept this code, perhaps
> suppressed under a "CONFIG_IPMI_SPMI" config option.

I just don't see enough remaining value in keeping the SPMI table
capability.  It seems to be redundant, and if truly so, just adds
unnecessary code, complexity, and maintenance aspects to the driver.  We
typically don't keep code in the kernel just for bring up efforts.


Thanks very much for expressing your concerns here.  I fully expected,
and still expect, the SPMI removal patch to be the contentious component
of this series.  Fostering a healthy discussion in these regards should
eventually help us understand that removing such is in fact tenable or,
that there really are valid technical reasons why it is not.

Myron
> 
> >Bela<


-- 
Myron Stowe                             HP Open Source Linux Lab (OSLL)

Re: [PATCH 3/4] ipmi: Convert tracking of the ACPI device pointer to a PNP device

[Myron Stowe]

On Thu, 2010-03-04 at 17:20 +0800, ykzhao wrote:
> On Thu, 2010-03-04 at 11:44 +0800, Myron Stowe wrote:
> > Convert PNP patch (git 9e368fa011d4e0aa050db348d69514900520e40b) to
> > maintain a pointer to a PNP device, 'pnp_dev', instead of the ACPI
> > device, 'acpi_dev', that is currently being tracked with PNP based
> > IPMI device discovery.
> 
> Hi, Myron
> 	Now the IPMI interface defined in ACPI namespace is detected by using
> PNP device driver. But it seems that this still belongs to ACPI
> detection mechanism.

Hi Yakui:

The ACPI namespace enumerated IPMI interface is indeed detected via PNP.
This definitely can be a little confusing, especially in light of the
fact that ACPI detection mechanisms exist.  The reasoning for using PNP
is based on the fact that within Linux, PNP subsumes ACPI (and ISA,
EIAS, and also PNP BIOS).  Using a Venn diagram to model the situation
produces something like:
  --------------------
  | PNP              |
  |     -----------  |
  |     | ACPI    |  |
  |     -----------  |
  |                  |
  --------------------
I left ISA, EISA, and PNP BIOS out of the Venn diagram since ascii based
diagrams are so horrific already but each would also be a distinct,
non-overlapping, component within PNP.

>  At the same time IPMI device defined in ACPI
> namespace is also used to enable the communication between ACPI and
> IPMI device. In such case it will be better to know whether the IPMI
> interface is discovered by using ACPI detection mechanism , which can
> be realized by comparing the info->dev with acpi_dev->dev.

I'm thinking your concern here is related to the IPMI op-region patch
you have produced that Corey has not yet taken in.

One can still obtain an 'acpi_device' handle from the 'pnp_dev' handle
that 'ipmi_pnp_probe()' is given.  The 'opregion_setup()' routine
provides such.  I was going to provide an example but looking at
'ipmi_pnp_probe()' it is already being used at the very top of the
routine so you should be able to use 'acpi_dev'; it is already provided
and setup.  More so, one will not make it into the body of
'ipmi_pnp_probe()' unless the device was enumerated via ACPI namespace
due to the check against the just mentioned 'acpi_dev' setup (in other
words, if there were an IPMI device that was enumerated via PNP BIOS the
check would have detected such and already bailed out).  This seems to
provide the means for detection type checking that you are alluding to.

> 	Can we still use the acpi_dev->dev to track the IPMI device
> discovery?

The existing 'acpi_dev' provides both a means for detection type
checking and, seems much simpler than carrying forward and maintaining
some type of info->dev acpi_dev->dev based comparison.

Myron
> 
> thanks.
>      Yakui
>         
> > 
> > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > ---
> > 
> >  drivers/char/ipmi/ipmi_si_intf.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > index 806ae83..37c6912 100644
> > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > @@ -1934,7 +1934,7 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> >  		info->irq_setup = std_irq_setup;
> >  	}
> >  
> > -	info->dev = &acpi_dev->dev;
> > +	info->dev = &dev->dev;
> >  	pnp_set_drvdata(dev, info);
> >  
> >  	return try_smi_init(info);
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Myron Stowe                             HP Open Source Linux Lab (OSLL)

Re: [PATCH 3/4] ipmi: Convert tracking of the ACPI device pointer to a PNP device

[ykzhao]

On Fri, 2010-03-05 at 04:48 +0800, Myron Stowe wrote:
> On Thu, 2010-03-04 at 17:20 +0800, ykzhao wrote:
> > On Thu, 2010-03-04 at 11:44 +0800, Myron Stowe wrote:
> > > Convert PNP patch (git 9e368fa011d4e0aa050db348d69514900520e40b) to
> > > maintain a pointer to a PNP device, 'pnp_dev', instead of the ACPI
> > > device, 'acpi_dev', that is currently being tracked with PNP based
> > > IPMI device discovery.
> > 
> > Hi, Myron
> > 	Now the IPMI interface defined in ACPI namespace is detected by using
> > PNP device driver. But it seems that this still belongs to ACPI
> > detection mechanism.
> 
> Hi Yakui:
> 
> The ACPI namespace enumerated IPMI interface is indeed detected via PNP.
> This definitely can be a little confusing, especially in light of the
> fact that ACPI detection mechanisms exist.  The reasoning for using PNP
> is based on the fact that within Linux, PNP subsumes ACPI (and ISA,
> EIAS, and also PNP BIOS).  Using a Venn diagram to model the situation
> produces something like:
>   --------------------
>   | PNP              |
>   |     -----------  |
>   |     | ACPI    |  |
>   |     -----------  |
>   |                  |
>   --------------------
> I left ISA, EISA, and PNP BIOS out of the Venn diagram since ascii based
> diagrams are so horrific already but each would also be a distinct,
> non-overlapping, component within PNP.
> 

Hi, Myron
    Thanks for the detailed explanation.
    It seems that the ACPI device is firstly added to the system. And
then we will add the corresponding PNP device based on detected ACPI
device so that most available PNP device drivers still can be used.
Although sometimes both the PNP device and ACPI device are added, they
will point to the same physical device. Right?
    Of course it will be clearer if the info->dev = pnp_dev->dev. It
indicates that it is tracked by PNP dev.

> >  At the same time IPMI device defined in ACPI
> > namespace is also used to enable the communication between ACPI and
> > IPMI device. In such case it will be better to know whether the IPMI
> > interface is discovered by using ACPI detection mechanism , which can
> > be realized by comparing the info->dev with acpi_dev->dev.
> 
> I'm thinking your concern here is related to the IPMI op-region patch
> you have produced that Corey has not yet taken in.

Yes. The ACPI IPMI opregion patch tries to enable the communication
between ACPI and IPMI device by comparing the info->dev and
acpi_dev->dev. Info->dev will be passed as the argument of
ipmi_smi_watcher callback function(new_smi), in which we create the
communication channel between ACPI and IPMI.

If we change the info->dev from acpi_dev->dev to pnp_dev->dev, then we
will use another mechanism to check whether the IPMI device is defined
in ACPI namespace. Of course it will be simpler.

> 
> One can still obtain an 'acpi_device' handle from the 'pnp_dev' handle
> that 'ipmi_pnp_probe()' is given.  The 'opregion_setup()' routine
> provides such.  I was going to provide an example but looking at
> 'ipmi_pnp_probe()' it is already being used at the very top of the
> routine so you should be able to use 'acpi_dev'; it is already provided
> and setup.  More so, one will not make it into the body of
> 'ipmi_pnp_probe()' unless the device was enumerated via ACPI namespace
> due to the check against the just mentioned 'acpi_dev' setup (in other
> words, if there were an IPMI device that was enumerated via PNP BIOS the
> check would have detected such and already bailed out).  This seems to
> provide the means for detection type checking that you are alluding to.

Very good explanation. And you are right. Now the ipmi pnp device driver
is only effective to the IPMI device defined in ACPI namespace. 

> 
> > 	Can we still use the acpi_dev->dev to track the IPMI device
> > discovery?
> 
> The existing 'acpi_dev' provides both a means for detection type
> checking and, seems much simpler than carrying forward and maintaining
> some type of info->dev acpi_dev->dev based comparison.
> 
> Myron
> > 
> > thanks.
> >      Yakui
> >         
> > > 
> > > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > > ---
> > > 
> > >  drivers/char/ipmi/ipmi_si_intf.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > > index 806ae83..37c6912 100644
> > > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > > @@ -1934,7 +1934,7 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> > >  		info->irq_setup = std_irq_setup;
> > >  	}
> > >  
> > > -	info->dev = &acpi_dev->dev;
> > > +	info->dev = &dev->dev;
> > >  	pnp_set_drvdata(dev, info);
> > >  
> > >  	return try_smi_init(info);
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> 


------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

RE: [Openipmi-developer] [PATCH 2/4] ipmi: Remove SPMI table based device discovery method

[Bela Lubkin]

Myron Stowe wrote:

> ACPI in new systems does tend to present a lot of challenges. 
>  My job is
> new platform enablement focused which more often than not 
> translates to
> "helping discover and fix BIOS/ACPI issues" so I definitely 
> understand.
> 
> You also astutely point out that ACPI's static tables are 
> available for
> use by an OS - or OSPM in ACPI speak - much earlier than its 
> namespace.
> This is true but it is also our belief that no one uses it except HP.

Are you watching what goes on in embedded systems or only
the PC / server marketplace?

> I see how one's life could be easier relying on the BIOS setting up
> static tables such as either the SMBIOS or SPMI.  That way the BIOS is
> taking the dynamic aspects into account relieving the OS, or 
> OS/platform
> bring up engineer, from such.  In such a case, what benefits does the
> SPMI table provide as opposed to the SMBIOS table?

It gives you twice as many chances to find a sufficiently
non-buggy table to get on with your work ;-}

> > I would be more comfortable if you kept this code, perhaps
> > suppressed under a "CONFIG_IPMI_SPMI" config option.
> 
> I just don't see enough remaining value in keeping the SPMI table
> capability.  It seems to be redundant, and if truly so, just adds
> unnecessary code, complexity, and maintenance aspects to the 
> driver.  We
> typically don't keep code in the kernel just for bring up efforts.

Well, I'm not doing any such bring-up work so I will
step out of the discussion here.  But I'll leave you
with a bit of a curse: if you remove this code, I
predict that the person it will come back to bite is
you...

>Bela<

Re: [PATCH 2/4] ipmi: Remove SPMI table based device discovery method

[Bjorn Helgaas]

On Friday 05 March 2010 05:58:12 am Bela Lubkin wrote:
> It gives you twice as many chances to find a sufficiently
> non-buggy table to get on with your work ;-}

I think this is the wrong way to look at it.  I don't want a
"let's try a bunch of things, maybe one will work" plan.  In my
opinion, it's better to have a single strategy that everybody
uses because that one way becomes well-tested and well-understood.

Windows only looks at ACPI namespace and PCI, and the obvious
consequence is that those mechanisms are tested and tend to work.

Linux uses SPMI and SMBIOS and (until recently) ignored the ACPI
namespace.  SPMI and SMBIOS are redundant copies of what's in the
namespace, and that redundancy makes inconsistencies more likely.

The result is that (a) we waste the BIOS engineers' time because
they have to implement multiple descriptions, (b) we waste our
time resolving inconsistencies between the descriptions, and (c)
we delay finding and fixing bugs in the canonical ACPI namespace
descriptions.

Bjorn

------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [PATCH 0/4] ipmi: remove SPMI and update core driver with dev_printk

[Myron Stowe]

On Thu, 2010-03-04 at 08:07 -0600, Corey Minyard wrote: 
> Myron Stowe wrote:
> > These patches remove the SPMI based IPMI device discovery mechanism and
> > update the driver's core to use dev_printk() and its constructs.
> >
> > As part of this patch series I wanted to remove the 'PFX' argument from
> > ipmi_of_probe()'s dev_printk constructs as I believe it produces redundant
> > output but I do not have a PPC platform to test against.
> >   
> I like the dev_printk() stuff.  The order of discovery change is 
> correct, too, I believe.  I don't have a PPC platform (with OF, anyway) 
> with IPMI, so I can't test that, either.

Bummer, I would have liked to clean up 'ipmi_of_probe()' as part of this
series as it is obvious it was not written in the style of the original
author - reading 'ipmi_pci_probe()', 'ipmi_pnp_probe()', and
'ipmi_of_probe()' side-by-side the latter sticks out like a sore thumb.
> 
> > Ultimately, I would like to see if it is possible to also remove IPMI's
> > SMBIOS based device discovery mechanism.
> >   
> Maybe in an ideal world, but I don't know where an ideal world is, so I 
> have to live in the one I'm in.  There's plenty of systems that only 
> document this in SMBIOS tables, there's plenty of systems with broken 
> ACPI, etc.  So SMBIOS and SPMI are going to have to stay.

It was a mistake on my part to make the comment about SMBIOS as the
patch series does not include such action and it unnecessarily creates
confusion with the current intent of removing SPMI as a discovery
method.

It would seem that the most likely situation requiring SPMI would be an
embedded system that does not use ACPI namespace and does not support
running Windows (Windows relies on ACPI namespace and/or PCI; it does
not look for, or use, SMBIOS and/or SPMI).  If anyone reading this
discussion thread knows of an explicit example in which SPMI is truly
required then obviously I would be wrong and could no longer pursue
such.  Its just that I'm not yet convinced of such based on my
experiences.

Myron

> -corey


-- 
Myron Stowe                             HP Open Source Linux Lab (OSLL)

Re: [PATCH 3/4] ipmi: Convert tracking of the ACPI device pointer to a PNP device

[Myron Stowe]

On Fri, 2010-03-05 at 09:46 +0800, ykzhao wrote:
> On Fri, 2010-03-05 at 04:48 +0800, Myron Stowe wrote:
> > On Thu, 2010-03-04 at 17:20 +0800, ykzhao wrote:
> > > On Thu, 2010-03-04 at 11:44 +0800, Myron Stowe wrote:
> > > > Convert PNP patch (git 9e368fa011d4e0aa050db348d69514900520e40b) to
> > > > maintain a pointer to a PNP device, 'pnp_dev', instead of the ACPI
> > > > device, 'acpi_dev', that is currently being tracked with PNP based
> > > > IPMI device discovery.
> > > 
> > > Hi, Myron
> > > 	Now the IPMI interface defined in ACPI namespace is detected by using
> > > PNP device driver. But it seems that this still belongs to ACPI
> > > detection mechanism.
> > 
> > Hi Yakui:
> > 
> > The ACPI namespace enumerated IPMI interface is indeed detected via PNP.
> > This definitely can be a little confusing, especially in light of the
> > fact that ACPI detection mechanisms exist.  The reasoning for using PNP
> > is based on the fact that within Linux, PNP subsumes ACPI (and ISA,
> > EIAS, and also PNP BIOS).  Using a Venn diagram to model the situation
> > produces something like:
> >   --------------------
> >   | PNP              |
> >   |     -----------  |
> >   |     | ACPI    |  |
> >   |     -----------  |
> >   |                  |
> >   --------------------
> > I left ISA, EISA, and PNP BIOS out of the Venn diagram since ascii based
> > diagrams are so horrific already but each would also be a distinct,
> > non-overlapping, component within PNP.
> > 
> 
> Hi, Myron
>     Thanks for the detailed explanation.
>     It seems that the ACPI device is firstly added to the system. And
> then we will add the corresponding PNP device based on detected ACPI
> device so that most available PNP device drivers still can be used.
> Although sometimes both the PNP device and ACPI device are added, they
> will point to the same physical device. Right?

Yes, a PNP device (struct pnp_dev) is obviously distinct from an ACPI
device (struct acpi_device) but they both end up pointing to the same
physical device.

>     Of course it will be clearer if the info->dev = pnp_dev->dev. It
> indicates that it is tracked by PNP dev.
> 
> > >  At the same time IPMI device defined in ACPI
> > > namespace is also used to enable the communication between ACPI and
> > > IPMI device. In such case it will be better to know whether the IPMI
> > > interface is discovered by using ACPI detection mechanism , which can
> > > be realized by comparing the info->dev with acpi_dev->dev.
> > 
> > I'm thinking your concern here is related to the IPMI op-region patch
> > you have produced that Corey has not yet taken in.
> 
> Yes. The ACPI IPMI opregion patch tries to enable the communication
> between ACPI and IPMI device by comparing the info->dev and
> acpi_dev->dev. Info->dev will be passed as the argument of
> ipmi_smi_watcher callback function(new_smi), in which we create the
> communication channel between ACPI and IPMI.
> 
> If we change the info->dev from acpi_dev->dev to pnp_dev->dev, then we
> will use another mechanism to check whether the IPMI device is defined
> in ACPI namespace. Of course it will be simpler.

Good, simpler tends to be better.

By the way, if there are conflicts between our two patch series I'll be
more than willing to help resolve such.  If it ends up being easier for
Corey to take in your series first I'll rework my series and resubmit
it.

Myron
> 
> > 
> > One can still obtain an 'acpi_device' handle from the 'pnp_dev' handle
> > that 'ipmi_pnp_probe()' is given.  The 'opregion_setup()' routine
> > provides such.  I was going to provide an example but looking at
> > 'ipmi_pnp_probe()' it is already being used at the very top of the
> > routine so you should be able to use 'acpi_dev'; it is already provided
> > and setup.  More so, one will not make it into the body of
> > 'ipmi_pnp_probe()' unless the device was enumerated via ACPI namespace
> > due to the check against the just mentioned 'acpi_dev' setup (in other
> > words, if there were an IPMI device that was enumerated via PNP BIOS the
> > check would have detected such and already bailed out).  This seems to
> > provide the means for detection type checking that you are alluding to.
> 
> Very good explanation. And you are right. Now the ipmi pnp device driver
> is only effective to the IPMI device defined in ACPI namespace. 
> 
> > 
> > > 	Can we still use the acpi_dev->dev to track the IPMI device
> > > discovery?
> > 
> > The existing 'acpi_dev' provides both a means for detection type
> > checking and, seems much simpler than carrying forward and maintaining
> > some type of info->dev acpi_dev->dev based comparison.
> > 
> > Myron
> > > 
> > > thanks.
> > >      Yakui
> > >         
> > > > 
> > > > Signed-off-by: Myron Stowe <myron.stowe@hp.com>
> > > > ---
> > > > 
> > > >  drivers/char/ipmi/ipmi_si_intf.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> > > > index 806ae83..37c6912 100644
> > > > --- a/drivers/char/ipmi/ipmi_si_intf.c
> > > > +++ b/drivers/char/ipmi/ipmi_si_intf.c
> > > > @@ -1934,7 +1934,7 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
> > > >  		info->irq_setup = std_irq_setup;
> > > >  	}
> > > >  
> > > > -	info->dev = &acpi_dev->dev;
> > > > +	info->dev = &dev->dev;
> > > >  	pnp_set_drvdata(dev, info);
> > > >  
> > > >  	return try_smi_init(info);
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > 
> > 
> 


-- 
Myron Stowe                             HP Open Source Linux Lab (OSLL)

RE: [Openipmi-developer] [PATCH 2/4] ipmi: Remove SPMI table based device discovery method

[Myron Stowe]

On Fri, 2010-03-05 at 04:58 -0800, Bela Lubkin wrote:
> Myron Stowe wrote:
> 
> > ACPI in new systems does tend to present a lot of challenges. 
> >  My job is
> > new platform enablement focused which more often than not 
> > translates to
> > "helping discover and fix BIOS/ACPI issues" so I definitely 
> > understand.
> > 
> > You also astutely point out that ACPI's static tables are 
> > available for
> > use by an OS - or OSPM in ACPI speak - much earlier than its 
> > namespace.
> > This is true but it is also our belief that no one uses it except HP.
> 
> Are you watching what goes on in embedded systems or only
> the PC / server marketplace?

No, I do not watch the embedded system space.  Would such folks tend to
track openipmi-developer@lists.sourceforge.net (at least with respect to
IPMI)?  If not, are you aware of a specific mail reflector that I should
be including in this discussion?

> 
> > I see how one's life could be easier relying on the BIOS setting up
> > static tables such as either the SMBIOS or SPMI.  That way the BIOS is
> > taking the dynamic aspects into account relieving the OS, or 
> > OS/platform
> > bring up engineer, from such.  In such a case, what benefits does the
> > SPMI table provide as opposed to the SMBIOS table?
> 
> It gives you twice as many chances to find a sufficiently
> non-buggy table to get on with your work ;-}
> 
> > > I would be more comfortable if you kept this code, perhaps
> > > suppressed under a "CONFIG_IPMI_SPMI" config option.
> > 
> > I just don't see enough remaining value in keeping the SPMI table
> > capability.  It seems to be redundant, and if truly so, just adds
> > unnecessary code, complexity, and maintenance aspects to the 
> > driver.  We
> > typically don't keep code in the kernel just for bring up efforts.
> 
> Well, I'm not doing any such bring-up work so I will
> step out of the discussion here.  But I'll leave you
> with a bit of a curse: if you remove this code, I
> predict that the person it will come back to bite is
> you...

I hope I'm not coming off as closed minded and causing you to disengage
as most every concern you have raised has been valid and needful of
consideration.  As mean as putting a jinx on me was (I'm mostly kidding
there), you are correct there too: it will be me that it comes back to
bite.  I still feel that this is worth pursuing as no progress is ever
made without some risk but I do want discussions to occur so that I can
learn, preferably beforehand, if I am wrong.

Myron
> 
> >Bela<


-- 
Myron Stowe                             HP Open Source Linux Lab (OSLL)

Re: [PATCH 3/4] ipmi: Convert tracking of the ACPI device pointer to a PNP device

[Bjorn Helgaas]

On Friday 05 March 2010 09:41:05 am Myron Stowe wrote:
> By the way, if there are conflicts between our two patch series I'll be
> more than willing to help resolve such.  If it ends up being easier for
> Corey to take in your series first I'll rework my series and resubmit
> it.

BTW, Corey, what's happening as far as merging Yakaui's IPMI opregion
patches?  I checked linux-next, and I don't see them there.  Are you
planning to push them upstream during this (soon-to-close) merge
window?

http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=history;f=drivers/char/ipmi/ipmi_si_intf.c;h=176f1751237f57807f3023d9427972dcb6614392;hb=HEAD

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] ipmi: remove SPMI and update core driver with dev_printk

[Myron Stowe]

On Thu, 2010-03-04 at 08:07 -0600, Corey Minyard wrote: 
> Myron Stowe wrote:
> > These patches remove the SPMI based IPMI device discovery mechanism and
> > update the driver's core to use dev_printk() and its constructs.
> >
snip 
> 
> > Ultimately, I would like to see if it is possible to also remove IPMI's
> > SMBIOS based device discovery mechanism.
> >   
> Maybe in an ideal world, but I don't know where an ideal world is, so I 
> have to live in the one I'm in.  There's plenty of systems that only 
> document this in SMBIOS tables, there's plenty of systems with broken 
> ACPI, etc.  So SMBIOS and SPMI are going to have to stay.
> 
Discussions concerning this patch series seem to have ceased and I have
not heard back from anyone that can definitively state that the SPMI
table is actively being used in the field.

At this point, it seems that the most logical ways to proceed are: 
      * Take the patch series as it is with the possible risk that we
        will encounter a system in the field that relies on SPMI.
        
      * Or, I can rework the patch series, basically splitting it into
        two.  The first series would include 
                ipmi: Raise precedence of PNP based discovery mechanisms (ACPI, PCI),
                ipmi: Convert tracking of the ACPI device pointer to a PNP device,
                ipmi: Update driver to use 'dev_printk()' and its constructs,
        with the second series including 
                ipmi: Remove SPMI table based device discovery method,
                ipmi: Further 'dev_printk()' conversion that is dependent on SPMI removal.
The idea being that the more contentious second series, now isolated,
could be easily retracted if necessary.
        

I do understand that it is possible that we will encounter a system that
relies on SPMI.  The most likely scenario would seem to be with respect
to embedded systems.  However, I think it is unlikely for the following
reasons (as always, please respond to any and all concerns as it is this
community that are the subject matter experts) and would still like to
consider removing such:

        The IPMI related SMBIOS and SPMI tables are static, the idea
        being that such tables can be referenced by the OS very early in
        its boot related processing; well before the OS has progressed
        to the point of ACPI namespace based control method
        availability.  A couple of points concerning such are: they seem
        relatively redundant with respect to each other, and, the
        current IPMI driver is not truly capable of taking advantage of
        such "static" capability[1].
        
        IPMI's SPMI table discovery mechanism was championed by HP.
        This was mainly for HP's proprietary HP-UX OS as it did want to
        take advantage of the early reference capability that static
        tables provide (I do not know if SMBIOS was considered, or if
        so, why it was not considered viable - perhaps someone on this
        list has that history).  As such, we seriously question whether
        other vendors have even included SPMI into their BIOS'.
        
        Modern systems rely on ACPI namespace and/or PCI as preferred
        enumeration techniques.  Windows only looks at these, and not
        SMBIOS or SPMI, which has bearing since x86 Linux typically runs
        on platforms also capable of running Windows.  Yes, embedded
        systems are the biggest exception here.

        Bjorn Helgaas also covered many of these points, and a few
        others, in his reply to Bela Lubkin concerning [Patch 2/4] which
        is worth re-reading.
        
I'm looking forward to any feedback or responses of any kind, even
rebuttals.

Thanks,

Myron
        

[1] I was not able to express this point as well as I would have liked.
The point I'm attempting to make is that the IPMI driver, as it
currently exists, is not capable of probing and attaching to a device
during early boot processing to really take advantage of "static"
tables.  The fact that it is capable of being built and run/installed as
a module is a big hint towards such but is not, in itself, definitive.
However, its PCI and ACPI probing mechanisms are definitive in that they
rely on their respective subsystems having already been setup which is
well after early boot processing.

> 
> -corey
> 


-- 
Myron Stowe                             HP Open Source Linux Lab (OSLL)

Re: [SPAM] - Re: [PATCH 0/4] ipmi: remove SPMI and update core driver with dev_printk - Email found in subject

[Andy Cress]

Myron,

How about this for an approach with SPMI:
Rather than remove it entirely, use '#ifdef SPMI_DEPRECATED' around that
code, so that it can still be enabled at compile time by those that
later find that they need it. 
And add a comment with instructions to notify this list if someone finds
that they need to enable it.   

That should prove conclusively whether it is used/needed or not.

Andy

-----Original Message-----
From: Myron Stowe [mailto:myron.stowe@hp.com] 
Sent: Tuesday, March 09, 2010 6:16 PM
To: Corey Minyard
Cc: linux-acpi@vger.kernel.org;
openipmi-developer@lists.sourceforge.net; lenb@kernel.org
Subject: [SPAM] - Re: [Openipmi-developer] [PATCH 0/4] ipmi: remove SPMI
and update core driver with dev_printk - Email found in subject

On Thu, 2010-03-04 at 08:07 -0600, Corey Minyard wrote: 
> Myron Stowe wrote:
> > These patches remove the SPMI based IPMI device discovery mechanism
and
> > update the driver's core to use dev_printk() and its constructs.
> >
snip 
> 
> > Ultimately, I would like to see if it is possible to also remove
IPMI's
> > SMBIOS based device discovery mechanism.
> >   
> Maybe in an ideal world, but I don't know where an ideal world is, so
I 
> have to live in the one I'm in.  There's plenty of systems that only 
> document this in SMBIOS tables, there's plenty of systems with broken 
> ACPI, etc.  So SMBIOS and SPMI are going to have to stay.
> 
Discussions concerning this patch series seem to have ceased and I have
not heard back from anyone that can definitively state that the SPMI
table is actively being used in the field.

At this point, it seems that the most logical ways to proceed are: 
      * Take the patch series as it is with the possible risk that we
        will encounter a system in the field that relies on SPMI.
        
      * Or, I can rework the patch series, basically splitting it into
        two.  The first series would include 
                ipmi: Raise precedence of PNP based discovery mechanisms
(ACPI, PCI),
                ipmi: Convert tracking of the ACPI device pointer to a
PNP device,
                ipmi: Update driver to use 'dev_printk()' and its
constructs,
        with the second series including 
                ipmi: Remove SPMI table based device discovery method,
                ipmi: Further 'dev_printk()' conversion that is
dependent on SPMI removal.
The idea being that the more contentious second series, now isolated,
could be easily retracted if necessary.
        

I do understand that it is possible that we will encounter a system that
relies on SPMI.  The most likely scenario would seem to be with respect
to embedded systems.  However, I think it is unlikely for the following
reasons (as always, please respond to any and all concerns as it is this
community that are the subject matter experts) and would still like to
consider removing such:

        The IPMI related SMBIOS and SPMI tables are static, the idea
        being that such tables can be referenced by the OS very early in
        its boot related processing; well before the OS has progressed
        to the point of ACPI namespace based control method
        availability.  A couple of points concerning such are: they seem
        relatively redundant with respect to each other, and, the
        current IPMI driver is not truly capable of taking advantage of
        such "static" capability[1].
        
        IPMI's SPMI table discovery mechanism was championed by HP.
        This was mainly for HP's proprietary HP-UX OS as it did want to
        take advantage of the early reference capability that static
        tables provide (I do not know if SMBIOS was considered, or if
        so, why it was not considered viable - perhaps someone on this
        list has that history).  As such, we seriously question whether
        other vendors have even included SPMI into their BIOS'.
        
        Modern systems rely on ACPI namespace and/or PCI as preferred
        enumeration techniques.  Windows only looks at these, and not
        SMBIOS or SPMI, which has bearing since x86 Linux typically runs
        on platforms also capable of running Windows.  Yes, embedded
        systems are the biggest exception here.

        Bjorn Helgaas also covered many of these points, and a few
        others, in his reply to Bela Lubkin concerning [Patch 2/4] which
        is worth re-reading.
        
I'm looking forward to any feedback or responses of any kind, even
rebuttals.

Thanks,

Myron
        

[1] I was not able to express this point as well as I would have liked.
The point I'm attempting to make is that the IPMI driver, as it
currently exists, is not capable of probing and attaching to a device
during early boot processing to really take advantage of "static"
tables.  The fact that it is capable of being built and run/installed as
a module is a big hint towards such but is not, in itself, definitive.
However, its PCI and ACPI probing mechanisms are definitive in that they
rely on their respective subsystems having already been setup which is
well after early boot processing.

> 
> -corey
> 


-- 
Myron Stowe                             HP Open Source Linux Lab (OSLL)




------------------------------------------------------------------------
------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
sts.sourceforge.net/lists/listinfo/openipmi-developer



------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [SPAM] - Re: [PATCH 0/4] ipmi: remove SPMI and update core driver with dev_printk - Email found in subject

[Corey Minyard]

Andy Cress wrote:
> Myron,
>
> How about this for an approach with SPMI:
> Rather than remove it entirely, use '#ifdef SPMI_DEPRECATED' around that
> code, so that it can still be enabled at compile time by those that
> later find that they need it. 
> And add a comment with instructions to notify this list if someone finds
> that they need to enable it.   
>
> That should prove conclusively whether it is used/needed or not.
>   
I'd prefer way to prove this without causing people problems in the 
field.  The average user is not going to look at the source code and go 
"Oh, this is what I need to do!"  It will be a painful process for them 
to figure out what happened.

I don't see a way to test this in the field without causing issues.  If 
this code does not hurt anything, then IMHO it needs to stay.  This is a 
legacy driver, really, so removing things of this nature is dangerous.  
If leaving the code in is a problem, then we need to figure something out.

I'm for the "two patch" approach mentioned by Myron.

-corey

> Andy
>
> -----Original Message-----
> From: Myron Stowe [mailto:myron.stowe@hp.com] 
> Sent: Tuesday, March 09, 2010 6:16 PM
> To: Corey Minyard
> Cc: linux-acpi@vger.kernel.org;
> openipmi-developer@lists.sourceforge.net; lenb@kernel.org
> Subject: [SPAM] - Re: [Openipmi-developer] [PATCH 0/4] ipmi: remove SPMI
> and update core driver with dev_printk - Email found in subject
>
> On Thu, 2010-03-04 at 08:07 -0600, Corey Minyard wrote: 
>   
>> Myron Stowe wrote:
>>     
>>> These patches remove the SPMI based IPMI device discovery mechanism
>>>       
> and
>   
>>> update the driver's core to use dev_printk() and its constructs.
>>>
>>>       
> snip 
>   
>>> Ultimately, I would like to see if it is possible to also remove
>>>       
> IPMI's
>   
>>> SMBIOS based device discovery mechanism.
>>>   
>>>       
>> Maybe in an ideal world, but I don't know where an ideal world is, so
>>     
> I 
>   
>> have to live in the one I'm in.  There's plenty of systems that only 
>> document this in SMBIOS tables, there's plenty of systems with broken 
>> ACPI, etc.  So SMBIOS and SPMI are going to have to stay.
>>
>>     
> Discussions concerning this patch series seem to have ceased and I have
> not heard back from anyone that can definitively state that the SPMI
> table is actively being used in the field.
>
> At this point, it seems that the most logical ways to proceed are: 
>       * Take the patch series as it is with the possible risk that we
>         will encounter a system in the field that relies on SPMI.
>         
>       * Or, I can rework the patch series, basically splitting it into
>         two.  The first series would include 
>                 ipmi: Raise precedence of PNP based discovery mechanisms
> (ACPI, PCI),
>                 ipmi: Convert tracking of the ACPI device pointer to a
> PNP device,
>                 ipmi: Update driver to use 'dev_printk()' and its
> constructs,
>         with the second series including 
>                 ipmi: Remove SPMI table based device discovery method,
>                 ipmi: Further 'dev_printk()' conversion that is
> dependent on SPMI removal.
> The idea being that the more contentious second series, now isolated,
> could be easily retracted if necessary.
>         
>
> I do understand that it is possible that we will encounter a system that
> relies on SPMI.  The most likely scenario would seem to be with respect
> to embedded systems.  However, I think it is unlikely for the following
> reasons (as always, please respond to any and all concerns as it is this
> community that are the subject matter experts) and would still like to
> consider removing such:
>
>         The IPMI related SMBIOS and SPMI tables are static, the idea
>         being that such tables can be referenced by the OS very early in
>         its boot related processing; well before the OS has progressed
>         to the point of ACPI namespace based control method
>         availability.  A couple of points concerning such are: they seem
>         relatively redundant with respect to each other, and, the
>         current IPMI driver is not truly capable of taking advantage of
>         such "static" capability[1].
>         
>         IPMI's SPMI table discovery mechanism was championed by HP.
>         This was mainly for HP's proprietary HP-UX OS as it did want to
>         take advantage of the early reference capability that static
>         tables provide (I do not know if SMBIOS was considered, or if
>         so, why it was not considered viable - perhaps someone on this
>         list has that history).  As such, we seriously question whether
>         other vendors have even included SPMI into their BIOS'.
>         
>         Modern systems rely on ACPI namespace and/or PCI as preferred
>         enumeration techniques.  Windows only looks at these, and not
>         SMBIOS or SPMI, which has bearing since x86 Linux typically runs
>         on platforms also capable of running Windows.  Yes, embedded
>         systems are the biggest exception here.
>
>         Bjorn Helgaas also covered many of these points, and a few
>         others, in his reply to Bela Lubkin concerning [Patch 2/4] which
>         is worth re-reading.
>         
> I'm looking forward to any feedback or responses of any kind, even
> rebuttals.
>
> Thanks,
>
> Myron
>         
>
> [1] I was not able to express this point as well as I would have liked.
> The point I'm attempting to make is that the IPMI driver, as it
> currently exists, is not capable of probing and attaching to a device
> during early boot processing to really take advantage of "static"
> tables.  The fact that it is capable of being built and run/installed as
> a module is a big hint towards such but is not, in itself, definitive.
> However, its PCI and ACPI probing mechanisms are definitive in that they
> rely on their respective subsystems having already been setup which is
> well after early boot processing.
>
>   
>> -corey
>>
>>     
>
>
>   


------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev

Re: [Openipmi-developer] [PATCH 0/4] ipmi: remove SPMI and update core driver with dev_printk - Email found in subject

[Myron Stowe]

On Wed, 2010-03-10 at 09:20 -0600, Corey Minyard wrote:
> Andy Cress wrote:
> > Myron,
> >
> > How about this for an approach with SPMI:
> > Rather than remove it entirely, use '#ifdef SPMI_DEPRECATED' around that
> > code, so that it can still be enabled at compile time by those that
> > later find that they need it. 
> > And add a comment with instructions to notify this list if someone finds
> > that they need to enable it.   
> >
> > That should prove conclusively whether it is used/needed or not.
> >   
> I'd prefer way to prove this without causing people problems in the 
> field.  The average user is not going to look at the source code and go 
> "Oh, this is what I need to do!"  It will be a painful process for them 
> to figure out what happened.
> 
> I don't see a way to test this in the field without causing issues.

Unfortunately I don't either (assuming that it would have).

>   If 
> this code does not hurt anything, then IMHO it needs to stay.

This code is causing HP problems (yes, the irony is not lost on me)
which was the basis of us researching whether or not it is truly
required and attempting to remove it.

>   This is a 
> legacy driver, really, so removing things of this nature is dangerous.  
> If leaving the code in is a problem, then we need to figure something out.
> 
> I'm for the "two patch" approach mentioned by Myron.

It looks like I need to concede here.  As I've said before, I'll rework
the patch.  My question at this point is - Is there any point in
creating the second patch series in the "two patch" approach?

Myron

P.S.  Thanks Andy for your implied support - or at least open minded
attitude.
> 
> -corey
> 
> > Andy
> >
> > -----Original Message-----
> > From: Myron Stowe [mailto:myron.stowe@hp.com] 
> > Sent: Tuesday, March 09, 2010 6:16 PM
> > To: Corey Minyard
> > Cc: linux-acpi@vger.kernel.org;
> > openipmi-developer@lists.sourceforge.net; lenb@kernel.org
> > Subject: [SPAM] - Re: [Openipmi-developer] [PATCH 0/4] ipmi: remove SPMI
> > and update core driver with dev_printk - Email found in subject
> >
> > On Thu, 2010-03-04 at 08:07 -0600, Corey Minyard wrote: 
> >   
> >> Myron Stowe wrote:
> >>     
> >>> These patches remove the SPMI based IPMI device discovery mechanism
> >>>       
> > and
> >   
> >>> update the driver's core to use dev_printk() and its constructs.
> >>>
> >>>       
> > snip 
> >   
> >>> Ultimately, I would like to see if it is possible to also remove
> >>>       
> > IPMI's
> >   
> >>> SMBIOS based device discovery mechanism.
> >>>   
> >>>       
> >> Maybe in an ideal world, but I don't know where an ideal world is, so
> >>     
> > I 
> >   
> >> have to live in the one I'm in.  There's plenty of systems that only 
> >> document this in SMBIOS tables, there's plenty of systems with broken 
> >> ACPI, etc.  So SMBIOS and SPMI are going to have to stay.
> >>
> >>     
> > Discussions concerning this patch series seem to have ceased and I have
> > not heard back from anyone that can definitively state that the SPMI
> > table is actively being used in the field.
> >
> > At this point, it seems that the most logical ways to proceed are: 
> >       * Take the patch series as it is with the possible risk that we
> >         will encounter a system in the field that relies on SPMI.
> >         
> >       * Or, I can rework the patch series, basically splitting it into
> >         two.  The first series would include 
> >                 ipmi: Raise precedence of PNP based discovery mechanisms
> > (ACPI, PCI),
> >                 ipmi: Convert tracking of the ACPI device pointer to a
> > PNP device,
> >                 ipmi: Update driver to use 'dev_printk()' and its
> > constructs,
> >         with the second series including 
> >                 ipmi: Remove SPMI table based device discovery method,
> >                 ipmi: Further 'dev_printk()' conversion that is
> > dependent on SPMI removal.
> > The idea being that the more contentious second series, now isolated,
> > could be easily retracted if necessary.
> >         
> >
> > I do understand that it is possible that we will encounter a system that
> > relies on SPMI.  The most likely scenario would seem to be with respect
> > to embedded systems.  However, I think it is unlikely for the following
> > reasons (as always, please respond to any and all concerns as it is this
> > community that are the subject matter experts) and would still like to
> > consider removing such:
> >
> >         The IPMI related SMBIOS and SPMI tables are static, the idea
> >         being that such tables can be referenced by the OS very early in
> >         its boot related processing; well before the OS has progressed
> >         to the point of ACPI namespace based control method
> >         availability.  A couple of points concerning such are: they seem
> >         relatively redundant with respect to each other, and, the
> >         current IPMI driver is not truly capable of taking advantage of
> >         such "static" capability[1].
> >         
> >         IPMI's SPMI table discovery mechanism was championed by HP.
> >         This was mainly for HP's proprietary HP-UX OS as it did want to
> >         take advantage of the early reference capability that static
> >         tables provide (I do not know if SMBIOS was considered, or if
> >         so, why it was not considered viable - perhaps someone on this
> >         list has that history).  As such, we seriously question whether
> >         other vendors have even included SPMI into their BIOS'.
> >         
> >         Modern systems rely on ACPI namespace and/or PCI as preferred
> >         enumeration techniques.  Windows only looks at these, and not
> >         SMBIOS or SPMI, which has bearing since x86 Linux typically runs
> >         on platforms also capable of running Windows.  Yes, embedded
> >         systems are the biggest exception here.
> >
> >         Bjorn Helgaas also covered many of these points, and a few
> >         others, in his reply to Bela Lubkin concerning [Patch 2/4] which
> >         is worth re-reading.
> >         
> > I'm looking forward to any feedback or responses of any kind, even
> > rebuttals.
> >
> > Thanks,
> >
> > Myron
> >         
> >
> > [1] I was not able to express this point as well as I would have liked.
> > The point I'm attempting to make is that the IPMI driver, as it
> > currently exists, is not capable of probing and attaching to a device
> > during early boot processing to really take advantage of "static"
> > tables.  The fact that it is capable of being built and run/installed as
> > a module is a big hint towards such but is not, in itself, definitive.
> > However, its PCI and ACPI probing mechanisms are definitive in that they
> > rely on their respective subsystems having already been setup which is
> > well after early boot processing.
> >
> >   
> >> -corey
> >>
> >>     
> >
> >
> >   
> 


-- 
Myron Stowe                             HP Open Source Linux Lab (OSLL)

RE: [Openipmi-developer] [PATCH 0/4] ipmi: remove SPMI and update core driver with dev_printk - Email found in subject

[Bela Lubkin]

I support the "#ifdef it out" approach, provided it is done according to usual Linux kernel
procedure.  ifdef it on CONFIG_IPMI_SPMI, add appropriate lines to the right Kconfig file,
turn it off by default.  Making it an obscure #ifdef with no proper configuration
infrastructure around it would be wrong.

Configuring a feature out (but leaving the escape hatch for people who need it on) is a
time-tested method of removing functionality when it is not 100% proven that it can be
safely removed in all cases.

>Bela<
________________________________________
From: Myron Stowe [myron.stowe@hp.com]
Sent: Wednesday, March 10, 2010 9:55 AM
To: Corey Minyard
Cc: linux-acpi@vger.kernel.org; openipmi-developer@lists.sourceforge.net; Andy Cress; lenb@kernel.org
Subject: Re: [Openipmi-developer] [PATCH 0/4] ipmi: remove SPMI and update core driver with dev_printk - Email found in subject

On Wed, 2010-03-10 at 09:20 -0600, Corey Minyard wrote:
> Andy Cress wrote:
> > Myron,
> >
> > How about this for an approach with SPMI:
> > Rather than remove it entirely, use '#ifdef SPMI_DEPRECATED' around that
> > code, so that it can still be enabled at compile time by those that
> > later find that they need it.
> > And add a comment with instructions to notify this list if someone finds
> > that they need to enable it.
> >
> > That should prove conclusively whether it is used/needed or not.
> >
> I'd prefer way to prove this without causing people problems in the
> field.  The average user is not going to look at the source code and go
> "Oh, this is what I need to do!"  It will be a painful process for them
> to figure out what happened.
>
> I don't see a way to test this in the field without causing issues.

Unfortunately I don't either (assuming that it would have).

>   If
> this code does not hurt anything, then IMHO it needs to stay.

This code is causing HP problems (yes, the irony is not lost on me)
which was the basis of us researching whether or not it is truly
required and attempting to remove it.

>   This is a
> legacy driver, really, so removing things of this nature is dangerous.
> If leaving the code in is a problem, then we need to figure something out.
>
> I'm for the "two patch" approach mentioned by Myron.

It looks like I need to concede here.  As I've said before, I'll rework
the patch.  My question at this point is - Is there any point in
creating the second patch series in the "two patch" approach?

Myron

P.S.  Thanks Andy for your implied support - or at least open minded
attitude.
>
> -corey
>
> > Andy
> >
> > -----Original Message-----
> > From: Myron Stowe [mailto:myron.stowe@hp.com]
> > Sent: Tuesday, March 09, 2010 6:16 PM
> > To: Corey Minyard
> > Cc: linux-acpi@vger.kernel.org;
> > openipmi-developer@lists.sourceforge.net; lenb@kernel.org
> > Subject: [SPAM] - Re: [Openipmi-developer] [PATCH 0/4] ipmi: remove SPMI
> > and update core driver with dev_printk - Email found in subject
> >
> > On Thu, 2010-03-04 at 08:07 -0600, Corey Minyard wrote:
> >
> >> Myron Stowe wrote:
> >>
> >>> These patches remove the SPMI based IPMI device discovery mechanism
> >>>
> > and
> >
> >>> update the driver's core to use dev_printk() and its constructs.
> >>>
> >>>
> > snip
> >
> >>> Ultimately, I would like to see if it is possible to also remove
> >>>
> > IPMI's
> >
> >>> SMBIOS based device discovery mechanism.
> >>>
> >>>
> >> Maybe in an ideal world, but I don't know where an ideal world is, so
> >>
> > I
> >
> >> have to live in the one I'm in.  There's plenty of systems that only
> >> document this in SMBIOS tables, there's plenty of systems with broken
> >> ACPI, etc.  So SMBIOS and SPMI are going to have to stay.
> >>
> >>
> > Discussions concerning this patch series seem to have ceased and I have
> > not heard back from anyone that can definitively state that the SPMI
> > table is actively being used in the field.
> >
> > At this point, it seems that the most logical ways to proceed are:
> >       * Take the patch series as it is with the possible risk that we
> >         will encounter a system in the field that relies on SPMI.
> >
> >       * Or, I can rework the patch series, basically splitting it into
> >         two.  The first series would include
> >                 ipmi: Raise precedence of PNP based discovery mechanisms
> > (ACPI, PCI),
> >                 ipmi: Convert tracking of the ACPI device pointer to a
> > PNP device,
> >                 ipmi: Update driver to use 'dev_printk()' and its
> > constructs,
> >         with the second series including
> >                 ipmi: Remove SPMI table based device discovery method,
> >                 ipmi: Further 'dev_printk()' conversion that is
> > dependent on SPMI removal.
> > The idea being that the more contentious second series, now isolated,
> > could be easily retracted if necessary.
> >
> >
> > I do understand that it is possible that we will encounter a system that
> > relies on SPMI.  The most likely scenario would seem to be with respect
> > to embedded systems.  However, I think it is unlikely for the following
> > reasons (as always, please respond to any and all concerns as it is this
> > community that are the subject matter experts) and would still like to
> > consider removing such:
> >
> >         The IPMI related SMBIOS and SPMI tables are static, the idea
> >         being that such tables can be referenced by the OS very early in
> >         its boot related processing; well before the OS has progressed
> >         to the point of ACPI namespace based control method
> >         availability.  A couple of points concerning such are: they seem
> >         relatively redundant with respect to each other, and, the
> >         current IPMI driver is not truly capable of taking advantage of
> >         such "static" capability[1].
> >
> >         IPMI's SPMI table discovery mechanism was championed by HP.
> >         This was mainly for HP's proprietary HP-UX OS as it did want to
> >         take advantage of the early reference capability that static
> >         tables provide (I do not know if SMBIOS was considered, or if
> >         so, why it was not considered viable - perhaps someone on this
> >         list has that history).  As such, we seriously question whether
> >         other vendors have even included SPMI into their BIOS'.
> >
> >         Modern systems rely on ACPI namespace and/or PCI as preferred
> >         enumeration techniques.  Windows only looks at these, and not
> >         SMBIOS or SPMI, which has bearing since x86 Linux typically runs
> >         on platforms also capable of running Windows.  Yes, embedded
> >         systems are the biggest exception here.
> >
> >         Bjorn Helgaas also covered many of these points, and a few
> >         others, in his reply to Bela Lubkin concerning [Patch 2/4] which
> >         is worth re-reading.
> >
> > I'm looking forward to any feedback or responses of any kind, even
> > rebuttals.
> >
> > Thanks,
> >
> > Myron
> >
> >
> > [1] I was not able to express this point as well as I would have liked.
> > The point I'm attempting to make is that the IPMI driver, as it
> > currently exists, is not capable of probing and attaching to a device
> > during early boot processing to really take advantage of "static"
> > tables.  The fact that it is capable of being built and run/installed as
> > a module is a big hint towards such but is not, in itself, definitive.
> > However, its PCI and ACPI probing mechanisms are definitive in that they
> > rely on their respective subsystems having already been setup which is
> > well after early boot processing.
> >
> >
> >> -corey
> >>
> >>
> >
> >
> >
>


--
Myron Stowe                             HP Open Source Linux Lab (OSLL)


------------------------------------------------------------------------------
Download Intel&#174; Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Openipmi-developer mailing list
Openipmi-developer@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openipmi-developer