aha152x.c - Cleanup, bugfixes, convert to accessors

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 <[email protected]>

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

drivers/scsi/aha152x.c +26 -24

index d7ca86f..cf49ed5 100644
@@ -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);

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

Quoted 15 lines · Randy Dunlap

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 ?

Quoted 2 lines

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

Quoted 15 lines · Randy Dunlap

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 ?

Quoted 2 lines

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

Quoted 19 lines · On Tue, 31 Jul 2007 10:59:51 +0300 Boaz Harrosh wrote:

Quoted 17 lines · Randy Dunlap

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

Quoted 5 lines

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

Quoted 27 lines · Randy Dunlap

Quoted 24 lines · On Tue, 31 Jul 2007 10:59:51 +0300 Boaz Harrosh wrote:

Quoted 10 lines · Randy Dunlap

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

Quoted 7 lines

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.

Quoted 2 lines

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?

Quoted 20 lines · On Wed, 01 Aug 2007 08:51:14 -0500 James Bottomley wrote:

Quoted 19 lines · Randy Dunlap

Quoted 17 lines · On Tue, 31 Jul 2007 10:59:51 +0300 Boaz Harrosh wrote:

Quoted 16 lines · Randy Dunlap

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

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

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.

Quoted 2 lines

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 <[email protected]>

Thanks.

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

Quoted 27 lines · Randy Dunlap

Quoted 25 lines · On Wed, 01 Aug 2007 08:51:14 -0500 James Bottomley wrote:

Quoted 24 lines · Randy Dunlap

Quoted 22 lines · On Tue, 31 Jul 2007 10:59:51 +0300 Boaz Harrosh wrote:

Quoted 9 lines · Randy Dunlap

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

Quoted 7 lines

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.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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:

-------------------------------------------------------------------------------------------------------------

drivers/scsi/aha152x.c +1 -1

index d30a307..5fe55d0 100644
@@ -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) {

drivers/scsi/aha152x.h +1 -0

index ac4bfa4..065612f 100644
@@ -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 */

drivers/scsi/pcmcia/aha152x_stub.c +13 -0

index 370802d..e2f5ea5 100644
@@ -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)

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?

Quoted 8 lines

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

Quoted 8 lines

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:

Quoted 57 lines

drivers/scsi/aha152x.c +1 -1

index d30a307..5fe55d0 100644
@@ -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) {

drivers/scsi/aha152x.h +1 -0

index ac4bfa4..065612f 100644
@@ -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 */

drivers/scsi/pcmcia/aha152x_stub.c +14 -1

index 370802d..e2f5ea5 100644
@@ -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;
-------------------------------------------------------------------------------------------------------------

Quoted 2 lines

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

Looks OK to me.

Quoted 4 lines

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?