On Wed, Mar 12 2008 at 17:15 +0200, Boaz Harrosh <bharrosh@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
> ---
> 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-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
