[openib-general] [PATCH] core+mthca: questions and proposal: kill ib_(de)alloc_device
Michael S. Tsirkin
mst at mellanox.co.il
Fri Sep 30 01:05:10 PDT 2005
Guys, looking with Leonid Keller at device.c raised several questions
with regard to what ib_alloc_device/ib_dealloc_device do:
1. Why is ib_device_register_sysfs called from ib_register_device,
but ib_device_unregister_sysfs from ib_dealloc_device?
2. Who is supposed to set reg_state back to IB_DEV_UNINITIALIZED?
Without it ib_dealloc_device does not seem to free the device
structure. Is this a memory leak?
3. ib_alloc_device does not set reg_state, it seems to rely on the fact that
IB_DEV_UNINITIALIZED = 0. Is that intentional?
4. For ib_alloc_device/ib_dealloc_device to work properly, it seems that
the device structure must have ib_device as the first member.
Is this limitation documented anywhere?
5. Why do we need reg_state in the device, at all?
I thought we can trust providers to call register/unregister
in proper order?
What do you say we simply let providers allocate the structure?
Can you really go wrong reducing the line count by more than 50 :) ?
# diffstat patches/alloc.patch
core/device.c | 51 +-----------------------------------------------
hw/mthca/mthca_main.c | 8 ++++---
include/rdma/ib_verbs.h | 9 --------
3 files changed, 7 insertions(+), 61 deletions(-)
---
Kill ib_alloc_device/ib_dealloc_device and the reg_state field.
Also solves what looks like a memory leak.
Index: linux-2.6.13/drivers/infiniband/core/device.c
===================================================================
--- linux-2.6.13.orig/drivers/infiniband/core/device.c 2005-09-30 12:31:22.000000000 +0300
+++ linux-2.6.13/drivers/infiniband/core/device.c 2005-09-30 12:31:39.000000000 +0300
@@ -149,51 +149,6 @@ static int alloc_name(char *name)
return 0;
}
-/**
- * ib_alloc_device - allocate an IB device struct
- * @size:size of structure to allocate
- *
- * Low-level drivers should use ib_alloc_device() to allocate &struct
- * ib_device. @size is the size of the structure to be allocated,
- * including any private data used by the low-level driver.
- * ib_dealloc_device() must be used to free structures allocated with
- * ib_alloc_device().
- */
-struct ib_device *ib_alloc_device(size_t size)
-{
- void *dev;
-
- BUG_ON(size < sizeof (struct ib_device));
-
- dev = kmalloc(size, GFP_KERNEL);
- if (!dev)
- return NULL;
-
- memset(dev, 0, size);
-
- return dev;
-}
-EXPORT_SYMBOL(ib_alloc_device);
-
-/**
- * ib_dealloc_device - free an IB device struct
- * @device:structure to free
- *
- * Free a structure allocated with ib_alloc_device().
- */
-void ib_dealloc_device(struct ib_device *device)
-{
- if (device->reg_state == IB_DEV_UNINITIALIZED) {
- kfree(device);
- return;
- }
-
- BUG_ON(device->reg_state != IB_DEV_UNREGISTERED);
-
- ib_device_unregister_sysfs(device);
-}
-EXPORT_SYMBOL(ib_dealloc_device);
-
static int add_client_context(struct ib_device *device, struct ib_client *client)
{
struct ib_client_data *context;
@@ -256,8 +211,6 @@ int ib_register_device(struct ib_device
list_add_tail(&device->core_list, &device_list);
- device->reg_state = IB_DEV_REGISTERED;
-
{
struct ib_client *client;
@@ -292,14 +245,14 @@ void ib_unregister_device(struct ib_devi
list_del(&device->core_list);
+ ib_device_unregister_sysfs(device);
+
up(&device_sem);
spin_lock_irqsave(&device->client_data_lock, flags);
list_for_each_entry_safe(context, tmp, &device->client_data_list, list)
kfree(context);
spin_unlock_irqrestore(&device->client_data_lock, flags);
-
- device->reg_state = IB_DEV_UNREGISTERED;
}
EXPORT_SYMBOL(ib_unregister_device);
Index: linux-2.6.13/drivers/infiniband/hw/mthca/mthca_main.c
===================================================================
--- linux-2.6.13.orig/drivers/infiniband/hw/mthca/mthca_main.c 2005-09-30 12:31:22.000000000 +0300
+++ linux-2.6.13/drivers/infiniband/hw/mthca/mthca_main.c 2005-09-30 12:31:39.000000000 +0300
@@ -1003,7 +1003,7 @@ static int __devinit mthca_init_one(stru
}
}
- mdev = (struct mthca_dev *) ib_alloc_device(sizeof *mdev);
+ mdev = kmalloc(sizeof *mdev, GFP_KERNEL);
if (!mdev) {
dev_err(&pdev->dev, "Device struct alloc failed, "
"aborting.\n");
@@ -1011,6 +1011,8 @@ static int __devinit mthca_init_one(stru
goto err_free_res;
}
+ memset(mdev, 0, sizeof *mdev);
+
mdev->pdev = pdev;
if (ddr_hidden)
@@ -1106,7 +1108,7 @@ err_free_dev:
if (mdev->mthca_flags & MTHCA_FLAG_MSI)
pci_disable_msi(pdev);
- ib_dealloc_device(&mdev->ib_dev);
+ kfree(mdev);
err_free_res:
mthca_release_regions(pdev, ddr_hidden);
@@ -1154,7 +1156,7 @@ static void __devexit mthca_remove_one(s
if (mdev->mthca_flags & MTHCA_FLAG_MSI)
pci_disable_msi(pdev);
- ib_dealloc_device(&mdev->ib_dev);
+ kfree(mdev);
mthca_release_regions(pdev, mdev->mthca_flags &
MTHCA_FLAG_DDR_HIDDEN);
pci_disable_device(pdev);
Index: linux-2.6.13/drivers/infiniband/include/rdma/ib_verbs.h
===================================================================
--- linux-2.6.13.orig/drivers/infiniband/include/rdma/ib_verbs.h 2005-09-30 12:31:30.000000000 +0300
+++ linux-2.6.13/drivers/infiniband/include/rdma/ib_verbs.h 2005-09-30 12:31:52.000000000 +0300
@@ -945,12 +945,6 @@ struct ib_device {
struct kobject ports_parent;
struct list_head port_list;
- enum {
- IB_DEV_UNINITIALIZED,
- IB_DEV_REGISTERED,
- IB_DEV_UNREGISTERED
- } reg_state;
-
int uverbs_abi_ver;
u8 node_type;
@@ -965,9 +959,6 @@ struct ib_client {
struct list_head list;
};
-struct ib_device *ib_alloc_device(size_t size);
-void ib_dealloc_device(struct ib_device *device);
-
int ib_register_device (struct ib_device *device);
void ib_unregister_device(struct ib_device *device);
--
MST
More information about the general
mailing list