* [PATCH] Try 3, Fix release function in IPMI device model
@ 2006-03-22 23:19 Corey Minyard
2006-03-23 8:02 ` Arjan van de Ven
0 siblings, 1 reply; 2+ messages in thread
From: Corey Minyard @ 2006-03-22 23:19 UTC (permalink / raw)
To: Linux Kernel
Cc: Andrew Morton, Yani Ioannou, greg, dtor_core, rmk+lkml, Arjan van de Ven
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 <minyard@acm.org>
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)
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [PATCH] Try 3, Fix release function in IPMI device model
2006-03-22 23:19 [PATCH] Try 3, Fix release function in IPMI device model Corey Minyard
@ 2006-03-23 8:02 ` Arjan van de Ven
0 siblings, 0 replies; 2+ messages in thread
From: Arjan van de Ven @ 2006-03-23 8:02 UTC (permalink / raw)
To: minyard
Cc: Linux Kernel, Andrew Morton, Yani Ioannou, greg, dtor_core, rmk+lkml
On Wed, 2006-03-22 at 17:19 -0600, Corey Minyard wrote:
> 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 :(.
please use a mutex not a semaphore... semaphores for mutex code are
deprecated
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2006-03-23 8:02 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-03-22 23:19 [PATCH] Try 3, Fix release function in IPMI device model Corey Minyard
2006-03-23 8:02 ` Arjan van de Ven
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®