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] sym53c8xx_2: lun to_clear flag not re-initialized (2.6.27.5)

[Koskinen Aaro (NSN - FI/Helsinki)]

Fix for the sym53c8xx_2 driver to initialize lun's to_clear flag after
a bus reset (a failed clear can trigger a bus reset and it should not
be attemped again after that).

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:20:27.000000000 +0200
@@ -1897,6 +1897,15 @@ void sym_start_up(struct Scsi_Host *shos
 		tp->head.sval = 0;
 		tp->head.wval = np->rv_scntl3;
 		tp->head.uval = 0;
+		if (tp->lun0p)
+			tp->lun0p->to_clear = 0;
+		if (tp->lunmp) {
+			int ln;
+
+			for (ln = 1; ln < SYM_CONF_MAX_LUN; ln++)
+				if (tp->lunmp[ln])
+					tp->lunmp[ln]->to_clear = 0;
+		}
 	}
 
 	/*

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

[Mike Christie]

Koskinen Aaro (NSN - FI/Helsinki) wrote:
> Fix for the sym53c8xx_2 driver to initialize lun's to_clear flag after
> a bus reset (a failed clear can trigger a bus reset and it should not
> be attemped again after that).
> 
> Signed-off-by: aaro.koskinen@nsn.com
> 

I think you just want to fix up your signed off line to be: 
Signed-off-by: Aaro Koskinen <aaro.koskinen@nsn.com>

James might to that for you when he merges it so you do not have to resend.


> ---
> 
> 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:20:27.000000000 +0200
> @@ -1897,6 +1897,15 @@ void sym_start_up(struct Scsi_Host *shos
>  		tp->head.sval = 0;
>  		tp->head.wval = np->rv_scntl3;
>  		tp->head.uval = 0;
> +		if (tp->lun0p)
> +			tp->lun0p->to_clear = 0;
> +		if (tp->lunmp) {
> +			int ln;
> +
> +			for (ln = 1; ln < SYM_CONF_MAX_LUN; ln++)
> +				if (tp->lunmp[ln])
> +					tp->lunmp[ln]->to_clear = 0;
> +		}
>  	}
>  
>  	/*
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Aaro Koskinen]

(Resent with proper formatting)

Fix for the sym53c8xx_2 driver to initialize lun's to_clear flag after
a bus reset (a failed clear can trigger a bus reset and it should not
be attemped again after that).

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

diff --git a/drivers/scsi/sym53c8xx_2/sym_hipd.c b/drivers/scsi/sym53c8xx_2/sym_hipd.c
index 98df165..b126a7a 100644
--- a/drivers/scsi/sym53c8xx_2/sym_hipd.c
+++ b/drivers/scsi/sym53c8xx_2/sym_hipd.c
@@ -1897,6 +1897,15 @@ void sym_start_up(struct Scsi_Host *shost, int reason)
 		tp->head.sval = 0;
 		tp->head.wval = np->rv_scntl3;
 		tp->head.uval = 0;
+		if (tp->lun0p)
+			tp->lun0p->to_clear = 0;
+		if (tp->lunmp) {
+			int ln;
+
+			for (ln = 1; ln < SYM_CONF_MAX_LUN; ln++)
+				if (tp->lunmp[ln])
+					tp->lunmp[ln]->to_clear = 0;
+		}
 	}
 
 	/*
-- 
1.5.4.3