>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 <[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
@@ -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 [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]
