isd200: Use scsi_get_cmnd for the extra translation command

This patch is for the 2.6.26 kernel. A more prominent fix for the sense_buffer allocation problem. It is based on top of an interim
fix: "isd200: Allocate sense_buffer for hacked up scsi_cmnd" that was sent for the 2.6.25 rc-fixes.

Only compile tested. Needs testing and ACK from USB Maintainers. (Or it can go through the USB tree)

James
please comment on the use of DID_REQUEUE as return status for the command in case of failure to allocate the extra command the first time.

Thanks
Boaz

---
From: Boaz Harrosh <[email protected]>
Date: Wed, 12 Mar 2008 15:41:41 +0200
Subject: [PATCH] isd200: Use scsi_get_cmnd for the extra translation command

don't let the isd200 driver allocate it's own scsi_command inside it's host private data. Use scsi-ml scsi_get_command() scsi_put_command for that. This is to insulate the driver from internal scsi-ml changes.

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/usb/storage/isd200.c |   27 ++++++++++++++++++---------
 1 files changed, 18 insertions(+), 9 deletions(-)

drivers/usb/storage/isd200.c +16 -7

index 2de1f1e..12d671b 100644
@@ -292,9 +292,8 @@ struct isd200_info {
 
 	/* maximum number of LUNs supported */
 	unsigned char MaxLUNs;
-	struct scsi_cmnd srb;
+	struct scsi_cmnd *srb;
 	struct scatterlist sg;
-	u8* sense_buffer;
 };
 
 

@@ -415,7 +414,7 @@ static void isd200_build_sense(struct us_data *us, struct scsi_cmnd *srb)
 static void isd200_set_srb(struct isd200_info *info,
 	enum dma_data_direction dir, void* buff, unsigned bufflen)
 {
-	struct scsi_cmnd *srb = &info->srb;
+	struct scsi_cmnd *srb = info->srb;
 
 	if (buff)
 		sg_init_one(&info->sg, buff, bufflen);

@@ -446,7 +445,7 @@ static int isd200_action( struct us_data *us, int action,
 	union ata_cdb ata;
 	struct scsi_device srb_dev;
 	struct isd200_info *info = (struct isd200_info *)us->extra;
-	struct scsi_cmnd *srb = &info->srb;
+	struct scsi_cmnd *srb = info->srb;
 	int status;
 
 	memset(&ata, 0, sizeof(ata));

@@ -1265,6 +1264,15 @@ static int isd200_scsi_to_ata(struct scsi_cmnd *srb, struct us_data *us,
 
 	memset(ataCdb, 0, sizeof(union ata_cdb));
 
+	/* very first time get an extra scsi_cmnd */
+	if (!info->srb) {
+		info->srb = scsi_get_command(srb->device, GFP_KERNEL);
+		if (!info->srb) {
+			srb->result = DID_REQUEUE << 16;
+			return 0;
+		}
+	}
+
 	/* SCSI Command */
 	switch (srb->cmnd[0]) {
 	case INQUIRY:

@@ -1471,7 +1479,10 @@ static void isd200_free_info_ptrs(void *info_)
 	if (info) {
 		kfree(info->id);
 		kfree(info->RegsBuf);
-		kfree(info->sense_buffer);
+
+		/* FIXME: Do we have a valid device here? */
+		if (info->srb)
+			scsi_put_command(info->srb);
 	}
 }
 

@@ -1497,13 +1508,11 @@ static int isd200_init_info(struct us_data *us)
 				kzalloc(sizeof(struct hd_driveid), GFP_KERNEL);
 		info->RegsBuf = (unsigned char *)
 				kmalloc(sizeof(info->ATARegs), GFP_KERNEL);
-		info->sense_buffer = kmalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
-		if (!info->id || !info->RegsBuf || !info->sense_buffer) {

James
please comment on the use of DID_REQUEUE as return status for the command in case of failure to allocate the extra command the first time.

Quoted 4 lines · Boaz Harrosh

James
please comment on the use of DID_REQUEUE as return status for the command in case of failure to allocate the extra command the first time.

I'm afraid it won't work. If you get a NULL return from scsi_get_command, it means that the command you already have was likely taken from the host free_list. If that command is in the memory clearing writeout path and we don't have any returning commands to replace the free_list, the system is now deadlocked.

James

Quoted 12 lines · James Bottomley

Quoted 4 lines · Boaz Harrosh

James
please comment on the use of DID_REQUEUE as return status for the command in case of failure to allocate the extra command the first time.

I'm afraid it won't work. If you get a NULL return from scsi_get_command, it means that the command you already have was likely taken from the host free_list. If that command is in the memory clearing writeout path and we don't have any returning commands to replace the free_list, the system is now deadlocked.

Quoted 2 lines

James

I'm not sure you are right. Please bear in mind that for isd200 an host has exactly one device. Now this *very first command* is some kind of INQUIRY in the discovery process, so it cannot be mounted for SWAP as yet. so it cannot be in the "clearing writeout path". Any kind of error on this first command will just result in an "empty" device, No?

I guess a newly mounted device on a memory starved system has lots of places to fail before and after this point in time.