[PATCH] Try 3, Fix release function in IPMI device model

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



I didn't get any comments on this, so I assume it is correct.
I fixed the spelling of Arjan's name (profuse apologies) in
this version, but otherwise it is the same.

I'm sorry to cause so much trouble with this.

-Corey



Arjan van de Ven pointed out that the changeover to the driver model
in the IPMI driver had some bugs in it dealing with the release
function and cleanup.  Then Russell King pointed out that you can't
put release functions in the same module as the unregistering code.

This patch fixes those problems and also adds a semaphore around the
BMC functions and converts the BMC counter to a kref, which I meant to
do earlier, but didn't write down :(.

Signed-off-by: Corey Minyard <[email protected]>

Index: linux-2.6.16/drivers/char/ipmi/ipmi_msghandler.c
===================================================================
--- linux-2.6.16.orig/drivers/char/ipmi/ipmi_msghandler.c
+++ linux-2.6.16/drivers/char/ipmi/ipmi_msghandler.c
@@ -163,11 +163,12 @@ struct ipmi_proc_entry
 
 struct bmc_device
 {
-	struct platform_device dev;
+	struct platform_device *dev;
 	struct ipmi_device_id  id;
 	unsigned char          guid[16];
 	int                    guid_set;
-	int                    interfaces;
+
+	struct kref	       refcount;
 
 	/* bmc device attributes */
 	struct device_attribute device_id_attr;
@@ -181,7 +182,6 @@ struct bmc_device
 	struct device_attribute guid_attr;
 	struct device_attribute aux_firmware_rev_attr;
 };
-#define to_bmc_device(device) container_of(device, struct bmc_device, dev)
 
 #define IPMI_IPMB_NUM_SEQ	64
 #define IPMI_MAX_CHANNELS       16
@@ -357,6 +357,7 @@ static struct device_driver ipmidriver =
 	.name = "ipmi",
 	.bus = &platform_bus_type
 };
+static DECLARE_MUTEX(ipmidriver_sem);
 
 #define MAX_IPMI_INTERFACES 4
 static ipmi_smi_t ipmi_interfaces[MAX_IPMI_INTERFACES];
@@ -1758,9 +1759,7 @@ static void remove_proc_entries(ipmi_smi
 static int __find_bmc_guid(struct device *dev, void *data)
 {
 	unsigned char *id = (unsigned char *) data;
-	struct bmc_device *bmc;
-
-	bmc = to_bmc_device(to_platform_device(dev));
+	struct bmc_device *bmc = dev_get_drvdata(dev);
 	return memcmp(bmc->guid, id, 16) == 0;
 }
 
@@ -1771,7 +1770,7 @@ struct bmc_device * ipmi_find_bmc_guid(s
 
 	dev = driver_find_device(drv, NULL, guid, __find_bmc_guid);
 	if (dev)
-		return to_bmc_device(to_platform_device(dev));
+		return dev_get_drvdata(dev);
 	else
 		return NULL;
 }
@@ -1784,9 +1783,8 @@ struct prod_dev_id {
 static int __find_bmc_prod_dev_id(struct device *dev, void *data)
 {
 	struct prod_dev_id *id = (struct prod_dev_id *) data;
-	struct bmc_device *bmc;
+	struct bmc_device *bmc = dev_get_drvdata(dev);
 
-	bmc = to_bmc_device(to_platform_device(dev));
 	return (bmc->id.product_id == id->product_id
 		&& bmc->id.product_id == id->product_id
 		&& bmc->id.device_id == id->device_id);
@@ -1804,23 +1802,17 @@ static struct bmc_device *ipmi_find_bmc_
 
 	dev = driver_find_device(drv, NULL, &id, __find_bmc_prod_dev_id);
 	if (dev)
-		return to_bmc_device(to_platform_device(dev));
+		return dev_get_drvdata(dev);
 	else
 		return NULL;
 }
 
-static void ipmi_bmc_release(struct device *dev)
-{
-	printk(KERN_DEBUG "ipmi_bmc release\n");
-}
-
 static ssize_t device_id_show(struct device *dev,
 			      struct device_attribute *attr,
 			      char *buf)
 {
-	struct bmc_device *bmc;
+	struct bmc_device *bmc = dev_get_drvdata(dev);
 
-	bmc = to_bmc_device(to_platform_device(dev));
 	return snprintf(buf, 10, "%u\n", bmc->id.device_id);
 }
 
@@ -1828,9 +1820,8 @@ static ssize_t provides_dev_sdrs_show(st
 				      struct device_attribute *attr,
 				      char *buf)
 {
-	struct bmc_device *bmc;
+	struct bmc_device *bmc = dev_get_drvdata(dev);
 
-	bmc = to_bmc_device(to_platform_device(dev));
 	return snprintf(buf, 10, "%u\n",
 			bmc->id.device_revision && 0x80 >> 7);
 }
@@ -1838,9 +1829,8 @@ static ssize_t provides_dev_sdrs_show(st
 static ssize_t revision_show(struct device *dev, struct device_attribute *attr,
 			     char *buf)
 {
-	struct bmc_device *bmc;
+	struct bmc_device *bmc = dev_get_drvdata(dev);
 
-	bmc = to_bmc_device(to_platform_device(dev));
 	return snprintf(buf, 20, "%u\n",
 			bmc->id.device_revision && 0x0F);
 }
@@ -1849,9 +1839,8 @@ static ssize_t firmware_rev_show(struct 
 				 struct device_attribute *attr,
 				 char *buf)
 {
-	struct bmc_device *bmc;
+	struct bmc_device *bmc = dev_get_drvdata(dev);
 
-	bmc = to_bmc_device(to_platform_device(dev));
 	return snprintf(buf, 20, "%u.%x\n", bmc->id.firmware_revision_1,
 			bmc->id.firmware_revision_2);
 }
@@ -1860,9 +1849,8 @@ static ssize_t ipmi_version_show(struct 
 				 struct device_attribute *attr,
 				 char *buf)
 {
-	struct bmc_device *bmc;
+	struct bmc_device *bmc = dev_get_drvdata(dev);
 
-	bmc = to_bmc_device(to_platform_device(dev));
 	return snprintf(buf, 20, "%u.%u\n",
 			ipmi_version_major(&bmc->id),
 			ipmi_version_minor(&bmc->id));
@@ -1872,9 +1860,8 @@ static ssize_t add_dev_support_show(stru
 				    struct device_attribute *attr,
 				    char *buf)
 {
-	struct bmc_device *bmc;
+	struct bmc_device *bmc = dev_get_drvdata(dev);
 
-	bmc = to_bmc_device(to_platform_device(dev));
 	return snprintf(buf, 10, "0x%02x\n",
 			bmc->id.additional_device_support);
 }
@@ -1883,9 +1870,8 @@ static ssize_t manufacturer_id_show(stru
 				    struct device_attribute *attr,
 				    char *buf)
 {
-	struct bmc_device *bmc;
+	struct bmc_device *bmc = dev_get_drvdata(dev);
 
-	bmc = to_bmc_device(to_platform_device(dev));
 	return snprintf(buf, 20, "0x%6.6x\n", bmc->id.manufacturer_id);
 }
 
@@ -1893,9 +1879,8 @@ static ssize_t product_id_show(struct de
 			       struct device_attribute *attr,
 			       char *buf)
 {
-	struct bmc_device *bmc;
+	struct bmc_device *bmc = dev_get_drvdata(dev);
 
-	bmc = to_bmc_device(to_platform_device(dev));
 	return snprintf(buf, 10, "0x%4.4x\n", bmc->id.product_id);
 }
 
@@ -1903,9 +1888,8 @@ static ssize_t aux_firmware_rev_show(str
 				     struct device_attribute *attr,
 				     char *buf)
 {
-	struct bmc_device *bmc;
+	struct bmc_device *bmc = dev_get_drvdata(dev);
 
-	bmc = to_bmc_device(to_platform_device(dev));
 	return snprintf(buf, 21, "0x%02x 0x%02x 0x%02x 0x%02x\n",
 			bmc->id.aux_firmware_revision[3],
 			bmc->id.aux_firmware_revision[2],
@@ -1916,50 +1900,60 @@ static ssize_t aux_firmware_rev_show(str
 static ssize_t guid_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
-	struct bmc_device *bmc;
+	struct bmc_device *bmc = dev_get_drvdata(dev);
 
-	bmc = to_bmc_device(to_platform_device(dev));
 	return snprintf(buf, 100, "%Lx%Lx\n",
 			(long long) bmc->guid[0],
 			(long long) bmc->guid[8]);
 }
 
+static void
+cleanup_bmc_device(struct kref *ref)
+{
+	struct bmc_device *bmc;
+
+	bmc = container_of(ref, struct bmc_device, refcount);
+
+	device_remove_file(&bmc->dev->dev,
+			   &bmc->device_id_attr);
+	device_remove_file(&bmc->dev->dev,
+			   &bmc->provides_dev_sdrs_attr);
+	device_remove_file(&bmc->dev->dev,
+			   &bmc->revision_attr);
+	device_remove_file(&bmc->dev->dev,
+			   &bmc->firmware_rev_attr);
+	device_remove_file(&bmc->dev->dev,
+			   &bmc->version_attr);
+	device_remove_file(&bmc->dev->dev,
+			   &bmc->add_dev_support_attr);
+	device_remove_file(&bmc->dev->dev,
+			   &bmc->manufacturer_id_attr);
+	device_remove_file(&bmc->dev->dev,
+			   &bmc->product_id_attr);
+	if (bmc->id.aux_firmware_revision_set)
+		device_remove_file(&bmc->dev->dev,
+				   &bmc->aux_firmware_rev_attr);
+	if (bmc->guid_set)
+		device_remove_file(&bmc->dev->dev,
+				   &bmc->guid_attr);
+	platform_device_unregister(bmc->dev);
+	kfree(bmc);
+}
+
 static void ipmi_bmc_unregister(ipmi_smi_t intf)
 {
 	struct bmc_device *bmc = intf->bmc;
 
 	sysfs_remove_link(&intf->si_dev->kobj, "bmc");
 	if (intf->my_dev_name) {
-		sysfs_remove_link(&bmc->dev.dev.kobj, intf->my_dev_name);
+		sysfs_remove_link(&bmc->dev->dev.kobj, intf->my_dev_name);
 		kfree(intf->my_dev_name);
 		intf->my_dev_name = NULL;
 	}
 
-	bmc->interfaces--;
-	if (bmc->interfaces == 0) {
-		device_remove_file(&bmc->dev.dev,
-				&bmc->device_id_attr);
-		device_remove_file(&bmc->dev.dev,
-				&bmc->provides_dev_sdrs_attr);
-		device_remove_file(&bmc->dev.dev,
-				&bmc->revision_attr);
-		device_remove_file(&bmc->dev.dev,
-				&bmc->firmware_rev_attr);
-		device_remove_file(&bmc->dev.dev,
-				&bmc->version_attr);
-		device_remove_file(&bmc->dev.dev,
-				&bmc->add_dev_support_attr);
-		device_remove_file(&bmc->dev.dev,
-				&bmc->manufacturer_id_attr);
-		device_remove_file(&bmc->dev.dev,
-				&bmc->product_id_attr);
-		device_remove_file(&bmc->dev.dev,
-				&bmc->aux_firmware_rev_attr);
-		if (bmc->guid_set)
-			device_remove_file(&bmc->dev.dev,
-					   &bmc->guid_attr);
-		platform_device_unregister(&bmc->dev);
-	}
+	down(&ipmidriver_sem);
+	kref_put(&bmc->refcount, cleanup_bmc_device);
+	up(&ipmidriver_sem);
 }
 
 static int ipmi_bmc_register(ipmi_smi_t intf)
@@ -1970,6 +1964,8 @@ static int ipmi_bmc_register(ipmi_smi_t 
 	int               size;
 	char              dummy[1];
 
+	down(&ipmidriver_sem);
+
 	/*
 	 * Try to find if there is an bmc_device struct
 	 * representing the interfaced BMC already
@@ -1990,7 +1986,8 @@ static int ipmi_bmc_register(ipmi_smi_t 
 		intf->bmc = old_bmc;
 		bmc = old_bmc;
 
-		bmc->interfaces++;
+		kref_get(&bmc->refcount);
+		up(&ipmidriver_sem);
 
 		printk(KERN_INFO
 		       "ipmi: interfacing existing BMC (man_id: 0x%6.6x,"
@@ -1999,18 +1996,27 @@ static int ipmi_bmc_register(ipmi_smi_t 
 		       bmc->id.product_id,
 		       bmc->id.device_id);
 	} else {
-		bmc->dev.name = "ipmi_bmc";
-		bmc->dev.id = bmc->id.device_id;
-		bmc->dev.dev.driver = &ipmidriver;
-		bmc->dev.dev.release = ipmi_bmc_release;
-		bmc->interfaces = 1;
+		bmc->dev = platform_device_alloc("ipmi_bmc",
+						 bmc->id.device_id);
+		if (! bmc->dev) {
+			printk(KERN_ERR
+			       "ipmi_msghandler:"
+			       " Unable to allocate platform device\n");
+			return -ENOMEM;
+		}
+		bmc->dev->dev.driver = &ipmidriver;
+		dev_set_drvdata(&bmc->dev->dev, bmc);
+		kref_init(&bmc->refcount);
 
-		rv = platform_device_register(&bmc->dev);
+		rv = platform_device_register(bmc->dev);
+		up(&ipmidriver_sem);
 		if (rv) {
 			printk(KERN_ERR
 			       "ipmi_msghandler:"
 			       " Unable to register bmc device: %d\n",
 			       rv);
+			/* Don't go to out_err, you can only do that if
+			   the device is registered already. */
 			return rv;
 		}
 
@@ -2066,27 +2072,27 @@ static int ipmi_bmc_register(ipmi_smi_t 
 		bmc->aux_firmware_rev_attr.attr.mode = S_IRUGO;
 		bmc->aux_firmware_rev_attr.show = aux_firmware_rev_show;
 
-		device_create_file(&bmc->dev.dev,
+		device_create_file(&bmc->dev->dev,
 				   &bmc->device_id_attr);
-		device_create_file(&bmc->dev.dev,
+		device_create_file(&bmc->dev->dev,
 				   &bmc->provides_dev_sdrs_attr);
-		device_create_file(&bmc->dev.dev,
+		device_create_file(&bmc->dev->dev,
 				   &bmc->revision_attr);
-		device_create_file(&bmc->dev.dev,
+		device_create_file(&bmc->dev->dev,
 				   &bmc->firmware_rev_attr);
-		device_create_file(&bmc->dev.dev,
+		device_create_file(&bmc->dev->dev,
 				   &bmc->version_attr);
-		device_create_file(&bmc->dev.dev,
+		device_create_file(&bmc->dev->dev,
 				   &bmc->add_dev_support_attr);
-		device_create_file(&bmc->dev.dev,
+		device_create_file(&bmc->dev->dev,
 				   &bmc->manufacturer_id_attr);
-		device_create_file(&bmc->dev.dev,
+		device_create_file(&bmc->dev->dev,
 				   &bmc->product_id_attr);
 		if (bmc->id.aux_firmware_revision_set)
-			device_create_file(&bmc->dev.dev,
+			device_create_file(&bmc->dev->dev,
 					   &bmc->aux_firmware_rev_attr);
 		if (bmc->guid_set)
-			device_create_file(&bmc->dev.dev,
+			device_create_file(&bmc->dev->dev,
 					   &bmc->guid_attr);
 
 		printk(KERN_INFO
@@ -2102,7 +2108,7 @@ static int ipmi_bmc_register(ipmi_smi_t 
 	 * and back.
 	 */
 	rv = sysfs_create_link(&intf->si_dev->kobj,
-			       &bmc->dev.dev.kobj, "bmc");
+			       &bmc->dev->dev.kobj, "bmc");
 	if (rv) {
 		printk(KERN_ERR
 		       "ipmi_msghandler: Unable to create bmc symlink: %d\n",
@@ -2121,7 +2127,7 @@ static int ipmi_bmc_register(ipmi_smi_t 
 	}
 	snprintf(intf->my_dev_name, size+1, "ipmi%d", intf->intf_num);
 
-	rv = sysfs_create_link(&bmc->dev.dev.kobj, &intf->si_dev->kobj,
+	rv = sysfs_create_link(&bmc->dev->dev.kobj, &intf->si_dev->kobj,
 			       intf->my_dev_name);
 	if (rv) {
 		kfree(intf->my_dev_name);
Index: linux-2.6.16/drivers/char/ipmi/ipmi_si_intf.c
===================================================================
--- linux-2.6.16.orig/drivers/char/ipmi/ipmi_si_intf.c
+++ linux-2.6.16/drivers/char/ipmi/ipmi_si_intf.c
@@ -206,7 +206,10 @@ struct smi_info
 
 	/* Driver model stuff. */
 	struct device *dev;
-	struct platform_device idev;
+	struct platform_device *pdev;
+
+	 /* True if we allocated the device, false if it came from
+	  * someplace else (like PCI). */
 	int dev_registered;
 
 	/* Slave address, could be reported from DMI. */
@@ -1161,7 +1164,6 @@ static void port_cleanup(struct smi_info
 
 		release_region (addr, mapsize);
 	}
-	kfree(info);
 }
 
 static int port_setup(struct smi_info *info)
@@ -1270,7 +1272,6 @@ static void mem_cleanup(struct smi_info 
 
 		release_mem_region(addr, mapsize);
 	}
-	kfree(info);
 }
 
 static int mem_setup(struct smi_info *info)
@@ -2233,11 +2234,6 @@ static int is_new_interface(struct smi_i
 	return 1;
 }
 
-static void si_release(struct device *dev)
-{
-	printk(KERN_DEBUG "ipmi_si release\n");
-}
-
 static int try_smi_init(struct smi_info *new_smi)
 {
 	int rv;
@@ -2367,12 +2363,18 @@ static int try_smi_init(struct smi_info 
 	if (!new_smi->dev) {
 		/* If we don't already have a device from something
 		 * else (like PCI), then register a new one. */
-		new_smi->idev.name = "ipmi_si";
-		new_smi->idev.id = new_smi->intf_num;
-		new_smi->idev.dev.driver = &ipmi_driver;
-		new_smi->idev.dev.release = si_release;
+		new_smi->pdev = platform_device_alloc("ipmi_si",
+						      new_smi->intf_num);
+		if (rv) {
+			printk(KERN_ERR
+			       "ipmi_si_intf:"
+			       " Unable to allocate platform device\n");
+			goto out_err_stop_timer;
+		}
+		new_smi->dev = &new_smi->pdev->dev;
+		new_smi->dev->driver = &ipmi_driver;
 
-		rv = platform_device_register(&new_smi->idev);
+		rv = platform_device_register(new_smi->pdev);
 		if (rv) {
 			printk(KERN_ERR
 			       "ipmi_si_intf:"
@@ -2381,7 +2383,6 @@ static int try_smi_init(struct smi_info 
 			       rv);
 			goto out_err_stop_timer;
 		}
-		new_smi->dev = &new_smi->idev.dev;
 		new_smi->dev_registered = 1;
 	}
 
@@ -2395,7 +2396,7 @@ static int try_smi_init(struct smi_info 
 		printk(KERN_ERR
 		       "ipmi_si: Unable to register device: error %d\n",
 		       rv);
-		goto out_err_unreg_dev;
+		goto out_err_stop_timer;
 	}
 
 	rv = ipmi_smi_add_proc_entry(new_smi->intf, "type",
@@ -2405,7 +2406,7 @@ static int try_smi_init(struct smi_info 
 		printk(KERN_ERR
 		       "ipmi_si: Unable to create proc entry: %d\n",
 		       rv);
-		goto out_err_unreg_dev;
+		goto out_err_stop_timer;
 	}
 
 	rv = ipmi_smi_add_proc_entry(new_smi->intf, "si_stats",
@@ -2415,7 +2416,7 @@ static int try_smi_init(struct smi_info 
 		printk(KERN_ERR
 		       "ipmi_si: Unable to create proc entry: %d\n",
 		       rv);
-		goto out_err_unreg_dev;
+		goto out_err_stop_timer;
 	}
 
 	list_add_tail(&new_smi->link, &smi_infos);
@@ -2426,10 +2427,6 @@ static int try_smi_init(struct smi_info 
 
 	return 0;
 
- out_err_unreg_dev:
-	if (new_smi->dev_registered)
-		platform_device_unregister(&new_smi->idev);
-
  out_err_stop_timer:
 	atomic_inc(&new_smi->stop_operation);
 	wait_for_timer_and_thread(new_smi);
@@ -2456,6 +2453,11 @@ static int try_smi_init(struct smi_info 
 	if (new_smi->io_cleanup)
 		new_smi->io_cleanup(new_smi);
 
+	if (new_smi->dev_registered)
+		platform_device_unregister(new_smi->pdev);
+
+	kfree(new_smi);
+
 	up(&smi_infos_lock);
 
 	return rv;
@@ -2583,9 +2585,6 @@ static void __devexit cleanup_one_si(str
 		       rv);
 	}
 
-	if (to_clean->dev_registered)
-		platform_device_unregister(&to_clean->idev);
-
 	to_clean->handlers->cleanup(to_clean->si_sm);
 
 	kfree(to_clean->si_sm);
@@ -2594,6 +2593,11 @@ static void __devexit cleanup_one_si(str
 		to_clean->addr_source_cleanup(to_clean);
 	if (to_clean->io_cleanup)
 		to_clean->io_cleanup(to_clean);
+
+	if (to_clean->dev_registered)
+		platform_device_unregister(to_clean->pdev);
+
+	kfree(to_clean);
 }
 
 static __exit void cleanup_ipmi_si(void)
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux