[PATCH 1/3] ipmi: Split device discovery and registration

[PATCH 1/3] ipmi: Split device discovery and registration

[Matthew Garrett]

The ipmi spec indicates that we should only make use of one si per bmc,
so separate device discovery and registration to make that possible.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/char/ipmi/ipmi_si_intf.c |  115 +++++++++++++++++++++++++-------------
 1 files changed, 75 insertions(+), 40 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 4462b11..d2bdf92 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -300,6 +300,7 @@ static int num_max_busy_us;
 
 static int unload_when_empty = 1;
 
+static int add_smi(struct smi_info *smi);
 static int try_smi_init(struct smi_info *smi);
 static void cleanup_one_si(struct smi_info *to_clean);
 
@@ -1777,7 +1778,9 @@ static int hotmod_handler(const char *val, struct kernel_param *kp)
 				info->irq_setup = std_irq_setup;
 			info->slave_addr = ipmb;
 
-			try_smi_init(info);
+			if (!add_smi(info))
+				if (try_smi_init(info))
+					cleanup_one_si(info);
 		} else {
 			/* remove */
 			struct smi_info *e, *tmp_e;
@@ -1863,7 +1866,9 @@ static __devinit void hardcode_find_bmc(void)
 			info->irq_setup = std_irq_setup;
 		info->slave_addr = slave_addrs[i];
 
-		try_smi_init(info);
+		if (!add_smi(info))
+			if (try_smi_init(info))
+				cleanup_one_si(info);
 	}
 }
 
@@ -2061,7 +2066,7 @@ static __devinit int try_init_spmi(struct SPMITable *spmi)
 	}
 	info->io.addr_data = spmi->addr.address;
 
-	try_smi_init(info);
+	add_smi(info);
 
 	return 0;
 }
@@ -2159,7 +2164,7 @@ static int __devinit ipmi_pnp_probe(struct pnp_dev *dev,
 	info->dev = &acpi_dev->dev;
 	pnp_set_drvdata(dev, info);
 
-	return try_smi_init(info);
+	return add_smi(info);
 
 err_free:
 	kfree(info);
@@ -2318,7 +2323,7 @@ static __devinit void try_init_dmi(struct dmi_ipmi_data *ipmi_data)
 	if (info->irq)
 		info->irq_setup = std_irq_setup;
 
-	try_smi_init(info);
+	add_smi(info);
 }
 
 static void __devinit dmi_find_bmc(void)
@@ -2421,7 +2426,7 @@ static int __devinit ipmi_pci_probe(struct pci_dev *pdev,
 	info->dev = &pdev->dev;
 	pci_set_drvdata(pdev, info);
 
-	return try_smi_init(info);
+	return add_smi(info);
 }
 
 static void __devexit ipmi_pci_remove(struct pci_dev *pdev)
@@ -2534,7 +2539,7 @@ static int __devinit ipmi_of_probe(struct of_device *dev,
 
 	dev_set_drvdata(&dev->dev, info);
 
-	return try_smi_init(info);
+	return add_smi(info);
 }
 
 static int __devexit ipmi_of_remove(struct of_device *dev)
@@ -2960,14 +2965,17 @@ static __devinit void default_find_bmc(void)
 		info->io.regsize = DEFAULT_REGSPACING;
 		info->io.regshift = 0;
 
-		if (try_smi_init(info) == 0) {
-			/* Found one... */
-			printk(KERN_INFO "ipmi_si: Found default %s state"
-			       " machine at %s address 0x%lx\n",
-			       si_to_str[info->si_type],
-			       addr_space_to_str[info->io.addr_type],
-			       info->io.addr_data);
-			return;
+		if (add_smi(info) == 0) {
+			if ((try_smi_init(info)) == 0) {
+				/* Found one... */
+				printk(KERN_INFO "ipmi_si: Found default %s"
+				" state machine at %s address 0x%lx\n",
+				si_to_str[info->si_type],
+				addr_space_to_str[info->io.addr_type],
+				info->io.addr_data);
+			} else {
+				cleanup_one_si(info);
+			}
 		}
 	}
 }
@@ -2986,9 +2994,36 @@ static int is_new_interface(struct smi_info *info)
 	return 1;
 }
 
+static int add_smi(struct smi_info *new_smi)
+{
+	int rv = 0;
+
+	printk(KERN_INFO "ipmi_si: Adding %s-specified %s state machine",
+			new_smi->addr_source, si_to_str[new_smi->si_type]);
+	mutex_lock(&smi_infos_lock);
+	if (!is_new_interface(new_smi)) {
+		printk(KERN_CONT ": duplicate interface\n");
+		rv = -EBUSY;
+		goto out_err;
+	}
+
+	printk(KERN_CONT "\n");
+
+	/* So we know not to free it unless we have allocated one. */
+	new_smi->intf = NULL;
+	new_smi->si_sm = NULL;
+	new_smi->handlers = NULL;
+
+	list_add_tail(&new_smi->link, &smi_infos);
+
+out_err:
+	mutex_unlock(&smi_infos_lock);
+	return rv;
+}
+
 static int try_smi_init(struct smi_info *new_smi)
 {
-	int rv;
+	int rv = 0;
 	int i;
 
 	if (new_smi->addr_source) {
@@ -3002,18 +3037,6 @@ static int try_smi_init(struct smi_info *new_smi)
 		       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");
-		rv = -EBUSY;
-		goto out_err;
-	}
-
-	/* So we know not to free it unless we have allocated one. */
-	new_smi->intf = NULL;
-	new_smi->si_sm = NULL;
-	new_smi->handlers = NULL;
-
 	switch (new_smi->si_type) {
 	case SI_KCS:
 		new_smi->handlers = &kcs_smi_handlers;
@@ -3174,8 +3197,6 @@ static int try_smi_init(struct smi_info *new_smi)
 		goto out_err_stop_timer;
 	}
 
-	list_add_tail(&new_smi->link, &smi_infos);
-
 	mutex_unlock(&smi_infos_lock);
 
 	printk(KERN_INFO "IPMI %s interface initialized\n",
@@ -3188,11 +3209,17 @@ static int try_smi_init(struct smi_info *new_smi)
 	wait_for_timer_and_thread(new_smi);
 
  out_err:
-	if (new_smi->intf)
+	new_smi->interrupt_disabled = 1;
+
+	if (new_smi->intf) {
 		ipmi_unregister_smi(new_smi->intf);
+		new_smi->intf = NULL;
+	}
 
-	if (new_smi->irq_cleanup)
+	if (new_smi->irq_cleanup) {
 		new_smi->irq_cleanup(new_smi);
+		new_smi->irq_cleanup = NULL;
+	}
 
 	/*
 	 * Wait until we know that we are out of any interrupt
@@ -3205,16 +3232,21 @@ static int try_smi_init(struct smi_info *new_smi)
 		if (new_smi->handlers)
 			new_smi->handlers->cleanup(new_smi->si_sm);
 		kfree(new_smi->si_sm);
+		new_smi->si_sm = NULL;
 	}
-	if (new_smi->addr_source_cleanup)
+	if (new_smi->addr_source_cleanup) {
 		new_smi->addr_source_cleanup(new_smi);
-	if (new_smi->io_cleanup)
+		new_smi->addr_source_cleanup = NULL;
+	}
+	if (new_smi->io_cleanup) {
 		new_smi->io_cleanup(new_smi);
+		new_smi->io_cleanup = NULL;
+	}
 
-	if (new_smi->dev_registered)
+	if (new_smi->dev_registered) {
 		platform_device_unregister(new_smi->pdev);
-
-	kfree(new_smi);
+		new_smi->dev_registered = 0;
+	}
 
 	mutex_unlock(&smi_infos_lock);
 
@@ -3317,7 +3349,7 @@ module_init(init_ipmi_si);
 
 static void cleanup_one_si(struct smi_info *to_clean)
 {
-	int           rv;
+	int           rv = 0;
 	unsigned long flags;
 
 	if (!to_clean)
@@ -3361,14 +3393,17 @@ static void cleanup_one_si(struct smi_info *to_clean)
 		schedule_timeout_uninterruptible(1);
 	}
 
-	rv = ipmi_unregister_smi(to_clean->intf);
+	if (to_clean->intf)
+		rv = ipmi_unregister_smi(to_clean->intf);
+
 	if (rv) {
 		printk(KERN_ERR
 		       "ipmi_si: Unable to unregister device: errno=%d\n",
 		       rv);
 	}
 
-	to_clean->handlers->cleanup(to_clean->si_sm);
+	if (to_clean->handlers)
+		to_clean->handlers->cleanup(to_clean->si_sm);
 
 	kfree(to_clean->si_sm);
 
-- 
1.6.5.2

[PATCH 2/3] ipmi: Only register one si per bmc

[Matthew Garrett]

Only register one si per bmc. Use any user-provided devices first, followed
by the first device with an irq, followed by the first device discovered.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/char/ipmi/ipmi_si_intf.c |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index d2bdf92..95af023 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -3258,6 +3258,7 @@ static __devinit int init_ipmi_si(void)
 	int  i;
 	char *str;
 	int  rv;
+	struct smi_info *e;
 
 	if (initialized)
 		return 0;
@@ -3292,6 +3293,14 @@ static __devinit int init_ipmi_si(void)
 
 	hardcode_find_bmc();
 
+	/* If the user gave us a device, they presumably want us to use it */
+	mutex_lock(&smi_infos_lock);
+	if (!list_empty(&smi_infos)) {
+		mutex_unlock(&smi_infos_lock);
+		return 0;
+	}
+	mutex_unlock(&smi_infos_lock);
+
 #ifdef CONFIG_DMI
 	dmi_find_bmc();
 #endif
@@ -3315,6 +3324,30 @@ static __devinit int init_ipmi_si(void)
 	of_register_platform_driver(&ipmi_of_platform_driver);
 #endif
 
+	/* Try to register something with interrupts first */
+
+	mutex_lock(&smi_infos_lock);
+	list_for_each_entry(e, &smi_infos, link) {
+		if (e->irq) {
+			if (!try_smi_init(e)) {
+				mutex_unlock(&smi_infos_lock);
+				return 0;
+			}
+		}
+	}
+
+	/* Fall back to the preferred device */
+
+	list_for_each_entry(e, &smi_infos, link) {
+		if (!e->irq) {
+			if (!try_smi_init(e)) {
+				mutex_unlock(&smi_infos_lock);
+				return 0;
+			}
+		}
+	}
+	mutex_unlock(&smi_infos_lock);
+
 	if (si_trydefaults) {
 		mutex_lock(&smi_infos_lock);
 		if (list_empty(&smi_infos)) {
-- 
1.6.5.2

[PATCH 3/3] ipmi: Change device discovery order

[Matthew Garrett]

The ipmi spec provides an ordering for si discovery. Change the driver
to match.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/char/ipmi/ipmi_si_intf.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 95af023..a36cab1 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -3301,23 +3301,24 @@ static __devinit int init_ipmi_si(void)
 	}
 	mutex_unlock(&smi_infos_lock);
 
-#ifdef CONFIG_DMI
-	dmi_find_bmc();
+#ifdef CONFIG_PCI
+	rv = pci_register_driver(&ipmi_pci_driver);
+	if (rv)
+		printk(KERN_ERR
+		       "init_ipmi_si: Unable to register PCI driver: %d\n",
+		       rv);
 #endif
 
 #ifdef CONFIG_ACPI
-	spmi_find_bmc();
+	pnp_register_driver(&ipmi_pnp_driver);
 #endif
+
 #ifdef CONFIG_ACPI
-	pnp_register_driver(&ipmi_pnp_driver);
+	spmi_find_bmc();
 #endif
 
-#ifdef CONFIG_PCI
-	rv = pci_register_driver(&ipmi_pci_driver);
-	if (rv)
-		printk(KERN_ERR
-		       "init_ipmi_si: Unable to register PCI driver: %d\n",
-		       rv);
+#ifdef CONFIG_DMI
+	dmi_find_bmc();
 #endif
 
 #ifdef CONFIG_PPC_OF
-- 
1.6.5.2

Re: [PATCH 2/3] ipmi: Only register one si per bmc

[Corey Minyard]

Matthew Garrett wrote:
> Only register one si per bmc. Use any user-provided devices first, followed
> by the first device with an irq, followed by the first device discovered.
>   
If I understand this correctly, this would really be "Only register one 
si per system".  Unfortunately, there are systems that have more than 
one BMC each with their own interface.  Also, I believe the driver would 
not function between the previous patch and this patch, which isn't the 
best.

The spec is rather fuzzy about the whole subject of multiple BMCs and 
interfaces, but reality is that there are systems that will be broken 
with this.

-corey

> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> ---
>  drivers/char/ipmi/ipmi_si_intf.c |   33 +++++++++++++++++++++++++++++++++
>  1 files changed, 33 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index d2bdf92..95af023 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -3258,6 +3258,7 @@ static __devinit int init_ipmi_si(void)
>  	int  i;
>  	char *str;
>  	int  rv;
> +	struct smi_info *e;
>  
>  	if (initialized)
>  		return 0;
> @@ -3292,6 +3293,14 @@ static __devinit int init_ipmi_si(void)
>  
>  	hardcode_find_bmc();
>  
> +	/* If the user gave us a device, they presumably want us to use it */
> +	mutex_lock(&smi_infos_lock);
> +	if (!list_empty(&smi_infos)) {
> +		mutex_unlock(&smi_infos_lock);
> +		return 0;
> +	}
> +	mutex_unlock(&smi_infos_lock);
> +
>  #ifdef CONFIG_DMI
>  	dmi_find_bmc();
>  #endif
> @@ -3315,6 +3324,30 @@ static __devinit int init_ipmi_si(void)
>  	of_register_platform_driver(&ipmi_of_platform_driver);
>  #endif
>  
> +	/* Try to register something with interrupts first */
> +
> +	mutex_lock(&smi_infos_lock);
> +	list_for_each_entry(e, &smi_infos, link) {
> +		if (e->irq) {
> +			if (!try_smi_init(e)) {
> +				mutex_unlock(&smi_infos_lock);
> +				return 0;
> +			}
> +		}
> +	}
> +
> +	/* Fall back to the preferred device */
> +
> +	list_for_each_entry(e, &smi_infos, link) {
> +		if (!e->irq) {
> +			if (!try_smi_init(e)) {
> +				mutex_unlock(&smi_infos_lock);
> +				return 0;
> +			}
> +		}
> +	}
> +	mutex_unlock(&smi_infos_lock);
> +
>  	if (si_trydefaults) {
>  		mutex_lock(&smi_infos_lock);
>  		if (list_empty(&smi_infos)) {
>   

Re: [PATCH 2/3] ipmi: Only register one si per bmc

[Matthew Garrett]

On Wed, Apr 21, 2010 at 01:09:45PM -0500, Corey Minyard wrote:

> If I understand this correctly, this would really be "Only register one  
> si per system".  Unfortunately, there are systems that have more than  
> one BMC each with their own interface.

The spec explicitly says that while a system may have multiple BMCs, 
only one BMC may respond to GetDeviceID (6.11 of the 2.0 spec). Is the 
real world irritatingly incompatible with this?

> Also, I believe the driver would not function between the previous 
> patch and this patch, which isn't the best.

That's true. I can fix that up fairly easily.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH 2/3] ipmi: Only register one si per bmc

[Corey Minyard]

Matthew Garrett wrote:
> On Wed, Apr 21, 2010 at 01:09:45PM -0500, Corey Minyard wrote:
>
>   
>> If I understand this correctly, this would really be "Only register one  
>> si per system".  Unfortunately, there are systems that have more than  
>> one BMC each with their own interface.
>>     
>
> The spec explicitly says that while a system may have multiple BMCs, 
> only one BMC may respond to GetDeviceID (6.11 of the 2.0 spec). Is the 
> real world irritatingly incompatible with this?
>   
That section is quite misleading.  There may be management controllers 
that are not BMCs, and they may have system interfaces.  There may only 
be on BMC in a system, though, per the spec.  That really has more to do 
with event handling and the main SDR repository, though.

However, IBM makes some systems that can plug together for scalability.  
Each individual system has a BMC, and when you plug them together into 
an SMP system, all the BMCs are still there.  At least that's how I 
understand it.  I'm not sure if the other BMCs become satellite MCs in 
that case, which would be legit, sort of.  So I guess the answer to you 
question would be: "Yes, the world is not compatible with the spec".

-corey

Re: [PATCH 2/3] ipmi: Only register one si per bmc

[Matthew Garrett]

On Wed, Apr 21, 2010 at 03:03:21PM -0500, Corey Minyard wrote:

> However, IBM makes some systems that can plug together for scalability.   
> Each individual system has a BMC, and when you plug them together into  
> an SMP system, all the BMCs are still there.  At least that's how I  
> understand it.  I'm not sure if the other BMCs become satellite MCs in  
> that case, which would be legit, sort of.  So I guess the answer to you  
> question would be: "Yes, the world is not compatible with the spec".

Hm. Ok. In that case we seem to be in a difficult position - we're not 
supposed to register multiple SIs per BMC, but we're also not supposed 
to send anything other than GetDeviceID to an SI before we decide to use 
it. The options here would seem to be to disambiguate by either 
registering all SIs of a type (on the assumption that if we have 
multiple BMCs, they'll all present their SIs in the same way, and we 
won't have multiple SIs of the same type if we don't have multiple 
BMCs), or taking a risk on that bit of the spec by getting the GUID for 
each SI.

-- 
Matthew Garrett | mjg59@srcf.ucam.org