Messages in this thread | | | Date | Wed, 22 Mar 2006 15:29:03 -0600 | From | Corey Minyard <> | Subject | Re: [PATCH 2/2] Add full sysfs support to the IPMI driver |
| |
Greg KH wrote:
>On Wed, Mar 22, 2006 at 10:01:09AM +0100, Arjan van de Ven wrote: > > >>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? >> >> > >Of course that's not ok. > >Come on people, does everyone think I just put that warning message in >the kernel for fun to force you to create an empty release function? >Why do people ignore the helpful hints that the kernel provides? > >I can take that check out and watch people get their code wrong even >more, as it sure doesn't seem like it is helping anyone out these >days... > >Corey, haven't we discussed this in the past? > > I don't think so, this is my first attempt at anything beyond a simple class device for the driver model. This code was a rework of something that came from someone else, so I never actually saw the kernel message.
I did look in the include files while doing this and a little at the driver-model docs, and I didn't see anything that jumped out and said "THIS IS IMPORTANT". The release field documented in devices.txt, for instance, says:
release: Callback to free the device after all references have gone away. This should be set by the allocator of the device (i.e. the bus driver that discovered the device).
It's not altogether obvious how to use this function or its importance from this text, though if I had thought about it I would have figured it out. If you want to affect behaviour, you need to add text like: "NOTE: You must keep the data that holds the device structure around until the release function is called. If you do not, something could be using the device structure after you free it's data, resulting in bad things happening". A comment should probably appear in the include file, too.
If the issue Russell brought up is really an issue, it truely needs some documentation written about it.
That said, I should have looked a little deeper on this. I saw it there, read the docs and include file, and thought, "That's probably just there to aid debugging," and left it in.
-Corey - 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/
| |