The makefile is a bit illogical. The dump-y stuff doesn't make much sense
unless you actually uses it :) E.g.:
export-objs := dump_base.o
dump-y += dump_base.o
dump-$(CONFIG_ALPHA) += dump_alpha.o
dump-$(CONFIG_IA64) += dump_ia64.o
dump-$(CONFIG_X86) += dump_i386.o
dump-objs += $(dump-y)
obj-$(CONFIG_CRASH_DUMP) += dump.o
obj-$(CONFIG_CRASH_DUMP_BLOCKDEV) += dump_blockdev.o
obj-$(CONFIG_CRASH_DUMP_COMPRESS_RLE) += dump_rle.o
obj-$(CONFIG_CRASH_DUMP_COMPRESS_GZIP) += dump_gzip.o
BTW, I can't see the alpha/ia64 stuff actually beeing included.
> +static kdev_t dump_device; /* the actual kdev_t device number */
should be dev_t, not kdev_t.
> +/*
> + * Name: dump_read_proc()
> + * Func: Read the proc data for dump tunables.
> + */
> +static int
> +dump_read_proc(char *page, char **start, off_t off,
> + int count, int *eof, void *data)
> +{
> + int len;
> + char *out = page;
> + struct proc_dir_entry *p = (struct proc_dir_entry *)data;
> +
> +
> + if (0 == strcmp(p->name, DUMP_LEVEL_NAME)) {
> + out += sprintf(out, "%d\n", dump_level);
> + len = out - page;
> + } else if (0 == strcmp(p->name, DUMP_FLAGS_NAME)) {
> + out += sprintf(out, "%d\n", dump_flags);
> + len = out - page;
> + } else if (0 == strcmp(p->name, DUMP_COMPRESS_NAME)) {
> + out += sprintf(out, "%d\n", dump_compress);
> + len = out - page;
> + } else if (0 == strcmp(p->name, DUMP_DEVICE_NAME)) {
> + out += sprintf(out, "0x%x\n", kdev_val(dump_device));
> + len = out - page;
> + } else {
> + return 0;
> + }
> + len -= off;
> + if (len < count) {
> + *eof = 1;
> + if (len <= 0) return 0;
> + } else {
> + len = count;
> + }
> + *start = page + off;
> + return len;
Again: if you have one value per file use (ro) sysctl!
> +#ifndef UTSNAME_ENTRY_SZ
> +#define UTSNAME_ENTRY_SZ 65
> +#endif
Shouldn't this be in a generic header?
> + dump_header_asm.dha_version = DUMP_ASM_VERSION_NUMBER;
> + dump_header_asm.dha_header_size = sizeof(struct __dump_header_asm);
Indentation looks broken. Please run all sourcefiles through unexpand(1)
> + /* if this is the second call to this function, clean up ... */
> + if (dump_okay && !dump_page_buf_0)
> + kfree((const void *)dump_page_buf_0);
kfree(NULL) is fine.. i.e.:
if (dump_okay)
kfree(dump_page_buf_0);
Dito for all other places.
> +/*
> + * Name: dump_release()
> + * Func: Release the dump device -- it's never necessary to call
> + * this function, but it's here regardless.
> + */
> +static int
> +dump_release(struct inode *i, struct file *f)
> +{
> + return 0;
> +}
Just remove it. You don't have to implement ->release.
> +
> +/*
> + * Name: dump_ioctl()
> + * Func: Allow all dump tunables through a standard ioctl() mechanism.
> + * This is far better than before, where we'd go through /proc,
> + * because now this will work for multiple OS and architectures.
> + */
> +static int
> +dump_ioctl(struct inode *i, struct file *f, unsigned int cmd, unsigned long arg)
> +{
> + /* check capabilities */
> + if (!capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> + /*
> + * This is the main mechanism for controlling get/set data
> + * for various dump device parameters. The real trick here
> + * is setting the dump device (DIOSDUMPDEV). That's what
> + * triggers everything else.
> + */
> + switch (cmd) {
> + /* set dump_device */
> + case DIOSDUMPDEV:
> + /* check flags */
> + if (!(f->f_flags & O_RDWR))
> + return -EPERM;
> +
> + __dump_open();
> + return (dump_open_kdev(to_kdev_t((dev_t)arg)));
> +
> + /* get dump_device */
> + case DIOGDUMPDEV:
> + return (put_user((long)kdev_val(dump_device), (long *)arg));
> +
> + /* set dump_level */
> + case DIOSDUMPLEVEL:
> + /* check flags */
> + if (!(f->f_flags & O_RDWR))
> + return -EPERM;
> +
> + /* make sure we have a positive value */
> + if (arg < 0)
> + return -EINVAL;
> + dump_level = (int)arg;
> +
> + if (dump_level > DUMP_LEVEL_KERN)
> + dump_unreserved_mem = 1;
> + else
> + dump_unreserved_mem = 0;
> +
> + if (dump_level > DUMP_LEVEL_USED)
> + dump_unreferenced_mem = 1;
> + else
> + dump_unreferenced_mem = 0;
> +
> + if (dump_level > DUMP_LEVEL_ALL_RAM)
> + dump_nonconventional_mem = 1;
> + else
> + dump_nonconventional_mem = 0;
> + break;
> +
> + /* get dump_level */
> + case DIOGDUMPLEVEL:
> + return (put_user((long)dump_level, (long *)arg));
> +
> + /* set dump_flags */
> + case DIOSDUMPFLAGS:
> + /* check flags */
> + if (!(f->f_flags & O_RDWR))
> + return -EPERM;
> +
> + /* make sure we have a positive value */
> + if (arg < 0)
> + return -EINVAL;
> +
> + dump_flags = (int)arg;
> + break;
> +
> + /* get dump_flags */
> + case DIOGDUMPFLAGS:
> + return (put_user((long)dump_flags, (long *)arg));
> +
> + /* set the dump_compress status */
> + case DIOSDUMPCOMPRESS:
> + /* check flags */
> + if (!(f->f_flags & O_RDWR))
> + return -EPERM;
> +
> + return (dump_compress_init((int)arg));
> +
> + /* get the dump_compress status */
> + case DIOGDUMPCOMPRESS:
> + return (put_user((long)dump_compress, (long *)arg));
This is still the wrong interface :) At least the read/write one value
stuff should be sysctl. For the rest ioctl is horribly ugly, but I'll
leave the decision to Linus whether he wants to merge more ugly ioctl
APIs.
> +
> + /* try to create our dump device */
> + if (register_chrdev(CRASH_DUMP_MAJOR, "dump", &dump_fops)) {
> + printk("cannot register dump character device!\n");
> + return -EBUSY;
> + }
> +
> + /* initialize the dump headers to zero */
> + memset(&dump_header, 0, sizeof(dump_header));
> + memset(&dump_header_asm, 0, sizeof(dump_header_asm));
You don't have to zero structures lying in .bss
> +
> + /* reset the dump function pointer */
> + dump_function_ptr = NULL;
pointless. the module memory will be vfree()ed anyway.
> +#ifdef MODULE
> +MODULE_AUTHOR("Matt D. Robinson <yakker@sourceforge.net>");
> +MODULE_DESCRIPTION("Linux Kernel Crash Dump (LKCD) driver");
> +MODULE_LICENSE("GPL");
> +#endif /* MODULE */
No need for the ifdef - it'll be stubbed out anyway.
> +kdev_t dump_dev; /* the actual kdev_t device number */
> +int dump_blk_size; /* sector size for dump_device */
> +int dump_blk_shift; /* shift need to convert to sector size */
> +struct bio *dump_bio; /* bio structure for io request */
> +loff_t dump_offset; /* the offset in the output device */
> +unsigned long dumpdev_limit; /* the size limit of the block device */
> +int dump_io_abort = 0; /* set by end_io func during io error */
> +
> +/* dump block device */
> +struct block_device *dump_bdev = NULL;
> +
> +/* indicates completion of each bio request */
> +volatile int bio_complete = 1;
Most of this should be static, right?
> + dump_bdev = bdget(kdev_t_to_nr(tmp_dump_device));
> + if (!dump_bdev) {
> + retval = -ENODEV;
> + goto err;
> + }
> +
> + if (blkdev_get(dump_bdev, O_RDWR | O_LARGEFILE, 0, BDEV_RAW)) {
> + retval = -ENODEV;
> + goto err1;
> + }
> +
> + if (!dump_page_buf) {
> + retval = -EINVAL;
> + goto err2;
> + }
You need to call bd_claim to get exclusion on the bdev.
> +err3: dump_free_bio();
> +err2: if (dump_bdev) {
> + blkdev_put(dump_bdev, BDEV_RAW);
> + dump_bdev = NULL;
> + }
> +err1: if (dump_bdev) {
> + bdput(dump_bdev);
> + dump_bdev = NULL;
> + }
> +err: return retval;
> +}
err3:
dump_free_bio();
err2:
if (dump_bdev)
blkdev_put(dump_bdev, BDEV_RAW);
goto err;
err1:
if (dump_bdev)
bdput(dump_bdev);
err:
return retval;
> +
> +static int __init
> +dump_blockdev_init(void)
> +{
> + dump_offset = 0;
once again, .bss is already per-zeroed :)
> +void
> +__dump_cleanup(void)
> +{
> + free_dha_stack();
> + return;
> +}
I've noticed these superflous returns in void function a lot
over the code. Linux kernel code usually avoids that.
> +static int __init
> +dump_compress_rle_init(void)
> +{
> + dump_register_compression(&dump_rle_compression);
> + return 0;
Shouldn't there be some kind of error reporting? i.e.
return dump_register_compression(&dump_rle_compression); ?
> +/* header definitions for s390 dump */
> +#define DUMP_MAGIC_S390 0xa8190173618f23fdULL /* s390 magic number */
> +#define S390_DUMP_HEADER_SIZE 4096
should go to asm-s390/dump.h?
-
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/