Messages in this thread | | | Date | Fri, 16 Oct 2009 14:57:52 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH v10 4/5] core: Add kernel message dumper to call on oopses and panics |
| |
* Artem Bityutskiy <dedekind1@gmail.com> wrote:
> On Fri, 2009-10-16 at 12:10 +0200, Ingo Molnar wrote: > > * Simon Kagstrom <simon.kagstrom@netinsight.net> wrote: > > > > > +int kmsg_dump_register(struct kmsg_dumper *dumper) > > > +{ > > > + unsigned long flags; > > > + > > > + /* The dump callback needs to be set */ > > > + if (!dumper->dump) > > > + return -EINVAL; > > > + > > > + spin_lock_irqsave(&dump_list_lock, flags); > > > + > > > + /* Don't allow registering multiple times */ > > > + if (dumper->registered) { > > > + spin_unlock_irqrestore(&dump_list_lock, flags); > > > + > > > + return -EBUSY; > > > + } > > > + > > > + dumper->registered = 1; > > > + list_add(&dumper->list, &dump_list); > > > + spin_unlock_irqrestore(&dump_list_lock, flags); > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL_GPL(kmsg_dump_register); > > > > > > i dont want to bikeshed paint this but this sequence caught my eyes. We > > generally do flatter and clearer locking sequences: > > > > int kmsg_dump_register(struct kmsg_dumper *dumper) > > { > > unsigned long flags; > > int err = -EBUSY; > > > > /* The dump callback needs to be set */ > > if (!dumper->dump) > > return -EINVAL; > > > > spin_lock_irqsave(&dump_list_lock, flags); > > > > /* Don't allow registering multiple times */ > > if (!dumper->registered) { > > dumper->registered = 1; > > list_add_tail(&dumper->list, &dump_list); > > err = 0; > > } > > > > spin_unlock_irqrestore(&dump_list_lock, flags); > > > > return err; > > } > > EXPORT_SYMBOL_GPL(kmsg_dump_register); > > And while we are on it, I think these extra lines before and after > spinlocks are unneeded and even a bit annoying :-)
To me they increase readability quite a bit as it allows me to concentrate on just the inner critical section without the distraction of the lock/unlock sequence. YMMV.
Ingo
| |