Re: [2/4] [PATCH]Diskdump - yet another crash dump function

From: Christoph Hellwig
Date: Thu May 27 2004 - 08:49:54 EST


On Thu, May 27, 2004 at 09:37:30PM +0900, Takao Indoh wrote:
> obj-$(CONFIG_BLK_DEV_SR) += sr_mod.o
> obj-$(CONFIG_CHR_DEV_SG) += sg.o
>
> +obj-$(CONFIG_SCSI_DUMP) += scsi_dump.o

please use tabs instead of space here.

> @@ -691,6 +691,10 @@
> {
> unsigned long flags;
>
> +#if defined(CONFIG_SCSI_DUMP) || defined(CONFIG_SCSI_DUMP_MODULE)
> + if (crashdump_mode())
> + return;
> +#endif

Please make sure crashdump_mode() is always defined so you don't need the
ifdef mess.

> +#include <linux/config.h>

not needed.

> +#include "scsi.h"

please don't use this in new code, use the include/scsi/*.h includes
instead.

> +#include "scsi_priv.h"

this is a private header for scsi_mod.ko as the name might suggest ;)
But given the rather strange exports I'd suggest to always build
scsi_dump.c into scsi_mod.ko anyway.

> +#include "hosts.h"

Please use <scsi/scsi_host.h>

> +#include "scsi_dump.h"
> +#include <scsi/scsi_ioctl.h>
> +
> +#include <linux/genhd.h>
> +#include <linux/utsname.h>
> +#include <linux/crc32.h>
> +#include <linux/diskdump.h>
> +#include <linux/diskdumplib.h>
> +#include <linux/delay.h>

please rework the include order, <linux/*.h> first, then <asm/*.h>,
then <scsi/*.h>, then private headers.

> +#define DEBUG 0
> +#if DEBUG
> +# define Dbg(x, ...) printk(KERN_INFO "scsi_dump:" x "\n", ## __VA_ARGS__)
> +#else
> +# define Dbg(x...)
> +#endif
> +
> +#define Err(x, ...) printk(KERN_ERR "scsi_dump: " x "\n", ## __VA_ARGS__);
> +#define Warn(x, ...) printk(KERN_WARNING "scsi_dump: " x "\n", ## __VA_ARGS__)
> +#define Info(x, ...) printk(x "\n", ## __VA_ARGS__)

please use the pr_* macros from kernel.h


> +static int quiesce_ok = 0;

on need to initialize to 0

> +static Scsi_Cmnd scsi_dump_cmnd;
> +static struct request scsi_dump_req;
> +static uint32_t module_crc;
> +
> +static void rw_intr(Scsi_Cmnd * scmd)

Please never use the Scsi_Foo types but the struct scsi_foo versions
(goes hand in hand with using <scsi/*.h>

> +static void init_scsi_command(Scsi_Device *sdev, Scsi_Cmnd *scmd, void *buf, int len, unsigned char direction, int set_lun)

plese make sure lines aren't longer than 80 characters.

> + if (!spin_is_locked(host->host_lock)) {
> + sanity = 0;
> + } else {
> + Warn("host_lock is held: host %d channel %d id %d lun %d",
> + host->host_no, sdev->channel, sdev->id, sdev->lun);
> + if (host->host_lock == &host->default_lock)
> + sanity = 1;
> + else
> + return -EIO;
> + }

This look bogus to me. Why handle the case of the default and per-driver
lock differently?

> +static int scsi_dump_add_device(struct disk_dump_device *dump_device)
> +{
> + Scsi_Device *sdev;
> +
> + sdev = dump_device->device;
> + if (!sdev->host->hostt->dump_ops)
> + return -ENOTSUPP;
> +
> + scsi_device_get(sdev); /* retval ignored ? */

Please fix this ;-)

> diff -Nur linux-2.6.6.org/drivers/scsi/scsi_dump.h linux-2.6.6/drivers/scsi/scsi_dump.h
> --- linux-2.6.6.org/drivers/scsi/scsi_dump.h 1970-01-01 09:00:00.000000000 +0900
> +++ linux-2.6.6/drivers/scsi/scsi_dump.h 2004-05-27 09:31:07.000000000 +0900
> @@ -0,0 +1,38 @@
> +#ifndef _SCSI_DUMP_H
> +#define _SCSI_DUMP_H

This file should go into include/scsi/.

> +struct scsi_dump_ops {
> + int (*sanity_check)(Scsi_Device *);
> + int (*quiesce)(Scsi_Device *);
> + int (*shutdown)(Scsi_Device *);
> + void (*poll)(Scsi_Device *);
> +};

But I'm not sure we need it at all. These should just go into the
scsi_host_template, imho.

> static void scsi_eh_done(struct scsi_cmnd *scmd)
> {
> +#if defined(CONFIG_SCSI_DUMP) || defined(CONFIG_SCSI_DUMP_MODULE)
> + if (crashdump_mode())
> + return;
> +#endif

Same comments as above, please avoid ifdefs.

> +#include "scsi_priv.h"
>
>
> /*
> @@ -107,3 +108,5 @@
> */
> EXPORT_SYMBOL(scsi_add_timer);
> EXPORT_SYMBOL(scsi_delete_timer);
> +
> +EXPORT_SYMBOL(scsi_decide_disposition);

prototype in scsi_priv.h == not exported

> --- linux-2.6.6.org/drivers/scsi/sd.c 2004-05-20 08:58:48.000000000 +0900
> +++ linux-2.6.6/drivers/scsi/sd.c 2004-05-27 09:24:46.000000000 +0900
> @@ -192,6 +192,21 @@
> up(&sd_ref_sem);
> }
>
> +#if defined(CONFIG_DISKDUMP) || defined(CONFIG_DISKDUMP_MODULE)
> +Scsi_Device *sd_find_scsi_device(dev_t dev)
> +{
> + struct gendisk *disk;
> + int part;
> + disk = get_gendisk(dev, &part);
> + if(disk && disk->private_data)
> + return scsi_disk(disk)->device;
> + else
> + return NULL;
> +}
> +
> +EXPORT_SYMBOL(sd_find_scsi_device);
> +#endif

Not the kind of interface we want exported. IMHO you shouldn't find
device by dev_t but add a dumpdevice sysfs attribute to the scsi_device
where you can echo 1 to to make it a possible dump device.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/