[PATCH] isd200: Use scsi_get_cmnd for the extra translation command

[PATCH] isd200: Use scsi_get_cmnd for the extra translation command

[Boaz Harrosh]

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 <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
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 <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
---
 drivers/usb/storage/isd200.c |   27 ++++++++++++++++++---------
 1 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 2de1f1e..12d671b 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -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) {
+		if (!info->id || !info->RegsBuf) {
 			isd200_free_info_ptrs(info);
 			kfree(info);
 			retStatus = ISD200_ERROR;
-		} else
-			info->srb.sense_buffer = info->sense_buffer;
+		}
 	}
 
 	if (retStatus == ISD200_GOOD) {
-- 
1.5.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] isd200: Use scsi_get_cmnd for the extra translation command

[Boaz Harrosh]

On Wed, Mar 12 2008 at 17:15 +0200, Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org> wrote:
> 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 <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
> 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 <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/usb/storage/isd200.c |   27 ++++++++++++++++++---------
>  1 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
> index 2de1f1e..12d671b 100644
> --- a/drivers/usb/storage/isd200.c
> +++ b/drivers/usb/storage/isd200.c
> @@ -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) {
> +		if (!info->id || !info->RegsBuf) {
>  			isd200_free_info_ptrs(info);
>  			kfree(info);
>  			retStatus = ISD200_ERROR;
> -		} else
> -			info->srb.sense_buffer = info->sense_buffer;
> +		}
>  	}
>  
>  	if (retStatus == ISD200_GOOD) {

I have re-audited the patch above and in theory it should work. The
"FIXME:" above, in isd200_free_info_ptrs, should be removed. This is because
scsi_get_command() will do a get_device() So the device will be held until
we do scsi_put_command() here.
But it does change the sequence of events a little bit for the isd200 devices
so please, if anyone has Hardware for the isd200, test this patch and see
that it can hot-unplug gracefully.

I will wait on testers and Maintainers ACKs and will repost without the FIXME:

Thanks in advance
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] isd200: Use scsi_get_cmnd for the extra translation command

[James Bottomley]

On Wed, 2008-03-12 at 17:15 +0200, Boaz Harrosh wrote:
> 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


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] isd200: Use scsi_get_cmnd for the extra translation command

[Boaz Harrosh]

On Wed, Mar 12 2008 at 19:00 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Wed, 2008-03-12 at 17:15 +0200, Boaz Harrosh wrote:
>> 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
> 

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.

Boaz

Re: [PATCH] isd200: Use scsi_get_cmnd for the extra translation command

[James Bottomley]

On Wed, 2008-03-12 at 19:27 +0200, Boaz Harrosh wrote:
> On Wed, Mar 12 2008 at 19:00 +0200, James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> > On Wed, 2008-03-12 at 17:15 +0200, Boaz Harrosh wrote:
> >> 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
> > 
> 
> 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? 

Not with DID_REQUEUE, it won't: it will go round until the command times
out, which is not too long, but unnecessary.

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

The basic problem, I suppose is that the whole structure is wrong.  The
only reason you try this strange method of allocation is because there's
no way to pull a command off the relevant pool without setting up the
host.  Export such a method and we can do the allocation at start of day
where it should be done.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html