[PATCH 1/4] scsi_free_command API change - Don't support GFP_DMA

[PATCH 1/4] scsi_free_command API change - Don't support GFP_DMA

[Boaz Harrosh]

Support of GFP_DMA for scsi_cmnd is going away. Don't let new
users get used to it. (None of the potential users need it)
Change that now before it has any users.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/scsi.c      |   23 +++++++++--------------
 include/scsi/scsi_cmnd.h |    4 ++--
 2 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 749c9c7..a120c04 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -386,13 +386,13 @@ static void scsi_put_host_cmd_pool(gfp_t gfp_mask)
 
 /**
  * scsi_allocate_command - get a fully allocated SCSI command
- * @gfp_mask:	allocation mask
+ * @gfp_mask:	allocation mask (must not be __GFP_DMA)
  *
  * This function is for use outside of the normal host based pools.
- * It allocates the relevant command and takes an additional reference
- * on the pool it used.  This function *must* be paired with
- * scsi_free_command which also has the identical mask, otherwise the
- * free pool counts will eventually go wrong and you'll trigger a bug.
+ * It allocates a command and takes an additional reference on the
+ * pool. This function *must* be paired with scsi_free_command
+ * otherwise the free pool counts will eventually go wrong and will
+ * not be de-allocated.
  *
  * This function should *only* be used by drivers that need a static
  * command allocation at start of day for internal functions.
@@ -410,16 +410,11 @@ EXPORT_SYMBOL(scsi_allocate_command);
 
 /**
  * scsi_free_command - free a command allocated by scsi_allocate_command
- * @gfp_mask:	mask used in the original allocation
  * @cmd:	command to free
- *
- * Note: using the original allocation mask is vital because that's
- * what determines which command pool we use to free the command.  Any
- * mismatch will cause the system to BUG eventually.
  */
-void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd)
+void scsi_free_command(struct scsi_cmnd *cmd)
 {
-	struct scsi_host_cmd_pool *pool = scsi_get_host_cmd_pool(gfp_mask);
+	struct scsi_host_cmd_pool *pool = scsi_get_host_cmd_pool(0);
 
 	/*
 	 * this could trigger if the mask to scsi_allocate_command
@@ -435,8 +430,8 @@ void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd)
 	 * reference we took above, and once to release the reference
 	 * originally taken by scsi_allocate_command
 	 */
-	scsi_put_host_cmd_pool(gfp_mask);
-	scsi_put_host_cmd_pool(gfp_mask);
+	scsi_put_host_cmd_pool(0);
+	scsi_put_host_cmd_pool(0);
 }
 EXPORT_SYMBOL(scsi_free_command);
 
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 8d20e60..2ebd52e 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -130,8 +130,8 @@ extern void scsi_release_buffers(struct scsi_cmnd *cmd);
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
 
-struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask);
-void scsi_free_command(gfp_t gfp_mask, struct scsi_cmnd *cmd);
+extern struct scsi_cmnd *scsi_allocate_command(gfp_t gfp_mask);
+extern void scsi_free_command(struct scsi_cmnd *cmd);
 
 static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
 {
-- 
1.5.3.3

[PATCH 2/4] isd200: Use new scsi_allocate_command()

[Boaz Harrosh]

Don't let isd200 struggle with scsi_cmnd allocation and construction.
Use the new scsi_allocate_command()/scsi_free_command() for that.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
CC: Alan Stern <stern@rowland.harvard.edu>
CC: Matthew Dharm <mdharm-usb@one-eyed-alien.net>
---
 drivers/usb/storage/isd200.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/storage/isd200.c b/drivers/usb/storage/isd200.c
index 971d13d..756d30f 100644
--- a/drivers/usb/storage/isd200.c
+++ b/drivers/usb/storage/isd200.c
@@ -292,7 +292,7 @@ struct isd200_info {
 
 	/* maximum number of LUNs supported */
 	unsigned char MaxLUNs;
-	struct scsi_cmnd srb;
+	struct scsi_cmnd *srb;
 	struct scatterlist sg;
 };
 
@@ -414,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);
@@ -445,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));
@@ -1470,7 +1470,8 @@ static void isd200_free_info_ptrs(void *info_)
 	if (info) {
 		kfree(info->id);
 		kfree(info->RegsBuf);
-		kfree(info->srb.sense_buffer);
+		if (info->srb)
+			scsi_free_command(info->srb);
 	}
 }
 
@@ -1496,9 +1497,8 @@ 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->srb.sense_buffer =
-				kmalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
-		if (!info->id || !info->RegsBuf || !info->srb.sense_buffer) {
+		info->srb = scsi_allocate_command(GFP_KERNEL);
+		if (!info->id || !info->RegsBuf || !info->srb) {
 			isd200_free_info_ptrs(info);
 			kfree(info);
 			retStatus = ISD200_ERROR;
-- 
1.5.3.3

[PATCH 3/3] gdth: consolidate __gdth_execute && gdth_execute

[Boaz Harrosh]

ioctl into gdth's char device would allocate an host-device at file-open
and would cache that device until the removal of the host, not at file
close. On the other hand a /proc access to gdth, and also flush() call at
host removal, would allocate a new device on every call and destroy it. Even
if one was allocated by file-open.

All this optimization is just a mess. But my primary concern is the construction
of the device just before destruction at flush(), looks extra pointless to me.

So allocate an host-device for every host, and remove it at host removal.
Now the existence of __gdth_execute is pointless and is removed. Also
gdth_execute can operate directly on gdth_ha_str.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/gdth.c      |   45 +++++++++++++++------------------------------
 drivers/scsi/gdth_proc.c |   16 ++++++++--------
 drivers/scsi/gdth_proc.h |    2 +-
 3 files changed, 24 insertions(+), 39 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index c6d6e7c..35ad3bf 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -439,10 +439,9 @@ static void gdth_scsi_done(struct scsi_cmnd *scp)
 		scp->scsi_done(scp);
 }
 
-int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
+int gdth_execute(gdth_ha_str *ha, gdth_cmd_str *gdtcmd, char *cmnd,
                    int timeout, u32 *info)
 {
-    gdth_ha_str *ha = shost_priv(sdev->host);
     Scsi_Cmnd *scp;
     struct gdth_cmndinfo cmndinfo;
     DECLARE_COMPLETION_ONSTACK(wait);
@@ -458,7 +457,7 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
 	return -ENOMEM;
     }
 
-    scp->device = sdev;
+    scp->device = ha->sdev;
     memset(&cmndinfo, 0, sizeof(cmndinfo));
 
     /* use request field to save the ptr. to completion struct. */
@@ -470,7 +469,7 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
     cmndinfo.internal_cmd_str = gdtcmd;
     cmndinfo.internal_command = 1;
 
-    TRACE(("__gdth_execute() cmd 0x%x\n", scp->cmnd[0]));
+    TRACE(("gdth_execute() cmd 0x%x\n", scp->cmnd[0]));
     __gdth_queuecommand(ha, scp, &cmndinfo);
 
     wait_for_completion(&wait);
@@ -483,16 +482,6 @@ int __gdth_execute(struct scsi_device *sdev, gdth_cmd_str *gdtcmd, char *cmnd,
     return rval;
 }
 
-int gdth_execute(struct Scsi_Host *shost, gdth_cmd_str *gdtcmd, char *cmnd,
-                 int timeout, u32 *info)
-{
-    struct scsi_device *sdev = scsi_get_host_dev(shost);
-    int rval = __gdth_execute(sdev, gdtcmd, cmnd, timeout, info);
-
-    scsi_free_host_dev(sdev);
-    return rval;
-}
-
 static void gdth_eval_mapping(ulong32 size, ulong32 *cyls, int *heads, int *secs)
 {
     *cyls = size /HEADS/SECS;
@@ -3997,13 +3986,6 @@ static int __gdth_queuecommand(gdth_ha_str *ha, struct scsi_cmnd *scp,
 
 static int gdth_open(struct inode *inode, struct file *filep)
 {
-    gdth_ha_str *ha;
-
-    list_for_each_entry(ha, &gdth_instances, list) {
-        if (!ha->sdev)
-            ha->sdev = scsi_get_host_dev(ha->shost);
-    }
-
     TRACE(("gdth_open()\n"));
     return 0;
 }
@@ -4109,7 +4091,7 @@ static int ioc_resetdrv(void __user *arg, char *cmnd)
     else
         cmd.u.cache.DeviceNo = res.number;
 
-    rval = __gdth_execute(ha->sdev, &cmd, cmnd, 30, NULL);
+    rval = gdth_execute(ha, &cmd, cmnd, 30, NULL);
     if (rval < 0)
         return rval;
     res.status = rval;
@@ -4217,7 +4199,7 @@ static int ioc_general(void __user *arg, char *cmnd)
         }
     }
 
-    rval = __gdth_execute(ha->sdev, &gen.command, cmnd, gen.timeout, &gen.info);
+    rval = gdth_execute(ha, &gen.command, cmnd, gen.timeout, &gen.info);
     if (rval < 0)
         return rval;
     gen.status = rval;
@@ -4273,7 +4255,7 @@ static int ioc_hdrlist(void __user *arg, char *cmnd)
                 cmd->u.cache64.DeviceNo = i;
             else
                 cmd->u.cache.DeviceNo = i;
-            if (__gdth_execute(ha->sdev, cmd, cmnd, 30, &cluster_type) == S_OK)
+            if (gdth_execute(ha, cmd, cmnd, 30, &cluster_type) == S_OK)
                 rsc->hdr_list[i].cluster_type = cluster_type;
         }
     } 
@@ -4323,7 +4305,7 @@ static int ioc_rescan(void __user *arg, char *cmnd)
             cmd->u.cache.DeviceNo = LINUX_OS;
         }
 
-        status = __gdth_execute(ha->sdev, cmd, cmnd, 30, &info);
+        status = gdth_execute(ha, cmd, cmnd, 30, &info);
         i = 0;
         hdr_cnt = (status == S_OK ? (ushort)info : 0);
     } else {
@@ -4339,7 +4321,7 @@ static int ioc_rescan(void __user *arg, char *cmnd)
         else 
             cmd->u.cache.DeviceNo = i;
 
-        status = __gdth_execute(ha->sdev, cmd, cmnd, 30, &info);
+        status = gdth_execute(ha, cmd, cmnd, 30, &info);
 
         spin_lock_irqsave(&ha->smp_lock, flags);
         rsc->hdr_list[i].bus = ha->virt_bus;
@@ -4373,7 +4355,7 @@ static int ioc_rescan(void __user *arg, char *cmnd)
         else
             cmd->u.cache.DeviceNo = i;
 
-        status = __gdth_execute(ha->sdev, cmd, cmnd, 30, &info);
+        status = gdth_execute(ha, cmd, cmnd, 30, &info);
 
         spin_lock_irqsave(&ha->smp_lock, flags);
         ha->hdr[i].devtype = (status == S_OK ? (ushort)info : 0);
@@ -4386,7 +4368,7 @@ static int ioc_rescan(void __user *arg, char *cmnd)
         else
             cmd->u.cache.DeviceNo = i;
 
-        status = __gdth_execute(ha->sdev, cmd, cmnd, 30, &info);
+        status = gdth_execute(ha, cmd, cmnd, 30, &info);
 
         spin_lock_irqsave(&ha->smp_lock, flags);
         ha->hdr[i].cluster_type = 
@@ -4401,7 +4383,7 @@ static int ioc_rescan(void __user *arg, char *cmnd)
         else
             cmd->u.cache.DeviceNo = i;
 
-        status = __gdth_execute(ha->sdev, cmd, cmnd, 30, &info);
+        status = gdth_execute(ha, cmd, cmnd, 30, &info);
 
         spin_lock_irqsave(&ha->smp_lock, flags);
         ha->hdr[i].rw_attribs = (status == S_OK ? (ushort)info : 0);
@@ -4599,7 +4581,7 @@ static void gdth_flush(gdth_ha_str *ha)
             }
             TRACE2(("gdth_flush(): flush ha %d drive %d\n", ha->hanum, i));
 
-            gdth_execute(ha->shost, &gdtcmd, cmnd, 30, NULL);
+            gdth_execute(ha, &gdtcmd, cmnd, 30, NULL);
         }
     }
 }
@@ -4737,6 +4719,7 @@ static int __init gdth_isa_probe_one(ulong32 isa_bios)
 	list_add_tail(&ha->list, &gdth_instances);
 
 	scsi_scan_host(shp);
+	ha->sdev = scsi_get_host_dev(ha->shost);
 
 	return 0;
 
@@ -4867,6 +4850,7 @@ static int __init gdth_eisa_probe_one(ushort eisa_slot)
 	list_add_tail(&ha->list, &gdth_instances);
 
 	scsi_scan_host(shp);
+	ha->sdev = scsi_get_host_dev(ha->shost);
 
 	return 0;
 
@@ -5013,6 +4997,7 @@ static int gdth_pci_probe_one(gdth_pci_str *pcistr,
 	pci_set_drvdata(ha->pdev, ha);
 
 	scsi_scan_host(shp);
+	ha->sdev = scsi_get_host_dev(ha->shost);
 
 	*ha_out = ha;
 
diff --git a/drivers/scsi/gdth_proc.c b/drivers/scsi/gdth_proc.c
index ce0228e..97c264a 100644
--- a/drivers/scsi/gdth_proc.c
+++ b/drivers/scsi/gdth_proc.c
@@ -84,7 +84,7 @@ static int gdth_set_asc_info(struct Scsi_Host *host, char *buffer,
                     gdtcmd.u.cache.BlockNo = 1;
                 }
 
-                gdth_execute(host, &gdtcmd, cmnd, 30, NULL);
+                gdth_execute(ha, &gdtcmd, cmnd, 30, NULL);
             }
         }
         if (!found)
@@ -137,7 +137,7 @@ static int gdth_set_asc_info(struct Scsi_Host *host, char *buffer,
         gdtcmd.u.ioctl.channel = INVALID_CHANNEL;
         pcpar->write_back = wb_mode==1 ? 0:1;
 
-        gdth_execute(host, &gdtcmd, cmnd, 30, NULL);
+        gdth_execute(ha, &gdtcmd, cmnd, 30, NULL);
 
         gdth_ioctl_free(ha, GDTH_SCRATCH, ha->pscratch, paddr);
         printk("Done.\n");
@@ -285,7 +285,7 @@ static int gdth_get_info(char *buffer,char **start,off_t offset,int length,
             if (pds->entries > cnt)
                 pds->entries = cnt;
 
-            if (gdth_execute(host, gdtcmd, cmnd, 30, NULL) != S_OK)
+            if (gdth_execute(ha, gdtcmd, cmnd, 30, NULL) != S_OK)
                 pds->count = 0;
 
             /* other IOCTLs must fit into area GDTH_SCRATCH/4 */
@@ -302,7 +302,7 @@ static int gdth_get_info(char *buffer,char **start,off_t offset,int length,
                 gdtcmd->u.ioctl.channel = 
                     ha->raw[i].address | ha->raw[i].id_list[j];
 
-                if (gdth_execute(host, gdtcmd, cmnd, 30, NULL) == S_OK) {
+                if (gdth_execute(ha, gdtcmd, cmnd, 30, NULL) == S_OK) {
                     strncpy(hrec,pdi->vendor,8);
                     strncpy(hrec+8,pdi->product,16);
                     strncpy(hrec+24,pdi->revision,4);
@@ -352,7 +352,7 @@ static int gdth_get_info(char *buffer,char **start,off_t offset,int length,
                         ha->raw[i].address | ha->raw[i].id_list[j];
                     pdef->sddc_type = 0x08;
 
-                    if (gdth_execute(host, gdtcmd, cmnd, 30, NULL) == S_OK) {
+                    if (gdth_execute(ha, gdtcmd, cmnd, 30, NULL) == S_OK) {
                         size = sprintf(buffer+len,
                                        " Grown Defects:\t%d\n",
                                        pdef->sddc_cnt);
@@ -398,7 +398,7 @@ static int gdth_get_info(char *buffer,char **start,off_t offset,int length,
                 gdtcmd->u.ioctl.param_size = sizeof(gdth_cdrinfo_str);
                 gdtcmd->u.ioctl.subfunc = CACHE_DRV_INFO;
                 gdtcmd->u.ioctl.channel = drv_no;
-                if (gdth_execute(host, gdtcmd, cmnd, 30, NULL) != S_OK)
+                if (gdth_execute(ha, gdtcmd, cmnd, 30, NULL) != S_OK)
                     break;
                 pcdi->ld_dtype >>= 16;
                 j++;
@@ -500,7 +500,7 @@ static int gdth_get_info(char *buffer,char **start,off_t offset,int length,
             gdtcmd->u.ioctl.param_size = sizeof(gdth_arrayinf_str);
             gdtcmd->u.ioctl.subfunc = ARRAY_INFO | LA_CTRL_PATTERN;
             gdtcmd->u.ioctl.channel = i;
-            if (gdth_execute(host, gdtcmd, cmnd, 30, NULL) == S_OK) {
+            if (gdth_execute(ha, gdtcmd, cmnd, 30, NULL) == S_OK) {
                 if (pai->ai_state == 0)
                     strcpy(hrec, "idle");
                 else if (pai->ai_state == 2)
@@ -574,7 +574,7 @@ static int gdth_get_info(char *buffer,char **start,off_t offset,int length,
             gdtcmd->u.ioctl.channel = i;
             phg->entries = MAX_HDRIVES;
             phg->offset = GDTOFFSOF(gdth_hget_str, entry[0]); 
-            if (gdth_execute(host, gdtcmd, cmnd, 30, NULL) == S_OK) {
+            if (gdth_execute(ha, gdtcmd, cmnd, 30, NULL) == S_OK) {
                 ha->hdr[i].ldr_no = i;
                 ha->hdr[i].rw_attribs = 0;
                 ha->hdr[i].start_sec = 0;
diff --git a/drivers/scsi/gdth_proc.h b/drivers/scsi/gdth_proc.h
index 45e6fda..faf4496 100644
--- a/drivers/scsi/gdth_proc.h
+++ b/drivers/scsi/gdth_proc.h
@@ -5,7 +5,7 @@
  * $Id: gdth_proc.h,v 1.16 2004/01/14 13:09:01 achim Exp $
  */
 
-int gdth_execute(struct Scsi_Host *shost, gdth_cmd_str *gdtcmd, char *cmnd,
+int gdth_execute(gdth_ha_str *ha, gdth_cmd_str *gdtcmd, char *cmnd,
                  int timeout, u32 *info);
 
 static int gdth_set_info(char *buffer,int length,struct Scsi_Host *host,
-- 
1.5.3.3

[PATCH 4/4] gdth: Use scsi_allocate_command for private command allocation

[Boaz Harrosh]

In gdth_execute() use scsi_allocate_command for allocation of
a command. To be insulated from future scsi_cmnd construction
considerations.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/gdth.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 35ad3bf..7778373 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -447,16 +447,10 @@ int gdth_execute(gdth_ha_str *ha, gdth_cmd_str *gdtcmd, char *cmnd,
     DECLARE_COMPLETION_ONSTACK(wait);
     int rval;
 
-    scp = kzalloc(sizeof(*scp), GFP_KERNEL);
+    scp = scsi_allocate_command(GFP_KERNEL);
     if (!scp)
         return -ENOMEM;
 
-    scp->sense_buffer = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
-    if (!scp->sense_buffer) {
-	kfree(scp);
-	return -ENOMEM;
-    }
-
     scp->device = ha->sdev;
     memset(&cmndinfo, 0, sizeof(cmndinfo));
 
@@ -477,8 +471,8 @@ int gdth_execute(gdth_ha_str *ha, gdth_cmd_str *gdtcmd, char *cmnd,
     rval = cmndinfo.status;
     if (info)
         *info = cmndinfo.info;
-    kfree(scp->sense_buffer);
-    kfree(scp);
+
+    scsi_free_command(scp);
     return rval;
 }
 
-- 
1.5.3.3

Re: [PATCH 0/4] Use of new scsi_allocate_command

[James Bottomley]

On Thu, 2008-05-01 at 16:47 +0300, Boaz Harrosh wrote:
> [PATCH 1/4] scsi_free_command API change - Don't support GFP_DMA
>   ISA support is going away and is not needed by any potential clients
>   of this code. So change the API now before it has any users.

What makes you think this?  We still have lots of ISA installations out
there (and some broken PCI ones requiring ISA DMA masks) that we have to
support.

For gdth you have an ISA board that *you* need to support, so the isa
board has to have this flag.  The reason no-one's run into a bug here is
that the problematic allocation is the sense buffer not the command.
Unfortunately, when the fix was added to the gdth driver for the sense
buffer separation:

commit 1b96f8955aaeeb05f7fb7ff548aa12415fbf3904
Author: Sven Schnelle <svens@bitebene.org>
Date:   Mon Mar 10 22:50:04 2008 +0100

    [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

We actually forgot about this case.  Right at the moment, any ioctl that
uses gdth_execute on an ISA device will probably fail if the executed
command resturns sense and there look to be paths down gdth_execute even
for special commands where the OpCode flips to a regular command (-1)
and so DMAs into the sense buffer.

James

Re: [PATCH 0/4] Use of new scsi_allocate_command

[Boaz Harrosh]

On Thu, May 01 2008 at 17:24 +0300, James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Thu, 2008-05-01 at 16:47 +0300, Boaz Harrosh wrote:
>> [PATCH 1/4] scsi_free_command API change - Don't support GFP_DMA
>>   ISA support is going away and is not needed by any potential clients
>>   of this code. So change the API now before it has any users.
> 
> What makes you think this?  We still have lots of ISA installations out
> there (and some broken PCI ones requiring ISA DMA masks) that we have to
> support.
> 

Believe me I'm an expert. I'm working on the complete removal of GFP_DMA dma
support, initiated by Andi Kleen. And will submit the complete work on Sunday
once reviewed by Andi and some preliminary test pass.

> For gdth you have an ISA board that *you* need to support, so the isa
> board has to have this flag.  The reason no-one's run into a bug here is
> that the problematic allocation is the sense buffer not the command.
> Unfortunately, when the fix was added to the gdth driver for the sense
> buffer separation:
> 
> commit 1b96f8955aaeeb05f7fb7ff548aa12415fbf3904
> Author: Sven Schnelle <svens-+2vGQ62mfxRg9hUCZPvPmw@public.gmane.org>
> Date:   Mon Mar 10 22:50:04 2008 +0100
> 
>     [SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference
> 
> We actually forgot about this case.  Right at the moment, any ioctl that
> uses gdth_execute on an ISA device will probably fail if the executed
> command resturns sense and there look to be paths down gdth_execute even
> for special commands where the OpCode flips to a regular command (-1)
> and so DMAs into the sense buffer.
> 
> James
> 
> 

I'm soon to submit a patch that fixes that for the ISA case.
[http://git.open-osd.org/gitweb.cgi?p=open-osd.git;a=commitdiff;h=f6aceea8a078907224d3e34fcd1118b92ef4e79a]

The call to pci_map_page() on an ISA case will it not bounce the buffer?

Any way it is a chicken and egg thing. I need the API stable to start converting drivers
and then use API in drivers. I thought it was pointless to do it in two stages for this little
thing since the thing is already broken now, and will be fixed very soon by me.

however we could do below patch to fix it. Use scsi_get_command(). It will not interfere with
the reserved command allocation and IO forward progress, because gdth_execute is always submitted
in parallel and never as a result of queuecommand. So there will be more contenders for the spare
command but they will never deadlock.

I was just reviewing those patches I think it will also be better because of the device locking
for the duration of gdth_execute.

---
From: Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
Subject: [PATCH 4/4] gdth: Use scsi_allocate_command for private command allocation

In gdth_execute() use scsi_allocate_command for allocation of
a command. To be insulated from future scsi_cmnd construction
considerations.

Signed-off-by: Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
---
 drivers/scsi/gdth.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 35ad3bf..32a5ee2 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -447,17 +447,10 @@ int gdth_execute(gdth_ha_str *ha, gdth_cmd_str *gdtcmd, char *cmnd,
     DECLARE_COMPLETION_ONSTACK(wait);
     int rval;
 
-    scp = kzalloc(sizeof(*scp), GFP_KERNEL);
+    scp = scsi_get_command(GFP_KERNEL);
     if (!scp)
         return -ENOMEM;
 
-    scp->sense_buffer = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
-    if (!scp->sense_buffer) {
-	kfree(scp);
-	return -ENOMEM;
-    }

Re: [PATCH 0/4] Use of new scsi_allocate_command

[Boaz Harrosh]

On Thu, May 01 2008 at 17:56 +0300, Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
> ---
> From: Boaz Harrosh <bharrosh@panasas.com>
> Subject: [PATCH 4/4] gdth: Use scsi_allocate_command for private command allocation
> 
> In gdth_execute() use scsi_allocate_command for allocation of
> a command. To be insulated from future scsi_cmnd construction
> considerations.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
<snip>

Rrrr wrong patch sorry for the noise. Here is a compiling one

---
From: Boaz Harrosh <bharrosh@panasas.com>
Subject: [PATCH] gdth: Use scsi_get_command for private command allocation

In gdth_execute() use scsi_get_command for allocation of
a command. To be insulated from future scsi_cmnd construction
considerations.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/gdth.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 35ad3bf..3cfbab6 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -447,17 +447,10 @@ int gdth_execute(gdth_ha_str *ha, gdth_cmd_str *gdtcmd, char *cmnd,
     DECLARE_COMPLETION_ONSTACK(wait);
     int rval;
 
-    scp = kzalloc(sizeof(*scp), GFP_KERNEL);
+    scp = scsi_get_command(ha->sdev, GFP_KERNEL);
     if (!scp)
         return -ENOMEM;
 
-    scp->sense_buffer = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
-    if (!scp->sense_buffer) {
-	kfree(scp);
-	return -ENOMEM;
-    }
-
-    scp->device = ha->sdev;
     memset(&cmndinfo, 0, sizeof(cmndinfo));
 
     /* use request field to save the ptr. to completion struct. */
@@ -477,8 +470,8 @@ int gdth_execute(gdth_ha_str *ha, gdth_cmd_str *gdtcmd, char *cmnd,
     rval = cmndinfo.status;
     if (info)
         *info = cmndinfo.info;
-    kfree(scp->sense_buffer);
-    kfree(scp);
+
+    scsi_put_command(scp);
     return rval;
 }
 
-- 
1.5.3.3

Re: [PATCH 0/4] Use of new scsi_allocate_command

[James Bottomley]

On Thu, 2008-05-01 at 17:56 +0300, Boaz Harrosh wrote:
> The call to pci_map_page() on an ISA case will it not bounce the
> buffer?

Only if the system isn't a PCI_DMA_BUS_IS_PHYS one.

That means basically either has an iommu/gart or has some silly swiotlb
to emulate one.

That means for standard x86 isa systems the answer is no: the buffer has
to be allocated within the isa region because pci_map is simply a
virt_to_phys.

James

Re: [PATCH 0/4] Use of new scsi_allocate_command

[Boaz Harrosh]

On Thu, May 01 2008 at 18:22 +0300, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Thu, 2008-05-01 at 17:56 +0300, Boaz Harrosh wrote:
>> The call to pci_map_page() on an ISA case will it not bounce the
>> buffer?
> 
> Only if the system isn't a PCI_DMA_BUS_IS_PHYS one.
> 
> That means basically either has an iommu/gart or has some silly swiotlb
> to emulate one.
> 
> That means for standard x86 isa systems the answer is no: the buffer has
> to be allocated within the isa region because pci_map is simply a
> virt_to_phys.
> 
> James
> 
OK Thanks, so the second patch then. That will solve it.

Boaz

Re: [PATCH 0/4] Use of new scsi_allocate_command

[James Bottomley]

On Thu, 2008-05-01 at 18:36 +0300, Boaz Harrosh wrote:
> On Thu, May 01 2008 at 18:22 +0300, James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> > On Thu, 2008-05-01 at 17:56 +0300, Boaz Harrosh wrote:
> >> The call to pci_map_page() on an ISA case will it not bounce the
> >> buffer?
> > 
> > Only if the system isn't a PCI_DMA_BUS_IS_PHYS one.
> > 
> > That means basically either has an iommu/gart or has some silly swiotlb
> > to emulate one.
> > 
> > That means for standard x86 isa systems the answer is no: the buffer has
> > to be allocated within the isa region because pci_map is simply a
> > virt_to_phys.
> > 
> > James
> > 
> OK Thanks, so the second patch then. That will solve it.

Um, your second patch is only changing isd200; it's hard to see how that
can fix a gdth problem ...

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 0/4] Use of new scsi_allocate_command

[Boaz Harrosh]

On Thu, May 01 2008 at 18:47 +0300, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Thu, 2008-05-01 at 18:36 +0300, Boaz Harrosh wrote:
>> On Thu, May 01 2008 at 18:22 +0300, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
>>> On Thu, 2008-05-01 at 17:56 +0300, Boaz Harrosh wrote:
>>>> The call to pci_map_page() on an ISA case will it not bounce the
>>>> buffer?
>>> Only if the system isn't a PCI_DMA_BUS_IS_PHYS one.
>>>
>>> That means basically either has an iommu/gart or has some silly swiotlb
>>> to emulate one.
>>>
>>> That means for standard x86 isa systems the answer is no: the buffer has
>>> to be allocated within the isa region because pci_map is simply a
>>> virt_to_phys.
>>>
>>> James
>>>
>> OK Thanks, so the second patch then. That will solve it.
> 
> Um, your second patch is only changing isd200; it's hard to see how that
> can fix a gdth problem ...
> 
> James
> 
> 

Sorry I meant the second version of the patch that uses scsi_get_command()

http://marc.info/?l=linux-scsi&m=120965483126936&w=2

Boaz

Re: [PATCH 0/4] Use of new scsi_allocate_command

[James Bottomley]

On Thu, 2008-05-01 at 18:59 +0300, Boaz Harrosh wrote:
> On Thu, May 01 2008 at 18:47 +0300, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > On Thu, 2008-05-01 at 18:36 +0300, Boaz Harrosh wrote:
> >> On Thu, May 01 2008 at 18:22 +0300, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> >>> On Thu, 2008-05-01 at 17:56 +0300, Boaz Harrosh wrote:
> >>>> The call to pci_map_page() on an ISA case will it not bounce the
> >>>> buffer?
> >>> Only if the system isn't a PCI_DMA_BUS_IS_PHYS one.
> >>>
> >>> That means basically either has an iommu/gart or has some silly swiotlb
> >>> to emulate one.
> >>>
> >>> That means for standard x86 isa systems the answer is no: the buffer has
> >>> to be allocated within the isa region because pci_map is simply a
> >>> virt_to_phys.
> >>>
> >>> James
> >>>
> >> OK Thanks, so the second patch then. That will solve it.
> > 
> > Um, your second patch is only changing isd200; it's hard to see how that
> > can fix a gdth problem ...
> > 
> > James
> > 
> > 
> 
> Sorry I meant the second version of the patch that uses scsi_get_command()
> 
> http://marc.info/?l=linux-scsi&m=120965483126936&w=2

No ... it still needs to use scsi_allocate_command() and pass the gfp
flags for DMA if necessary.

James

Re: [PATCH 0/4] Use of new scsi_allocate_command

[Boaz Harrosh]

On Thu, May 01 2008 at 19:02 +0300, James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Thu, 2008-05-01 at 18:59 +0300, Boaz Harrosh wrote:
>> On Thu, May 01 2008 at 18:47 +0300, James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
>>> On Thu, 2008-05-01 at 18:36 +0300, Boaz Harrosh wrote:
>>>> On Thu, May 01 2008 at 18:22 +0300, James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
>>>>> On Thu, 2008-05-01 at 17:56 +0300, Boaz Harrosh wrote:
>>>>>> The call to pci_map_page() on an ISA case will it not bounce the
>>>>>> buffer?
>>>>> Only if the system isn't a PCI_DMA_BUS_IS_PHYS one.
>>>>>
>>>>> That means basically either has an iommu/gart or has some silly swiotlb
>>>>> to emulate one.
>>>>>
>>>>> That means for standard x86 isa systems the answer is no: the buffer has
>>>>> to be allocated within the isa region because pci_map is simply a
>>>>> virt_to_phys.
>>>>>
>>>>> James
>>>>>
>>>> OK Thanks, so the second patch then. That will solve it.
>>> Um, your second patch is only changing isd200; it's hard to see how that
>>> can fix a gdth problem ...
>>>
>>> James
>>>
>>>
>> Sorry I meant the second version of the patch that uses scsi_get_command()
>>
>> http://marc.info/?l=linux-scsi&m=120965483126936&w=2
> 
> No ... it still needs to use scsi_allocate_command() and pass the gfp
> flags for DMA if necessary.
> 
> James
> 
> 

But I'm removing all that very soon.

Here is a patch that fixes that ISA thing. Please submit it before the 
original [PATCH 4/4].

--
From: Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
Subject: [PATCH 3.5/4] gdth: Fix sense handling in the ISA case

Allocate the 16 bytes buffer from host space which is of the right mask
in the ISA case.

Signed-off-by: Boaz Harrosh <bharrosh-C4P08NqkoRlBDgjK7y7TUQ@public.gmane.org>
---
 drivers/scsi/gdth.c |   11 ++++++++---
 drivers/scsi/gdth.h |    1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 3cfbab6..fb61c79 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -2647,8 +2647,8 @@ static int gdth_fill_raw_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, unchar b)
         }
 
     } else {
-        page = virt_to_page(scp->sense_buffer);
-        offset = (ulong)scp->sense_buffer & ~PAGE_MASK;
+        page = virt_to_page(cmndinfo->sense);
+        offset = (ulong)cmndinfo->sense & ~PAGE_MASK;
         sense_paddr = pci_map_page(ha->pdev,page,offset,
                                    16,PCI_DMA_FROMDEVICE);
 
@@ -3317,9 +3317,14 @@ static int gdth_sync_event(gdth_ha_str *ha, int service, unchar index,
             pci_unmap_sg(ha->pdev, scsi_sglist(scp), scsi_sg_count(scp),
                          cmndinfo->dma_dir);
 
-        if (cmndinfo->sense_paddr)
+        if (cmndinfo->sense_paddr) {
             pci_unmap_page(ha->pdev, cmndinfo->sense_paddr, 16,
                                                            PCI_DMA_FROMDEVICE);
+            /* this here is called before gdth_next so it will not
+             * overwrite fake sense returned there.
+             */
+            memcpy(scp->sense_buffer, cmndinfo->sense, 16);
+	}
 
         if (ha->status == S_OK) {
             cmndinfo->status = S_OK;
diff --git a/drivers/scsi/gdth.h b/drivers/scsi/gdth.h
index ca92476..445ea7f 100644
--- a/drivers/scsi/gdth.h
+++ b/drivers/scsi/gdth.h
@@ -914,6 +914,7 @@ typedef struct {
         int index;
         int internal_command;                   /* don't call scsi_done */
         gdth_cmd_str *internal_cmd_str;         /* crier for internal messages*/
+        u8 sense[16];                           /* 16 is used allover */
         dma_addr_t sense_paddr;                 /* sense dma-addr */
         unchar priority;
         int timeout;
-- 
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 0/4] Use of new scsi_allocate_command

[James Bottomley]

On Thu, 2008-05-01 at 19:25 +0300, Boaz Harrosh wrote:
> On Thu, May 01 2008 at 19:02 +0300, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > On Thu, 2008-05-01 at 18:59 +0300, Boaz Harrosh wrote:
> >> On Thu, May 01 2008 at 18:47 +0300, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> >>> On Thu, 2008-05-01 at 18:36 +0300, Boaz Harrosh wrote:
> >>>> On Thu, May 01 2008 at 18:22 +0300, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> >>>>> On Thu, 2008-05-01 at 17:56 +0300, Boaz Harrosh wrote:
> >>>>>> The call to pci_map_page() on an ISA case will it not bounce the
> >>>>>> buffer?
> >>>>> Only if the system isn't a PCI_DMA_BUS_IS_PHYS one.
> >>>>>
> >>>>> That means basically either has an iommu/gart or has some silly swiotlb
> >>>>> to emulate one.
> >>>>>
> >>>>> That means for standard x86 isa systems the answer is no: the buffer has
> >>>>> to be allocated within the isa region because pci_map is simply a
> >>>>> virt_to_phys.
> >>>>>
> >>>>> James
> >>>>>
> >>>> OK Thanks, so the second patch then. That will solve it.
> >>> Um, your second patch is only changing isd200; it's hard to see how that
> >>> can fix a gdth problem ...
> >>>
> >>> James
> >>>
> >>>
> >> Sorry I meant the second version of the patch that uses scsi_get_command()
> >>
> >> http://marc.info/?l=linux-scsi&m=120965483126936&w=2
> > 
> > No ... it still needs to use scsi_allocate_command() and pass the gfp
> > flags for DMA if necessary.
> > 
> > James
> > 
> > 
> 
> But I'm removing all that very soon.
> 
> Here is a patch that fixes that ISA thing. Please submit it before the 
> original [PATCH 4/4].
> 
> --
> From: Boaz Harrosh <bharrosh@panasas.com>
> Subject: [PATCH 3.5/4] gdth: Fix sense handling in the ISA case
> 
> Allocate the 16 bytes buffer from host space which is of the right mask
> in the ISA case.
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  drivers/scsi/gdth.c |   11 ++++++++---
>  drivers/scsi/gdth.h |    1 +
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index 3cfbab6..fb61c79 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -2647,8 +2647,8 @@ static int gdth_fill_raw_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, unchar b)
>          }
>  
>      } else {
> -        page = virt_to_page(scp->sense_buffer);
> -        offset = (ulong)scp->sense_buffer & ~PAGE_MASK;
> +        page = virt_to_page(cmndinfo->sense);
> +        offset = (ulong)cmndinfo->sense & ~PAGE_MASK;
>          sense_paddr = pci_map_page(ha->pdev,page,offset,
>                                     16,PCI_DMA_FROMDEVICE);
>  
> @@ -3317,9 +3317,14 @@ static int gdth_sync_event(gdth_ha_str *ha, int service, unchar index,
>              pci_unmap_sg(ha->pdev, scsi_sglist(scp), scsi_sg_count(scp),
>                           cmndinfo->dma_dir);
>  
> -        if (cmndinfo->sense_paddr)
> +        if (cmndinfo->sense_paddr) {
>              pci_unmap_page(ha->pdev, cmndinfo->sense_paddr, 16,
>                                                             PCI_DMA_FROMDEVICE);
> +            /* this here is called before gdth_next so it will not
> +             * overwrite fake sense returned there.
> +             */
> +            memcpy(scp->sense_buffer, cmndinfo->sense, 16);
> +	}
>  
>          if (ha->status == S_OK) {
>              cmndinfo->status = S_OK;
> diff --git a/drivers/scsi/gdth.h b/drivers/scsi/gdth.h
> index ca92476..445ea7f 100644
> --- a/drivers/scsi/gdth.h
> +++ b/drivers/scsi/gdth.h
> @@ -914,6 +914,7 @@ typedef struct {
>          int index;
>          int internal_command;                   /* don't call scsi_done */
>          gdth_cmd_str *internal_cmd_str;         /* crier for internal messages*/
> +        u8 sense[16];                           /* 16 is used allover */
>          dma_addr_t sense_paddr;                 /* sense dma-addr */
>          unchar priority;
>          int timeout;


Er, but

     1. struct cmdinfo is stack allocated; you can't dma to stack
     2. even if you fix this, the structure is packed and will have the
        original ppc coherence issues
     3. Finally, even on the stack, it's not necessarily reachable with
        the DMA mask

The whole point of managing sense buffer allocations centrally is to
prevent driver writers from falling into all these traps.  I really
don't think going back to hand rolled bounce buffering imporves the
situation.

James

Re: [PATCH 0/4] Use of new scsi_allocate_command

[Boaz Harrosh]

On Thu, May 01 2008 at 19:38 +0300, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
<snip> 
> Er, but
> 
>      1. struct cmdinfo is stack allocated; you can't dma to stack
>      2. even if you fix this, the structure is packed and will have the
>         original ppc coherence issues
>      3. Finally, even on the stack, it's not necessarily reachable with
>         the DMA mask
> 

Ho, thanks for the catch. I really goofed here.

> The whole point of managing sense buffer allocations centrally is to
> prevent driver writers from falling into all these traps.  I really
> don't think going back to hand rolled bounce buffering imporves the
> situation.
> 

We don't have a choice Andi is removing all this. ISA is becoming
ISA's driver private problem.

> James
> 
> 

Here is a new attempt at fixing the ISA case for gdth. Please forgive
me for that nasty bug that stared me in the face.

---
From: Boaz Harrosh <bharrosh@panasas.com>
Subject: [PATCH 3.5/4] gdth: Fix sense handling in the ISA case

Allocate the 16 bytes buffer from host space which is of the right mask
in the ISA case. And always use cmndinfo from host space.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/gdth.c |   33 +++++++++++++++++++++------------
 drivers/scsi/gdth.h |    1 +
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 3cfbab6..805c616 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -443,7 +443,7 @@ int gdth_execute(gdth_ha_str *ha, gdth_cmd_str *gdtcmd, char *cmnd,
                    int timeout, u32 *info)
 {
     Scsi_Cmnd *scp;
-    struct gdth_cmndinfo cmndinfo;
+    struct gdth_cmndinfo *cmndinfo;
     DECLARE_COMPLETION_ONSTACK(wait);
     int rval;
 
@@ -451,26 +451,29 @@ int gdth_execute(gdth_ha_str *ha, gdth_cmd_str *gdtcmd, char *cmnd,
     if (!scp)
         return -ENOMEM;
 
-    memset(&cmndinfo, 0, sizeof(cmndinfo));
+    cmndinfo = gdth_get_cmndinfo(ha);
+    if (!cmndinfo)
+        return -ENOMEM;
 
     /* use request field to save the ptr. to completion struct. */
     scp->request = (struct request *)&wait;
     scp->timeout_per_command = timeout*HZ;
     scp->cmd_len = 12;
     memcpy(scp->cmnd, cmnd, 12);
-    cmndinfo.priority = IOCTL_PRI;
-    cmndinfo.internal_cmd_str = gdtcmd;
-    cmndinfo.internal_command = 1;
+    cmndinfo->priority = IOCTL_PRI;
+    cmndinfo->internal_cmd_str = gdtcmd;
+    cmndinfo->internal_command = 1;
 
     TRACE(("gdth_execute() cmd 0x%x\n", scp->cmnd[0]));
-    __gdth_queuecommand(ha, scp, &cmndinfo);
+    __gdth_queuecommand(ha, scp, cmndinfo);
 
     wait_for_completion(&wait);
 
-    rval = cmndinfo.status;
+    rval = cmndinfo->status;
     if (info)
-        *info = cmndinfo.info;
+        *info = cmndinfo->info;
 
+    gdth_put_cmndinfo(cmndinfo);
     scsi_put_command(scp);
     return rval;
 }
@@ -2647,8 +2650,8 @@ static int gdth_fill_raw_cmd(gdth_ha_str *ha, Scsi_Cmnd *scp, unchar b)
         }
 
     } else {
-        page = virt_to_page(scp->sense_buffer);
-        offset = (ulong)scp->sense_buffer & ~PAGE_MASK;
+        page = virt_to_page(cmndinfo->sense);
+        offset = (ulong)cmndinfo->sense & ~PAGE_MASK;
         sense_paddr = pci_map_page(ha->pdev,page,offset,
                                    16,PCI_DMA_FROMDEVICE);
 
@@ -3317,9 +3320,14 @@ static int gdth_sync_event(gdth_ha_str *ha, int service, unchar index,
             pci_unmap_sg(ha->pdev, scsi_sglist(scp), scsi_sg_count(scp),
                          cmndinfo->dma_dir);
 
-        if (cmndinfo->sense_paddr)
+        if (cmndinfo->sense_paddr) {
             pci_unmap_page(ha->pdev, cmndinfo->sense_paddr, 16,
                                                            PCI_DMA_FROMDEVICE);
+            /* this here is called before gdth_next so it will not
+             * overwrite fake sense returned there.
+             */
+            memcpy(scp->sense_buffer, cmndinfo->sense, 16);
+	}
 
         if (ha->status == S_OK) {
             cmndinfo->status = S_OK;
@@ -3950,7 +3958,8 @@ static int gdth_queuecommand(struct scsi_cmnd *scp,
     TRACE(("gdth_queuecommand() cmd 0x%x\n", scp->cmnd[0]));
 
     cmndinfo = gdth_get_cmndinfo(ha);
-    BUG_ON(!cmndinfo);
+    if(!cmndinfo)
+	return 1; /* too meany commands retry later */
 
     scp->scsi_done = done;
     gdth_update_timeout(scp, scp->timeout_per_command * 6);
diff --git a/drivers/scsi/gdth.h b/drivers/scsi/gdth.h
index ca92476..445ea7f 100644
--- a/drivers/scsi/gdth.h
+++ b/drivers/scsi/gdth.h
@@ -914,6 +914,7 @@ typedef struct {
         int index;
         int internal_command;                   /* don't call scsi_done */
         gdth_cmd_str *internal_cmd_str;         /* crier for internal messages*/
+        u8 sense[16];                           /* 16 is used allover */
         dma_addr_t sense_paddr;                 /* sense dma-addr */
         unchar priority;
         int timeout;
-- 
1.5.3.3

Re: [PATCH 0/4] Use of new scsi_allocate_command

[James Bottomley]

On Thu, 2008-05-01 at 20:06 +0300, Boaz Harrosh wrote:
> On Thu, May 01 2008 at 19:38 +0300, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> <snip> 
> > Er, but
> > 
> >      1. struct cmdinfo is stack allocated; you can't dma to stack
> >      2. even if you fix this, the structure is packed and will have the
> >         original ppc coherence issues
> >      3. Finally, even on the stack, it's not necessarily reachable with
> >         the DMA mask
> > 
> 
> Ho, thanks for the catch. I really goofed here.
> 
> > The whole point of managing sense buffer allocations centrally is to
> > prevent driver writers from falling into all these traps.  I really
> > don't think going back to hand rolled bounce buffering imporves the
> > situation.
> > 
> 
> We don't have a choice Andi is removing all this. ISA is becoming
> ISA's driver private problem.

No he's not .. not if it's going to cause this type of grief.

But I think you misread his patch set.  He still has a flag equivalent
of unchecked_isa_dma  it's just called sense_buffer_isa instead.  The
shift in his patch set is 

     1. it removes the need to allocate commands in dma'able memory
     2. it removes the need for the ULD request path to allocate in
        dma'able memory (it uses the block bounce path instead)
     3. it *can't* remove the need to dma to the sense buffer, hence the
        flag.

So it looks like the re-rolled patch set (when Andi gets around to
posting it) will use one pool for the cmd buffer, but separate isa and
non-isa pools for the sense buffer.  Thus it should just work for gdth
with scsi_allocate_command predicated on sense_buffer_isa.

James

Re: [PATCH 0/4] Use of new scsi_allocate_command

[Boaz Harrosh]

On Thu, May 01 2008 at 20:33 +0300, James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
> On Thu, 2008-05-01 at 20:06 +0300, Boaz Harrosh wrote:
>> On Thu, May 01 2008 at 19:38 +0300, James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
>> <snip> 
>>> Er, but
>>>
>>>      1. struct cmdinfo is stack allocated; you can't dma to stack
>>>      2. even if you fix this, the structure is packed and will have the
>>>         original ppc coherence issues
>>>      3. Finally, even on the stack, it's not necessarily reachable with
>>>         the DMA mask
>>>
>> Ho, thanks for the catch. I really goofed here.
>>
>>> The whole point of managing sense buffer allocations centrally is to
>>> prevent driver writers from falling into all these traps.  I really
>>> don't think going back to hand rolled bounce buffering imporves the
>>> situation.
>>>
>> We don't have a choice Andi is removing all this. ISA is becoming
>> ISA's driver private problem.
> 
> No he's not .. not if it's going to cause this type of grief.
> 
> But I think you misread his patch set.  He still has a flag equivalent
> of unchecked_isa_dma  it's just called sense_buffer_isa instead.  The
> shift in his patch set is 
> 
>      1. it removes the need to allocate commands in dma'able memory
>      2. it removes the need for the ULD request path to allocate in
>         dma'able memory (it uses the block bounce path instead)
>      3. it *can't* remove the need to dma to the sense buffer, hence the
>         flag.
> 
> So it looks like the re-rolled patch set (when Andi gets around to
> posting it) will use one pool for the cmd buffer, but separate isa and
> non-isa pools for the sense buffer.  Thus it should just work for gdth
> with scsi_allocate_command predicated on sense_buffer_isa.
> 
> James
> 

I know all about Andi's patches, since I offered to help him as I
have some experience with scsi drivers. And it will be me that will
post the next round of ISA patches.

I have the complete body of work here:
http://git.open-osd.org/gitweb.cgi?p=open-osd.git;a=shortlog;h=boaz_sense_effort

I planed to first submit up to the Remove-ISA-dma tag and only then RFC and try
to push the rest of the work.

In the new ISA patchset we got rid of the sense_buffer_isa and converted all ISA
drivers to take care of their own sense problems. Which is much more simple then
one might think. This is because of 2 reasons. 1 - because to support it in Andi's
new mask allocation scheme was an hack, and 2 because I wanted to get rid of them
anyway.

I apologize in advance for any grief the all effort might cause, and I try to take
it upon myself to do all the work, I will very much appreciate any review, thanks
for your help already.

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 0/4] Use of new scsi_allocate_command

[James Bottomley]

On Thu, 2008-05-01 at 21:14 +0300, Boaz Harrosh wrote:
> On Thu, May 01 2008 at 20:33 +0300, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > On Thu, 2008-05-01 at 20:06 +0300, Boaz Harrosh wrote:
> >> On Thu, May 01 2008 at 19:38 +0300, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> >> <snip> 
> >>> Er, but
> >>>
> >>>      1. struct cmdinfo is stack allocated; you can't dma to stack
> >>>      2. even if you fix this, the structure is packed and will have the
> >>>         original ppc coherence issues
> >>>      3. Finally, even on the stack, it's not necessarily reachable with
> >>>         the DMA mask
> >>>
> >> Ho, thanks for the catch. I really goofed here.
> >>
> >>> The whole point of managing sense buffer allocations centrally is to
> >>> prevent driver writers from falling into all these traps.  I really
> >>> don't think going back to hand rolled bounce buffering imporves the
> >>> situation.
> >>>
> >> We don't have a choice Andi is removing all this. ISA is becoming
> >> ISA's driver private problem.
> > 
> > No he's not .. not if it's going to cause this type of grief.
> > 
> > But I think you misread his patch set.  He still has a flag equivalent
> > of unchecked_isa_dma  it's just called sense_buffer_isa instead.  The
> > shift in his patch set is 
> > 
> >      1. it removes the need to allocate commands in dma'able memory
> >      2. it removes the need for the ULD request path to allocate in
> >         dma'able memory (it uses the block bounce path instead)
> >      3. it *can't* remove the need to dma to the sense buffer, hence the
> >         flag.
> > 
> > So it looks like the re-rolled patch set (when Andi gets around to
> > posting it) will use one pool for the cmd buffer, but separate isa and
> > non-isa pools for the sense buffer.  Thus it should just work for gdth
> > with scsi_allocate_command predicated on sense_buffer_isa.
> > 
> > James
> > 
> 
> I know all about Andi's patches, since I offered to help him as I
> have some experience with scsi drivers. And it will be me that will
> post the next round of ISA patches.
> 
> I have the complete body of work here:
> http://git.open-osd.org/gitweb.cgi?p=open-osd.git;a=shortlog;h=boaz_sense_effort
> 
> I planed to first submit up to the Remove-ISA-dma tag and only then RFC and try
> to push the rest of the work.

But most of what you're doing in that patch set doesn't work.  The whole
reason for separating the sense buffer from the scsi command is the DMA
problem of cache line interference caused by doing DMA to an area
embedded in a structure that has host accessed variables in it.  With
what you're doing embedding the sense buffer in the internal control
areas, you're re-introducing that bug.

Additionally, it really doesn't make sense for every driver to do a roll
your own allocator.  It makes far more sense for them to be allocated
centrally (and correctly) so there's only one place it needs fixing if
there's yet another problem discovered.

James

Re: [PATCH 0/4] Use of new scsi_allocate_command

[Andi Kleen]

> But most of what you're doing in that patch set doesn't work.  The whole
> reason for separating the sense buffer from the scsi command is the DMA
> problem of cache line interference caused by doing DMA to an area
> embedded in a structure that has host accessed variables in it.  With
> what you're doing embedding the sense buffer in the internal control
> areas, you're re-introducing that bug.

Hmm if that was really a problem it would be trivial to pad it to cache
lines. On what card did you see that? Also I don't think the current
sense pool is cache line aligned anyways, so there is already
potentially this problem with other data.

Anyways from my audit of the ISA drivers I found that very few (only one
or two) do actually DMA to the sense buffer anyways. Most just memcpy
it from somewhere else. DMA is really the oddball case for the ISA
drivers (that is why with the old unchecked_isa_dma there was a lot of
unnecessary work for this)

> Additionally, it really doesn't make sense for every driver to do a roll
> your own allocator.  

It is only a very small number of drivers. And GFP_DMA in the mid layer
has many problems.

> It makes far more sense for them to be allocated
> centrally (and correctly) so there's only one place it needs fixing if
> there's yet another problem discovered.

The main issue with GFP_DMA in the mid layer (that is why i started my
patchkit originally) was that GFP_DMA does not work very well. First it
means different things on different architectures, then on x86 if you go
outside old legacy ISA drivers the drivers with limited DMA masks
typically don't need 24bit but something larger. I think for such
special situations (again we're talking only about a relatively small
number of drivers) it is much better to do the mapping and allocation in
the low level driver who knows far better what it really needs.

-Andi


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