Re: [PATCH] add device_configure to the transport classes

Re: [PATCH] add device_configure to the transport classes

[Martin Peschke3]

James,

now that there is more and more stuff handed over to transport classes,
would it be feasible to put LUN there as well, as its form appears to be
transport specific as well? (e.g. 64 bit for FC while other transports
may be unable to convey 64 bit LUNs)

I see that this particular patch is not exactly what was needed to achieve
this, because at the time of the evaluation INQUIRY data the LUN has
already been adapted to the way LUNs are stored in scsi_device.

I am also not sure yet how this approach would go with REPORT_LUNS,
which returns 64 bit LUNs for all transports. But for everybody interested
in 64 bit LUNs, it would certainly clean things up.

Martin Peschke


James Bottomley <James.Bottomley@SteelEye.com>@vger.kernel.org on
05/10/2004 18:11:01

Sent by:    linux-scsi-owner@vger.kernel.org


To:    SCSI Mailing List <linux-scsi@vger.kernel.org>
cc:
Subject:    [PATCH] add device_configure to the transport classes


The idea here is to have a callback that's invoked after inquiry is
complete so that the transport class can pick fields it's interested in
out of the inquiry data.  The hope would be to have some of those fields
that are currently scsi_device flags go away.

This is against the scsi-target-2.6 tree

James

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/10/05 11:05:24-05:00 jejb@pashleys.(none)
#   add device_configure to the transport classes
#
#   This allows attributes to be picked out of the INQUIRY fields and
#   placed into transport specific capability flags
#
#   Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>
#
# include/scsi/scsi_transport_spi.h
#   2004/10/05 11:05:12-05:00 jejb@pashleys.(none) +17 -0
#   add device_configure to the transport classes
#
# include/scsi/scsi_transport.h
#   2004/10/05 11:05:12-05:00 jejb@pashleys.(none) +1 -0
#   add device_configure to the transport classes
#
# drivers/scsi/scsi_transport_spi.c
#   2004/10/05 11:05:12-05:00 jejb@pashleys.(none) +18 -0
#   add device_configure to the transport classes
#
# drivers/scsi/scsi_scan.c
#   2004/10/05 11:05:12-05:00 jejb@pashleys.(none) +4 -1
#   add device_configure to the transport classes
#
diff -Nru a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c             2004-10-05 11:08:24 -05:00
+++ b/drivers/scsi/scsi_scan.c             2004-10-05 11:08:24 -05:00
@@ -613,7 +613,10 @@
             if (*bflags & BLIST_NOT_LOCKABLE)
                         sdev->lockable = 0;

-            if(sdev->host->hostt->slave_configure)
+            if (sdev->host->transportt->device_configure)
+                        sdev->host->transportt->device_configure(sdev);
+
+            if (sdev->host->hostt->slave_configure)
                         sdev->host->hostt->slave_configure(sdev);

             /*
diff -Nru a/drivers/scsi/scsi_transport_spi.c
b/drivers/scsi/scsi_transport_spi.c
--- a/drivers/scsi/scsi_transport_spi.c          2004-10-05 11:08:24 -05:00
+++ b/drivers/scsi/scsi_transport_spi.c          2004-10-05 11:08:24 -05:00
@@ -150,6 +150,23 @@
             return 0;
 }

+static int spi_configure_device(struct scsi_device *sdev)
+{
+            struct scsi_target *starget = sdev->sdev_target;
+
+            /* Populate the target capability fields with the values
+             * gleaned from the device inquiry */
+
+            spi_support_sync(starget) = scsi_device_sync(sdev);
+            spi_support_wide(starget) = scsi_device_wide(sdev);
+            spi_support_dt(starget) = scsi_device_dt(sdev);
+            spi_support_dt_only(starget) = scsi_device_dt_only(sdev);
+            spi_support_ius(starget) = scsi_device_ius(sdev);
+            spi_support_qas(starget) = scsi_device_qas(sdev);
+
+            return 0;
+}
+
 static int spi_setup_transport_attrs(struct scsi_target *starget)
 {
             spi_period(starget) = -1;           /* illegal value */
@@ -783,6 +800,7 @@
             i->t.target_attrs = &i->attrs[0];
             i->t.target_class = &spi_transport_class;
             i->t.target_setup = &spi_setup_transport_attrs;
+            i->t.device_configure = &spi_configure_device;
             i->t.target_size = sizeof(struct spi_transport_attrs);
             i->t.host_attrs = &i->host_attrs[0];
             i->t.host_class = &spi_host_class;
diff -Nru a/include/scsi/scsi_transport.h b/include/scsi/scsi_transport.h
--- a/include/scsi/scsi_transport.h        2004-10-05 11:08:24 -05:00
+++ b/include/scsi/scsi_transport.h        2004-10-05 11:08:24 -05:00
@@ -36,6 +36,7 @@

             /* Constructor functions */
             int (*device_setup)(struct scsi_device *);
+            int (*device_configure)(struct scsi_device *);
             int (*target_setup)(struct scsi_target *);
             int (*host_setup)(struct Scsi_Host *);

diff -Nru a/include/scsi/scsi_transport_spi.h
b/include/scsi/scsi_transport_spi.h
--- a/include/scsi/scsi_transport_spi.h          2004-10-05 11:08:24 -05:00
+++ b/include/scsi/scsi_transport_spi.h          2004-10-05 11:08:24 -05:00
@@ -37,6 +37,13 @@
             unsigned int pcomp_en:1;/* Precompensation enabled */
             unsigned int initial_dv:1; /* DV done to this target yet  */
             unsigned long flags;          /* flags field for drivers to
use */
+            /* Device Properties fields */
+            unsigned int support_sync:1; /* synchronous support */
+            unsigned int support_wide:1; /* wide support */
+            unsigned int support_dt:1; /* allows DT phases */
+            unsigned int support_dt_only; /* disallows ST phases */
+            unsigned int support_ius; /* support Information Units */
+            unsigned int support_qas; /* supports quick arbitration and
selection */
             /* Private Fields */
             unsigned int dv_pending:1; /* Internal flag */
             struct semaphore dv_sem; /* semaphore to serialise dv */
@@ -65,8 +72,18 @@
 #define spi_rti(x)            (((struct spi_transport_attrs
*)&(x)->starget_data)->rti)
 #define spi_pcomp_en(x)             (((struct spi_transport_attrs
*)&(x)->starget_data)->pcomp_en)
 #define spi_initial_dv(x)           (((struct spi_transport_attrs
*)&(x)->starget_data)->initial_dv)
+
+#define spi_support_sync(x)         (((struct spi_transport_attrs
*)&(x)->starget_data)->support_sync)
+#define spi_support_wide(x)         (((struct spi_transport_attrs
*)&(x)->starget_data)->support_wide)
+#define spi_support_dt(x)           (((struct spi_transport_attrs
*)&(x)->starget_data)->support_dt)
+#define spi_support_dt_only(x)            (((struct spi_transport_attrs
*)&(x)->starget_data)->support_dt_only)
+#define spi_support_ius(x)          (((struct spi_transport_attrs
*)&(x)->starget_data)->support_ius)
+#define spi_support_qas(x)          (((struct spi_transport_attrs
*)&(x)->starget_data)->support_qas)
+
 #define spi_flags(x)          (((struct spi_transport_attrs
*)&(x)->starget_data)->flags)
 #define spi_signalling(h)           (((struct spi_host_attrs
*)&(h)->shost_data)->signalling)
+
+

 /* The functions by which the transport class and the driver communicate
*/
 struct spi_function_template {

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

Re: [PATCH] add device_configure to the transport classes

[James Bottomley]

On Tue, 2004-10-05 at 12:10, Martin Peschke3 wrote:
> now that there is more and more stuff handed over to transport classes,
> would it be feasible to put LUN there as well, as its form appears to be
> transport specific as well? (e.g. 64 bit for FC while other transports
> may be unable to convey 64 bit LUNs)

Well, I'm not sure about that, primarily because the LUN specifications
in the standards aren't transport specific.

> I see that this particular patch is not exactly what was needed to achieve
> this, because at the time of the evaluation INQUIRY data the LUN has
> already been adapted to the way LUNs are stored in scsi_device.
> 
> I am also not sure yet how this approach would go with REPORT_LUNS,
> which returns 64 bit LUNs for all transports. But for everybody interested
> in 64 bit LUNs, it would certainly clean things up.

Yes, that's another reason to make it transport specific.  Even SPI code
has to understand 64 bit LUN returns.

James

Re: [PATCH] add device_configure to the transport classes

[Douglas Gilbert]

James Bottomley wrote:
> On Tue, 2004-10-05 at 12:10, Martin Peschke3 wrote:
> 
>>now that there is more and more stuff handed over to transport classes,
>>would it be feasible to put LUN there as well, as its form appears to be
>>transport specific as well? (e.g. 64 bit for FC while other transports
>>may be unable to convey 64 bit LUNs)
> 
> 
> Well, I'm not sure about that, primarily because the LUN specifications
> in the standards aren't transport specific.

That is right, 64 bit LUNs were mandated in SAM-2
(standardized in 2003) and SAM-3 (soon to become a standard).
So the Linux SCSI subsystem's 32 bit LUNs have been deficient
for some time. [Incidentally I am probably responsible for
bumping the lun representation from 8 bit to 32 bit some years
back. From memory the only reason we didn't go to 64 bits
was uncertainties about 64 bit arithmetic on various target
platforms. Those days are long since gone.]

When SCSI devices start using advanced LUN features like
"well known logical units" (SPC-3 rev 21 section 8)
we are really going to start hurting.

So I sympathize with Martin; in the past he has tried to get
the mid-level's representation expanded from 32 bits to 64 bits
and failed. Now that he tries to shadow luns in the FCP
transport (like is already being done for port identifiers
and names which _are_ transport defined (i.e. arguably should
not be in the mid-level)) he seems to get turned down on the
basis of architecture.

>>I see that this particular patch is not exactly what was needed to achieve
>>this, because at the time of the evaluation INQUIRY data the LUN has
>>already been adapted to the way LUNs are stored in scsi_device.

The SCSI INQUIRY command seems a pretty fluid beast. There is
potentially a large amount of very important information in
its vital product pages (especially the device identification
page descriptors) with other information formerly held by the
INQUIRY command being passed off to new SCSI commands
(e.g. REPORT SUPPORTED OPERATION CODES).

The ugliest aspect of an INQUIRY is the way SPI information
is conveyed in byte 56. That information should have been in
the protocol specific port control mode page (and is for more
recently defined protocols and transports).

>>I am also not sure yet how this approach would go with REPORT_LUNS,
>>which returns 64 bit LUNs for all transports. But for everybody interested
>>in 64 bit LUNs, it would certainly clean things up.
> 
> 
> Yes, that's another reason to make it transport specific.  Even SPI code
> has to understand 64 bit LUN returns.

So does James now support Martin's proposal or is there a "not"
missing in the first sentence of the above paragraph?

Doug Gilbert

Re: [PATCH] add device_configure to the transport classes

[Matthew Wilcox]

On Wed, Oct 06, 2004 at 02:59:38PM +1000, Douglas Gilbert wrote:
> >Yes, that's another reason to make it transport specific.  Even SPI code
> >has to understand 64 bit LUN returns.
> 
> So does James now support Martin's proposal or is there a "not"
> missing in the first sentence of the above paragraph?

I queried James about that yesterday; he said the "not" was missing.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] add device_configure to the transport classes

[Martin Peschke3]

> Really, a lun should just work as SAM defines it.  I.e. the meaning of
> the 64 bit quantity is interpreted by the transport but the mid-layer
> knows how to display  and use it.

Okay. How do LLDDs fit in here? (Does transport here mean transport in the
general meaning, or, in the context of our implementation, transport class
or LLDD?)
If it is up to the transport to interprete a 64 bit quantity, and the
transport (class) is connected to and used by the LLDD, does your statement
then read that it would be a good idea to pass a 64 bit quantity around,
which, in whatever (yet to be understood) way, is also communicated to
LLDDs?

> I think the real transport specific addressing modes are actually the
> target "numbers" aren't they?  If we could make them transport specific
> there'd be no need to impose fictitious target numbers to fibre channel
> WWNs for instance.

Sorry, I was not clear here. I referred to LUN addressing modes.
However, you are right that target ids are transport specific and vary
in size, formats and semantics. I have lost track of whether transport
attributes introduced for these characteristics already do this job, or
whether htey are just more information to be displayed.

Martin

Re: [PATCH] add device_configure to the transport classes

[Martin Peschke3]

> That is right, 64 bit LUNs were mandated in SAM-2
> (standardized in 2003) and SAM-3 (soon to become a standard).

So, do you know how to read table A.2? (Yes, it's only "informative",
and therewith not normative)

> When SCSI devices start using advanced LUN features like
> "well known logical units" (SPC-3 rev 21 section 8)
> we are really going to start hurting.

I see. The bone of contention from my perspective has been that LLDDs
are currently required to guess in which way the mid-layer has compressed
a LUN, and the resulting question: "Which LUN addressing modes does
your LLDD support?" Ouch!

Martin

Re: [PATCH] add device_configure to the transport classes

[Luben Tuikov]

Martin Peschke3 wrote:
> 
> 
> 
> 
>  > That is right, 64 bit LUNs were mandated in SAM-2
>  > (standardized in 2003) and SAM-3 (soon to become a standard).
> 
> So, do you know how to read table A.2? (Yes, it's only "informative",
> and therewith not normative)
> 
>  > When SCSI devices start using advanced LUN features like
>  > "well known logical units" (SPC-3 rev 21 section 8)
>  > we are really going to start hurting.
> 
> I see. The bone of contention from my perspective has been that LLDDs
> are currently required to guess in which way the mid-layer has compressed
> a LUN, and the resulting question: "Which LUN addressing modes does
> your LLDD support?" Ouch!

I remember we talked about 64 bit LUNs 2 years ago on linux-scsi.

I think the LUN (structure) should be opaque to SCSI Core.
So in effect, user applications and LLDDs would interpret it,
but SCSI Core would simply pass it back and forth.

	Luben

[PATCH] add device_configure to the transport classes

[James Bottomley]

The idea here is to have a callback that's invoked after inquiry is
complete so that the transport class can pick fields it's interested in
out of the inquiry data.  The hope would be to have some of those fields
that are currently scsi_device flags go away.

This is against the scsi-target-2.6 tree

James

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/10/05 11:05:24-05:00 jejb@pashleys.(none) 
#   add device_configure to the transport classes
#   
#   This allows attributes to be picked out of the INQUIRY fields and
#   placed into transport specific capability flags
#   
#   Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>
# 
# include/scsi/scsi_transport_spi.h
#   2004/10/05 11:05:12-05:00 jejb@pashleys.(none) +17 -0
#   add device_configure to the transport classes
# 
# include/scsi/scsi_transport.h
#   2004/10/05 11:05:12-05:00 jejb@pashleys.(none) +1 -0
#   add device_configure to the transport classes
# 
# drivers/scsi/scsi_transport_spi.c
#   2004/10/05 11:05:12-05:00 jejb@pashleys.(none) +18 -0
#   add device_configure to the transport classes
# 
# drivers/scsi/scsi_scan.c
#   2004/10/05 11:05:12-05:00 jejb@pashleys.(none) +4 -1
#   add device_configure to the transport classes
# 
diff -Nru a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c	2004-10-05 11:08:24 -05:00
+++ b/drivers/scsi/scsi_scan.c	2004-10-05 11:08:24 -05:00
@@ -613,7 +613,10 @@
 	if (*bflags & BLIST_NOT_LOCKABLE)
 		sdev->lockable = 0;
 
-	if(sdev->host->hostt->slave_configure)
+	if (sdev->host->transportt->device_configure)
+		sdev->host->transportt->device_configure(sdev);
+
+	if (sdev->host->hostt->slave_configure)
 		sdev->host->hostt->slave_configure(sdev);
 
 	/*
diff -Nru a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
--- a/drivers/scsi/scsi_transport_spi.c	2004-10-05 11:08:24 -05:00
+++ b/drivers/scsi/scsi_transport_spi.c	2004-10-05 11:08:24 -05:00
@@ -150,6 +150,23 @@
 	return 0;
 }
 
+static int spi_configure_device(struct scsi_device *sdev)
+{
+	struct scsi_target *starget = sdev->sdev_target;
+
+	/* Populate the target capability fields with the values
+	 * gleaned from the device inquiry */
+
+	spi_support_sync(starget) = scsi_device_sync(sdev);
+	spi_support_wide(starget) = scsi_device_wide(sdev);
+	spi_support_dt(starget) = scsi_device_dt(sdev);
+	spi_support_dt_only(starget) = scsi_device_dt_only(sdev);
+	spi_support_ius(starget) = scsi_device_ius(sdev);
+	spi_support_qas(starget) = scsi_device_qas(sdev);
+
+	return 0;
+}
+
 static int spi_setup_transport_attrs(struct scsi_target *starget)
 {
 	spi_period(starget) = -1;	/* illegal value */
@@ -783,6 +800,7 @@
 	i->t.target_attrs = &i->attrs[0];
 	i->t.target_class = &spi_transport_class;
 	i->t.target_setup = &spi_setup_transport_attrs;
+	i->t.device_configure = &spi_configure_device;
 	i->t.target_size = sizeof(struct spi_transport_attrs);
 	i->t.host_attrs = &i->host_attrs[0];
 	i->t.host_class = &spi_host_class;
diff -Nru a/include/scsi/scsi_transport.h b/include/scsi/scsi_transport.h
--- a/include/scsi/scsi_transport.h	2004-10-05 11:08:24 -05:00
+++ b/include/scsi/scsi_transport.h	2004-10-05 11:08:24 -05:00
@@ -36,6 +36,7 @@
 
 	/* Constructor functions */
 	int (*device_setup)(struct scsi_device *);
+	int (*device_configure)(struct scsi_device *);
 	int (*target_setup)(struct scsi_target *);
 	int (*host_setup)(struct Scsi_Host *);
 
diff -Nru a/include/scsi/scsi_transport_spi.h b/include/scsi/scsi_transport_spi.h
--- a/include/scsi/scsi_transport_spi.h	2004-10-05 11:08:24 -05:00
+++ b/include/scsi/scsi_transport_spi.h	2004-10-05 11:08:24 -05:00
@@ -37,6 +37,13 @@
 	unsigned int pcomp_en:1;/* Precompensation enabled */
 	unsigned int initial_dv:1; /* DV done to this target yet  */
 	unsigned long flags;	/* flags field for drivers to use */
+	/* Device Properties fields */
+	unsigned int support_sync:1; /* synchronous support */
+	unsigned int support_wide:1; /* wide support */
+	unsigned int support_dt:1; /* allows DT phases */
+	unsigned int support_dt_only; /* disallows ST phases */
+	unsigned int support_ius; /* support Information Units */
+	unsigned int support_qas; /* supports quick arbitration and selection */
 	/* Private Fields */
 	unsigned int dv_pending:1; /* Internal flag */
 	struct semaphore dv_sem; /* semaphore to serialise dv */
@@ -65,8 +72,18 @@
 #define spi_rti(x)	(((struct spi_transport_attrs *)&(x)->starget_data)->rti)
 #define spi_pcomp_en(x)	(((struct spi_transport_attrs *)&(x)->starget_data)->pcomp_en)
 #define spi_initial_dv(x)	(((struct spi_transport_attrs *)&(x)->starget_data)->initial_dv)
+
+#define spi_support_sync(x)	(((struct spi_transport_attrs *)&(x)->starget_data)->support_sync)
+#define spi_support_wide(x)	(((struct spi_transport_attrs *)&(x)->starget_data)->support_wide)
+#define spi_support_dt(x)	(((struct spi_transport_attrs *)&(x)->starget_data)->support_dt)
+#define spi_support_dt_only(x)	(((struct spi_transport_attrs *)&(x)->starget_data)->support_dt_only)
+#define spi_support_ius(x)	(((struct spi_transport_attrs *)&(x)->starget_data)->support_ius)
+#define spi_support_qas(x)	(((struct spi_transport_attrs *)&(x)->starget_data)->support_qas)
+
 #define spi_flags(x)	(((struct spi_transport_attrs *)&(x)->starget_data)->flags)
 #define spi_signalling(h)	(((struct spi_host_attrs *)&(h)->shost_data)->signalling)
+
+
 
 /* The functions by which the transport class and the driver communicate */
 struct spi_function_template {