Messages in this thread | | | Date | Thu, 20 Nov 2008 16:14:38 +0200 | From | Felipe Balbi <> | Subject | Re: [PATCH] led: simplify led_trigger_register_simple |
| |
On Thu, Nov 20, 2008 at 01:33:28PM +0000, ext Richard Purdie wrote: > On Thu, 2008-11-13 at 21:10 +0200, Felipe Balbi wrote: > > On Thu, Nov 13, 2008 at 08:14:16PM +0200, Felipe Balbi wrote: > > > On Thu, Nov 13, 2008 at 12:38:32PM +0000, Richard Purdie wrote: > > > > The simple triggers were designed to cause minimum interference to the > > > > usually external subsystem code they were added into. As an example this > > > > meant things like errors were just handled gracefully with a printk > > > > warning and did not take down the whole subsystem. I therefore don't > > > > regard this patch as a simplification, more a complication. > > > > > > That's a matter of changing the return ERR_PTR(err); back to a printk. > > > > And here you are. I still think we should at least kfree(trigger) in > > case of error, though. > > This patch now just changes the calling convention of the function which > doesn't seem to serve much purpose.
Well, it's your call then. But I don't like the idea of declaring a pointer, passing a pointer to that pointer to a function and changing the value of the original pointer.
I think allocating inside led_trigger_register_simple() and returning the pointer would look better.
> In answer to your question about kfree, I agree it needs to be called > upon error. The callers should just be calling > led_trigger_unregister_simple() in their failure paths though which > should take care of all problems? I know we used to register the simple > triggers late in paths so no error handling was needed to keep the code > simple and minimise the LED triggers impact on those systems.
Well, led_trigger_register_simple() doesn't return anything. Imagine led_trigger_register_simple() fails, but the driver author decides it's not a failure if, let's say, a led doesn't turn on when we insert a mmc card to the slot since it doesn't change functionality.
Now, imagine the user notes the led is not turning on and decides to unload and reload the module to try again. Once again the led doesn't go on. If the user keeps trying, it's quite a dangerous memory leak, right ?
-- balbi
| |