lkml.org 
[lkml]   [2002]   [Oct]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] 2.5.44: lkcd (9/9): dump driver and build files
On Mon, Oct 21, 2002 at 03:16:05AM -0700, Matt D. Robinson wrote:
> diff -Naur linux-2.5.44.orig/drivers/dump/Makefile linux-2.5.44.lkcd/drivers/dump/Makefile
> --- linux-2.5.44.orig/drivers/dump/Makefile Wed Dec 31 16:00:00 1969
> +++ linux-2.5.44.lkcd/drivers/dump/Makefile Sat Oct 19 12:39:15 2002
> @@ -0,0 +1,30 @@
> +#
> +# Makefile for the dump device drivers.
> +#
> +# 12 June 2000, Christoph Hellwig <schch@pe.tu-clausthal.de>
> +# Rewritten by Matt D. Robinson (yakker@sourceforge.net) for
> +# the dump directory.

*cough* is that an old mail address and I can't even remember having
touched that file.. :) What about remoing that note..

> +#include <linux/devfs_fs_kernel.h>

Can't see any devfs code used in this file..

> +#include <linux/delay.h>
> +#include <linux/reboot.h>
> +#include <linux/fs.h>
> +#include <linux/mm.h>
> +#include <linux/swap.h>
> +#include <linux/dump.h>
> +#include <linux/smp_lock.h>

Can't see lock_kernel/unlock_kernel used in this file,
please check whether you have more unneeded includes.

> +/*
> + * Handle printing of 64-bit values
> + *
> + * NOTE: on ia64 %llx is recommended for ia32.
> + * on RedHat 7.2 %llx work in user space but not in the kernel.
> + * Perhaps this is dependent on the kernel version.
> + */
> +#if BITS_PER_LONG == 64
> +#define PU64X "%lx"
> +#else
> +#define PU64X "%Lx"
> +#endif

Argg. Just always cast to long long and use %llx.

> +/* Dump tunables */
> +kdev_t dump_device; /* the actual kdev_t device number */

Should be dev_T.

> +int dump_level; /* the current dump level */
> +int dump_compress; /* whether to try to compress each page */
> +int dump_flags; /* whether to try to compress each page */
> +
> +/* Dump Notifier Tunables */
> +long dump_scheduler_enabled = 0; /* Default: scheduler is disabled */
> +long dump_interrupts_enabled = 1; /* Default: interrupts stay enabled */
> +long dump_nondisruptive_enabled = 1; /* Default: non-disruptive enabled */
> +
> +/* Other global fields */
> +void *dump_page_buf; /* dump page buffer for memcpy()! */
> +void *dump_page_buf_0; /* dump page buffer returned by kmalloc */
> +dump_header_t dump_header; /* the primary dump header */
> +dump_header_asm_t dump_header_asm; /* the arch-specific dump header */
> +loff_t dump_fpos; /* the offset in the output device */
> +int dump_mbanks; /* number of physical memory banks */
> +dump_mbank_t dump_mbank[MAXCHUNKS];/* describes layout of physical memory */
> +long dump_unreserved_mem = 0; /* Save Pages even if it isn't reserved */
> +long dump_unreferenced_mem = 0; /* Save Pages even if page_count == 0 */
> +long dump_nonconventional_mem = 0; /* Save non-conventional mem (firmware) */
> +volatile int dump_started = 0; /* Indicated we are about to dump */

Using volatile is almost always a bug. USe atomic variables
or bitops instead.

> +static struct dump_operations *dump_device_ops = 0;
> +
> +static int dump_compress_none(char *old, int oldsize, char *new, int newsize);

How many of those could be static?

> +
> +static dump_compress_t dump_none_compression = {
> + compress_type: DUMP_COMPRESS_NONE,
> + compress_func: dump_compress_none,
> +};

C99 initializers.

> +#if DUMP_DEBUG
> +void dump_bp(void) {}
> +#endif

Doesn't seem to be implemented at all..

> + * 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);
> +}

This wants to be changed to either seq_file or sysctl. Sysctl
might be better for the one value per file approach.

> +static int
> +dump_compress_none(char *old, int oldsize, char *new, int newsize)
> +{
> + /* just return the old size */
> + return (oldsize);

return style again.

> +void dump_scheduler_enable(void)
> +{
> + DUMP_DPRINTF("setting dump_scheduler_enabled = 1\n");
> + dump_scheduler_enabled = 1;
> +}

Can't see this called anywhere

> +
> +void dump_interrupts_disable(void)
> +{
> + DUMP_DPRINTF("setting dump_interrupts_enabled = 0\n");
> + dump_interrupts_enabled = 0;
> +}

Dito.

> +
> +void dump_nondisruptive_disable(void)
> +{
> + DUMP_DPRINTF("setting dump_nondisruptive_enabled = 0\n");
> + dump_nondisruptive_enabled = 0;
> +}

Dito.

> +static int
> +dump_add_page(unsigned long page_index, unsigned long *toffset, int pass)
> +{
> +#if defined(CONFIG_X86) || defined(CONFIG_ALPHA)
> + extern int page_is_ram(unsigned long);
> +#endif

Yuck. Please just declare page_is_ram for every port.

> +#if !defined(CONFIG_DISCONTIGMEM) && !defined(CONFIG_IA64)
> + p = (struct page *) &(mem_map[page_index]);

Yuck. Please don't use mem_map in common code.

> +#ifdef CONFIG_HIGHMEM
> + if (PageHighMem(p)) {
> + /*
> + * Since this can be executed from IRQ context,
> + * reentrance on the same CPU must be avoided:
> + */
> + vaddr = kmap_atomic(p, KM_USER0);
> + }
> + else

kmap_atomic is a noop for !CONFIG_HIGHMEM - make this code unconditional.

> +#if !defined(CONFIG_DISCONTIGMEM) && !defined(CONFIG_IA64)
> + if (!kern_addr_valid(dp.dp_address)) {
> + /* dump of I/O memory not supported yet */
> + printk(KERN_ALERT "dump_add_page: !kern_addr_valid"
> + "(dp.dp_address: " PU64X "\n", dp.dp_address);
> + return(1);
> + }
> +#endif

Why don't you do this uncoditional?

> +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;
> +
> + /*
> + * REMIND: Still in development:
> + * We will consider reserved pages a initial proxy
> + * for kernel pages.
> + */
> + if (dump_level > DUMP_LEVEL_KERN)
> + dump_unreserved_mem = 1;
> + else
> + dump_unreserved_mem = 0;
> +
> + /*
> + * REMIND: Still in development:
> + * Using refcount > 1 as a proxy for kernel & user
> + * pages.
> + */
> + if (dump_level > DUMP_LEVEL_USED)
> + dump_unreferenced_mem = 1;
> + else
> + dump_unreferenced_mem = 0;
> +
> + /*
> + * REMIND: Still in development:
> + * This may not be 100% stable on all ia64
> + * memory banks. Accessing uncached memory
> + * with cached accesses can cause subsequent
> + * bus errors when the cache is flushed.
> + */
> + 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));
> +
> + }
> + return (0);

I don't think ioctl is the right interface. Most of those want to be sysctls.

> +/*
> + * Name: dump_open()
> + * Func: Open the dump device for use when the system crashes.
> + */
> +static int
> +dump_open(struct inode *i, struct file *f)
> +{
> + /* opening a device is straightforward -- nothing to do here */
> + return (0);
> +}

Remove it.

> +/*
> + * Name: dump_init()
> + * Func: Initialize the dump process. This will set up any architecture
> + * dependent code. The big key is we need the memory offsets before
> + * the page table is initialized, because the base memory offset
> + * is changed after paging_init() is called.
> + */
> +int __init
> +dump_init(void)

static

> +{
> + struct sysinfo info;
> + int i;
> +
> + /* try to initialize /proc interfaces */
> + if (dump_proc_init() < 0) {
> + DUMP_PRINTF("dump_proc_init failed!; dump not initialized\n");
> + return (-EBUSY);
> + }
> +
> + /* try to create our dump device */
> + if (register_chrdev(DUMP_MAJOR, "dump", &dump_fops)) {
> + DUMP_PRINTF("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));
> +
> +#if !defined(CONFIG_DISCONTIGMEM) && !defined(CONFIG_IA64)
> + /*
> + * CONFIG_DISCONTIGMEM and CONFIG_IA64 systems are responsible
> + * for initializing dump_mbank[] in __dump_init().
> + */
> + dump_mbanks = 1;
> + dump_mbank[0].start = 0;
> + dump_mbank[0].end = (((u64) max_mapnr) << PAGE_SHIFT) - 1;
> + dump_mbank[0].type = DUMP_MBANK_TYPE_CONVENTIONAL_MEMORY;
> +#endif

Just move it to arch code for everyone.

> +void __exit
> +dump_cleanup(void)

static

> + /* try to create our dump device */
> + if (unregister_chrdev(DUMP_MAJOR, "dump"))
> + DUMP_PRINT("cannot unregister dump character device!\n");

the printk is rather pointless..

> +#ifdef MODULE
> +#if (LINUX_VERSION_CODE < KERNEL_VERSION(2,4,16))
> +#if !defined(MODULE_LICENSE)
> +#define MODULE_LICENSE(str)
> +#endif
> +#endif

kill this ifdef junk.

> +
> +#define DUMP_MODULE_NAME "dump_blockdev"

doesn't seem to be used.

> +
> +/*
> + * -----------------------------------------------------------------------
> + * V A R I A B L E S
> + * -----------------------------------------------------------------------
> + */
> +kdev_t dump_dev; /* the actual kdev_t device number */

didn't we already have this one in dump_base.c?

> +void *dump_page_buffer; /* dump page buffer for memcpy()! */
> +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 */
> +struct block_device *dump_bdev = NULL;
> +unsigned long dumpdev_limit; /* the size limit of the block device */
> +volatile int bio_complete = 1;/* Indicates the completion of each bio request */
> +int dump_io_abort = 0; /* set by end_io routine during io error */

Shouldn't much of this be static? again volatile is seldomly a good idea.

> +static void
> +dump_free_bio(void)
> +{
> + if (dump_bio && dump_bio->bi_io_vec)
> + kfree(dump_bio->bi_io_vec);
> + if (dump_bio)
> + kfree(dump_bio);
> +}

kfree(NULL) is fine.

> +/*
> + * Name: dump_open_kdev()
> + * Func: Try to open the kdev_t argument as the real dump device.
> + * This is where all the work is done for setting up the dump
> + * device. It's assumed at this point that by passing in the
> + * dump device's major/minor number, we can open it up, check
> + * it out, and use it for whatever purposes.
> + */
> +static int
> +dump_open_kdev(kdev_t tmp_dump_device)

Should take a dev_t.

> + /* release earlier dump_bdev if present */
> + if (dump_bdev) {
> + blkdev_put(dump_bdev, BDEV_RAW);
> + dump_bdev = NULL;
> + }

Shouldn't this have gone away in ->close?

> +
> + 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;
> + }

bd_claim is missing

> +
> + if ((dump_page_buffer== (void *)NULL)) {

(void *)NULL) is silly

> + retval = -EINVAL;
> + goto err2;
> + }
> +
> + if (dump_bio)
> + dump_free_bio();
> +
> + if ((dump_bio = kmalloc(sizeof(struct bio), GFP_KERNEL)) == NULL) {
> + DUMP_PRINTF("Cannot allocate bio\n");
> + retval = -ENOMEM;
> + goto err2;
> + }

Shouldn't you use the generic bio allocator?

> +static int
> +dev_dump_ioctl(unsigned int cmd, unsigned long arg)
> +{
> + int ret = 0;
> +
> + /*
> + * 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) {
> + case DIOSDUMPDEV:
> + ret = dump_open_kdev(to_kdev_t((dev_t)arg));
> + break;
> + case DIOSDUMPMEM:
> + dump_page_buffer = (void *)arg;
> + break;
> + default:
> + break;
> +
> + }
> + return (ret);
> +}

ioctl is not a replacement for sane interfaces again!

> +static struct dump_operations dump_blockdev_ops = {
> + dump_open: dev_dump_open,
> + dump_release: dev_dump_release,
> + dump_ioctl: dev_dump_ioctl,
> + dump_write: dev_dump_write,
> + dump_seek: dev_dump_seek,
> + dump_start: dev_dump_start,
> + dump_end: dev_dump_end,
> + dump_ready: dev_dump_ready,
> +};

Indentation seems strange, wants C99 initializers.

> +int __init
> +dump_blockdev_init(void)

static

> +{
> + dump_page_buffer = NULL;
> + dump_offset = 0;

.bbs is per-zeroed.

> +void __exit
> +dump_blockdev_cleanup(void)

static.

> +#ifdef MODULE
> +#if (LINUX_VERSION_CODE < KERNEL_VERSION(2,4,16))
> +#if !defined(MODULE_LICENSE)
> +#define MODULE_LICENSE(str)
> +#endif
> +#endif
> +
> +MODULE_AUTHOR("Mohamed. Abbas");
> +MODULE_DESCRIPTION("Block Device Driver for Linux Kernel Crash Dump (LKCD)");
> +#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,16))
> +MODULE_LICENSE("GPL");
> +#endif
> +#endif /* MODULE */

more ifdef-crap to kill.

> + int i;
> + void *ptr;
> +
> + if (dump_header_asm.dha_stack[0])
> + return 0;
> +
> + ptr = vmalloc(THREAD_SIZE * num_online_cpus());
> + if (!ptr) {

very strange indention..

> +typedef struct _dump_header_asm_s {

Please DON'T do typedefs. just struct _dump_header_asm!

> + /* the dump magic number -- unique to verify dump is valid */
> + uint64_t dha_magic_number;

u64 & co

> +#ifdef __KERNEL__

kernel/user headers usually aren't shared..

> +/* define TRUE and FALSE for use in our dump modules */
> +#ifndef FALSE
> +#define FALSE 0
> +#endif
> +
> +#ifndef TRUE
> +#define TRUE 1
> +#endif

Please just use 0/1 like evreryone else.

> +#if DUMP_DEBUG
> +void dump_bp(void); /* Called when something exceptional occurs */
> +#define DUMP_BP() dump_bp() /* Breakpoint */
> +#else
> +#define DUMP_BP()
> +#endif

It doesn't look like this actually works.

> +/* necessary header definitions in all cases */
> +#define DUMP_KIOBUF_NUMBER 0xdeadbeef /* special number for kiobuf maps */

kiobufs are gone in 2.5.44.

> +typedef struct dump_mbank {
> + u64 start;
> + u64 end;
> + int type;
> + int pad1;
> + long pad2;
> +} dump_mbank_t;

identation is once again rather strange.

> +#ifndef KERNEL_VERSION
> +#define KERNEL_VERSION(a,b,c) (((a) << 16) | ((b) << 8) | (c))
> +#endif

kill this.

-
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/

\
 
 \ /
  Last update: 2005-03-22 13:30    [W:0.090 / U:0.628 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site