lkml.org 
[lkml]   [2010]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC] persistent store (version 2) (part 1 of 2)
    On Tue, 30 Nov 2010 16:20:50 -0800
    "Luck, Tony" <tony.luck@intel.com> wrote:

    > Here's version 2 with most of the fixes suggested in the discussion
    > for version 1 (see http://marc.info/?l=linux-arch&m=129045078803118&w=2).

    I haven't really thought about the overall concept, but I saw stuff to
    nitpick at.


    > Stuff that I haven't done:
    >
    > 1) Still has the "daft" echo filename > erase to clear entries from the
    > persistent store. I looked hard at /sys and can't see how to get a
    > notification for an unlink(2). If I'm missing something, would somebody
    > please send me a clue. If this is a fatal flaw - then this will have to
    > move to debugfs (or turn into its own filesystem type). But I'd really
    > prefer to not give up the convenience of /sys
    >
    > 2) Did not implement Jim's suggestion to save OOPs ... I'm not at
    > all sure how to let the user configure this. mtdoops.c has a module
    > parameter for it - but I gave up on letting pstore be a module ...
    > it needs to be initialized before the platform driver, and there doesn't
    > seem to be much point in having it be unloadable.
    >
    > I did write the ERST platform hooks (they are part 2 of 2, but I'd advise
    > the faint of heart not to look too closely at the ACPI/APEI'isms involved).
    > Here's a sample of what it looks like (with a debug call to panic() from
    > the pstore_erase() function added for testing purposes):
    >
    > $ cd /sys/firmware/pstore
    > $ ls -l
    > total 0
    > -r--r--r-- 1 root root 7896 Nov 30 15:38 dmesg-1
    > --w------- 1 root root 0 Nov 30 15:38 erase
    > $ cat dmesg-1 | tail -12
    > <4>[ 201.840197] mtrr: type mismatch for 90000000,2000000 old: write-back new: write-combining
    > <0>[ 1044.792139] Kernel panic - not syncing: Testing pstore
    > <4>[ 1044.792147] Pid: 7634, comm: bash Not tainted 2.6.36-rc6 #13
    > <4>[ 1044.792150] Call Trace:
    > <4>[ 1044.792166] [<ffffffff815c7266>] panic+0xbc/0x1c6
    > <4>[ 1044.792176] [<ffffffff810e6c50>] ? get_write_access+0x1e/0x50
    > <4>[ 1044.792182] [<ffffffff810ea77d>] ? do_filp_open+0x1f4/0x594
    > <4>[ 1044.792193] [<ffffffff814d72f5>] pstore_erase+0x41/0x14f
    > <4>[ 1044.792199] [<ffffffff8112fbf2>] write+0x11e/0x160
    > <4>[ 1044.792208] [<ffffffff810dec87>] vfs_write+0xb3/0x13a
    > <4>[ 1044.792213] [<ffffffff810deddc>] sys_write+0x4c/0x73
    > <4>[ 1044.792222] [<ffffffff81002bdb>] system_call_fastpath+0x16/0x1b
    >
    >
    > ...
    >
    > --- /dev/null
    > +++ b/drivers/firmware/pstore.c
    > @@ -0,0 +1,294 @@
    > +/*
    > + * Persistent Storage - pstore.c
    > + *
    > + * Copyright (C) 2010 Intel Corporation <tony.luck@intel.com>
    > + *
    > + * This code is the generic layer to export data records from platform
    > + * level persistent storage via sysfs.
    > + *
    > + * This program is free software; you can redistribute it and/or modify
    > + * it under the terms of the GNU General Public License version 2 as
    > + * published by the Free Software Foundation.
    > + *
    > + * This program is distributed in the hope that it will be useful,
    > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
    > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
    > + * GNU General Public License for more details.
    > + *
    > + * You should have received a copy of the GNU General Public License
    > + * along with this program; if not, write to the Free Software
    > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
    > + */
    > +
    > +#include <linux/atomic.h>
    > +#include <linux/types.h>
    > +#include <linux/errno.h>
    > +#include <linux/init.h>
    > +#include <linux/kmsg_dump.h>
    > +#include <linux/module.h>
    > +#include <linux/pstore.h>
    > +#include <linux/string.h>
    > +#include <linux/sysfs.h>
    > +#include <linux/slab.h>
    > +#include <linux/uaccess.h>
    > +
    > +MODULE_AUTHOR("Tony Luck <tony.luck@intel.com>");
    > +MODULE_DESCRIPTION("sysfs interface to persistent storage");
    > +MODULE_LICENSE("GPL");
    > +
    > +static DEFINE_SPINLOCK(pstore_lock);

    It'd be nice to know what pstore_lock actually locks.

    > +static LIST_HEAD(pstore_list);
    > +static struct kset *pstore_kset;

    It locks psinfo (at least), so I'd suggest that all things which
    pstore_lock locks be collected immediately after its definition.

    > +#define PSTORE_NAMELEN 16
    > +
    > +struct pstore_entry {
    > + struct bin_attribute attr;
    > + char name[PSTORE_NAMELEN];
    > + u64 id;
    > + int type;
    > + int size;
    > + struct list_head list;
    > + char data[];
    > +};
    > +
    > +static int pstore_create_sysfs_entry(struct pstore_entry *new_pstore);
    > +
    > +static struct pstore_info *psinfo;
    > +
    > +static char *pstore_buf;
    > +
    >
    > ...
    >
    > +/*
    > + * platform specific persistent storage driver registers with
    > + * us here. Read out all the records right away and install
    > + * them in /sys. Register with kmsg_dump to save last part
    > + * of console log on panic.
    > + */
    > +int
    > +pstore_register(struct pstore_info *psi)

    bah, ia64 coding style leaking into core kernel.

    > +{
    > + struct pstore_entry *new_pstore;
    > + unsigned long size, ps_maxsize;
    > + u64 id;
    > + int rc = 0, type;
    > +
    > + spin_lock(&pstore_lock);
    > + if (psinfo) {
    > + spin_unlock(&pstore_lock);
    > + return -EBUSY;
    > + }
    > + if (!psi->reader || !psi->writer || !psi->eraser) {
    > + spin_unlock(&pstore_lock);
    > + return -EINVAL;

    It doesn't seem appropriate to check this here. It's a programming
    error! Just install the thing and let the kernel oops - the programmer
    will notice.

    > + }
    > + psinfo = psi;
    > + spin_unlock(&pstore_lock);
    > +
    > + ps_maxsize = psi->header_size + psi->data_size + psi->footer_size;
    > + pstore_buf = kzalloc(ps_maxsize, GFP_KERNEL);
    > + if (!pstore_buf)
    > + return -ENOMEM;

    Here the state of the driver gets screwed up. We've installed the
    psinfo and we're unable to install a new one, but the one which we
    installed is only partially constructed. And it will cause oopses if
    we actually try to use it.

    > + for (;;) {
    > + if (psi->reader(&id, &type, pstore_buf, &size) <= 0)
    > + break;
    > + new_pstore = kzalloc(sizeof(struct pstore_entry) + size,
    > + GFP_KERNEL);
    > + if (!new_pstore) {
    > + rc = -ENOMEM;
    > + break;
    > + }
    > + new_pstore->id = id;
    > + new_pstore->type = type;
    > + new_pstore->size = size;
    > + memcpy(new_pstore->data, pstore_buf + psi->header_size, size);
    > + if (pstore_create_sysfs_entry(new_pstore)) {
    > + kfree(new_pstore);
    > + rc = -EINVAL;
    > + break;
    > + }
    > + }
    > +
    > + kobject_uevent(&pstore_kset->kobj, KOBJ_ADD);
    > +
    > + kmsg_dump_register(&pstore_dumper);
    > +
    > + return rc;
    > +}
    > +EXPORT_SYMBOL_GPL(pstore_register);

    So I'd suggest that this function *fully* back out if anything fails.
    That means restoring psinfo. And fixing the error-path memory leaks
    which might be there (I didn't check too hard).

    Bonus points for implementing it with only one `return' statement ;)

    >
    > ...
    >
    > +/*
    > + * Erase records by writing their filename to the "erase" file. E.g.
    > + * # echo "dmesg-0" > erase
    > + */

    Since when are "records" identified by "filenames"?

    filenames refer to files! And lo, that's what we have here. A
    filesystem! The files are created by the kernel and are read and
    unlinked by userspace.

    So why not implement this whole thing as a proper filesystem?

    > +static ssize_t pstore_erase(struct file *filp, struct kobject *kobj,
    > + struct bin_attribute *bin_attr,
    > + char *buf, loff_t pos, size_t count)
    > +{
    > + struct pstore_entry *search_pstore, *n;
    > + int len1, len2, found = 0;
    > +
    > + len1 = count;
    > + if (buf[len1 - 1] == '\n')
    > + len1--;
    > +
    > + spin_lock(&pstore_lock);
    > +
    > + /*
    > + * Find this record
    > + */
    > + list_for_each_entry_safe(search_pstore, n, &pstore_list, list) {
    > + len2 = strlen(search_pstore->name);
    > + if (len1 == len2 && memcmp(buf, search_pstore->name,
    > + len1) == 0) {
    > + found = 1;
    > + break;
    > + }
    > + }

    hm, what's going on here.

    Can that line really have a trailing \n?

    Part of this code assumes that we're using null-terminated strings but
    other parts don't do it that way.

    All seems a bit confusing and messed up. Might be improved via
    appropriate use of strim() and sysfs_streq(). Would definitely be
    improved via a comment explaining what's going on here!

    > + if (!found) {
    > + spin_unlock(&pstore_lock);
    > + return -EINVAL;
    > + }
    > +
    > + if (psinfo->eraser(search_pstore->id)) {
    > + spin_unlock(&pstore_lock);

    I just discovered something else which pstore_lock locks.

    > + return -EIO;

    A single return site per function is better. Multiple-returns often
    lead to locking errors and memory leaks as the code evolves.

    > + }
    > +
    > + list_del(&search_pstore->list);
    > +
    > + spin_unlock(&pstore_lock);
    > +
    > + sysfs_remove_bin_file(&pstore_kset->kobj, &search_pstore->attr);
    > +
    > + kfree(search_pstore);
    > +
    > + return count;
    > +}
    > +
    > +static struct bin_attribute attr_erase = {
    > + .attr = {.name = "erase", .mode = 0200},
    > + .write = pstore_erase,
    > +};
    > +
    > +static int
    > +pstore_create_sysfs_entry(struct pstore_entry *new_pstore)
    > +{
    > + static atomic_t next;
    > + int error, seq;
    > +
    > + seq = atomic_add_return(1, &next);
    > +
    > + switch (new_pstore->type) {
    > + case PSTORE_TYPE_DMESG:
    > + sprintf(new_pstore->name, "dmesg-%d", seq);
    > + break;
    > + case PSTORE_TYPE_MCE:
    > + sprintf(new_pstore->name, "mce-%d", seq);
    > + break;
    > + default:
    > + sprintf(new_pstore->name, "type%d-%d", new_pstore->type, seq);
    > + break;
    > + }
    > +
    > + sysfs_attr_init(&new_pstore->attr.attr);
    > + new_pstore->attr.size = new_pstore->size;
    > + new_pstore->attr.read = pstore_show;
    > + new_pstore->attr.attr.name = new_pstore->name;
    > + new_pstore->attr.attr.mode = 0444;
    > + error = sysfs_create_bin_file(&pstore_kset->kobj, &new_pstore->attr);
    > + if (!error) {
    > + spin_lock(&pstore_lock);
    > + list_add(&new_pstore->list, &pstore_list);
    > + spin_unlock(&pstore_lock);
    > + }
    > + return error;
    > +}

    Wait. It *is* a filesystem.

    <looks back at the changelog>

    Nope, that was secret info.

    So why can't I remove these "records" with rm?

    >
    > ...
    >
    > +#if defined(CONFIG_PSTORE) || defined(CONFIG_PSTORE_MODULE)
    > +extern int pstore_register(struct pstore_info *);
    > +extern int pstore_write(int type, char *buf, unsigned long size);
    > +#else
    > +static inline int
    > +pstore_register(struct pstore_info *psi)
    > +{
    > + return -ENODEV;
    > +}
    > +static inline int
    > +pstore_write(int type, char *buf, unsigned long size)
    > +{
    > + return -ENODEV;
    > +}

    write() takes a size_t.

    > +#endif
    > +
    > +#endif /*_LINUX_PSTORE_H*/


    \
     
     \ /
      Last update: 2010-12-02 01:55    [W:0.049 / U:30.976 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site