lkml.org 
[lkml]   [2006]   [Mar]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
Subject[PATCH] Fix release function in IPMI device model
>On Tue, 2006-03-21 at 16:13 -0600, Corey Minyard wrote:
>>+static void ipmi_bmc_release(struct device *dev)
>>+{
>>+ printk(KERN_DEBUG "ipmi_bmc release\n");
>>+}
>eehhhh NO.
>Please read the many comments and documentations about why a release
>function is NOT allowed to be empty. In fact the kernel warned you about
>that, didn't it?

Arjun, is this ok? I'm not 100% sure I understand all the subtleties of
the device model, but I assume all I have to do is keep around the
device memory until the release function is called. Also, the device
might come from somplace else (if it is on the PCI bus, for instance),
so I just free the memory in that case since the memory for the
device structure didn't come from this memory.

And "many comments" might be stretching it. I found one comment in the
device model documentataion directory, nothing in any of the include
files describing the device model. I should have looked more carefully
when converting the patch over from Yani, but something non-obvious and
import like this deserves a comment in both the documentation and include
file by the release function, something like:

/* IMPORTANT: You must define a release function, and the memory for the
device structure *MUST* be kept around until the release function is
called. */

Here's the patch...


Arjun 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. This patch fixes that 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
@@ -168,7 +168,8 @@ struct bmc_device
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;
@@ -352,6 +353,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];
@@ -1800,7 +1802,7 @@ static struct bmc_device *ipmi_find_bmc_

static void ipmi_bmc_release(struct device *dev)
{
- printk(KERN_DEBUG "ipmi_bmc release\n");
+ kfree(to_bmc_device(to_platform_device(dev)));
}

static ssize_t device_id_show(struct device *dev,
@@ -1913,6 +1915,38 @@ static ssize_t guid_show(struct device *
(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);
+}
+
static void ipmi_bmc_unregister(ipmi_smi_t intf)
{
struct bmc_device *bmc = intf->bmc;
@@ -1924,31 +1958,9 @@ static void ipmi_bmc_unregister(ipmi_smi
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)
@@ -1959,6 +1971,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
@@ -1979,7 +1993,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,"
@@ -1992,14 +2007,17 @@ static int ipmi_bmc_register(ipmi_smi_t
bmc->dev.id = bmc->id.device_id;
bmc->dev.dev.driver = &ipmidriver;
bmc->dev.dev.release = ipmi_bmc_release;
- bmc->interfaces = 1;
+ kref_init(&bmc->refcount);

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;
}

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
@@ -231,6 +231,7 @@ struct smi_info

struct list_head link;
};
+#define to_smi_device(device) container_of(device, struct smi_info, idev)

static int try_smi_init(struct smi_info *smi);

@@ -1161,7 +1162,6 @@ static void port_cleanup(struct smi_info

release_region (addr, mapsize);
}
- kfree(info);
}

static int port_setup(struct smi_info *info)
@@ -1270,7 +1270,6 @@ static void mem_cleanup(struct smi_info

release_mem_region(addr, mapsize);
}
- kfree(info);
}

static int mem_setup(struct smi_info *info)
@@ -2235,7 +2234,7 @@ static int is_new_interface(struct smi_i

static void si_release(struct device *dev)
{
- printk(KERN_DEBUG "ipmi_si release\n");
+ kfree(to_smi_device(to_platform_device(dev)));
}

static int try_smi_init(struct smi_info *new_smi)
@@ -2395,7 +2394,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 +2404,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 +2414,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 +2425,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 +2451,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->idev);
+ else
+ kfree(new_smi);
+
up(&smi_infos_lock);

return rv;
@@ -2583,9 +2583,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 +2591,13 @@ 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)
+ /* We registered the device, let the release free it. */
+ platform_device_unregister(&to_clean->idev);
+ else
+ /* The device came from elsewhere, we free now. */
+ 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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
\
 
 \ /
  Last update: 2006-03-22 17:00    [W:0.026 / U:0.688 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site