[patch 0/6] marginalize HCIL a bit

[patch 0/6] marginalize HCIL a bit

[Jeff Garzik]

This patch series makes a tiny bit of progress on the marginalize-SPI
todo list.

Patches:
1) s/scsi_scan_target/spi_scan_target/
2) remove unused scsi_scan_single_target()
3) add scsi_scan_target()
4) kill all uses of spi_scan_target()
5) kill spi_scan_target(), __spi_scan_target()
6) misc cleanups

[patch 1/6] SCSI HCIL: s/scsi_scan_target/spi_scan_target/

[Jeff Garzik]

Signed-off-by: Jeff Garzik <jgarzik@pobox.com>


7c3609190a89e051779a50f1b139eeb0856d7c0b
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 327c5d7..db98b54 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1306,7 +1306,7 @@ void scsi_rescan_device(struct device *d
 }
 EXPORT_SYMBOL(scsi_rescan_device);
 
-static void __scsi_scan_target(struct device *parent, unsigned int channel,
+static void __spi_scan_target(struct device *parent, unsigned int channel,
 		unsigned int id, unsigned int lun, int rescan)
 {
 	struct Scsi_Host *shost = dev_to_shost(parent);
@@ -1357,7 +1357,7 @@ static void __scsi_scan_target(struct de
 }
 
 /**
- * scsi_scan_target - scan a target id, possibly including all LUNs on the
+ * spi_scan_target - scan a target id, possibly including all LUNs on the
  *     target.
  * @parent:	host to scan
  * @channel:	channel to scan
@@ -1372,17 +1372,17 @@ static void __scsi_scan_target(struct de
  *     First try a REPORT LUN scan, if that does not scan the target, do a
  *     sequential scan of LUNs on the target id.
  **/
-void scsi_scan_target(struct device *parent, unsigned int channel,
+void spi_scan_target(struct device *parent, unsigned int channel,
 		      unsigned int id, unsigned int lun, int rescan)
 {
 	struct Scsi_Host *shost = dev_to_shost(parent);
 
 	down(&shost->scan_mutex);
 	if (scsi_host_scan_allowed(shost))
-		__scsi_scan_target(parent, channel, id, lun, rescan);
+		__spi_scan_target(parent, channel, id, lun, rescan);
 	up(&shost->scan_mutex);
 }
-EXPORT_SYMBOL(scsi_scan_target);
+EXPORT_SYMBOL(spi_scan_target);
 
 static void scsi_scan_channel(struct Scsi_Host *shost, unsigned int channel,
 			      unsigned int id, unsigned int lun, int rescan)
@@ -1407,11 +1407,11 @@ static void scsi_scan_channel(struct Scs
 				order_id = shost->max_id - id - 1;
 			else
 				order_id = id;
-			__scsi_scan_target(&shost->shost_gendev, channel,
+			__spi_scan_target(&shost->shost_gendev, channel,
 					order_id, lun, rescan);
 		}
 	else
-		__scsi_scan_target(&shost->shost_gendev, channel,
+		__spi_scan_target(&shost->shost_gendev, channel,
 				id, lun, rescan);
 }
 
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 2cab556..91d23e9 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -1673,7 +1673,7 @@ fc_scsi_scan_rport(void *data)
 {
 	struct fc_rport *rport = (struct fc_rport *)data;
 
-	scsi_scan_target(&rport->dev, rport->channel, rport->scsi_target_id,
+	spi_scan_target(&rport->dev, rport->channel, rport->scsi_target_id,
 			SCAN_WILD_CARD, 1);
 }
 
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 1d145d2..f6c1a96 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -578,7 +578,7 @@ int sas_rphy_add(struct sas_rphy *rphy)
 	spin_unlock(&sas_host->lock);
 
 	if (rphy->scsi_target_id != -1) {
-		scsi_scan_target(&rphy->dev, parent->number,
+		spi_scan_target(&rphy->dev, parent->number,
 				rphy->scsi_target_id, ~0, 0);
 	}
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 7ece056..814a7d8 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -248,8 +248,8 @@ extern int scsi_device_quiesce(struct sc
 extern void scsi_device_resume(struct scsi_device *sdev);
 extern void scsi_target_quiesce(struct scsi_target *);
 extern void scsi_target_resume(struct scsi_target *);
-extern void scsi_scan_target(struct device *parent, unsigned int channel,
-			     unsigned int id, unsigned int lun, int rescan);
+extern void spi_scan_target(struct device *parent, unsigned int channel,
+			    unsigned int id, unsigned int lun, int rescan);
 extern void scsi_target_reap(struct scsi_target *);
 extern void scsi_target_block(struct device *);
 extern void scsi_target_unblock(struct device *);

[patch 2/6] SCSI HCIL: remove unused scsi_scan_single_target()

[Jeff Garzik]

Signed-off-by: Jeff Garzik <jgarzik@pobox.com>


9e80a2ad9451917b9e0b8717aac4b5358cc6cf3e
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index db98b54..0cbcbe7 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1452,19 +1452,6 @@ void scsi_scan_host(struct Scsi_Host *sh
 }
 EXPORT_SYMBOL(scsi_scan_host);
 
-/**
- * scsi_scan_single_target - scan the given SCSI target
- * @shost:         adapter to scan
- * @chan:          channel to scan
- * @id:            target id to scan
- **/
-void scsi_scan_single_target(struct Scsi_Host *shost, 
-	unsigned int chan, unsigned int id)
-{
-	scsi_scan_host_selected(shost, chan, id, SCAN_WILD_CARD, 1);
-}
-EXPORT_SYMBOL(scsi_scan_single_target);
-
 void scsi_forget_host(struct Scsi_Host *shost)
 {
 	struct scsi_device *sdev;
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 69313ba..26b3f70 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -634,8 +634,6 @@ extern void scsi_flush_work(struct Scsi_
 extern struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *, int);
 extern int __must_check scsi_add_host(struct Scsi_Host *, struct device *);
 extern void scsi_scan_host(struct Scsi_Host *);
-extern void scsi_scan_single_target(struct Scsi_Host *, unsigned int,
-	unsigned int);
 extern void scsi_rescan_device(struct device *);
 extern void scsi_remove_host(struct Scsi_Host *);
 extern struct Scsi_Host *scsi_host_get(struct Scsi_Host *);

Re: [patch 2/6] SCSI HCIL: remove unused scsi_scan_single_target()

[Matthew Wilcox]

On Sat, Oct 22, 2005 at 09:38:10PM -0400, Jeff Garzik wrote:
> -/**
> - * scsi_scan_single_target - scan the given SCSI target
> - * @shost:         adapter to scan
> - * @chan:          channel to scan
> - * @id:            target id to scan
> - **/
> -void scsi_scan_single_target(struct Scsi_Host *shost, 
> -	unsigned int chan, unsigned int id)
> -{
> -	scsi_scan_host_selected(shost, chan, id, SCAN_WILD_CARD, 1);
> -}
> -EXPORT_SYMBOL(scsi_scan_single_target);
> -

Adrian Bunk already tried to kill this one in February and hch NAKed it:
http://www.uwsg.iu.edu/hypermail/linux/kernel/0502.3/1115.html

Christoph, can it be killed yet?  ;-)

[patch 3/6] SCSI HCIL: add scsi_scan_target()

[Jeff Garzik]

__spi_scan_target() becomes a wrapper around __scsi_scan_target()

Signed-off-by: Jeff Garzik <jgarzik@pobox.com>


eb622527338014ebd80e10d60821d1f1559e2ac3
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 0cbcbe7..92fc94b 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1306,23 +1306,11 @@ void scsi_rescan_device(struct device *d
 }
 EXPORT_SYMBOL(scsi_rescan_device);
 
-static void __spi_scan_target(struct device *parent, unsigned int channel,
-		unsigned int id, unsigned int lun, int rescan)
+static void __scsi_scan_target(struct scsi_target *starget,
+		unsigned int lun, int rescan)
 {
-	struct Scsi_Host *shost = dev_to_shost(parent);
 	int bflags = 0;
 	int res;
-	struct scsi_target *starget;
-
-	if (shost->this_id == id)
-		/*
-		 * Don't scan the host adapter
-		 */
-		return;
-
-	starget = scsi_alloc_target(parent, channel, id);
-	if (!starget)
-		return;
 
 	get_device(&starget->dev);
 	if (lun != SCAN_WILD_CARD) {
@@ -1356,6 +1344,23 @@ static void __spi_scan_target(struct dev
 	put_device(&starget->dev);
 }
 
+static void __spi_scan_target(struct device *parent, unsigned int channel,
+		unsigned int id, unsigned int lun, int rescan)
+{
+	struct Scsi_Host *shost = dev_to_shost(parent);
+	struct scsi_target *starget;
+
+	if (shost->this_id == id)
+		/*
+		 * Don't scan the host adapter
+		 */
+		return;
+
+	starget = scsi_alloc_target(parent, channel, id);
+	if (starget)
+		__scsi_scan_target(starget, lun, rescan);
+}
+
 /**
  * spi_scan_target - scan a target id, possibly including all LUNs on the
  *     target.
@@ -1384,6 +1389,31 @@ void spi_scan_target(struct device *pare
 }
 EXPORT_SYMBOL(spi_scan_target);
 
+/**
+ * scsi_scan_target - scan a target id, possibly including all LUNs on the
+ *     target.
+ * @target:	target to scan
+ * @lun:	Specific LUN to scan or SCAN_WILD_CARD
+ * @rescan:	passed to LUN scanning routines
+ *
+ * Description:
+ *     Scan the target id on @parent, @channel, and @id. Scan at least LUN 0,
+ *     and possibly all LUNs on the target id.
+ *
+ *     First try a REPORT LUN scan, if that does not scan the target, do a
+ *     sequential scan of LUNs on the target id.
+ **/
+void scsi_scan_target(struct scsi_target *target, unsigned int lun, int rescan)
+{
+	struct Scsi_Host *shost = dev_to_shost(target->dev.parent);
+
+	down(&shost->scan_mutex);
+	if (scsi_host_scan_allowed(shost))
+		__scsi_scan_target(target, lun, rescan);
+	up(&shost->scan_mutex);
+}
+EXPORT_SYMBOL(scsi_scan_target);
+
 static void scsi_scan_channel(struct Scsi_Host *shost, unsigned int channel,
 			      unsigned int id, unsigned int lun, int rescan)
 {
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 814a7d8..c7cdb23 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -250,6 +250,8 @@ extern void scsi_target_quiesce(struct s
 extern void scsi_target_resume(struct scsi_target *);
 extern void spi_scan_target(struct device *parent, unsigned int channel,
 			    unsigned int id, unsigned int lun, int rescan);
+extern void scsi_scan_target(struct scsi_target *target,
+			     unsigned int lun, int rescan);
 extern void scsi_target_reap(struct scsi_target *);
 extern void scsi_target_block(struct device *);
 extern void scsi_target_unblock(struct device *);

Re: [patch 3/6] SCSI HCIL: add scsi_scan_target()

[Matthew Wilcox]

On Sat, Oct 22, 2005 at 09:38:49PM -0400, Jeff Garzik wrote:
> +/**
> + * scsi_scan_target - scan a target id, possibly including all LUNs on the
> + *     target.
> + * @target:	target to scan
> + * @lun:	Specific LUN to scan or SCAN_WILD_CARD
> + * @rescan:	passed to LUN scanning routines
> + *
> + * Description:
> + *     Scan the target id on @parent, @channel, and @id. Scan at least LUN 0,
> + *     and possibly all LUNs on the target id.

Need to fix @parent, @channel and @id in the kerneldoc.  I think Randy
has some automatic kerneldoc checking scripts?

Re: [patch 3/6] SCSI HCIL: add scsi_scan_target()

[Jeff Garzik]

Matthew Wilcox wrote:
> On Sat, Oct 22, 2005 at 09:38:49PM -0400, Jeff Garzik wrote:
> 
>>+/**
>>+ * scsi_scan_target - scan a target id, possibly including all LUNs on the
>>+ *     target.
>>+ * @target:	target to scan
>>+ * @lun:	Specific LUN to scan or SCAN_WILD_CARD
>>+ * @rescan:	passed to LUN scanning routines
>>+ *
>>+ * Description:
>>+ *     Scan the target id on @parent, @channel, and @id. Scan at least LUN 0,
>>+ *     and possibly all LUNs on the target id.
> 
> 
> Need to fix @parent, @channel and @id in the kerneldoc.

where?  The above looks right to me.

	Jeff

Re: [patch 3/6] SCSI HCIL: add scsi_scan_target()

[Matthew Wilcox]

On Sat, Oct 22, 2005 at 09:54:49PM -0400, Jeff Garzik wrote:
> Matthew Wilcox wrote:
> >On Sat, Oct 22, 2005 at 09:38:49PM -0400, Jeff Garzik wrote:
> >
> >>+/**
> >>+ * scsi_scan_target - scan a target id, possibly including all LUNs on 
> >>the
> >>+ *     target.
> >>+ * @target:	target to scan
> >>+ * @lun:	Specific LUN to scan or SCAN_WILD_CARD
> >>+ * @rescan:	passed to LUN scanning routines
> >>+ *
> >>+ * Description:
> >>+ *     Scan the target id on @parent, @channel, and @id. Scan at least 
> >>LUN 0,
> >>+ *     and possibly all LUNs on the target id.
> >
> >
> >Need to fix @parent, @channel and @id in the kerneldoc.
> 
> where?  The above looks right to me.

"Scan the target id on @parent, @channel, and @id"

these aren't parameters to the function.  How about just deleting that
sentence ...

"Scan at least LUN 0, and possibly all LUNs on @target"

Re: [patch 3/6] SCSI HCIL: add scsi_scan_target()

[Jeff Garzik]

Matthew Wilcox wrote:
> "Scan the target id on @parent, @channel, and @id"

Ah, in the text.  I see it now, thanks.

	Jeff

Re: [patch 3/6] SCSI HCIL: add scsi_scan_target()

[Randy.Dunlap]

On Sat, 22 Oct 2005 19:50:42 -0600 Matthew Wilcox wrote:

> On Sat, Oct 22, 2005 at 09:38:49PM -0400, Jeff Garzik wrote:
> > +/**
> > + * scsi_scan_target - scan a target id, possibly including all LUNs on the
> > + *     target.
> > + * @target:	target to scan
> > + * @lun:	Specific LUN to scan or SCAN_WILD_CARD
> > + * @rescan:	passed to LUN scanning routines
> > + *
> > + * Description:
> > + *     Scan the target id on @parent, @channel, and @id. Scan at least LUN 0,
> > + *     and possibly all LUNs on the target id.
> 
> Need to fix @parent, @channel and @id in the kerneldoc.  I think Randy
> has some automatic kerneldoc checking scripts?

Nothing fancy.  I use scripts/kernel-doc and a small starter
(or feeder) to it as copied from Documentation/kernel-doc-nano-HOWTO.txt.
I call it kdoc_function:

kdoc_function scsi_scan.c scsi_scan_target [text] --> text mode output
or
kdoc_function scsi_scan.c scsi_scan_target man --> man page output

This just enables someone to do quick/early checking on one file
at a time without resorting to "make *docs".  While you tell it
just one function to format and print, it checks the entire file
and lists errors.  Using it on scsi_scan.c::scsi_target_reap()
in 2.6.14-rc5 gives:

/////////////////// begin ////////////////////////
Warning(scsi_scan.c:203): No description found for parameter 'starget'
Warning(scsi_scan.c:203): No description found for parameter 'lun'
Warning(scsi_scan.c:203): No description found for parameter 'hostdata'
Function:

void scsi_target_reap (struct scsi_target * starget);

Arguments:

starget
         target to be checked


Description:

This is used after removing a LUN or doing a last put of the target
it checks atomically that nothing is using the target and removes
it if so.
Description:

This is used after removing a LUN or doing a last put of the target
it checks atomically that nothing is using the target and removes
it if so.


  /* ----- end function ----- */
Warning(scsi_scan.c:613): No description found for parameter 'sdev'
Warning(scsi_scan.c:806): No description found for parameter 'sdevp'
Warning(scsi_scan.c:806): No description found for parameter 'rescan'
Warning(scsi_scan.c:806): No description found for parameter 'hostdata'
Warning(scsi_scan.c:912): No description found for parameter 'scsi_level'
Warning(scsi_scan.c:912): No description found for parameter 'rescan'
Warning(scsi_scan.c:1035): No description found for parameter 'lun'
Warning(scsi_scan.c:1065): No description found for parameter 'starget'
Warning(scsi_scan.c:1065): No description found for parameter 'bflags'
Warning(scsi_scan.c:1065): No description found for parameter 'rescan'
////////////// end //////////////

Note:  The   /* ----- end function ----- */
is a local addition to my kernel-doc script (improves readability
for me).

http://www.xenotime.net/linux/scripts/kdoc_function
---
~Randy

[patch 4/6] SCSI HCIL: kill all uses of spi_scan_target()

[Jeff Garzik]

Allocate a target at each callsite, then call scsi_scan_target()

Also, minor stuff:

- export scsi_alloc_target().  eventually this will wind up
  renamed to spi_alloc_target(), and become a wrapper around
  scsi_alloc_target().

- s/scsi_scan_channel/spi_scan_channel/

Signed-off-by: Jeff Garzik <jgarzik@pobox.com>


d35ab3386289899854cb66f877acca185eb83392
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 92fc94b..a48d958 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -326,8 +326,8 @@ static struct scsi_target *__scsi_find_t
 	return found_starget;
 }
 
-static struct scsi_target *scsi_alloc_target(struct device *parent,
-					     int channel, uint id)
+struct scsi_target *scsi_alloc_target(struct device *parent,
+				      int channel, uint id)
 {
 	struct Scsi_Host *shost = dev_to_shost(parent);
 	struct device *dev = NULL;
@@ -402,6 +402,7 @@ static struct scsi_target *scsi_alloc_ta
 	kfree(starget);
 	return found_target;
 }
+EXPORT_SYMBOL(scsi_alloc_target);
 
 /**
  * scsi_target_reap - check to see if target is in use and destroy if not
@@ -1414,10 +1415,11 @@ void scsi_scan_target(struct scsi_target
 }
 EXPORT_SYMBOL(scsi_scan_target);
 
-static void scsi_scan_channel(struct Scsi_Host *shost, unsigned int channel,
-			      unsigned int id, unsigned int lun, int rescan)
+static void spi_scan_channel(struct Scsi_Host *shost, unsigned int channel,
+			     unsigned int id, unsigned int lun, int rescan)
 {
 	uint order_id;
+	struct scsi_target *starget;
 
 	if (id == SCAN_WILD_CARD)
 		for (id = 0; id < shost->max_id; ++id) {
@@ -1437,12 +1439,23 @@ static void scsi_scan_channel(struct Scs
 				order_id = shost->max_id - id - 1;
 			else
 				order_id = id;
-			__spi_scan_target(&shost->shost_gendev, channel,
-					order_id, lun, rescan);
+
+			if (shost->this_id == order_id)
+				/*
+				 * Don't scan the host adapter
+				 */
+				continue;
+
+			starget = scsi_alloc_target(&shost->shost_gendev,
+						    channel, order_id);
+			if (starget)
+				__scsi_scan_target(starget, lun, rescan);
 		}
-	else
-		__spi_scan_target(&shost->shost_gendev, channel,
-				id, lun, rescan);
+	else {
+		starget = scsi_alloc_target(&shost->shost_gendev, channel, id);
+		if (starget)
+			__scsi_scan_target(starget, lun, rescan);
+	}
 }
 
 int scsi_scan_host_selected(struct Scsi_Host *shost, unsigned int channel,
@@ -1461,10 +1474,10 @@ int scsi_scan_host_selected(struct Scsi_
 		if (channel == SCAN_WILD_CARD)
 			for (channel = 0; channel <= shost->max_channel;
 			     channel++)
-				scsi_scan_channel(shost, channel, id, lun,
+				spi_scan_channel(shost, channel, id, lun,
 						  rescan);
 		else
-			scsi_scan_channel(shost, channel, id, lun, rescan);
+			spi_scan_channel(shost, channel, id, lun, rescan);
 	}
 	up(&shost->scan_mutex);
 
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 91d23e9..f7ae486 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -1672,9 +1672,12 @@ static void
 fc_scsi_scan_rport(void *data)
 {
 	struct fc_rport *rport = (struct fc_rport *)data;
+	struct scsi_target *starget;
 
-	spi_scan_target(&rport->dev, rport->channel, rport->scsi_target_id,
-			SCAN_WILD_CARD, 1);
+	starget = scsi_alloc_target(&rport->dev, rport->channel,
+				    rport->scsi_target_id);
+	if (starget)
+		scsi_scan_target(starget, SCAN_WILD_CARD, 1);
 }
 
 
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index f6c1a96..cf40f74 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -578,8 +578,12 @@ int sas_rphy_add(struct sas_rphy *rphy)
 	spin_unlock(&sas_host->lock);
 
 	if (rphy->scsi_target_id != -1) {
-		spi_scan_target(&rphy->dev, parent->number,
-				rphy->scsi_target_id, ~0, 0);
+		struct scsi_target *starget;
+
+		starget = scsi_alloc_target(&rphy->dev, parent->number,
+					    rphy->scsi_target_id);
+		if (starget)
+			scsi_scan_target(starget, ~0, 0);
 	}
 
 	return 0;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index c7cdb23..c5e31fe 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -246,6 +246,8 @@ extern int scsi_device_set_state(struct 
 				 enum scsi_device_state state);
 extern int scsi_device_quiesce(struct scsi_device *sdev);
 extern void scsi_device_resume(struct scsi_device *sdev);
+extern struct scsi_target *scsi_alloc_target(struct device *parent,
+					     int channel, uint id);
 extern void scsi_target_quiesce(struct scsi_target *);
 extern void scsi_target_resume(struct scsi_target *);
 extern void spi_scan_target(struct device *parent, unsigned int channel,

Re: [patch 4/6] SCSI HCIL: kill all uses of spi_scan_target()

[Matthew Wilcox]

On Sat, Oct 22, 2005 at 09:40:12PM -0400, Jeff Garzik wrote:
> - export scsi_alloc_target().  eventually this will wind up
>   renamed to spi_alloc_target(), and become a wrapper around
>   scsi_alloc_target().

Should this be a _GPL export?  All current users are in transport
classes, so it feels a bit internal to me.

[patch 5/6] SCSI HCIL: kill spi_scan_target(), __spi_scan_target()

[Jeff Garzik]

Signed-off-by: Jeff Garzik <jgarzik@pobox.com>


4accfa89ef54c2f7405de58da9b40aa38ef876ca
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index a48d958..3b7a0a7 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1345,51 +1345,6 @@ static void __scsi_scan_target(struct sc
 	put_device(&starget->dev);
 }
 
-static void __spi_scan_target(struct device *parent, unsigned int channel,
-		unsigned int id, unsigned int lun, int rescan)
-{
-	struct Scsi_Host *shost = dev_to_shost(parent);
-	struct scsi_target *starget;
-
-	if (shost->this_id == id)
-		/*
-		 * Don't scan the host adapter
-		 */
-		return;
-
-	starget = scsi_alloc_target(parent, channel, id);
-	if (starget)
-		__scsi_scan_target(starget, lun, rescan);
-}
-
-/**
- * spi_scan_target - scan a target id, possibly including all LUNs on the
- *     target.
- * @parent:	host to scan
- * @channel:	channel to scan
- * @id:		target id to scan
- * @lun:	Specific LUN to scan or SCAN_WILD_CARD
- * @rescan:	passed to LUN scanning routines
- *
- * Description:
- *     Scan the target id on @parent, @channel, and @id. Scan at least LUN 0,
- *     and possibly all LUNs on the target id.
- *
- *     First try a REPORT LUN scan, if that does not scan the target, do a
- *     sequential scan of LUNs on the target id.
- **/
-void spi_scan_target(struct device *parent, unsigned int channel,
-		      unsigned int id, unsigned int lun, int rescan)
-{
-	struct Scsi_Host *shost = dev_to_shost(parent);
-
-	down(&shost->scan_mutex);
-	if (scsi_host_scan_allowed(shost))
-		__spi_scan_target(parent, channel, id, lun, rescan);
-	up(&shost->scan_mutex);
-}
-EXPORT_SYMBOL(spi_scan_target);
-
 /**
  * scsi_scan_target - scan a target id, possibly including all LUNs on the
  *     target.
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index c5e31fe..83cc223 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -250,8 +250,6 @@ extern struct scsi_target *scsi_alloc_ta
 					     int channel, uint id);
 extern void scsi_target_quiesce(struct scsi_target *);
 extern void scsi_target_resume(struct scsi_target *);
-extern void spi_scan_target(struct device *parent, unsigned int channel,
-			    unsigned int id, unsigned int lun, int rescan);
 extern void scsi_scan_target(struct scsi_target *target,
 			     unsigned int lun, int rescan);
 extern void scsi_target_reap(struct scsi_target *);

[patch 6/6] SCSI HCIL: misc cleanups

[Jeff Garzik]

- eliminate use of 'channel' and 'id' in starget_for_each_device()
- unexport __scsi_device_lookup_by_target()
- add some whitespace after '==', on one line

Signed-off-by: Jeff Garzik <jgarzik@pobox.com>


65036fc5edddd0ddf25f99a6e83c2d8fb7ebd46f
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 1f0ebab..88aea32 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -1108,13 +1108,10 @@ EXPORT_SYMBOL(__scsi_iterate_devices);
 void starget_for_each_device(struct scsi_target *starget, void * data,
 		     void (*fn)(struct scsi_device *, void *))
 {
-	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 	struct scsi_device *sdev;
 
-	shost_for_each_device(sdev, shost) {
-		if ((sdev->channel == starget->channel) &&
-		    (sdev->id == starget->id))
-			fn(sdev, data);
+	list_for_each_entry(sdev, &starget->devices, same_target_siblings) {
+		fn(sdev, data);
 	}
 }
 EXPORT_SYMBOL(starget_for_each_device);
@@ -1133,19 +1130,18 @@ EXPORT_SYMBOL(starget_for_each_device);
  * they're need to access the device list in irq context.  Otherwise you
  * really want to use scsi_device_lookup_by_target instead.
  **/
-struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *starget,
+static struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *starget,
 						   uint lun)
 {
 	struct scsi_device *sdev;
 
 	list_for_each_entry(sdev, &starget->devices, same_target_siblings) {
-		if (sdev->lun ==lun)
+		if (sdev->lun == lun)
 			return sdev;
 	}
 
 	return NULL;
 }
-EXPORT_SYMBOL(__scsi_device_lookup_by_target);
 
 /**
  * scsi_device_lookup_by_target - find a device given the target
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 83cc223..5b492f6 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -192,8 +192,6 @@ extern struct scsi_device *__scsi_device
 						uint, uint, uint);
 extern struct scsi_device *scsi_device_lookup_by_target(struct scsi_target *,
 							uint);
-extern struct scsi_device *__scsi_device_lookup_by_target(struct scsi_target *,
-							  uint);
 extern void starget_for_each_device(struct scsi_target *, void *,
 		     void (*fn)(struct scsi_device *, void *));
 

Re: [patch 6/6] SCSI HCIL: misc cleanups

[Matthew Wilcox]

On Sat, Oct 22, 2005 at 09:41:24PM -0400, Jeff Garzik wrote:
> - eliminate use of 'channel' and 'id' in starget_for_each_device()
> - unexport __scsi_device_lookup_by_target()
> - add some whitespace after '==', on one line

All looks good to me.

Re: [patch 0/6] marginalize HCIL a bit

[Jeff Garzik]

Jeff Garzik wrote:
> This patch series makes a tiny bit of progress on the marginalize-SPI
> todo list.
> 
> Patches:
> 1) s/scsi_scan_target/spi_scan_target/
> 2) remove unused scsi_scan_single_target()
> 3) add scsi_scan_target()
> 4) kill all uses of spi_scan_target()
> 5) kill spi_scan_target(), __spi_scan_target()
> 6) misc cleanups


hmmm.  On second thought, I should probably leave __spi_scan_target() in 
there as a static function, to delete the now duplicated code in 
spi_scan_channel().

Re: [patch 0/6] marginalize HCIL a bit

[James Bottomley]

On Sat, 2005-10-22 at 21:36 -0400, Jeff Garzik wrote:
> This patch series makes a tiny bit of progress on the marginalize-SPI
> todo list.
> 
> Patches:
> 1) s/scsi_scan_target/spi_scan_target/
> 2) remove unused scsi_scan_single_target()
> 3) add scsi_scan_target()
> 4) kill all uses of spi_scan_target()
> 5) kill spi_scan_target(), __spi_scan_target()
> 6) misc cleanups

There's an unaddressed lifetime problem in all of this:  Originally the
target object exists solely internally and has its lifetime managed by
the mid-layer (it actually exists only as long as there are LUNs on it).

In your code cleanups, you keep the scsi_target_reap() function (which
is what checks the children and tries to destroy the device if it
doesn't find any) private (well, unexported).  So, on return from your
new scsi_scan_target(), the target pointer might be invalid (already
freed) if you didn't take a reference to starget->dev.  That's counter
to the way lifetime management of objects usually works.

I think the choices are

1. Make the target an explicit object (like it's peers scsi_device and
scsi_host), so the layer creating it is responsible for managing it.
This will get tricky, particularly as we'd need at least lun removal
notifications so the creating layer can decide on destruction.
2. Move scsi_scan_target() into scsi_priv.h to imply only transport
classes should be using it (where there'll be much more scrutiny on
getting the unusual rules right).

James

Re: [patch 0/6] marginalize HCIL a bit

[Luben Tuikov]

On 10/23/05 11:29, James Bottomley wrote:
> There's an unaddressed lifetime problem in all of this:  Originally the
> target object exists solely internally and has its lifetime managed by
> the mid-layer (it actually exists only as long as there are LUNs on it).

And this has always been wrong and I objected to this back when it
was introduced a few years ago.

> In your code cleanups, you keep the scsi_target_reap() function (which
> is what checks the children and tries to destroy the device if it
> doesn't find any) private (well, unexported).  So, on return from your
> new scsi_scan_target(), the target pointer might be invalid (already
> freed) if you didn't take a reference to starget->dev.  That's counter
> to the way lifetime management of objects usually works.
> 
> I think the choices are
> 
> 1. Make the target an explicit object (like it's peers scsi_device and
> scsi_host), so the layer creating it is responsible for managing it.

Which is exactly what I've been proposing: struct scsi_domain_device { };
similarly to how it is done in the SAS Transport Layer/Stack in the link
of my signature.

The new struct scsi_domain_device { } would be a logial (not imposed)
superclass around the transport's domain device representation.
The Transport Layer _registers_ a struct scsi_domain_device { };
and then SCSI Core scans for LUs and does LU bookkeeping.

LUs -> SCSI Core
SCSI Domain Targets -> Transport Layer around its own domain device
representation:

struct scsi_domain_device {
	void *domain_device;    /* opaque to SCSI Core */
	
	struct list_head LU_list;
	...
};

All of this functionality and infrastructure is present in the
SAS Stack and could be taken almost as is.

> This will get tricky, particularly as we'd need at least lun removal
> notifications so the creating layer can decide on destruction.

LU management is SCSI Core's task -- you'll get notified if
the _domain_ device went away, so that you can clean up your LU
representation.

See for example sas_discover_event(struct sas_port *, enum discover_event ev)
for event DISCE_PORT_GONE handled in the discover thread.
sas_unregister_domain_devices() eventually bubbles up to SCSI Core,
and dynamically allocated are handled by their release method (kobject
infrastructure).

	Luben
-- 
http://linux.adaptec.com/sas/
http://www.adaptec.com/sas/

Re: [patch 0/6] marginalize HCIL a bit

[James Bottomley]

On Mon, 2005-10-24 at 11:49 -0400, Luben Tuikov wrote:
> On 10/23/05 11:29, James Bottomley wrote:
> > There's an unaddressed lifetime problem in all of this:  Originally the
> > target object exists solely internally and has its lifetime managed by
> > the mid-layer (it actually exists only as long as there are LUNs on it).
> 
> And this has always been wrong and I objected to this back when it
> was introduced a few years ago.

Only in your opinion.  The reason it was done this way was to protect
drivers from lifetime management, as I explained when it was done.

> > In your code cleanups, you keep the scsi_target_reap() function (which
> > is what checks the children and tries to destroy the device if it
> > doesn't find any) private (well, unexported).  So, on return from your
> > new scsi_scan_target(), the target pointer might be invalid (already
> > freed) if you didn't take a reference to starget->dev.  That's counter
> > to the way lifetime management of objects usually works.
> > 
> > I think the choices are
> > 
> > 1. Make the target an explicit object (like it's peers scsi_device and
> > scsi_host), so the layer creating it is responsible for managing it.
> 
> Which is exactly what I've been proposing: struct scsi_domain_device { };
> similarly to how it is done in the SAS Transport Layer/Stack in the link
> of my signature.
> 
> The new struct scsi_domain_device { } would be a logial (not imposed)
> superclass around the transport's domain device representation.
> The Transport Layer _registers_ a struct scsi_domain_device { };
> and then SCSI Core scans for LUs and does LU bookkeeping.

That's what scsi_scan_target() does.

> LUs -> SCSI Core
> SCSI Domain Targets -> Transport Layer around its own domain device
> representation:
> 
> struct scsi_domain_device {
> 	void *domain_device;    /* opaque to SCSI Core */
> 	
> 	struct list_head LU_list;
> 	...
> };
> 
> All of this functionality and infrastructure is present in the
> SAS Stack and could be taken almost as is.

Amazingly enough, it's also already present in the transport classes
(see vanilla linux kernel source code).

If you look at your domain device with all the sas pieces removed,
you'll see that scsi_target actually covers all the remaining bits
(there are one or two pieces covered by struct generic_device which you
have to have extra components for because you implemented kobjects
instead of a generic device).  scsi_target contains a variable space for
the transport classes to use for their transport specific pieces (which
is where you could have put all the sas specific bits).

The only real difference is that under the current infrastructure scsi
targets aren't designed to stack.  Realistically, the way you have it
implemented, you have several different devices lumped into your domain
device (end, edge, fanout, sata) with different initialisations and
behaviours, so scsi_target maps exactly to end devices.  I'm really not
a huge fan of object type overloading unless there's a good reason for
it, so in the generic device world, you'd probably have different
structures for the other device types (although nothing precludes
implementation as an overloaded type).

James

Re: [patch 0/6] marginalize HCIL a bit

[Luben Tuikov]

On 10/24/05 12:50, James Bottomley wrote:
> On Mon, 2005-10-24 at 11:49 -0400, Luben Tuikov wrote:
> 
>>On 10/23/05 11:29, James Bottomley wrote:
>>
>>>There's an unaddressed lifetime problem in all of this:  Originally the
>>>target object exists solely internally and has its lifetime managed by
>>>the mid-layer (it actually exists only as long as there are LUNs on it).
>>
>>And this has always been wrong and I objected to this back when it
>>was introduced a few years ago.
> 
> 
> Only in your opinion.  The reason it was done this way was to protect
> drivers from lifetime management, as I explained when it was done.

No, not only in my opinion.

What I wanted and still want is _true_ domain device representation.
"scsi_target" doesn't describe a "target" or domain device.

If you want finer control and better representation (one follows
the other) then you'd need to have a proper represention of
a domain device.  This will also help you get rid of HCIL, which
I've been asking for since 2002/3.

>>The new struct scsi_domain_device { } would be a logial (not imposed)
>>superclass around the transport's domain device representation.
>>The Transport Layer _registers_ a struct scsi_domain_device { };
>>and then SCSI Core scans for LUs and does LU bookkeeping.
> 
> 
> That's what scsi_scan_target() does.

HCIL.  It needs to be passed a stuct scsi_domain_device { }, where
this device could be on any domain whatsoever: USB, SBP, SAS, FC, etc.

Then when SCSI Core starts scanning for LU, the Transport Layer
knows how to route those: USB, SBP, SAS, FC, etc.

>>LUs -> SCSI Core
>>SCSI Domain Targets -> Transport Layer around its own domain device
>>representation:
>>
>>struct scsi_domain_device {
>>	void *domain_device;    /* opaque to SCSI Core */
>>	
>>	struct list_head LU_list;
>>	...
>>};
>>
>>All of this functionality and infrastructure is present in the
>>SAS Stack and could be taken almost as is.
> 
> 
> Amazingly enough, it's also already present in the transport classes
> (see vanilla linux kernel source code).

1. What are we talking about here?
2. File location, name and line please.  Thank you.

> If you look at your domain device with all the sas pieces removed,
> you'll see that scsi_target actually covers all the remaining bits
> (there are one or two pieces covered by struct generic_device which you
> have to have extra components for because you implemented kobjects
> instead of a generic device).

And there is a reason for this: domain devices are NOT and WILL NEVER
BE "generic device".  Domain device representations exist only in
the Transport Layer.  What you seem to suggest with your "transport
attributes" is this "appendages-modules" which intersperse everywhere
in SCSI Core and LLDD like octopus's arms.  See USB Storage and SBP.

> scsi_target contains a variable space for
> the transport classes to use for their transport specific pieces (which
> is where you could have put all the sas specific bits).

Absolutely NOT.  Those "transport specific pieces" should be completely
OPAQUE to SCSI Core -- as you saw in my previous email, the
"transport specific pieces" as you called them were 
"void *domain_device;  /* opaque to SCSI Core */".

I guess the proper question to ask now is: "Can you envision it?"

> The only real difference is that under the current infrastructure scsi
> targets aren't designed to stack.  Realistically, the way you have it
> implemented, you have several different devices lumped into your domain
> device (end, edge, fanout, sata) with different initialisations and

1. How this is implemented is Layer dependent (USB/SBP/FC/SAS/iSCSI/etc).
2. A struct domain_device can be _only_ one of end/edge/fanout/sata/etc,
   and only one of those.
Only devices which _make_sense_ to SCSI Core are registered with SCSI Core,
i.e. end devices.  Other type of devices (e.g. expanders) that are
NOT SCSI devices are not registered with SCSI Core, neither should they
be visible anywhere outside of the respective Layer (SAS in this case).

> behaviours, so scsi_target maps exactly to end devices.  I'm really not
> a huge fan of object type overloading unless there's a good reason for
> it, so in the generic device world, you'd probably have different
> structures for the other device types (although nothing precludes
> implementation as an overloaded type).

All this is _outside_ of the scope of a Transport Layer/Stack,
e.g. see USB Storage and SBP.

	Luben
-- 
http://linux.adaptec.com/sas/
http://www.adaptec.com/sas/

Re: [patch 0/6] marginalize HCIL a bit

[James Bottomley]

On Mon, 2005-10-24 at 13:18 -0400, Luben Tuikov wrote:
> >>All of this functionality and infrastructure is present in the
> >>SAS Stack and could be taken almost as is.
> > 
> > 
> > Amazingly enough, it's also already present in the transport classes
> > (see vanilla linux kernel source code).
> 
> 1. What are we talking about here?
> 2. File location, name and line please.  Thank you.

The best commented file is scsi_transport_spi.c; you can see how it adds
spi specific pieces to the scsi_target structure using the generic
transport class capabilities.

> > If you look at your domain device with all the sas pieces removed,
> > you'll see that scsi_target actually covers all the remaining bits
> > (there are one or two pieces covered by struct generic_device which you
> > have to have extra components for because you implemented kobjects
> > instead of a generic device).
> 
> And there is a reason for this: domain devices are NOT and WILL NEVER
> BE "generic device".  Domain device representations exist only in
> the Transport Layer.  What you seem to suggest with your "transport
> attributes" is this "appendages-modules" which intersperse everywhere
> in SCSI Core and LLDD like octopus's arms.  See USB Storage and SBP.

I think you don't quite understand what a generic device is:  It's a
structure which is embedded within other structures, exactly like a
kobject.  The reason for using generic devices instead of kobjects is
that they provide a wider range of useful functionality.

> > scsi_target contains a variable space for
> > the transport classes to use for their transport specific pieces (which
> > is where you could have put all the sas specific bits).
> 
> Absolutely NOT.  Those "transport specific pieces" should be completely
> OPAQUE to SCSI Core -- as you saw in my previous email, the
> "transport specific pieces" as you called them were 
> "void *domain_device;  /* opaque to SCSI Core */".

They *are* opaque to the scsi mid-layer.  Refer to the code in the
vanilla kernel.

> I guess the proper question to ask now is: "Can you envision it?"
> 
> > The only real difference is that under the current infrastructure scsi
> > targets aren't designed to stack.  Realistically, the way you have it
> > implemented, you have several different devices lumped into your domain
> > device (end, edge, fanout, sata) with different initialisations and
> 
> 1. How this is implemented is Layer dependent (USB/SBP/FC/SAS/iSCSI/etc).
> 2. A struct domain_device can be _only_ one of end/edge/fanout/sata/etc,
>    and only one of those.
> Only devices which _make_sense_ to SCSI Core are registered with SCSI Core,
> i.e. end devices.  Other type of devices (e.g. expanders) that are
> NOT SCSI devices are not registered with SCSI Core, neither should they
> be visible anywhere outside of the respective Layer (SAS in this case).

That *is* how the transport classes work.  The obvious example being a
FC rport which has no existence outside of the FC transport class and is
not understood at all by the mid-layer.  Refer to the code in the
vanilla kernel.

James

Re: [patch 0/6] marginalize HCIL a bit

[Luben Tuikov]

On 10/24/05 16:28, James Bottomley wrote:
> On Mon, 2005-10-24 at 13:18 -0400, Luben Tuikov wrote:
> 
>>>>All of this functionality and infrastructure is present in the
>>>>SAS Stack and could be taken almost as is.
>>>
>>>
>>>Amazingly enough, it's also already present in the transport classes
>>>(see vanilla linux kernel source code).
>>
>>1. What are we talking about here?
>>2. File location, name and line please.  Thank you.
> 
> 
> The best commented file is scsi_transport_spi.c; you can see how it adds
> spi specific pieces to the scsi_target structure using the generic
> transport class capabilities.

Again, no file name or line number, plus at that point I can see that
this is going nowhere, well past the "all of this functinality...".

> I think you don't quite understand what a generic device is:  It's a

Hey, in those recent threads, I've been told by you and Jeff that I don't
understand anything.  Why does this now surprise you?  Apparently only you
and Jeff understand everything.  Can we move on now.

> structure which is embedded within other structures, exactly like a
> kobject.  The reason for using generic devices instead of kobjects is
> that they provide a wider range of useful functionality.

Again, domain devices should stay only in the transport layer.  There
is no _need_ to represent them with "generic device" as they have
no _meaning_ outisde the transport (layer).

>>>scsi_target contains a variable space for
>>>the transport classes to use for their transport specific pieces (which
>>>is where you could have put all the sas specific bits).
>>
>>Absolutely NOT.  Those "transport specific pieces" should be completely
>>OPAQUE to SCSI Core -- as you saw in my previous email, the
>>"transport specific pieces" as you called them were 
>>"void *domain_device;  /* opaque to SCSI Core */".
> 
> 
> They *are* opaque to the scsi mid-layer.  Refer to the code in the
> vanilla kernel.

The vanilla kernel has nothing, not even remotely similar to what
I have in the SAS Stack.  Other than the USB and SBP code.

Again: file location, name and line number please.

See how you just say: "the vanilla kernel has it all" and hope that
people will just believe, because let's face it, how many readers
will actually open the Linux source code and go and find it?

But you see when I say that it is there, I actually give
file names, locations, line numbers, function names and even
cut and paste code.

>>I guess the proper question to ask now is: "Can you envision it?"
>>
>>
>>>The only real difference is that under the current infrastructure scsi
>>>targets aren't designed to stack.  Realistically, the way you have it
>>>implemented, you have several different devices lumped into your domain
>>>device (end, edge, fanout, sata) with different initialisations and
>>
>>1. How this is implemented is Layer dependent (USB/SBP/FC/SAS/iSCSI/etc).
>>2. A struct domain_device can be _only_ one of end/edge/fanout/sata/etc,
>>   and only one of those.
>>Only devices which _make_sense_ to SCSI Core are registered with SCSI Core,
>>i.e. end devices.  Other type of devices (e.g. expanders) that are
>>NOT SCSI devices are not registered with SCSI Core, neither should they
>>be visible anywhere outside of the respective Layer (SAS in this case).
> 
> 
> That *is* how the transport classes work.  The obvious example being a

It *is not*.  What your "transport attribute classes" are is a work around
SDI.  The template they stem off of, transport_class, is just exporting
_attributes_.

For example take a look at the event management in the SAS Stack.  Take
a look at all other things it implements.  Such concepts belong to
a separate layer, which doesn't yield to generalization as you've tried
to do.

> FC rport which has no existence outside of the FC transport class and is
> not understood at all by the mid-layer.  Refer to the code in the
> vanilla kernel.

FC is the last example I'd look at as far as anything "proper" is to
be implemented.  I mean how many variations did it go over?

Actually, it is a good thing you refer people to that code, then
they can take a look at the SAS Stack and can make their own conlusions.

	Luben
-- 
http://linux.adaptec.com/sas/
http://www.adaptec.com/sas/

Re: [patch 0/6] marginalize HCIL a bit

[James Bottomley]

On Mon, 2005-10-24 at 16:41 -0400, Luben Tuikov wrote:
> > The best commented file is scsi_transport_spi.c; you can see how it adds
> > spi specific pieces to the scsi_target structure using the generic
> > transport class capabilities.
> 
> Again, no file name or line number, plus at that point I can see that
> this is going nowhere, well past the "all of this functinality...".

I consider scsi_transport_spi.c to be a file name.  Try lines 38-1219.

> > I think you don't quite understand what a generic device is:  It's a
> 
> Hey, in those recent threads, I've been told by you and Jeff that I don't
> understand anything.  Why does this now surprise you?  Apparently only you
> and Jeff understand everything.  Can we move on now.
> 
> > structure which is embedded within other structures, exactly like a
> > kobject.  The reason for using generic devices instead of kobjects is
> > that they provide a wider range of useful functionality.
> 
> Again, domain devices should stay only in the transport layer.  There
> is no _need_ to represent them with "generic device" as they have
> no _meaning_ outisde the transport (layer).

Um, that's not what you said here:

http://marc.theaimsgroup.com/?l=linux-scsi&m=112887522221936&w=2

You said:

> struct scsi_domain_device { ... }; (to be created) is your friend.
> 
> The only way that that design
> 	"should be capable of representing any 
> 	 SCSI domain (FC/SPI/SBP etc ..)"
> 
> Is if it _does not_ have any knowlege about the underlying
> physical domain -- just as it is shown in SAM (and that is the whole point).
> Else you get in this neverending cat-and-mouse game.  If you have the
> abstraction right, then whatever new transport comes along, it would
> be properly represented.

So that's precisely a generic domain device.

But my point is that what's in include/device.h which the kernel calls a
generic device is simply an abstraction that has certain useful
properties; properties which we make use of.  Identical to the way a
kobject has a more limited set of useful properties.

> >>>scsi_target contains a variable space for
> >>>the transport classes to use for their transport specific pieces (which
> >>>is where you could have put all the sas specific bits).
> >>
> >>Absolutely NOT.  Those "transport specific pieces" should be completely
> >>OPAQUE to SCSI Core -- as you saw in my previous email, the
> >>"transport specific pieces" as you called them were 
> >>"void *domain_device;  /* opaque to SCSI Core */".
> > 
> > 
> > They *are* opaque to the scsi mid-layer.  Refer to the code in the
> > vanilla kernel.
> 
> The vanilla kernel has nothing, not even remotely similar to what
> I have in the SAS Stack.  Other than the USB and SBP code.

You've changed the grounds.  Your original claim was that the added
properties weren't opaque, which they are.

> >>>The only real difference is that under the current infrastructure scsi
> >>>targets aren't designed to stack.  Realistically, the way you have it
> >>>implemented, you have several different devices lumped into your domain
> >>>device (end, edge, fanout, sata) with different initialisations and
> >>
> >>1. How this is implemented is Layer dependent (USB/SBP/FC/SAS/iSCSI/etc).
> >>2. A struct domain_device can be _only_ one of end/edge/fanout/sata/etc,
> >>   and only one of those.
> >>Only devices which _make_sense_ to SCSI Core are registered with SCSI Core,
> >>i.e. end devices.  Other type of devices (e.g. expanders) that are
> >>NOT SCSI devices are not registered with SCSI Core, neither should they
> >>be visible anywhere outside of the respective Layer (SAS in this case).
> > 
> > 
> > That *is* how the transport classes work.  The obvious example being a
> 
> It *is not*.  What your "transport attribute classes" are is a work around
> SDI.  The template they stem off of, transport_class, is just exporting
> _attributes_.

No, that's what you keep trying to claim they are.  In fact they're
attributes coupled with library functions.  A good example being
spi_dv_device(struct scsi_device *sdev) which performs Domain Validation
(an SPI specific function) on a given SCSI device.  That is contained
within the SPI transport class and definitely isn't an attribute, it's a
service used by SPI specific drivers.

> For example take a look at the event management in the SAS Stack.  Take
> a look at all other things it implements.  Such concepts belong to
> a separate layer, which doesn't yield to generalization as you've tried
> to do.

In the transport classes, layer specific code is confined, that's what
the spi_ functions do in the spi transport class.  There's no equivalent
in the generic code, it's just a template builder for the transport
specific code.

> > FC rport which has no existence outside of the FC transport class and is
> > not understood at all by the mid-layer.  Refer to the code in the
> > vanilla kernel.
> 
> FC is the last example I'd look at as far as anything "proper" is to
> be implemented.  I mean how many variations did it go over?

Ah, so you accept that the FC transport class does do this but you just
don't want it to be admitted as a valid example because the class grew
organically?  As Jeff has tried to explain, that's how linux development
goes.

James

Re: [patch 0/6] marginalize HCIL a bit

[Luben Tuikov]

On 10/24/05 17:12, James Bottomley wrote:
> 
> I consider scsi_transport_spi.c to be a file name.  Try lines 38-1219.

This started when I said that LU scanning and bookeeping is done
easily when you have struct scsi_domain_deivice representing
a domain device which the transport discovered but other than
that knows nothing about it.  And then I pointed to my code
where and how it is done: drivers/scsi/sas/sas_discover.c.

But you decided to spin-doctor it and quote over 1200 lines
in source code.

You have to be more specific. For example:
drivers/scsi/sas/sas_discover.c, line 1083:

/**
 * sas_do_lu_discovery -- Discover LUs of a SCSI device
 * @dev: pointer to a domain device of interest
 *
 * Discover logical units present in the SCSI device.  I'd like this
 * to be moved to SCSI Core, but SCSI Core has no concept of a "SCSI
 * device with a SCSI Target port".  A SCSI device with a SCSI Target
 * port is a device which the _transport_ found, but other than that,
 * the transport has little or _no_ knowledge about the device.
 * Ideally, a LLDD would register a "SCSI device with a SCSI Target
 * port" with SCSI Core and then SCSI Core would do LU discovery of
 * that device.
 *
 * REPORT LUNS is mandatory.  If a device doesn't support it,
 * it is broken and you should return it.  Nevertheless, we
 * assume (optimistically) that the link hasn't been severed and
 * that maybe we can get to the device anyhow.
 */
static int sas_do_lu_discovery(struct domain_device *dev)
{
	int  res;
	u8  *buffer;
	int  size;

	res = sas_get_report_luns(dev, &buffer, &size);
	if (res) {
		SAS_DPRINTK("dev %llx didn't reply to REPORT LUNS, trying "
			    "LUN 0 anyway\n",
			    SAS_ADDR(dev->sas_addr));
		size = 16;
		buffer = kzalloc(size, GFP_KERNEL);
	}

	res = sas_register_lu(dev, buffer, size);
	if (res) {
		SAS_DPRINTK("dev %llx didn't report any LUs\n",
			    SAS_ADDR(dev->sas_addr));
		res = 0;
	}

	kfree(buffer);
	return res;
}


>>Again, domain devices should stay only in the transport layer.  There
>>is no _need_ to represent them with "generic device" as they have
>>no _meaning_ outisde the transport (layer).
> 
> 
> Um, that's not what you said here:
> 
> http://marc.theaimsgroup.com/?l=linux-scsi&m=112887522221936&w=2
> 
> You said:
> 
> 
>>struct scsi_domain_device { ... }; (to be created) is your friend.
>>
>>The only way that that design
>>	"should be capable of representing any 
>>	 SCSI domain (FC/SPI/SBP etc ..)"
>>
>>Is if it _does not_ have any knowlege about the underlying
>>physical domain -- just as it is shown in SAM (and that is the whole point).
>>Else you get in this neverending cat-and-mouse game.  If you have the
>>abstraction right, then whatever new transport comes along, it would
>>be properly represented.

I don't see any contradiction.  Did you mean to imply that I'm
contradicting myself?  Did you mean to imply that I'm all of a sudden
changing what I think? (and thus, subsequently code I've written?)

I'd like to point you to the source code at the 1st link
of my sig.  It speaks of itself.

> So that's precisely a generic domain device.

This is *your conclusion*.

What I said is that if you want to get rid of HCIL and if you want
to embrace the future _quickly_ (which sadly seems to have passed
by Linux SCSI), you need to actually _represent_ the objects which
you want to ... well control and represent.

The proper layering is that the transport sitting _below_
SCSI Core (not as an "appendage" on the side as in your "transport attributes")
finds transport devices and how it represents them is its own business.
E.g. in the SAS Transport Layer/Stack it is "struct domain_devices",
USB does it differently and SBP does it differently.

It is best if SCSI Core, has a "struct scsi_domain_device" in order
to do LU scanning properly and when it does so, this is what
is passes to Execute Command SCSI RPC, so that the transport layer/LLDD
knows how to map this: WWN, HCIL, etc, to whatever is the underlying
device and transport.

> But my point is that what's in include/device.h which the kernel calls a
> generic device is simply an abstraction that has certain useful
> properties; properties which we make use of.  Identical to the way a
> kobject has a more limited set of useful properties.

You do not need to export that as "generic device".  I know that
you would like to have your hands in everything, but it is best
to keep layers separated (for many reasons).

As to kobject: as I've explained many times: the discover
process discovers the domain and represents it internally as it
was discovered it in the physical world.  A _natural_ extension
was to tack a struct kobject to each internal struct/object
and show the result in sysfs.  This is exactly what you see
here: http://marc.theaimsgroup.com/?l=linux-scsi&m=112629509826900&w=2

>>>>>scsi_target contains a variable space for
>>>>>the transport classes to use for their transport specific pieces (which
>>>>>is where you could have put all the sas specific bits).
>>>>
>>>>Absolutely NOT.  Those "transport specific pieces" should be completely
>>>>OPAQUE to SCSI Core -- as you saw in my previous email, the
>>>>"transport specific pieces" as you called them were 
>>>>"void *domain_device;  /* opaque to SCSI Core */".
>>>
>>>
>>>They *are* opaque to the scsi mid-layer.  Refer to the code in the
>>>vanilla kernel.
>>
>>The vanilla kernel has nothing, not even remotely similar to what
>>I have in the SAS Stack.  Other than the USB and SBP code.
> 
> 
> You've changed the grounds.  Your original claim was that the added
> properties weren't opaque, which they are.

You seem to be very successfull at twisting my words around, or
telling other people that I've "changed grounds".

I've _always_ contended that things should be kept separated
and that LLDD/Transport Layer representations should be opaque
to the layer above.  I even wrote code to show this.

See the whole infrastructure and code in the 1st link in my sig
for proof of this.

Let's for example take file include/scsi/sas/sas_class.h:

1. Look at the phy representation, and the void *lldd_phy at
the bottom -- opaque!

struct sas_phy {
/* private: */
	struct kobject phy_kobj;

	/* protected by ha->event_lock */
	struct list_head   port_event_list;
	struct list_head   phy_event_list;
	struct sas_event   port_events[PORT_NUM_EVENTS];
	struct sas_event   phy_events[PHY_NUM_EVENTS];

	int error;

/* public: */
	/* The following are class:RO, driver:R/W */
	int            enabled;	  /* must be set */

	int            id;	  /* must be set */
	enum sas_class class;
	enum sas_proto iproto;
	enum sas_proto tproto;

	enum sas_phy_type  type;
	enum sas_phy_role  role;
	enum sas_oob_mode  oob_mode;
	enum sas_phy_linkrate linkrate;

	u8   *sas_addr;		  /* must be set */
	u8   attached_sas_addr[SAS_ADDR_SIZE]; /* class:RO, driver: R/W */

	spinlock_t     frame_rcvd_lock;
	u8             *frame_rcvd; /* must be set */
	int            frame_rcvd_size;

	spinlock_t     sas_prim_lock;
	u32            sas_prim;

	struct list_head port_phy_el; /* driver:RO */
	struct sas_port      *port; /* Class:RW, driver: RO */

	struct sas_ha_struct *ha; /* may be set; the class sets it anyway */

	void *lldd_phy;		  /* not touched by the sas_class_code */
};

2. Now look at the port representation and the void *lldd_port
at the bottom -- opaque:

struct sas_port {
/* private: */
	struct kobject port_kobj;
	struct kset    phy_kset;
	struct kset    dev_kset;

	struct completion port_gone_completion;

	struct sas_discovery disc;
	struct domain_device *port_dev;
	struct list_head dev_list;
	enum   sas_phy_linkrate linkrate;

	struct scsi_id_map id_map;

/* public: */
	int id;

	enum sas_class   class;
	u8               sas_addr[SAS_ADDR_SIZE];
	u8               attached_sas_addr[SAS_ADDR_SIZE];
	enum sas_proto   iproto;
	enum sas_proto   tproto;

	enum sas_oob_mode oob_mode;

	spinlock_t       phy_list_lock;
	struct list_head phy_list;
	int              num_phys;
	u32              phy_mask;

	struct sas_ha_struct *ha;

	void *lldd_port;	  /* not touched by the sas class code */
};

3. Now look at the HA representation and the void *lldd_ha at the
bottom -- opaque:

struct sas_ha_struct {
/* private: */
	struct kset      ha_kset; /* "this" */
	struct kset      phy_kset;
	struct kset      port_kset;

	struct semaphore event_sema;
	int              event_thread_kill;

	spinlock_t       event_lock;
	struct list_head ha_event_list;
	struct sas_event ha_events[HA_NUM_EVENTS];
	u32              porte_mask; /* mask of phys for port events */
	u32              phye_mask; /* mask of phys for phy events */

	struct scsi_core core;

/* public: */
	char *sas_ha_name;
	struct pci_dev *pcidev;	  /* should be set */
	struct module *lldd_module; /* should be set */
	struct sas_ha_hw_profile hw_profile;

	u8 *sas_addr;		  /* must be set */
	u8 hashed_sas_addr[HASHED_SAS_ADDR_SIZE];

	spinlock_t      phy_port_lock;
	struct sas_phy  **sas_phy; /* array of valid pointers, must be set */
	struct sas_port **sas_port; /* array of valid pointers, must be set */
	int             num_phys; /* must be set, gt 0, static */

	/* LLDD calls these to notify the class of an event. */
	void (*notify_ha_event)(struct sas_ha_struct *, enum ha_event);
	void (*notify_port_event)(struct sas_phy *, enum port_event);
	void (*notify_phy_event)(struct sas_phy *, enum phy_event);

	/* The class calls these to notify the LLDD of an event. */
	void (*lldd_port_formed)(struct sas_phy *);
	void (*lldd_port_deformed)(struct sas_phy *);

	/* The class calls these when a device is found or gone. */
	int  (*lldd_dev_found)(struct domain_device *);
	void (*lldd_dev_gone)(struct domain_device *);

	/* The class calls this to send a task for execution. */
	int lldd_max_execute_num;
	int lldd_queue_size;
	int (*lldd_execute_task)(struct sas_task *, int num,
				 unsigned long gfp_flags);

	/* Task Management Functions. Must be called from process context. */
	int (*lldd_abort_task)(struct sas_task *);
	int (*lldd_abort_task_set)(struct domain_device *, u8 *lun);
	int (*lldd_clear_aca)(struct domain_device *, u8 *lun);
	int (*lldd_clear_task_set)(struct domain_device *, u8 *lun);
	int (*lldd_I_T_nexus_reset)(struct domain_device *);
	int (*lldd_lu_reset)(struct domain_device *, u8 *lun);
	int (*lldd_query_task)(struct sas_task *);

	/* Port and Adapter management */
	int (*lldd_clear_nexus_port)(struct sas_port *);
	int (*lldd_clear_nexus_ha)(struct sas_ha_struct *);

	/* Phy management */
	int (*lldd_control_phy)(struct sas_phy *, enum phy_func);

	void *lldd_ha;		  /* not touched by sas class code */
};

Let's look at include/scsi/sas/sas_discover.h file:

4. Now let's take a look at struct domain_device, bottom of structure,
see void *lldd_dev?  Opaque:

struct domain_device {
	struct kobject    dev_obj;
	enum sas_dev_type dev_type;

	enum sas_phy_linkrate linkrate;
	enum sas_phy_linkrate min_linkrate;
	enum sas_phy_linkrate max_linkrate;

	int  pathways;

	struct domain_device *parent;
	struct list_head siblings; /* devices on the same level */
	struct sas_port *port;	  /* shortcut to root of the tree */

	struct list_head dev_list_node;

	enum sas_proto    iproto;
	enum sas_proto    tproto;

	u8  sas_addr[SAS_ADDR_SIZE];
	u8  hashed_sas_addr[HASHED_SAS_ADDR_SIZE];

	u8  frame_rcvd[32];

	union {
		struct expander_device ex_dev;
		struct end_device      end_dev;
		struct sata_device     sata_dev; /* STP & directly attached */
	};

	void *lldd_dev;
};

Now let's look at the UPPER Layer opaque representations.

5. Here is the LU struct, see the void *uldd_dev at the bottom?
Opaque!

struct LU {
	struct kobject   lu_obj;
	struct list_head list;

	struct domain_device *parent;

	u8     LUN[8];
	int    inquiry_valid_data_len;
	u8     inquiry_data[SAS_INQUIRY_DATA_LEN];
	struct scsi_core_mapping map;

	enum task_management_type tm_type;

	void  *uldd_dev;
};

Now let's take an example of both UPPER and LOWER opaque
representations:

6. File include/scsi/sas/sas_task.h, struct sas_task,
at the bottom: both uldd_task and lldd_task are OPAQUE!

struct sas_task {
	struct domain_device *dev;
	struct list_head      list;

	spinlock_t   task_state_lock;
	unsigned     task_state_flags;

	enum   sas_proto      task_proto;

	/* Used by the discovery code. */
	struct timer_list     timer;
	struct completion     completion;

	union {
		struct sas_ata_task ata_task;
		struct sas_smp_task smp_task;
		struct sas_ssp_task ssp_task;
	};

	struct scatterlist *scatter;
	int    num_scatter;
	u32    total_xfer_len;
	u8     data_dir:2;	  /* Use PCI_DMA_... */

	struct task_status_struct task_status;
	void   (*task_done)(struct sas_task *task);

	void   *lldd_task;	  /* for use by LLDDs */
	void   *uldd_task;
};

>>>>>The only real difference is that under the current infrastructure scsi
>>>>>targets aren't designed to stack.  Realistically, the way you have it
>>>>>implemented, you have several different devices lumped into your domain
>>>>>device (end, edge, fanout, sata) with different initialisations and
>>>>
>>>>1. How this is implemented is Layer dependent (USB/SBP/FC/SAS/iSCSI/etc).
>>>>2. A struct domain_device can be _only_ one of end/edge/fanout/sata/etc,
>>>>  and only one of those.
>>>>Only devices which _make_sense_ to SCSI Core are registered with SCSI Core,
>>>>i.e. end devices.  Other type of devices (e.g. expanders) that are
>>>>NOT SCSI devices are not registered with SCSI Core, neither should they
>>>>be visible anywhere outside of the respective Layer (SAS in this case).
>>>
>>>
>>>That *is* how the transport classes work.  The obvious example being a
>>
>>It *is not*.  What your "transport attribute classes" are is a work around
>>SDI.  The template they stem off of, transport_class, is just exporting
>>_attributes_.
> 
> 
> No, that's what you keep trying to claim they are.  In fact they're
> attributes coupled with library functions.  A good example being

Which is nothing but *SDI*.

> spi_dv_device(struct scsi_device *sdev) which performs Domain Validation
> (an SPI specific function) on a given SCSI device.  That is contained
> within the SPI transport class and definitely isn't an attribute, it's a
> service used by SPI specific drivers.

1. It is an "appendage".
2. Your "transport attributes" were never _designed_ as a management
   infrastructe because of, amongst other things, see 1.
How many "iterations" did it take to get there?

I tell you again: SCSI Core needs to get slimmer, more straightforward
without those appendages and "transport everything" being everywhere
like octopus' hands...

Layering, in Linux SCSI would do miracles.

BTW, just take a look at USB Storage and SBP code.  The infra is exactly
the same as the SAS Transport Stack.

>>For example take a look at the event management in the SAS Stack.  Take
>>a look at all other things it implements.  Such concepts belong to
>>a separate layer, which doesn't yield to generalization as you've tried
>>to do.
> 
> 
> In the transport classes, layer specific code is confined, that's what
> the spi_ functions do in the spi transport class.  There's no equivalent
> in the generic code, it's just a template builder for the transport
> specific code.

It is one "appendage" next to another "appendage".  What you need
is a _clean_ design and infrastructure, e.g. USB Storage, SBP,
SAS Transport Stack/Layer.

What you fail to admit is that it was not designed as such in the first
place.

> Ah, so you accept that the FC transport class does do this but you just

Ah, so that's what you do: wait for me to say something to hook on and
then spin-doctor words around?

No, I don't accept that.  You basically fail to understand how
the SCSI stack works in SAM, and insist on _your_ solution as per
SCSI Core, just because you have the power to block things from
going in through SCSI Core.

Sad, very sad.  You have to keep a more open mind: USB Storage,
SBP, SAS, ...

> don't want it to be admitted as a valid example because the class grew
> organically?  As Jeff has tried to explain, that's how linux development
> goes.

So what you're suggesting is that no other company out there can
submit code to Linux?

Or if it does, it needs to justify code to the James-Bottomleys
and Christoph-Hellwigs of the list?

How knowlegable are you of SAS?

Just because you put things like this:

	"that's how linux development goes."

companies are discouraged to develop good, quality code in-house
for Linux and then submit good code.  Why?  Because the overhead
of the process of _conficing you_ of x, y and z is way too
expensive in just letting "the community" go hog-wild and then
telling customers: "It is not officially supported.  It is supported
by the community."

So, what those companies do instead is _hire_you_ to write the code
for them.  Isn't that fun?  Hey, what a way to secure a future.

Now the question remains: what about the _quality_ of the
code when written like that?  What about the costs?  Testing?
Deployment?

I refer you to the current, broken, sas transport _attributes_
which you included in the kernel, and the idea of enumerating
all phys on the domain...  So much about quality.

How about logistics?  How about technical knowlege?  How about
*customer satisfaction*?

As you seem to work for a company which has a Linux product,
you should know how well customers want to hear "We sell it,
but we don't support it -- it is supported by ``the community''."

Have a good day,
	Luben
-- 
http://linux.adaptec.com/sas/
http://www.adaptec.com/sas/