[BUG] ipmi: NULL device printing in check_set_rcv_irq()

[BUG] ipmi: NULL device printing in check_set_rcv_irq()

[Corentin Labbe]

Hello

On one of my servers I got this line in dmesg
(NULL device *): The BMC does not support setting the recv irq bit, compensating, but the BMC needs to be fixed.

This printing is done via a dev_warn(smi_info->dev,) in drivers/char/ipmi/ipmi_si_intf.c:check_clr_rcv_irq() but smi_info->dev is set only latter in try_smi_init().

I do not know which way is better to fix it, either move the code block which register the device before check_for_broken_irqs(), or perhaps replace dev_warn by pr_warn.

Regards
Corentin Labbe

[PATCH] Move platform device creation earlier in the initialization

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Some logs are printed out early using smi->dev, but on a platform device
that is not created until later.  So move the creation of that device
structure earlier in the sequence so it can be used for printing.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---

Corentin, can you try the attached patch?  It moves the platform device
allocation earlier, but I don't really want to actually add the platform
device until later.

 drivers/char/ipmi/ipmi_si_intf.c | 46 +++++++++++++++++++++++++++-------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index cb45108..751c281 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -3502,6 +3502,7 @@ static int try_smi_init(struct smi_info *new_smi)
 {
 	int rv = 0;
 	int i;
+	char *init_name = NULL;
 
 	printk(KERN_INFO PFX "Trying %s-specified %s state"
 	       " machine at %s address 0x%lx, slave address 0x%x,"
@@ -3531,6 +3532,26 @@ static int try_smi_init(struct smi_info *new_smi)
 		goto out_err;
 	}
 
+	/* Do this early so it's available for logs. */
+	if (!new_smi->dev) {
+		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d", 0);
+
+		/*
+		 * If we don't already have a device from something
+		 * else (like PCI), then register a new one.
+		 */
+		new_smi->pdev = platform_device_alloc("ipmi_si",
+						      new_smi->intf_num);
+		if (!new_smi->pdev) {
+			pr_err(PFX "Unable to allocate platform device\n");
+			goto out_err;
+		}
+		new_smi->dev = &new_smi->pdev->dev;
+		new_smi->dev->driver = &ipmi_driver.driver;
+		/* Nulled by device_add() */
+		new_smi->dev->init_name = init_name;
+	}
+
 	/* Allocate the state machine's data and initialize it. */
 	new_smi->si_sm = kmalloc(new_smi->handlers->size(), GFP_KERNEL);
 	if (!new_smi->si_sm) {
@@ -3604,21 +3625,7 @@ static int try_smi_init(struct smi_info *new_smi)
 		atomic_set(&new_smi->req_events, 1);
 	}
 
-	if (!new_smi->dev) {
-		/*
-		 * If we don't already have a device from something
-		 * else (like PCI), then register a new one.
-		 */
-		new_smi->pdev = platform_device_alloc("ipmi_si",
-						      new_smi->intf_num);
-		if (!new_smi->pdev) {
-			printk(KERN_ERR PFX
-			       "Unable to allocate platform device\n");
-			goto out_err;
-		}
-		new_smi->dev = &new_smi->pdev->dev;
-		new_smi->dev->driver = &ipmi_driver.driver;
-
+	if (new_smi->pdev) {
 		rv = platform_device_add(new_smi->pdev);
 		if (rv) {
 			printk(KERN_ERR PFX
@@ -3668,6 +3675,9 @@ static int try_smi_init(struct smi_info *new_smi)
 	dev_info(new_smi->dev, "IPMI %s interface initialized\n",
 		 si_to_str[new_smi->si_type]);
 
+	WARN_ON(new_smi->dev->init_name != NULL);
+	kfree(init_name);
+
 	return 0;
 
 out_err_stop_timer:
@@ -3712,8 +3722,14 @@ static int try_smi_init(struct smi_info *new_smi)
 	if (new_smi->dev_registered) {
 		platform_device_unregister(new_smi->pdev);
 		new_smi->dev_registered = false;
+		new_smi->pdev = NULL;
+	} else if (new_smi->pdev) {
+		platform_device_put(new_smi->pdev);
+		new_smi->pdev = NULL;
 	}
 
+	kfree(init_name);
+
 	return rv;
 }
 
-- 
2.7.4

Re: [PATCH] Move platform device creation earlier in the initialization

[Corentin Labbe]

On Mon, Nov 07, 2016 at 12:35:10PM -0600, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Some logs are printed out early using smi->dev, but on a platform device
> that is not created until later.  So move the creation of that device
> structure earlier in the sequence so it can be used for printing.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---
> 
> Corentin, can you try the attached patch?  It moves the platform device
> allocation earlier, but I don't really want to actually add the platform
> device until later.
> 

It works.

After patching:
[30786.391668] IPMI System Interface driver.
[30786.391706] ipmi_si: probing via SMBIOS
[30786.391709] ipmi_si: SMBIOS: io 0xca8 regsize 1 spacing 4 irq 10
[30786.391712] ipmi_si: Adding SMBIOS-specified kcs state machine
[30786.391717] ipmi_si: Trying SMBIOS-specified kcs state machine at i/o address 0xca8, slave address 0x20, irq 10
[30786.582985] ipmi_si ipmi_si.0: The BMC does not support setting the recv irq bit, compensating, but the BMC needs to be fixed.
[30786.615091] ipmi_si ipmi_si.0: Using irq 10
[30786.645423] ipmi_si ipmi_si.0: Found new BMC (man_id: 0x0002a2, prod_id: 0x0100, dev_id: 0x20)
[30786.645481] ipmi_si ipmi_si.0: IPMI kcs interface initialized

Retrying with original ipmi_si:
[30813.525306] IPMI System Interface driver.
[30813.525363] ipmi_si: probing via SMBIOS
[30813.525368] ipmi_si: SMBIOS: io 0xca8 regsize 1 spacing 4 irq 10
[30813.525372] ipmi_si: Adding SMBIOS-specified kcs state machine
[30813.525379] ipmi_si: Trying SMBIOS-specified kcs state machine at i/o address 0xca8, slave address 0x20, irq 10
[30813.708519] (NULL device *): The BMC does not support setting the recv irq bit, compensating, but the BMC needs to be fixed.
[30813.740591] ipmi_si ipmi_si.0: Using irq 10
[30813.773238] ipmi_si ipmi_si.0: Found new BMC (man_id: 0x0002a2, prod_id: 0x0100, dev_id: 0x20)
[30813.773286] ipmi_si ipmi_si.0: IPMI kcs interface initialized

So Tested-by: Corentin Labbe <clabbe.montjoie@gmail.com>

Thanks

>  drivers/char/ipmi/ipmi_si_intf.c | 46 +++++++++++++++++++++++++++-------------
>  1 file changed, 31 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index cb45108..751c281 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -3502,6 +3502,7 @@ static int try_smi_init(struct smi_info *new_smi)
>  {
>  	int rv = 0;
>  	int i;
> +	char *init_name = NULL;
>  
>  	printk(KERN_INFO PFX "Trying %s-specified %s state"
>  	       " machine at %s address 0x%lx, slave address 0x%x,"
> @@ -3531,6 +3532,26 @@ static int try_smi_init(struct smi_info *new_smi)
>  		goto out_err;
>  	}
>  
> +	/* Do this early so it's available for logs. */
> +	if (!new_smi->dev) {
> +		init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d", 0);
> +
> +		/*
> +		 * If we don't already have a device from something
> +		 * else (like PCI), then register a new one.
> +		 */
> +		new_smi->pdev = platform_device_alloc("ipmi_si",
> +						      new_smi->intf_num);
> +		if (!new_smi->pdev) {
> +			pr_err(PFX "Unable to allocate platform device\n");
> +			goto out_err;
> +		}
> +		new_smi->dev = &new_smi->pdev->dev;
> +		new_smi->dev->driver = &ipmi_driver.driver;
> +		/* Nulled by device_add() */
> +		new_smi->dev->init_name = init_name;
> +	}
> +
>  	/* Allocate the state machine's data and initialize it. */
>  	new_smi->si_sm = kmalloc(new_smi->handlers->size(), GFP_KERNEL);
>  	if (!new_smi->si_sm) {
> @@ -3604,21 +3625,7 @@ static int try_smi_init(struct smi_info *new_smi)
>  		atomic_set(&new_smi->req_events, 1);
>  	}
>  
> -	if (!new_smi->dev) {
> -		/*
> -		 * If we don't already have a device from something
> -		 * else (like PCI), then register a new one.
> -		 */
> -		new_smi->pdev = platform_device_alloc("ipmi_si",
> -						      new_smi->intf_num);
> -		if (!new_smi->pdev) {
> -			printk(KERN_ERR PFX
> -			       "Unable to allocate platform device\n");
> -			goto out_err;
> -		}
> -		new_smi->dev = &new_smi->pdev->dev;
> -		new_smi->dev->driver = &ipmi_driver.driver;
> -
> +	if (new_smi->pdev) {
>  		rv = platform_device_add(new_smi->pdev);
>  		if (rv) {
>  			printk(KERN_ERR PFX
> @@ -3668,6 +3675,9 @@ static int try_smi_init(struct smi_info *new_smi)
>  	dev_info(new_smi->dev, "IPMI %s interface initialized\n",
>  		 si_to_str[new_smi->si_type]);
>  
> +	WARN_ON(new_smi->dev->init_name != NULL);
> +	kfree(init_name);
> +
>  	return 0;
>  
>  out_err_stop_timer:
> @@ -3712,8 +3722,14 @@ static int try_smi_init(struct smi_info *new_smi)
>  	if (new_smi->dev_registered) {
>  		platform_device_unregister(new_smi->pdev);
>  		new_smi->dev_registered = false;
> +		new_smi->pdev = NULL;
> +	} else if (new_smi->pdev) {
> +		platform_device_put(new_smi->pdev);
> +		new_smi->pdev = NULL;
>  	}
>  
> +	kfree(init_name);
> +
>  	return rv;
>  }
>  
> -- 
> 2.7.4
>