Messages in this thread | | | Subject | Re: [PATCH v10 4/5] core: Add kernel message dumper to call on oopses and panics | From | Artem Bityutskiy <> | Date | Fri, 16 Oct 2009 14:00:28 +0300 |
| |
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 :-)
-- Best Regards, Artem Bityutskiy (Артём Битюцкий)
-- 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/
| |