scsi_free_command API change - Don't support GFP_DMA

PATCH New Boaz Harrosh 5 participants 19 messages May 1, 2008 6 messages not in the archive

Subsystems (3)

SCSI USB USB mass storage driver

lore

Share & tools

lore

Boaz Harrosh

author

2008-05-01 13:56 UTC #1 lore raw

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

In reply to a message not in the archive

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

---
 drivers/scsi/scsi.c      |   23 +++++++++--------------
 include/scsi/scsi_cmnd.h |    4 ++--
 2 files changed, 11 insertions(+), 16 deletions(-)

drivers/scsi/scsi.c +9 ~~-14~~

index 749c9c7..a120c04 100644
@@ -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);

include/scsi/scsi_cmnd.h +2 -2

index 8d20e60..2ebd52e 100644
@@ -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)
 {

Signature · 1 line

1.5.3.3

Boaz Harrosh

author

2008-05-01 13:59 UTC #2 lore raw

PATCH 2/4 isd200: Use new scsi_allocate_command()

In reply to a message not in the archive

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

CC Alan Stern <[email protected]>

CC Matthew Dharm <[email protected]>

---
 drivers/usb/storage/isd200.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

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

index 971d13d..756d30f 100644
@@ -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;

Signature · 1 line

1.5.3.3

Boaz Harrosh

author

2008-05-01 14:03 UTC #3 lore raw

PATCH 3/3 gdth: consolidate __gdth_execute && gdth_execute

In reply to a message not in the archive

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

---
 drivers/scsi/gdth.c      |   45 +++++++++++++++------------------------------
 drivers/scsi/gdth_proc.c |   16 ++++++++--------
 drivers/scsi/gdth_proc.h |    2 +-
 3 files changed, 24 insertions(+), 39 deletions(-)

drivers/scsi/gdth.c +15 ~~-30~~

index c6d6e7c..35ad3bf 100644
@@ -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;

drivers/scsi/gdth_proc.c +8 -8

index ce0228e..97c264a 100644
@@ -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;

drivers/scsi/gdth_proc.h +1 -1

index 45e6fda..faf4496 100644
@@ -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,

Signature · 1 line

1.5.3.3

Boaz Harrosh

author

2008-05-01 14:06 UTC #4 lore raw

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

In reply to a message not in the archive

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

---
 drivers/scsi/gdth.c |   12 +++---------
 1 files changed, 3 insertions(+), 9 deletions(-)

drivers/scsi/gdth.c +3 -9

index 35ad3bf..7778373 100644
@@ -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;
 }

Signature · 1 line

1.5.3.3

James Bottomley

maintainer

2008-05-01 14:24 UTC #5 lore raw

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

In reply to a message not in the archive

Quoted 3 lines · Boaz Harrosh

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

Boaz Harrosh

2008-05-01 14:56 UTC #7 lore raw

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

In reply to a message not in the archive

Quoted 9 lines · James Bottomley

Quoted 3 lines · Boaz Harrosh
[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.

Quoted 12 lines

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 <[email protected]>
Date:   Mon Mar 10 22:50:04 2008 +0100
[SCSI] gdth: Allocate sense_buffer to prevent NULL pointer dereference

Quoted 9 lines

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

---
 drivers/scsi/gdth.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

drivers/scsi/gdth.c +1 -6

index 35ad3bf..32a5ee2 100644
@@ -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;
-    }

Boaz Harrosh

author

2008-05-01 15:13 UTC #8 lore raw

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

Quoted 11 lines · Boaz Harrosh

---
From: Boaz Harrosh <[email protected]>
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 <[email protected]>
---

<snip>

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

---
From: Boaz Harrosh <[email protected]>
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 <[email protected]>

---
 drivers/scsi/gdth.c |   13 +++----------
 1 files changed, 3 insertions(+), 10 deletions(-)

drivers/scsi/gdth.c +3 ~~-10~~

index 35ad3bf..3cfbab6 100644
@@ -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;
 }

Signature · 1 line

1.5.3.3

James Bottomley

maintainer

2008-05-01 15:22 UTC #9 lore raw

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

In reply to Boaz Harrosh

Quoted 2 lines · Boaz Harrosh

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

Boaz Harrosh

author

2008-05-01 15:38 UTC #10 lore raw

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

Quoted 6 lines · James Bottomley

Quoted 2 lines · Boaz Harrosh
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.

Quoted 9 lines

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

James Bottomley

2008-05-01 15:47 UTC #12 lore raw

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

In reply to a message not in the archive

Quoted 10 lines · Boaz Harrosh

Quoted 9 lines · James Bottomley
Quoted 2 lines · Boaz Harrosh
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.

Quoted 7 lines

Quoted 6 lines
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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Boaz Harrosh

author

2008-05-01 15:59 UTC #13 lore raw

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

Quoted 14 lines · James Bottomley

Quoted 13 lines · Boaz Harrosh
Quoted 12 lines · James Bottomley
Quoted 2 lines · Boaz Harrosh
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.

Quoted 10 lines

Quoted 3 lines
Quoted 2 lines
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

James Bottomley

maintainer

2008-05-01 16:02 UTC #14 lore raw

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

Quoted 22 lines · Boaz Harrosh

Quoted 21 lines · James Bottomley
Quoted 9 lines · Boaz Harrosh
Quoted 8 lines · James Bottomley
Quoted 2 lines · Boaz Harrosh
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.
Quoted 7 lines
Quoted 6 lines
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 ...

Quoted 7 lines

Quoted 3 lines
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

Boaz Harrosh

2008-05-01 16:25 UTC #16 lore raw

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

In reply to a message not in the archive

Quoted 25 lines · James Bottomley

Quoted 24 lines · Boaz Harrosh
Quoted 23 lines · James Bottomley
Quoted 9 lines · Boaz Harrosh
Quoted 8 lines · James Bottomley
Quoted 2 lines · Boaz Harrosh
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.
Quoted 7 lines
Quoted 6 lines
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

Quoted 10 lines

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

---
 drivers/scsi/gdth.c |   11 ++++++++---
 drivers/scsi/gdth.h |    1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

drivers/scsi/gdth.c +8 -3

index 3cfbab6..fb61c79 100644
@@ -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;

drivers/scsi/gdth.h +1 -0

index ca92476..445ea7f 100644
@@ -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;

Signature · 6 lines

1.5.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

James Bottomley

maintainer

2008-05-01 16:38 UTC #17 lore raw

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

Quoted 91 lines · Boaz Harrosh

Quoted 25 lines · James Bottomley
Quoted 24 lines · Boaz Harrosh
Quoted 23 lines · James Bottomley
Quoted 9 lines · Boaz Harrosh
Quoted 8 lines · James Bottomley
Quoted 2 lines · Boaz Harrosh
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.
Quoted 7 lines
Quoted 6 lines
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
Quoted 10 lines
Quoted 3 lines
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 <[email protected]>
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 <[email protected]>
---
 drivers/scsi/gdth.c |   11 ++++++++---
 drivers/scsi/gdth.h |    1 +
 2 files changed, 9 insertions(+), 3 deletions(-)
drivers/scsi/gdth.c +8 -3
index 3cfbab6..fb61c79 100644
@@ -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;
drivers/scsi/gdth.h +0 -0
index ca92476..445ea7f 100644

drivers/scsi/gdth.h typedef struct { · line 920

Quoted 7 lines

drivers/scsi/gdth.h +1 -0

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

Boaz Harrosh

author

2008-05-01 17:06 UTC #18 lore raw

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

On Thu, May 01 2008 at 19:38 +0300, James Bottomley <[email protected]> wrote:
<snip>

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 3 lines

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

---
 drivers/scsi/gdth.c |   33 +++++++++++++++++++++------------
 drivers/scsi/gdth.h |    1 +
 2 files changed, 22 insertions(+), 12 deletions(-)

drivers/scsi/gdth.c +21 ~~-12~~

index 3cfbab6..805c616 100644
@@ -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);

drivers/scsi/gdth.h +1 -0

index ca92476..445ea7f 100644
@@ -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;

Signature · 1 line

1.5.3.3

James Bottomley

maintainer

2008-05-01 17:33 UTC #19 lore raw

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

Quoted 13 lines · Boaz Harrosh

On Thu, May 01 2008 at 19:38 +0300, James Bottomley <[email protected]> wrote:
<snip>
Quoted 8 lines
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.

Quoted 8 lines

Quoted 5 lines
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

Boaz Harrosh

2008-05-01 18:14 UTC #21 lore raw

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

In reply to a message not in the archive

Quoted 33 lines · James Bottomley

Quoted 12 lines · Boaz Harrosh
On Thu, May 01 2008 at 19:38 +0300, James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/[email protected]> wrote:
<snip>
Quoted 8 lines
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.
Quoted 7 lines
Quoted 5 lines
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.

Quoted 7 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

James Bottomley

maintainer

2008-05-01 20:33 UTC #22 lore raw

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

Quoted 46 lines · Boaz Harrosh

Quoted 33 lines · James Bottomley
Quoted 12 lines · Boaz Harrosh
On Thu, May 01 2008 at 19:38 +0300, James Bottomley <[email protected]> wrote:
<snip>
Quoted 8 lines
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.
Quoted 7 lines
Quoted 5 lines
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.
Quoted 7 lines
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.

Quoted 5 lines

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

Andi Kleen

2008-05-01 21:41 UTC #24 lore raw

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

In reply to a message not in the archive

Quoted 7 lines

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)

Quoted 2 lines

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.

Quoted 3 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html