Re: [PATCH] sym53c8xx_2: slave_alloc/destroy safety (2.6.27.5)

Re: [PATCH] sym53c8xx_2: slave_alloc/destroy safety (2.6.27.5)

[Tony Battersby]

(resend - trying a different email address)

This patch can cause a NULL-pointer dereference and kernel oops.  In
sym53c8xx_slave_alloc(), there are starget_printk()s that use
tp->starget, e.g.:

starget_printk(KERN_INFO, tp->starget, "Scan at boot disabled in NVRAM\n");
...
starget_printk(KERN_INFO, tp->starget, "Multiple LUNs disabled in NVRAM\n");

However, you moved the setting of tp->starget to the end of the
function, so the starget_printk() above tries to dereference an
uninitialized pointer.

BUG: unable to handle kernel NULL pointer dereference at 0000015c
IP: [<c0243e13>] dev_driver_string+0x3/0x30
*pde = 00000000 
Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
Modules linked in: sym53c8xx(+) sg scsi_transport_spi mptsas mptscsih
scsi_transport_sas tms_iscsi tms mptctl mptbase w83781d hwmon_vid
i2c_piix4 i2c_core e1000 emlog ftdi_sio usbserial [last unloaded:
sym53c8xx]
 
Pid: 1145, comm: insmod Not tainted (2.6.27.10 #2)
EIP: 0060:[<c0243e13>] EFLAGS: 00010002 CPU: 0
EIP is at dev_driver_string+0x3/0x30
EAX: 00000014 EBX: 00000110 ECX: 00000007 EDX: 00000014
ESI: ce62d7f0 EDI: 00000000 EBP: ce4f1a08 ESP: ce4f19e0
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process insmod (pid: 1145, ti=ce4f0000 task=ce55d788 task.ti=ce4f0000)
Stack: ce4f1a08 d092ec70 00000005 00000000 00000000 ce402000 00000292
ce62d7f0 
       cf0a2bf0 cf0a2c04 ce4f1a2c c026236f 00000000 c025aac0 00000000
ce5ec7f0 
       ce5ec7f0 00000000 ce5ec958 ce4f1ae8 c026254d c0145ccd c0411cc0
c0411ce0 
Call Trace:
 [<d092ec70>] ? sym53c8xx_slave_alloc+0x160/0x190 [sym53c8xx]
 [<c026236f>] ? scsi_alloc_sdev+0x18f/0x200
 [<c025aac0>] ? scsi_device_lookup_by_target+0x60/0x80
 [<c026254d>] ? scsi_probe_and_add_lun+0xcd/0xb40
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<c0145d5b>] ? trace_hardirqs_on+0xb/0x10
 [<c03275f8>] ? mutex_unlock+0x8/0x10
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<c01d8702>] ? kobject_get+0x12/0x20
 [<c0244653>] ? get_device+0x13/0x20
 [<c0262026>] ? scsi_alloc_target+0x1e6/0x270
 [<c02631b8>] ? __scsi_scan_target+0xe8/0x6c0
 [<c0145d5b>] ? trace_hardirqs_on+0xb/0x10
 [<c0145b55>] ? mark_held_locks+0x65/0x80
 [<c0145d5b>] ? trace_hardirqs_on+0xb/0x10
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<c0327302>] ? __mutex_lock_common+0x1f2/0x2f0
 [<c026386b>] ? scsi_scan_host_selected+0x4b/0x140
 [<c0263802>] ? scsi_scan_channel+0x72/0x90
 [<c02638ed>] ? scsi_scan_host_selected+0xcd/0x140
 [<c0265eaa>] ? scsi_proc_host_add+0x4a/0xa0
 [<c02639d6>] ? do_scsi_scan_host+0x76/0x80
 [<c0263c8a>] ? scsi_scan_host+0x15a/0x190
 [<c0328ab9>] ? _spin_unlock_irqrestore+0x49/0x60
 [<d0937c8a>] ? sym2_probe+0x89a/0x92e [sym53c8xx]
 [<c01f4e2e>] ? pci_device_probe+0x5e/0x80
 [<c024717e>] ? driver_probe_device+0x7e/0x170
 [<c02472e5>] ? __driver_attach+0x75/0x80
 [<c0246a59>] ? bus_for_each_dev+0x49/0x70
 [<c0246ff9>] ? driver_attach+0x19/0x20
 [<c0247270>] ? __driver_attach+0x0/0x80
 [<c024635c>] ? bus_add_driver+0xac/0x220
 [<c01f4a40>] ? pci_device_remove+0x0/0x40
 [<c024747f>] ? driver_register+0x4f/0x120
 [<c01eb9b2>] ? __spin_lock_init+0x32/0x60
 [<d0864000>] ? sym2_init+0x0/0xf6 [sym53c8xx]
 [<c01f4cae>] ? __pci_register_driver+0x5e/0xa0
 [<d0864000>] ? sym2_init+0x0/0xf6 [sym53c8xx]
 [<d0864087>] ? sym2_init+0x87/0xf6 [sym53c8xx]
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<d0864000>] ? sym2_init+0x0/0xf6 [sym53c8xx]
 [<c010102a>] ? _stext+0x2a/0x140
 [<c0145d5b>] ? trace_hardirqs_on+0xb/0x10
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<c014d725>] ? sys_init_module+0x85/0x1b0
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<c01ddb94>] ? trace_hardirqs_on_thunk+0xc/0x10
 [<c0103031>] ? sysenter_do_call+0x12/0x35
 =======================
Code: ff ff e9 6c fe ff ff 8b 45 cc bf ed ff ff ff e8 d4 7b f2 ff e9 5a
fe ff ff 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 55 89 c2 <8b> 80
48 01 00 00 89 e5 85 c0 74 04 8b 00 5d c3 8b 82 44 01 00 
EIP: [<c0243e13>] dev_driver_string+0x3/0x30 SS:ESP 0068:ce4f19e0
---[ end trace 856efca87f217e80 ]---

Tony Battersby
Cybernetics

Re: [PATCH] sym53c8xx_2: slave_alloc/destroy safety (2.6.27.5)

[Tony Battersby]

Just found another problem with this patch: sym_alloc_lcb() allocates
memory with GFP_KERNEL, and you are calling it while holding a spinlock
(np->s.host->host_lock).  Either do the allocation without holding the
spinlock, or else use GFP_ATOMIC.

BUG: sleeping function called from invalid context at mm/slab.c:3043
in_atomic():0, irqs_disabled():1
2 locks held by insmod/1216:
 #0:  (&shost->scan_mutex){--..}, at: [<c026386b>]
scsi_scan_host_selected+0x4b/0x140
 #1:  (shost->host_lock){++..}, at: [<d092eb5c>]
sym53c8xx_slave_alloc+0x4c/0x190 [sym53c8xx]
irq event stamp: 7322
hardirqs last  enabled at (7321): [<c0145d5b>] trace_hardirqs_on+0xb/0x10
hardirqs last disabled at (7322): [<c014120b>] trace_hardirqs_off+0xb/0x10
softirqs last  enabled at (7204): [<c0127c32>] __do_softirq+0x102/0x120
softirqs last disabled at (7185): [<c0127ca7>] do_softirq+0x57/0x60
Pid: 1216, comm: insmod Not tainted 2.6.27.10 #2
 [<c0127ca7>] ? do_softirq+0x57/0x60
 [<c011b2e6>] __might_sleep+0xc6/0xf0
 [<c016bcfd>] __kmalloc+0x11d/0x150
 [<d09304b1>] ? sym_alloc_lcb+0xe1/0x180 [sym53c8xx]
 [<d09304b1>] sym_alloc_lcb+0xe1/0x180 [sym53c8xx]
 [<d092ec02>] sym53c8xx_slave_alloc+0xf2/0x190 [sym53c8xx]
 [<c026236f>] scsi_alloc_sdev+0x18f/0x200
 [<c025aac0>] ? scsi_device_lookup_by_target+0x60/0x80
 [<c026254d>] scsi_probe_and_add_lun+0xcd/0xb40
 [<c02632da>] __scsi_scan_target+0x20a/0x6c0
 [<c0145d5b>] ? trace_hardirqs_on+0xb/0x10
 [<c0145d5b>] ? trace_hardirqs_on+0xb/0x10
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<c0327302>] ? __mutex_lock_common+0x1f2/0x2f0
 [<c026386b>] ? scsi_scan_host_selected+0x4b/0x140
 [<c0263802>] scsi_scan_channel+0x72/0x90
 [<c02638ed>] scsi_scan_host_selected+0xcd/0x140
 [<c0265eaa>] ? scsi_proc_host_add+0x4a/0xa0
 [<c02639d6>] do_scsi_scan_host+0x76/0x80
 [<c0263c8a>] scsi_scan_host+0x15a/0x190
 [<c0328ab9>] ? _spin_unlock_irqrestore+0x49/0x60
 [<d0937c8a>] sym2_probe+0x89a/0x92e [sym53c8xx]
 [<c01f4e2e>] pci_device_probe+0x5e/0x80
 [<c024717e>] driver_probe_device+0x7e/0x170
 [<c02472e5>] __driver_attach+0x75/0x80
 [<c0246a59>] bus_for_each_dev+0x49/0x70
 [<c0246ff9>] driver_attach+0x19/0x20
 [<c0247270>] ? __driver_attach+0x0/0x80
 [<c024635c>] bus_add_driver+0xac/0x220
 [<c01f4a40>] ? pci_device_remove+0x0/0x40
 [<c024747f>] driver_register+0x4f/0x120
 [<c01eb9b2>] ? __spin_lock_init+0x32/0x60
 [<d0864000>] ? sym2_init+0x0/0xf6 [sym53c8xx]
 [<c01f4cae>] __pci_register_driver+0x5e/0xa0
 [<d0864000>] ? sym2_init+0x0/0xf6 [sym53c8xx]
 [<d0864087>] sym2_init+0x87/0xf6 [sym53c8xx]
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<d0864000>] ? sym2_init+0x0/0xf6 [sym53c8xx]
 [<c010102a>] _stext+0x2a/0x140
 [<c0145d5b>] ? trace_hardirqs_on+0xb/0x10
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<c014d725>] sys_init_module+0x85/0x1b0
 [<c0145ccd>] ? trace_hardirqs_on_caller+0xbd/0x140
 [<c01ddb94>] ? trace_hardirqs_on_thunk+0xc/0x10
 [<c0103031>] sysenter_do_call+0x12/0x35
 [<c01211f8>] ? __mmdrop+0x28/0x30
 =======================

Tony Battersby
Cybernetics
 

Re: [PATCH] sym53c8xx_2: slave_alloc/destroy safety (2.6.27.5)

[Aaro Koskinen]

Thanks. The updated patch is below.

...

Make the sym53c8xx_2 driver slave_alloc/destroy less unsafe. References
to the destroyed LCB are cleared from the target structure (instead of
leaving a dangling pointer), and when the last LCB for the target is
destroyed the reference to the upper layer target data is cleared. The
host lock is used to prevent a race with the interrupt handler. Also
user commands are prevented for targets with all LCBs destroyed.

Signed-off-by: Aaro Koskinen <Aaro.Koskinen@nokia.com>
---
 drivers/scsi/sym53c8xx_2/sym_glue.c |   66 +++++++++++++++++++++++++++-------
 drivers/scsi/sym53c8xx_2/sym_hipd.c |   40 ++++++++++++++++++++-
 drivers/scsi/sym53c8xx_2/sym_hipd.h |    2 +
 3 files changed, 93 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/sym53c8xx_2/sym_glue.c b/drivers/scsi/sym53c8xx_2/sym_glue.c
index d39107b..073e5b6 100644
--- a/drivers/scsi/sym53c8xx_2/sym_glue.c
+++ b/drivers/scsi/sym53c8xx_2/sym_glue.c
@@ -737,11 +737,14 @@ static int sym53c8xx_slave_alloc(struct scsi_device *sdev)
 	struct sym_hcb *np = sym_get_hcb(sdev->host);
 	struct sym_tcb *tp = &np->target[sdev->id];
 	struct sym_lcb *lp;
+	unsigned long flags;
+	int error;
 
 	if (sdev->id >= SYM_CONF_MAX_TARGET || sdev->lun >= SYM_CONF_MAX_LUN)
 		return -ENXIO;
 
-	tp->starget = sdev->sdev_target;
+	spin_lock_irqsave(np->s.host->host_lock, flags);
+
 	/*
 	 * Fail the device init if the device is flagged NOSCAN at BOOT in
 	 * the NVRAM.  This may speed up boot and maintain coherency with
@@ -753,26 +756,37 @@ static int sym53c8xx_slave_alloc(struct scsi_device *sdev)
 
 	if (tp->usrflags & SYM_SCAN_BOOT_DISABLED) {
 		tp->usrflags &= ~SYM_SCAN_BOOT_DISABLED;
-		starget_printk(KERN_INFO, tp->starget,
+		starget_printk(KERN_INFO, sdev->sdev_target,
 				"Scan at boot disabled in NVRAM\n");
-		return -ENXIO;
+		error = -ENXIO;
+		goto out;
 	}
 
 	if (tp->usrflags & SYM_SCAN_LUNS_DISABLED) {
-		if (sdev->lun != 0)
-			return -ENXIO;
-		starget_printk(KERN_INFO, tp->starget,
+		if (sdev->lun != 0) {
+			error = -ENXIO;
+			goto out;
+		}
+		starget_printk(KERN_INFO, sdev->sdev_target,
 				"Multiple LUNs disabled in NVRAM\n");
 	}
 
 	lp = sym_alloc_lcb(np, sdev->id, sdev->lun);
-	if (!lp)
-		return -ENOMEM;
+	if (!lp) {
+		error = -ENOMEM;
+		goto out;
+	}
+	if (tp->nlcb == 1)
+		tp->starget = sdev->sdev_target;
 
 	spi_min_period(tp->starget) = tp->usr_period;
 	spi_max_width(tp->starget) = tp->usr_width;
 
-	return 0;
+	error = 0;
+out:
+	spin_unlock_irqrestore(np->s.host->host_lock, flags);
+
+	return error;
 }
 
 /*
@@ -819,12 +833,34 @@ static int sym53c8xx_slave_configure(struct scsi_device *sdev)
 static void sym53c8xx_slave_destroy(struct scsi_device *sdev)
 {
 	struct sym_hcb *np = sym_get_hcb(sdev->host);
-	struct sym_lcb *lp = sym_lp(&np->target[sdev->id], sdev->lun);
+	struct sym_tcb *tp = &np->target[sdev->id];
+	struct sym_lcb *lp = sym_lp(tp, sdev->lun);
+	unsigned long flags;
+
+	spin_lock_irqsave(np->s.host->host_lock, flags);
+
+	if (lp->busy_itlq || lp->busy_itl) {
+		/*
+		 * This really shouldn't happen, but we can't return an error
+		 * so let's try to stop all on-going I/O.
+		 */
+		starget_printk(KERN_WARNING, tp->starget,
+			       "Removing busy LCB (%d)\n", sdev->lun);
+		sym_reset_scsi_bus(np, 1);
+	}
 
-	if (lp->itlq_tbl)
-		sym_mfree_dma(lp->itlq_tbl, SYM_CONF_MAX_TASK * 4, "ITLQ_TBL");
-	kfree(lp->cb_tags);
-	sym_mfree_dma(lp, sizeof(*lp), "LCB");
+	if (sym_free_lcb(np, sdev->id, sdev->lun) == 0) {
+		/*
+		 * It was the last unit for this target.
+		 */
+		tp->head.sval        = 0;
+		tp->head.wval        = np->rv_scntl3;
+		tp->head.uval        = 0;
+		tp->tgoal.check_nego = 1;
+		tp->starget	     = NULL;
+	}
+
+	spin_unlock_irqrestore(np->s.host->host_lock, flags);
 }
 
 /*
@@ -890,6 +926,8 @@ static void sym_exec_user_command (struct sym_hcb *np, struct sym_usrcmd *uc)
 			if (!((uc->target >> t) & 1))
 				continue;
 			tp = &np->target[t];
+			if (!tp->nlcb)
+				continue;
 
 			switch (uc->cmd) {
 
diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.c b/drivers/scsi/sym53c8xx_2/sym_hipd.c
index 98df165..a38ed04 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.c
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.c
@@ -4953,7 +4953,7 @@ struct sym_lcb *sym_alloc_lcb (struct sym_hcb *np, u_char tn, u_char ln)
 	 */
 	if (ln && !tp->lunmp) {
 		tp->lunmp = kcalloc(SYM_CONF_MAX_LUN, sizeof(struct sym_lcb *),
-				GFP_KERNEL);
+				GFP_ATOMIC);
 		if (!tp->lunmp)
 			goto fail;
 	}
@@ -4973,6 +4973,7 @@ struct sym_lcb *sym_alloc_lcb (struct sym_hcb *np, u_char tn, u_char ln)
 		tp->lun0p = lp;
 		tp->head.lun0_sa = cpu_to_scr(vtobus(lp));
 	}
+	tp->nlcb++;
 
 	/*
 	 *  Let the itl task point to error handling.
@@ -5050,6 +5051,43 @@ fail:
 }
 
 /*
+ *  Lun control block deallocation. Returns the number of valid remaing LCBs
+ *  for the target.
+ */
+int sym_free_lcb(struct sym_hcb *np, u_char tn, u_char ln)
+{
+	struct sym_tcb *tp = &np->target[tn];
+	struct sym_lcb *lp = sym_lp(tp, ln);
+
+	tp->nlcb--;
+
+	if (ln) {
+		if (!tp->nlcb) {
+			kfree(tp->lunmp);
+			sym_mfree_dma(tp->luntbl, 256, "LUNTBL");
+			tp->lunmp = NULL;
+			tp->luntbl = NULL;
+			tp->head.luntbl_sa = cpu_to_scr(vtobus(np->badluntbl));
+		} else {
+			tp->luntbl[ln] = cpu_to_scr(vtobus(&np->badlun_sa));
+			tp->lunmp[ln] = NULL;
+		}
+	} else {
+		tp->lun0p = NULL;
+		tp->head.lun0_sa = cpu_to_scr(vtobus(&np->badlun_sa));
+	}
+
+	if (lp->itlq_tbl) {
+		sym_mfree_dma(lp->itlq_tbl, SYM_CONF_MAX_TASK*4, "ITLQ_TBL");
+		kfree(lp->cb_tags);
+	}
+
+	sym_mfree_dma(lp, sizeof(*lp), "LCB");
+
+	return tp->nlcb;
+}
+
+/*
  *  Queue a SCSI IO to the controller.
  */
 int sym_queue_scsiio(struct sym_hcb *np, struct scsi_cmnd *cmd, struct sym_ccb *cp)
diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.h b/drivers/scsi/sym53c8xx_2/sym_hipd.h
index ad07880..2ec25e1 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.h
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.h
@@ -400,6 +400,7 @@ struct sym_tcb {
 	 *  An array of bus addresses is used on reselection.
 	 */
 	u32	*luntbl;	/* LCBs bus address table	*/
+	int	nlcb;		/* Number of valid LCBs (including LUN #0) */
 
 	/*
 	 *  LUN table used by the C code.
@@ -1061,6 +1062,7 @@ int sym_clear_tasks(struct sym_hcb *np, int cam_status, int target, int lun, int
 struct sym_ccb *sym_get_ccb(struct sym_hcb *np, struct scsi_cmnd *cmd, u_char tag_order);
 void sym_free_ccb(struct sym_hcb *np, struct sym_ccb *cp);
 struct sym_lcb *sym_alloc_lcb(struct sym_hcb *np, u_char tn, u_char ln);
+int sym_free_lcb(struct sym_hcb *np, u_char tn, u_char ln);
 int sym_queue_scsiio(struct sym_hcb *np, struct scsi_cmnd *csio, struct sym_ccb *cp);
 int sym_abort_scsiio(struct sym_hcb *np, struct scsi_cmnd *ccb, int timed_out);
 int sym_reset_scsi_target(struct sym_hcb *np, int target);
-- 
1.5.4.3

Re: [PATCH] sym53c8xx_2: slave_alloc/destroy safety (2.6.27.5)

[James Bottomley]

On Tue, 2008-12-30 at 12:10 +0200, Aaro Koskinen wrote:
> Thanks. The updated patch is below.

Tony,

Since we're lacking an active maintainer on this one and you seem
interested, could you run these patches through your tests to make sure
they're reasonably OK and then respond with an ack?

Thanks,

James

Re: [PATCH] sym53c8xx_2: slave_alloc/destroy safety (2.6.27.5)

[Tony Battersby]

James Bottomley wrote:
> On Tue, 2008-12-30 at 12:10 +0200, Aaro Koskinen wrote:
>   
>> Thanks. The updated patch is below.
>>     
>
> Tony,
>
> Since we're lacking an active maintainer on this one and you seem
> interested, could you run these patches through your tests to make sure
> they're reasonably OK and then respond with an ack?
>
> Thanks,
>
> James
>
>
>
>   
This patch survived an overnight testing of "modprobe sym53c8xx;
modprobe -r sym53c8xx" in a loop, using a SCSI cable of marginal quality
that sometimes fails DV, with a mix of single-lun and multi-lun devices
of various SCSI revision levels, with many kernel debug options
enabled.  The only kernel warning message I see is the pre-existing and
already-reported problem about dma_free_coherent() being called with
interrupts disabled during rmmod, but that is not caused by this patch.

>From the description of this patch, I assume that it is a cleanup
instead of a fix for an actual problem.  Correspondingly, I don't have a
specific testcase that is improved by this patch.  I also have not
analyzed the source to see what the patch is trying to do.  However, the
current form of the patch passes testing as described above.

I will reply to the other patches in separate messages later.

Tested-by: Tony Battersby <tonyb@cybernetics.com>

Tony

Re: [PATCH] sym53c8xx_2: slave_alloc/destroy safety (2.6.27.5)

[Aaro Koskinen]

Hello,

ext Tony Battersby wrote:
> From the description of this patch, I assume that it is a cleanup
> instead of a fix for an actual problem.  Correspondingly, I don't have a
> specific testcase that is improved by this patch.  I also have not
> analyzed the source to see what the patch is trying to do.  However, the
> current form of the patch passes testing as described above.

You can try to do the following test with and without the patch:

echo "scsi remove-single-device some_valid_address_with_a_target > 
/proc/scsi/scsi"

Then do e.g. "resetdev" or "cleardev" to that target through the drivers 
/proc interface.

A.

Re: [PATCH] sym53c8xx_2: slave_alloc/destroy safety (2.6.27.5)

[Tony Battersby]

Aaro Koskinen wrote:
> Hello,
>
> ext Tony Battersby wrote:
>   
>> From the description of this patch, I assume that it is a cleanup
>> instead of a fix for an actual problem.  Correspondingly, I don't have a
>> specific testcase that is improved by this patch.  I also have not
>> analyzed the source to see what the patch is trying to do.  However, the
>> current form of the patch passes testing as described above.
>>     
>
> You can try to do the following test with and without the patch:
>
> echo "scsi remove-single-device some_valid_address_with_a_target > 
> /proc/scsi/scsi"
>
> Then do e.g. "resetdev" or "cleardev" to that target through the drivers 
> /proc interface.
>
> A.
>
>   
OK, I just tested that and verified that your patch fixes it.  Thanks!

Tony

Re: [PATCH] sym53c8xx_2: Keep transfer negotiations valid (2.6.27.5)

[Tony Battersby]

On a device that negotiates asynchronous, this patch causes a WDTR
exchange on every command (not just inquiry/request sense), even on
devices that don't advertise wide support in the inquiry data.  This is
unnecessary overhead, and may (in theory) cause problems for old devices
that don't handle WDTR correctly.  The original code avoided this by
setting nego = 0 in sym_prepare_nego() if the current agreement matched
the goal.  However, it was pointed out that the original code was buggy
because the sym_sir_bad_scsi_status()/S_CHECK_COND branch does not start
a new negotiation (to me it looks like the original code actually does
work if the negotiation will be PPR but not if the negotiation will be
WDTR/SDTR).

This patch forces either WDTR or PPR on every inquiry and request
sense.  I think it would be better if the negotiation was skipped if the
current agreement and the goal are both narrow/asynchronous; that way we
don't try to negotiate at all for devices that don't support wide or sync.

If the current agreement is 16-bit wide with a nonzero sync offset, and
only the sync parameters need to be renegotiated (e.g. during DV), the
original code will use just SDTR without WDTR, and this patch will use
WDTR followed by SDTR.  I agree that this is probably a good change. 
However, if the current agreement is 8-bit narrow and only the sync
parameters need to be renegotiated, then I think it would be better to
use SDTR only.  That way we don't rely on WDTR working in order to use
SDTR successfully.

To summarize, I think the following would be the best policy:

on every inquiry and request sense:
1) if both the current and goal are narrow/async, then don't negotiate
2) if the current and goal are both 8-bit narrow but with nonzero
offset, then use SDTR only
3) if either the current or goal are 16-bit wide, then use WDTR+SDTR or PPR

on commands other than inquiry and request sense:
4) if the current and goal are the same, then don't negotiate
5) if the current and goal are both 8-bit narrow but with nonzero
offset, then use SDTR only
6) if either the current or goal are 16-bit wide, then use WDTR+SDTR or PPR

If you tell sym_prepare_nego() whether or not the command is inquiry or
request sense, then it should be fairly straightforward to implement
this policy.

Tony

Re: [PATCH] sym53c8xx_2: Keep transfer negotiations valid (2.6.27.5)

[Aaro Koskinen]

Hello,

ext Tony Battersby wrote:
> On a device that negotiates asynchronous, this patch causes a WDTR
> exchange on every command (not just inquiry/request sense), even on
> devices that don't advertise wide support in the inquiry data.  This is
> unnecessary overhead, and may (in theory) cause problems for old devices
> that don't handle WDTR correctly.

Yes, this is overlooked in the patch. Also if there is no synch nego or 
WDTR is rejected, then check_nego flag will stay on forever.

> This patch forces either WDTR or PPR on every inquiry and request
> sense.  I think it would be better if the negotiation was skipped if the
> current agreement and the goal are both narrow/asynchronous; that way we
> don't try to negotiate at all for devices that don't support wide or sync.

[...]

> However, if the current agreement is 8-bit narrow and only the sync
> parameters need to be renegotiated, then I think it would be better to
> use SDTR only.  That way we don't rely on WDTR working in order to use
> SDTR successfully.

These are both valid points. I will update the patch.

A.

Re: [PATCH] sym53c8xx_2: Keep transfer negotiations valid (2.6.27.5)

[Aaro Koskinen]

(The patch updated based on testing and comments from Tony Battersby.)

Change the sym53c8xx_2 driver negotiation logic so that the driver will
tolerate better device removals. Negotiation message(s) will be sent
with every INQUIRY and REQUEST SENSE command, and whenever there is a
change in goals or when the device reports check condition.

The patch was made specifically to address the case where you hotswap
the disk using remove-single-device/add-single-device commands through
/proc/scsi/scsi. Without the patch the driver keeps using old transfer
parameters even though the target is reset and reports check condition,
so the data transfer of the very first INQUIRY will fail.

Signed-off-by: Aaro Koskinen <Aaro.Koskinen@nokia.com>
---
 drivers/scsi/sym53c8xx_2/sym_hipd.c |   35 ++++++++++++++++++++++++++---------
 drivers/scsi/sym53c8xx_2/sym_hipd.h |    1 +
 2 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.c b/drivers/scsi/sym53c8xx_2/sym_hipd.c
index 98df165..fe6359d 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.c
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.c
@@ -1433,13 +1433,12 @@ static int sym_prepare_nego(struct sym_hcb *np, struct sym_ccb *cp, u_char *msgp
 	 * Many devices implement PPR in a buggy way, so only use it if we
 	 * really want to.
 	 */
-	if (goal->offset &&
-	    (goal->iu || goal->dt || goal->qas || (goal->period < 0xa))) {
+	if (goal->renego == NS_PPR || (goal->offset &&
+	    (goal->iu || goal->dt || goal->qas || (goal->period < 0xa)))) {
 		nego = NS_PPR;
-	} else if (spi_width(starget) != goal->width) {
+	} else if (goal->renego == NS_WIDE || goal->width) {
 		nego = NS_WIDE;
-	} else if (spi_period(starget) != goal->period ||
-		   spi_offset(starget) != goal->offset) {
+	} else if (goal->renego == NS_SYNC || goal->offset) {
 		nego = NS_SYNC;
 	} else {
 		goal->check_nego = 0;
@@ -2049,11 +2048,13 @@ static void sym_setwide(struct sym_hcb *np, int target, u_char wide)
 	struct sym_tcb *tp = &np->target[target];
 	struct scsi_target *starget = tp->starget;
 
-	if (spi_width(starget) == wide)
-		return;
-
 	sym_settrans(np, target, 0, 0, 0, wide, 0, 0);
 
+	if (wide)
+		tp->tgoal.renego = NS_WIDE;
+	else
+		tp->tgoal.renego = 0;
+	tp->tgoal.check_nego = 0;
 	tp->tgoal.width = wide;
 	spi_offset(starget) = 0;
 	spi_period(starget) = 0;
@@ -2080,6 +2081,12 @@ sym_setsync(struct sym_hcb *np, int target,
 
 	sym_settrans(np, target, 0, ofs, per, wide, div, fak);
 
+	if (wide)
+		tp->tgoal.renego = NS_WIDE;
+	else if (ofs)
+		tp->tgoal.renego = NS_SYNC;
+	else
+		tp->tgoal.renego = 0;
 	spi_period(starget) = per;
 	spi_offset(starget) = ofs;
 	spi_iu(starget) = spi_dt(starget) = spi_qas(starget) = 0;
@@ -2106,6 +2113,10 @@ sym_setpprot(struct sym_hcb *np, int target, u_char opts, u_char ofs,
 
 	sym_settrans(np, target, opts, ofs, per, wide, div, fak);
 
+	if (wide || ofs)
+		tp->tgoal.renego = NS_PPR;
+	else
+		tp->tgoal.renego = 0;
 	spi_width(starget) = tp->tgoal.width = wide;
 	spi_period(starget) = tp->tgoal.period = per;
 	spi_offset(starget) = tp->tgoal.offset = ofs;
@@ -3516,6 +3527,7 @@ static void sym_sir_task_recovery(struct sym_hcb *np, int num)
 			spi_dt(starget) = 0;
 			spi_qas(starget) = 0;
 			tp->tgoal.check_nego = 1;
+			tp->tgoal.renego = 0;
 		}
 
 		/*
@@ -5135,9 +5147,14 @@ int sym_queue_scsiio(struct sym_hcb *np, struct scsi_cmnd *cmd, struct sym_ccb *
 	/*
 	 *  Build a negotiation message if needed.
 	 *  (nego_status is filled by sym_prepare_nego())
+	 *
+	 *  Always negotiate on INQUIRY and REQUEST SENSE.
+	 *
 	 */
 	cp->nego_status = 0;
-	if (tp->tgoal.check_nego && !tp->nego_cp && lp) {
+	if ((tp->tgoal.check_nego ||
+	     cmd->cmnd[0] == INQUIRY || cmd->cmnd[0] == REQUEST_SENSE) &&
+	    !tp->nego_cp && lp) {
 		msglen += sym_prepare_nego(np, cp, msgptr + msglen);
 	}
 
diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.h b/drivers/scsi/sym53c8xx_2/sym_hipd.h
index ad07880..233a3d0 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.h
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.h
@@ -354,6 +354,7 @@ struct sym_trans {
 	unsigned int dt:1;
 	unsigned int qas:1;
 	unsigned int check_nego:1;
+	unsigned int renego:2;
 };
 
 /*
-- 
1.5.4.3

Re: [PATCH] sym53c8xx_2: Keep transfer negotiations valid (2.6.27.5)

[Tony Battersby]

Aaro Koskinen wrote:
> (The patch updated based on testing and comments from Tony Battersby.)
>
> Change the sym53c8xx_2 driver negotiation logic so that the driver will
> tolerate better device removals. Negotiation message(s) will be sent
> with every INQUIRY and REQUEST SENSE command, and whenever there is a
> change in goals or when the device reports check condition.
>
> The patch was made specifically to address the case where you hotswap
> the disk using remove-single-device/add-single-device commands through
> /proc/scsi/scsi. Without the patch the driver keeps using old transfer
> parameters even though the target is reset and reports check condition,
> so the data transfer of the very first INQUIRY will fail.
>
>   
Thanks for your efforts.  I am a bit busy right now, but I'll give it a
test next week sometime.  Stay tuned.

Tony

Re: [PATCH] sym53c8xx_2: Keep transfer negotiations valid (2.6.27.5)

[Tony Battersby]

Aaro Koskinen wrote:
> (The patch updated based on testing and comments from Tony Battersby.)
>
> Change the sym53c8xx_2 driver negotiation logic so that the driver will
> tolerate better device removals. Negotiation message(s) will be sent
> with every INQUIRY and REQUEST SENSE command, and whenever there is a
> change in goals or when the device reports check condition.
>
> The patch was made specifically to address the case where you hotswap
> the disk using remove-single-device/add-single-device commands through
> /proc/scsi/scsi. Without the patch the driver keeps using old transfer
> parameters even though the target is reset and reports check condition,
> so the data transfer of the very first INQUIRY will fail.
>
> Signed-off-by: Aaro Koskinen <Aaro.Koskinen@nokia.com>
> ---
>  drivers/scsi/sym53c8xx_2/sym_hipd.c |   35 ++++++++++++++++++++++++++---------
>  drivers/scsi/sym53c8xx_2/sym_hipd.h |    1 +
>  2 files changed, 27 insertions(+), 9 deletions(-)
>
>   

Looks good.

James, please add this to the list of pending sym53c8xx patches.

Tested-by: Tony Battersby <tonyb@cybernetics.com>

Re: [PATCH] sym53c8xx_2: lun to_clear flag not re-initialized (2.6.27.5)

[Tony Battersby]

This patch works as advertised and fixes a real bug.

Tested-by: Tony Battersby <tonyb@cybernetics.com>

[PATCH 1/1] sym53c8xx_2: Fix validation (Fix hotplug support).

[michaelc]

From: Mike Christie <michaelc@cs.wisc.edu>

The patch and description is from Aaro Koskinen. He sent us the
patch against our fedora kernel, but he is short on time and did not have
time to send it upstream, so I am sending it for him so it does not sit in
just our trees.

This patch applies to scsi-fc-fixes.

>From Aaro Koskinen:

Principles for SCSI negotiation:

If the driver and a target have a different idea of the transfer
parameters, all data transfers will fail. Usually either the driver or the
target will hang during the data phase, or commands are being rejected due
to extraneous data etc.

(a) To prevent this, targets report check condition/unit attention before
any data transfer whenever there has been a device reset (only way to
invalidate all negotiations). So, this can be used to trigger
renegotiation.

(b) An exception to above, INQUIRY and REQUEST SENSE commands are always
executed immediately by targets, so with these commands the driver can
never be sure what the negotiation values are, so there must be a
renegotiation before sending the command.

Couple notes about the patch:

- It seems there is a bug in the current driver (in the beforementioned
kernels), the sym_sir_bad_scsi_status()/S_CHECK_COND branch is actually
NOT starting a new negotiation, although it definitely should. The patch
fixes that.

- The patch deletes code from sym_sir_task_recovery()/M_RESET branch. It's
unnecessary to reset negotiation status there, since any reset eventually
triggers S_CHECK_COND.

- It seems that the SPI "domain validation" consists solely of INQUIRY
commands. As a result, if (b) is followed, targets that support PPR would
never get to the point of doing PPR transfers during domain validation
(but immediately after on the next command following the dv), since each
INQUIRY re-starts the negotiation cycle. This patch solves this by making
it possible to do the negotiation cycle WIDE -> SYNC -> PPR during a
single SCSI command.

Test cases that I executed:

1. remove/add-single-device (through /proc/scsi) without physical disk
removal: OK
2. remove/add-single-device with physical disk removal & insertion: OK
3. same as (2) but add-single-device was done before the disk was ready:
this usually succeeds with "spinning disk up" msg, however, if I give the
command too early just before the hot swap led turns green I sometimes get
a selection timeout - if I then do a manual retry (another
add-single-device) it always succeeds. I'm not sure why scsi_scan.c stops
after a failure without any automatic retries but I think this behaviour
is not related to this patch.
4. rip the disk off without telling Linux, and then put it back and do
remove/add-single-device: OK

Used HW (disks & controller):

Attached devices:
Host: scsi1 Channel: 00 Id: 00 Lun: 00
  Vendor: FUJITSU  Model: MAW3073NP        Rev: 4701
  Type:   Direct-Access                    ANSI  SCSI revision: 03
Host: scsi0 Channel: 00 Id: 00 Lun: 00
  Vendor: FUJITSU  Model: MAW3073NP        Rev: 4701
  Type:   Direct-Access                    ANSI  SCSI revision: 03

03:01.0 SCSI storage controller: LSI Logic / Symbios Logic 53c1010 66MHz
Ultra3 SCSI Adapter (rev 01)
03:01.1 SCSI storage controller: LSI Logic / Symbios Logic 53c1010 66MHz
Ultra3 SCSI Adapter (rev 01)

sym0: <1010-66> rev 0x1 at pci 0000:03:01.0 irq 50
sym0: No NVRAM, ID 7, Fast-80, LVD, parity checking
sym1: <1010-66> rev 0x1 at pci 0000:03:01.1 irq 50
sym1: No NVRAM, ID 7, Fast-80, LVD, parity checking

Signed-off-by: Aaro Koskinen <aaro.koskinen@nsn.com>
Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
Cc: Matthew Wilcox <matthew@wil.cx>

---
 drivers/scsi/sym53c8xx_2/sym_hipd.c |   66 ++++++++++++++++++++++++----------
 drivers/scsi/sym53c8xx_2/sym_hipd.h |    1 +
 2 files changed, 47 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.c b/drivers/scsi/sym53c8xx_2/sym_hipd.c
index 98df165..41ae184 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.c
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.c
@@ -1419,7 +1419,8 @@ static void sym_check_goals(struct sym_hcb *np, struct scsi_target *starget,
  *  negotiation and the nego_status field of the CCB.
  *  Returns the size of the message in bytes.
  */
-static int sym_prepare_nego(struct sym_hcb *np, struct sym_ccb *cp, u_char *msgptr)
+static int sym_prepare_nego(struct sym_hcb *np, struct sym_ccb *cp,
+			    u_char *msgptr, int renego)
 {
 	struct sym_tcb *tp = &np->target[cp->target];
 	struct scsi_target *starget = tp->starget;
@@ -1433,7 +1434,9 @@ static int sym_prepare_nego(struct sym_hcb *np, struct sym_ccb *cp, u_char *msgp
 	 * Many devices implement PPR in a buggy way, so only use it if we
 	 * really want to.
 	 */
-	if (goal->offset &&
+	if (renego && goal->renego) {
+		nego = goal->renego;
+	} else if (goal->offset &&
 	    (goal->iu || goal->dt || goal->qas || (goal->period < 0xa))) {
 		nego = NS_PPR;
 	} else if (spi_width(starget) != goal->width) {
@@ -2054,6 +2057,7 @@ static void sym_setwide(struct sym_hcb *np, int target, u_char wide)
 
 	sym_settrans(np, target, 0, 0, 0, wide, 0, 0);
 
+	tp->tgoal.renego = NS_WIDE;
 	tp->tgoal.width = wide;
 	spi_offset(starget) = 0;
 	spi_period(starget) = 0;
@@ -2080,6 +2084,7 @@ sym_setsync(struct sym_hcb *np, int target,
 
 	sym_settrans(np, target, 0, ofs, per, wide, div, fak);
 
+	tp->tgoal.renego = NS_WIDE; /* SYNC will follow WIDE. */
 	spi_period(starget) = per;
 	spi_offset(starget) = ofs;
 	spi_iu(starget) = spi_dt(starget) = spi_qas(starget) = 0;
@@ -2106,6 +2111,7 @@ sym_setpprot(struct sym_hcb *np, int target, u_char opts, u_char ofs,
 
 	sym_settrans(np, target, opts, ofs, per, wide, div, fak);
 
+	tp->tgoal.renego = NS_PPR;
 	spi_width(starget) = tp->tgoal.width = wide;
 	spi_period(starget) = tp->tgoal.period = per;
 	spi_offset(starget) = tp->tgoal.offset = ofs;
@@ -3074,7 +3080,7 @@ static void sym_sir_bad_scsi_status(struct sym_hcb *np, int num, struct sym_ccb
 		 *  cp->nego_status is filled by sym_prepare_nego().
 		 */
 		cp->nego_status = 0;
-		msglen += sym_prepare_nego(np, cp, &cp->scsi_smsg2[msglen]);
+		msglen += sym_prepare_nego(np, cp, &cp->scsi_smsg2[msglen], 1);
 		/*
 		 *  Message table indirect structure.
 		 */
@@ -3498,24 +3504,12 @@ static void sym_sir_task_recovery(struct sym_hcb *np, int num)
 		/*
 		 *  If we sent a M_RESET, then a hardware reset has 
 		 *  been performed by the target.
-		 *  - Reset everything to async 8 bit
-		 *  - Tell ourself to negotiate next time :-)
 		 *  - Prepare to clear all disconnected CCBs for 
 		 *    this target from our task list (lun=task=-1)
 		 */
-		lun = -1;
 		task = -1;
 		if (np->abrt_msg[0] == M_RESET) {
-			tp->head.sval = 0;
-			tp->head.wval = np->rv_scntl3;
-			tp->head.uval = 0;
-			spi_period(starget) = 0;
-			spi_offset(starget) = 0;
-			spi_width(starget) = 0;
-			spi_iu(starget) = 0;
-			spi_dt(starget) = 0;
-			spi_qas(starget) = 0;
-			tp->tgoal.check_nego = 1;
+			lun = -1;
 		}
 
 		/*
@@ -4011,9 +4005,31 @@ static void sym_sync_nego(struct sym_hcb *np, struct sym_tcb *tp, struct sym_ccb
 	if (req) {	/* Was a request, send response. */
 		cp->nego_status = NS_SYNC;
 		OUTL_DSP(np, SCRIPTB_BA(np, sdtr_resp));
+	} else {	/* Was a response. */
+		/*
+		 * Negotiate for PPR immediately after SYNC response.
+		 */
+		if (tp->tgoal.offset &&
+		    (tp->tgoal.iu || tp->tgoal.dt || tp->tgoal.qas ||
+		     (tp->tgoal.period < 0xa))) {
+			spi_populate_ppr_msg(np->msgout, tp->tgoal.period,
+					     tp->tgoal.offset, tp->tgoal.width,
+					     (tp->tgoal.iu ? PPR_OPT_IU : 0) |
+					     (tp->tgoal.dt ? PPR_OPT_DT : 0) |
+					    (tp->tgoal.qas ? PPR_OPT_QAS : 0));
+
+			if (DEBUG_FLAGS & DEBUG_NEGO) {
+				sym_print_nego_msg(np, cp->target,
+				                   "ppr msgout", np->msgout);
+			}
+
+			cp->nego_status = NS_PPR;
+			OUTB(np, HS_PRT, HS_NEGOTIATE);
+			OUTL_DSP(np, SCRIPTB_BA(np, ppr_resp));
+			return;
+		} else
+			OUTL_DSP(np, SCRIPTA_BA(np, clrack));
 	}
-	else		/* Was a response, we are done. */
-		OUTL_DSP(np, SCRIPTA_BA(np, clrack));
 	return;
 
 reject_it:
@@ -5137,8 +5153,18 @@ int sym_queue_scsiio(struct sym_hcb *np, struct scsi_cmnd *cmd, struct sym_ccb *
 	 *  (nego_status is filled by sym_prepare_nego())
 	 */
 	cp->nego_status = 0;
-	if (tp->tgoal.check_nego && !tp->nego_cp && lp) {
-		msglen += sym_prepare_nego(np, cp, msgptr + msglen);
+	if (!tp->nego_cp && lp) {
+		int renego;
+
+		/*
+		 * Always renegotiate on INQUIRY and REQUEST SENSE.
+		 */
+		renego = (cmd->cmnd[0] == INQUIRY ||
+			  cmd->cmnd[0] == REQUEST_SENSE);
+		if (tp->tgoal.check_nego || renego) {
+			msglen += sym_prepare_nego(np, cp, msgptr + msglen,
+						   renego);
+		}
 	}
 
 	/*
diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.h b/drivers/scsi/sym53c8xx_2/sym_hipd.h
index ad07880..233a3d0 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.h
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.h
@@ -354,6 +354,7 @@ struct sym_trans {
 	unsigned int dt:1;
 	unsigned int qas:1;
 	unsigned int check_nego:1;
+	unsigned int renego:2;
 };
 
 /*
-- 
1.5.4.1

Re: [PATCH 1/1] sym53c8xx_2: Fix validation (Fix hotplug support).

[James Bottomley]

On Sun, 2008-08-17 at 15:18 -0500, michaelc@cs.wisc.edu wrote:
> From: Mike Christie <michaelc@cs.wisc.edu>
> 
> The patch and description is from Aaro Koskinen. He sent us the
> patch against our fedora kernel, but he is short on time and did not have
> time to send it upstream, so I am sending it for him so it does not sit in
> just our trees.
> 
> This patch applies to scsi-fc-fixes.

One of the things that's missing from this is really the problem it's
trying to solve ... the below is just an analysis of potential bugs in
the sym2 code.

> >From Aaro Koskinen:
> 
> Principles for SCSI negotiation:
> 
> If the driver and a target have a different idea of the transfer
> parameters, all data transfers will fail. Usually either the driver or the
> target will hang during the data phase, or commands are being rejected due
> to extraneous data etc.
> 
> (a) To prevent this, targets report check condition/unit attention before
> any data transfer whenever there has been a device reset (only way to
> invalidate all negotiations). So, this can be used to trigger
> renegotiation.
> 
> (b) An exception to above, INQUIRY and REQUEST SENSE commands are always
> executed immediately by targets, so with these commands the driver can
> never be sure what the negotiation values are, so there must be a
> renegotiation before sending the command.

Actually, not necessarily a renegotiation.  What usually happens is that
a driver simply adds in the negotiation messages to these two commands
with the parameters it thinks its currently operating at.  The specs
require confirmatory replies to this.

> Couple notes about the patch:
> 
> - It seems there is a bug in the current driver (in the beforementioned
> kernels), the sym_sir_bad_scsi_status()/S_CHECK_COND branch is actually
> NOT starting a new negotiation, although it definitely should. The patch
> fixes that.

This seems to be a bug in sym_prepare_nego(), yes ... it should do
either a PPR or WDTR/SDTR sequence ... there is a case where it won't do
anything when asked.

> - The patch deletes code from sym_sir_task_recovery()/M_RESET branch. It's
> unnecessary to reset negotiation status there, since any reset eventually
> triggers S_CHECK_COND.

It looks like a reasonable safety feature against problem drives ... I'd
be hesitant to remove it.

> - It seems that the SPI "domain validation" consists solely of INQUIRY
> commands.

Actually, no.  DV uses Inquiry solely for non DT negotiations because of
potential echo buffer problems in certain drives but it uses the echo
buffer for DT verification.

>  As a result, if (b) is followed, targets that support PPR would
> never get to the point of doing PPR transfers during domain validation
> (but immediately after on the next command following the dv), since each
> INQUIRY re-starts the negotiation cycle. This patch solves this by making
> it possible to do the negotiation cycle WIDE -> SYNC -> PPR during a
> single SCSI command.

I don't quite follow this.  The way SPI DV works is that we start out
fully async for the first INQUIRY, then move to wide async (to check no
bus width domain problems, which are the most common) then jump to the
highest speed supported by the device and transport.  i.e. we always do

async -> WDTR -> SDTR

or

async -> WDTR -> PPR

we never go

async -> WDTR -> SDTR -> PPR.

What's the actual problem you are seeing?

James

Re: [PATCH 1/1] sym53c8xx_2: Fix validation (Fix hotplug support).

[Mike Christie]

James Bottomley wrote:
> On Sun, 2008-08-17 at 15:18 -0500, michaelc@cs.wisc.edu wrote:
>> From: Mike Christie <michaelc@cs.wisc.edu>
>>
>> The patch and description is from Aaro Koskinen. He sent us the
>> patch against our fedora kernel, but he is short on time and did not have
>> time to send it upstream, so I am sending it for him so it does not sit in
>> just our trees.
>>
>> This patch applies to scsi-fc-fixes.
> 
> One of the things that's missing from this is really the problem it's
> trying to solve ... the below is just an analysis of potential bugs in
> the sym2 code.
> 

Sorry. I meant to add a link to the mail with the bug report. Here is my 
initial post:
http://marc.info/?l=linux-scsi&m=120898142212407&w=2

Basically users are trying to do a hot unplug and hotplug add of a disk. 
They will do:

1. echo 1 > /sys/block/sdb/device/delete
(or do it from proc)
2. Remove the disk physically.
3. Insert new disk.
4. Rescan from sysfs
(or from proc).
5. For the rescan we can either get:

A. inquiry from scsi_scan.c will timeout and the driver's bus reset 
funtion will do a BUS RESET (bdr failed and so we got to the bus reset 
handler). This will succeed, and when the inqiury is resent it will 
succeed and the rescan will find the device and everything is fine.

B. inquiry is failed with 0x100ff. We see this error message from 
scsi_scan.c:

scsi_scan_host_selected: <1:0:0:0>
scsi scan: INQUIRY to host 1 channel 0 id 0 lun 0
scsi scan: 1st INQUIRY failed with code 0x100ff


I will let Aaro handle the other questions because I know nothing about SPI.

Re: [PATCH 1/1] sym53c8xx_2: Fix validation (Fix hotplug support).

[James Bottomley]

On Sun, 2008-08-17 at 22:47 -0500, Mike Christie wrote:
> James Bottomley wrote:
> > On Sun, 2008-08-17 at 15:18 -0500, michaelc@cs.wisc.edu wrote:
> >> From: Mike Christie <michaelc@cs.wisc.edu>
> >>
> >> The patch and description is from Aaro Koskinen. He sent us the
> >> patch against our fedora kernel, but he is short on time and did not have
> >> time to send it upstream, so I am sending it for him so it does not sit in
> >> just our trees.
> >>
> >> This patch applies to scsi-fc-fixes.
> > 
> > One of the things that's missing from this is really the problem it's
> > trying to solve ... the below is just an analysis of potential bugs in
> > the sym2 code.
> > 
> 
> Sorry. I meant to add a link to the mail with the bug report. Here is my 
> initial post:
> http://marc.info/?l=linux-scsi&m=120898142212407&w=2
> 
> Basically users are trying to do a hot unplug and hotplug add of a disk. 
> They will do:
> 
> 1. echo 1 > /sys/block/sdb/device/delete
> (or do it from proc)
> 2. Remove the disk physically.
> 3. Insert new disk.
> 4. Rescan from sysfs
> (or from proc).
> 5. For the rescan we can either get:
> 
> A. inquiry from scsi_scan.c will timeout and the driver's bus reset 
> funtion will do a BUS RESET (bdr failed and so we got to the bus reset 
> handler). This will succeed, and when the inqiury is resent it will 
> succeed and the rescan will find the device and everything is fine.
> 
> B. inquiry is failed with 0x100ff. We see this error message from 
> scsi_scan.c:
> 
> scsi_scan_host_selected: <1:0:0:0>
> scsi scan: INQUIRY to host 1 channel 0 id 0 lun 0
> scsi scan: 1st INQUIRY failed with code 0x100ff
> 
> 
> I will let Aaro handle the other questions because I know nothing about SPI.

Actually, the report says everything goes fine if they notify the mid
layer through sysfs before doing the removal.  The problem is on
unnotified hot swap with a rescan afterwards.

The problem in the second case is the parameter mismatch.  The HBA is
thinking previous transfer parameters and the drive is thinking async.

Tacking the parameters on to the messages before inquiry and request
sense is the standards suggested way out of this, so that's what the
driver should be doing.  However, unnotified hot swap is also the
completely incorrect way of doing this.  You could end up with the
kernel connecting a device or filesystem wrongly.  Notify, remove, add
and rescan is the correct way of handling this.

James

Re: [PATCH 1/1] sym53c8xx_2: Fix validation (Fix hotplug support).

[Mike Christie]

James Bottomley wrote:
> On Sun, 2008-08-17 at 22:47 -0500, Mike Christie wrote:
>> James Bottomley wrote:
>>> On Sun, 2008-08-17 at 15:18 -0500, michaelc@cs.wisc.edu wrote:
>>>> From: Mike Christie <michaelc@cs.wisc.edu>
>>>>
>>>> The patch and description is from Aaro Koskinen. He sent us the
>>>> patch against our fedora kernel, but he is short on time and did not have
>>>> time to send it upstream, so I am sending it for him so it does not sit in
>>>> just our trees.
>>>>
>>>> This patch applies to scsi-fc-fixes.
>>> One of the things that's missing from this is really the problem it's
>>> trying to solve ... the below is just an analysis of potential bugs in
>>> the sym2 code.
>>>
>> Sorry. I meant to add a link to the mail with the bug report. Here is my 
>> initial post:
>> http://marc.info/?l=linux-scsi&m=120898142212407&w=2
>>
>> Basically users are trying to do a hot unplug and hotplug add of a disk. 
>> They will do:
>>
>> 1. echo 1 > /sys/block/sdb/device/delete
>> (or do it from proc)
>> 2. Remove the disk physically.
>> 3. Insert new disk.
>> 4. Rescan from sysfs
>> (or from proc).
>> 5. For the rescan we can either get:
>>
>> A. inquiry from scsi_scan.c will timeout and the driver's bus reset 
>> funtion will do a BUS RESET (bdr failed and so we got to the bus reset 
>> handler). This will succeed, and when the inqiury is resent it will 
>> succeed and the rescan will find the device and everything is fine.
>>
>> B. inquiry is failed with 0x100ff. We see this error message from 
>> scsi_scan.c:
>>
>> scsi_scan_host_selected: <1:0:0:0>
>> scsi scan: INQUIRY to host 1 channel 0 id 0 lun 0
>> scsi scan: 1st INQUIRY failed with code 0x100ff
>>
>>
>> I will let Aaro handle the other questions because I know nothing about SPI.
> 
> Actually, the report says everything goes fine if they notify the mid
> layer through sysfs before doing the removal.  The problem is on
> unnotified hot swap with a rescan afterwards.

Yeah, the remove part is fine. It is if we try to add a disk back in 
where we hit problems. I think I was not clear when I wrote the first mail
http://marc.info/?l=linux-scsi&m=120898142212407&w=2

When I wrote this:
"However, if we physically plug the disk back in and try to readd
it through the sysfs/proc scanning interfaces, it looks like "

I did not mean that we skipped #1 and #2 above.

> 
> The problem in the second case is the parameter mismatch.  The HBA is
> thinking previous transfer parameters and the drive is thinking async.
> 
> Tacking the parameters on to the messages before inquiry and request
> sense is the standards suggested way out of this, so that's what the
> driver should be doing.  However, unnotified hot swap is also the
> completely incorrect way of doing this.  You could end up with the
> kernel connecting a device or filesystem wrongly.  Notify, remove, add
> and rescan is the correct way of handling this.
> 

I think I am doing the sequence above and it is not working. For the 
"Notify" stage, is that when we do the echo into sysfs or proc to remove 
the device? If so that is my #1 above. Then for "Remove" that is my #2. 
And "Add" is my #3. And "Rescan" is my #4.

Or for some of those stages do you mean we need to use one of the driver 
ioctls to make sure the driver cleans up too?

[PATCH] sym53c8xx_2: Keep transfer negotiations valid (2.6.27.5)

[Koskinen Aaro (NSN - FI/Helsinki)]

I have reworked (simplified) the patch that was discussed couple months
back (http://marc.info/?l=linux-scsi&m=121900434020133&w=2). Again the
goal is to ensure that host/target transfer parameters do not go out of
sync (in the case the target is removed and added back either forcibly
or by using remove/add-single-device through /proc/scsi/scsi):

	- sym_prepare_nego() now always fills a message which is either
	  wide or ppr (the driver will do sync after wide when needed
	  as before).

	- it is called when the host changes parameters, or if the
	command is INQUIRY or REQUEST SENSE (including check cond case).

---

Change the sym53c8xx_2 driver negotiation logic so that the driver will
tolerate better device removals. Negotiation message(s) will be sent
with every INQUIRY and REQUEST SENSE command, and whenever there is a
change in goals or when the device reports check condition.

Signed-off-by: aaro.koskinen@nsn.com

---

diff -uprN -X linux-2.6.27.5-orig/Documentation/dontdiff linux-2.6.27.5-orig/drivers/scsi/sym53c8xx_2/sym_hipd.c linux-2.6.27.5/drivers/scsi/sym53c8xx_2/sym_hipd.c
--- linux-2.6.27.5-orig/drivers/scsi/sym53c8xx_2/sym_hipd.c	2008-11-07 19:55:34.000000000 +0200
+++ linux-2.6.27.5/drivers/scsi/sym53c8xx_2/sym_hipd.c	2008-11-19 12:15:05.000000000 +0200
@@ -1413,7 +1413,7 @@ static void sym_check_goals(struct sym_h
 }
 
 /*
- *  Prepare the next negotiation message if needed.
+ *  Prepare the next negotiation message.
  *
  *  Fill in the part of message buffer that contains the 
  *  negotiation and the nego_status field of the CCB.
@@ -1433,24 +1433,14 @@ static int sym_prepare_nego(struct sym_h
 	 * Many devices implement PPR in a buggy way, so only use it if we
 	 * really want to.
 	 */
-	if (goal->offset &&
-	    (goal->iu || goal->dt || goal->qas || (goal->period < 0xa))) {
+	if (goal->renego == NS_PPR || (goal->offset &&
+	    (goal->iu || goal->dt || goal->qas || (goal->period < 0xa)))) {
 		nego = NS_PPR;
-	} else if (spi_width(starget) != goal->width) {
-		nego = NS_WIDE;
-	} else if (spi_period(starget) != goal->period ||
-		   spi_offset(starget) != goal->offset) {
-		nego = NS_SYNC;
 	} else {
-		goal->check_nego = 0;
-		nego = 0;
+		nego = NS_WIDE; /* SYNC will follow WIDE if needed. */
 	}
 
 	switch (nego) {
-	case NS_SYNC:
-		msglen += spi_populate_sync_msg(msgptr + msglen, goal->period,
-				goal->offset);
-		break;
 	case NS_WIDE:
 		msglen += spi_populate_width_msg(msgptr + msglen, goal->width);
 		break;
@@ -1469,7 +1459,6 @@ static int sym_prepare_nego(struct sym_h
 		tp->nego_cp = cp; /* Keep track a nego will be performed */
 		if (DEBUG_FLAGS & DEBUG_NEGO) {
 			sym_print_nego_msg(np, cp->target, 
-					  nego == NS_SYNC ? "sync msgout" :
 					  nego == NS_WIDE ? "wide msgout" :
 					  "ppr msgout", msgptr);
 		}
@@ -2049,11 +2038,9 @@ static void sym_setwide(struct sym_hcb *
 	struct sym_tcb *tp = &np->target[target];
 	struct scsi_target *starget = tp->starget;
 
-	if (spi_width(starget) == wide)
-		return;
-
 	sym_settrans(np, target, 0, 0, 0, wide, 0, 0);
 
+	tp->tgoal.renego = NS_WIDE;
 	tp->tgoal.width = wide;
 	spi_offset(starget) = 0;
 	spi_period(starget) = 0;
@@ -2080,6 +2067,7 @@ sym_setsync(struct sym_hcb *np, int targ
 
 	sym_settrans(np, target, 0, ofs, per, wide, div, fak);
 
+	tp->tgoal.renego = NS_WIDE; /* SYNC will follow WIDE. */
 	spi_period(starget) = per;
 	spi_offset(starget) = ofs;
 	spi_iu(starget) = spi_dt(starget) = spi_qas(starget) = 0;
@@ -2106,6 +2094,7 @@ sym_setpprot(struct sym_hcb *np, int tar
 
 	sym_settrans(np, target, opts, ofs, per, wide, div, fak);
 
+	tp->tgoal.renego = NS_PPR;
 	spi_width(starget) = tp->tgoal.width = wide;
 	spi_period(starget) = tp->tgoal.period = per;
 	spi_offset(starget) = tp->tgoal.offset = ofs;
@@ -5135,9 +5124,14 @@ int sym_queue_scsiio(struct sym_hcb *np,
 	/*
 	 *  Build a negotiation message if needed.
 	 *  (nego_status is filled by sym_prepare_nego())
+	 *
+	 *  Always negotiate on INQUIRY and REQUEST SENSE.
+	 *
 	 */
 	cp->nego_status = 0;
-	if (tp->tgoal.check_nego && !tp->nego_cp && lp) {
+	if ((tp->tgoal.check_nego ||
+	     cmd->cmnd[0] == INQUIRY || cmd->cmnd[0] == REQUEST_SENSE) &&
+	    !tp->nego_cp && lp) {
 		msglen += sym_prepare_nego(np, cp, msgptr + msglen);
 	}
 
diff -uprN -X linux-2.6.27.5-orig/Documentation/dontdiff linux-2.6.27.5-orig/drivers/scsi/sym53c8xx_2/sym_hipd.h linux-2.6.27.5/drivers/scsi/sym53c8xx_2/sym_hipd.h
--- linux-2.6.27.5-orig/drivers/scsi/sym53c8xx_2/sym_hipd.h	2008-11-07 19:55:34.000000000 +0200
+++ linux-2.6.27.5/drivers/scsi/sym53c8xx_2/sym_hipd.h	2008-11-19 12:14:46.000000000 +0200
@@ -354,6 +354,7 @@ struct sym_trans {
 	unsigned int dt:1;
 	unsigned int qas:1;
 	unsigned int check_nego:1;
+	unsigned int renego:2;
 };
 
 /*

Re: [PATCH] sym53c8xx_2: Keep transfer negotiations valid (2.6.27.5)

[Aaro Koskinen]

(Resent with proper formatting.)

Change the sym53c8xx_2 driver negotiation logic so that the driver will
tolerate better device removals. Negotiation message(s) will be sent
with every INQUIRY and REQUEST SENSE command, and whenever there is a
change in goals or when the device reports check condition.

Signed-off-by: Aaro Koskinen <Aaro.Koskinen@nokia.com>
---
 drivers/scsi/sym53c8xx_2/sym_hipd.c |   32 +++++++++++++-------------------
 drivers/scsi/sym53c8xx_2/sym_hipd.h |    1 +
 2 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.c b/drivers/scsi/sym53c8xx_2/sym_hipd.c
index 98df165..2f25b89 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.c
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.c
@@ -1413,7 +1413,7 @@ static void sym_check_goals(struct sym_hcb *np, struct scsi_target *starget,
 }
 
 /*
- *  Prepare the next negotiation message if needed.
+ *  Prepare the next negotiation message.
  *
  *  Fill in the part of message buffer that contains the 
  *  negotiation and the nego_status field of the CCB.
@@ -1433,24 +1433,14 @@ static int sym_prepare_nego(struct sym_hcb *np, struct sym_ccb *cp, u_char *msgp
 	 * Many devices implement PPR in a buggy way, so only use it if we
 	 * really want to.
 	 */
-	if (goal->offset &&
-	    (goal->iu || goal->dt || goal->qas || (goal->period < 0xa))) {
+	if (goal->renego == NS_PPR || (goal->offset &&
+	    (goal->iu || goal->dt || goal->qas || (goal->period < 0xa)))) {
 		nego = NS_PPR;
-	} else if (spi_width(starget) != goal->width) {
-		nego = NS_WIDE;
-	} else if (spi_period(starget) != goal->period ||
-		   spi_offset(starget) != goal->offset) {
-		nego = NS_SYNC;
 	} else {
-		goal->check_nego = 0;
-		nego = 0;
+		nego = NS_WIDE; /* SYNC will follow WIDE if needed. */
 	}
 
 	switch (nego) {
-	case NS_SYNC:
-		msglen += spi_populate_sync_msg(msgptr + msglen, goal->period,
-				goal->offset);
-		break;
 	case NS_WIDE:
 		msglen += spi_populate_width_msg(msgptr + msglen, goal->width);
 		break;
@@ -1469,7 +1459,6 @@ static int sym_prepare_nego(struct sym_hcb *np, struct sym_ccb *cp, u_char *msgp
 		tp->nego_cp = cp; /* Keep track a nego will be performed */
 		if (DEBUG_FLAGS & DEBUG_NEGO) {
 			sym_print_nego_msg(np, cp->target, 
-					  nego == NS_SYNC ? "sync msgout" :
 					  nego == NS_WIDE ? "wide msgout" :
 					  "ppr msgout", msgptr);
 		}
@@ -2049,11 +2038,9 @@ static void sym_setwide(struct sym_hcb *np, int target, u_char wide)
 	struct sym_tcb *tp = &np->target[target];
 	struct scsi_target *starget = tp->starget;
 
-	if (spi_width(starget) == wide)
-		return;
-
 	sym_settrans(np, target, 0, 0, 0, wide, 0, 0);
 
+	tp->tgoal.renego = NS_WIDE;
 	tp->tgoal.width = wide;
 	spi_offset(starget) = 0;
 	spi_period(starget) = 0;
@@ -2080,6 +2067,7 @@ sym_setsync(struct sym_hcb *np, int target,
 
 	sym_settrans(np, target, 0, ofs, per, wide, div, fak);
 
+	tp->tgoal.renego = NS_WIDE; /* SYNC will follow WIDE. */
 	spi_period(starget) = per;
 	spi_offset(starget) = ofs;
 	spi_iu(starget) = spi_dt(starget) = spi_qas(starget) = 0;
@@ -2106,6 +2094,7 @@ sym_setpprot(struct sym_hcb *np, int target, u_char opts, u_char ofs,
 
 	sym_settrans(np, target, opts, ofs, per, wide, div, fak);
 
+	tp->tgoal.renego = NS_PPR;
 	spi_width(starget) = tp->tgoal.width = wide;
 	spi_period(starget) = tp->tgoal.period = per;
 	spi_offset(starget) = tp->tgoal.offset = ofs;
@@ -5135,9 +5124,14 @@ int sym_queue_scsiio(struct sym_hcb *np, struct scsi_cmnd *cmd, struct sym_ccb *
 	/*
 	 *  Build a negotiation message if needed.
 	 *  (nego_status is filled by sym_prepare_nego())
+	 *
+	 *  Always negotiate on INQUIRY and REQUEST SENSE.
+	 *
 	 */
 	cp->nego_status = 0;
-	if (tp->tgoal.check_nego && !tp->nego_cp && lp) {
+	if ((tp->tgoal.check_nego ||
+	     cmd->cmnd[0] == INQUIRY || cmd->cmnd[0] == REQUEST_SENSE) &&
+	    !tp->nego_cp && lp) {
 		msglen += sym_prepare_nego(np, cp, msgptr + msglen);
 	}
 
diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.h b/drivers/scsi/sym53c8xx_2/sym_hipd.h
index ad07880..233a3d0 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.h
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.h
@@ -354,6 +354,7 @@ struct sym_trans {
 	unsigned int dt:1;
 	unsigned int qas:1;
 	unsigned int check_nego:1;
+	unsigned int renego:2;
 };
 
 /*
-- 
1.5.4.3