mce: fix warning messages about static struct mce_device

From: Greg Kroah-Hartman <[email protected]>

When suspending, there was a large list of warnings going something like:

Device 'machinecheck1' does not have a release() function, it is broken and must be fixed

This patch turns the static mce_devices into dynamically allocated, and properly frees them when they are removed from the system. It solves the warning messages on my laptop here.

Reported-by "Srivatsa S. Bhat" <[email protected]>

Reported-by Linus Torvalds <[email protected]>

Cc Kay Sievers <[email protected]>

Cc Tony Luck <[email protected]>

Cc Borislav Petkov <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

---

Linus, this should fix the warnings on your laptop, it does on mine.

Quoted 3 lines · Greg KH

From: Greg Kroah-Hartman <[email protected]>

When suspending, there was a large list of warnings going something like:

Not only suspend, during poweroff we'll also see these annoying warnings as reported here: http://lkml.org/lkml/2012/1/10/504

Quoted 13 lines

Device 'machinecheck1' does not have a release() function, it is broken and must be fixed

This patch turns the static mce_devices into dynamically allocated, and properly frees them when they are removed from the system. It solves the warning messages on my laptop here.

Reported-by "Srivatsa S. Bhat" <[email protected]>

Reported-by Linus Torvalds <[email protected]>

Cc Kay Sievers <[email protected]>

Cc Tony Luck <[email protected]>

Cc Borislav Petkov <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

Quoted 3 lines

---

Linus, this should fix the warnings on your laptop, it does on mine.

Patch tested.

Thanks, this also fixed the warnings for my KVM/Qemu guests.

Linus, do you want to take this directly from the email, or do you want me to put it in a tree to pull from?

Quoted 3 lines · Greg KH

Linus, do you want to take this directly from the email, or do you want me to put it in a tree to pull from?

Do you have anything else pending before I close the merge window? If so, put it in a tree with those.

If not, let me know, and I'll just take the patch as-is.

arch/x86/include/asm/mce.h static inline void enable_p5_mce(void) {} · line 154

Quoted 10 lines · Greg KH

arch/x86/include/asm/mce.h +1 -1

index f35ce43..6aefb14 100644
@@ -151,7 +151,7 @@ static inline void enable_p5_mce(void) {}
 
 void mce_setup(struct mce *m);
 void mce_log(struct mce *m);
-DECLARE_PER_CPU(struct device, mce_device);
+extern struct device *mce_device[CONFIG_NR_CPUS];

Minor nit, i don't think we have any other such [CONFIG_NR_CPUS] pattern in the kernel.

This should be something like:

  DECLARE_PER_CPU(struct device *, mce_device);

Or the pointer should be attached to the CPU info structure.

Quoted 17 lines · Ingo Molnar

arch/x86/include/asm/mce.h static inline void enable_p5_mce(void) {} · line 154

Quoted 10 lines · Greg KH

arch/x86/include/asm/mce.h +1 -1

index f35ce43..6aefb14 100644
@@ -151,7 +151,7 @@ static inline void enable_p5_mce(void) {}
 
 void mce_setup(struct mce *m);
 void mce_log(struct mce *m);
-DECLARE_PER_CPU(struct device, mce_device);
+extern struct device *mce_device[CONFIG_NR_CPUS];

Minor nit, i don't think we have any other such [CONFIG_NR_CPUS] pattern in the kernel.

Quoted 3 lines

This should be something like:

  DECLARE_PER_CPU(struct device *, mce_device);

That is what we used to have, but with just a static struct device. We really don't need this to be in the per-cpu area, a flat array should be just fine, why can't we use the CONFIG_NR_CPUS value? Should we use something else?

Quoted 1 line

Or the pointer should be attached to the CPU info structure.

Ok, I have no objection to that, do you want me to make a patch doing that, now that this is already in Linus's tree?

Quoted 17 lines · Ingo Molnar

arch/x86/include/asm/mce.h static inline void enable_p5_mce(void) {} · line 154

Quoted 10 lines · Greg KH

arch/x86/include/asm/mce.h +1 -1

index f35ce43..6aefb14 100644
@@ -151,7 +151,7 @@ static inline void enable_p5_mce(void) {}
 
 void mce_setup(struct mce *m);
 void mce_log(struct mce *m);
-DECLARE_PER_CPU(struct device, mce_device);
+extern struct device *mce_device[CONFIG_NR_CPUS];

Minor nit, i don't think we have any other such [CONFIG_NR_CPUS] pattern in the kernel.

Quoted 3 lines

This should be something like:

  DECLARE_PER_CPU(struct device *, mce_device);

That is what we used to have, but with just a static struct device. [...]

[...] We really don't need this to be in the per-cpu area, a flat array should be just fine, why can't we use the CONFIG_NR_CPUS value? Should we use something else?

Quoted 1 line

Or the pointer should be attached to the CPU info structure.

Ok, I have no objection to that, do you want me to make a patch doing that, now that this is already in Linus's tree?

Quoted 22 lines · Greg KH

Quoted 20 lines · Ingo Molnar

arch/x86/include/asm/mce.h static inline void enable_p5_mce(void) {} · line 154

Quoted 10 lines · Greg KH

arch/x86/include/asm/mce.h +1 -1

index f35ce43..6aefb14 100644
@@ -151,7 +151,7 @@ static inline void enable_p5_mce(void) {}
 
 void mce_setup(struct mce *m);
 void mce_log(struct mce *m);
-DECLARE_PER_CPU(struct device, mce_device);
+extern struct device *mce_device[CONFIG_NR_CPUS];

Minor nit, i don't think we have any other such [CONFIG_NR_CPUS] pattern in the kernel.

This should be something like:

  DECLARE_PER_CPU(struct device *, mce_device);

Quoted 2 lines

That is what we used to have, but with just a static struct device. [...]

Which was fine in itself for a per CPU data structure - wouldnt the warning be fixed by memset()-ing before registering the device or such, if device registry absolutely needs a pre-zeroed buffer?

I still think there must be some bug/assumption lurking in the device layer - do you require all device allocations to be one via zalloc()? Seems like a weird and unrobust requirement.

I don't object to the quick fix that gets rid of the warnings, but that quick fix came at the price of leaving the real bug unfixed and at the price of introducing a new ugliness ;-)

Quoted 3 lines

[...] We really don't need this to be in the per-cpu area, a flat array should be just fine, why can't we use the CONFIG_NR_CPUS value? Should we use something else?

By that argument we don't really need PER_CPU() areas to begin with, a flat [CONFIG_NR_CPUS] array is just fine, right?

Amongst other things we use PER_CPU to have an array of just 2 elements on a dual core system, even if it boots a CONFIG_NR_CPUS=512 distro kernel. That saves RAM, and with higher CONFIG_NR_CPUS values it adds up quickly.

Quoted 2 lines

Quoted 1 line

Or the pointer should be attached to the CPU info structure.

Quoted 2 lines

Ok, I have no objection to that, do you want me to make a patch doing that, now that this is already in Linus's tree?

Would be nice if you could do that or some other equivalent solution, i'd really not like to see the [CONFIG_NR_CPUS] pattern to spread in the kernel, we spent a lot of time getting rid of such uses ;-)

Quoted 9 lines

Quoted 9 lines

Quoted 6 lines

Minor nit, i don't think we have any other such [CONFIG_NR_CPUS] pattern in the kernel.

This should be something like:

  DECLARE_PER_CPU(struct device *, mce_device);

That is what we used to have, but with just a static struct device. [...]

Quoted 4 lines

Which was fine in itself for a per CPU data structure - wouldnt the warning be fixed by memset()-ing before registering the device or such, if device registry absolutely needs a pre-zeroed buffer?

It was already fixed that way, but the problem is that you can not have statically allocated 'struct device' objects in the system. That's what my add-on patch fixed, also resolving the syslog messages saying there was no release function for the device as well.

Quoted 3 lines

I still think there must be some bug/assumption lurking in the device layer - do you require all device allocations to be one via zalloc()? Seems like a weird and unrobust requirement.

Yes, that's always been the requirement.

Quoted 10 lines

Amongst other things we use PER_CPU to have an array of just 2 elements on a dual core system, even if it boots a CONFIG_NR_CPUS=512 distro kernel. That saves RAM, and with higher CONFIG_NR_CPUS values it adds up quickly.

Quoted 4 lines

Quoted 1 line

Or the pointer should be attached to the CPU info structure.

Ok, I have no objection to that, do you want me to make a patch doing that, now that this is already in Linus's tree?

Quoted 4 lines

Would be nice if you could do that or some other equivalent solution, i'd really not like to see the [CONFIG_NR_CPUS] pattern to spread in the kernel, we spent a lot of time getting rid of such uses ;-)

Ok, I'll work on resolving this.

It was already fixed that way, but the problem is that you can not have statically allocated 'struct device' objects in the system.

There's an additional requirement: Device structures may not be reused. Not even if the caller clears all the fields to 0 in between. That was the real bug in the original code -- and adding a dummy release routine wouldn't fix it.

Quoted 2 lines · Greg

It was already fixed that way, but the problem is that you can not have statically allocated 'struct device' objects in the system.

Quoted 4 lines · and then Alan

There's an additional requirement: Device structures may not be reused. Not even if the caller clears all the fields to 0 in between. That was the real bug in the original code -- and adding a dummy release routine wouldn't fix it.

There seems to be some curious logic happening here which I don't understand at all. How can the code that deals with 'struct device' tell whether it was statically declared or dynamically allocated? Why would it care?

What happens if we play by the rules using a dynamic structure and do

device_register() + device_create_file(dev)
...
device_remove_file(dev) + device_unregister()

then later come back to re-add this and by pure random fluke kzalloc() gives us back the exact same block of memory that we used for dev before?

By Alan's logic we are screwed - we are re-using the same device structure (unless kfree() + kzalloc() does some magic pixie dust thing so that this same block of memory is now not the same device structure we had before, even though it has the same address).

In summary - I can totally buy the argument that you must start with a zeroed struct device (and that it is just fine that device_unregister() doesn't waste cpu cycles cleaning up the structure just in case someone will re-use it, because that isn't going to be the common case).

I just don't understand the magical difference between a static structure that has been memset() to all zero, and a dynamic block returned from kzalloc().

Quoted 2 lines · Greg

It was already fixed that way, but the problem is that you can not have statically allocated 'struct device' objects in the system.

Quoted 4 lines · and then Alan

There's an additional requirement: Device structures may not be reused. Not even if the caller clears all the fields to 0 in between. That was the real bug in the original code -- and adding a dummy release routine wouldn't fix it.

There seems to be some curious logic happening here which I don't understand at all. How can the code that deals with 'struct device' tell whether it was statically declared or dynamically allocated? Why would it care?

What happens if we play by the rules using a dynamic structure and do

device_register() + device_create_file(dev)
...
device_remove_file(dev) + device_unregister()

then later come back to re-add this and by pure random fluke kzalloc() gives us back the exact same block of memory that we used for dev before?

By Alan's logic we are screwed - we are re-using the same device structure (unless kfree() + kzalloc() does some magic pixie dust thing so that this same block of memory is now not the same device structure we had before, even though it has the same address).

In summary - I can totally buy the argument that you must start with a zeroed struct device (and that it is just fine that device_unregister() doesn't waste cpu cycles cleaning up the structure just in case someone will re-use it, because that isn't going to be the common case).

I just don't understand the magical difference between a static structure that has been memset() to all zero, and a dynamic block returned from kzalloc().

There's nothing special about the driver model code in this respect. The same restriction applies wherever object lifetimes are controlled by reference counting.

This way, the new object is full functional, does not conflict with the older one, and also all the users of the old memory are still fine and can cleanup a lazy as they need without much synchronization.

Quoted 3 lines · Alan Stern

There's nothing special about the driver model code in this respect. The same restriction applies wherever object lifetimes are controlled by reference counting.

Right. But it might not be obvious what 's the background here:

An allocated device object(memory) usually represents an actual device(hardware). The object can have N users. Every of the users is required to take a reference to the object, which pins the object's memory as long as any of the N users might need to access it.

In a hotplug world, we deal with device-removal. On disconnect, we usually just orphan the object, we remove it from visibility, disconnect the device <-> object relation.

All of the N users with a reference can still access the memory, they just do not talk to a real device anymore. The invalidated/orphaned state is communicated otherwise by locks and flags in the device object. Only after all of the N users left the object alone, the memory of the orphan if free'd.

If in the time-window between disconnecting the object from the device and freeing the orphaned object's memory, the same device comes back, we allocate a new object which is associated with the device. It usually has the same name and same properties as the original one.

This way, the new object is full functional, does not conflict with the older one, and also all the users of the old memory are still fine and can cleanup a lazy as they need without much synchronization.

Now, all that might not apply to machinecheck, and it might be that machinecheck is fully able to handle all that just fine with the statically allocated same memory -- allocating new device memory on hotplug is still the model that should always be preferred over any other, if possible.

It's usually the simplest safest and most flexible for anything that can come and go at any time, and memory which might be in used by other running code.

Quoted 3 lines

There's nothing special about the driver model code in this respect. The same restriction applies wherever object lifetimes are controlled by reference counting.

Right. But it might not be obvious what 's the background here:

An allocated device object(memory) usually represents an actual device(hardware). The object can have N users. Every of the users is required to take a reference to the object, which pins the object's memory as long as any of the N users might need to access it.

In a hotplug world, we deal with device-removal. On disconnect, we usually just orphan the object, we remove it from visibility, disconnect the device <-> object relation.

All of the N users with a reference can still access the memory, they just do not talk to a real device anymore. The invalidated/orphaned state is communicated otherwise by locks and flags in the device object. Only after all of the N users left the object alone, the memory of the orphan if free'd.

Quoted 17 lines · Kay Sievers

Quoted 3 lines

There's nothing special about the driver model code in this respect. The same restriction applies wherever object lifetimes are controlled by reference counting.

Right. But it might not be obvious what 's the background here:

An allocated device object(memory) usually represents an actual device(hardware). The object can have N users. Every of the users is required to take a reference to the object, which pins the object's memory as long as any of the N users might need to access it.

In a hotplug world, we deal with device-removal. On disconnect, we usually just orphan the object, we remove it from visibility, disconnect the device <-> object relation.

Quoted 5 lines

All of the N users with a reference can still access the memory, they just do not talk to a real device anymore. The invalidated/orphaned state is communicated otherwise by locks and flags in the device object. Only after all of the N users left the object alone, the memory of the orphan if free'd.

But this is not what happened here - it's a special piece of fundamental hardware that doesnt hot-plug separately from the CPU and that has just a single "user".

So i'm curious, why wasn't the memset() enough? It should have resolved the bug AFAICS.

Quoted 4 lines · Ingo Molnar

But this is not what happened here - it's a special piece of fundamental hardware that doesnt hot-plug separately from the CPU and that has just a single "user".

Quoted 3 lines

So i'm curious, why wasn't the memset() enough? It should have resolved the bug AFAICS.

It did! The memset _did_ fix the bug.

Or to put it another way, even though no code takes references to these device structures (can you really guarantee that even now?), [...]

[...] how do you prevent references being taken in future versions of the kernel? Calling memset while there still are outstanding references very definitely _is_ a bug.

Quoted 3 lines · Alan Stern

Or to put it another way, even though no code takes references to these device structures (can you really guarantee that even now?), [...]

That's a good question. Do these devices get exposed to user-space access via sysfs somehow, where such an extra reference could materialize?

Quoted 3 lines

[...] how do you prevent references being taken in future versions of the kernel? Calling memset while there still are outstanding references very definitely _is_ a bug.

Unless there's a reference i missed the 'refcounting' here is very simple and does not exist at all.

IMHO you should not impose complexity and abstraction on code just because. Now i don't care much about the extra pointer, but the [NR_CPUS] array was doubly ugly (and Greg promised to fix that, so no problems from me).

So the conclusion is that the reasons stated here don't really apply. The one i mentioned *does* apply: the original bug was due to sloppy life time rules of this driver buffer, and that came back to bite us. Making it dynamic solves that kind of problem. In that sense the warning is fine too.

Quoted 28 lines · Ingo Molnar

Quoted 24 lines · Greg KH

Quoted 17 lines · Ingo Molnar

arch/x86/include/asm/mce.h static inline void enable_p5_mce(void) {} · line 154

Quoted 10 lines · Greg KH

arch/x86/include/asm/mce.h +1 -1

index f35ce43..6aefb14 100644
@@ -151,7 +151,7 @@ static inline void enable_p5_mce(void) {}
 
 void mce_setup(struct mce *m);
 void mce_log(struct mce *m);
-DECLARE_PER_CPU(struct device, mce_device);
+extern struct device *mce_device[CONFIG_NR_CPUS];

Minor nit, i don't think we have any other such [CONFIG_NR_CPUS] pattern in the kernel.

Quoted 3 lines

This should be something like:

  DECLARE_PER_CPU(struct device *, mce_device);

That is what we used to have, but with just a static struct device. [...]

Quoted 4 lines

Which was fine in itself for a per CPU data structure - wouldnt the warning be fixed by memset()-ing before registering the device or such, if device registry absolutely needs a pre-zeroed buffer?

It was already fixed that way, but the problem is that you can not have statically allocated 'struct device' objects in the system. [...]

Quoted 4 lines

I don't object to the quick fix that gets rid of the warnings, but that quick fix came at the price of leaving the real bug unfixed and at the price of introducing a new ugliness ;-)

Nope, all of the bugs are now fixed :)

Quoted 7 lines

Quoted 3 lines

[...] We really don't need this to be in the per-cpu area, a flat array should be just fine, why can't we use the CONFIG_NR_CPUS value? Should we use something else?

By that argument we don't really need PER_CPU() areas to begin with, a flat [CONFIG_NR_CPUS] array is just fine, right?

I never said that, only for this type of variable.

Quoted 4 lines

Would be nice if you could do that or some other equivalent solution, i'd really not like to see the [CONFIG_NR_CPUS] pattern to spread in the kernel, we spent a lot of time getting rid of such uses ;-)

Ok, I'll work on resolving this.

From: Greg Kroah-Hartman <[email protected]>

When I previously fixed up the mce_device code, I used a static array of the pointers. It was (rightfully) pointed out to me that I should be using the per_cpu code instead.

This patch converts the code over to that structure, moving the variable back into the per_cpu area, like it used to be for 3.2 and earlier.

Signed-off-by Greg Kroah-Hartman <[email protected]>

From: Greg Kroah-Hartman <[email protected]>

When suspending, there was a large list of warnings going something like:

Device 'machinecheck1' does not have a release() function, it is broken and must be fixed

This patch turns the static mce_devices into dynamically allocated, and properly frees them when they are removed from the system. It solves the warning messages on my laptop here.

/* Per cpu device init. All of the cpus still share the same ctrl bank: */ static __cpuinit int mce_device_create(unsigned int cpu)
{

arch/x86/kernel/cpu/mcheck/mce.c +6 -2

-	struct device *dev = &per_cpu(mce_device, cpu);
+	struct device *dev;
 	int err;
 	int i, j;
 
 	if (!mce_available(&boot_cpu_data))
 		return -EIO;
 
-	memset(dev, 0, sizeof(struct device));
+	dev = kzalloc(sizeof *dev, GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
 	dev->id  = cpu;
 	dev->bus = &mce_subsys;
+	dev->release = &mce_device_release;
 
 	err = device_register(dev);
 	if (err)

@@ -2030,6 +2038,7 @@ static __cpuinit int mce_device_create(unsigned int cpu)
 			goto error2;
 	}
 	cpumask_set_cpu(cpu, mce_device_initialized);
+	mce_device[cpu] = dev;
 
 	return 0;
 error2:

@@ -2046,7 +2055,7 @@ error:
 
 /* Make sure there are no machine checks on offlined CPUs. */

arch/x86/kernel/cpu/mcheck/mce_amd.c +1 -0

index ba0b94a..786e76a 100644
@@ -523,6 +523,7 @@ static __cpuinit int threshold_create_bank(unsigned int cpu, unsigned int bank)
 {
 	int i, err = 0;
 	struct threshold_bank *b = NULL;
+	struct device *dev = mce_device[cpu];
 	char name[32];
 
 	sprintf(name, "threshold_bank%i", bank);

arch/x86/kernel/cpu/mcheck/mce_amd.c +0 -0

 	if (!b->kobj)
 		goto out_free;
 

@@ -585,8 +585,9 @@ static __cpuinit int threshold_create_bank(unsigned int cpu, unsigned int bank)
 		if (i == cpu)

Quoted 10 lines · Greg KH

From: Greg Kroah-Hartman <[email protected]>

When suspending, there was a large list of warnings going something like:

Device 'machinecheck1' does not have a release() function, it is broken and must be fixed

This patch turns the static mce_devices into dynamically allocated, and properly frees them when they are removed from the system. It solves the warning messages on my laptop here.

...

Quoted 45 lines

/* Per cpu device init. All of the cpus still share the same ctrl bank: */ static __cpuinit int mce_device_create(unsigned int cpu)
{

arch/x86/kernel/cpu/mcheck/mce.c +6 -2

-	struct device *dev = &per_cpu(mce_device, cpu);
+	struct device *dev;
 	int err;
 	int i, j;
 
 	if (!mce_available(&boot_cpu_data))
 		return -EIO;
 
-	memset(dev, 0, sizeof(struct device));
+	dev = kzalloc(sizeof *dev, GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
 	dev->id  = cpu;
 	dev->bus = &mce_subsys;
+	dev->release = &mce_device_release;
 
 	err = device_register(dev);
 	if (err)

@@ -2030,6 +2038,7 @@ static __cpuinit int mce_device_create(unsigned int cpu)
 			goto error2;
 	}
 	cpumask_set_cpu(cpu, mce_device_initialized);
+	mce_device[cpu] = dev;
 
 	return 0;
 error2:

@@ -2046,7 +2055,7 @@ error:
 
 /* Make sure there are no machine checks on offlined CPUs. */

arch/x86/kernel/cpu/mcheck/mce_amd.c +1 -0

index ba0b94a..786e76a 100644
@@ -523,6 +523,7 @@ static __cpuinit int threshold_create_bank(unsigned int cpu, unsigned int bank)
 {
 	int i, err = 0;
 	struct threshold_bank *b = NULL;
+	struct device *dev = mce_device[cpu];
 	char name[32];
 
 	sprintf(name, "threshold_bank%i", bank);

arch/x86/kernel/cpu/mcheck/mce_amd.c static __cpuinit int threshold_create_bank(unsigned int cpu, unsigned int bank) · line 547

Quoted 6 lines

arch/x86/kernel/cpu/mcheck/mce_amd.c +1 -2

@@ -543,8 +544,7 @@ static __cpuinit int threshold_create_bank(unsigned int cpu, unsigned int bank)
 		if (!b)
 			goto out;
 
-		err = sysfs_create_link(&per_cpu(mce_device, cpu).kobj,
-					b->kobj, name);
+		err = sysfs_create_link(&dev->kobj, b->kobj, name);

I don't think dereferencing 'dev' like this is safe when booting up. (See below for another such instance of dereferencing dev.)

Both mcheck_init_device() and threshold_init_device() are device_initcalls. And the latter depends on the former, because the former dynamically allocates and fills the 'mce_device' array of pointers.

So, what guarantees that this ordering is preserved? IOW, what ensures that mcheck_init_device() is completed before running threshold_init_device()?

Or am I missing something?