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

[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).

[Koskinen Aaro (NSN - FI/Helsinki)]

James Bottomley wrote:
> 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.

To me, the code looked like it was added there to compensate the bug 
mentioned above. Anyway, it probably does no harm to leave it there.

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

Yes, you are correct.

>>  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?

If you consider this patch, the WDTR message will be resent when the DV 
jumps to the highest speed, and then, if you look at the code in 
sym_wide_nego(), the driver will also send SDTR after the target 
responds to WDTR since the goal offset is now set. So what you will get 
is WDTR -> SDTR -> PPR.

But now that I think of it, perhaps the code in sym_wide_nego() should 
already check whether to send PPR instead of SDTR if the goals are 
appropriate.

A.

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

[James Bottomley]

On Mon, 2008-08-18 at 14:25 +0300, Koskinen Aaro (NSN - FI/Helsinki)
wrote:
> James Bottomley wrote:
> > 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.
> 
> To me, the code looked like it was added there to compensate the bug 
> mentioned above. Anyway, it probably does no harm to leave it there.

Sure.

> >> - 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.
> 
> Yes, you are correct.
> 
> >>  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?
> 
> If you consider this patch, the WDTR message will be resent when the DV 
> jumps to the highest speed, and then, if you look at the code in 
> sym_wide_nego(), the driver will also send SDTR after the target 
> responds to WDTR since the goal offset is now set.

Actually, it only does this if the offset is non-zero (which is the
async signal).  We take care in SPI to try wide async after narrow
async, so the SDTR piece of this code is never excited.

Once we jump to the highest speed (assuming it exceeds SDTR goals), PPR
will be used exclusively.

> So what you will get 
> is WDTR -> SDTR -> PPR.
> 
> But now that I think of it, perhaps the code in sym_wide_nego() should 
> already check whether to send PPR instead of SDTR if the goals are 
> appropriate.

No ... these two are orthogonal.  Standards mandate either WDTR/STDR or
PPR negotiation.  Prudence (and a ton of drive problems) dictate you
always do WDTR/SDTR unless the goals can only be achieved via PPR.  So,
you should never be able to get into the sym_wide_nego() code unless
your goals are short of PPR (or the device sends an unsolicited WDTR).

James

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

[Koskinen Aaro (NSN - FI/Helsinki)]

ext James Bottomley wrote:
> 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.

The mid-layer notification has no effect on the sym driver, the result 
is exactly the same as with the unnotified hot swap.

A.

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