From f334b60b43a0927f4ab1187cbdb4582f5227c3b1 Mon Sep 17 00:00:00 2001 From: Venkatesh Pallipadi Date: Tue, 19 Dec 2006 13:01:29 -0800 Subject: kref refcnt and false positives With WARN_ON addition to kobject_init() [ http://kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.19/2.6.19-mm1/dont-use/broken-out/gregkh-driver-kobject-warn.patch ] I started seeing following WARNING on CPU offline followed by online on my x86_64 system. WARNING at lib/kobject.c:172 kobject_init() Call Trace: [] dump_trace+0xaa/0x3ef [] show_trace+0x3a/0x50 [] dump_stack+0x15/0x17 [] kobject_init+0x3f/0x8a [] kobject_register+0x1a/0x3e [] sysdev_register+0x5b/0xf9 [] mce_create_device+0x77/0xf4 [] mce_cpu_callback+0x3a/0xe5 [] notifier_call_chain+0x26/0x3b [] raw_notifier_call_chain+0x9/0xb [] _cpu_up+0xb4/0xdc [] cpu_up+0x2b/0x42 [] store_online+0x4a/0x72 [] sysdev_store+0x24/0x26 [] sysfs_write_file+0xcf/0xfc [] vfs_write+0xae/0x154 [] sys_write+0x47/0x6f [] system_call+0x7e/0x83 DWARF2 unwinder stuck at system_call+0x7e/0x83 Leftover inexact backtrace: This is a false positive as mce.c is unregistering/registering sysfs interfaces cleanly on hotplug. kref_put() and conditional decrement of refcnt seems to be the root cause for this and the patch below resolves the issue for me. Signed-off-by: Venkatesh Pallipadi Signed-off-by: Andrew Morton Signed-off-by: Greg Kroah-Hartman --- lib/kref.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/kref.c b/lib/kref.c index 4a467faf1367..0d07cc31c818 100644 --- a/lib/kref.c +++ b/lib/kref.c @@ -52,12 +52,7 @@ int kref_put(struct kref *kref, void (*release)(struct kref *kref)) WARN_ON(release == NULL); WARN_ON(release == (void (*)(struct kref *))kfree); - /* - * if current count is one, we are the last user and can release object - * right now, avoiding an atomic operation on 'refcount' - */ - if ((atomic_read(&kref->refcount) == 1) || - (atomic_dec_and_test(&kref->refcount))) { + if (atomic_dec_and_test(&kref->refcount)) { release(kref); return 1; } -- cgit v1.2.3 From 542cfce6f36e8c43f71ae9c235b78497f350ae55 Mon Sep 17 00:00:00 2001 From: "Aneesh Kumar K.V" Date: Tue, 19 Dec 2006 13:01:27 -0800 Subject: kobject: kobject_uevent() returns manageable value Since kobject_uevent() function does not return an integer value to indicate if its operation was completed with success or not, it is worth changing it in order to report a proper status (success or error) instead of returning void. [randy.dunlap@oracle.com: Fix inline kobject functions] Cc: Mauricio Lin Signed-off-by: Aneesh Kumar K.V Signed-off-by: Randy Dunlap Signed-off-by: Andrew Morton Signed-off-by: Greg Kroah-Hartman --- include/linux/kobject.h | 11 ++++++----- lib/kobject_uevent.c | 44 ++++++++++++++++++++++++++++++-------------- 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/include/linux/kobject.h b/include/linux/kobject.h index d1c8d28fa92e..76538fcf2c4e 100644 --- a/include/linux/kobject.h +++ b/include/linux/kobject.h @@ -265,8 +265,8 @@ extern int __must_check subsys_create_file(struct subsystem * , struct subsys_attribute *); #if defined(CONFIG_HOTPLUG) -void kobject_uevent(struct kobject *kobj, enum kobject_action action); -void kobject_uevent_env(struct kobject *kobj, enum kobject_action action, +int kobject_uevent(struct kobject *kobj, enum kobject_action action); +int kobject_uevent_env(struct kobject *kobj, enum kobject_action action, char *envp[]); int add_uevent_var(char **envp, int num_envp, int *cur_index, @@ -274,11 +274,12 @@ int add_uevent_var(char **envp, int num_envp, int *cur_index, const char *format, ...) __attribute__((format (printf, 7, 8))); #else -static inline void kobject_uevent(struct kobject *kobj, enum kobject_action action) { } -static inline void kobject_uevent_env(struct kobject *kobj, +static inline int kobject_uevent(struct kobject *kobj, enum kobject_action action) +{ return 0; } +static inline int kobject_uevent_env(struct kobject *kobj, enum kobject_action action, char *envp[]) -{ } +{ return 0; } static inline int add_uevent_var(char **envp, int num_envp, int *cur_index, char *buffer, int buffer_size, int *cur_len, diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c index a1922765ff31..84272ed77f03 100644 --- a/lib/kobject_uevent.c +++ b/lib/kobject_uevent.c @@ -63,8 +63,11 @@ static char *action_to_string(enum kobject_action action) * @action: action that is happening (usually KOBJ_MOVE) * @kobj: struct kobject that the action is happening to * @envp_ext: pointer to environmental data + * + * Returns 0 if kobject_uevent() is completed with success or the + * corresponding error when it fails. */ -void kobject_uevent_env(struct kobject *kobj, enum kobject_action action, +int kobject_uevent_env(struct kobject *kobj, enum kobject_action action, char *envp_ext[]) { char **envp; @@ -79,14 +82,16 @@ void kobject_uevent_env(struct kobject *kobj, enum kobject_action action, u64 seq; char *seq_buff; int i = 0; - int retval; + int retval = 0; int j; pr_debug("%s\n", __FUNCTION__); action_string = action_to_string(action); - if (!action_string) - return; + if (!action_string) { + pr_debug("kobject attempted to send uevent without action_string!\n"); + return -EINVAL; + } /* search the kset we belong to */ top_kobj = kobj; @@ -95,31 +100,39 @@ void kobject_uevent_env(struct kobject *kobj, enum kobject_action action, top_kobj = top_kobj->parent; } while (!top_kobj->kset && top_kobj->parent); } - if (!top_kobj->kset) - return; + if (!top_kobj->kset) { + pr_debug("kobject attempted to send uevent without kset!\n"); + return -EINVAL; + } kset = top_kobj->kset; uevent_ops = kset->uevent_ops; /* skip the event, if the filter returns zero. */ if (uevent_ops && uevent_ops->filter) - if (!uevent_ops->filter(kset, kobj)) - return; + if (!uevent_ops->filter(kset, kobj)) { + pr_debug("kobject filter function caused the event to drop!\n"); + return 0; + } /* environment index */ envp = kzalloc(NUM_ENVP * sizeof (char *), GFP_KERNEL); if (!envp) - return; + return -ENOMEM; /* environment values */ buffer = kmalloc(BUFFER_SIZE, GFP_KERNEL); - if (!buffer) + if (!buffer) { + retval = -ENOMEM; goto exit; + } /* complete object path */ devpath = kobject_get_path(kobj, GFP_KERNEL); - if (!devpath) + if (!devpath) { + retval = -ENOENT; goto exit; + } /* originating subsystem */ if (uevent_ops && uevent_ops->name) @@ -204,7 +217,7 @@ exit: kfree(devpath); kfree(buffer); kfree(envp); - return; + return retval; } EXPORT_SYMBOL_GPL(kobject_uevent_env); @@ -214,10 +227,13 @@ EXPORT_SYMBOL_GPL(kobject_uevent_env); * * @action: action that is happening (usually KOBJ_ADD and KOBJ_REMOVE) * @kobj: struct kobject that the action is happening to + * + * Returns 0 if kobject_uevent() is completed with success or the + * corresponding error when it fails. */ -void kobject_uevent(struct kobject *kobj, enum kobject_action action) +int kobject_uevent(struct kobject *kobj, enum kobject_action action) { - kobject_uevent_env(kobj, action, NULL); + return kobject_uevent_env(kobj, action, NULL); } EXPORT_SYMBOL_GPL(kobject_uevent); -- cgit v1.2.3 From 1f21782e63da81f56401a813a52091ef2703838f Mon Sep 17 00:00:00 2001 From: Adrian Bunk Date: Tue, 19 Dec 2006 13:01:28 -0800 Subject: Driver core: proper prototype for drivers/base/init.c:driver_init() Add a prototype for driver_init() in include/linux/device.h. Also remove a static function of the same name in drivers/acpi/ibm_acpi.c to ibm_acpi_driver_init() to fix the namespace collision. Signed-off-by: Adrian Bunk Acked-by: Henrique de Moraes Holschuh Signed-off-by: Andrew Morton Signed-off-by: Greg Kroah-Hartman --- drivers/acpi/ibm_acpi.c | 4 ++-- include/linux/device.h | 2 ++ init/main.c | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/acpi/ibm_acpi.c b/drivers/acpi/ibm_acpi.c index 003a9876c968..5a8445959f67 100644 --- a/drivers/acpi/ibm_acpi.c +++ b/drivers/acpi/ibm_acpi.c @@ -352,7 +352,7 @@ static char *next_cmd(char **cmds) return start; } -static int driver_init(void) +static int ibm_acpi_driver_init(void) { printk(IBM_INFO "%s v%s\n", IBM_DESC, IBM_VERSION); printk(IBM_INFO "%s\n", IBM_URL); @@ -1605,7 +1605,7 @@ static int fan_write(char *buf) static struct ibm_struct ibms[] = { { .name = "driver", - .init = driver_init, + .init = ibm_acpi_driver_init, .read = driver_read, }, { diff --git a/include/linux/device.h b/include/linux/device.h index 49ab53ce92dc..f44247fe8135 100644 --- a/include/linux/device.h +++ b/include/linux/device.h @@ -433,6 +433,8 @@ static inline int device_is_registered(struct device *dev) return dev->is_registered; } +void driver_init(void); + /* * High level routines for use by the bus drivers */ diff --git a/init/main.c b/init/main.c index e3f0bb20b4dd..2b1cdaab45e6 100644 --- a/init/main.c +++ b/init/main.c @@ -53,6 +53,7 @@ #include #include #include +#include #include #include @@ -94,7 +95,6 @@ extern void pidmap_init(void); extern void prio_tree_init(void); extern void radix_tree_init(void); extern void free_initmem(void); -extern void driver_init(void); extern void prepare_namespace(void); #ifdef CONFIG_ACPI extern void acpi_early_init(void); -- cgit v1.2.3