[patch 0/6] aha152x.c - Cleanup, bugfixes, convert to accessors

[patch 0/6] aha152x.c - Cleanup, bugfixes, convert to accessors

[Boaz Harrosh]

In motivation for the !use_sg cleanup and use of accessors
I needed to do some restructuring of the aha152x.c driver.

The patchset has been tested by Randy Dunlap on a PCMCIA
hardware based on aha152x.c. And some bugs where fixed
in the process by Randy.

The problematic parts were that the driver writes all over the
scsi_cmnd members in couple of places. To untangle that I divided
the work to stages for easier review and testing. I was very 
verbose at each patch explanation, mainly for myself to try and
prove that the changes are sound. 

[patch 1/6] aha152x in debug mode
 Enabling debug prints would not compile...

[patch 2/6] aha152x.c - use bounce buffer
 Override .slave_alloc() to force use of a bounce buffer
 This fixes a kernel crash with HIGHMEM=y configs.

[patch 3/6] aha152x.c - Preliminary fixes and comments
 Some weird typos and some hard coded numbers made the code
 very hard to understand.

[patch 4/6]  aha152x.c - Clean Reset path
 On the Reset code path the driver would save, write, than 
 restore scsi_cmnd members. Here I propose a solution that
 does not need to do that.

[patch 5/6] aha152x.c - Fix check_condition code-path
  In case of a Status return from a target. The driver
  would re-queue the command with a REQUEST_SENSE read.
  Again save, write, restore. I cleaned that up to a stage
  I can be happy with. Also while at it I fixed a BUG with
  "resid".

[patch 6/6] aha152x.c - use data accessors and !use_sg cleanup
  And now everything is ready for this one.

Jams I am not getting any response from Jürgen E. Fischer which
is to my understanding the maintainer of this driver. With Randy's
testing review, could we accept these changes?

Boaz

-
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

[PATCH 1/6] aha152x.c - In debug mode

[Boaz Harrosh]

The symbol <debug_locks> conflicts with the rather global one in
include/linux/locks.h.

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>

---
 drivers/scsi/aha152x.c |   10 +++++-----
 drivers/scsi/aha152x.h |    2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index 85f2394..0afc0c9 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -289,18 +289,18 @@ static LIST_HEAD(aha152x_host_list);
 		if(spin_is_locked(&QLOCK)) { \
 			DPRINTK(debug_intr, DEBUG_LEAD "(%s:%d) already locked at %s:%d\n", CMDINFO(CURRENT_SC), __FUNCTION__, __LINE__, QLOCKER, QLOCKERL); \
 		} \
-		DPRINTK(debug_locks, DEBUG_LEAD "(%s:%d) locking\n", CMDINFO(CURRENT_SC), __FUNCTION__, __LINE__); \
+		DPRINTK(debug_locking, DEBUG_LEAD "(%s:%d) locking\n", CMDINFO(CURRENT_SC), __FUNCTION__, __LINE__); \
 		spin_lock_irqsave(&QLOCK,flags); \
-		DPRINTK(debug_locks, DEBUG_LEAD "(%s:%d) locked\n", CMDINFO(CURRENT_SC), __FUNCTION__, __LINE__); \
+		DPRINTK(debug_locking, DEBUG_LEAD "(%s:%d) locked\n", CMDINFO(CURRENT_SC), __FUNCTION__, __LINE__); \
 		QLOCKER=__FUNCTION__; \
 		QLOCKERL=__LINE__; \
 	} while(0)
 
 #define DO_UNLOCK(flags)	\
 	do { \
-		DPRINTK(debug_locks, DEBUG_LEAD "(%s:%d) unlocking (locked at %s:%d)\n", CMDINFO(CURRENT_SC), __FUNCTION__, __LINE__, QLOCKER, QLOCKERL); \
+		DPRINTK(debug_locking, DEBUG_LEAD "(%s:%d) unlocking (locked at %s:%d)\n", CMDINFO(CURRENT_SC), __FUNCTION__, __LINE__, QLOCKER, QLOCKERL); \
 		spin_unlock_irqrestore(&QLOCK,flags); \
-		DPRINTK(debug_locks, DEBUG_LEAD "(%s:%d) unlocked\n", CMDINFO(CURRENT_SC), __FUNCTION__, __LINE__); \
+		DPRINTK(debug_locking, DEBUG_LEAD "(%s:%d) unlocked\n", CMDINFO(CURRENT_SC), __FUNCTION__, __LINE__); \
 		QLOCKER="(not locked)"; \
 		QLOCKERL=0; \
 	} while(0)
@@ -3395,7 +3395,7 @@ static int aha152x_proc_info(struct Scsi_Host *shpnt, char *buffer, char **start
 	PDEBUG(debug_datai, "data in");
 	PDEBUG(debug_datao, "data out");
 	PDEBUG(debug_eh, "eh");
-	PDEBUG(debug_locks, "locks");
+	PDEBUG(debug_locking, "locks");
 	PDEBUG(debug_phases, "phases");
 
 	SPRINTF("\n");
diff --git a/drivers/scsi/aha152x.h b/drivers/scsi/aha152x.h
index d2add24..ac4bfa4 100644
--- a/drivers/scsi/aha152x.h
+++ b/drivers/scsi/aha152x.h
@@ -298,7 +298,7 @@ typedef union {
 enum {
   debug_procinfo  = 0x0001,
   debug_queue     = 0x0002,
-  debug_locks     = 0x0004,
+  debug_locking   = 0x0004,
   debug_intr      = 0x0008,
   debug_selection = 0x0010,
   debug_msgo      = 0x0020,
-- 
1.5.2.2.249.g45fd

[PATCH 2/6] aha152x.c - use bounce buffer

[Boaz Harrosh]

Cause highmem buffers to be bounced to low memory until this
driver supports highmem addresses.  Otherwise it just oopses
on NULL buffer addresses.

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>

---
 drivers/scsi/aha152x.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index 0afc0c9..c841f11 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -3474,6 +3474,12 @@ static int aha152x_proc_info(struct Scsi_Host *shpnt, char *buffer, char **start
 	return thislength < length ? thislength : length;
 }
 
+static int aha152x_adjust_queue(struct scsi_device *device)
+{
+	blk_queue_bounce_limit(device->request_queue, BLK_BOUNCE_HIGH);
+	return 0;
+}
+
 static struct scsi_host_template aha152x_driver_template = {
 	.module				= THIS_MODULE,
 	.name				= AHA152X_REVID,
@@ -3490,6 +3496,7 @@ static struct scsi_host_template aha152x_driver_template = {
 	.sg_tablesize			= SG_ALL,
 	.cmd_per_lun			= 1,
 	.use_clustering			= DISABLE_CLUSTERING,
+	.slave_alloc			= aha152x_adjust_queue,
 };
 
 #if !defined(PCMCIA)
-- 
1.5.2.2.249.g45fd

[PATCH 3/6] aha152x.c - Preliminary fixes and some comments

[Boaz Harrosh]

  hunk by hunk:
  - CHECK_CONDITION is what happens to cmnd->status >> 1
    or after status_byte() macro. But here it is used
    directly on status which means 0x1 which is an undefined
    bit in the standard. And is a status that will never
    return from a target.

  - in busfree_run at the DONE_SC phase we have 3 distinct
    operation:
	1-if(DONE_SC->SCp.phase & check_condition)
          The REQUEST_SENSE command return.
          - Restore original command
	  - Than continue to operation 3.
	2-if(DONE_SC->SCp.Status==SAM_STAT_CHECK_CONDITION)
          A regular command returned with a status.
	  - Internally re-Q a REQUEST_SENSE.
	  - Do not do operation 3.
	3-
	  - Complete the command and return it to scsi-ml
     So the 0x2 in both these operations (1,2) means the scsi
     check-condition status, hence SAM_STAT_CHECK_CONDITION

  - Here the code asks about !(DONE_SC->SCp.Status & not_issued)
    but "not_issued" is an enum belonging to the "phase" member
    and not to the Status returned from target. The reason this
    works is because not_issued==1 and Also CHECK_CONDITION==1
    (remember from hunk 1). So actually the code was asking
    !(DONE_SC->SCp.Status & CHECK_CONDITION). Which means
    "Has the status been read from target yet?"
    Staus is read at status_run(). "not_issued" is
    cleared in seldo_run() which is usually earlier than
    status_run().

  So this patch does nothing as far as assembly is concerned
  but it does let the reader understand what is going on.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>

---
 drivers/scsi/aha152x.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index c841f11..dc7d84b 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -986,7 +986,7 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 	SCpnt->scsi_done	= done;
 	SCpnt->resid 		= SCpnt->request_bufflen;
 	SCpnt->SCp.phase	= not_issued | phase;
-	SCpnt->SCp.Status	= CHECK_CONDITION;
+	SCpnt->SCp.Status	= 0x1; /* Ilegal status by SCSI standard */
 	SCpnt->SCp.Message	= 0;
 	SCpnt->SCp.have_data_in	= 0;
 	SCpnt->SCp.sent_command	= 0;
@@ -1574,12 +1574,12 @@ static void busfree_run(struct Scsi_Host *shpnt)
 			cmd->use_sg          = sc->use_sg;
 			cmd->cmd_len         = sc->cmd_len;
 
-			cmd->SCp.Status = 0x02;
+			cmd->SCp.Status = SAM_STAT_CHECK_CONDITION;
 
 			HOSTDATA(shpnt)->commands--;
 			if (!HOSTDATA(shpnt)->commands)
 				SETPORT(PORTA, 0);	/* turn led off */
-		} else if(DONE_SC->SCp.Status==0x02) {
+		} else if(DONE_SC->SCp.Status==SAM_STAT_CHECK_CONDITION) {
 #if defined(AHA152X_STAT)
 			HOSTDATA(shpnt)->busfree_with_check_condition++;
 #endif
@@ -1587,7 +1587,7 @@ static void busfree_run(struct Scsi_Host *shpnt)
 			DPRINTK(debug_eh, ERR_LEAD "CHECK CONDITION found\n", CMDINFO(DONE_SC));
 #endif
 
-			if(!(DONE_SC->SCp.Status & not_issued)) {
+			if(!(DONE_SC->SCp.phase & not_issued)) {
 				Scsi_Cmnd *ptr = DONE_SC;
 				DONE_SC=NULL;
 #if 0
-- 
1.5.2.2.249.g45fd

[PATCH 4/6] aha152x.c - Clean Reset path

[Boaz Harrosh]

  What Reset code was doing:  Save command's important/dangerous
  Info on stack. NULL those members from scsi_cmnd.
  Issue a Reset. wait for it to finish than restore members
  and return.

  What I do is save or NULL nothing. But use the "resetting"
  hint in aha152x_internal_queue() to NULL out working members
  and leave struct scsi_cmnd alone.

  The indent here looks funny but it will change/drop in last
  patch and it is clear this way what changed.
---
 drivers/scsi/aha152x.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index dc7d84b..d7ca86f 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -1022,6 +1022,14 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 	   SCp.buffer           : next buffer
 	   SCp.buffers_residual : left buffers in list
 	   SCp.phase            : current state of the command */
+
+	if(phase & resetting) {
+		SCpnt->SCp.ptr           = NULL;
+		SCpnt->SCp.this_residual = 0;
+		SCpnt->resid             = 0;
+		SCpnt->SCp.buffer           = NULL;
+		SCpnt->SCp.buffers_residual = 0;
+	} else {
 	if (SCpnt->use_sg) {
 		SCpnt->SCp.buffer           = (struct scatterlist *) SCpnt->request_buffer;
 		SCpnt->SCp.ptr              = SG_ADDRESS(SCpnt->SCp.buffer);
@@ -1033,6 +1041,7 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 		SCpnt->SCp.buffer           = NULL;
 		SCpnt->SCp.buffers_residual = 0;
 	}
+	}
 
 	DO_LOCK(flags);
 
@@ -1150,9 +1159,6 @@ static int aha152x_device_reset(Scsi_Cmnd * SCpnt)
 	DECLARE_COMPLETION(done);
 	int ret, issued, disconnected;
 	unsigned char old_cmd_len = SCpnt->cmd_len;
-	unsigned short old_use_sg = SCpnt->use_sg;
-	void *old_buffer = SCpnt->request_buffer;
-	unsigned old_bufflen = SCpnt->request_bufflen;
 	unsigned long flags;
 	unsigned long timeleft;
 
@@ -1174,9 +1180,6 @@ static int aha152x_device_reset(Scsi_Cmnd * SCpnt)
 	DO_UNLOCK(flags);
 
 	SCpnt->cmd_len         = 0;
-	SCpnt->use_sg          = 0;
-	SCpnt->request_buffer  = NULL;
-	SCpnt->request_bufflen = 0;
 
 	aha152x_internal_queue(SCpnt, &done, resetting, reset_done);
 
@@ -1189,9 +1192,6 @@ static int aha152x_device_reset(Scsi_Cmnd * SCpnt)
 	}
 
 	SCpnt->cmd_len         = old_cmd_len;
-	SCpnt->use_sg          = old_use_sg;
-  	SCpnt->request_buffer  = old_buffer;
-       	SCpnt->request_bufflen = old_bufflen;
 
 	DO_LOCK(flags);
 
-- 
1.5.2.2.249.g45fd

[PATCH 5/6] aha152x.c - Fix check_condition code-path

[Boaz Harrosh]

  check_condition code-path was similar but more
  complicated to Reset. It went like this:
  1. extra space was allocated at aha152x_scdata for mirroring
    scsi_cmnd members.
  2. At aha152x_internal_queue() every not check_condition
    (REQUEST_SENSE) command was copied to above members in
    case of error.
  3. At busfree_run() in the DONE_CS phase if a Status of
    SAM_STAT_CHECK_CONDITION was detected. The command was
    re-queued Internally using aha152x_internal_queue(,,check_condition,)
    The old command members are over written with the
    REQUEST_SENSE info.
  4. At busfree_run() in the DONE_CS phase again. If it is a
    check_condition command, info was restored from mirror
    made at first call to aha152x_internal_queue() (see 2)
    and the command is completed.

  What I did is:
  1. Allocate less space in aha152x_scdata only for the 16-byte
    original command. (which is actually not needed by scsi-ml
    anymore at this stage. But this is to much knowledge of scsi-ml)
  2. If Status == SAM_STAT_CHECK_CONDITION, then like before
     re-queue a REQUEST_SENSE command. But only now save original
     command members. (Less of them)
  3. In aha152x_internal_queue(), just like for Reset, use the
    check_condition hint to set differently the working members.
    execute the command.
  4. At busfree_run() in the DONE_CS phase again. restore needed
     members.

  While at it. This patch fixes a BUG. Old code when sending
  a REQUEST_SENSE for a failed command. Would than return with
  cmd->resid == 0 which was the status of the REQUEST_SENSE.
  The failing command resid was lost. And when would resid
  be interesting if not on a failing command?

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>

---
 drivers/scsi/aha152x.c |   55 +++++++++++++++++++++++------------------------
 1 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index d7ca86f..cf49ed5 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -552,14 +552,11 @@ struct aha152x_hostdata {
 struct aha152x_scdata {
 	Scsi_Cmnd *next;	/* next sc in queue */
 	struct completion *done;/* semaphore to block on */
-	unsigned char cmd_len;
-	unsigned char cmnd[MAX_COMMAND_SIZE];
-	unsigned short use_sg;
-	unsigned request_bufflen;
-	void *request_buffer;
+	unsigned char aha_orig_cmd_len;
+	unsigned char aha_orig_cmnd[MAX_COMMAND_SIZE];
+	int aha_orig_resid;
 };
 
-
 /* access macros for hostdata */
 
 #define HOSTDATA(shpnt)		((struct aha152x_hostdata *) &shpnt->hostdata)
@@ -997,20 +994,11 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 			return FAILED;
 		}
 	} else {
-		struct aha152x_scdata *sc;
-
 		SCpnt->host_scribble = kmalloc(sizeof(struct aha152x_scdata), GFP_ATOMIC);
 		if(SCpnt->host_scribble==0) {
 			printk(ERR_LEAD "allocation failed\n", CMDINFO(SCpnt));
 			return FAILED;
 		}
-
-		sc = SCDATA(SCpnt);
-		memcpy(sc->cmnd, SCpnt->cmnd, sizeof(sc->cmnd));
-		sc->request_buffer  = SCpnt->request_buffer;
-		sc->request_bufflen = SCpnt->request_bufflen;
-		sc->use_sg          = SCpnt->use_sg;
-		sc->cmd_len         = SCpnt->cmd_len;
 	}
 
 	SCNEXT(SCpnt)		= NULL;
@@ -1023,10 +1011,16 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 	   SCp.buffers_residual : left buffers in list
 	   SCp.phase            : current state of the command */
 
-	if(phase & resetting) {
-		SCpnt->SCp.ptr           = NULL;
-		SCpnt->SCp.this_residual = 0;
-		SCpnt->resid             = 0;
+	if (phase & (check_condition|resetting)) {
+		if (phase & check_condition) {
+			SCpnt->SCp.ptr           = SCpnt->sense_buffer;
+			SCpnt->SCp.this_residual = sizeof(SCpnt->sense_buffer);
+			SCpnt->resid             = sizeof(SCpnt->sense_buffer);
+		} else {
+			SCpnt->SCp.ptr           = NULL;
+			SCpnt->SCp.this_residual = 0;
+			SCpnt->resid             = 0;
+		}
 		SCpnt->SCp.buffer           = NULL;
 		SCpnt->SCp.buffers_residual = 0;
 	} else {
@@ -1568,11 +1562,9 @@ static void busfree_run(struct Scsi_Host *shpnt)
 #endif
 
 			/* restore old command */
-			memcpy(cmd->cmnd, sc->cmnd, sizeof(sc->cmnd));
-			cmd->request_buffer  = sc->request_buffer;
-			cmd->request_bufflen = sc->request_bufflen;
-			cmd->use_sg          = sc->use_sg;
-			cmd->cmd_len         = sc->cmd_len;
+			memcpy(cmd->cmnd, sc->aha_orig_cmnd, sizeof(cmd->cmnd));
+			cmd->cmd_len = sc->aha_orig_cmd_len;
+			cmd->resid = sc->aha_orig_resid;
 
 			cmd->SCp.Status = SAM_STAT_CHECK_CONDITION;
 
@@ -1588,12 +1580,22 @@ static void busfree_run(struct Scsi_Host *shpnt)
 #endif
 
 			if(!(DONE_SC->SCp.phase & not_issued)) {
+				struct aha152x_scdata *sc;
 				Scsi_Cmnd *ptr = DONE_SC;
 				DONE_SC=NULL;
 #if 0
 				DPRINTK(debug_eh, ERR_LEAD "requesting sense\n", CMDINFO(ptr));
 #endif
 
+				/* save old command */
+				sc = SCDATA(ptr);
+				/* It was allocated in aha152x_internal_queue? */
+				BUG_ON(!sc);
+				memcpy(sc->aha_orig_cmnd, ptr->cmnd,
+				                            sizeof(ptr->cmnd));
+				sc->aha_orig_cmd_len = ptr->cmd_len;
+				sc->aha_orig_resid = ptr->resid;
+
 				ptr->cmnd[0]         = REQUEST_SENSE;
 				ptr->cmnd[1]         = 0;
 				ptr->cmnd[2]         = 0;
@@ -1601,10 +1603,7 @@ static void busfree_run(struct Scsi_Host *shpnt)
 				ptr->cmnd[4]         = sizeof(ptr->sense_buffer);
 				ptr->cmnd[5]         = 0;
 				ptr->cmd_len         = 6;
-				ptr->use_sg          = 0; 
-				ptr->request_buffer  = ptr->sense_buffer;
-				ptr->request_bufflen = sizeof(ptr->sense_buffer);
-			
+
 				DO_UNLOCK(flags);
 				aha152x_internal_queue(ptr, NULL, check_condition, ptr->scsi_done);
 				DO_LOCK(flags);
-- 
1.5.2.2.249.g45fd

Re: [PATCH 5/6] aha152x.c - Fix check_condition code-path

[Randy Dunlap]

On Sun, 29 Jul 2007 22:27:06 +0300 Boaz Harrosh wrote:

> 
>   check_condition code-path was similar but more
>   complicated to Reset. It went like this:
>   1. extra space was allocated at aha152x_scdata for mirroring
>     scsi_cmnd members.
>   2. At aha152x_internal_queue() every not check_condition
>     (REQUEST_SENSE) command was copied to above members in
>     case of error.
>   3. At busfree_run() in the DONE_CS phase if a Status of
>     SAM_STAT_CHECK_CONDITION was detected. The command was
>     re-queued Internally using aha152x_internal_queue(,,check_condition,)
>     The old command members are over written with the
>     REQUEST_SENSE info.
>   4. At busfree_run() in the DONE_CS phase again. If it is a
>     check_condition command, info was restored from mirror
>     made at first call to aha152x_internal_queue() (see 2)
>     and the command is completed.

Since you grok all of that (above), maybe you can help here:

With these 6 patches applied, I do the following:

1.  insert PCMCIA aha152x card with SCSI drive attached (/dev/sdb4)
2.  mount -t vfat /dev/sdb4 /mnt/disk
3.  play with /mnt/disk
4.  umount /mnt/disk

Now I would like to rmmod the aha152x_cs module, but its use count
is 2.  Even if I eject the card, its use count stays at 2.
Maybe the reset or check_condition patch doesn't clean up correctly,
or one of them isn't releasing a used resource ?

(this is 2.6.23-rc1 + your 6 patches + 1 acpi seq-file throttling fix.)

[  376.206253] pccard: PCMCIA card inserted into slot 0
[  376.211235] cs: memory probe 0xdfc00000-0xdfcfffff: excluding 0xdfc00000-0xdfc0ffff 0xdfcf0000-0xdfcfffff
[  376.226859] pcmcia: registering new device pcmcia0.0
[  376.332335] aha152x: resetting bus...
[  376.689008] aha152x2: vital data: rev=1, io=0x2340 (0x2340/0x2340), irq=3, scsiid=7, reconnect=enabled, parity=enabled, synchronous=enabled, delay=100, extended translation=disabled
[  376.713448] aha152x2: trying software interrupt, ok.
[  377.713637] scsi2 : Adaptec 152x SCSI driver; $Revision: 2.7 $
[  377.857969] (scsi2:4:0) Synchronous Data Transfer Request period = 200 ns offset = 8 
[  377.867504] scsi 2:0:4:0: Direct-Access     iomega   jaz 2GB          E.17 PQ: 0 ANSI: 2
[  377.879079] sd 2:0:4:0: [sdb] Spinning up disk......<6>SysRq : Changing Loglevel
[  378.158714] Loglevel set to 8
[  378.224925] ......ready
[  378.618907] sd 2:0:4:0: [sdb] 2091050 512-byte hardware sectors (1071 MB)
[  378.628155] sd 2:0:4:0: [sdb] Write Protect is off
[  378.632944] sd 2:0:4:0: [sdb] Mode Sense: 39 00 10 08
[  378.642617] sd 2:0:4:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA
[  378.653722] sd 2:0:4:0: [sdb] 2091050 512-byte hardware sectors (1071 MB)
[  378.662927] sd 2:0:4:0: [sdb] Write Protect is off
[  378.667718] sd 2:0:4:0: [sdb] Mode Sense: 39 00 10 08
[  378.677401] sd 2:0:4:0: [sdb] Write cache: enabled, read cache: enabled, supports DPO and FUA
[  378.686028]  sdb: sdb4
[  378.723811] sd 2:0:4:0: [sdb] Attached SCSI removable disk
[  378.737794] sd 2:0:4:0: Attached scsi generic sg2 type 0
[  378.800377] modprobe used greatest stack depth: 4212 bytes left
[  378.881432] pcmcia: Detected deprecated PCMCIA ioctl usage from process: hald.
[  378.888659] pcmcia: This interface will soon be removed from the kernel; please expect breakage unless you upgrade to new tools.
[  378.900205] pcmcia: see http://www.kernel.org/pub/linux/utils/kernel/pcmcia/pcmcia.html for details.
[  388.150827] pccard: card ejected from slot 0
[  388.160260] sd 2:0:4:0: [sdb] Synchronizing SCSI cache
[  389.692020] (scsi2:4:0) cannot reuse command
[  395.689698] sd 2:0:4:0: scsi: Device offlined - not ready after error recovery
[  399.745374] (scsi2:4:0) cannot reuse command
[  405.316897] sd 2:0:4:0: scsi: Device offlined - not ready after error recovery
[  406.792484] (scsi2:4:0) cannot reuse command
[  412.428955] sd 2:0:4:0: scsi: Device offlined - not ready after error recovery
[  413.855090] (scsi2:4:0) cannot reuse command



>   What I did is:
>   1. Allocate less space in aha152x_scdata only for the 16-byte
>     original command. (which is actually not needed by scsi-ml
>     anymore at this stage. But this is to much knowledge of scsi-ml)
>   2. If Status == SAM_STAT_CHECK_CONDITION, then like before
>      re-queue a REQUEST_SENSE command. But only now save original
>      command members. (Less of them)
>   3. In aha152x_internal_queue(), just like for Reset, use the
>     check_condition hint to set differently the working members.
>     execute the command.
>   4. At busfree_run() in the DONE_CS phase again. restore needed
>      members.
> 
>   While at it. This patch fixes a BUG. Old code when sending
>   a REQUEST_SENSE for a failed command. Would than return with
>   cmd->resid == 0 which was the status of the REQUEST_SENSE.
>   The failing command resid was lost. And when would resid
>   be interesting if not on a failing command?
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> 
> ---
>  drivers/scsi/aha152x.c |   55 +++++++++++++++++++++++------------------------
>  1 files changed, 27 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
> index d7ca86f..cf49ed5 100644
> --- a/drivers/scsi/aha152x.c
> +++ b/drivers/scsi/aha152x.c
> @@ -552,14 +552,11 @@ struct aha152x_hostdata {
>  struct aha152x_scdata {
>  	Scsi_Cmnd *next;	/* next sc in queue */
>  	struct completion *done;/* semaphore to block on */
> -	unsigned char cmd_len;
> -	unsigned char cmnd[MAX_COMMAND_SIZE];
> -	unsigned short use_sg;
> -	unsigned request_bufflen;
> -	void *request_buffer;
> +	unsigned char aha_orig_cmd_len;
> +	unsigned char aha_orig_cmnd[MAX_COMMAND_SIZE];
> +	int aha_orig_resid;
>  };
>  
> -
>  /* access macros for hostdata */
>  
>  #define HOSTDATA(shpnt)		((struct aha152x_hostdata *) &shpnt->hostdata)
> @@ -997,20 +994,11 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
>  			return FAILED;
>  		}
>  	} else {
> -		struct aha152x_scdata *sc;
> -
>  		SCpnt->host_scribble = kmalloc(sizeof(struct aha152x_scdata), GFP_ATOMIC);
>  		if(SCpnt->host_scribble==0) {
>  			printk(ERR_LEAD "allocation failed\n", CMDINFO(SCpnt));
>  			return FAILED;
>  		}
> -
> -		sc = SCDATA(SCpnt);
> -		memcpy(sc->cmnd, SCpnt->cmnd, sizeof(sc->cmnd));
> -		sc->request_buffer  = SCpnt->request_buffer;
> -		sc->request_bufflen = SCpnt->request_bufflen;
> -		sc->use_sg          = SCpnt->use_sg;
> -		sc->cmd_len         = SCpnt->cmd_len;
>  	}
>  
>  	SCNEXT(SCpnt)		= NULL;
> @@ -1023,10 +1011,16 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
>  	   SCp.buffers_residual : left buffers in list
>  	   SCp.phase            : current state of the command */
>  
> -	if(phase & resetting) {
> -		SCpnt->SCp.ptr           = NULL;
> -		SCpnt->SCp.this_residual = 0;
> -		SCpnt->resid             = 0;
> +	if (phase & (check_condition|resetting)) {
> +		if (phase & check_condition) {
> +			SCpnt->SCp.ptr           = SCpnt->sense_buffer;
> +			SCpnt->SCp.this_residual = sizeof(SCpnt->sense_buffer);
> +			SCpnt->resid             = sizeof(SCpnt->sense_buffer);
> +		} else {
> +			SCpnt->SCp.ptr           = NULL;
> +			SCpnt->SCp.this_residual = 0;
> +			SCpnt->resid             = 0;
> +		}
>  		SCpnt->SCp.buffer           = NULL;
>  		SCpnt->SCp.buffers_residual = 0;
>  	} else {
> @@ -1568,11 +1562,9 @@ static void busfree_run(struct Scsi_Host *shpnt)
>  #endif
>  
>  			/* restore old command */
> -			memcpy(cmd->cmnd, sc->cmnd, sizeof(sc->cmnd));
> -			cmd->request_buffer  = sc->request_buffer;
> -			cmd->request_bufflen = sc->request_bufflen;
> -			cmd->use_sg          = sc->use_sg;
> -			cmd->cmd_len         = sc->cmd_len;
> +			memcpy(cmd->cmnd, sc->aha_orig_cmnd, sizeof(cmd->cmnd));
> +			cmd->cmd_len = sc->aha_orig_cmd_len;
> +			cmd->resid = sc->aha_orig_resid;
>  
>  			cmd->SCp.Status = SAM_STAT_CHECK_CONDITION;
>  
> @@ -1588,12 +1580,22 @@ static void busfree_run(struct Scsi_Host *shpnt)
>  #endif
>  
>  			if(!(DONE_SC->SCp.phase & not_issued)) {
> +				struct aha152x_scdata *sc;
>  				Scsi_Cmnd *ptr = DONE_SC;
>  				DONE_SC=NULL;
>  #if 0
>  				DPRINTK(debug_eh, ERR_LEAD "requesting sense\n", CMDINFO(ptr));
>  #endif
>  
> +				/* save old command */
> +				sc = SCDATA(ptr);
> +				/* It was allocated in aha152x_internal_queue? */
> +				BUG_ON(!sc);
> +				memcpy(sc->aha_orig_cmnd, ptr->cmnd,
> +				                            sizeof(ptr->cmnd));
> +				sc->aha_orig_cmd_len = ptr->cmd_len;
> +				sc->aha_orig_resid = ptr->resid;
> +
>  				ptr->cmnd[0]         = REQUEST_SENSE;
>  				ptr->cmnd[1]         = 0;
>  				ptr->cmnd[2]         = 0;
> @@ -1601,10 +1603,7 @@ static void busfree_run(struct Scsi_Host *shpnt)
>  				ptr->cmnd[4]         = sizeof(ptr->sense_buffer);
>  				ptr->cmnd[5]         = 0;
>  				ptr->cmd_len         = 6;
> -				ptr->use_sg          = 0; 
> -				ptr->request_buffer  = ptr->sense_buffer;
> -				ptr->request_bufflen = sizeof(ptr->sense_buffer);
> -			
> +
>  				DO_UNLOCK(flags);
>  				aha152x_internal_queue(ptr, NULL, check_condition, ptr->scsi_done);
>  				DO_LOCK(flags);
> -- 

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 5/6] aha152x.c - Fix check_condition code-path

[Boaz Harrosh]

Randy Dunlap wrote:
> 
> Since you grok all of that (above), maybe you can help here:
> 
> With these 6 patches applied, I do the following:
> 
> 1.  insert PCMCIA aha152x card with SCSI drive attached (/dev/sdb4)
> 2.  mount -t vfat /dev/sdb4 /mnt/disk
> 3.  play with /mnt/disk
> 4.  umount /mnt/disk
> 
> Now I would like to rmmod the aha152x_cs module, but its use count
> is 2.  Even if I eject the card, its use count stays at 2.
> Maybe the reset or check_condition patch doesn't clean up correctly,
> or one of them isn't releasing a used resource ?
> 
> (this is 2.6.23-rc1 + your 6 patches + 1 acpi seq-file throttling fix.)
> 

I had an hard look and a very careful line-by-line compare
and I can't find anything obvious. Could you do a bisect.
maybe it will give me a clue as to where to look. Also please
Enable debug prints. Maybe the driver is stuck at some state 
and does not exit.

Thanks
Boaz

Re: [PATCH 5/6] aha152x.c - Fix check_condition code-path

[Randy Dunlap]

On Tue, 31 Jul 2007 10:59:51 +0300 Boaz Harrosh wrote:

> Randy Dunlap wrote:
> > 
> > Since you grok all of that (above), maybe you can help here:
> > 
> > With these 6 patches applied, I do the following:
> > 
> > 1.  insert PCMCIA aha152x card with SCSI drive attached (/dev/sdb4)
> > 2.  mount -t vfat /dev/sdb4 /mnt/disk
> > 3.  play with /mnt/disk
> > 4.  umount /mnt/disk
> > 
> > Now I would like to rmmod the aha152x_cs module, but its use count
> > is 2.  Even if I eject the card, its use count stays at 2.
> > Maybe the reset or check_condition patch doesn't clean up correctly,
> > or one of them isn't releasing a used resource ?
> > 
> > (this is 2.6.23-rc1 + your 6 patches + 1 acpi seq-file throttling fix.)
> > 
> 
> I had an hard look and a very careful line-by-line compare
> and I can't find anything obvious. Could you do a bisect.
> maybe it will give me a clue as to where to look. Also please
> Enable debug prints. Maybe the driver is stuck at some state 
> and does not exit.

Matthew's remove_host() patch didn't affect the use-count and
non-rmmod-ability, so I'll do a quick bisect now.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 5/6] aha152x.c - Fix check_condition code-path

[Randy Dunlap]

On Tue, 31 Jul 2007 10:59:51 +0300 Boaz Harrosh wrote:

> Randy Dunlap wrote:
> > 
> > Since you grok all of that (above), maybe you can help here:
> > 
> > With these 6 patches applied, I do the following:
> > 
> > 1.  insert PCMCIA aha152x card with SCSI drive attached (/dev/sdb4)
> > 2.  mount -t vfat /dev/sdb4 /mnt/disk
> > 3.  play with /mnt/disk
> > 4.  umount /mnt/disk
> > 
> > Now I would like to rmmod the aha152x_cs module, but its use count
> > is 2.  Even if I eject the card, its use count stays at 2.
> > Maybe the reset or check_condition patch doesn't clean up correctly,
> > or one of them isn't releasing a used resource ?
> > 
> > (this is 2.6.23-rc1 + your 6 patches + 1 acpi seq-file throttling fix.)
> > 
> 
> I had an hard look and a very careful line-by-line compare
> and I can't find anything obvious. Could you do a bisect.
> maybe it will give me a clue as to where to look. Also please
> Enable debug prints. Maybe the driver is stuck at some state 
> and does not exit.

The good news is that this problem has nothing to do with this
patch series.  The bad news is that this problem is there anyway.

When I umount /mnt/disk and then eject the SCSI adapter card,
a sync-cache SCSI command is sent to the LLD and its use count
increments, but the LLD cannot send this command to the drive.
The LLD never recovers from this situation.


[   88.821277] pccard: card ejected from slot 0
[   88.830891] sd 2:0:4:0: [sdb] Synchronizing SCSI cache
[   88.836187] (scsi2:4:0) queue: f7fab1a0; cmd_len=10 pieces=0 size=0 cmnd=Synchronize Cac
he(10) 35 00 00 00 00 00 00 00 00 00
[   88.847545] (scsi-1:-1:-1) (aha152x_internal_queue:1039) locking
[   88.853528] (scsi-1:-1:-1) (aha152x_internal_queue:1039) locked
[   88.859437] (scsi-1:-1:-1) (aha152x_internal_queue:1055) unlocking (locked at aha152x_in
ternal_queue:1039)
[   88.869051] (scsi-1:-1:-1) (aha152x_internal_queue:1055) unlocked
[   89.884469] (scsi-1:-1:-1) (aha152x_abort:1113) locking
[   89.889717] (scsi-1:-1:-1) (aha152x_abort:1113) locked
[   89.894835] (scsi-1:-1:-1) (aha152x_abort:1123) unlocking (locked at aha152x_abort:1113)
[   89.902904] (scsi-1:-1:-1) (aha152x_abort:1123) unlocked
[   89.908224] (scsi2:4:0) queue: f7fab1a0; cmd_len=6 pieces=0 size=0 cmnd=Test Unit Ready 
00 00 00 00 00 00
[   89.917878] (scsi-1:-1:-1) (aha152x_internal_queue:1039) locking
[   89.923861] (scsi-1:-1:-1) (aha152x_internal_queue:1039) locked
[   89.929764] (scsi-1:-1:-1) (aha152x_internal_queue:1055) unlocking (locked at aha152x_in
ternal_queue:1039)
[   89.939375] (scsi-1:-1:-1) (aha152x_internal_queue:1055) unlocked
[   90.415863] (scsi-1:-1:-1) (aha152x_abort:1113) locking
[   90.421098] (scsi-1:-1:-1) (aha152x_abort:1113) locked
[   90.426219] (scsi-1:-1:-1) (aha152x_abort:1123) unlocking (locked at aha152x_abort:1113)
[   90.434289] (scsi-1:-1:-1) (aha152x_abort:1123) unlocked
[   90.439593] (scsi-1:-1:-1) (aha152x_device_reset:1173) locking
[   90.445412] (scsi-1:-1:-1) (aha152x_device_reset:1173) locked
[   90.451134] (scsi-1:-1:-1) (aha152x_device_reset:1176) unlocking (locked at aha152x_devi
ce_reset:1173)
[   90.460413] (scsi-1:-1:-1) (aha152x_device_reset:1176) unlocked
[   90.466319] (scsi2:4:0) queue: f7fab1a0; cmd_len=0 pieces=0 size=0 cmnd=Synchronize Cach
e(10) 35 00 00 00 00 00 00 00 00 00
[   90.477686] (scsi2:4:0) cannot reuse command

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 5/6] aha152x.c - Fix check_condition code-path

[James Bottomley]

On Tue, 2007-07-31 at 11:40 -0700, Randy Dunlap wrote:
> On Tue, 31 Jul 2007 10:59:51 +0300 Boaz Harrosh wrote:
> 
> > Randy Dunlap wrote:
> > > 
> > > Since you grok all of that (above), maybe you can help here:
> > > 
> > > With these 6 patches applied, I do the following:
> > > 
> > > 1.  insert PCMCIA aha152x card with SCSI drive attached (/dev/sdb4)
> > > 2.  mount -t vfat /dev/sdb4 /mnt/disk
> > > 3.  play with /mnt/disk
> > > 4.  umount /mnt/disk
> > > 
> > > Now I would like to rmmod the aha152x_cs module, but its use count
> > > is 2.  Even if I eject the card, its use count stays at 2.
> > > Maybe the reset or check_condition patch doesn't clean up correctly,
> > > or one of them isn't releasing a used resource ?
> > > 
> > > (this is 2.6.23-rc1 + your 6 patches + 1 acpi seq-file throttling fix.)
> > > 
> > 
> > I had an hard look and a very careful line-by-line compare
> > and I can't find anything obvious. Could you do a bisect.
> > maybe it will give me a clue as to where to look. Also please
> > Enable debug prints. Maybe the driver is stuck at some state 
> > and does not exit.
> 
> The good news is that this problem has nothing to do with this
> patch series.  The bad news is that this problem is there anyway.

So on a functionality basis you're prepared to ack this patch set on the
basis of empirical testing on the grounds that the bug predates them?

> When I umount /mnt/disk and then eject the SCSI adapter card,
> a sync-cache SCSI command is sent to the LLD and its use count
> increments, but the LLD cannot send this command to the drive.
> The LLD never recovers from this situation.
> 
> 
> [   88.821277] pccard: card ejected from slot 0
> [   88.830891] sd 2:0:4:0: [sdb] Synchronizing SCSI cache
> [   88.836187] (scsi2:4:0) queue: f7fab1a0; cmd_len=10 pieces=0 size=0 cmnd=Synchronize Cac
> he(10) 35 00 00 00 00 00 00 00 00 00
> [   88.847545] (scsi-1:-1:-1) (aha152x_internal_queue:1039) locking
> [   88.853528] (scsi-1:-1:-1) (aha152x_internal_queue:1039) locked
> [   88.859437] (scsi-1:-1:-1) (aha152x_internal_queue:1055) unlocking (locked at aha152x_in
> ternal_queue:1039)
> [   88.869051] (scsi-1:-1:-1) (aha152x_internal_queue:1055) unlocked
> [   89.884469] (scsi-1:-1:-1) (aha152x_abort:1113) locking
> [   89.889717] (scsi-1:-1:-1) (aha152x_abort:1113) locked
> [   89.894835] (scsi-1:-1:-1) (aha152x_abort:1123) unlocking (locked at aha152x_abort:1113)
> [   89.902904] (scsi-1:-1:-1) (aha152x_abort:1123) unlocked
> [   89.908224] (scsi2:4:0) queue: f7fab1a0; cmd_len=6 pieces=0 size=0 cmnd=Test Unit Ready 
> 00 00 00 00 00 00
> [   89.917878] (scsi-1:-1:-1) (aha152x_internal_queue:1039) locking
> [   89.923861] (scsi-1:-1:-1) (aha152x_internal_queue:1039) locked
> [   89.929764] (scsi-1:-1:-1) (aha152x_internal_queue:1055) unlocking (locked at aha152x_in
> ternal_queue:1039)
> [   89.939375] (scsi-1:-1:-1) (aha152x_internal_queue:1055) unlocked
> [   90.415863] (scsi-1:-1:-1) (aha152x_abort:1113) locking
> [   90.421098] (scsi-1:-1:-1) (aha152x_abort:1113) locked
> [   90.426219] (scsi-1:-1:-1) (aha152x_abort:1123) unlocking (locked at aha152x_abort:1113)
> [   90.434289] (scsi-1:-1:-1) (aha152x_abort:1123) unlocked
> [   90.439593] (scsi-1:-1:-1) (aha152x_device_reset:1173) locking
> [   90.445412] (scsi-1:-1:-1) (aha152x_device_reset:1173) locked
> [   90.451134] (scsi-1:-1:-1) (aha152x_device_reset:1176) unlocking (locked at aha152x_devi
> ce_reset:1173)
> [   90.460413] (scsi-1:-1:-1) (aha152x_device_reset:1176) unlocked
> [   90.466319] (scsi2:4:0) queue: f7fab1a0; cmd_len=0 pieces=0 size=0 cmnd=Synchronize Cach
> e(10) 35 00 00 00 00 00 00 00 00 00
> [   90.477686] (scsi2:4:0) cannot reuse command

OK ... I'll take a look into this ... it seems to be something trivial
in the state machine.

James

Re: [PATCH 5/6] aha152x.c - Fix check_condition code-path

[Randy Dunlap]

On Wed, 01 Aug 2007 08:51:14 -0500 James Bottomley wrote:

> On Tue, 2007-07-31 at 11:40 -0700, Randy Dunlap wrote:
> > On Tue, 31 Jul 2007 10:59:51 +0300 Boaz Harrosh wrote:
> > 
> > > Randy Dunlap wrote:
> > > > 
> > > > Since you grok all of that (above), maybe you can help here:
> > > > 
> > > > With these 6 patches applied, I do the following:
> > > > 
> > > > 1.  insert PCMCIA aha152x card with SCSI drive attached (/dev/sdb4)
> > > > 2.  mount -t vfat /dev/sdb4 /mnt/disk
> > > > 3.  play with /mnt/disk
> > > > 4.  umount /mnt/disk
> > > > 
> > > > Now I would like to rmmod the aha152x_cs module, but its use count
> > > > is 2.  Even if I eject the card, its use count stays at 2.
> > > > Maybe the reset or check_condition patch doesn't clean up correctly,
> > > > or one of them isn't releasing a used resource ?
> > > > 
> > > > (this is 2.6.23-rc1 + your 6 patches + 1 acpi seq-file throttling fix.)
> > > > 
> > > 
> > > I had an hard look and a very careful line-by-line compare
> > > and I can't find anything obvious. Could you do a bisect.
> > > maybe it will give me a clue as to where to look. Also please
> > > Enable debug prints. Maybe the driver is stuck at some state 
> > > and does not exit.
> > 
> > The good news is that this problem has nothing to do with this
> > patch series.  The bad news is that this problem is there anyway.
> 
> So on a functionality basis you're prepared to ack this patch set on the
> basis of empirical testing on the grounds that the bug predates them?

Yes.  Acked-and-tested-by: Randy Dunlap <randy.dunlap@oracle.com>

Thanks.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 5/6] aha152x.c - Fix check_condition code-path

[Boaz Harrosh]

Randy Dunlap wrote:
> On Wed, 01 Aug 2007 08:51:14 -0500 James Bottomley wrote:
> 
>> On Tue, 2007-07-31 at 11:40 -0700, Randy Dunlap wrote:
>>> On Tue, 31 Jul 2007 10:59:51 +0300 Boaz Harrosh wrote:
>>>
>>>> Randy Dunlap wrote:
>>>>> Since you grok all of that (above), maybe you can help here:
>>>>>
>>>>> With these 6 patches applied, I do the following:
>>>>>
>>>>> 1.  insert PCMCIA aha152x card with SCSI drive attached (/dev/sdb4)
>>>>> 2.  mount -t vfat /dev/sdb4 /mnt/disk
>>>>> 3.  play with /mnt/disk
>>>>> 4.  umount /mnt/disk
>>>>>
>>>>> Now I would like to rmmod the aha152x_cs module, but its use count
>>>>> is 2.  Even if I eject the card, its use count stays at 2.
>>>>> Maybe the reset or check_condition patch doesn't clean up correctly,
>>>>> or one of them isn't releasing a used resource ?
>>>>>
>>>>> (this is 2.6.23-rc1 + your 6 patches + 1 acpi seq-file throttling fix.)
>>>>>
>>>> I had an hard look and a very careful line-by-line compare
>>>> and I can't find anything obvious. Could you do a bisect.
>>>> maybe it will give me a clue as to where to look. Also please
>>>> Enable debug prints. Maybe the driver is stuck at some state 
>>>> and does not exit.
>>> The good news is that this problem has nothing to do with this
>>> patch series.  The bad news is that this problem is there anyway.
I was afraid of that. I will try and see what other PCMCIA drivers
are doing in this situation. It looks like the driver should bailout
much earlier in the event that the card is not present. But it lets
the request in anyway. The original driver was not written for hotplug
PCMCIA maybe that's why.

Please correct me if I'm wrong
1. pccard senses an eject coming and is doing device_unregister()
2. sd.c in sd_shutdown() is doing a Synchronize Cache.
3. The command is queued but since card is not there anymore an interrupt never
   comes and the command is just stuck.

4. After one second an abort comes in an is returned with success (line 1113)
[   88.869051] (scsi-1:-1:-1) (aha152x_internal_queue:1055) unlocked
[   89.884469] (scsi-1:-1:-1) (aha152x_abort:1113) locking

5. Now a Test Unit Ready comes in. Already a good second and more after
   the eject. The card is clearly not powered by now.
   aha152x_internal_queue should check it's own presence. and return 
   appropriate value.
   Two questions:
   a. What should be the return value from a queuecommand handler
      when the card is no longer present?
   b. What should we check to know we no longer have a card?

6. One more second passes and 2nd abort comes in.
7. Than a reset comes in. Here too Should driver check for hardware
   presence.

since ds.c is doing: p_dev->_removed=1;
before the shutdown. Maybe a solution is to have aha152x_stub.c,
which is the only one that knows of PCMCIA, Override queuecommand
and just check for p_dev->_removed==1. Something like:

-------------------------------------------------------------------------------------------------------------
diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index d30a307..5fe55d0 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -1062,7 +1063,7 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
  *  queue a command
  *
  */
-static int aha152x_queue(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *))
+int aha152x_queue(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *))
 {
 #if 0
 	if(*SCpnt->cmnd == REQUEST_SENSE) {
diff --git a/drivers/scsi/aha152x.h b/drivers/scsi/aha152x.h
index ac4bfa4..065612f 100644
--- a/drivers/scsi/aha152x.h
+++ b/drivers/scsi/aha152x.h
@@ -333,5 +333,6 @@ struct aha152x_setup {
 struct Scsi_Host *aha152x_probe_one(struct aha152x_setup *);
 void aha152x_release(struct Scsi_Host *);
 int aha152x_host_reset_host(struct Scsi_Host *);
+int aha152x_queue(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *));
 
 #endif /* _AHA152X_H */
diff --git a/drivers/scsi/pcmcia/aha152x_stub.c b/drivers/scsi/pcmcia/aha152x_stub.c
index 370802d..e2f5ea5 100644
--- a/drivers/scsi/pcmcia/aha152x_stub.c
+++ b/drivers/scsi/pcmcia/aha152x_stub.c
@@ -137,6 +137,19 @@ static void aha152x_detach(struct pcmcia_device *link)
 } /* aha152x_detach */
 
 /*====================================================================*/
+static int aha152x_queue_cs(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *))
+{
+struct pcmcia_device *p_dev;
+struct device *dev = &SCpnt->device->sdev_gendev;
+
+	p_dev = to_pcmcia_dev(dev);
+	if (p_dev->_removed==1)
+		return ENODEV;
+	
+	return aha152x_queue(SCpnt, done);
+}
+
+/*====================================================================*/
 
 #define CS_CHECK(fn, ret) \
 do { last_fn = (fn); if ((last_ret = (ret)) != 0) goto cs_failed; } while (0)
@@ -201,6 +214,7 @@ static int aha152x_config_cs(struct pcmcia_device *link)
 	goto cs_failed;
     }
 
+    host->hostt->queuecommand = aha152x_queue_cs;
     sprintf(info->node.dev_name, "scsi%d", host->host_no);
     link->dev_node = &info->node;
     info->host = host;
-------------------------------------------------------------------------------------------------------------


But you will have to totally check me out on that: to_pcmcia_dev() 
thing above.
And maybe it is plain lobotomy. I wish sd.c could Just signal the
SCSI device that it is on the shutdown path.

Or maybe my original Question. How can a card identify it's un-presence?

>> So on a functionality basis you're prepared to ack this patch set on the
>> basis of empirical testing on the grounds that the bug predates them?
> 
> Yes.  Acked-and-tested-by: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Thanks.
Thank you Randy.
Boaz

> ---
> ~Randy
> *** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 5/6] aha152x.c - Fix check_condition code-path

[Randy Dunlap]

On Thu, 02 Aug 2007 14:26:47 +0300 Boaz Harrosh wrote:

> Randy Dunlap wrote:
> > On Wed, 01 Aug 2007 08:51:14 -0500 James Bottomley wrote:
> > 
> >> On Tue, 2007-07-31 at 11:40 -0700, Randy Dunlap wrote:
> >>> On Tue, 31 Jul 2007 10:59:51 +0300 Boaz Harrosh wrote:
> >>>
> >>>> Randy Dunlap wrote:
> >>>>> Since you grok all of that (above), maybe you can help here:
> >>>>>
> >>>>> With these 6 patches applied, I do the following:
> >>>>>
> >>>>> 1.  insert PCMCIA aha152x card with SCSI drive attached (/dev/sdb4)
> >>>>> 2.  mount -t vfat /dev/sdb4 /mnt/disk
> >>>>> 3.  play with /mnt/disk
> >>>>> 4.  umount /mnt/disk
> >>>>>
> >>>>> Now I would like to rmmod the aha152x_cs module, but its use count
> >>>>> is 2.  Even if I eject the card, its use count stays at 2.
> >>>>> Maybe the reset or check_condition patch doesn't clean up correctly,
> >>>>> or one of them isn't releasing a used resource ?
> >>>>>
> >>>>> (this is 2.6.23-rc1 + your 6 patches + 1 acpi seq-file throttling fix.)
> >>>>>
> >>>> I had an hard look and a very careful line-by-line compare
> >>>> and I can't find anything obvious. Could you do a bisect.
> >>>> maybe it will give me a clue as to where to look. Also please
> >>>> Enable debug prints. Maybe the driver is stuck at some state 
> >>>> and does not exit.
> >>> The good news is that this problem has nothing to do with this
> >>> patch series.  The bad news is that this problem is there anyway.
> I was afraid of that. I will try and see what other PCMCIA drivers
> are doing in this situation. It looks like the driver should bailout
> much earlier in the event that the card is not present. But it lets
> the request in anyway. The original driver was not written for hotplug
> PCMCIA maybe that's why.

Yes, Christoph would like to see the ISA & PCMCIA drivers as separate
drivers, using the correct driver models, and not using that ugly
#include .c source file trick.


> Please correct me if I'm wrong
> 1. pccard senses an eject coming and is doing device_unregister()
> 2. sd.c in sd_shutdown() is doing a Synchronize Cache.
> 3. The command is queued but since card is not there anymore an interrupt never
>    comes and the command is just stuck.
> 
> 4. After one second an abort comes in an is returned with success (line 1113)
> [   88.869051] (scsi-1:-1:-1) (aha152x_internal_queue:1055) unlocked
> [   89.884469] (scsi-1:-1:-1) (aha152x_abort:1113) locking

This all sounds correct to me (AFAIK).

> 5. Now a Test Unit Ready comes in. Already a good second and more after
>    the eject. The card is clearly not powered by now.
>    aha152x_internal_queue should check it's own presence. and return 
>    appropriate value.
>    Two questions:
>    a. What should be the return value from a queuecommand handler
>       when the card is no longer present?
>    b. What should we check to know we no longer have a card?

Anyone??

> 6. One more second passes and 2nd abort comes in.
> 7. Than a reset comes in. Here too Should driver check for hardware
>    presence.
> 
> since ds.c is doing: p_dev->_removed=1;
> before the shutdown. Maybe a solution is to have aha152x_stub.c,
> which is the only one that knows of PCMCIA, Override queuecommand
> and just check for p_dev->_removed==1. Something like:
> 
> 
>  -------------------------------------------------------------------------------------------------------------
> diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
> index d30a307..5fe55d0 100644
> --- a/drivers/scsi/aha152x.c
> +++ b/drivers/scsi/aha152x.c
> @@ -1062,7 +1063,7 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
>   *  queue a command
>   *
>   */
> -static int aha152x_queue(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *))
> +int aha152x_queue(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *))
>  {
>  #if 0
>  	if(*SCpnt->cmnd == REQUEST_SENSE) {
> diff --git a/drivers/scsi/aha152x.h b/drivers/scsi/aha152x.h
> index ac4bfa4..065612f 100644
> --- a/drivers/scsi/aha152x.h
> +++ b/drivers/scsi/aha152x.h
> @@ -333,5 +333,6 @@ struct aha152x_setup {
>  struct Scsi_Host *aha152x_probe_one(struct aha152x_setup *);
>  void aha152x_release(struct Scsi_Host *);
>  int aha152x_host_reset_host(struct Scsi_Host *);
> +int aha152x_queue(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *));
>  
>  #endif /* _AHA152X_H */
> diff --git a/drivers/scsi/pcmcia/aha152x_stub.c b/drivers/scsi/pcmcia/aha152x_stub.c
> index 370802d..e2f5ea5 100644
> --- a/drivers/scsi/pcmcia/aha152x_stub.c
> +++ b/drivers/scsi/pcmcia/aha152x_stub.c
> @@ -137,6 +137,19 @@ static void aha152x_detach(struct pcmcia_device *link)
>  } /* aha152x_detach */
>  
>  /*====================================================================*/
> +static int aha152x_queue_cs(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *))
> +{
> +struct pcmcia_device *p_dev;
> +struct device *dev = &SCpnt->device->sdev_gendev;
> +
> +	p_dev = to_pcmcia_dev(dev);
> +	if (p_dev->_removed==1)
> +		return ENODEV;
> +	
> +	return aha152x_queue(SCpnt, done);
> +}
> +
> +/*====================================================================*/
>  
>  #define CS_CHECK(fn, ret) \
>  do { last_fn = (fn); if ((last_ret = (ret)) != 0) goto cs_failed; } while (0)
> @@ -201,6 +214,7 @@ static int aha152x_config_cs(struct pcmcia_device *link)
>  	goto cs_failed;
>      }
>  
> +    host->hostt->queuecommand = aha152x_queue_cs;
>      sprintf(info->node.dev_name, "scsi%d", host->host_no);
>      link->dev_node = &info->node;
>      info->host = host;
> -------------------------------------------------------------------------------------------------------------

This makes sense, but it didn't work:

[   68.771566] pccard: card ejected from slot 0
[   68.781006] sd 2:0:4:0: [sdb] Synchronizing SCSI cache
[   68.786311] (scsi2:4:0) queue: f7f1a950; cmd_len=10 pieces=0 size=0 cmnd=Synchronize Cache(10) 35 00 00 00 00 00 00 00 00 00
[   68.797671] (scsi2:4:0) queue-simple: dir=3, buffer=sglist=00000000, ptr=ADDR=00000000, this_resid=0, bufs_resid=0
[   68.807976] (scsi-1:-1:-1) (aha152x_internal_queue:1075) locking
[   68.813957] (scsi-1:-1:-1) (aha152x_internal_queue:1075) locked
[   68.819853] (scsi-1:-1:-1) expecting:  (busfree)
[   68.824460] (scsi-1:-1:-1) (aha152x_internal_queue:1091) unlocking (locked at aha152x_internal_queue:1075)
[   68.834072] (scsi-1:-1:-1) (aha152x_internal_queue:1091) unlocked
[   69.925905] (scsi2:4:0) abort(f7f1a950)<7>(scsi-1:-1:-1) (show_queues:3041) locking
[   69.933592] (scsi-1:-1:-1) (show_queues:3041) locked
[   69.938538] queue status:
[   69.938539] issue_SC:
[   69.944902] sd 2:0:4:0: f7f1a950: cmnd=(Synchronize Cache(10) 35 00 00 00 00 00 00 00 00 00
[   69.953403] ); request_bufflen=0; resid=0; phase |not issued|; next=0x00000000
[   69.960624] (scsi-1:-1:-1) (show_queues:3045) unlocking (locked at show_queues:3041)
[   69.968360] (scsi-1:-1:-1) (show_queues:3045) unlocked
[   69.973488] current_SC:
[   69.975936] none
[   69.977781] disconnected_SC:
[   70.047619] enabled interrupts ( ENSELDO ENSELDI ENSELINGO ENSWRAP ENSDONE ENSPIORDY ENDMADONE ENSELTIMO ENATNTARG ENPHASEMIS ENBUSFREE ENSCSIPERR ENPHASECHG ENREQINIT )
[   70.063021] (scsi-1:-1:-1) (aha152x_abort:1149) locking
[   70.068236] (scsi-1:-1:-1) (aha152x_abort:1149) locked
[   70.073353] (scsi2:4:0) not yet issued - SUCCESS
[   70.077953] (scsi-1:-1:-1) (aha152x_abort:1159) unlocking (locked at aha152x_abort:1149)
[   70.086020] (scsi-1:-1:-1) (aha152x_abort:1159) unlocked
[   70.091325] (scsi2:4:0) queue: f7f1a950; cmd_len=6 pieces=0 size=0 cmnd=Test Unit Ready 00 00 00 00 00 00
[   70.100971] (scsi2:4:0) queue-simple: dir=3, buffer=sglist=00000000, ptr=ADDR=00000000, this_resid=0, bufs_resid=0
[   70.111274] (scsi-1:-1:-1) (aha152x_internal_queue:1075) locking
[   70.117256] (scsi-1:-1:-1) (aha152x_internal_queue:1075) locked
[   70.123152] (scsi-1:-1:-1) expecting:  (busfree)
[   70.127756] (scsi-1:-1:-1) (aha152x_internal_queue:1091) unlocking (locked at aha152x_internal_queue:1075)
[   70.137368] (scsi-1:-1:-1) (aha152x_internal_queue:1091) unlocked
[   71.127877] (scsi2:4:0) abort(f7f1a950)<7>(scsi-1:-1:-1) (show_queues:3041) locking
[   71.135588] (scsi-1:-1:-1) (show_queues:3041) locked
[   71.140534] queue status:
[   71.140535] issue_SC:
[   71.146895] sd 2:0:4:0: f7f1a950: cmnd=(Test Unit Ready 00 00 00 00 00 00
[   71.153777] ); request_bufflen=0; resid=0; phase |not issued|; next=0x00000000
[   71.160999] (scsi-1:-1:-1) (show_queues:3045) unlocking (locked at show_queues:3041)
[   71.168759] (scsi-1:-1:-1) (show_queues:3045) unlocked
[   71.173887] current_SC:
[   71.176336] none
[   71.178174] disconnected_SC:
[   71.248007] enabled interrupts ( ENSELDO ENSELDI ENSELINGO ENSWRAP ENSDONE ENSPIORDY ENDMADONE ENSELTIMO ENATNTARG ENPHASEMIS ENBUSFREE ENSCSIPERR ENPHASECHG ENREQINIT )
[   71.263415] (scsi-1:-1:-1) (aha152x_abort:1149) locking
[   71.268632] (scsi-1:-1:-1) (aha152x_abort:1149) locked
[   71.273748] (scsi2:4:0) not yet issued - SUCCESS
[   71.278350] (scsi-1:-1:-1) (aha152x_abort:1159) unlocking (locked at aha152x_abort:1149)
[   71.286436] (scsi-1:-1:-1) (aha152x_abort:1159) unlocked
[   71.291750] (scsi2:4:0) aha152x_device_reset(f7f1a950)<7>(scsi-1:-1:-1) (show_queues:3041) locking
[   71.300726] (scsi-1:-1:-1) (show_queues:3041) locked
[   71.305669] queue status:
[   71.305669] issue_SC:
[   71.312029] (scsi-1:-1:-1) (show_queues:3045) unlocking (locked at show_queues:3041)
[   71.319769] (scsi-1:-1:-1) (show_queues:3045) unlocked
[   71.324898] current_SC:
[   71.327340] none
[   71.329182] disconnected_SC:
[   71.399049] enabled interrupts ( ENSELDO ENSELDI ENSELINGO ENSWRAP ENSDONE ENSPIORDY ENDMADONE ENSELTIMO ENATNTARG ENPHASEMIS ENBUSFREE ENSCSIPERR ENPHASECHG ENREQINIT )
[   71.414455] (scsi-1:-1:-1) (aha152x_device_reset:1206) locking
[   71.420274] (scsi-1:-1:-1) (aha152x_device_reset:1206) locked
[   71.425996] (scsi-1:-1:-1) (aha152x_device_reset:1209) unlocking (locked at aha152x_device_reset:1206)
[   71.435275] (scsi-1:-1:-1) (aha152x_device_reset:1209) unlocked
[   71.441208] (scsi2:4:0) queue: f7f1a950; cmd_len=0 pieces=0 size=0 cmnd=Synchronize Cache(10) 35 00 00 00 00 00 00 00 00 00
[   71.452569] (scsi2:4:0) cannot reuse command
[   75.237817] (scsi-1:-1:-1) (aha152x_device_reset:1218) locking
[   75.243664] (scsi-1:-1:-1) (aha152x_device_reset:1218) locked
[   75.249387] (scsi-1:-1:-1) (aha152x_device_reset:1220) unlocking (locked at aha152x_device_reset:1218)
[   75.258686] (scsi-1:-1:-1) (aha152x_device_reset:1220) unlocked
[   75.264592] (scsi-1:-1:-1) (aha152x_device_reset:1225) locking
[   75.270414] (scsi-1:-1:-1) (aha152x_device_reset:1225) locked
[   75.276137] (scsi-1:-1:-1) (aha152x_device_reset:1246) unlocking (locked at aha152x_device_reset:1225)
[   75.285417] (scsi-1:-1:-1) (aha152x_device_reset:1246) unlocked
[   75.291345] (scsi-1:-1:-1) (aha152x_bus_reset_host:1285) locking
[   75.297340] (scsi-1:-1:-1) (aha152x_bus_reset_host:1285) locked
[   75.303237] scsi2: bus reset<7>(scsi-1:-1:-1) (show_queues:3041) already locked at aha152x_bus_reset_host:1285
[   75.313213] (scsi-1:-1:-1) (show_queues:3041) locking
[   75.318245] BUG: spinlock recursion on CPU#0, scsi_eh_2/6093
[   75.323882]  lock: f7f4a518, .magic: dead4ead, .owner: scsi_eh_2/6093, .owner_cpu: 0
[   75.331591]  [<c0103644>] show_trace_log_lvl+0x1a/0x2f
[   75.336727]  [<c0104031>] show_trace+0x12/0x14
[   75.341170]  [<c0104049>] dump_stack+0x16/0x18
[   75.345615]  [<c01d780a>] spin_bug+0x94/0xdf
[   75.349885]  [<c01d78f7>] _raw_spin_lock+0x35/0x102
[   75.354759]  [<c033d5f0>] _spin_lock_irqsave+0xc/0x11
[   75.359811]  [<f8b78490>] show_queues+0x122/0x478 [aha152x_cs]
[   75.365643]  [<f8b78c72>] aha152x_bus_reset_host+0x1d0/0x3af [aha152x_cs]
[   75.372422]  [<f8b78ece>] aha152x_bus_reset+0xc/0xe [aha152x_cs]
[   75.378423]  [<c027552d>] scsi_try_bus_reset+0x46/0x9a
[   75.383558]  [<c0275f49>] scsi_eh_ready_devs+0x26d/0x41b
[   75.388865]  [<c02768fe>] scsi_error_handler+0x2d6/0x460
[   75.394174]  [<c0129512>] kthread+0x3b/0x61
[   75.398356]  [<c01032bf>] kernel_thread_helper+0x7/0x10
[   75.403580]  =======================
[  211.336753] BUG: spinlock lockup on CPU#0, scsi_eh_2/6093, f7f4a518
[  211.342993]  [<c0103644>] show_trace_log_lvl+0x1a/0x2f
[  211.348127]  [<c0104031>] show_trace+0x12/0x14
[  211.352571]  [<c0104049>] dump_stack+0x16/0x18
[  211.357018]  [<c01d799c>] _raw_spin_lock+0xda/0x102
[  211.361893]  [<c033d5f0>] _spin_lock_irqsave+0xc/0x11
[  211.366943]  [<f8b78490>] show_queues+0x122/0x478 [aha152x_cs]
[  211.372770]  [<f8b78c72>] aha152x_bus_reset_host+0x1d0/0x3af [aha152x_cs]
[  211.379550]  [<f8b78ece>] aha152x_bus_reset+0xc/0xe [aha152x_cs]
[  211.385552]  [<c027552d>] scsi_try_bus_reset+0x46/0x9a
[  211.390686]  [<c0275f49>] scsi_eh_ready_devs+0x26d/0x41b
[  211.395994]  [<c02768fe>] scsi_error_handler+0x2d6/0x460
[  211.401302]  [<c0129512>] kthread+0x3b/0x61
[  211.405483]  [<c01032bf>] kernel_thread_helper+0x7/0x10
[  211.410705]  =======================



> But you will have to totally check me out on that: to_pcmcia_dev() 
> thing above.

Looks OK to me.

> And maybe it is plain lobotomy. I wish sd.c could Just signal the
> SCSI device that it is on the shutdown path.
> 
> Or maybe my original Question. How can a card identify it's un-presence?


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 5/6] aha152x.c - Fix check_condition code-path

[James Bottomley]

On Thu, 2007-08-02 at 12:09 -0700, Randy Dunlap wrote:
> > 5. Now a Test Unit Ready comes in. Already a good second and more after
> >    the eject. The card is clearly not powered by now.
> >    aha152x_internal_queue should check it's own presence. and return 
> >    appropriate value.
> >    Two questions:
> >    a. What should be the return value from a queuecommand handler
> >       when the card is no longer present?
> >    b. What should we check to know we no longer have a card?
> 
> Anyone??

a. Yes, it should return DID_NO_CONNECT via the command result code (as
in set result to this shifted correctly and return 0).
b. no idea ... that depends on the card.

James

Re: [PATCH 5/6] aha152x.c - Fix check_condition code-path

[Jürgen E. Fischer]

Hi Randy,

On Thu, Aug 02, 2007 at 12:09:26 -0700, Randy Dunlap wrote:
> > 6. One more second passes and 2nd abort comes in.
> > 7. Than a reset comes in. Here too Should driver check for hardware
> >    presence.
> > 
> > since ds.c is doing: p_dev->_removed=1;
> > before the shutdown. Maybe a solution is to have aha152x_stub.c,
> > which is the only one that knows of PCMCIA, Override queuecommand
> > and just check for p_dev->_removed==1. Something like:

> This makes sense, but it didn't work:

Looks like that's just because aha152x_bus_reset_host locks up when
debugging output is on.


--- a/aha152x.c	2007-08-02 21:27:12.756833348 +0200
+++ b/aha152x.c	2007-08-02 21:28:12.320227674 +0200
@@ -1102,7 +1102,7 @@
 
 #if defined(AHA152X_DEBUG)
 	if(HOSTDATA(shpnt)->debug & debug_eh) {
-		printk(DEBUG_LEAD "abort(%p)", CMDINFO(SCpnt), SCpnt);
+		printk(DEBUG_LEAD "abort(%p)\n", CMDINFO(SCpnt), SCpnt);
 		show_queues(shpnt);
 	}
 #endif
@@ -1267,15 +1267,15 @@
 {
 	unsigned long flags;
 
-	DO_LOCK(flags);
-
 #if defined(AHA152X_DEBUG)
 	if(HOSTDATA(shpnt)->debug & debug_eh) {
-		printk(KERN_DEBUG "scsi%d: bus reset", shpnt->host_no);
+		printk(KERN_DEBUG "scsi%d: bus reset\n", shpnt->host_no);
 		show_queues(shpnt);
 	}
 #endif
 
+	DO_LOCK(flags);
+
 	free_hard_reset_SCs(shpnt, &ISSUE_SC);
 	free_hard_reset_SCs(shpnt, &DISCONNECTED_SC);
 

Re: [PATCH 5/6] aha152x.c - Fix check_condition code-path

[Randy Dunlap]

On Thu, 2 Aug 2007 12:09:26 -0700 Randy Dunlap wrote:

 -------------------------------------------------------------------------------------------------------------
> > diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
> > index d30a307..5fe55d0 100644
> > --- a/drivers/scsi/aha152x.c
> > +++ b/drivers/scsi/aha152x.c
> > @@ -1062,7 +1063,7 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
> >   *  queue a command
> >   *
> >   */
> > -static int aha152x_queue(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *))
> > +int aha152x_queue(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *))
> >  {
> >  #if 0
> >  	if(*SCpnt->cmnd == REQUEST_SENSE) {
> > diff --git a/drivers/scsi/aha152x.h b/drivers/scsi/aha152x.h
> > index ac4bfa4..065612f 100644
> > --- a/drivers/scsi/aha152x.h
> > +++ b/drivers/scsi/aha152x.h
> > @@ -333,5 +333,6 @@ struct aha152x_setup {
> >  struct Scsi_Host *aha152x_probe_one(struct aha152x_setup *);
> >  void aha152x_release(struct Scsi_Host *);
> >  int aha152x_host_reset_host(struct Scsi_Host *);
> > +int aha152x_queue(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *));
> >  
> >  #endif /* _AHA152X_H */
> > diff --git a/drivers/scsi/pcmcia/aha152x_stub.c b/drivers/scsi/pcmcia/aha152x_stub.c
> > index 370802d..e2f5ea5 100644
> > --- a/drivers/scsi/pcmcia/aha152x_stub.c
> > +++ b/drivers/scsi/pcmcia/aha152x_stub.c
> > @@ -137,6 +137,19 @@ static void aha152x_detach(struct pcmcia_device *link)
> >  } /* aha152x_detach */
> >  
> >  /*====================================================================*/
> > +static int aha152x_queue_cs(Scsi_Cmnd *SCpnt, void (*done)(Scsi_Cmnd *))
> > +{
> > +struct pcmcia_device *p_dev;
> > +struct device *dev = &SCpnt->device->sdev_gendev;
> > +
> > +	p_dev = to_pcmcia_dev(dev);
> > +	if (p_dev->_removed==1)
> > +		return ENODEV;
> > +	
> > +	return aha152x_queue(SCpnt, done);
> > +}
> > +
> > +/*====================================================================*/
> >  
> >  #define CS_CHECK(fn, ret) \
> >  do { last_fn = (fn); if ((last_ret = (ret)) != 0) goto cs_failed; } while (0)
> > @@ -201,6 +214,7 @@ static int aha152x_config_cs(struct pcmcia_device *link)
> >  	goto cs_failed;
> >      }
> >  
> > +    host->hostt->queuecommand = aha152x_queue_cs;
> >      sprintf(info->node.dev_name, "scsi%d", host->host_no);
> >      link->dev_node = &info->node;
> >      info->host = host;
> > -------------------------------------------------------------------------------------------------------------
> 
> 
> > But you will have to totally check me out on that: to_pcmcia_dev() 
> > thing above.
> 
> Looks OK to me.

but it must not be.  p_dev->_removed is never seen ==1,
so I printk() p_dev above and in drivers/pcmcia/ds.c when
_removed = 1; // is being set.
and they are not the same struct pcmcia_device.

If we can get this working....

> > And maybe it is plain lobotomy. I wish sd.c could Just signal the
> > SCSI device that it is on the shutdown path.
> > 
> > Or maybe my original Question. How can a card identify it's un-presence?


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

[PATCH 6/6] aha152x.c - use data accessors and !use_sg cleanup

[Boaz Harrosh]

  And finally this is the regular !use_sg cleanup
  and use of data accessors.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>

---
 drivers/scsi/aha152x.c |   91 +++++++++++++++++++++++++-----------------------
 1 files changed, 47 insertions(+), 44 deletions(-)

diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index cf49ed5..d30a307 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -322,6 +322,12 @@ static LIST_HEAD(aha152x_host_list);
                         (cmd) ? ((cmd)->device->id & 0x0f) : -1, \
 			(cmd) ? ((cmd)->device->lun & 0x07) : -1
 
+static inline void
+CMD_INC_RESID(struct scsi_cmnd *cmd, int inc)
+{
+	scsi_set_resid(cmd, scsi_get_resid(cmd) + inc);
+}
+
 #define DELAY_DEFAULT 1000
 
 #if defined(PCMCIA)
@@ -975,13 +981,13 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 #if defined(AHA152X_DEBUG)
 	if (HOSTDATA(shpnt)->debug & debug_queue) {
 		printk(INFO_LEAD "queue: %p; cmd_len=%d pieces=%d size=%u cmnd=",
-		       CMDINFO(SCpnt), SCpnt, SCpnt->cmd_len, SCpnt->use_sg, SCpnt->request_bufflen);
+		       CMDINFO(SCpnt), SCpnt, SCpnt->cmd_len,
+		       scsi_sg_count(SCpnt), scsi_bufflen(SCpnt));
 		__scsi_print_command(SCpnt->cmnd);
 	}
 #endif
 
 	SCpnt->scsi_done	= done;
-	SCpnt->resid 		= SCpnt->request_bufflen;
 	SCpnt->SCp.phase	= not_issued | phase;
 	SCpnt->SCp.Status	= 0x1; /* Ilegal status by SCSI standard */
 	SCpnt->SCp.Message	= 0;
@@ -1011,30 +1017,24 @@ static int aha152x_internal_queue(Scsi_Cmnd *SCpnt, struct completion *complete,
 	   SCp.buffers_residual : left buffers in list
 	   SCp.phase            : current state of the command */
 
-	if (phase & (check_condition|resetting)) {
+	if ((phase & (check_condition|resetting)) || !scsi_sglist(SCpnt)) {
 		if (phase & check_condition) {
 			SCpnt->SCp.ptr           = SCpnt->sense_buffer;
 			SCpnt->SCp.this_residual = sizeof(SCpnt->sense_buffer);
-			SCpnt->resid             = sizeof(SCpnt->sense_buffer);
+			scsi_set_resid(SCpnt, sizeof(SCpnt->sense_buffer));
 		} else {
 			SCpnt->SCp.ptr           = NULL;
 			SCpnt->SCp.this_residual = 0;
-			SCpnt->resid             = 0;
+			scsi_set_resid(SCpnt, 0);
 		}
 		SCpnt->SCp.buffer           = NULL;
 		SCpnt->SCp.buffers_residual = 0;
 	} else {
-	if (SCpnt->use_sg) {
-		SCpnt->SCp.buffer           = (struct scatterlist *) SCpnt->request_buffer;
+		scsi_set_resid(SCpnt, scsi_bufflen(SCpnt));
+		SCpnt->SCp.buffer           = scsi_sglist(SCpnt);
 		SCpnt->SCp.ptr              = SG_ADDRESS(SCpnt->SCp.buffer);
 		SCpnt->SCp.this_residual    = SCpnt->SCp.buffer->length;
-		SCpnt->SCp.buffers_residual = SCpnt->use_sg - 1;
-	} else {
-		SCpnt->SCp.ptr              = (char *) SCpnt->request_buffer;
-		SCpnt->SCp.this_residual    = SCpnt->request_bufflen;
-		SCpnt->SCp.buffer           = NULL;
-		SCpnt->SCp.buffers_residual = 0;
-	}
+		SCpnt->SCp.buffers_residual = scsi_sg_count(SCpnt) - 1;
 	}
 
 	DO_LOCK(flags);
@@ -1525,8 +1525,8 @@ static void busfree_run(struct Scsi_Host *shpnt)
 			/* target sent DISCONNECT */
 			DPRINTK(debug_selection, DEBUG_LEAD "target disconnected at %d/%d\n",
 				CMDINFO(CURRENT_SC),
-				CURRENT_SC->resid,
-				CURRENT_SC->request_bufflen);
+				scsi_get_resid(CURRENT_SC),
+				scsi_bufflen(CURRENT_SC));
 #if defined(AHA152X_STAT)
 			HOSTDATA(shpnt)->disconnections++;
 #endif
@@ -1564,7 +1564,7 @@ static void busfree_run(struct Scsi_Host *shpnt)
 			/* restore old command */
 			memcpy(cmd->cmnd, sc->aha_orig_cmnd, sizeof(cmd->cmnd));
 			cmd->cmd_len = sc->aha_orig_cmd_len;
-			cmd->resid = sc->aha_orig_resid;
+			scsi_set_resid(cmd, sc->aha_orig_resid);
 
 			cmd->SCp.Status = SAM_STAT_CHECK_CONDITION;
 
@@ -1594,7 +1594,7 @@ static void busfree_run(struct Scsi_Host *shpnt)
 				memcpy(sc->aha_orig_cmnd, ptr->cmnd,
 				                            sizeof(ptr->cmnd));
 				sc->aha_orig_cmd_len = ptr->cmd_len;
-				sc->aha_orig_resid = ptr->resid;
+				sc->aha_orig_resid = scsi_get_resid(ptr);
 
 				ptr->cmnd[0]         = REQUEST_SENSE;
 				ptr->cmnd[1]         = 0;
@@ -2179,7 +2179,8 @@ static void datai_init(struct Scsi_Host *shpnt)
 	DATA_LEN=0;
 	DPRINTK(debug_datai,
 		DEBUG_LEAD "datai_init: request_bufflen=%d resid=%d\n",
-		CMDINFO(CURRENT_SC), CURRENT_SC->request_bufflen, CURRENT_SC->resid);
+		CMDINFO(CURRENT_SC), scsi_bufflen(CURRENT_SC),
+		scsi_get_resid(CURRENT_SC));
 }
 
 static void datai_run(struct Scsi_Host *shpnt)
@@ -2292,11 +2293,12 @@ static void datai_run(struct Scsi_Host *shpnt)
 
 static void datai_end(struct Scsi_Host *shpnt)
 {
-	CURRENT_SC->resid -= GETSTCNT();
+	CMD_INC_RESID(CURRENT_SC, -GETSTCNT());
 
 	DPRINTK(debug_datai,
 		DEBUG_LEAD "datai_end: request_bufflen=%d resid=%d stcnt=%d\n",
-		CMDINFO(CURRENT_SC), CURRENT_SC->request_bufflen, CURRENT_SC->resid, GETSTCNT());
+		CMDINFO(CURRENT_SC), scsi_bufflen(CURRENT_SC),
+		scsi_get_resid(CURRENT_SC), GETSTCNT());
 
 	SETPORT(SXFRCTL0, CH1|CLRSTCNT);
 	SETPORT(DMACNTRL0, 0);
@@ -2317,11 +2319,12 @@ static void datao_init(struct Scsi_Host *shpnt)
 	SETPORT(SIMODE0, 0);
 	SETPORT(SIMODE1, ENSCSIPERR | ENSCSIRST | ENPHASEMIS | ENBUSFREE );
 
-	DATA_LEN = CURRENT_SC->resid;
+	DATA_LEN = scsi_get_resid(CURRENT_SC);
 
 	DPRINTK(debug_datao,
 		DEBUG_LEAD "datao_init: request_bufflen=%d; resid=%d\n",
-		CMDINFO(CURRENT_SC), CURRENT_SC->request_bufflen, CURRENT_SC->resid);
+		CMDINFO(CURRENT_SC), scsi_bufflen(CURRENT_SC),
+		scsi_get_resid(CURRENT_SC));
 }
 
 static void datao_run(struct Scsi_Host *shpnt)
@@ -2345,7 +2348,7 @@ static void datao_run(struct Scsi_Host *shpnt)
 			SETPORT(DMACNTRL0,WRITE_READ|ENDMA|_8BIT);
 			SETPORT(DATAPORT, *CURRENT_SC->SCp.ptr++);
 			CURRENT_SC->SCp.this_residual--;
-			CURRENT_SC->resid--;
+			CMD_INC_RESID(CURRENT_SC, -1);
 			SETPORT(DMACNTRL0,WRITE_READ|ENDMA);
 		}
 
@@ -2354,7 +2357,7 @@ static void datao_run(struct Scsi_Host *shpnt)
 			outsw(DATAPORT, CURRENT_SC->SCp.ptr, data_count);
 			CURRENT_SC->SCp.ptr           += 2 * data_count;
 			CURRENT_SC->SCp.this_residual -= 2 * data_count;
-			CURRENT_SC->resid             -= 2 * data_count;
+			CMD_INC_RESID(CURRENT_SC, -2 * data_count);
 	  	}
 
 		if(CURRENT_SC->SCp.this_residual==0 && CURRENT_SC->SCp.buffers_residual>0) {
@@ -2380,35 +2383,34 @@ static void datao_run(struct Scsi_Host *shpnt)
 static void datao_end(struct Scsi_Host *shpnt)
 {
 	if(TESTLO(DMASTAT, DFIFOEMP)) {
-		int data_count = (DATA_LEN - CURRENT_SC->resid) - GETSTCNT();
+		int data_count = (DATA_LEN - scsi_get_resid(CURRENT_SC)) -
+		                                                    GETSTCNT();
 
 		DPRINTK(debug_datao, DEBUG_LEAD "datao: %d bytes to resend (%d written, %d transferred)\n",
 			CMDINFO(CURRENT_SC),
 			data_count,
-			DATA_LEN-CURRENT_SC->resid,
+			DATA_LEN - scsi_get_resid(CURRENT_SC),
 			GETSTCNT());
 
-		CURRENT_SC->resid += data_count;
+		CMD_INC_RESID(CURRENT_SC, data_count);
 
-		if(CURRENT_SC->use_sg) {
-			data_count -= CURRENT_SC->SCp.ptr - SG_ADDRESS(CURRENT_SC->SCp.buffer);
-			while(data_count>0) {
-				CURRENT_SC->SCp.buffer--;
-				CURRENT_SC->SCp.buffers_residual++;
-				data_count -= CURRENT_SC->SCp.buffer->length;
-			}
-			CURRENT_SC->SCp.ptr           = SG_ADDRESS(CURRENT_SC->SCp.buffer) - data_count;
-			CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length + data_count;
-		} else {
-			CURRENT_SC->SCp.ptr           -= data_count;
-			CURRENT_SC->SCp.this_residual += data_count;
+		data_count -= CURRENT_SC->SCp.ptr -
+		                             SG_ADDRESS(CURRENT_SC->SCp.buffer);
+		while(data_count>0) {
+			CURRENT_SC->SCp.buffer--;
+			CURRENT_SC->SCp.buffers_residual++;
+			data_count -= CURRENT_SC->SCp.buffer->length;
 		}
+		CURRENT_SC->SCp.ptr = SG_ADDRESS(CURRENT_SC->SCp.buffer) -
+		                                                     data_count;
+		CURRENT_SC->SCp.this_residual = CURRENT_SC->SCp.buffer->length +
+		                                                     data_count;
 	}
 
 	DPRINTK(debug_datao, DEBUG_LEAD "datao_end: request_bufflen=%d; resid=%d; stcnt=%d\n",
 		CMDINFO(CURRENT_SC),
-		CURRENT_SC->request_bufflen,
-		CURRENT_SC->resid,
+		scsi_bufflen(CURRENT_SC),
+		scsi_get_resid(CURRENT_SC),
 		GETSTCNT());
 
 	SETPORT(SXFRCTL0, CH1|CLRCH1|CLRSTCNT);
@@ -2935,7 +2937,7 @@ static void show_command(Scsi_Cmnd *ptr)
 	__scsi_print_command(ptr->cmnd);
 
 	printk(KERN_DEBUG "); request_bufflen=%d; resid=%d; phase |",
-	       ptr->request_bufflen, ptr->resid);
+	       scsi_bufflen(ptr), scsi_get_resid(ptr));
 
 	if (ptr->SCp.phase & not_issued)
 		printk("not issued|");
@@ -3005,7 +3007,8 @@ static int get_command(char *pos, Scsi_Cmnd * ptr)
 		SPRINTF("0x%02x ", ptr->cmnd[i]);
 
 	SPRINTF("); resid=%d; residual=%d; buffers=%d; phase |",
-		ptr->resid, ptr->SCp.this_residual, ptr->SCp.buffers_residual);
+		scsi_get_resid(ptr), ptr->SCp.this_residual,
+		ptr->SCp.buffers_residual);
 
 	if (ptr->SCp.phase & not_issued)
 		SPRINTF("not issued|");
-- 
1.5.2.2.249.g45fd