[PATCH] add extra inquiry byte 56 data to struct scsi_device

[PATCH] add extra inquiry byte 56 data to struct scsi_device

[James Bottomley]

The idea here is to get a full range of device capabilites in the
scsi_device structure so there's absolutely no need at all for drivers
to snoop the inquiry data.

The missing parameters were:

dt_only - means the device only supports DT transfers (not ST)
ius - means the device supports information unit (IU) transfers
qas - means the device supports the Quick Arbitration and Selection
protocol

Our rather misnamed ones are:

ppr - means the device does DT (DT negotiation may only be done via PPR)
wdtr - means the device does wide
sdtr - means the device does sync

James

===== drivers/scsi/scsi_scan.c 1.127 vs edited =====
--- 1.127/drivers/scsi/scsi_scan.c	2004-06-25 06:56:28 -05:00
+++ edited/drivers/scsi/scsi_scan.c	2004-08-20 11:30:09 -05:00
@@ -576,9 +576,16 @@
 	sdev->lockable = sdev->removable;
 	sdev->soft_reset = (inq_result[7] & 1) && ((inq_result[3] & 7) == 2);
 
-	if (sdev->scsi_level >= SCSI_3 || (sdev->inquiry_len > 56 &&
-		inq_result[56] & 0x04))
-		sdev->ppr = 1;
+	if (sdev->inquiry_len > 56) {
+		if (inq_result[56] & 0x04)
+			sdev->ppr = 1;
+		if ((inq_result[56] & 0x0c) == 0x04)
+			sdev->dt_only = 1;
+		if (inq_result[56] & 0x01)
+			sdev->ius = 1;
+		if (inq_result[56] & 0x02)
+			sdev->qas = 1;
+	}
 	if (inq_result[7] & 0x60)
 		sdev->wdtr = 1;
 	if (inq_result[7] & 0x10)
===== include/scsi/scsi_device.h 1.19 vs edited =====
--- 1.19/include/scsi/scsi_device.h	2004-07-07 11:24:13 -05:00
+++ edited/include/scsi/scsi_device.h	2004-08-20 11:31:05 -05:00
@@ -89,6 +92,10 @@
 	unsigned sdtr:1;	/* Device supports SDTR messages */
 	unsigned wdtr:1;	/* Device supports WDTR messages */
 	unsigned ppr:1;		/* Device supports PPR messages */
+	unsigned dt_only:1;	/* Device only supports DT */
+	unsigned ius:1;		/* Device Supports IUs */
+	unsigned qas:1;		/* Device Support Quick Arbitration
+				 * and Selection */
 	unsigned tagged_supported:1;	/* Supports SCSI-II tagged queuing */
 	unsigned simple_tags:1;	/* simple queue tag messages are enabled */
 	unsigned ordered_tags:1;/* ordered queue tag messages are enabled */

Re: [PATCH] add extra inquiry byte 56 data to struct scsi_device

[Jeff Garzik]

James Bottomley wrote:
> +	if (sdev->inquiry_len > 56) {
> +		if (inq_result[56] & 0x04)
> +			sdev->ppr = 1;
> +		if ((inq_result[56] & 0x0c) == 0x04)
> +			sdev->dt_only = 1;
> +		if (inq_result[56] & 0x01)
> +			sdev->ius = 1;
> +		if (inq_result[56] & 0x02)
> +			sdev->qas = 1;
> +	}


Do you forsee ever adding more feature bits like this?

It seems to me that a more maintainable approach is to cache relevant 
INQUIRY data in a buffer, and then create accessor macros for 
programmers' use.

That way you don't have to keep adding members (really, mnemonics) to a 
data structure, and associated initialization code, each time the 
mid-layer or some low-level drivers want to test a new feature bit. 
Initialization via memcpy is easy and automatic.

	Jeff

Re: [PATCH] add extra inquiry byte 56 data to struct scsi_device

[James Bottomley]

On Fri, 2004-08-20 at 23:21, Jeff Garzik wrote:
> Do you forsee ever adding more feature bits like this?

Never say never, but the standard looks now to be fixed.  The only
waiting extension of SPI5 was fast-360, which doesn't look like being
realised.

> It seems to me that a more maintainable approach is to cache relevant 
> INQUIRY data in a buffer, and then create accessor macros for 
> programmers' use.
> 
> That way you don't have to keep adding members (really, mnemonics) to a 
> data structure, and associated initialization code, each time the 
> mid-layer or some low-level drivers want to test a new feature bit. 
> Initialization via memcpy is easy and automatic.

Only if someone else converts all the drivers...but yet, if more bits
come along, that's probably what we should do.

James

Re: [PATCH] add extra inquiry byte 56 data to struct scsi_device

[James Bottomley]

On Fri, 2004-08-20 at 23:21, Jeff Garzik wrote:
> It seems to me that a more maintainable approach is to cache relevant 
> INQUIRY data in a buffer, and then create accessor macros for 
> programmers' use.
> 
> That way you don't have to keep adding members (really, mnemonics) to a 
> data structure, and associated initialization code, each time the 
> mid-layer or some low-level drivers want to test a new feature bit. 
> Initialization via memcpy is easy and automatic.

Actually, given the fact that we cache the inquiry data anyway, this
seems to be phenomenally easy to do.

James

===== include/scsi/scsi_device.h 1.19 vs edited =====
--- 1.19/include/scsi/scsi_device.h	2004-07-07 11:24:13 -05:00
+++ edited/include/scsi/scsi_device.h	2004-08-20 22:54:00 -05:00
@@ -191,5 +194,37 @@
 static inline int scsi_device_online(struct scsi_device *sdev)
 {
 	return sdev->sdev_state != SDEV_OFFLINE;
+}
+
+/* accessor functions for the SCSI parameters */
+static inline int scsi_device_sync(struct scsi_device *sdev)
+{
+	return sdev->sdtr;
+}
+static inline int scsi_device_wide(struct scsi_device *sdev)
+{
+	return sdev->wdtr;
+}
+static inline int scsi_device_dt(struct scsi_device *sdev)
+{
+	return sdev->ppr;
+}
+static inline int scsi_device_dt_only(struct scsi_device *sdev)
+{
+	if (sdev->inquiry_len < 57)
+		return 0;
+	return (sdev->inquiry[56] & 0x0c) == 0x04;
+}
+static inline int scsi_device_ius(struct scsi_device *sdev)
+{
+	if (sdev->inquiry_len < 57)
+		return 0;
+	return sdev->inquiry[56] & 0x01;
+}
+static inline int scsi_device_qas(struct scsi_device *sdev)
+{
+	if (sdev->inquiry_len < 57)
+		return 0;
+	return sdev->inquiry[56] & 0x02;
 }
 #endif /* _SCSI_SCSI_DEVICE_H */

Re: [PATCH] add extra inquiry byte 56 data to struct scsi_device

[Jeff Garzik]

James Bottomley wrote:
> +/* accessor functions for the SCSI parameters */
> +static inline int scsi_device_sync(struct scsi_device *sdev)
> +{
> +	return sdev->sdtr;
> +}
> +static inline int scsi_device_wide(struct scsi_device *sdev)
> +{
> +	return sdev->wdtr;
> +}
> +static inline int scsi_device_dt(struct scsi_device *sdev)
> +{
> +	return sdev->ppr;
> +}
> +static inline int scsi_device_dt_only(struct scsi_device *sdev)
> +{
> +	if (sdev->inquiry_len < 57)
> +		return 0;
> +	return (sdev->inquiry[56] & 0x0c) == 0x04;
> +}
> +static inline int scsi_device_ius(struct scsi_device *sdev)
> +{
> +	if (sdev->inquiry_len < 57)
> +		return 0;
> +	return sdev->inquiry[56] & 0x01;
> +}
> +static inline int scsi_device_qas(struct scsi_device *sdev)
> +{
> +	if (sdev->inquiry_len < 57)
> +		return 0;
> +	return sdev->inquiry[56] & 0x02;


Very nice.  Two nits (of course),
* my fingers would prefer scsidev_foo or scsi_inq_foo
* might as well take the opportunity to kill sdtr/wdtr/ppr :)

Re: [PATCH] add extra inquiry byte 56 data to struct scsi_device

[Randy.Dunlap]

On Fri, 20 Aug 2004 23:21:48 -0400 Jeff Garzik wrote:

| James Bottomley wrote:
| > +	if (sdev->inquiry_len > 56) {
| > +		if (inq_result[56] & 0x04)
| > +			sdev->ppr = 1;
| > +		if ((inq_result[56] & 0x0c) == 0x04)
| > +			sdev->dt_only = 1;
| > +		if (inq_result[56] & 0x01)
| > +			sdev->ius = 1;
| > +		if (inq_result[56] & 0x02)
| > +			sdev->qas = 1;
| > +	}
| 
| 
| Do you forsee ever adding more feature bits like this?
| 
| It seems to me that a more maintainable approach is to cache relevant 
| INQUIRY data in a buffer, and then create accessor macros for 
| programmers' use.
| 
| That way you don't have to keep adding members (really, mnemonics) to a 

Mnemonics would be names, not numbers.  (hint hint)

| data structure, and associated initialization code, each time the 
| mid-layer or some low-level drivers want to test a new feature bit. 
| Initialization via memcpy is easy and automatic.


--
~Randy

Re: [PATCH] add extra inquiry byte 56 data to struct scsi_device

[Jeff Garzik]

On Fri, Aug 20, 2004 at 09:38:57PM -0700, Randy.Dunlap wrote:
> On Fri, 20 Aug 2004 23:21:48 -0400 Jeff Garzik wrote:
> 
> | James Bottomley wrote:
> | > +	if (sdev->inquiry_len > 56) {
> | > +		if (inq_result[56] & 0x04)
> | > +			sdev->ppr = 1;
> | > +		if ((inq_result[56] & 0x0c) == 0x04)
> | > +			sdev->dt_only = 1;
> | > +		if (inq_result[56] & 0x01)
> | > +			sdev->ius = 1;
> | > +		if (inq_result[56] & 0x02)
> | > +			sdev->qas = 1;
> | > +	}
> | 
> | 
> | Do you forsee ever adding more feature bits like this?
> | 
> | It seems to me that a more maintainable approach is to cache relevant 
> | INQUIRY data in a buffer, and then create accessor macros for 
> | programmers' use.
> | 
> | That way you don't have to keep adding members (really, mnemonics) to a 
> 
> Mnemonics would be names, not numbers.

Nod.  I was referring to the names being added to the struct.

	Jeff

Re: [PATCH] add extra inquiry byte 56 data to struct scsi_device

[Douglas Gilbert]

James Bottomley wrote:
> The idea here is to get a full range of device capabilites in the
> scsi_device structure so there's absolutely no need at all for drivers
> to snoop the inquiry data.
> 
> The missing parameters were:
> 
> dt_only - means the device only supports DT transfers (not ST)
> ius - means the device supports information unit (IU) transfers
> qas - means the device supports the Quick Arbitration and Selection
> protocol
> 
> Our rather misnamed ones are:
> 
> ppr - means the device does DT (DT negotiation may only be done via PPR)
> wdtr - means the device does wide
> sdtr - means the device does sync

James,
What you are proposing is only relevant for the SPI
transport. Maybe the accessors should be placed in
scsi_tranport_spi.c and struct scsi_device get a
a transport dependent "hang on". Then some of that older
SPI stuff could get migrated out of struct scsi_device
so it stops growing.

Doug Gilbert

Re: [PATCH] add extra inquiry byte 56 data to struct scsi_device

[James Bottomley]

On Sat, 2004-08-21 at 09:03, Douglas Gilbert wrote:
> What you are proposing is only relevant for the SPI
> transport. Maybe the accessors should be placed in
> scsi_tranport_spi.c and struct scsi_device get a
> a transport dependent "hang on". Then some of that older
> SPI stuff could get migrated out of struct scsi_device
> so it stops growing.

Perhaps as a long term goal.  However, getting inquiry snooping out of
the drivers is currently more important than converting all the SPI
drivers over to using the transport class.

James