Re: [PATCH 2/2] Add full sysfs support to the IPMI driver

[Corey Minyard]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/