sym53c8xx_2: slave_alloc/destroy safety (2.6.27.5)

(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

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

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.

---
 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(-)

index d39107b..073e5b6 100644
@@ -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) {
 

index 98df165..a38ed04 100644
@@ -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)

index ad07880..2ec25e1 100644
@@ -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

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.

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.

Tony

Hello,

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

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

Hello,

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.

[...]

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

A.

(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.

---
 drivers/scsi/sym53c8xx_2/sym_hipd.c |   35 ++++++++++++++++++++++++++---------
 drivers/scsi/sym53c8xx_2/sym_hipd.h |    1 +
 2 files changed, 27 insertions(+), 9 deletions(-)

index 98df165..fe6359d 100644
@@ -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);
 	}
 

index ad07880..233a3d0 100644
@@ -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

Thanks for your efforts. I am a bit busy right now, but I'll give it a test next week sometime. Stay tuned.

Tony

Looks good.

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

This patch works as advertised and fixes a real bug.