[PATCH] ipmi: Fix memleaking for add_smi when duplicating happen

[PATCH] ipmi: Fix memleaking for add_smi when duplicating happen

[Yinghai Lu]

also print out the reg spacing and size for spmi and smbios.
so bios guys could have idea to make them consistent.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/char/ipmi/ipmi_si_intf.c |   47 ++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 8 deletions(-)

Index: linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
===================================================================
--- linux-2.6.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
@@ -1804,9 +1804,12 @@ static int hotmod_handler(const char *va
 				info->irq_setup = std_irq_setup;
 			info->slave_addr = ipmb;
 
-			if (!add_smi(info))
+			if (!add_smi(info)) {
 				if (try_smi_init(info))
 					cleanup_one_si(info);
+			} else {
+				kfree(info);
+			}
 		} else {
 			/* remove */
 			struct smi_info *e, *tmp_e;
@@ -1890,9 +1893,12 @@ static __devinit void hardcode_find_bmc(
 			info->irq_setup = std_irq_setup;
 		info->slave_addr = slave_addrs[i];
 
-		if (!add_smi(info))
+		if (!add_smi(info)) {
 			if (try_smi_init(info))
 				cleanup_one_si(info);
+		} else {
+			kfree(info);
+		}
 	}
 }
 
@@ -2088,7 +2094,13 @@ static __devinit int try_init_spmi(struc
 	}
 	info->io.addr_data = spmi->addr.address;
 
-	add_smi(info);
+	pr_info("ipmi_si: SPMI: %s %#lx regsize %d spacing %d irq %d\n",
+		 (info->io.addr_type == IPMI_IO_ADDR_SPACE) ? "io" : "mem",
+		 info->io.addr_data, info->io.regsize, info->io.regspacing,
+		 info->irq);
+
+	if (add_smi(info))
+		kfree(info);
 
 	return 0;
 }
@@ -2204,7 +2216,10 @@ static int __devinit ipmi_pnp_probe(stru
 		 res, info->io.regsize, info->io.regspacing,
 		 info->irq);
 
-	return add_smi(info);
+	if (add_smi(info))
+		goto err_free;
+
+	return 0;
 
 err_free:
 	kfree(info);
@@ -2362,7 +2377,13 @@ static __devinit void try_init_dmi(struc
 	if (info->irq)
 		info->irq_setup = std_irq_setup;
 
-	add_smi(info);
+	pr_info("ipmi_si: SMBIOS: %s %#lx regsize %d spacing %d irq %d\n",
+		 (info->io.addr_type == IPMI_IO_ADDR_SPACE) ? "io" : "mem",
+		 info->io.addr_data, info->io.regsize, info->io.regspacing,
+		 info->irq);
+
+	if (add_smi(info))
+		kfree(info);
 }
 
 static void __devinit dmi_find_bmc(void)
@@ -2468,7 +2489,10 @@ static int __devinit ipmi_pci_probe(stru
 		&pdev->resource[0], info->io.regsize, info->io.regspacing,
 		info->irq);
 
-	return add_smi(info);
+	if (add_smi(info))
+		kfree(info);
+
+	return 0;
 }
 
 static void __devexit ipmi_pci_remove(struct pci_dev *pdev)
@@ -2581,7 +2605,12 @@ static int __devinit ipmi_of_probe(struc
 
 	dev_set_drvdata(&dev->dev, info);
 
-	return add_smi(info);
+	if (add_smi(info)) {
+		kfree(info);
+		return -EBUSY;
+	}
+
+	return 0;
 }
 
 static int __devexit ipmi_of_remove(struct of_device *dev)
@@ -3018,6 +3047,8 @@ static __devinit void default_find_bmc(v
 				info->io.addr_data);
 			} else
 				cleanup_one_si(info);
+		} else {
+			kfree(info);
 		}
 	}
 }
@@ -3045,7 +3076,7 @@ static int add_smi(struct smi_info *new_
 			si_to_str[new_smi->si_type]);
 	mutex_lock(&smi_infos_lock);
 	if (!is_new_interface(new_smi)) {
-		printk(KERN_CONT PFX "duplicate interface\n");
+		printk(KERN_CONT " duplicate interface\n");
 		rv = -EBUSY;
 		goto out_err;
 	}

Re: [PATCH] ipmi: Fix memleaking for add_smi when duplicating happen

[Corey Minyard]

Please run this through checkpatch, as it has coding style violations.

Also, this patch appears to fix bugs in addition to adding the print.  
Can we have a separate patch for that?

I'm also not clear on the reason for this.  I believe all this 
information is already available in /proc/ipmi/<if#>/params.  I don't 
think there is a strong reason to print it to the log.

-corey

On 07/26/2010 08:25 PM, Yinghai Lu wrote:
> also print out the reg spacing and size for spmi and smbios.
> so bios guys could have idea to make them consistent.
>
> Signed-off-by: Yinghai Lu<yinghai@kernel.org>
>
> ---
>   drivers/char/ipmi/ipmi_si_intf.c |   47 ++++++++++++++++++++++++++++++++-------
>   1 file changed, 39 insertions(+), 8 deletions(-)
>
> Index: linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
> ===================================================================
> --- linux-2.6.orig/drivers/char/ipmi/ipmi_si_intf.c
> +++ linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
> @@ -1804,9 +1804,12 @@ static int hotmod_handler(const char *va
>   				info->irq_setup = std_irq_setup;
>   			info->slave_addr = ipmb;
>
> -			if (!add_smi(info))
> +			if (!add_smi(info)) {
>   				if (try_smi_init(info))
>   					cleanup_one_si(info);
> +			} else {
> +				kfree(info);
> +			}
>   		} else {
>   			/* remove */
>   			struct smi_info *e, *tmp_e;
> @@ -1890,9 +1893,12 @@ static __devinit void hardcode_find_bmc(
>   			info->irq_setup = std_irq_setup;
>   		info->slave_addr = slave_addrs[i];
>
> -		if (!add_smi(info))
> +		if (!add_smi(info)) {
>   			if (try_smi_init(info))
>   				cleanup_one_si(info);
> +		} else {
> +			kfree(info);
> +		}
>   	}
>   }
>
> @@ -2088,7 +2094,13 @@ static __devinit int try_init_spmi(struc
>   	}
>   	info->io.addr_data = spmi->addr.address;
>
> -	add_smi(info);
> +	pr_info("ipmi_si: SPMI: %s %#lx regsize %d spacing %d irq %d\n",
> +		 (info->io.addr_type == IPMI_IO_ADDR_SPACE) ? "io" : "mem",
> +		 info->io.addr_data, info->io.regsize, info->io.regspacing,
> +		 info->irq);
> +
> +	if (add_smi(info))
> +		kfree(info);
>
>   	return 0;
>   }
> @@ -2204,7 +2216,10 @@ static int __devinit ipmi_pnp_probe(stru
>   		 res, info->io.regsize, info->io.regspacing,
>   		 info->irq);
>
> -	return add_smi(info);
> +	if (add_smi(info))
> +		goto err_free;
> +
> +	return 0;
>
>   err_free:
>   	kfree(info);
> @@ -2362,7 +2377,13 @@ static __devinit void try_init_dmi(struc
>   	if (info->irq)
>   		info->irq_setup = std_irq_setup;
>
> -	add_smi(info);
> +	pr_info("ipmi_si: SMBIOS: %s %#lx regsize %d spacing %d irq %d\n",
> +		 (info->io.addr_type == IPMI_IO_ADDR_SPACE) ? "io" : "mem",
> +		 info->io.addr_data, info->io.regsize, info->io.regspacing,
> +		 info->irq);
> +
> +	if (add_smi(info))
> +		kfree(info);
>   }
>
>   static void __devinit dmi_find_bmc(void)
> @@ -2468,7 +2489,10 @@ static int __devinit ipmi_pci_probe(stru
>   		&pdev->resource[0], info->io.regsize, info->io.regspacing,
>   		info->irq);
>
> -	return add_smi(info);
> +	if (add_smi(info))
> +		kfree(info);
> +
> +	return 0;
>   }
>
>   static void __devexit ipmi_pci_remove(struct pci_dev *pdev)
> @@ -2581,7 +2605,12 @@ static int __devinit ipmi_of_probe(struc
>
>   	dev_set_drvdata(&dev->dev, info);
>
> -	return add_smi(info);
> +	if (add_smi(info)) {
> +		kfree(info);
> +		return -EBUSY;
> +	}
> +
> +	return 0;
>   }
>
>   static int __devexit ipmi_of_remove(struct of_device *dev)
> @@ -3018,6 +3047,8 @@ static __devinit void default_find_bmc(v
>   				info->io.addr_data);
>   			} else
>   				cleanup_one_si(info);
> +		} else {
> +			kfree(info);
>   		}
>   	}
>   }
> @@ -3045,7 +3076,7 @@ static int add_smi(struct smi_info *new_
>   			si_to_str[new_smi->si_type]);
>   	mutex_lock(&smi_infos_lock);
>   	if (!is_new_interface(new_smi)) {
> -		printk(KERN_CONT PFX "duplicate interface\n");
> +		printk(KERN_CONT " duplicate interface\n");
>   		rv = -EBUSY;
>   		goto out_err;
>   	}
>    

Re: [PATCH] ipmi: Fix memleaking for add_smi when duplicating happen

[Yinghai Lu]

On 07/26/2010 07:05 PM, Corey Minyard wrote:
> Please run this through checkpatch, as it has coding style violations.

yhlu@linux-siqj:~/xx/xx/kernel/tip/linux-2.6> ./scripts/checkpatch.pl patches/ipmi_reg_size.patch
total: 0 errors, 0 warnings, 105 lines checked

patches/ipmi_reg_size.patch has no obvious style problems and is ready for submission.
yhlu@linux-siqj:~/xx/xx/kernel/tip/linux-2.6> 

> 
> Also, this patch appears to fix bugs in addition to adding the print. 
> Can we have a separate patch for that?

in the comment log, i already mentioned that. 

will separate it to twol

> 
> I'm also not clear on the reason for this.  I believe all this
> information is already available in /proc/ipmi/<if#>/params.  I don't
> think there is a strong reason to print it to the log.

then why there is printing for ACPI path and pci path?

Yinghai

Re: [PATCH] ipmi: Fix memleaking for add_smi when duplicating happen

[Corey Minyard]

On 07/26/2010 11:41 PM, Yinghai Lu wrote:
> On 07/26/2010 07:05 PM, Corey Minyard wrote:
>    
>> Please run this through checkpatch, as it has coding style violations.
>>      
> yhlu@linux-siqj:~/xx/xx/kernel/tip/linux-2.6>  ./scripts/checkpatch.pl patches/ipmi_reg_size.patch
> total: 0 errors, 0 warnings, 105 lines checked
>
> patches/ipmi_reg_size.patch has no obvious style problems and is ready for submission.
> yhlu@linux-siqj:~/xx/xx/kernel/tip/linux-2.6>
>    
Sorry, you are right.

>    
>> Also, this patch appears to fix bugs in addition to adding the print.
>> Can we have a separate patch for that?
>>      
> in the comment log, i already mentioned that.
>
> will separate it to twol
>    
Ok, thanks

>    
>> I'm also not clear on the reason for this.  I believe all this
>> information is already available in /proc/ipmi/<if#>/params.  I don't
>> think there is a strong reason to print it to the log.
>>      
> then why there is printing for ACPI path and pci path?
>    
Well, I'm not sure on this.  You are right, it is printed for those 
paths and not for DMI or SPMI cases.  Printing too much information is 
not generally a good idea, but this may be useful.  I guess to make it 
consistent it would be best to add this.

-corey

[PATCH 1/2] ipmi: Fix memleaking for add_smi when duplicating happen

[Yinghai Lu]

need free the temp info struct when we have duplicated ones

-v2: seperate printing change to another patch

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/char/ipmi/ipmi_si_intf.c |   35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
===================================================================
--- linux-2.6.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
@@ -1804,9 +1804,12 @@ static int hotmod_handler(const char *va
 				info->irq_setup = std_irq_setup;
 			info->slave_addr = ipmb;
 
-			if (!add_smi(info))
+			if (!add_smi(info)) {
 				if (try_smi_init(info))
 					cleanup_one_si(info);
+			} else {
+				kfree(info);
+			}
 		} else {
 			/* remove */
 			struct smi_info *e, *tmp_e;
@@ -1890,9 +1893,12 @@ static __devinit void hardcode_find_bmc(
 			info->irq_setup = std_irq_setup;
 		info->slave_addr = slave_addrs[i];
 
-		if (!add_smi(info))
+		if (!add_smi(info)) {
 			if (try_smi_init(info))
 				cleanup_one_si(info);
+		} else {
+			kfree(info);
+		}
 	}
 }
 
@@ -2088,7 +2094,8 @@ static __devinit int try_init_spmi(struc
 	}
 	info->io.addr_data = spmi->addr.address;
 
-	add_smi(info);
+	if (add_smi(info))
+		kfree(info);
 
 	return 0;
 }
@@ -2204,7 +2211,10 @@ static int __devinit ipmi_pnp_probe(stru
 		 res, info->io.regsize, info->io.regspacing,
 		 info->irq);
 
-	return add_smi(info);
+	if (add_smi(info))
+		goto err_free;
+
+	return 0;
 
 err_free:
 	kfree(info);
@@ -2362,7 +2372,8 @@ static __devinit void try_init_dmi(struc
 	if (info->irq)
 		info->irq_setup = std_irq_setup;
 
-	add_smi(info);
+	if (add_smi(info))
+		kfree(info);
 }
 
 static void __devinit dmi_find_bmc(void)
@@ -2468,7 +2479,10 @@ static int __devinit ipmi_pci_probe(stru
 		&pdev->resource[0], info->io.regsize, info->io.regspacing,
 		info->irq);
 
-	return add_smi(info);
+	if (add_smi(info))
+		kfree(info);
+
+	return 0;
 }
 
 static void __devexit ipmi_pci_remove(struct pci_dev *pdev)
@@ -2581,7 +2595,12 @@ static int __devinit ipmi_of_probe(struc
 
 	dev_set_drvdata(&dev->dev, info);
 
-	return add_smi(info);
+	if (add_smi(info)) {
+		kfree(info);
+		return -EBUSY;
+	}
+
+	return 0;
 }
 
 static int __devexit ipmi_of_remove(struct of_device *dev)
@@ -3018,6 +3037,8 @@ static __devinit void default_find_bmc(v
 				info->io.addr_data);
 			} else
 				cleanup_one_si(info);
+		} else {
+			kfree(info);
 		}
 	}
 }

Re: [PATCH 1/2] ipmi: Fix memleaking for add_smi when duplicating happen

[Myron Stowe]

On Mon, 2010-07-26 at 21:57 -0700, Yinghai Lu wrote:
> need free the temp info struct when we have duplicated ones

Nice catch on these.  Could you improve the patch description please.

Myron
> 
> -v2: seperate printing change to another patch
> 
> Signed-off-by: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  drivers/char/ipmi/ipmi_si_intf.c |   35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
> ===================================================================
> --- linux-2.6.orig/drivers/char/ipmi/ipmi_si_intf.c
> +++ linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
> @@ -1804,9 +1804,12 @@ static int hotmod_handler(const char *va
>  				info->irq_setup = std_irq_setup;
>  			info->slave_addr = ipmb;
>  
> -			if (!add_smi(info))
> +			if (!add_smi(info)) {
>  				if (try_smi_init(info))
>  					cleanup_one_si(info);
> +			} else {
> +				kfree(info);
> +			}
>  		} else {
>  			/* remove */
>  			struct smi_info *e, *tmp_e;
> @@ -1890,9 +1893,12 @@ static __devinit void hardcode_find_bmc(
>  			info->irq_setup = std_irq_setup;
>  		info->slave_addr = slave_addrs[i];
>  
> -		if (!add_smi(info))
> +		if (!add_smi(info)) {
>  			if (try_smi_init(info))
>  				cleanup_one_si(info);
> +		} else {
> +			kfree(info);
> +		}
>  	}
>  }
>  
> @@ -2088,7 +2094,8 @@ static __devinit int try_init_spmi(struc
>  	}
>  	info->io.addr_data = spmi->addr.address;
>  
> -	add_smi(info);
> +	if (add_smi(info))
> +		kfree(info);
>  
>  	return 0;
>  }
> @@ -2204,7 +2211,10 @@ static int __devinit ipmi_pnp_probe(stru
>  		 res, info->io.regsize, info->io.regspacing,
>  		 info->irq);
>  
> -	return add_smi(info);
> +	if (add_smi(info))
> +		goto err_free;
> +
> +	return 0;
>  
>  err_free:
>  	kfree(info);
> @@ -2362,7 +2372,8 @@ static __devinit void try_init_dmi(struc
>  	if (info->irq)
>  		info->irq_setup = std_irq_setup;
>  
> -	add_smi(info);
> +	if (add_smi(info))
> +		kfree(info);
>  }
>  
>  static void __devinit dmi_find_bmc(void)
> @@ -2468,7 +2479,10 @@ static int __devinit ipmi_pci_probe(stru
>  		&pdev->resource[0], info->io.regsize, info->io.regspacing,
>  		info->irq);
>  
> -	return add_smi(info);
> +	if (add_smi(info))
> +		kfree(info);
> +
> +	return 0;
>  }
>  
>  static void __devexit ipmi_pci_remove(struct pci_dev *pdev)
> @@ -2581,7 +2595,12 @@ static int __devinit ipmi_of_probe(struc
>  
>  	dev_set_drvdata(&dev->dev, info);
>  
> -	return add_smi(info);
> +	if (add_smi(info)) {
> +		kfree(info);
> +		return -EBUSY;
> +	}
> +
> +	return 0;
>  }
>  
>  static int __devexit ipmi_of_remove(struct of_device *dev)
> @@ -3018,6 +3037,8 @@ static __devinit void default_find_bmc(v
>  				info->io.addr_data);
>  			} else
>  				cleanup_one_si(info);
> +		} else {
> +			kfree(info);
>  		}
>  	}
>  }
> 


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

[PATCH 2/2] ipmi: print info for spmi and smbios path like acpi and pci

[Yinghai Lu]

print out the reg spacing and size for spmi and smbios.
so bios guys could have idea to make them consistent.

also remove extra PFX on duplicating path.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/char/ipmi/ipmi_si_intf.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
===================================================================
--- linux-2.6.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6/drivers/char/ipmi/ipmi_si_intf.c
@@ -2094,6 +2094,11 @@ static __devinit int try_init_spmi(struc
 	}
 	info->io.addr_data = spmi->addr.address;
 
+	pr_info("ipmi_si: SPMI: %s %#lx regsize %d spacing %d irq %d\n",
+		 (info->io.addr_type == IPMI_IO_ADDR_SPACE) ? "io" : "mem",
+		 info->io.addr_data, info->io.regsize, info->io.regspacing,
+		 info->irq);
+
 	if (add_smi(info))
 		kfree(info);
 
@@ -2372,6 +2377,11 @@ static __devinit void try_init_dmi(struc
 	if (info->irq)
 		info->irq_setup = std_irq_setup;
 
+	pr_info("ipmi_si: SMBIOS: %s %#lx regsize %d spacing %d irq %d\n",
+		 (info->io.addr_type == IPMI_IO_ADDR_SPACE) ? "io" : "mem",
+		 info->io.addr_data, info->io.regsize, info->io.regspacing,
+		 info->irq);
+
 	if (add_smi(info))
 		kfree(info);
 }
@@ -3066,7 +3076,7 @@ static int add_smi(struct smi_info *new_
 			si_to_str[new_smi->si_type]);
 	mutex_lock(&smi_infos_lock);
 	if (!is_new_interface(new_smi)) {
-		printk(KERN_CONT PFX "duplicate interface\n");
+		printk(KERN_CONT " duplicate interface\n");
 		rv = -EBUSY;
 		goto out_err;
 	}