Re: [PATCH] scsi_device refcounting and list lockdown

James Bottomley
2003-10-30T22:41:11Z

On Mon, 2003-10-27 at 09:57, Christoph Hellwig wrote:
> --- 1.44/drivers/scsi/53c700.c Sat Sep 20 11:35:53 2003
> +++ edited/drivers/scsi/53c700.c Mon Oct 27 12:01:48 2003
> @@ -1057,15 +1057,18 @@
> __u8 lun;
> struct NCR_700_command_slot *slot;
> __u8 reselection_id = hostdata->reselection_id;
> - Scsi_Device *SDp;
> + struct scsi_device *SDp;
> + unsigned long flags;
>
> lun = hostdata->msgin[0] & 0x1f;
>
> hostdata->reselection_id = 0xff;
> DEBUG(("scsi%d: (%d:%d) RESELECTED!\n",
> host->host_no, reselection_id, lun));
> +
> /* clear the reselection indicator */
> - SDp = scsi_find_device(host, 0, reselection_id, lun);
> + spin_lock_irqsave(host->host_lock, flags);
> + SDp = __scsi_device_lookup(host, 0, reselection_id, lun);
> if(unlikely(SDp == NULL)) {
> printk(KERN_ERR "scsi%d: (%d:%d) HAS NO device\n",
> host->host_no, reselection_id, lun);

This is wrong. The messages are processed in the middle of a SCSI
transaction, so the host lock is already held.

[...]

> ===== drivers/scsi/scsi.h 1.91 vs edited =====
> --- 1.91/drivers/scsi/scsi.h Sat Sep 20 13:50:50 2003
> +++ edited/drivers/scsi/scsi.h Mon Oct 27 12:01:41 2003
> @@ -116,12 +116,6 @@
> #define scsi_to_pci_dma_dir(scsi_dir) ((int)(scsi_dir))
> #define scsi_to_sbus_dma_dir(scsi_dir) ((int)(scsi_dir))
>
> -/*
> - * This is the crap from the old error handling code. We have it in a special
> - * place so that we can more easily delete it later on.
> - */
> -#include "scsi_obsolete.h"
> -
> /* obsolete typedef junk. */
> #include "scsi_typedefs.h"

This isn't a bug fix, and removing the obsolete include breaks all of
the drivers relying on its defines (cpqfc, qlogicisp, qlogicfc,
ncr53c8xx etc.)

> ===== drivers/scsi/scsi_error.c 1.65 vs edited =====
> --- 1.65/drivers/scsi/scsi_error.c Sun Sep 21 19:49:36 2003
> +++ edited/drivers/scsi/scsi_error.c Mon Oct 27 12:01:49 2003
> @@ -22,11 +22,10 @@
> #include <linux/kernel.h>
> #include <linux/interrupt.h>
> #include <linux/blkdev.h>
> -#include <linux/smp_lock.h>

This is a required include for lock/unlock_kernel() calls used in the
error handler thread.

> ===== include/scsi/scsi_host.h 1.12 vs edited =====
> --- 1.12/include/scsi/scsi_host.h Thu Oct 16 10:56:58 2003
> +++ edited/include/scsi/scsi_host.h Mon Oct 27 12:01:50 2003
> @@ -125,15 +125,6 @@
> int (* eh_host_reset_handler)(struct scsi_cmnd *);
>
> /*
> - * Old EH handlers, no longer used. Make them warn the user of old
> - * drivers by using a wrong type
> - *
> - * Status: MORE THAN OBSOLETE
> - */
> - int (* abort)(int);
> - int (* reset)(int, int);
> -
> - /*

This isn't a bugfix either...it will just break unconverted drivers.

James