RFC: Transport identifier

RFC: Transport identifier

[Martin K. Petersen]

There are a few cases where it would be useful to know which transport
is associated with a scsi_device.  For instance when determining whether
to send a READ CAPACITY(16) to a device or not:

    static int sd_try_rc16_first(struct scsi_device *sdp)
    {
	if (scsi_device_transport(sdp) == SCSI_TRANSPORT_USB)
	   return 0; /* Run screaming for the hills */
    [...]

This patch implements support for a transport identifier in the
scsi_host.  The id defaults to SPI and it is explicitly overridden in
the host templates for FC, SAS, USB, etc. drivers.

It also looks like the availability of this transport id could improve
the sysfs parsing in lsscsi.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

---

diff --git a/drivers/firewire/fw-sbp2.c b/drivers/firewire/fw-sbp2.c
--- a/drivers/firewire/fw-sbp2.c
+++ b/drivers/firewire/fw-sbp2.c
@@ -1622,6 +1622,7 @@ static struct scsi_host_template scsi_dr
 	.cmd_per_lun		= 1,
 	.can_queue		= 1,
 	.sdev_attrs		= sbp2_scsi_sysfs_attrs,
+	.transport_id		= SCSI_TRANSPORT_SBP,
 };
 
 MODULE_AUTHOR("Kristian Hoegsberg <krh@bitplanet.net>");
diff --git a/drivers/ieee1394/sbp2.c b/drivers/ieee1394/sbp2.c
--- a/drivers/ieee1394/sbp2.c
+++ b/drivers/ieee1394/sbp2.c
@@ -345,6 +345,7 @@ static struct scsi_host_template sbp2_sh
 	.cmd_per_lun		 = SBP2_MAX_CMDS,
 	.can_queue		 = SBP2_MAX_CMDS,
 	.sdev_attrs		 = sbp2_sysfs_sdev_attrs,
+	.transport_id		 = SCSI_TRANSPORT_SBP,
 };
 
 #define SBP2_ROM_VALUE_WILDCARD ~0         /* match all */
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -603,6 +603,7 @@ static struct scsi_host_template iscsi_i
 	.use_clustering         = DISABLE_CLUSTERING,
 	.proc_name              = "iscsi_iser",
 	.this_id                = -1,
+	.transport_id		= SCSI_TRANSPORT_ISCSI,
 };
 
 static struct iscsi_transport iscsi_iser_transport = {
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1594,6 +1594,7 @@ static struct scsi_host_template srp_tem
 	.cmd_per_lun			= SRP_SQ_SIZE,
 	.use_clustering			= ENABLE_CLUSTERING,
 	.shost_attrs			= srp_host_attrs
+	.transport_id			= SCSI_TRANSPORT_ISCSI,
 };
 
 static int srp_add_target(struct srp_host *host, struct srp_target_port *target)
diff --git a/drivers/message/fusion/mptfc.c b/drivers/message/fusion/mptfc.c
--- a/drivers/message/fusion/mptfc.c
+++ b/drivers/message/fusion/mptfc.c
@@ -131,6 +131,7 @@ static struct scsi_host_template mptfc_d
 	.cmd_per_lun			= 7,
 	.use_clustering			= ENABLE_CLUSTERING,
 	.shost_attrs			= mptscsih_host_attrs,
+	.transport_id			= SCSI_TRANSPORT_FC,
 };
 
 /****************************************************************************
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -1047,6 +1047,7 @@ static struct scsi_host_template mptsas_
 	.cmd_per_lun			= 7,
 	.use_clustering			= ENABLE_CLUSTERING,
 	.shost_attrs			= mptscsih_host_attrs,
+	.transport_id			= SCSI_TRANSPORT_SAS,
 };
 
 static int mptsas_get_linkerrors(struct sas_phy *phy)
diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
--- a/drivers/s390/scsi/zfcp_scsi.c
+++ b/drivers/s390/scsi/zfcp_scsi.c
@@ -504,6 +504,7 @@ struct fc_function_template zfcp_transpo
 	.show_host_speed = 1,
 	.show_host_port_id = 1,
 	.disable_target_scan = 1,
+	.transport_id = SCSI_TRANSPORT_FC,
 };
 
 struct zfcp_data zfcp_data = {
diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -149,6 +149,15 @@ int scsi_host_set_state(struct Scsi_Host
 EXPORT_SYMBOL(scsi_host_set_state);
 
 /**
+ * scsi_host_transport_id - Get the transport for a SCSI host
+ * @shost:	pointer to SCSI host
+ */
+enum scsi_transport_id scsi_host_transport_id(struct Scsi_Host *host)
+{
+	return host->transport_id;
+}
+
+/**
  * scsi_remove_host - remove a scsi host
  * @shost:	a pointer to a scsi host to remove
  **/
@@ -358,6 +367,7 @@ struct Scsi_Host *scsi_host_alloc(struct
 	shost->unchecked_isa_dma = sht->unchecked_isa_dma;
 	shost->use_clustering = sht->use_clustering;
 	shost->ordered_tag = sht->ordered_tag;
+	shost->transport_id = sht->transport_id;
 
 	if (sht->supported_mode == MODE_UNKNOWN)
 		/* means we didn't set it ... default to INITIATOR */
diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
--- a/drivers/scsi/iscsi_tcp.c
+++ b/drivers/scsi/iscsi_tcp.c
@@ -836,6 +836,7 @@ static struct scsi_host_template iscsi_s
 	.slave_configure        = iscsi_sw_tcp_slave_configure,
 	.proc_name		= "iscsi_tcp",
 	.this_id		= -1,
+	.transport_id		= SCSI_TRANSPORT_ISCSI,
 };
 
 static struct iscsi_transport iscsi_sw_tcp_transport = {
diff --git a/drivers/scsi/lpfc/lpfc_scsi.c b/drivers/scsi/lpfc/lpfc_scsi.c
--- a/drivers/scsi/lpfc/lpfc_scsi.c
+++ b/drivers/scsi/lpfc/lpfc_scsi.c
@@ -2956,6 +2956,7 @@ struct scsi_host_template lpfc_template 
 	.use_clustering		= ENABLE_CLUSTERING,
 	.shost_attrs		= lpfc_hba_attrs,
 	.max_sectors		= 0xFFFF,
+	.transport_id		= SCSI_TRANSPORT_FC,
 };
 
 struct scsi_host_template lpfc_vport_template = {
@@ -2976,4 +2977,5 @@ struct scsi_host_template lpfc_vport_tem
 	.use_clustering		= ENABLE_CLUSTERING,
 	.shost_attrs		= lpfc_vport_attrs,
 	.max_sectors		= 0xFFFF,
+	.transport_id		= SCSI_TRANSPORT_FC,
 };
diff --git a/drivers/scsi/megaraid/megaraid_sas.c b/drivers/scsi/megaraid/megaraid_sas.c
--- a/drivers/scsi/megaraid/megaraid_sas.c
+++ b/drivers/scsi/megaraid/megaraid_sas.c
@@ -1309,6 +1309,7 @@ static struct scsi_host_template megasas
 	.eh_timed_out = megasas_reset_timer,
 	.bios_param = megasas_bios_param,
 	.use_clustering = ENABLE_CLUSTERING,
+	.transport_id = SCSI_TRANSPORT_SAS,
 };
 
 /**
diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
--- a/drivers/scsi/qla2xxx/qla_os.c
+++ b/drivers/scsi/qla2xxx/qla_os.c
@@ -147,6 +147,7 @@ static struct scsi_host_template qla2x00
 	 */
 	.max_sectors		= 0xFFFF,
 	.shost_attrs		= qla2x00_host_attrs,
+	.transport_id		= SCSI_TRANSPORT_FC,
 };
 
 struct scsi_host_template qla24xx_driver_template = {
@@ -175,6 +176,7 @@ struct scsi_host_template qla24xx_driver
 
 	.max_sectors		= 0xFFFF,
 	.shost_attrs		= qla2x00_host_attrs,
+	.transport_id		= SCSI_TRANSPORT_FC,
 };
 
 static struct scsi_transport_template *qla2xxx_transport_template = NULL;
diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c
--- a/drivers/scsi/qla4xxx/ql4_os.c
+++ b/drivers/scsi/qla4xxx/ql4_os.c
@@ -103,6 +103,7 @@ static struct scsi_host_template qla4xxx
 	.sg_tablesize		= SG_ALL,
 
 	.max_sectors		= 0xFFFF,
+	.transport_id		= SCSI_TRANSPORT_ISCSI,
 };
 
 static struct iscsi_transport qla4xxx_iscsi_transport = {
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -244,6 +244,20 @@ show_shost_active_mode(struct device *de
 
 static DEVICE_ATTR(active_mode, S_IRUGO | S_IWUSR, show_shost_active_mode, NULL);
 
+/* These strings must match the scsi_transport_id enum in scsi.h */
+static const char * const transport_names[] = {
+	"spi", "fc", "sas", "iscsi", "sbp", "usb", "ata",
+};
+
+static ssize_t
+show_shost_transport_id(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct Scsi_Host *shost = class_to_shost(dev);
+
+	return snprintf(buf, 20, "%s\n", transport_names[shost->transport_id]);
+}
+static DEVICE_ATTR(transport_id, S_IRUGO, show_shost_transport_id, NULL);
+
 shost_rd_attr(unique_id, "%u\n");
 shost_rd_attr(host_busy, "%hu\n");
 shost_rd_attr(cmd_per_lun, "%hd\n");
@@ -268,6 +282,7 @@ static struct attribute *scsi_sysfs_shos
 	&dev_attr_active_mode.attr,
 	&dev_attr_prot_capabilities.attr,
 	&dev_attr_prot_guard_type.attr,
+	&dev_attr_transport_id.attr,
 	NULL
 };
 
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -553,7 +553,9 @@ struct scsi_host_template usb_stor_host_
 	.sdev_attrs =			sysfs_device_attr_list,
 
 	/* module management */
-	.module =			THIS_MODULE
+	.module =			THIS_MODULE,
+
+	.transport_id = 		SCSI_TRANSPORT_USB,
 };
 
 /* To Report "Illegal Request: Invalid Field in CDB */
diff --git a/include/linux/libata.h b/include/linux/libata.h
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1144,6 +1144,7 @@ extern struct device_attribute *ata_comm
 	.slave_configure	= ata_scsi_slave_config,	\
 	.slave_destroy		= ata_scsi_slave_destroy,	\
 	.bios_param		= ata_std_bios_param,		\
+	.transport_id		= SCSI_TRANSPORT_ATA,		\
 	.sdev_attrs		= ata_common_sdev_attrs
 
 #define ATA_NCQ_SHT(drv_name)					\
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -533,4 +533,16 @@ static inline __u32 scsi_to_u32(__u8 *pt
 	return (ptr[0]<<24) + (ptr[1]<<16) + (ptr[2]<<8) + ptr[3];
 }
 
+enum scsi_transport_id {
+	SCSI_TRANSPORT_SPI,
+	SCSI_TRANSPORT_FC,
+	SCSI_TRANSPORT_SAS,
+	SCSI_TRANSPORT_ISCSI,
+	SCSI_TRANSPORT_SBP,
+	SCSI_TRANSPORT_USB,
+	SCSI_TRANSPORT_ATA,
+};
+
+extern enum scsi_transport_id scsi_host_transport_id(struct Scsi_Host *);
+
 #endif /* _SCSI_SCSI_H */
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -455,6 +455,11 @@ static inline int scsi_device_protection
 	return sdev->scsi_level > SCSI_2 && sdev->inquiry[5] & (1<<0);
 }
 
+static inline int scsi_device_transport(struct scsi_device *sdev)
+{
+	return scsi_host_transport_id(sdev->host);
+}
+
 #define MODULE_ALIAS_SCSI_DEVICE(type) \
 	MODULE_ALIAS("scsi:t-" __stringify(type) "*")
 #define SCSI_DEVICE_MODALIAS_FMT "scsi:t-0x%02x"
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -471,6 +471,11 @@ struct scsi_host_template {
 	struct device_attribute **sdev_attrs;
 
 	/*
+	 * Type of transport
+	 */
+	enum scsi_transport_id transport_id;
+
+	/*
 	 * List of hosts per template.
 	 *
 	 * This is only for use by scsi_module.c for legacy templates.
@@ -648,6 +653,7 @@ struct Scsi_Host {
 	
 
 	enum scsi_host_state shost_state;
+	enum scsi_transport_id transport_id;
 
 	/* ldm bits */
 	struct device		shost_gendev, shost_dev;

Re: RFC: Transport identifier

[Julian Calaby]

On Thu, Feb 26, 2009 at 15:31, Martin K. Petersen
<martin.petersen@oracle.com> wrote:
>
> There are a few cases where it would be useful to know which transport
> is associated with a scsi_device.  For instance when determining whether
> to send a READ CAPACITY(16) to a device or not:
>
>    static int sd_try_rc16_first(struct scsi_device *sdp)
>    {
>        if (scsi_device_transport(sdp) == SCSI_TRANSPORT_USB)
>           return 0; /* Run screaming for the hills */
>    [...]
>
> This patch implements support for a transport identifier in the
> scsi_host.  The id defaults to SPI and it is explicitly overridden in
> the host templates for FC, SAS, USB, etc. drivers.
>
> It also looks like the availability of this transport id could improve
> the sysfs parsing in lsscsi.
>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

[snip]

> +/* These strings must match the scsi_transport_id enum in scsi.h */
> +static const char * const transport_names[] = {
> +       "spi", "fc", "sas", "iscsi", "sbp", "usb", "ata",
> +};

Firstly, this should probably mark which name is for which value. (I
know it's obvious, but it could get confusing if there are ever user
friendly names here.)

[snip]

> +enum scsi_transport_id {
> +       SCSI_TRANSPORT_SPI,
> +       SCSI_TRANSPORT_FC,
> +       SCSI_TRANSPORT_SAS,
> +       SCSI_TRANSPORT_ISCSI,
> +       SCSI_TRANSPORT_SBP,
> +       SCSI_TRANSPORT_USB,
> +       SCSI_TRANSPORT_ATA,
> +};

Should there be a comment here pointing out that the names must also
be updated if new items are added?

Thanks,

-- 

Julian Calaby

Email: julian.calaby@gmail.com
.Plan: http://sites.google.com/site/juliancalaby/
--
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: RFC: Transport identifier

[Joel Becker]

On Thu, Feb 26, 2009 at 03:54:30PM +1100, Julian Calaby wrote:
> On Thu, Feb 26, 2009 at 15:31, Martin K. Petersen
> <martin.petersen@oracle.com> wrote:
> >
> > There are a few cases where it would be useful to know which transport
> > is associated with a scsi_device.  For instance when determining whether
> > to send a READ CAPACITY(16) to a device or not:
> >
> >    static int sd_try_rc16_first(struct scsi_device *sdp)
> >    {
> >        if (scsi_device_transport(sdp) == SCSI_TRANSPORT_USB)
> >           return 0; /* Run screaming for the hills */
> >    [...]
> >
> > This patch implements support for a transport identifier in the
> > scsi_host.  The id defaults to SPI and it is explicitly overridden in
> > the host templates for FC, SAS, USB, etc. drivers.
> >
> > It also looks like the availability of this transport id could improve
> > the sysfs parsing in lsscsi.
> >
> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> [snip]
> 
> > +/* These strings must match the scsi_transport_id enum in scsi.h */
> > +static const char * const transport_names[] = {
> > +       "spi", "fc", "sas", "iscsi", "sbp", "usb", "ata",
> > +};
> 
> Firstly, this should probably mark which name is for which value. (I
> know it's obvious, but it could get confusing if there are ever user
> friendly names here.)

I like doing this:

static const char * const transport_names[] = {
	[SCSI_TRANSPORT_SPI]		= "spi",
	...
};

Joel

-- 

"It is not the function of our government to keep the citizen from
 falling into error; it is the function of the citizen to keep the
 government from falling into error."
	- Robert H. Jackson

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127
--
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: RFC: Transport identifier

[Martin K. Petersen]

>>>>> "Joel" == Joel Becker <Joel.Becker@oracle.com> writes:

Joel> I like doing this:

Joel> static const char * const transport_names[] = {
Joel> 	[SCSI_TRANSPORT_SPI] = "spi", ...
Joel> };

Joel> Joel

Oh, slick!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: RFC: Transport identifier

[Douglas Gilbert]

Martin K. Petersen wrote:
> There are a few cases where it would be useful to know which transport
> is associated with a scsi_device.  For instance when determining whether
> to send a READ CAPACITY(16) to a device or not:
> 
>     static int sd_try_rc16_first(struct scsi_device *sdp)
>     {
> 	if (scsi_device_transport(sdp) == SCSI_TRANSPORT_USB)
> 	   return 0; /* Run screaming for the hills */
>     [...]
> 
> This patch implements support for a transport identifier in the
> scsi_host.  The id defaults to SPI and it is explicitly overridden in
> the host templates for FC, SAS, USB, etc. drivers.
> 
> It also looks like the availability of this transport id could improve
> the sysfs parsing in lsscsi.

lsscsi could do with some help detecting the transport.
It uses an untidy set of heuristics now that can easily
be tripped up by sysfs changes.

It may be helpful to differentiate between a "near end"
transport (i.e. between the initiator and the target) and
the transport (and possibly different command set) that
the logical unit (LU) reports.

Doug Gilbert

Re: RFC: Transport identifier

[Mike Christie]

Martin K. Petersen wrote:
> There are a few cases where it would be useful to know which transport
> is associated with a scsi_device.  For instance when determining whether
> to send a READ CAPACITY(16) to a device or not:
> 
>     static int sd_try_rc16_first(struct scsi_device *sdp)
>     {
> 	if (scsi_device_transport(sdp) == SCSI_TRANSPORT_USB)
> 	   return 0; /* Run screaming for the hills */
>     [...]
> 
> This patch implements support for a transport identifier in the
> scsi_host.  The id defaults to SPI and it is explicitly overridden in
> the host templates for FC, SAS, USB, etc. drivers.
> 
> It also looks like the availability of this transport id could improve
> the sysfs parsing in lsscsi.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 


> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1594,6 +1594,7 @@ static struct scsi_host_template srp_tem
>  	.cmd_per_lun			= SRP_SQ_SIZE,
>  	.use_clustering			= ENABLE_CLUSTERING,
>  	.shost_attrs			= srp_host_attrs
> +	.transport_id			= SCSI_TRANSPORT_ISCSI,
>  };


This one is a new type, SCSI_TRANSPORT_SRP.

There is a new iscsi driver in Linus and James's current tree in 
drivers/scsi/cxgb3i. And there is a new fc one in drivers/scsi/fcoe.

Re: RFC: Transport identifier

[James Bottomley]

On Wed, 2009-02-25 at 23:31 -0500, Martin K. Petersen wrote:
> There are a few cases where it would be useful to know which transport
> is associated with a scsi_device.  For instance when determining whether
> to send a READ CAPACITY(16) to a device or not:
> 
>     static int sd_try_rc16_first(struct scsi_device *sdp)
>     {
> 	if (scsi_device_transport(sdp) == SCSI_TRANSPORT_USB)
> 	   return 0; /* Run screaming for the hills */
>     [...]
> 
> This patch implements support for a transport identifier in the
> scsi_host.  The id defaults to SPI and it is explicitly overridden in
> the host templates for FC, SAS, USB, etc. drivers.
> 
> It also looks like the availability of this transport id could improve
> the sysfs parsing in lsscsi.

I'd really rather not put transport specific knowledge back into the
mid-layer ... the whole idea of the transport classes was to take it out
as much as possible.

The other thought is that a lot of devices nowadays are bridged (all
SCSI DVDs have SPI to ATA bridges; a lot of high end USB storage or
enclosures has USB to ATA bridges), so a single transport identifier
doesn't quite cover it.

The final thought is that a lot of what you're looking for is actually
in the PROTOCOL field of a VPD inquiry, so it might be possible to use
that to obviate a lot of this.

James

Re: RFC: Transport identifier

[Martin K. Petersen]

>>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:

James> I'd really rather not put transport specific knowledge back into
James> the mid-layer ... the whole idea of the transport classes was to
James> take it out as much as possible.

I felt that implementing transport classes for everything else was a
huge overkill for a simple identifier.


James> The other thought is that a lot of devices nowadays are bridged
James> (all SCSI DVDs have SPI to ATA bridges; a lot of high end USB
James> storage or enclosures has USB to ATA bridges), so a single
James> transport identifier doesn't quite cover it.

Nope.  And it was not means to be concise.  It was meant to be a hint as
to what kind of technology was at play.

To a large extent a dubious_transport bit would suffice.  But I also
wanted to remedy the lsscsi problem while I was at it.  As Doug
mentioned the current heuristics are icky.


James> The final thought is that a lot of what you're looking for is
James> actually in the PROTOCOL field of a VPD inquiry, so it might be
James> possible to use that to obviate a lot of this.

You mean the protocol mode page?  Or the version descriptors in INQUIRY?

I don't have a single device that provides the version descriptors.
Sadly.

I'm fine with the mode page approach, although a quick test shows only
very recent drives fill it out.  I'll try it on all my spindles in the
lab and see what I discover.

Maybe we can reverse the logic a bit and key off of whether the protocol
mode page is provided.  And consider devices dubious if it's not?

The thing is we need to start sending RC16 to a lot of drives very soon
because of the 4KB thing.  And as we've seen RC16 breaks a lot of junk
devices.  So we need a good indicator other than the SCSI rev. because
that unfortunately doesn't cut it.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: RFC: Transport identifier

[FUJITA Tomonori]

On Thu, 26 Feb 2009 15:32:24 -0500
"Martin K. Petersen" <martin.petersen@oracle.com> wrote:

> >>>>> "James" == James Bottomley <James.Bottomley@HansenPartnership.com> writes:
> 
> James> I'd really rather not put transport specific knowledge back into
> James> the mid-layer ... the whole idea of the transport classes was to
> James> take it out as much as possible.
> 
> I felt that implementing transport classes for everything else was a
> huge overkill for a simple identifier.

But touching every lld is not nice. How about setting
shost->transport_id in each transport class (like the attached patch)?
I guess that it would be better to move this to transport class
completely though.


> James> The other thought is that a lot of devices nowadays are bridged
> James> (all SCSI DVDs have SPI to ATA bridges; a lot of high end USB
> James> storage or enclosures has USB to ATA bridges), so a single
> James> transport identifier doesn't quite cover it.
> 
> Nope.  And it was not means to be concise.  It was meant to be a hint as
> to what kind of technology was at play.
> 
> To a large extent a dubious_transport bit would suffice.  But I also
> wanted to remedy the lsscsi problem while I was at it.  As Doug
> mentioned the current heuristics are icky.

Doug's idea (or something like that) sounds reasonable to me.


> James> The final thought is that a lot of what you're looking for is
> James> actually in the PROTOCOL field of a VPD inquiry, so it might be
> James> possible to use that to obviate a lot of this.
> 
> You mean the protocol mode page?  Or the version descriptors in INQUIRY?
> 
> I don't have a single device that provides the version descriptors.
> Sadly.

I don't too. I checked the PROTOCOL field of a VPD inquiry for other
reasons and none of my disk set it.


diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index da63802..010cc33 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -140,6 +140,33 @@ static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL);
 #define shost_rd_attr(field, format_string) \
 shost_rd_attr2(field, field, format_string)
 
+static const struct {
+	enum scsi_transport_id value;
+	char *name;
+} scsi_transport_names[] = {
+	{SCSI_TRANSPORT_SPI, "spi"},
+	{SCSI_TRANSPORT_FC, "fc"},
+	{SCSI_TRANSPORT_SAS, "sas"},
+	{SCSI_TRANSPORT_ISCSI, "iscsi"},
+	{SCSI_TRANSPORT_SBP, "sbp"},
+	{SCSI_TRANSPORT_USB, "usb"},
+	{SCSI_TRANSPORT_ATA, "ata"},
+};
+
+static const char *scsi_transport_name(enum scsi_transport_id id)
+{
+	int i;
+	char *name = NULL;
+
+	for (i = 0; i < ARRAY_SIZE(scsi_transport_names); i++) {
+		if (scsi_transport_names[i].value == id) {
+			name = scsi_transport_names[i].name;
+			break;
+		}
+	}
+	return name;
+}
+
 /*
  * Create the actual show/store functions and data structures.
  */
@@ -244,6 +271,19 @@ show_shost_active_mode(struct device *dev,
 
 static DEVICE_ATTR(active_mode, S_IRUGO | S_IWUSR, show_shost_active_mode, NULL);
 
+static ssize_t
+show_shost_transport_name(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct Scsi_Host *shost = class_to_shost(dev);
+	const char *name = scsi_transport_name(shost->transport_id);
+
+	if (!name)
+		return -EINVAL;
+	return snprintf(buf, 20, "%s\n", name);
+}
+static DEVICE_ATTR(transport_name, S_IRUGO, show_shost_transport_name, NULL);
+
 shost_rd_attr(unique_id, "%u\n");
 shost_rd_attr(host_busy, "%hu\n");
 shost_rd_attr(cmd_per_lun, "%hd\n");
@@ -268,6 +308,7 @@ static struct attribute *scsi_sysfs_shost_attrs[] = {
 	&dev_attr_active_mode.attr,
 	&dev_attr_prot_capabilities.attr,
 	&dev_attr_prot_guard_type.attr,
+	&dev_attr_transport_name.attr,
 	NULL
 };
 
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 5f77417..093bdb5 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -410,6 +410,7 @@ static int fc_host_setup(struct transport_container *tc, struct device *dev,
 		fc_host->work_q = NULL;
 		return -ENOMEM;
 	}
+	shost->transport_id = SCSI_TRANSPORT_FC;
 
 	return 0;
 }
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
index 75c9297..9bfbbb8 100644
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -253,6 +253,7 @@ static int iscsi_setup_host(struct transport_container *tc, struct device *dev,
 						ihost->scan_workq_name);
 	if (!ihost->scan_workq)
 		return -ENOMEM;
+	shost->transport_id = SCSI_TRANSPORT_ISCSI;
 	return 0;
 }
 
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 50988cb..8335de8 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -287,6 +287,7 @@ static int sas_host_setup(struct transport_container *tc, struct device *dev,
 		dev_printk(KERN_ERR, dev, "fail to a bsg device %d\n",
 			   shost->host_no);
 
+	shost->transport_id = SCSI_TRANSPORT_SAS;
 	return 0;
 }
 
diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index 21a045e..0533977 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -63,6 +63,7 @@ static int srp_host_setup(struct transport_container *tc, struct device *dev,
 	struct srp_host_attrs *srp_host = to_srp_host_attrs(shost);
 
 	atomic_set(&srp_host->next_port_id, 0);
+	shost->transport_id = SCSI_TRANSPORT_SRP;
 	return 0;
 }
 
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index a109165..4d1d75e 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -533,4 +533,14 @@ static inline __u32 scsi_to_u32(__u8 *ptr)
 	return (ptr[0]<<24) + (ptr[1]<<16) + (ptr[2]<<8) + ptr[3];
 }
 
+enum scsi_transport_id {
+	SCSI_TRANSPORT_SPI = 0,
+	SCSI_TRANSPORT_FC,
+	SCSI_TRANSPORT_SAS,
+	SCSI_TRANSPORT_ISCSI,
+	SCSI_TRANSPORT_SBP,
+	SCSI_TRANSPORT_USB,
+	SCSI_TRANSPORT_ATA,
+};
+
 #endif /* _SCSI_SCSI_H */
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index d123ca8..54fc90f 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -648,6 +648,7 @@ struct Scsi_Host {
 	
 
 	enum scsi_host_state shost_state;
+	enum scsi_transport_id transport_id;
 
 	/* ldm bits */
 	struct device		shost_gendev, shost_dev;

Re: RFC: Transport identifier

[Martin K. Petersen]

>>>>> "Tomo" == FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> writes:

Tomo> How about setting shost->transport_id in each transport class
Tomo> (like the attached patch)?  I guess that it would be better to
Tomo> move this to transport class completely though.

The following patch below sits somewhere between my original take and
your patch.  Those transports that have classes implemented fill out the
id like you did.  The ones that don't (ATA, FireWire and USB) set it
manually.  Best of both worlds, IMHO.

The patch provides a solution for lsscsi scanning.  I'll save the
scsi_device portion for another time.


>> I don't have a single device that provides the version descriptors.
>> Sadly.

Tomo> I don't too. I checked the PROTOCOL field of a VPD inquiry for
Tomo> other reasons and none of my disk set it.

I tested in my lab today and out of about 30 drive models I only had one
that had the protocol mode pages filled out.  Not a single drive had the
version descriptors.  I also tried a few USB and FireWire drives.
Zilch.

I was planning on rolling an orthogonal patch that exposed device/port
protocol.  But given the test results I'm not so sure it's worth the
effort.


diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3215,6 +3215,7 @@ int ata_scsi_add_hosts(struct ata_host *
 		shost->max_lun = 1;
 		shost->max_channel = 1;
 		shost->max_cmd_len = 16;
+		shost->transport_id = SCSI_TRANSPORT_ATA;
 
 		/* Schedule policy is determined by ->qc_defer()
 		 * callback and it needs to see every deferred qc.
diff --git a/drivers/firewire/fw-sbp2.c b/drivers/firewire/fw-sbp2.c
--- a/drivers/firewire/fw-sbp2.c
+++ b/drivers/firewire/fw-sbp2.c
@@ -1144,6 +1144,7 @@ static int sbp2_probe(struct device *dev
 	if (scsi_add_host(shost, &unit->device) < 0)
 		goto fail_shost_put;
 
+	shost->transport_id = SCSI_TRANSPORT_SBP;
 	fw_device_get(device);
 	fw_unit_get(unit);
 
diff --git a/drivers/ieee1394/sbp2.c b/drivers/ieee1394/sbp2.c
--- a/drivers/ieee1394/sbp2.c
+++ b/drivers/ieee1394/sbp2.c
@@ -880,6 +880,7 @@ static struct sbp2_lu *sbp2_alloc_device
 	}
 
 	shost->hostdata[0] = (unsigned long)lu;
+	shost->transport_id = SCSI_TRANSPORT_SBP;
 
 	if (!scsi_add_host(shost, &ud->device)) {
 		lu->shost = shost;
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -140,6 +140,35 @@ static DEVICE_ATTR(name, S_IRUGO, show_#
 #define shost_rd_attr(field, format_string) \
 shost_rd_attr2(field, field, format_string)
 
+static const struct {
+	enum scsi_transport_id value;
+	char *name;
+} scsi_transport_names[] = {
+	{ SCSI_TRANSPORT_UNKNOWN,	"unknown"	},
+	{ SCSI_TRANSPORT_SPI,		"spi"		},
+	{ SCSI_TRANSPORT_FC,		"fc"		},
+	{ SCSI_TRANSPORT_SAS,		"sas"		},
+	{ SCSI_TRANSPORT_ISCSI,		"iscsi"		},
+	{ SCSI_TRANSPORT_SBP,		"sbp"		},
+	{ SCSI_TRANSPORT_USB,		"usb"		},
+	{ SCSI_TRANSPORT_ATA,		"ata"		},
+};
+
+static const char *scsi_transport_name(enum scsi_transport_id id)
+{
+	int i;
+	char *name = NULL;
+
+	for (i = 0; i < ARRAY_SIZE(scsi_transport_names); i++) {
+		if (scsi_transport_names[i].value == id) {
+			name = scsi_transport_names[i].name;
+			break;
+		}
+	}
+
+	return name;
+}
+
 /*
  * Create the actual show/store functions and data structures.
  */
@@ -244,6 +273,20 @@ show_shost_active_mode(struct device *de
 
 static DEVICE_ATTR(active_mode, S_IRUGO | S_IWUSR, show_shost_active_mode, NULL);
 
+static ssize_t
+show_shost_transport_name(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct Scsi_Host *shost = class_to_shost(dev);
+	const char *name = scsi_transport_name(shost->transport_id);
+
+	if (!name)
+		return -EINVAL;
+
+	return snprintf(buf, 20, "%s\n", name);
+}
+static DEVICE_ATTR(transport_name, S_IRUGO, show_shost_transport_name, NULL);
+
 shost_rd_attr(unique_id, "%u\n");
 shost_rd_attr(host_busy, "%hu\n");
 shost_rd_attr(cmd_per_lun, "%hd\n");
@@ -268,6 +311,7 @@ static struct attribute *scsi_sysfs_shos
 	&dev_attr_active_mode.attr,
 	&dev_attr_prot_capabilities.attr,
 	&dev_attr_prot_guard_type.attr,
+	&dev_attr_transport_name.attr,
 	NULL
 };
 
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -410,6 +410,7 @@ static int fc_host_setup(struct transpor
 		fc_host->work_q = NULL;
 		return -ENOMEM;
 	}
+	shost->transport_id = SCSI_TRANSPORT_FC;
 
 	return 0;
 }
diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
--- a/drivers/scsi/scsi_transport_iscsi.c
+++ b/drivers/scsi/scsi_transport_iscsi.c
@@ -253,6 +253,7 @@ static int iscsi_setup_host(struct trans
 						ihost->scan_workq_name);
 	if (!ihost->scan_workq)
 		return -ENOMEM;
+	shost->transport_id = SCSI_TRANSPORT_ISCSI;
 	return 0;
 }
 
diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -287,6 +287,7 @@ static int sas_host_setup(struct transpo
 		dev_printk(KERN_ERR, dev, "fail to a bsg device %d\n",
 			   shost->host_no);
 
+	shost->transport_id = SCSI_TRANSPORT_SAS;
 	return 0;
 }
 
diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -63,6 +63,7 @@ static int srp_host_setup(struct transpo
 	struct srp_host_attrs *srp_host = to_srp_host_attrs(shost);
 
 	atomic_set(&srp_host->next_port_id, 0);
+	shost->transport_id = SCSI_TRANSPORT_SRP;
 	return 0;
 }
 
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -1041,6 +1041,7 @@ static int storage_probe(struct usb_inte
 	 * Allow 16-byte CDBs and thus > 2TB
 	 */
 	host->max_cmd_len = 16;
+	host->transport_id = SCSI_TRANSPORT_USB;
 	us = host_to_us(host);
 	memset(us, 0, sizeof(struct us_data));
 	mutex_init(&(us->dev_mutex));
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -533,4 +533,15 @@ static inline __u32 scsi_to_u32(__u8 *pt
 	return (ptr[0]<<24) + (ptr[1]<<16) + (ptr[2]<<8) + ptr[3];
 }
 
+enum scsi_transport_id {
+	SCSI_TRANSPORT_UNKNOWN = 0,
+	SCSI_TRANSPORT_SPI,
+	SCSI_TRANSPORT_FC,
+	SCSI_TRANSPORT_SAS,
+	SCSI_TRANSPORT_ISCSI,
+	SCSI_TRANSPORT_SBP,
+	SCSI_TRANSPORT_USB,
+	SCSI_TRANSPORT_ATA,
+};
+
 #endif /* _SCSI_SCSI_H */
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -648,6 +648,7 @@ struct Scsi_Host {
 	
 
 	enum scsi_host_state shost_state;
+	enum scsi_transport_id transport_id;
 
 	/* ldm bits */
 	struct device		shost_gendev, shost_dev;

Re: RFC: Transport identifier

[Matthew Wilcox]

On Fri, Feb 27, 2009 at 11:13:03PM -0500, Martin K. Petersen wrote:
> The following patch below sits somewhere between my original take and
> your patch.  Those transports that have classes implemented fill out the
> id like you did.  The ones that don't (ATA, FireWire and USB) set it
> manually.  Best of both worlds, IMHO.

You seem to have missed the patch to scsi_transport_spi.c.
There's also a lot of parallel SCSI drivers that don't use
scsi_transport_spi.c yet.  I'm OK with them being 'unknown' and
eventually people patching them (or perhaps better, converting them to
use scsi_transport_spi!), but thought it was worth mentioning.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> diff --git a/drivers/firewire/fw-sbp2.c b/drivers/firewire/fw-sbp2.c
> diff --git a/drivers/ieee1394/sbp2.c b/drivers/ieee1394/sbp2.c
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
> diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: RFC: Transport identifier

[FUJITA Tomonori]

On Fri, 27 Feb 2009 21:50:14 -0700
Matthew Wilcox <matthew@wil.cx> wrote:

> On Fri, Feb 27, 2009 at 11:13:03PM -0500, Martin K. Petersen wrote:
> > The following patch below sits somewhere between my original take and
> > your patch.  Those transports that have classes implemented fill out the
> > id like you did.  The ones that don't (ATA, FireWire and USB) set it
> > manually.  Best of both worlds, IMHO.
> 
> You seem to have missed the patch to scsi_transport_spi.c.
> There's also a lot of parallel SCSI drivers that don't use
> scsi_transport_spi.c yet.  I'm OK with them being 'unknown' and
> eventually people patching them (or perhaps better, converting them to
> use scsi_transport_spi!), but thought it was worth mentioning.

Yeah, that's why my patch sets SCSI_TRANSPORT_SPI to 0. So a lot of
parallel SCSI drivers that don't use spi class can get 'spi'
properly. It's better to convert them to use spi class but I really
doubt someone would do.

Re: RFC: Transport identifier

[Martin K. Petersen]

>>>>> "Tomo" == FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> writes:

Tomo> Yeah, that's why my patch sets SCSI_TRANSPORT_SPI to 0. So a lot
Tomo> of parallel SCSI drivers that don't use spi class can get 'spi'
Tomo> properly.

Oh, duh.  I added the unknown bits last minute to check for devices that
didn't have a class.  Shows you how many parallel devices I have left :)

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: RFC: Transport identifier

[Stefan Richter]

Martin K. Petersen wrote:
> The thing is we need to start sending RC16 to a lot of drives very soon
> because of the 4KB thing.  And as we've seen RC16 breaks a lot of junk
> devices.  So we need a good indicator other than the SCSI rev. because
> that unfortunately doesn't cut it.

/Is/ the used transport protocol a good indicator for whether a
particular target breaks by READ CAPACITY 16?  I have doubts.  Command
set support is independent of transport protocol support.
-- 
Stefan Richter
-=====-==--= --=- ===--
http://arcgraph.de/sr/

Re: RFC: Transport identifier

[Matthew Wilcox]

On Sat, Feb 28, 2009 at 03:53:31PM +0100, Stefan Richter wrote:
> Martin K. Petersen wrote:
> > The thing is we need to start sending RC16 to a lot of drives very soon
> > because of the 4KB thing.  And as we've seen RC16 breaks a lot of junk
> > devices.  So we need a good indicator other than the SCSI rev. because
> > that unfortunately doesn't cut it.
> 
> /Is/ the used transport protocol a good indicator for whether a
> particular target breaks by READ CAPACITY 16?  I have doubts.  Command
> set support is independent of transport protocol support.

You must admit there's a striking correlation between USB devices and
completely failing to follow the SCSI spec.  Our current workaround of
clamping USB devices at a SCSI_2 level does avoid much of the pain.

I can only hope that UAS devices actually implement SCSI instead of
flicking bits at random until vista doesn't crash every time you plug
it in.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: RFC: Transport identifier

[Stefan Richter]

Matthew Wilcox wrote:
> On Sat, Feb 28, 2009 at 03:53:31PM +0100, Stefan Richter wrote:
>> /Is/ the used transport protocol a good indicator for whether a
>> particular target breaks by READ CAPACITY 16?  I have doubts.  Command
>> set support is independent of transport protocol support.
> 
> You must admit there's a striking correlation between USB devices and
> completely failing to follow the SCSI spec.  Our current workaround of
> clamping USB devices at a SCSI_2 level does avoid much of the pain.

OK, but currently implementations typically are:
 1. transport layer driver detects device with known flaws, switches on
    a device quirk flag,
 2. transport layer enables quirk for all devices which it serves by,
    default, possibly disables quirk for some whitelisted devices.

Pulling the SCSI level down in usb-storage belongs into the latter category.

Martin's proposal for transport identifiers includes the suggestion of a
third strategy:
 3. transport layer identifies itself, upper layer enable some quirks
    based on that information (alone or in combination with whatever
    other information).

So, while it may be prudent to deduct "it's USB" -> "don't try READ
CAPACITY 16", why not keep implementing this in way #2?
-- 
Stefan Richter
-=====-==--= --=- ===--
http://arcgraph.de/sr/

Re: RFC: Transport identifier

[James Bottomley]

On Sat, 2009-02-28 at 16:19 +0100, Stefan Richter wrote:
> Matthew Wilcox wrote:
> > On Sat, Feb 28, 2009 at 03:53:31PM +0100, Stefan Richter wrote:
> >> /Is/ the used transport protocol a good indicator for whether a
> >> particular target breaks by READ CAPACITY 16?  I have doubts.  Command
> >> set support is independent of transport protocol support.
> > 
> > You must admit there's a striking correlation between USB devices and
> > completely failing to follow the SCSI spec.  Our current workaround of
> > clamping USB devices at a SCSI_2 level does avoid much of the pain.
> 
> OK, but currently implementations typically are:
>  1. transport layer driver detects device with known flaws, switches on
>     a device quirk flag,
>  2. transport layer enables quirk for all devices which it serves by,
>     default, possibly disables quirk for some whitelisted devices.
> 
> Pulling the SCSI level down in usb-storage belongs into the latter category.
> 
> Martin's proposal for transport identifiers includes the suggestion of a
> third strategy:
>  3. transport layer identifies itself, upper layer enable some quirks
>     based on that information (alone or in combination with whatever
>     other information).

Actually, that's the piece of this proposal I'm explicitly saying no to:
we're not going to code transport knowledge into the mid-layer because
it's setting us up to get things wrong.

The thing that may be useful in all of this is for udev to get the
transport type from sysfs ... I'm not entirely sure about this one,
since udev seems to do a pretty good job on its own without an exported
identifier, but I can see an explicit one might be useful to it.

> So, while it may be prudent to deduct "it's USB" -> "don't try READ
> CAPACITY 16", why not keep implementing this in way #2?

We will.  That way if a wonderfully compliant USB device comes along
(big if, I know) you can white list it in the USB (or SBP-2) layer and
pass along its true SCSI compliance level.

James

Re: RFC: Transport identifier

[Martin K. Petersen]

>>>>> "Stefan" == Stefan Richter <stefanr@s5r6.in-berlin.de> writes:

Stefan> So, while it may be prudent to deduct "it's USB" -> "don't try
Stefan> READ CAPACITY 16", why not keep implementing this in way #2?

I predict our scanning will involve much more heuristics than it does
now.  In relatively near future we will have to start using a less
conservative approach to sending commands because 4KB drives and drives
with odd alignment require us to use READ CAPACITY(16).

The triggers at the device level which would help (protocol mode pages,
version descriptors) are generally not filled out.  Not even in brand
new enterprise drives.

For DIF I had the luxury of being able to trigger off the PROTECT bit in
INQUIRY.  And even that broke because some USB devices returned random
garbage causing DIF to be enabled by accident.

The other problem we are facing is that right now the reported version
kinda-sorta works but that may not continue to be the case.  There are
several drive vendors that deliberately return a much older version than
the command set actually supported by the drive.  This is to work around
problems in legacy proprietary operating systems.

Originally my plan was to key off of the presence of the block limits
VPD page and submit RC16 if that was present.  But the VPD is mostly
aimed at RAID vendors and I'm told that drive vendors are not going to
bother filling it out.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: RFC: Transport identifier

[Martin K. Petersen]

>>>>> "Stefan" == Stefan Richter <stefanr@s5r6.in-berlin.de> writes:

Stefan> particular target breaks by READ CAPACITY 16?  I have doubts.
Stefan> Command set support is independent of transport protocol
Stefan> support.

I think that's a separate discussion we need to have.  My original patch
facilitates a check like that but it's not the only use for the
transport_id.

-- 
Martin K. Petersen	Oracle Linux Engineering