lkml.org 
[lkml]   [2008]   [Nov]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2008-11-20 15:19    [W:0.261 / U:0.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site