[patch 0/8] Change SG_ALL to mean "any size"

[patch 0/8] Change SG_ALL to mean "any size"

[Boaz Harrosh]

I have scanned through the scsi tree and checked all places that used
SG_ALL. Those drivers that where not ready for SG_ALL to mean "any size"
where changed to what ever is appropriate for this driver.
Mainly 3 groups.

Those that meant SG_ALL == 255:

  [PATCH 1/8] firewire: ieee1394: Move away from SG_ALL
  [PATCH 2/8] scsi-drivers Don't use SG_ALL as allocation size

Those who are not sg-chaining ready:

  [PATCH 3/8] NCR5380: Not sg-chain ready
  [PATCH 4/8] wd33c93: Not sg-chain ready
  [PATCH 5/8] arm/scsi: Not sg-chain ready
  [PATCH 6/8] scsi: Drivers not ready for sg-chaining

And those that just got confused:
  [PATCH 7/8] a100u2w: advansys: initio: Wrong use of SG_ALL


And finally the switch of SG_ALL to ~0, that is MAX_ what ever
type it is.
  [PATCH 8/8] Change SG_ALL to mean "any size"

Please request if you want any patches squashed or split.

These are based on the scsi-bidi git-tree and mainly on top
of the: "[SCSI] remove use_sg_chaining" patch. James do you
want that I reverse the dependency so your patch will be on 
top of these patches? (For bisect-ability)

Boaz

[PATCH 1/8] firewire: ieee1394: Move away from SG_ALL

[Boaz Harrosh]

  SG_ALL wants to be ~0 meaning "any future size".
  Below group of drivers preallocate a scatter list
  buffer of max_size, so set that size to be 255 (Like before).

  A better schema can be advised with a more dynamic allocation.
  Perhaps from a kmem_cache.

  List of drivers/files:
	drivers/firewire/fw-sbp2.c
	drivers/ieee1394/sbp2.[ch]

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/firewire/fw-sbp2.c |    6 ++++--
 drivers/ieee1394/sbp2.c    |    2 +-
 drivers/ieee1394/sbp2.h    |    4 +++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/firewire/fw-sbp2.c b/drivers/firewire/fw-sbp2.c
index c2169d2..078501c 100644
--- a/drivers/firewire/fw-sbp2.c
+++ b/drivers/firewire/fw-sbp2.c
@@ -151,6 +151,7 @@ struct sbp2_target {
 };
 
 #define SBP2_MAX_SG_ELEMENT_LENGTH	0xf000
+#define SBP2_MAX_SG_COUNT		255
 #define SBP2_MAX_SECTORS		255	/* Max sectors supported */
 #define SBP2_ORB_TIMEOUT		2000	/* Timeout in ms */
 
@@ -272,7 +273,8 @@ struct sbp2_command_orb {
 	scsi_done_fn_t done;
 	struct sbp2_logical_unit *lu;
 
-	struct sbp2_pointer page_table[SG_ALL] __attribute__((aligned(8)));
+	struct sbp2_pointer page_table[SBP2_MAX_SG_COUNT]
+						__attribute__((aligned(8)));
 	dma_addr_t page_table_bus;
 };
 
@@ -1329,7 +1331,7 @@ static struct scsi_host_template scsi_driver_template = {
 	.slave_configure	= sbp2_scsi_slave_configure,
 	.eh_abort_handler	= sbp2_scsi_abort,
 	.this_id		= -1,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= SBP2_MAX_SG_COUNT,
 	.use_clustering		= ENABLE_CLUSTERING,
 	.cmd_per_lun		= 1,
 	.can_queue		= 1,
diff --git a/drivers/ieee1394/sbp2.c b/drivers/ieee1394/sbp2.c
index 1eda11a..070763a 100644
--- a/drivers/ieee1394/sbp2.c
+++ b/drivers/ieee1394/sbp2.c
@@ -324,7 +324,7 @@ static struct scsi_host_template sbp2_shost_template = {
 	.slave_configure	 = sbp2scsi_slave_configure,
 	.slave_destroy		 = sbp2scsi_slave_destroy,
 	.this_id		 = -1,
-	.sg_tablesize		 = SG_ALL,
+	.sg_tablesize		 = SBP2_MAX_SG_COUNT,
 	.use_clustering		 = ENABLE_CLUSTERING,
 	.cmd_per_lun		 = SBP2_MAX_CMDS,
 	.can_queue		 = SBP2_MAX_CMDS,
diff --git a/drivers/ieee1394/sbp2.h b/drivers/ieee1394/sbp2.h
index 333a4bb..ae31788 100644
--- a/drivers/ieee1394/sbp2.h
+++ b/drivers/ieee1394/sbp2.h
@@ -222,6 +222,7 @@ struct sbp2_status_block {
  */
 
 #define SBP2_MAX_SG_ELEMENT_LENGTH		0xf000
+#define SBP2_MAX_SG_COUNT			255
 #define SBP2_MAX_SECTORS			255
 /* There is no real limitation of the queue depth (i.e. length of the linked
  * list of command ORBs) at the target. The chosen depth is merely an
@@ -257,7 +258,8 @@ struct sbp2_command_info {
 
 	/* Also need s/g structure for each sbp2 command */
 	struct sbp2_unrestricted_page_table
-		scatter_gather_element[SG_ALL] __attribute__((aligned(8)));
+		scatter_gather_element[SBP2_MAX_SG_COUNT]
+						__attribute__((aligned(8)));
 	dma_addr_t sge_dma;
 	void *sge_buffer;
 	dma_addr_t cmd_dma;
-- 
1.5.3.3

Re: [PATCH 1/8] firewire: ieee1394: Move away from SG_ALL

[Stefan Richter]

Boaz Harrosh wrote:
>   SG_ALL wants to be ~0 meaning "any future size".
>   Below group of drivers preallocate a scatter list
>   buffer of max_size, so set that size to be 255 (Like before).
> 
>   A better schema can be advised with a more dynamic allocation.
>   Perhaps from a kmem_cache.
> 
>   List of drivers/files:
> 	drivers/firewire/fw-sbp2.c
> 	drivers/ieee1394/sbp2.[ch]
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>

Acked-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

> ---
>  drivers/firewire/fw-sbp2.c |    6 ++++--
>  drivers/ieee1394/sbp2.c    |    2 +-
>  drivers/ieee1394/sbp2.h    |    4 +++-
>  3 files changed, 8 insertions(+), 4 deletions(-)

Since I wasn't Cc'd, I suppose you don't want me to take it upstream via
linux1394-2.6.git?

My guess is that there won't be conflicts of this with linux1394-2.6.git
anytime soon, so taking it into any other tree should be fine.
-- 
Stefan Richter
-=====-==--- ---= =---=
http://arcgraph.de/sr/

Re: [PATCH 1/8] firewire: ieee1394: Move away from SG_ALL

[Stefan Richter]

Stefan Richter wrote:
> My guess is that there won't be conflicts of this with linux1394-2.6.git
> anytime soon, so taking it into any other tree should be fine.

Wrong, there should already be a (trivial) conflict due to a patch from
December.  Another (trivially) conflicting patch was sent to
linux1394-devel a few hours ago.  Both patches will land in Linus' tree
early during the next merge window.  So either refresh the patch when
it's time, or give it to me.  (Whatever, try not to add to akpm's pain.)
-- 
Stefan Richter
-=====-==--- ---= =--==
http://arcgraph.de/sr/

Re: [PATCH 1/8] firewire: ieee1394: Move away from SG_ALL

[James Bottomley]

On Sat, 2008-01-19 at 08:59 +0100, Stefan Richter wrote:
> Stefan Richter wrote:
> > My guess is that there won't be conflicts of this with linux1394-2.6.git
> > anytime soon, so taking it into any other tree should be fine.
> 
> Wrong, there should already be a (trivial) conflict due to a patch from
> December.  Another (trivially) conflicting patch was sent to
> linux1394-devel a few hours ago.  Both patches will land in Linus' tree
> early during the next merge window.  So either refresh the patch when
> it's time, or give it to me.  (Whatever, try not to add to akpm's pain.)

Actually, I'm already tracking potential conflicts.  The
linux1394-2.6.git is part of the merge candidate tree I'm building
nightly.  These patches will go through the (now slightly misnamed)
scsi-bidi-2.6.git tree.  That one's built on top of all the currently
conflicting trees, and is scheduled to be the last thing merged in the
merge window (or at least to wait until the others its built on have
gone in).

James

Re: [PATCH 1/8] firewire: ieee1394: Move away from SG_ALL

[Stefan Richter]

James Bottomley wrote:
> Actually, I'm already tracking potential conflicts.  The
> linux1394-2.6.git is part of the merge candidate tree I'm building
> nightly.

Right, I saw your announcement of this tree.

> These patches will go through the (now slightly misnamed)
> scsi-bidi-2.6.git tree.  That one's built on top of all the currently
> conflicting trees, and is scheduled to be the last thing merged in the
> merge window (or at least to wait until the others its built on have
> gone in).

Good, everything should come together nicely then.
-- 
Stefan Richter
-=====-==--- ---= =--==
http://arcgraph.de/sr/

[PATCH 2/8] scsi-drivers Don't use SG_ALL as allocation size

[Boaz Harrosh]

  below list of drivers have used SG_ALL as a size to
  preallocate maximum possible command's sg-count.
  This is no longer possible since the maximum is not
  set at compile time but as a run time configuration.

  A better schema can be advised with a more dynamic allocation.
  Perhaps from a kmem_cache.

  Affected drivers/files:
	drivers/scsi/atari_scsi.[ch]
	drivers/scsi/eata_pio.c
	drivers/scsi/ibmvscsi/ibmvscsi.[ch]
	drivers/scsi/mac53c94.c
	drivers/scsi/mesh.c
	drivers/scsi/nsp32.h
	drivers/scsi/qla1280.c
	drivers/scsi/qla2xxx/qla_os.c
	drivers/scsi/qla4xxx/ql4_def.h

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/atari_scsi.c        |    8 ++++----
 drivers/scsi/atari_scsi.h        |    4 ++--
 drivers/scsi/eata_pio.c          |    5 ++++-
 drivers/scsi/ibmvscsi/ibmvscsi.c |    8 +++++---
 drivers/scsi/ibmvscsi/ibmvscsi.h |    1 +
 drivers/scsi/mac53c94.c          |    8 ++++++--
 drivers/scsi/mesh.c              |    5 ++++-
 drivers/scsi/nsp32.h             |    2 +-
 drivers/scsi/qla1280.c           |    3 ++-
 drivers/scsi/qla2xxx/qla_os.c    |    2 ++
 drivers/scsi/qla4xxx/ql4_def.h   |    1 +
 11 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index f5732d8..b47b61d 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -620,7 +620,8 @@ int atari_scsi_detect(struct scsi_host_template *host)
 	/* Force sg_tablesize to 0 on a Falcon! */
 	host->sg_tablesize =
 		!IS_A_TT() ? ATARI_FALCON_SG_TABLESIZE :
-		(setup_sg_tablesize >= 0) ? setup_sg_tablesize : ATARI_TT_SG_TABLESIZE;
+		(setup_sg_tablesize > 0) ? setup_sg_tablesize :
+							ATARI_TT_SG_TABLESIZE;
 
 	if (setup_hostid >= 0)
 		host->this_id = setup_hostid;
@@ -792,9 +793,8 @@ void __init atari_scsi_setup(char *str, int *ints)
 	if (ints[0] >= 3) {
 		if (ints[3] >= 0) {
 			setup_sg_tablesize = ints[3];
-			/* Must be <= SG_ALL (255) */
-			if (setup_sg_tablesize > SG_ALL)
-				setup_sg_tablesize = SG_ALL;
+			if (setup_sg_tablesize > ATARI_TT_SG_TABLESIZE)
+				setup_sg_tablesize = ATARI_TT_SG_TABLESIZE;
 		}
 	}
 	if (ints[0] >= 4) {
diff --git a/drivers/scsi/atari_scsi.h b/drivers/scsi/atari_scsi.h
index efadb8d..21b8c91 100644
--- a/drivers/scsi/atari_scsi.h
+++ b/drivers/scsi/atari_scsi.h
@@ -35,11 +35,11 @@ int atari_scsi_release (struct Scsi_Host *);
 
 #define ATARI_TT_CAN_QUEUE		16
 #define ATARI_TT_CMD_PER_LUN		8
-#define ATARI_TT_SG_TABLESIZE		SG_ALL
+#define ATARI_TT_SG_TABLESIZE		255
 
 #define ATARI_FALCON_CAN_QUEUE		8
 #define ATARI_FALCON_CMD_PER_LUN	1
-#define ATARI_FALCON_SG_TABLESIZE	SG_NONE
+#define ATARI_FALCON_SG_TABLESIZE	1
 
 #define	DEFAULT_USE_TAGGED_QUEUING	0
 
diff --git a/drivers/scsi/eata_pio.c b/drivers/scsi/eata_pio.c
index b5a6092..8913d9d 100644
--- a/drivers/scsi/eata_pio.c
+++ b/drivers/scsi/eata_pio.c
@@ -691,6 +691,9 @@ static unsigned int print_selftest(unsigned int base)
 	return (!(inb(base + HA_RSTATUS) & HA_SERROR));
 }
 
+/* FIXME: is this driver sg-chainning ready? */
+#define EATA_MAX_SG 255
+
 static int register_pio_HBA(long base, struct get_conf *gc, struct pci_dev *pdev)
 {
 	unsigned long size = 0;
@@ -801,7 +804,7 @@ static int register_pio_HBA(long base, struct get_conf *gc, struct pci_dev *pdev
 	sh->this_id = gc->scsi_id[3];
 	sh->can_queue = 1;
 	sh->cmd_per_lun = 1;
-	sh->sg_tablesize = SG_ALL;
+	sh->sg_tablesize = EATA_MAX_SG;
 
 	hd->channel = 0;
 
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 78d46a9..9b1fc94 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -173,7 +173,8 @@ static void release_event_pool(struct event_pool *pool,
 			++in_use;
 		if (pool->events[i].ext_list) {
 			dma_free_coherent(hostdata->dev,
-				  SG_ALL * sizeof(struct srp_direct_buf),
+				  IBMVSCSI_MAX_SG *
+						sizeof(struct srp_direct_buf),
 				  pool->events[i].ext_list,
 				  pool->events[i].ext_list_token);
 		}
@@ -423,7 +424,8 @@ static int map_sg_data(struct scsi_cmnd *cmd,
 	if (!evt_struct->ext_list) {
 		evt_struct->ext_list = (struct srp_direct_buf *)
 			dma_alloc_coherent(dev,
-					   SG_ALL * sizeof(struct srp_direct_buf),
+					   IBMVSCSI_MAX_SG *
+						sizeof(struct srp_direct_buf),
 					   &evt_struct->ext_list_token, 0);
 		if (!evt_struct->ext_list) {
 			sdev_printk(KERN_ERR, cmd->device,
@@ -1598,7 +1600,7 @@ static struct scsi_host_template driver_template = {
 	.cmd_per_lun = 16,
 	.can_queue = IBMVSCSI_MAX_REQUESTS_DEFAULT,
 	.this_id = -1,
-	.sg_tablesize = SG_ALL,
+	.sg_tablesize = IBMVSCSI_MAX_SG,
 	.use_clustering = ENABLE_CLUSTERING,
 	.shost_attrs = ibmvscsi_attrs,
 };
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.h b/drivers/scsi/ibmvscsi/ibmvscsi.h
index 46e850e..e3e3a2e 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.h
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.h
@@ -46,6 +46,7 @@ struct Scsi_Host;
 
 #define IBMVSCSI_MAX_REQUESTS_DEFAULT 100
 #define IBMVSCSI_MAX_CMDS_PER_LUN 64
+#define IBMVSCSI_MAX_SG 255
 
 /* ------------------------------------------------------------
  * Data Structures
diff --git a/drivers/scsi/mac53c94.c b/drivers/scsi/mac53c94.c
index b12ad7c..9ef0272 100644
--- a/drivers/scsi/mac53c94.c
+++ b/drivers/scsi/mac53c94.c
@@ -392,6 +392,9 @@ static void set_dma_cmds(struct fsc_state *state, struct scsi_cmnd *cmd)
 	cmd->SCp.this_residual = total;
 }
 
+/* Dynamicaly allocated at init */
+#define MAC53c94_MAX_SG 255
+
 static struct scsi_host_template mac53c94_template = {
 	.proc_name	= "53c94",
 	.name		= "53C94",
@@ -399,7 +402,7 @@ static struct scsi_host_template mac53c94_template = {
 	.eh_host_reset_handler = mac53c94_host_reset,
 	.can_queue	= 1,
 	.this_id	= 7,
-	.sg_tablesize	= SG_ALL,
+	.sg_tablesize	= MAC53c94_MAX_SG,
 	.cmd_per_lun	= 1,
 	.use_clustering	= DISABLE_CLUSTERING,
 };
@@ -464,7 +467,8 @@ static int mac53c94_probe(struct macio_dev *mdev, const struct of_device_id *mat
 	 * XXX FIXME: Use DMA consistent routines
 	 */
        	dma_cmd_space = kmalloc((host->sg_tablesize + 2) *
-       				sizeof(struct dbdma_cmd), GFP_KERNEL);
+				sizeof(struct dbdma_cmd),
+				GFP_KERNEL | GFP_DMA);
        	if (dma_cmd_space == 0) {
        		printk(KERN_ERR "mac53c94: couldn't allocate dma "
        		       "command space for %s\n", node->full_name);
diff --git a/drivers/scsi/mesh.c b/drivers/scsi/mesh.c
index 651d09b..e4e151d 100644
--- a/drivers/scsi/mesh.c
+++ b/drivers/scsi/mesh.c
@@ -1832,6 +1832,9 @@ static int mesh_shutdown(struct macio_dev *mdev)
 	return 0;
 }
 
+/* allocated at probe time */
+#define MESH_MAX_SG 255 
+
 static struct scsi_host_template mesh_template = {
 	.proc_name			= "mesh",
 	.name				= "MESH",
@@ -1840,7 +1843,7 @@ static struct scsi_host_template mesh_template = {
 	.eh_host_reset_handler		= mesh_host_reset,
 	.can_queue			= 20,
 	.this_id			= 7,
-	.sg_tablesize			= SG_ALL,
+	.sg_tablesize			= MESH_MAX_SG,
 	.cmd_per_lun			= 2,
 	.use_clustering			= DISABLE_CLUSTERING,
 };
diff --git a/drivers/scsi/nsp32.h b/drivers/scsi/nsp32.h
index 6715ecb..ace52f5 100644
--- a/drivers/scsi/nsp32.h
+++ b/drivers/scsi/nsp32.h
@@ -444,7 +444,7 @@ typedef u16 u16_le;
 /************************************************************************
  * structure for DMA/Scatter Gather list
  */
-#define NSP32_SG_SIZE		SG_ALL
+#define NSP32_SG_SIZE		255
 
 typedef struct _nsp32_sgtable {
 	/* values must be little endian */
diff --git a/drivers/scsi/qla1280.c b/drivers/scsi/qla1280.c
index 68c0d09..c0319da 100644
--- a/drivers/scsi/qla1280.c
+++ b/drivers/scsi/qla1280.c
@@ -422,6 +422,7 @@
 #define IS_ISP1x160(ha)        (ha->pdev->device == PCI_DEVICE_ID_QLOGIC_ISP10160 || \
 				ha->pdev->device == PCI_DEVICE_ID_QLOGIC_ISP12160)
 
+#define QLA_MAX_SG 255
 
 static int qla1280_probe_one(struct pci_dev *, const struct pci_device_id *);
 static void qla1280_remove_one(struct pci_dev *);
@@ -4201,7 +4202,7 @@ static struct scsi_host_template qla1280_driver_template = {
 	.bios_param		= qla1280_biosparam,
 	.can_queue		= 0xfffff,
 	.this_id		= -1,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= QLA_MAX_SG,
 	.cmd_per_lun		= 1,
 	.use_clustering		= ENABLE_CLUSTERING,
 };
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index 78d1103..e368f38 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -111,6 +111,8 @@ static int qla2x00_device_reset(scsi_qla_host_t *, fc_port_t *);
 static int qla2x00_change_queue_depth(struct scsi_device *, int);
 static int qla2x00_change_queue_type(struct scsi_device *, int);
 
+#define QLA2XXX_MAX_SG 255
+
 struct scsi_host_template qla2x00_driver_template = {
 	.module			= THIS_MODULE,
 	.name			= QLA2XXX_DRIVER_NAME,
diff --git a/drivers/scsi/qla4xxx/ql4_def.h b/drivers/scsi/qla4xxx/ql4_def.h
index accaf69..64cd43b 100644
--- a/drivers/scsi/qla4xxx/ql4_def.h
+++ b/drivers/scsi/qla4xxx/ql4_def.h
@@ -101,6 +101,7 @@
 #define MBOX_AEN_REG_COUNT	5
 #define MAX_INIT_RETRIES	5
 #define IOCB_HIWAT_CUSHION	16
+#define QLA_MAX_SG		255
 
 /*
  * Buffer sizes
-- 
1.5.3.3

Re: [PATCH 2/8] scsi-drivers Don't use SG_ALL as allocation size

[Andrew Vasquez]

On Thu, 17 Jan 2008, Boaz Harrosh wrote:

>   below list of drivers have used SG_ALL as a size to
>   preallocate maximum possible command's sg-count.
>   This is no longer possible since the maximum is not
>   set at compile time but as a run time configuration.
> 
>   A better schema can be advised with a more dynamic allocation.
>   Perhaps from a kmem_cache.
> 
>   Affected drivers/files:
> 	drivers/scsi/atari_scsi.[ch]
> 	drivers/scsi/eata_pio.c
> 	drivers/scsi/ibmvscsi/ibmvscsi.[ch]
> 	drivers/scsi/mac53c94.c
> 	drivers/scsi/mesh.c
> 	drivers/scsi/nsp32.h
> 	drivers/scsi/qla1280.c
> 	drivers/scsi/qla2xxx/qla_os.c
> 	drivers/scsi/qla4xxx/ql4_def.h
...

There's no functional change in your patches to qla2xxx and qla4xxx.
Perhaps a cut-n-paste typo from qla1280.c (which should be capped):

> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 78d1103..e368f38 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
> @@ -111,6 +111,8 @@ static int qla2x00_device_reset(scsi_qla_host_t *, fc_port_t *);
>  static int qla2x00_change_queue_depth(struct scsi_device *, int);
>  static int qla2x00_change_queue_type(struct scsi_device *, int);
>  
> +#define QLA2XXX_MAX_SG 255
> +
>  struct scsi_host_template qla2x00_driver_template = {
>  	.module			= THIS_MODULE,
>  	.name			= QLA2XXX_DRIVER_NAME,
> diff --git a/drivers/scsi/qla4xxx/ql4_def.h b/drivers/scsi/qla4xxx/ql4_def.h
> index accaf69..64cd43b 100644
> --- a/drivers/scsi/qla4xxx/ql4_def.h
> +++ b/drivers/scsi/qla4xxx/ql4_def.h
> @@ -101,6 +101,7 @@
>  #define MBOX_AEN_REG_COUNT	5
>  #define MAX_INIT_RETRIES	5
>  #define IOCB_HIWAT_CUSHION	16
> +#define QLA_MAX_SG		255


I don't forsee any issues with maintaining SG_ALL (~0) usage within
qla2xxx and qla4xxx, as the number of unused entries on the HBA's
request-queue shall ultimately be the rate-limiting factor.

--
av

Re: [PATCH 2/8] scsi-drivers Don't use SG_ALL as allocation size

[Boaz Harrosh]

On Thu, Jan 17 2008 at 19:47 +0200, Andrew Vasquez <andrew.vasquez@qlogic.com> wrote:
> On Thu, 17 Jan 2008, Boaz Harrosh wrote:
> 
>>   below list of drivers have used SG_ALL as a size to
>>   preallocate maximum possible command's sg-count.
>>   This is no longer possible since the maximum is not
>>   set at compile time but as a run time configuration.
>>
>>   A better schema can be advised with a more dynamic allocation.
>>   Perhaps from a kmem_cache.
>>
>>   Affected drivers/files:
>> 	drivers/scsi/atari_scsi.[ch]
>> 	drivers/scsi/eata_pio.c
>> 	drivers/scsi/ibmvscsi/ibmvscsi.[ch]
>> 	drivers/scsi/mac53c94.c
>> 	drivers/scsi/mesh.c
>> 	drivers/scsi/nsp32.h
>> 	drivers/scsi/qla1280.c
>> 	drivers/scsi/qla2xxx/qla_os.c
>> 	drivers/scsi/qla4xxx/ql4_def.h
> ...
> 
> There's no functional change in your patches to qla2xxx and qla4xxx.
> Perhaps a cut-n-paste typo from qla1280.c (which should be capped):
> 
>> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
>> index 78d1103..e368f38 100644
>> --- a/drivers/scsi/qla2xxx/qla_os.c
>> +++ b/drivers/scsi/qla2xxx/qla_os.c
>> @@ -111,6 +111,8 @@ static int qla2x00_device_reset(scsi_qla_host_t *, fc_port_t *);
>>  static int qla2x00_change_queue_depth(struct scsi_device *, int);
>>  static int qla2x00_change_queue_type(struct scsi_device *, int);
>>  
>> +#define QLA2XXX_MAX_SG 255
>> +
>>  struct scsi_host_template qla2x00_driver_template = {
>>  	.module			= THIS_MODULE,
>>  	.name			= QLA2XXX_DRIVER_NAME,
>> diff --git a/drivers/scsi/qla4xxx/ql4_def.h b/drivers/scsi/qla4xxx/ql4_def.h
>> index accaf69..64cd43b 100644
>> --- a/drivers/scsi/qla4xxx/ql4_def.h
>> +++ b/drivers/scsi/qla4xxx/ql4_def.h
>> @@ -101,6 +101,7 @@
>>  #define MBOX_AEN_REG_COUNT	5
>>  #define MAX_INIT_RETRIES	5
>>  #define IOCB_HIWAT_CUSHION	16
>> +#define QLA_MAX_SG		255
> 
> 
> I don't forsee any issues with maintaining SG_ALL (~0) usage within
> qla2xxx and qla4xxx, as the number of unused entries on the HBA's
> request-queue shall ultimately be the rate-limiting factor.
> 
> --
Please forgive me I have forgot to put the use of this constants at
body, see patch below.

I have seen the use of a device memory at the scsi_for_each_sg loop and was
not at all sure what is the size limit for that. So I played it safe,
actually I changed nothing. Please give me the OK to remove ql[24]xxx from
these patches, and have them change to SG_ALL==~0 in the last patch.
Or should I resend the patch with below added.

Thanks for catching this
Boaz

--  
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
index e368f38..b0999b6 100644
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -134,7 +134,7 @@ struct scsi_host_template qla2x00_driver_template = {
 	.this_id		= -1,
 	.cmd_per_lun		= 3,
 	.use_clustering		= ENABLE_CLUSTERING,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= QLA2XXX_MAX_SG,
 
 	/*
 	 * The RISC allows for each command to transfer (2^32-1) bytes of data,
@@ -165,7 +165,7 @@ struct scsi_host_template qla24xx_driver_template = {
 	.this_id		= -1,
 	.cmd_per_lun		= 3,
 	.use_clustering		= ENABLE_CLUSTERING,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= QLA2XXX_MAX_SG,
 
 	.max_sectors		= 0xFFFF,
 	.shost_attrs		= qla2x00_host_attrs,
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
index b128da5..0b8de79 100644
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -94,7 +94,7 @@ static struct scsi_host_template qla4xxx_driver_template = {
 	.this_id		= -1,
 	.cmd_per_lun		= 3,
 	.use_clustering		= ENABLE_CLUSTERING,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= QLA_MAX_SG,
 
 	.max_sectors		= 0xFFFF,
 };

Re: [PATCH 2/8] scsi-drivers Don't use SG_ALL as allocation size

[Andrew Vasquez]

On Thu, 17 Jan 2008, Boaz Harrosh wrote:

> > I don't forsee any issues with maintaining SG_ALL (~0) usage within
> > qla2xxx and qla4xxx, as the number of unused entries on the HBA's
> > request-queue shall ultimately be the rate-limiting factor.
> > 
> > --
> Please forgive me I have forgot to put the use of this constants at
> body, see patch below.
> 
> I have seen the use of a device memory at the scsi_for_each_sg loop and was
> not at all sure what is the size limit for that. So I played it safe,
> actually I changed nothing. Please give me the OK to remove ql[24]xxx from
> these patches,

I believe that's best, simply remove the qla2xxx/qla4xxx component
from your patchset.

Thanks, AV

Re: [PATCH 2/8 ver2] scsi-drivers Don't use SG_ALL as allocation size

[Boaz Harrosh]

Subject: [PATCH] scsi-drivers Don't use SG_ALL as allocation size

  below list of drivers have used SG_ALL as a size to
  preallocate maximum possible command's sg-count.
  This is no longer possible since the maximum is not
  set at compile time but as a run time configuration.

  A better schema can be advised with a more dynamic allocation.
  Perhaps from a kmem_cache.

  Affected drivers/files:
	drivers/scsi/atari_scsi.[ch]
	drivers/scsi/eata_pio.c
	drivers/scsi/ibmvscsi/ibmvscsi.[ch]
	drivers/scsi/mac53c94.c
	drivers/scsi/mesh.c
	drivers/scsi/nsp32.h
	drivers/scsi/qla1280.c

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/atari_scsi.c        |    8 ++++----
 drivers/scsi/atari_scsi.h        |    4 ++--
 drivers/scsi/eata_pio.c          |    5 ++++-
 drivers/scsi/ibmvscsi/ibmvscsi.c |    8 +++++---
 drivers/scsi/ibmvscsi/ibmvscsi.h |    1 +
 drivers/scsi/mac53c94.c          |    8 ++++++--
 drivers/scsi/mesh.c              |    5 ++++-
 drivers/scsi/nsp32.h             |    2 +-
 drivers/scsi/qla1280.c           |    3 ++-
 9 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/atari_scsi.c b/drivers/scsi/atari_scsi.c
index f5732d8..b47b61d 100644
--- a/drivers/scsi/atari_scsi.c
+++ b/drivers/scsi/atari_scsi.c
@@ -620,7 +620,8 @@ int atari_scsi_detect(struct scsi_host_template *host)
 	/* Force sg_tablesize to 0 on a Falcon! */
 	host->sg_tablesize =
 		!IS_A_TT() ? ATARI_FALCON_SG_TABLESIZE :
-		(setup_sg_tablesize >= 0) ? setup_sg_tablesize : ATARI_TT_SG_TABLESIZE;
+		(setup_sg_tablesize > 0) ? setup_sg_tablesize :
+							ATARI_TT_SG_TABLESIZE;
 
 	if (setup_hostid >= 0)
 		host->this_id = setup_hostid;
@@ -792,9 +793,8 @@ void __init atari_scsi_setup(char *str, int *ints)
 	if (ints[0] >= 3) {
 		if (ints[3] >= 0) {
 			setup_sg_tablesize = ints[3];
-			/* Must be <= SG_ALL (255) */
-			if (setup_sg_tablesize > SG_ALL)
-				setup_sg_tablesize = SG_ALL;
+			if (setup_sg_tablesize > ATARI_TT_SG_TABLESIZE)
+				setup_sg_tablesize = ATARI_TT_SG_TABLESIZE;
 		}
 	}
 	if (ints[0] >= 4) {
diff --git a/drivers/scsi/atari_scsi.h b/drivers/scsi/atari_scsi.h
index efadb8d..21b8c91 100644
--- a/drivers/scsi/atari_scsi.h
+++ b/drivers/scsi/atari_scsi.h
@@ -35,11 +35,11 @@ int atari_scsi_release (struct Scsi_Host *);
 
 #define ATARI_TT_CAN_QUEUE		16
 #define ATARI_TT_CMD_PER_LUN		8
-#define ATARI_TT_SG_TABLESIZE		SG_ALL
+#define ATARI_TT_SG_TABLESIZE		255
 
 #define ATARI_FALCON_CAN_QUEUE		8
 #define ATARI_FALCON_CMD_PER_LUN	1
-#define ATARI_FALCON_SG_TABLESIZE	SG_NONE
+#define ATARI_FALCON_SG_TABLESIZE	1
 
 #define	DEFAULT_USE_TAGGED_QUEUING	0
 
diff --git a/drivers/scsi/eata_pio.c b/drivers/scsi/eata_pio.c
index b5a6092..8913d9d 100644
--- a/drivers/scsi/eata_pio.c
+++ b/drivers/scsi/eata_pio.c
@@ -691,6 +691,9 @@ static unsigned int print_selftest(unsigned int base)
 	return (!(inb(base + HA_RSTATUS) & HA_SERROR));
 }
 
+/* FIXME: is this driver sg-chainning ready? */
+#define EATA_MAX_SG 255
+
 static int register_pio_HBA(long base, struct get_conf *gc, struct pci_dev *pdev)
 {
 	unsigned long size = 0;
@@ -801,7 +804,7 @@ static int register_pio_HBA(long base, struct get_conf *gc, struct pci_dev *pdev
 	sh->this_id = gc->scsi_id[3];
 	sh->can_queue = 1;
 	sh->cmd_per_lun = 1;
-	sh->sg_tablesize = SG_ALL;
+	sh->sg_tablesize = EATA_MAX_SG;
 
 	hd->channel = 0;
 
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.c b/drivers/scsi/ibmvscsi/ibmvscsi.c
index 78d46a9..9b1fc94 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.c
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.c
@@ -173,7 +173,8 @@ static void release_event_pool(struct event_pool *pool,
 			++in_use;
 		if (pool->events[i].ext_list) {
 			dma_free_coherent(hostdata->dev,
-				  SG_ALL * sizeof(struct srp_direct_buf),
+				  IBMVSCSI_MAX_SG *
+						sizeof(struct srp_direct_buf),
 				  pool->events[i].ext_list,
 				  pool->events[i].ext_list_token);
 		}
@@ -423,7 +424,8 @@ static int map_sg_data(struct scsi_cmnd *cmd,
 	if (!evt_struct->ext_list) {
 		evt_struct->ext_list = (struct srp_direct_buf *)
 			dma_alloc_coherent(dev,
-					   SG_ALL * sizeof(struct srp_direct_buf),
+					   IBMVSCSI_MAX_SG *
+						sizeof(struct srp_direct_buf),
 					   &evt_struct->ext_list_token, 0);
 		if (!evt_struct->ext_list) {
 			sdev_printk(KERN_ERR, cmd->device,
@@ -1598,7 +1600,7 @@ static struct scsi_host_template driver_template = {
 	.cmd_per_lun = 16,
 	.can_queue = IBMVSCSI_MAX_REQUESTS_DEFAULT,
 	.this_id = -1,
-	.sg_tablesize = SG_ALL,
+	.sg_tablesize = IBMVSCSI_MAX_SG,
 	.use_clustering = ENABLE_CLUSTERING,
 	.shost_attrs = ibmvscsi_attrs,
 };
diff --git a/drivers/scsi/ibmvscsi/ibmvscsi.h b/drivers/scsi/ibmvscsi/ibmvscsi.h
index 46e850e..e3e3a2e 100644
--- a/drivers/scsi/ibmvscsi/ibmvscsi.h
+++ b/drivers/scsi/ibmvscsi/ibmvscsi.h
@@ -46,6 +46,7 @@ struct Scsi_Host;
 
 #define IBMVSCSI_MAX_REQUESTS_DEFAULT 100
 #define IBMVSCSI_MAX_CMDS_PER_LUN 64
+#define IBMVSCSI_MAX_SG 255
 
 /* ------------------------------------------------------------
  * Data Structures
diff --git a/drivers/scsi/mac53c94.c b/drivers/scsi/mac53c94.c
index b12ad7c..9ef0272 100644
--- a/drivers/scsi/mac53c94.c
+++ b/drivers/scsi/mac53c94.c
@@ -392,6 +392,9 @@ static void set_dma_cmds(struct fsc_state *state, struct scsi_cmnd *cmd)
 	cmd->SCp.this_residual = total;
 }
 
+/* Dynamicaly allocated at init */
+#define MAC53c94_MAX_SG 255
+
 static struct scsi_host_template mac53c94_template = {
 	.proc_name	= "53c94",
 	.name		= "53C94",
@@ -399,7 +402,7 @@ static struct scsi_host_template mac53c94_template = {
 	.eh_host_reset_handler = mac53c94_host_reset,
 	.can_queue	= 1,
 	.this_id	= 7,
-	.sg_tablesize	= SG_ALL,
+	.sg_tablesize	= MAC53c94_MAX_SG,
 	.cmd_per_lun	= 1,
 	.use_clustering	= DISABLE_CLUSTERING,
 };
@@ -464,7 +467,8 @@ static int mac53c94_probe(struct macio_dev *mdev, const struct of_device_id *mat
 	 * XXX FIXME: Use DMA consistent routines
 	 */
        	dma_cmd_space = kmalloc((host->sg_tablesize + 2) *
-       				sizeof(struct dbdma_cmd), GFP_KERNEL);
+				sizeof(struct dbdma_cmd),
+				GFP_KERNEL | GFP_DMA);
        	if (dma_cmd_space == 0) {
        		printk(KERN_ERR "mac53c94: couldn't allocate dma "
        		       "command space for %s\n", node->full_name);
diff --git a/drivers/scsi/mesh.c b/drivers/scsi/mesh.c
index 651d09b..2a8654a 100644
--- a/drivers/scsi/mesh.c
+++ b/drivers/scsi/mesh.c
@@ -1832,6 +1832,9 @@ static int mesh_shutdown(struct macio_dev *mdev)
 	return 0;
 }
 
+/* allocated at probe time */
+#define MESH_MAX_SG 255
+
 static struct scsi_host_template mesh_template = {
 	.proc_name			= "mesh",
 	.name				= "MESH",
@@ -1840,7 +1843,7 @@ static struct scsi_host_template mesh_template = {
 	.eh_host_reset_handler		= mesh_host_reset,
 	.can_queue			= 20,
 	.this_id			= 7,
-	.sg_tablesize			= SG_ALL,
+	.sg_tablesize			= MESH_MAX_SG,
 	.cmd_per_lun			= 2,
 	.use_clustering			= DISABLE_CLUSTERING,
 };
diff --git a/drivers/scsi/nsp32.h b/drivers/scsi/nsp32.h
index 6715ecb..ace52f5 100644
--- a/drivers/scsi/nsp32.h
+++ b/drivers/scsi/nsp32.h
@@ -444,7 +444,7 @@ typedef u16 u16_le;
 /************************************************************************
  * structure for DMA/Scatter Gather list
  */
-#define NSP32_SG_SIZE		SG_ALL
+#define NSP32_SG_SIZE		255
 
 typedef struct _nsp32_sgtable {
 	/* values must be little endian */
diff --git a/drivers/scsi/qla1280.c b/drivers/scsi/qla1280.c
index 68c0d09..c0319da 100644
--- a/drivers/scsi/qla1280.c
+++ b/drivers/scsi/qla1280.c
@@ -422,6 +422,7 @@
 #define IS_ISP1x160(ha)        (ha->pdev->device == PCI_DEVICE_ID_QLOGIC_ISP10160 || \
 				ha->pdev->device == PCI_DEVICE_ID_QLOGIC_ISP12160)
 
+#define QLA_MAX_SG 255
 
 static int qla1280_probe_one(struct pci_dev *, const struct pci_device_id *);
 static void qla1280_remove_one(struct pci_dev *);
@@ -4201,7 +4202,7 @@ static struct scsi_host_template qla1280_driver_template = {
 	.bios_param		= qla1280_biosparam,
 	.can_queue		= 0xfffff,
 	.this_id		= -1,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= QLA_MAX_SG,
 	.cmd_per_lun		= 1,
 	.use_clustering		= ENABLE_CLUSTERING,
 };
-- 
1.5.3.3

[PATCH 3/8] NCR5380: Not sg-chain ready

[Boaz Harrosh]

  NCR5380 core is not sg-chain ready, so maximum scatterlist supported
  is SCSI_MAX_SG_SEGMENTS. Define it so, in core, and let all drivers
  use it for the sg_tablesize in host templates.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/NCR5380.h       |    3 +++
 drivers/scsi/arm/cumana_1.c  |    2 +-
 drivers/scsi/arm/oak.c       |    2 +-
 drivers/scsi/dmx3191d.c      |    2 +-
 drivers/scsi/dtc.c           |    2 +-
 drivers/scsi/g_NCR5380.c     |    2 +-
 drivers/scsi/mac_scsi.c      |    9 ++++-----
 drivers/scsi/mac_scsi.h      |    4 ----
 drivers/scsi/sun3_scsi.c     |    4 ++--
 drivers/scsi/sun3_scsi.h     |    4 ----
 drivers/scsi/sun3_scsi_vme.c |    4 ++--
 11 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
index bdc468c..cd7e258 100644
--- a/drivers/scsi/NCR5380.h
+++ b/drivers/scsi/NCR5380.h
@@ -37,6 +37,9 @@
 #define NCR5380_PUBLIC_RELEASE 7
 #define NCR53C400_PUBLIC_RELEASE 2
 
+/* FIXME: Not sg-chain ready */
+#define NCR5380_MAX_SG SCSI_MAX_SG_SEGMENTS
+
 #define NDEBUG_ARBITRATION	0x1
 #define NDEBUG_AUTOSENSE	0x2
 #define NDEBUG_DMA		0x4
diff --git a/drivers/scsi/arm/cumana_1.c b/drivers/scsi/arm/cumana_1.c
index 49d838e..6315730 100644
--- a/drivers/scsi/arm/cumana_1.c
+++ b/drivers/scsi/arm/cumana_1.c
@@ -220,7 +220,7 @@ static struct scsi_host_template cumanascsi_template = {
 	.eh_bus_reset_handler	= NCR5380_bus_reset,
 	.can_queue		= 16,
 	.this_id		= 7,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= NCR5380_MAX_SG,
 	.cmd_per_lun		= 2,
 	.unchecked_isa_dma	= 0,
 	.use_clustering		= DISABLE_CLUSTERING,
diff --git a/drivers/scsi/arm/oak.c b/drivers/scsi/arm/oak.c
index 849cdf8..69d9afd 100644
--- a/drivers/scsi/arm/oak.c
+++ b/drivers/scsi/arm/oak.c
@@ -123,7 +123,7 @@ static struct scsi_host_template oakscsi_template = {
 	.eh_bus_reset_handler	= NCR5380_bus_reset,
 	.can_queue		= 16,
 	.this_id		= 7,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= NCR5380_MAX_SG,
 	.cmd_per_lun		= 2,
 	.use_clustering		= DISABLE_CLUSTERING,
 	.proc_name		= "oakscsi",
diff --git a/drivers/scsi/dmx3191d.c b/drivers/scsi/dmx3191d.c
index fa738ec..92e89b8 100644
--- a/drivers/scsi/dmx3191d.c
+++ b/drivers/scsi/dmx3191d.c
@@ -63,7 +63,7 @@ static struct scsi_host_template dmx3191d_driver_template = {
 	.eh_bus_reset_handler	= NCR5380_bus_reset,
 	.can_queue		= 32,
 	.this_id		= 7,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= NCR5380_MAX_SG,
 	.cmd_per_lun		= 2,
 	.use_clustering		= DISABLE_CLUSTERING,
 };
diff --git a/drivers/scsi/dtc.c b/drivers/scsi/dtc.c
index c2677ba..f8e5dd9 100644
--- a/drivers/scsi/dtc.c
+++ b/drivers/scsi/dtc.c
@@ -479,7 +479,7 @@ static struct scsi_host_template driver_template = {
 	.bios_param     		= dtc_biosparam,
 	.can_queue      		= CAN_QUEUE,
 	.this_id        		= 7,
-	.sg_tablesize   		= SG_ALL,
+	.sg_tablesize   		= NCR5380_MAX_SG,
 	.cmd_per_lun    		= CMD_PER_LUN,
 	.use_clustering 		= DISABLE_CLUSTERING,
 };
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 75585a5..aded3be 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -922,7 +922,7 @@ static struct scsi_host_template driver_template = {
 	.bios_param     	= NCR5380_BIOSPARAM,
 	.can_queue      	= CAN_QUEUE,
         .this_id        	= 7,
-        .sg_tablesize   	= SG_ALL,
+        .sg_tablesize   	= NCR5380_MAX_SG,
 	.cmd_per_lun    	= CMD_PER_LUN,
         .use_clustering		= DISABLE_CLUSTERING,
 };
diff --git a/drivers/scsi/mac_scsi.c b/drivers/scsi/mac_scsi.c
index 3b09ab2..d8a27c0 100644
--- a/drivers/scsi/mac_scsi.c
+++ b/drivers/scsi/mac_scsi.c
@@ -180,9 +180,8 @@ static int __init mac_scsi_setup(char *str) {
 	if (ints[0] >= 3) {
 		if (ints[3] >= 0) {
 			setup_sg_tablesize = ints[3];
-			/* Must be <= SG_ALL (255) */
-			if (setup_sg_tablesize > SG_ALL)
-				setup_sg_tablesize = SG_ALL;
+			if (setup_sg_tablesize > NCR5380_MAX_SG)
+				setup_sg_tablesize = NCR5380_MAX_SG;
 		}
 	}
 	if (ints[0] >= 4) {
@@ -252,7 +251,7 @@ int macscsi_detect(struct scsi_host_template * tpnt)
     tpnt->cmd_per_lun =
 	(setup_cmd_per_lun > 0) ? setup_cmd_per_lun : CMD_PER_LUN;
     tpnt->sg_tablesize = 
-	(setup_sg_tablesize >= 0) ? setup_sg_tablesize : SG_TABLESIZE;
+	(setup_sg_tablesize > 0) ? setup_sg_tablesize : 1;
 
     if (setup_hostid >= 0)
 	tpnt->this_id = setup_hostid;
@@ -590,7 +589,7 @@ static struct scsi_host_template driver_template = {
 	.eh_bus_reset_handler		= macscsi_bus_reset,
 	.can_queue			= CAN_QUEUE,
 	.this_id			= 7,
-	.sg_tablesize			= SG_ALL,
+	.sg_tablesize			= NCR5380_MAX_SG,
 	.cmd_per_lun			= CMD_PER_LUN,
 	.unchecked_isa_dma		= 0,
 	.use_clustering			= DISABLE_CLUSTERING
diff --git a/drivers/scsi/mac_scsi.h b/drivers/scsi/mac_scsi.h
index d26e331..3bc9919 100644
--- a/drivers/scsi/mac_scsi.h
+++ b/drivers/scsi/mac_scsi.h
@@ -41,10 +41,6 @@
 #define CAN_QUEUE 16
 #endif
 
-#ifndef SG_TABLESIZE
-#define SG_TABLESIZE SG_NONE
-#endif
-
 #ifndef USE_TAGGED_QUEUING
 #define	USE_TAGGED_QUEUING 0
 #endif
diff --git a/drivers/scsi/sun3_scsi.c b/drivers/scsi/sun3_scsi.c
index e606cf0..ff06208 100644
--- a/drivers/scsi/sun3_scsi.c
+++ b/drivers/scsi/sun3_scsi.c
@@ -222,7 +222,7 @@ int sun3scsi_detect(struct scsi_host_template * tpnt)
 	tpnt->cmd_per_lun =
 		(setup_cmd_per_lun > 0) ? setup_cmd_per_lun : CMD_PER_LUN;
 	tpnt->sg_tablesize = 
-		(setup_sg_tablesize >= 0) ? setup_sg_tablesize : SG_TABLESIZE;
+		(setup_sg_tablesize > 0) ? setup_sg_tablesize : 1;
 
 	if (setup_hostid >= 0)
 		tpnt->this_id = setup_hostid;
@@ -631,7 +631,7 @@ static struct scsi_host_template driver_template = {
 	.eh_bus_reset_handler  	= sun3scsi_bus_reset,
 	.can_queue		= CAN_QUEUE,
 	.this_id		= 7,
-	.sg_tablesize		= SG_TABLESIZE,
+	.sg_tablesize		= NCR5380_MAX_SG,
 	.cmd_per_lun		= CMD_PER_LUN,
 	.use_clustering		= DISABLE_CLUSTERING
 };
diff --git a/drivers/scsi/sun3_scsi.h b/drivers/scsi/sun3_scsi.h
index b29a9d6..b44e7b4 100644
--- a/drivers/scsi/sun3_scsi.h
+++ b/drivers/scsi/sun3_scsi.h
@@ -63,10 +63,6 @@ static int sun3scsi_release (struct Scsi_Host *);
 #define CAN_QUEUE 16
 #endif
 
-#ifndef SG_TABLESIZE
-#define SG_TABLESIZE SG_NONE
-#endif
-
 #ifndef MAX_TAGS
 #define MAX_TAGS 32
 #endif
diff --git a/drivers/scsi/sun3_scsi_vme.c b/drivers/scsi/sun3_scsi_vme.c
index 02d9727..e9c3632 100644
--- a/drivers/scsi/sun3_scsi_vme.c
+++ b/drivers/scsi/sun3_scsi_vme.c
@@ -170,7 +170,7 @@ static int sun3scsi_detect(struct scsi_host_template * tpnt)
 	tpnt->cmd_per_lun =
 		(setup_cmd_per_lun > 0) ? setup_cmd_per_lun : CMD_PER_LUN;
 	tpnt->sg_tablesize = 
-		(setup_sg_tablesize >= 0) ? setup_sg_tablesize : SG_TABLESIZE;
+		(setup_sg_tablesize > 0) ? setup_sg_tablesize : 1;
 	
 	if (setup_hostid >= 0)
 		tpnt->this_id = setup_hostid;
@@ -574,7 +574,7 @@ static struct scsi_host_template driver_template = {
 	.eh_bus_reset_handler  	= sun3scsi_bus_reset,
 	.can_queue		= CAN_QUEUE,
 	.this_id		= 7,
-	.sg_tablesize		= SG_TABLESIZE,
+	.sg_tablesize		= NCR5380_MAX_SG,
 	.cmd_per_lun		= CMD_PER_LUN,
 	.use_clustering		= DISABLE_CLUSTERING
 };
-- 
1.5.3.3

[PATCH 4/8] wd33c93: Not sg-chain ready

[Boaz Harrosh]

  wd33c93 core is not sg-chain ready, so maximum scatterlist supported
  is SCSI_MAX_SG_SEGMENTS. Define it so, in core, and let all drivers
  use it for the sg_tablesize in host templates.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/a2091.c   |    2 +-
 drivers/scsi/a3000.c   |    2 +-
 drivers/scsi/gvp11.c   |    2 +-
 drivers/scsi/mvme147.c |    2 +-
 drivers/scsi/sgiwd93.c |    2 +-
 drivers/scsi/wd33c93.h |    2 ++
 6 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/a2091.c b/drivers/scsi/a2091.c
index 23f27c9..a5a511f 100644
--- a/drivers/scsi/a2091.c
+++ b/drivers/scsi/a2091.c
@@ -217,7 +217,7 @@ static struct scsi_host_template driver_template = {
 	.eh_host_reset_handler	= wd33c93_host_reset,
 	.can_queue		= CAN_QUEUE,
 	.this_id		= 7,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= WD_MAX_SG,
 	.cmd_per_lun		= CMD_PER_LUN,
 	.use_clustering		= DISABLE_CLUSTERING
 };
diff --git a/drivers/scsi/a3000.c b/drivers/scsi/a3000.c
index d7255c8..aaadf73 100644
--- a/drivers/scsi/a3000.c
+++ b/drivers/scsi/a3000.c
@@ -222,7 +222,7 @@ static struct scsi_host_template driver_template = {
 	.eh_host_reset_handler	= wd33c93_host_reset,
 	.can_queue		= CAN_QUEUE,
 	.this_id		= 7,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= WD_MAX_SG,
 	.cmd_per_lun		= CMD_PER_LUN,
 	.use_clustering		= ENABLE_CLUSTERING
 };
diff --git a/drivers/scsi/gvp11.c b/drivers/scsi/gvp11.c
index 37741e9..e56c257 100644
--- a/drivers/scsi/gvp11.c
+++ b/drivers/scsi/gvp11.c
@@ -371,7 +371,7 @@ static struct scsi_host_template driver_template = {
 	.eh_host_reset_handler	= wd33c93_host_reset,
 	.can_queue		= CAN_QUEUE,
 	.this_id		= 7,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= WD_MAX_SG,
 	.cmd_per_lun		= CMD_PER_LUN,
 	.use_clustering		= DISABLE_CLUSTERING
 };
diff --git a/drivers/scsi/mvme147.c b/drivers/scsi/mvme147.c
index be41aad..e69c108 100644
--- a/drivers/scsi/mvme147.c
+++ b/drivers/scsi/mvme147.c
@@ -140,7 +140,7 @@ static struct scsi_host_template driver_template = {
 	.eh_host_reset_handler	= wd33c93_host_reset,
 	.can_queue		= CAN_QUEUE,
 	.this_id		= 7,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= WD_MAX_SG,
 	.cmd_per_lun		= CMD_PER_LUN,
 	.use_clustering		= ENABLE_CLUSTERING
 };
diff --git a/drivers/scsi/sgiwd93.c b/drivers/scsi/sgiwd93.c
index d4ebe8c..60cc3d1 100644
--- a/drivers/scsi/sgiwd93.c
+++ b/drivers/scsi/sgiwd93.c
@@ -208,7 +208,7 @@ static struct scsi_host_template sgiwd93_template = {
 	.eh_host_reset_handler	= wd33c93_host_reset,
 	.can_queue		= 16,
 	.this_id		= 7,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= WD_MAX_SG,
 	.cmd_per_lun		= 8,
 	.use_clustering		= DISABLE_CLUSTERING,
 };
diff --git a/drivers/scsi/wd33c93.h b/drivers/scsi/wd33c93.h
index 00123f2..e54d7f4 100644
--- a/drivers/scsi/wd33c93.h
+++ b/drivers/scsi/wd33c93.h
@@ -339,6 +339,8 @@ struct WD33C93_hostdata {
 #define PR_TEST      1<<6
 #define PR_STOP      1<<7
 
+/* FIXME: Not sg-chain ready */
+#define WD_MAX_SG SCSI_MAX_SG_SEGMENTS
 
 void wd33c93_init (struct Scsi_Host *instance, const wd33c93_regs regs,
          dma_setup_t setup, dma_stop_t stop, int clock_freq);
-- 
1.5.3.3

[PATCH 5/8] arm/scsi: Not sg-chain ready

[Boaz Harrosh]

  arm-scsi core is not sg-chain ready, so maximum scatterlist supported
  is SCSI_MAX_SG_SEGMENTS. Define it so, in core, and let all drivers
  use it for the sg_tablesize in host templates.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/arm/acornscsi.c |    2 +-
 drivers/scsi/arm/arxescsi.c  |    2 +-
 drivers/scsi/arm/cumana_2.c  |    2 +-
 drivers/scsi/arm/eesox.c     |    2 +-
 drivers/scsi/arm/powertec.c  |    2 +-
 drivers/scsi/arm/scsi.h      |    2 ++
 6 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/arm/acornscsi.c b/drivers/scsi/arm/acornscsi.c
index 3bedf24..58a88a8 100644
--- a/drivers/scsi/arm/acornscsi.c
+++ b/drivers/scsi/arm/acornscsi.c
@@ -2981,7 +2981,7 @@ static struct scsi_host_template acornscsi_template = {
 	.reset			= acornscsi_reset,
 	.can_queue		= 16,
 	.this_id		= 7,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= ARM_SCSI_MAX_SG,
 	.cmd_per_lun		= 2,
 	.unchecked_isa_dma	= 0,
 	.use_clustering		= DISABLE_CLUSTERING,
diff --git a/drivers/scsi/arm/arxescsi.c b/drivers/scsi/arm/arxescsi.c
index 2836fe2..a35e7c1 100644
--- a/drivers/scsi/arm/arxescsi.c
+++ b/drivers/scsi/arm/arxescsi.c
@@ -270,7 +270,7 @@ static struct scsi_host_template arxescsi_template = {
 	.eh_abort_handler		= fas216_eh_abort,
 	.can_queue			= 0,
 	.this_id			= 7,
-	.sg_tablesize			= SG_ALL,
+	.sg_tablesize			= ARM_SCSI_MAX_SG,
 	.cmd_per_lun			= 1,
 	.use_clustering			= DISABLE_CLUSTERING,
 	.proc_name			= "arxescsi",
diff --git a/drivers/scsi/arm/cumana_2.c b/drivers/scsi/arm/cumana_2.c
index 68a6412..3fe690c 100644
--- a/drivers/scsi/arm/cumana_2.c
+++ b/drivers/scsi/arm/cumana_2.c
@@ -390,7 +390,7 @@ static struct scsi_host_template cumanascsi2_template = {
 	.eh_abort_handler		= fas216_eh_abort,
 	.can_queue			= 1,
 	.this_id			= 7,
-	.sg_tablesize			= SG_ALL,
+	.sg_tablesize			= ARM_SCSI_MAX_SG,
 	.cmd_per_lun			= 1,
 	.use_clustering			= DISABLE_CLUSTERING,
 	.proc_name			= "cumanascsi2",
diff --git a/drivers/scsi/arm/eesox.c b/drivers/scsi/arm/eesox.c
index bb2477b..e0a1cf6 100644
--- a/drivers/scsi/arm/eesox.c
+++ b/drivers/scsi/arm/eesox.c
@@ -508,7 +508,7 @@ static struct scsi_host_template eesox_template = {
 	.eh_abort_handler		= fas216_eh_abort,
 	.can_queue			= 1,
 	.this_id			= 7,
-	.sg_tablesize			= SG_ALL,
+	.sg_tablesize			= ARM_SCSI_MAX_SG,
 	.cmd_per_lun			= 1,
 	.use_clustering			= DISABLE_CLUSTERING,
 	.proc_name			= "eesox",
diff --git a/drivers/scsi/arm/powertec.c b/drivers/scsi/arm/powertec.c
index d9a546d..ca27047 100644
--- a/drivers/scsi/arm/powertec.c
+++ b/drivers/scsi/arm/powertec.c
@@ -302,7 +302,7 @@ static struct scsi_host_template powertecscsi_template = {
 
 	.can_queue			= 8,
 	.this_id			= 7,
-	.sg_tablesize			= SG_ALL,
+	.sg_tablesize			= ARM_SCSI_MAX_SG,
 	.cmd_per_lun			= 2,
 	.use_clustering			= ENABLE_CLUSTERING,
 	.proc_name			= "powertec",
diff --git a/drivers/scsi/arm/scsi.h b/drivers/scsi/arm/scsi.h
index c7415fc..dffd073 100644
--- a/drivers/scsi/arm/scsi.h
+++ b/drivers/scsi/arm/scsi.h
@@ -14,6 +14,8 @@
 
 #define BELT_AND_BRACES
 
+/* FIXME: all drivers that depend on this are not chain-safe */
+#define ARM_SCSI_MAX_SG SCSI_MAX_SG_SEGMENTS
 /*
  * The scatter-gather list handling.  This contains all
  * the yucky stuff that needs to be fixed properly.
-- 
1.5.3.3

scsi: Drivers not ready for sg-chaining

[Boaz Harrosh]

  All below drivers are not sg-chain ready do to incomplete software.
  Once fixed they can move back to SG_ALL. For now they are stuck on
  SCSI_MAX_SG_SEGMENTS.

  Affected drivers/files:
	drivers/scsi/aha152x.c
	drivers/scsi/esp_scsi.[ch]
	drivers/scsi/imm.c
	drivers/scsi/in2000.h
	drivers/scsi/pcmcia/nsp_cs.c
	drivers/scsi/ppa.c
	drivers/scsi/tmscsim.c

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/aha152x.c       |    5 ++++-
 drivers/scsi/esp_scsi.c      |    2 +-
 drivers/scsi/esp_scsi.h      |    3 +++
 drivers/scsi/imm.c           |    5 ++++-
 drivers/scsi/in2000.h        |    2 +-
 drivers/scsi/pcmcia/nsp_cs.c |    5 ++++-
 drivers/scsi/ppa.c           |    4 +++-
 drivers/scsi/tmscsim.c       |    5 ++++-
 8 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/aha152x.c b/drivers/scsi/aha152x.c
index 6ccdc96..6be6008 100644
--- a/drivers/scsi/aha152x.c
+++ b/drivers/scsi/aha152x.c
@@ -3460,6 +3460,9 @@ static int aha152x_adjust_queue(struct scsi_device *device)
 	return 0;
 }
 
+/* FIXME: Not sg-chain ready */
+#define AHA152X_MAX_SG SCSI_MAX_SG_SEGMENTS
+
 static struct scsi_host_template aha152x_driver_template = {
 	.module				= THIS_MODULE,
 	.name				= AHA152X_REVID,
@@ -3473,7 +3476,7 @@ static struct scsi_host_template aha152x_driver_template = {
 	.bios_param			= aha152x_biosparam,
 	.can_queue			= 1,
 	.this_id			= 7,
-	.sg_tablesize			= SG_ALL,
+	.sg_tablesize			= AHA152X_MAX_SG,
 	.cmd_per_lun			= 1,
 	.use_clustering			= DISABLE_CLUSTERING,
 	.slave_alloc			= aha152x_adjust_queue,
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
index bfdee59..83165f2 100644
--- a/drivers/scsi/esp_scsi.c
+++ b/drivers/scsi/esp_scsi.c
@@ -2603,7 +2603,7 @@ struct scsi_host_template scsi_esp_template = {
 	.eh_host_reset_handler	= esp_eh_host_reset_handler,
 	.can_queue		= 7,
 	.this_id		= 7,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= ESP_MAX_SG,
 	.use_clustering		= ENABLE_CLUSTERING,
 	.max_sectors		= 0xffff,
 	.skip_settle_delay	= 1,
diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
index d5576d5..0f86a90 100644
--- a/drivers/scsi/esp_scsi.h
+++ b/drivers/scsi/esp_scsi.h
@@ -289,6 +289,9 @@ struct esp_cmd_entry {
 #define ESP_MAX_LUN		8
 #define ESP_MAX_TAG		256
 
+/* FIXME: not sg-chain ready */
+#define ESP_MAX_SG SCSI_MAX_SG_SEGMENTS
+
 struct esp_lun_data {
 	struct esp_cmd_entry	*non_tagged_cmd;
 	int			num_tagged;
diff --git a/drivers/scsi/imm.c b/drivers/scsi/imm.c
index f97d172..6b81692 100644
--- a/drivers/scsi/imm.c
+++ b/drivers/scsi/imm.c
@@ -1112,6 +1112,9 @@ static int imm_adjust_queue(struct scsi_device *device)
 	return 0;
 }
 
+/* FIXME: not sg-chain ready */
+#define IMM_MAX_SG SCSI_MAX_SG_SEGMENTS
+
 static struct scsi_host_template imm_template = {
 	.module			= THIS_MODULE,
 	.proc_name		= "imm",
@@ -1123,7 +1126,7 @@ static struct scsi_host_template imm_template = {
 	.eh_host_reset_handler	= imm_reset,
 	.bios_param		= imm_biosparam,
 	.this_id		= 7,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= IMM_MAX_SG,
 	.cmd_per_lun		= 1,
 	.use_clustering		= ENABLE_CLUSTERING,
 	.can_queue		= 1,
diff --git a/drivers/scsi/in2000.h b/drivers/scsi/in2000.h
index 0fb8b06..388e16f 100644
--- a/drivers/scsi/in2000.h
+++ b/drivers/scsi/in2000.h
@@ -405,7 +405,7 @@ static int in2000_bus_reset(Scsi_Cmnd *);
 
 
 #define IN2000_CAN_Q    16
-#define IN2000_SG       SG_ALL
+#define IN2000_SG       SCSI_MAX_SG_SEGMENTS /* FIXME: not sg-chain ready */
 #define IN2000_CPL      2
 #define IN2000_HOST_ID  7
 
diff --git a/drivers/scsi/pcmcia/nsp_cs.c b/drivers/scsi/pcmcia/nsp_cs.c
index 5082ca3..a8f6d24 100644
--- a/drivers/scsi/pcmcia/nsp_cs.c
+++ b/drivers/scsi/pcmcia/nsp_cs.c
@@ -77,6 +77,9 @@ static int       free_ports = 0;
 module_param(free_ports, bool, 0);
 MODULE_PARM_DESC(free_ports, "Release IO ports after configuration? (default: 0 (=no))");
 
+/* FIXME: not sg-chain ready */
+#define NSP_MAX_SG SCSI_MAX_SG_SEGMENTS
+
 static struct scsi_host_template nsp_driver_template = {
 	.proc_name	         = "nsp_cs",
 	.proc_info		 = nsp_proc_info,
@@ -88,7 +91,7 @@ static struct scsi_host_template nsp_driver_template = {
 	.eh_host_reset_handler	 = nsp_eh_host_reset,
 	.can_queue		 = 1,
 	.this_id		 = NSP_INITIATOR_ID,
-	.sg_tablesize		 = SG_ALL,
+	.sg_tablesize		 = NSP_MAX_SG,
 	.cmd_per_lun		 = 1,
 	.use_clustering		 = DISABLE_CLUSTERING,
 };
diff --git a/drivers/scsi/ppa.c b/drivers/scsi/ppa.c
index f655ae3..a47309d 100644
--- a/drivers/scsi/ppa.c
+++ b/drivers/scsi/ppa.c
@@ -23,6 +23,8 @@
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_host.h>
 
+/* FIXME: Not chainning safe */
+#define PPA_MAX_SG SCSI_MAX_SG_SEGMENTS
 
 static void ppa_reset_pulse(unsigned int base);
 
@@ -986,7 +988,7 @@ static struct scsi_host_template ppa_template = {
 	.eh_host_reset_handler	= ppa_reset,
 	.bios_param		= ppa_biosparam,
 	.this_id		= -1,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= PPA_MAX_SG,
 	.cmd_per_lun		= 1,
 	.use_clustering		= ENABLE_CLUSTERING,
 	.can_queue		= 1,
diff --git a/drivers/scsi/tmscsim.c b/drivers/scsi/tmscsim.c
index 5b04ddf..6a7824a 100644
--- a/drivers/scsi/tmscsim.c
+++ b/drivers/scsi/tmscsim.c
@@ -2194,6 +2194,9 @@ static int dc390_slave_configure(struct scsi_device *sdev)
 	return 0;
 }
 
+/* FIXME: not sg-chain ready */
+#define DC390_MAX_SG SCSI_MAX_SG_SEGMENTS
+
 static struct scsi_host_template driver_template = {
 	.module			= THIS_MODULE,
 	.proc_name		= "tmscsim", 
@@ -2206,7 +2209,7 @@ static struct scsi_host_template driver_template = {
 	.eh_bus_reset_handler	= DC390_bus_reset,
 	.can_queue		= 1,
 	.this_id		= 7,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= DC390_MAX_SG,
 	.cmd_per_lun		= 1,
 	.use_clustering		= ENABLE_CLUSTERING,
 	.max_sectors		= 0x4000, /* 8MiB = 16 * 1024 * 512 */
-- 
1.5.3.3

Re: scsi: Drivers not ready for sg-chaining

[James Bottomley]

On Thu, 2008-01-17 at 18:51 +0200, Boaz Harrosh wrote:
> All below drivers are not sg-chain ready do to incomplete software.
>   Once fixed they can move back to SG_ALL. For now they are stuck on
>   SCSI_MAX_SG_SEGMENTS.
> 
>   Affected drivers/files:
> 	drivers/scsi/aha152x.c

This seems to process an element at a time and should be fixed by
sg_next()

> 	drivers/scsi/esp_scsi.[ch]

As does this.

> 	drivers/scsi/imm.c

And this.

> 	drivers/scsi/in2000.h

And this (.c not .h)

> 	drivers/scsi/pcmcia/nsp_cs.c

This uses a software table, so should be fine.

> 	drivers/scsi/ppa.c

This does element at a time, so should be fixed by sg_next()

> 	drivers/scsi/tmscsim.c

as does this.

Given where we are in the cycle, it's probably best just to set SG_ALL
to 128 so there's no possibility of problems.  Individual drivers can
make the tradeoff on how they handle larger lists going beyond this.

James

---

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index d1299e9..530ff4c 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -6,6 +6,7 @@
 #include <linux/types.h>
 #include <linux/workqueue.h>
 #include <linux/mutex.h>
+#include <scsi/scsi.h>
 
 struct request_queue;
 struct block_device;
@@ -25,12 +26,15 @@ struct blk_queue_tags;
  * NONE: Self evident.	Host adapter is not capable of scatter-gather.
  * ALL:	 Means that the host adapter module can do scatter-gather,
  *	 and that there is no limit to the size of the table to which
- *	 we scatter/gather data.
+ *	 we scatter/gather data.  The value we set here is the maximum
+ *	 single element sglist.  To use chained sglists, the adapter
+ *	 has to set a value beyond ALL (and correctly use the chain
+ *	 handling API.
  * Anything else:  Indicates the maximum number of chains that can be
  *	 used in one scatter-gather request.
  */
 #define SG_NONE 0
-#define SG_ALL 0xff
+#define SG_ALL	SCSI_MAX_SG_SEGMENTS
 
 #define MODE_UNKNOWN 0x00
 #define MODE_INITIATOR 0x01


James

Re: scsi: Drivers not ready for sg-chaining

[Boaz Harrosh]

On Sun, Feb 10 2008 at 17:42 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Thu, 2008-01-17 at 18:51 +0200, Boaz Harrosh wrote:
>> All below drivers are not sg-chain ready do to incomplete software.
>>   Once fixed they can move back to SG_ALL. For now they are stuck on
>>   SCSI_MAX_SG_SEGMENTS.
>>
>>   Affected drivers/files:
>> 	drivers/scsi/aha152x.c
> 
> This seems to process an element at a time and should be fixed by
> sg_next()
> 
>> 	drivers/scsi/esp_scsi.[ch]
> 
> As does this.
> 
>> 	drivers/scsi/imm.c
> 
> And this.
> 
>> 	drivers/scsi/in2000.h
> 
> And this (.c not .h)
> 
>> 	drivers/scsi/pcmcia/nsp_cs.c
> 
> This uses a software table, so should be fine.
> 
>> 	drivers/scsi/ppa.c
> 
> This does element at a time, so should be fixed by sg_next()
> 
>> 	drivers/scsi/tmscsim.c
> 
> as does this.
> 
> Given where we are in the cycle, it's probably best just to set SG_ALL
> to 128 so there's no possibility of problems.  Individual drivers can
> make the tradeoff on how they handle larger lists going beyond this.
> 
> James
> 
> ---
> 
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index d1299e9..530ff4c 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -6,6 +6,7 @@
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
>  #include <linux/mutex.h>
> +#include <scsi/scsi.h>
>  
>  struct request_queue;
>  struct block_device;
> @@ -25,12 +26,15 @@ struct blk_queue_tags;
>   * NONE: Self evident.	Host adapter is not capable of scatter-gather.
>   * ALL:	 Means that the host adapter module can do scatter-gather,
>   *	 and that there is no limit to the size of the table to which
> - *	 we scatter/gather data.
> + *	 we scatter/gather data.  The value we set here is the maximum
> + *	 single element sglist.  To use chained sglists, the adapter
> + *	 has to set a value beyond ALL (and correctly use the chain
> + *	 handling API.
>   * Anything else:  Indicates the maximum number of chains that can be
>   *	 used in one scatter-gather request.
>   */
>  #define SG_NONE 0
> -#define SG_ALL 0xff
> +#define SG_ALL	SCSI_MAX_SG_SEGMENTS
>  
>  #define MODE_UNKNOWN 0x00
>  #define MODE_INITIATOR 0x01
> 
> 
> James
> 
> 

My patches *do not* attempt to fix the sg_chaining support. They only
make all the drivers that use SG_ALL to use SCSI_MAX_SG_SEGMENTS.
One by One, and not globally as your suggestion.

This is for two reasons.
1. So drivers can be individually fixed and in the patch that fixes them
   they can go back to SG_ALL.

2. Those drivers that have been using SG_ALL correctly and were converted
   to support sg-chaining are not penalized because of bad/old drivers

3. Some drivers in this patchset are converted to use a real internal
   driver limit. That does not necessarily match SCSI_MAX_SG_SEGMENTS.
   In the event that SCSI_MAX_SG_SEGMENTS wants to change.

The bulk of the patchset is very much mechanical and is not dangerous
and was ACKed by the more important maintainers. (That is where the
changes are more then trivial). So I don't see why they cannot get
a proper review and be accepted. Instead of doing the safe but the
wrong thing, cross tree.

Boaz

Re: scsi: Drivers not ready for sg-chaining

[James Bottomley]

On Sun, 2008-02-10 at 18:08 +0200, Boaz Harrosh wrote:
> My patches *do not* attempt to fix the sg_chaining support. They only
> make all the drivers that use SG_ALL to use SCSI_MAX_SG_SEGMENTS.
> One by One, and not globally as your suggestion.

Yes, I know ... but it does need fixing for the listed drivers.

> This is for two reasons.
> 1. So drivers can be individually fixed and in the patch that fixes them
>    they can go back to SG_ALL.

No, it's so SG_ALL can mean use chaining ... I'm not sure that's
desirable for the default value.  Particularly for devices that key
internal sglist arrays off SG_ALL

> 2. Those drivers that have been using SG_ALL correctly and were converted
>    to support sg-chaining are not penalized because of bad/old drivers

I don't see they're penalised this way either ... they just have to set
a higher value in their host template.

> 3. Some drivers in this patchset are converted to use a real internal
>    driver limit. That does not necessarily match SCSI_MAX_SG_SEGMENTS.
>    In the event that SCSI_MAX_SG_SEGMENTS wants to change.

Yes, I looked at those they're all either safe or currently (eventually)
do the right thing.

> The bulk of the patchset is very much mechanical and is not dangerous
> and was ACKed by the more important maintainers. (That is where the
> changes are more then trivial). So I don't see why they cannot get
> a proper review and be accepted. Instead of doing the safe but the
> wrong thing, cross tree.

What's wrong about this?

James

Re: scsi: Drivers not ready for sg-chaining

[Boaz Harrosh]

On Sun, Feb 10 2008 at 18:16 +0200, James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Sun, 2008-02-10 at 18:08 +0200, Boaz Harrosh wrote:
>> My patches *do not* attempt to fix the sg_chaining support. They only
>> make all the drivers that use SG_ALL to use SCSI_MAX_SG_SEGMENTS.
>> One by One, and not globally as your suggestion.
> 
> Yes, I know ... but it does need fixing for the listed drivers.

That is another patchset that its time as not yet come.
Once every thing settles I will send these too.
That's why I put the list as a reminder for me to check upon later.
 
> 
>> This is for two reasons.
>> 1. So drivers can be individually fixed and in the patch that fixes them
>>    they can go back to SG_ALL.
> 
> No, it's so SG_ALL can mean use chaining ... I'm not sure that's
> desirable for the default value.  Particularly for devices that key
> internal sglist arrays off SG_ALL
> 

I have fixed *all* these drivers that are based off SG_ALL for table sizes,
and all other reasons. exactly so the SG_ALL will continue to mean what
it means in English. (And this was triggered by your request)

>> 2. Those drivers that have been using SG_ALL correctly and were converted
>>    to support sg-chaining are not penalized because of bad/old drivers
> 
> I don't see they're penalised this way either ... they just have to set
> a higher value in their host template.
> 

It was you who wanted that to be SG_ALL. I wanted just an hard coded = ~0.

>> 3. Some drivers in this patchset are converted to use a real internal
>>    driver limit. That does not necessarily match SCSI_MAX_SG_SEGMENTS.
>>    In the event that SCSI_MAX_SG_SEGMENTS wants to change.
> 
> Yes, I looked at those they're all either safe or currently (eventually)
> do the right thing.
> 

What ??

>> The bulk of the patchset is very much mechanical and is not dangerous
>> and was ACKed by the more important maintainers. (That is where the
>> changes are more then trivial). So I don't see why they cannot get
>> a proper review and be accepted. Instead of doing the safe but the
>> wrong thing, cross tree.
> 
> What's wrong about this?
> 

I don't want to repeat myself. If it's fine with you, I trust your
final judgment. You are welcome to submit a patch that fixes all the
good drivers that are regressed by your suggestion.

> James
> 
> 

Boaz

Re: scsi: Drivers not ready for sg-chaining

[James Bottomley]

On Sun, 2008-02-10 at 18:36 +0200, Boaz Harrosh wrote:
> >> 2. Those drivers that have been using SG_ALL correctly and were converted
> >>    to support sg-chaining are not penalized because of bad/old drivers
> > 
> > I don't see they're penalised this way either ... they just have to set
> > a higher value in their host template.

> It was you who wanted that to be SG_ALL. I wanted just an hard coded = ~0.

Yes, I've changed my mind.  I think the best value for SG_ALL is the max
single table and we'll do a dynamic increase for drivers that really
have considered all the chaining implications.  That doesn't mean we
don't make the others chain ready ... we do ... we just don't force it
on them.

> I don't want to repeat myself. If it's fine with you, I trust your
> final judgment. You are welcome to submit a patch that fixes all the
> good drivers that are regressed by your suggestion.

Lowering the figure from current 255 to 128 isn't really going to cause
any regressions.  It will actually save a few unnecessary allocations
for some drivers.

James

[PATCH 7/8] a100u2w: advansys: initio: Wrong use of SG_ALL

[Boaz Harrosh]

  These drivers used the SG_ALL constant, But actually
  meant some other define.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 drivers/scsi/a100u2w.c  |    3 +--
 drivers/scsi/advansys.c |    8 +++-----
 drivers/scsi/initio.c   |    2 +-
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/scsi/a100u2w.c b/drivers/scsi/a100u2w.c
index f608d4a..82d5210 100644
--- a/drivers/scsi/a100u2w.c
+++ b/drivers/scsi/a100u2w.c
@@ -1068,7 +1068,7 @@ static struct scsi_host_template inia100_template = {
 	.eh_device_reset_handler = inia100_device_reset,
 	.can_queue		= 1,
 	.this_id		= 1,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= TOTAL_SG_ENTRY,
 	.cmd_per_lun 		= 1,
 	.use_clustering		= ENABLE_CLUSTERING,
 };
@@ -1150,7 +1150,6 @@ static int __devinit inia100_probe_one(struct pci_dev *pdev,
 	shost->max_lun = 16;
 	shost->irq = pdev->irq;
 	shost->this_id = host->scsi_id;	/* Assign HCS index */
-	shost->sg_tablesize = TOTAL_SG_ENTRY;
 
 	/* Initial orc chip           */
 	error = request_irq(pdev->irq, inia100_intr, IRQF_SHARED,
diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c
index 492702b..45d58d3 100644
--- a/drivers/scsi/advansys.c
+++ b/drivers/scsi/advansys.c
@@ -13735,13 +13735,11 @@ static int __devinit advansys_board_found(struct Scsi_Host *shost,
 	}
 
 	/*
-	 * The value of 'sg_tablesize' can not exceed the SCSI
-	 * mid-level driver definition of SG_ALL. SG_ALL also
-	 * must not be exceeded, because it is used to define the
+	 * The value of 'sg_tablesize' can not exceed the
 	 * size of the scatter-gather table in 'struct asc_sg_head'.
 	 */
-	if (shost->sg_tablesize > SG_ALL) {
-		shost->sg_tablesize = SG_ALL;
+	if (shost->sg_tablesize > ASC_MAX_SG_LIST) {
+		shost->sg_tablesize = ASC_MAX_SG_LIST;
 	}
 
 	ASC_DBG(1, "sg_tablesize: %d\n", shost->sg_tablesize);
diff --git a/drivers/scsi/initio.c b/drivers/scsi/initio.c
index 52238ee..02cb133 100644
--- a/drivers/scsi/initio.c
+++ b/drivers/scsi/initio.c
@@ -2829,7 +2829,7 @@ static struct scsi_host_template initio_template = {
 	.bios_param		= i91u_biosparam,
 	.can_queue		= MAX_TARGETS * i91u_MAXQUEUE,
 	.this_id		= 1,
-	.sg_tablesize		= SG_ALL,
+	.sg_tablesize		= TOTAL_SG_ENTRY,
 	.cmd_per_lun		= 1,
 	.use_clustering		= ENABLE_CLUSTERING,
 };
-- 
1.5.3.3

[PATCH 8/8] Change SG_ALL to mean "any size"

[Boaz Harrosh]

  Change the definition of SG_ALL from 255, which was the biggest possible
  scatter list array in 32 bit ARCHs (minus one who knows why), to be ~0.
  Which means that LLD has no size limit on number of scatter-list array
  and the actual size will be determined by upper layers.
  Also SG_NONE is removed because it is no longer supported.
  Note: SG_ALL can not be used as an allocation size anymore.

  FIXME: Important/capable drivers like iscsi have moved away from
         SG_ALL to hard coded numbers like 4096. They should reuse
         SG_ALL now.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 include/scsi/scsi_host.h |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 5c58d59..6032d07 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -21,16 +21,14 @@ struct blk_queue_tags;
 
 
 /*
- * The various choices mean:
- * NONE: Self evident.	Host adapter is not capable of scatter-gather.
+ * values to use with .sg_tablesize:
  * ALL:	 Means that the host adapter module can do scatter-gather,
  *	 and that there is no limit to the size of the table to which
  *	 we scatter/gather data.
  * Anything else:  Indicates the maximum number of chains that can be
  *	 used in one scatter-gather request.
  */
-#define SG_NONE 0
-#define SG_ALL 0xff
+#define SG_ALL (~0)
 
 #define MODE_UNKNOWN 0x00
 #define MODE_INITIATOR 0x01
-- 
1.5.3.3

Re: [patch 0/8] Change SG_ALL to mean "any size"

[Stefan Richter]

Boaz Harrosh wrote:
> Those that meant SG_ALL == 255:
> 
>   [PATCH 1/8] firewire: ieee1394: Move away from SG_ALL

They simply mean SG_ALL == something, as long as it isn't unreasonably
small or large.
-- 
Stefan Richter
-=====-==--- ---= =---=
http://arcgraph.de/sr/