TYPE_RBC cache fixes (sbp2.c affected)

TYPE_RBC cache fixes (sbp2.c affected)

[Al Viro]

	a) TYPE_SDAD renamed to TYPE_RBC and taken to scsi.h
	b) in sbp2.c remapping of TYPE_RPB to TYPE_DISK turned off
	c) relevant places in midlayer and sd.c taught to accept TYPE_RBC
	d) sd.c::sd_read_cache_type() looks into page 6 when dealing with
TYPE_RBC - these guys have writeback cache flag there and are not guaranteed
to have page 8 at all.
	e) sd_read_cache_type() got an extra sanity check - it checks that
it got the page it asked for before using its contents.  And screams if
mismatch had happened.  Rationale: there are broken devices out there that
are "helpful" enough to go for "I don't have a page you've asked for, here,
have another one".  For example, PL3507 had been caught doing just that...
	f) sbp2 sets sdev->use_10_for_rw and sdev->use_10_for_ms instead
of bothering to remap READ6/WRITE6/MOD_SENSE, so most of the conversions
in there are gone now.

	Incidentally, I wonder if USB storage devices that have no
mode page 8 are simply RBC ones.  I haven't touched that, but it might
be interesting to check...
	
Signed-off-by: Al Viro <viro@parcelfarce.linux.theplanet.co.uk>
----
diff -urN RC12-rc4-base/drivers/ieee1394/sbp2.c RC12-rc4-rbc/drivers/ieee1394/sbp2.c
--- RC12-rc4-base/drivers/ieee1394/sbp2.c	2005-05-07 04:04:56.000000000 -0400
+++ RC12-rc4-rbc/drivers/ieee1394/sbp2.c	2005-05-15 12:17:39.716295613 -0400
@@ -1070,7 +1070,7 @@
 static __inline__ int sbp2_command_conversion_device_type(u8 device_type)
 {
 	return (((device_type == TYPE_DISK) ||
-		 (device_type == TYPE_SDAD) ||
+		 (device_type == TYPE_RBC) ||
 		 (device_type == TYPE_ROM)) ? 1:0);
 }
 
@@ -2111,102 +2111,6 @@
  */
 static void sbp2_check_sbp2_command(struct scsi_id_instance_data *scsi_id, unchar *cmd)
 {
-	unchar new_cmd[16];
-	u8 device_type = SBP2_DEVICE_TYPE (scsi_id->sbp2_device_type_and_lun);
-
-	SBP2_DEBUG("sbp2_check_sbp2_command");
-
-	switch (*cmd) {
-
-		case READ_6:
-
-			if (sbp2_command_conversion_device_type(device_type)) {
-
-				SBP2_DEBUG("Convert READ_6 to READ_10");
-
-				/*
-				 * Need to turn read_6 into read_10
-				 */
-				new_cmd[0] = 0x28;
-				new_cmd[1] = (cmd[1] & 0xe0);
-				new_cmd[2] = 0x0;
-				new_cmd[3] = (cmd[1] & 0x1f);
-				new_cmd[4] = cmd[2];
-				new_cmd[5] = cmd[3];
-				new_cmd[6] = 0x0;
-				new_cmd[7] = 0x0;
-				new_cmd[8] = cmd[4];
-				new_cmd[9] = cmd[5];
-
-				memcpy(cmd, new_cmd, 10);
-
-			}
-
-			break;
-
-		case WRITE_6:
-
-			if (sbp2_command_conversion_device_type(device_type)) {
-
-				SBP2_DEBUG("Convert WRITE_6 to WRITE_10");
-
-				/*
-				 * Need to turn write_6 into write_10
-				 */
-				new_cmd[0] = 0x2a;
-				new_cmd[1] = (cmd[1] & 0xe0);
-				new_cmd[2] = 0x0;
-				new_cmd[3] = (cmd[1] & 0x1f);
-				new_cmd[4] = cmd[2];
-				new_cmd[5] = cmd[3];
-				new_cmd[6] = 0x0;
-				new_cmd[7] = 0x0;
-				new_cmd[8] = cmd[4];
-				new_cmd[9] = cmd[5];
-
-				memcpy(cmd, new_cmd, 10);
-
-			}
-
-			break;
-
-		case MODE_SENSE:
-
-			if (sbp2_command_conversion_device_type(device_type)) {
-
-				SBP2_DEBUG("Convert MODE_SENSE_6 to MODE_SENSE_10");
-
-				/*
-				 * Need to turn mode_sense_6 into mode_sense_10
-				 */
-				new_cmd[0] = 0x5a;
-				new_cmd[1] = cmd[1];
-				new_cmd[2] = cmd[2];
-				new_cmd[3] = 0x0;
-				new_cmd[4] = 0x0;
-				new_cmd[5] = 0x0;
-				new_cmd[6] = 0x0;
-				new_cmd[7] = 0x0;
-				new_cmd[8] = cmd[4];
-				new_cmd[9] = cmd[5];
-
-				memcpy(cmd, new_cmd, 10);
-
-			}
-
-			break;
-
-		case MODE_SELECT:
-
-			/*
-			 * TODO. Probably need to change mode select to 10 byte version
-			 */
-
-		default:
-			break;
-	}
-
-	return;
 }
 
 /*
@@ -2272,14 +2176,6 @@
 			}
 
 			/*
-			 * Check for Simple Direct Access Device and change it to TYPE_DISK
-			 */
-			if ((scsi_buf[0] & 0x1f) == TYPE_SDAD) {
-				SBP2_DEBUG("Changing TYPE_SDAD to TYPE_DISK");
-				scsi_buf[0] &= 0xe0;
-			}
-
-			/*
 			 * Fix ansi revision and response data format
 			 */
 			scsi_buf[2] |= 2;
@@ -2287,27 +2183,6 @@
 
 			break;
 
-		case MODE_SENSE:
-
-			if (sbp2_command_conversion_device_type(device_type)) {
-
-				SBP2_DEBUG("Modify mode sense response (10 byte version)");
-
-				scsi_buf[0] = scsi_buf[1];	/* Mode data length */
-				scsi_buf[1] = scsi_buf[2];	/* Medium type */
-				scsi_buf[2] = scsi_buf[3];	/* Device specific parameter */
-				scsi_buf[3] = scsi_buf[7];	/* Block descriptor length */
-				memcpy(scsi_buf + 4, scsi_buf + 8, scsi_buf[0]);
-			}
-
-			break;
-
-		case MODE_SELECT:
-
-			/*
-			 * TODO. Probably need to change mode select to 10 byte version
-			 */
-
 		default:
 			break;
 	}
@@ -2690,7 +2565,8 @@
 static int sbp2scsi_slave_configure (struct scsi_device *sdev)
 {
 	blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
-
+	sdev->use_10_for_rw = 1;
+	sdev->use_10_for_ms = 1;
 	return 0;
 }
 
diff -urN RC12-rc4-base/drivers/ieee1394/sbp2.h RC12-rc4-rbc/drivers/ieee1394/sbp2.h
--- RC12-rc4-base/drivers/ieee1394/sbp2.h	2005-05-07 04:04:56.000000000 -0400
+++ RC12-rc4-rbc/drivers/ieee1394/sbp2.h	2005-05-15 12:17:39.716295613 -0400
@@ -266,10 +266,6 @@
 #define SBP2_MAX_UDS_PER_NODE		16	/* Maximum scsi devices per node */
 #define SBP2_MAX_SECTORS		255	/* Max sectors supported */
 
-#ifndef TYPE_SDAD
-#define TYPE_SDAD			0x0e	/* simplified direct access device */
-#endif
-
 /*
  * SCSI direction table...
  * (now used as a back-up in case the direction passed down from above is "unknown")
diff -urN RC12-rc4-base/drivers/scsi/scsi_scan.c RC12-rc4-rbc/drivers/scsi/scsi_scan.c
--- RC12-rc4-base/drivers/scsi/scsi_scan.c	2005-05-07 04:05:00.000000000 -0400
+++ RC12-rc4-rbc/drivers/scsi/scsi_scan.c	2005-05-15 12:17:39.717295419 -0400
@@ -625,6 +625,7 @@
 	case TYPE_MEDIUM_CHANGER:
 	case TYPE_ENCLOSURE:
 	case TYPE_COMM:
+	case TYPE_RBC:
 		sdev->writeable = 1;
 		break;
 	case TYPE_WORM:
diff -urN RC12-rc4-base/drivers/scsi/sd.c RC12-rc4-rbc/drivers/scsi/sd.c
--- RC12-rc4-base/drivers/scsi/sd.c	2005-05-07 04:05:00.000000000 -0400
+++ RC12-rc4-rbc/drivers/scsi/sd.c	2005-05-15 12:30:38.769387199 -0400
@@ -1368,17 +1368,26 @@
  */
 static void
 sd_read_cache_type(struct scsi_disk *sdkp, char *diskname,
-		   struct scsi_request *SRpnt, unsigned char *buffer) {
+		   struct scsi_request *SRpnt, unsigned char *buffer)
+{
 	int len = 0, res;
 
-	const int dbd = 0;	   /* DBD */
-	const int modepage = 0x08; /* current values, cache page */
+	int dbd;
+	int modepage;
 	struct scsi_mode_data data;
 	struct scsi_sense_hdr sshdr;
 
 	if (sdkp->device->skip_ms_page_8)
 		goto defaults;
 
+	if (sdkp->device->type == TYPE_RBC) {
+		modepage = 6;
+		dbd = 8;
+	} else {
+		modepage = 8;
+		dbd = 0;
+	}
+
 	/* cautiously ask */
 	res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer, 4, &data);
 
@@ -1409,11 +1418,20 @@
 			"write back, no read (daft)"
 		};
 		int ct = 0;
-		int offset = data.header_length +
-			data.block_descriptor_length + 2;
+		int offset = data.header_length + data.block_descriptor_length;
 
-		sdkp->WCE = ((buffer[offset] & 0x04) != 0);
-		sdkp->RCD = ((buffer[offset] & 0x01) != 0);
+		if ((buffer[offset] & 0x3f) != modepage) {
+			printk(KERN_ERR "%s: got wrong page\n", diskname);
+			goto defaults;
+		}
+
+		if (modepage == 8) {
+			sdkp->WCE = ((buffer[offset + 2] & 0x04) != 0);
+			sdkp->RCD = ((buffer[offset + 2] & 0x01) != 0);
+		} else {
+			sdkp->WCE = ((buffer[offset + 2] & 0x01) == 0);
+			sdkp->RCD = 0;
+		}
 
 		ct =  sdkp->RCD + 2*sdkp->WCE;
 
@@ -1533,7 +1551,7 @@
 	int error;
 
 	error = -ENODEV;
-	if ((sdp->type != TYPE_DISK) && (sdp->type != TYPE_MOD))
+	if (sdp->type != TYPE_DISK && sdp->type != TYPE_MOD && sdp->type != TYPE_RBC)
 		goto out;
 
 	SCSI_LOG_HLQUEUE(3, printk("sd_attach: scsi device: <%d,%d,%d,%d>\n", 
@@ -1570,7 +1588,7 @@
 	sdkp->openers = 0;
 
 	if (!sdp->timeout) {
-		if (sdp->type == TYPE_DISK)
+		if (sdp->type != TYPE_MOD)
 			sdp->timeout = SD_TIMEOUT;
 		else
 			sdp->timeout = SD_MOD_TIMEOUT;
diff -urN RC12-rc4-base/include/scsi/scsi.h RC12-rc4-rbc/include/scsi/scsi.h
--- RC12-rc4-base/include/scsi/scsi.h	2005-05-07 04:05:05.000000000 -0400
+++ RC12-rc4-rbc/include/scsi/scsi.h	2005-05-15 12:17:39.718295225 -0400
@@ -210,6 +210,7 @@
 #define TYPE_COMM           0x09    /* Communications device */
 #define TYPE_ENCLOSURE      0x0d    /* Enclosure Services Device */
 #define TYPE_RAID           0x0c
+#define TYPE_RBC	    0x0e
 #define TYPE_NO_LUN         0x7f
 
 /*

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Douglas Gilbert]

Al Viro wrote:
> 	a) TYPE_SDAD renamed to TYPE_RBC and taken to scsi.h
> 	b) in sbp2.c remapping of TYPE_RPB to TYPE_DISK turned off
> 	c) relevant places in midlayer and sd.c taught to accept TYPE_RBC
> 	d) sd.c::sd_read_cache_type() looks into page 6 when dealing with

Al,
Adding some information:

The name of mode page 6 is "RBC device parameters".
References for RBC:
http://www.t10.org/ftp/t10/drafts/rbc/rbc-r10a.pdf
http://www.t10.org/ftp/t10/drafts/rbc/rbc-a101.pdf

I just added mode page 6 to my sdparm beta.

> TYPE_RBC - these guys have writeback cache flag there and are not guaranteed
> to have page 8 at all.

The Write(back) Cache Disable (WCD) flag in mode page 6
is logically flipped from WCE in DASD (i.e. disk) caching
mode page (page number 8).

> 	e) sd_read_cache_type() got an extra sanity check - it checks that
> it got the page it asked for before using its contents.  And screams if
> mismatch had happened.  Rationale: there are broken devices out there that
> are "helpful" enough to go for "I don't have a page you've asked for, here,
> have another one".  For example, PL3507 had been caught doing just that...
> 	f) sbp2 sets sdev->use_10_for_rw and sdev->use_10_for_ms instead
> of bothering to remap READ6/WRITE6/MOD_SENSE, so most of the conversions
> in there are gone now.

RBC makes MODE_SENSE/SELECT (6) support mandatory but does
not support MODE_SENSE/SELECT(10).

> 	Incidentally, I wonder if USB storage devices that have no
> mode page 8 are simply RBC ones.  I haven't touched that, but it might
> be interesting to check...

I tried on a SanDisk SDDR-31 which doesn't support
any mode pages at all! Same with a SanDisk mini Cruzer.
My USB enclosure with a ATA disk inside is an
abomination wrt MODE SENSE. It doesn't respond to
MODE SENSE 6 but does to a MODE SENSE 10 but with
a mode parameter header for the 6 byte variant.
Any code that silently switches mode sense 6 to
10 (or vice versa) and doesn't fix the response is just
wrong.

Doug Gilbert

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Al Viro]

On Mon, May 16, 2005 at 01:26:54PM +1000, Douglas Gilbert wrote:
> >TYPE_RBC - these guys have writeback cache flag there and are not 
> >guaranteed
> >to have page 8 at all.
> 
> The Write(back) Cache Disable (WCD) flag in mode page 6
> is logically flipped from WCE in DASD (i.e. disk) caching
> mode page (page number 8).

Thus the
                if (modepage == 8) {
                        sdkp->WCE = ((buffer[offset + 2] & 0x04) != 0);
                        sdkp->RCD = ((buffer[offset + 2] & 0x01) != 0);
                } else {
                        sdkp->WCE = ((buffer[offset + 2] & 0x01) == 0);
                        sdkp->RCD = 0;
                }
in there - note the inverse check in the second case.

> >	f) sbp2 sets sdev->use_10_for_rw and sdev->use_10_for_ms instead
> >of bothering to remap READ6/WRITE6/MOD_SENSE, so most of the conversions
> >in there are gone now.
> 
> RBC makes MODE_SENSE/SELECT (6) support mandatory but does
> not support MODE_SENSE/SELECT(10).

I know.  However, behaviour of sbp2 before that patch was to remap
MODE_SENSE(6) to MODE_SENSE(10).  I'd rather not change that without
heavy testing - note that here we are talking about firewire devices
that talk sbp2 and happen to claim RBC as command set.  *IF* they
work with driver in Linus' tree, they will with ->use_10_for_...;
if/when we decide to change that behaviour, we'd better get ready for
random breakage.  IMO that's a separate story - let's keep the current
behaviour for now and just get rid of (badly broken) remapping in
favour of using flags.

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Douglas Gilbert]

Al Viro wrote:
> 	a) TYPE_SDAD renamed to TYPE_RBC and taken to scsi.h
> 	b) in sbp2.c remapping of TYPE_RPB to TYPE_DISK turned off
> 	c) relevant places in midlayer and sd.c taught to accept TYPE_RBC
> 	d) sd.c::sd_read_cache_type() looks into page 6 when dealing with
> TYPE_RBC - these guys have writeback cache flag there and are not guaranteed
> to have page 8 at all.
> 	e) sd_read_cache_type() got an extra sanity check - it checks that
> it got the page it asked for before using its contents.  And screams if
> mismatch had happened.  Rationale: there are broken devices out there that
> are "helpful" enough to go for "I don't have a page you've asked for, here,
> have another one".  For example, PL3507 had been caught doing just that...
> 	f) sbp2 sets sdev->use_10_for_rw and sdev->use_10_for_ms instead
> of bothering to remap READ6/WRITE6/MOD_SENSE, so most of the conversions
> in there are gone now.

Al,
I applied this patch and tested it with a Lava
Firewire-HC enclosure and a Seagate 7200.7 ATA disk.
The enclosure uses a OXFW900-TQ-A bridge chip.

It works ok, with a few wrinkles. MODE SENSE+SELECT
(6) work properly but MODE SENSE(10) responds with
the 6 byte variant response!? If nothing else it
was useful for testing sdparm and sg_modes coping
with such cases.

I will submit a patch for driver/scsi/scsi.c so it
stops saying the device type (in the output of
"cat /proc/scsi/scsi") is "unknown" for RBC devices.

Here is some output from sdparm (version 0.92):

# sdparm /dev/sdd -a
     /dev/sdd: ST380011  A                    [pdt=0xe]
RBC device parameters (RBC) mode page:
 >>> warning: mode page seems malformed, try '--flexible'
   WCD         0  [ def:  0, saved:  0]
   LBS       45310  [ def:45310, saved:45310]
   NLBS        0  [ def:  0, saved:  0]
   P_P         0  [ def:  0, saved:  0]
   READD       0  [ def:  0, saved:  0]
   WRITED      0  [ def:  0, saved:  0]
   FORMATD     0  [ def:  0, saved:  0]
   LOCKD       0  [ def:  0, saved:  0]

# sdparm /dev/sdd -a -f
     /dev/sdd: ST380011  A                    [pdt=0xe]
RBC device parameters (RBC) mode page:
   WCD         0  [ def:  0, saved:  0]
   LBS       512  [ def:512, saved:512]
   NLBS      156301488  [ def:156301488, saved:156301488]
   P_P       254  [ def:254, saved:254]
   READD       0  [ def:  0, saved:  0]
   WRITED      0  [ def:  0, saved:  0]
   FORMATD     0  [ def:  0, saved:  0]
   LOCKD       0  [ def:  0, saved:  0]

# sdparm /dev/sdd -a -6
     /dev/sdd: ST380011  A                    [pdt=0xe]
RBC device parameters (RBC) mode page:
   WCD         0  [ def:  0, saved:  0]
   LBS       512  [ def:512, saved:512]
   NLBS      156301488  [ def:156301488, saved:156301488]
   P_P       254  [ def:254, saved:254]
   READD       0  [ def:  0, saved:  0]
   WRITED      0  [ def:  0, saved:  0]
   FORMATD     0  [ def:  0, saved:  0]
   LOCKD       0  [ def:  0, saved:  0]

# sdparm /dev/sdd -s WCD -6
     /dev/sdd: ST380011  A                    [pdt=0xe]

# echo $?
0

# sdparm /dev/sdd -g WCD -6
WCD         1  [ def:  1, saved:  1]

# sdparm -i /dev/sdd
     /dev/sdb: ST380011  A                    [pdt=0xe]
Device identification VPD page:
   Addressed logical unit:
     id_type: EUI-64 based,  code_set: Binary
       [0x00043b000000071d]

I found that the WCD field and no others could
be changed with sdparm. When other fields were
changed there was no complaint from the device
but a subsequent get showed no change.

This experiment shows that if sd and the mid level rely
on the response from a MODE SENSE (10) without doing a
sanity check, then they will be using corrupted data.

Doug Gilbert

Re: TYPE_RBC cache fixes (sbp2.c affected)

[James Bottomley]

On Mon, 2005-05-16 at 02:59 +0100, Al Viro wrote:

>  static int sbp2scsi_slave_configure (struct scsi_device *sdev)
>  {
>  	blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
> -
> +	sdev->use_10_for_rw = 1;
> +	sdev->use_10_for_ms = 1;
>  	return 0;
>  }

This looks wrong.  The RBC standard only specifies the six byte commands
to be mandatory ... so shouldn't this be sdev->use_10_for_ms = 0?

James

Re: TYPE_RBC cache fixes (sbp2.c affected)

[James Bottomley]

Here's a tiny update that means we print the correct ASCII type
information

James

--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -111,6 +111,7 @@ const char *const scsi_device_types[MAX_
 	"Unknown          ",
 	"RAID             ",
 	"Enclosure        ",
+	"Direct-Access-RBC",
 };
 EXPORT_SYMBOL(scsi_device_types);
 
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -28,7 +28,7 @@ extern const unsigned char scsi_command_
  *	SCSI device types
  */
 
-#define MAX_SCSI_DEVICE_CODE 14
+#define MAX_SCSI_DEVICE_CODE 15
 extern const char *const scsi_device_types[MAX_SCSI_DEVICE_CODE];
 
 /*
@@ -211,8 +211,8 @@ static inline int scsi_status_is_good(in
 				     * - treated as TYPE_DISK */
 #define TYPE_MEDIUM_CHANGER 0x08
 #define TYPE_COMM           0x09    /* Communications device */
-#define TYPE_ENCLOSURE      0x0d    /* Enclosure Services Device */
 #define TYPE_RAID           0x0c
+#define TYPE_ENCLOSURE      0x0d    /* Enclosure Services Device */
 #define TYPE_RBC	    0x0e
 #define TYPE_NO_LUN         0x7f
 

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Jeff Garzik]

James Bottomley wrote:
> On Mon, 2005-05-16 at 02:59 +0100, Al Viro wrote:
> 
> 
>> static int sbp2scsi_slave_configure (struct scsi_device *sdev)
>> {
>> 	blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
>>-
>>+	sdev->use_10_for_rw = 1;
>>+	sdev->use_10_for_ms = 1;
>> 	return 0;
>> }
> 
> 
> This looks wrong.  The RBC standard only specifies the six byte commands
> to be mandatory ... so shouldn't this be sdev->use_10_for_ms = 0?

That's why its in sbp2-specific code...

The above code certainly applies to real-world cases, at least.  Just 
look at the code that was removed...  MS(10) handling.

	Jeff

Re: TYPE_RBC cache fixes (sbp2.c affected)

[James Bottomley]

On Sat, 2005-05-21 at 11:38 -0400, Jeff Garzik wrote:
> That's why its in sbp2-specific code...
> 
> The above code certainly applies to real-world cases, at least.  Just 
> look at the code that was removed...  MS(10) handling.

I know that; and I know it has the effect of the replaced code.
However, I don't understand why SBC2 feels entitled to ignore the RBC
standard here ... that standard was primarily created for SBC2 devices.

So was the code in sbc2 because non-RBC devices needed the 10 byte
command and RBC ones just happened not to reject it?  My sense here is
that we should have code in scsi_scan.c to set sdev->use_10_for_rw and
reset sdev->use_10_for_ms before the slave configure predicated on
TYPE_RBC.

Does anyone actually have one of these RBC devices and does it reject
the six byte mode sense commands?

James

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Al Viro]

On Sat, May 21, 2005 at 11:00:56AM -0500, James Bottomley wrote:
> On Sat, 2005-05-21 at 11:38 -0400, Jeff Garzik wrote:
> > That's why its in sbp2-specific code...
> > 
> > The above code certainly applies to real-world cases, at least.  Just 
> > look at the code that was removed...  MS(10) handling.
> 
> I know that; and I know it has the effect of the replaced code.
> However, I don't understand why SBC2 feels entitled to ignore the RBC
> standard here ... that standard was primarily created for SBC2 devices.

Tell that to firmware authors, why don't you?
 
> So was the code in sbc2 because non-RBC devices needed the 10 byte
> command and RBC ones just happened not to reject it?  My sense here is
> that we should have code in scsi_scan.c to set sdev->use_10_for_rw and
> reset sdev->use_10_for_ms before the slave configure predicated on
> TYPE_RBC.
> 
> Does anyone actually have one of these RBC devices and does it reject
> the six byte mode sense commands?

Yes, will check and do not expect the results to apply to other devices...

Re: TYPE_RBC cache fixes (sbp2.c affected)

[James Bottomley]

On Sat, 2005-05-21 at 17:22 +0100, Al Viro wrote:
> Tell that to firmware authors, why don't you?

I do ... but they don't listen ...
 
> > Does anyone actually have one of these RBC devices and does it reject
> > the six byte mode sense commands?
> 
> Yes, will check and do not expect the results to apply to other devices...

Thanks ... I'd be surprised if the entire class of RBC devices simply
ignored the standard; I wouldn't be surprised to find one or two that
are out of spec.

James

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Douglas Gilbert]

James Bottomley wrote:
> On Sat, 2005-05-21 at 17:22 +0100, Al Viro wrote:
> 
>>Tell that to firmware authors, why don't you?
> 
> 
> I do ... but they don't listen ...
>  
> 
>>>Does anyone actually have one of these RBC devices and does it reject
>>>the six byte mode sense commands?
>>
>>Yes, will check and do not expect the results to apply to other devices...
> 
> 
> Thanks ... I'd be surprised if the entire class of RBC devices simply
> ignored the standard; I wouldn't be surprised to find one or two that
> are out of spec.
> 
> James
> 
> 
> -
> 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

James,
Perhaps __scsi_mode_sense() could do a simple sanity
check: for any valid mode page [after the header and
block descriptor(s) are stepped over]:
      ((mpage[0] & 3f) == page_num)

If a response fails that test, we don't believe it.

Doug Gilbert

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Douglas Gilbert]

Douglas Gilbert wrote:
> James Bottomley wrote:
> 
>> On Sat, 2005-05-21 at 17:22 +0100, Al Viro wrote:
>>
>>> Tell that to firmware authors, why don't you?
>>
>>
>>
>> I do ... but they don't listen ...
>>  
>>
>>>> Does anyone actually have one of these RBC devices and does it reject
>>>> the six byte mode sense commands?
>>>
>>>
>>> Yes, will check and do not expect the results to apply to other 
>>> devices...
>>
>>
>>
>> Thanks ... I'd be surprised if the entire class of RBC devices simply
>> ignored the standard; I wouldn't be surprised to find one or two that
>> are out of spec.
>>
>> James
> 
> James,
> Perhaps __scsi_mode_sense() could do a simple sanity
> check: for any valid mode page [after the header and
> block descriptor(s) are stepped over]:
>      ((mpage[0] & 3f) == page_num)
> 
> If a response fails that test, we don't believe it.

A bit more accurate sanity check:
     if ((page_num > 0) && (page_num < 0x3f) &&
         ((mpage[0] & 3f) == page_num))
	// page looks ok (even if subpage_num was 0xff)
     else
	// nah

Mode page number 0 is the unit attention vendor specific
page which is not necessarily in "mode page format".
The vendor that I know does use it, does at least follow
mode page format.
A requested page_num of 0x3f and a requested subpage_num
of 0xff are wildcards.

Doug Gilbert



-------------------------------------------------------
This SF.Net email is sponsored by Oracle Space Sweepstakes
Want to be the first software developer in space?
Enter now for the Oracle Space Sweepstakes!
http://ads.osdn.com/?ad_id=7412&alloc_id=16344&op=click

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Douglas Gilbert]

Al + James,
Some comments about the original patch:

<snip>
> diff -urN RC12-rc4-base/drivers/scsi/sd.c RC12-rc4-rbc/drivers/scsi/sd.c
> --- RC12-rc4-base/drivers/scsi/sd.c	2005-05-07 04:05:00.000000000 -0400
> +++ RC12-rc4-rbc/drivers/scsi/sd.c	2005-05-15 12:30:38.769387199 -0400
> @@ -1368,17 +1368,26 @@
>   */
>  static void
>  sd_read_cache_type(struct scsi_disk *sdkp, char *diskname,
> -		   struct scsi_request *SRpnt, unsigned char *buffer) {
> +		   struct scsi_request *SRpnt, unsigned char *buffer)
> +{
>  	int len = 0, res;
>  
> -	const int dbd = 0;	   /* DBD */
> -	const int modepage = 0x08; /* current values, cache page */
> +	int dbd;
> +	int modepage;
>  	struct scsi_mode_data data;
>  	struct scsi_sense_hdr sshdr;
>  
>  	if (sdkp->device->skip_ms_page_8)
>  		goto defaults;
>  
> +	if (sdkp->device->type == TYPE_RBC) {
> +		modepage = 6;
> +		dbd = 8;

Al,
In my experience setting the DBD flag only increases the
chance of failure (from devices that don't understand the
DBD (i.e. disable block descriptors) bit. Also dbd should
be set (to 1) or cleared; not set to 8. Best to leave it clear
(the default) as the offset calculation below takes into
account any returned block descriptors.

James,
scsi_lib.c::__scsi_mode_sense() has a bug in it.
If dbd is set then both the DBD and LLBA bits in the
MODE SENSE cdb are set. However LLBA is not defined for
MODE SENSE 6 (in SPC or RBC). That may be why Al's
hardware doesn't like MODE SENSE 6 cdbs issued by the
SCSI mid level :-)

> +	} else {
> +		modepage = 8;
> +		dbd = 0;
> +	}
> +
>  	/* cautiously ask */
>  	res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer, 4, &data);
>  
> @@ -1409,11 +1418,20 @@
>  			"write back, no read (daft)"
>  		};
>  		int ct = 0;
> -		int offset = data.header_length +
> -			data.block_descriptor_length + 2;
> +		int offset = data.header_length + data.block_descriptor_length;
>  
> -		sdkp->WCE = ((buffer[offset] & 0x04) != 0);
> -		sdkp->RCD = ((buffer[offset] & 0x01) != 0);
> +		if ((buffer[offset] & 0x3f) != modepage) {
> +			printk(KERN_ERR "%s: got wrong page\n", diskname);
> +			goto defaults;
> +		}

So here is the sanity check that I have been talking
about. On my hardware since a MODE SENSE 10 was issued,
the response is corrupt (actually the response for the
corresponding MODE SENSE 6 is returned) so the exercise
becomes futile. Note that my hardware complies with
the RBC standard in properly supporting MODE SENSE 6.
[The RBC standard doesn't say anything about what should
happen when MODE SENSE 10 is issued :-)]

To work on my hardware the next move would be to
"sdev->use_10_for_ms = 0;" and try again (and if
that fails give up).

<snip>

Doug Gilbert

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Douglas Gilbert]

[-- Attachment #1: Type: text/plain, Size: 1391 bytes --]

James Bottomley wrote:
> Here's a tiny update that means we print the correct ASCII type
> information
> 
> James
> 
> --- a/drivers/scsi/scsi.c
> +++ b/drivers/scsi/scsi.c
> @@ -111,6 +111,7 @@ const char *const scsi_device_types[MAX_
>  	"Unknown          ",
>  	"RAID             ",
>  	"Enclosure        ",
> +	"Direct-Access-RBC",
>  };
>  EXPORT_SYMBOL(scsi_device_types);
>  
> --- a/include/scsi/scsi.h
> +++ b/include/scsi/scsi.h
> @@ -28,7 +28,7 @@ extern const unsigned char scsi_command_
>   *	SCSI device types
>   */
>  
> -#define MAX_SCSI_DEVICE_CODE 14
> +#define MAX_SCSI_DEVICE_CODE 15
>  extern const char *const scsi_device_types[MAX_SCSI_DEVICE_CODE];
>  
>  /*
> @@ -211,8 +211,8 @@ static inline int scsi_status_is_good(in
>  				     * - treated as TYPE_DISK */
>  #define TYPE_MEDIUM_CHANGER 0x08
>  #define TYPE_COMM           0x09    /* Communications device */
> -#define TYPE_ENCLOSURE      0x0d    /* Enclosure Services Device */
>  #define TYPE_RAID           0x0c
> +#define TYPE_ENCLOSURE      0x0d    /* Enclosure Services Device */
>  #define TYPE_RBC	    0x0e
>  #define TYPE_NO_LUN         0x7f
>  
> 
> 
> -

James,
Here is my take which goes a bit further.
One thing my patch does is change "CD-ROM"
to "CD/DVD" which might hurt any program
that parsers 'cat /proc/scsi/scsi". The
comments are taken from spc3r23 section 6.4.2
table 83.

Doug Gilbert

[-- Attachment #2: scsi_2612rc4i1.diff --]
[-- Type: text/x-patch, Size: 1899 bytes --]

--- linux/include/scsi/scsi.h	2005-05-21 13:08:06.000000000 +1000
+++ linux/include/scsi/scsi.h2612rc4i1	2005-05-22 17:51:50.000000000 +1000
@@ -28,7 +28,7 @@
  *	SCSI device types
  */
 
-#define MAX_SCSI_DEVICE_CODE 14
+#define MAX_SCSI_DEVICE_CODE 32
 extern const char *const scsi_device_types[MAX_SCSI_DEVICE_CODE];
 
 /*
--- linux/drivers/scsi/scsi.c	2005-05-08 15:12:20.000000000 +1000
+++ linux/drivers/scsi/scsi.c2612rc4i1	2005-05-22 17:50:24.000000000 +1000
@@ -97,20 +97,38 @@
 #endif
 
 const char *const scsi_device_types[MAX_SCSI_DEVICE_CODE] = {
-	"Direct-Access    ",
-	"Sequential-Access",
-	"Printer          ",
-	"Processor        ",
-	"WORM             ",
-	"CD-ROM           ",
-	"Scanner          ",
-	"Optical Device   ",
-	"Medium Changer   ",
-	"Communications   ",
-	"Unknown          ",
-	"Unknown          ",
-	"RAID             ",
-	"Enclosure        ",
+	"Direct-Access    ",	/* SBC-2 */
+	"Sequential-Access",	/* SSC-2 */
+	"Printer          ",	/* SSC */
+	"Processor        ",	/* SPC-2 */
+	"WORM             ",	/* SBC */
+	"CD/DVD           ",	/* MMC-4,5 */
+	"Scanner          ",	/* SCSI-2 */
+	"Optical Device   ",	/* SBC */
+	"Medium Changer   ",	/* SMC-2 */
+	"Communications   ",	/* SCSI-2 */
+	"Graphics(0xa)    ",
+	"Graphics(0xb)    ",
+	"RAID             ",	/* SCC-2 */
+	"Enclosure        ",	/* SES-2 */
+	"Simplified D-A   ",	/* RBC */
+	"Optical card rw  ",	/* OCRW */
+	"Bridge controller",	/* BCC */	/* 0x10 */
+	"Object storage   ",	/* OSD */
+	"Automation drv   ",	/* ADT */
+	"Unknown(0x13)    ",
+	"Unknown(0x14)    ",
+	"Unknown(0x15)    ",
+	"Unknown(0x16)    ",
+	"Unknown(0x17)    ",
+	"Unknown(0x18)    ",
+	"Unknown(0x19)    ",
+	"Unknown(0x1a)    ",
+	"Unknown(0x1b)    ",
+	"Unknown(0x1c)    ",
+	"Unknown(0x1e)    ",
+	"Well known lu    ",	/* SPC-3 */
+	"no dev on this lu",	/* SPC-3 */	/* 0x1f */
 };
 EXPORT_SYMBOL(scsi_device_types);
 

Re: TYPE_RBC cache fixes (sbp2.c affected)

[James Bottomley]

On Sun, 2005-05-22 at 16:31 +1000, Douglas Gilbert wrote:
> In my experience setting the DBD flag only increases the
> chance of failure (from devices that don't understand the
> DBD (i.e. disable block descriptors) bit. Also dbd should
> be set (to 1) or cleared; not set to 8. Best to leave it clear
> (the default) as the offset calculation below takes into
> account any returned block descriptors.

DBD is a listed *requirement* of RBC devices ... so I think we have to
have it.  Also, it's a pass through to __scsi_mode_sense() not a bit
flag (i.e. to set dbd in the command header, you have to set it to its
correct bit position, i.e. 8).

> James,
> scsi_lib.c::__scsi_mode_sense() has a bug in it.
> If dbd is set then both the DBD and LLBA bits in the
> MODE SENSE cdb are set. However LLBA is not defined for
> MODE SENSE 6 (in SPC or RBC). That may be why Al's
> hardware doesn't like MODE SENSE 6 cdbs issued by the
> SCSI mid level :-)

no, look again; the statement is:

	cmd[1] = dbd & 0x18;	/* allows DBD and LLBA bits */

So if you set dbd  0x08, you get dbd and 0x10 you get LLBA etc.

However, I agree, we shouldn't allow the setting of LLBA on MODE SENSE
6, fixed below.

> > +		if ((buffer[offset] & 0x3f) != modepage) {
> > +			printk(KERN_ERR "%s: got wrong page\n", diskname);
> > +			goto defaults;
> > +		}
> 
> So here is the sanity check that I have been talking
> about. On my hardware since a MODE SENSE 10 was issued,
> the response is corrupt (actually the response for the
> corresponding MODE SENSE 6 is returned) so the exercise
> becomes futile. Note that my hardware complies with
> the RBC standard in properly supporting MODE SENSE 6.
> [The RBC standard doesn't say anything about what should
> happen when MODE SENSE 10 is issued :-)]
> 
> To work on my hardware the next move would be to
> "sdev->use_10_for_ms = 0;" and try again (and if
> that fails give up).

Well ... what I was wondering is whether to predicate the setting of
use_10_for_ms in the firewire slave_configure on if (sdev->type !=
TYPE_RBC).

However, checking for corrupt mode pages in the routine seems like a
good idea as well, does the attached work?

James

--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1593,6 +1593,7 @@ __scsi_mode_sense(struct scsi_request *s
 			len = 4;
 
 		cmd[0] = MODE_SENSE;
+		cmd[1] &= 0x08; /* only DBD is legal */
 		cmd[4] = len;
 		header_length = 4;
 	}
@@ -1629,12 +1630,25 @@ __scsi_mode_sense(struct scsi_request *s
 	if(scsi_status_is_good(sreq->sr_result)) {
 		data->header_length = header_length;
 		if(use_10_for_ms) {
+			int actual_page;
+
 			data->length = buffer[0]*256 + buffer[1] + 2;
 			data->medium_type = buffer[2];
 			data->device_specific = buffer[3];
 			data->longlba = buffer[4] & 0x01;
 			data->block_descriptor_length = buffer[6]*256
 				+ buffer[7];
+
+			/* Sanity check the return: some devices give
+			 * rubbish back in response to ms(10) commands
+			 * but work with ms(6) */
+			actual_page = 
+				buffer[header_length +
+				       data->block_descriptor_length] & 0x3f;
+			if (actual_page != modepage) {
+				sreq->sr_device->use_10_for_ms = 0;
+				goto retry;
+			}
 		} else {
 			data->length = buffer[0] + 1;
 			data->medium_type = buffer[1];

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Douglas Gilbert]

James Bottomley wrote:
> On Sun, 2005-05-22 at 16:31 +1000, Douglas Gilbert wrote:
> 
>>In my experience setting the DBD flag only increases the
>>chance of failure (from devices that don't understand the
>>DBD (i.e. disable block descriptors) bit. Also dbd should
>>be set (to 1) or cleared; not set to 8. Best to leave it clear
>>(the default) as the offset calculation below takes into
>>account any returned block descriptors.
> 
> 
> DBD is a listed *requirement* of RBC devices ... so I think we have to
> have it.  Also, it's a pass through to __scsi_mode_sense() not a bit
> flag (i.e. to set dbd in the command header, you have to set it to its
> correct bit position, i.e. 8).

No wonder our colleagues in Redmond don't want a
bar of RBC and tell USB and 1394 driver writers
to convert MODE SENSE 10 from their OS as required.

Why doesn't the RBC standard leave the DBD switch as
it is in SPC and simply say that MODE SENSE responses
shall not contain block descriptors?? That would
be too simple.

Another strange thing I saw in RBC is the 5 byte
field containing the number of logical blocks field
in the RBC device parameters mode page. Trouble is
RBC only supports READ CAPACITY (10) which is limited
to 4 bytes for the number of logical blocks.

>>James,
>>scsi_lib.c::__scsi_mode_sense() has a bug in it.
>>If dbd is set then both the DBD and LLBA bits in the
>>MODE SENSE cdb are set. However LLBA is not defined for
>>MODE SENSE 6 (in SPC or RBC). That may be why Al's
>>hardware doesn't like MODE SENSE 6 cdbs issued by the
>>SCSI mid level :-)
> 
> 
> no, look again; the statement is:
> 
> 	cmd[1] = dbd & 0x18;	/* allows DBD and LLBA bits */

Coffee didn't make up for that 4 hours of sleep
I lost to that travesty in Wales, but I digress ...

> So if you set dbd  0x08, you get dbd and 0x10 you get LLBA etc.

I feel ill.

> However, I agree, we shouldn't allow the setting of LLBA on MODE SENSE
> 6, fixed below.

Now I feel better.

For my hardware it makes no difference whether DBD
is set or not (MODE SENSE/SELECT 6 works while MODE
SENSE 10 returns a MODE SENSE 6 response).

>>>+		if ((buffer[offset] & 0x3f) != modepage) {
>>>+			printk(KERN_ERR "%s: got wrong page\n", diskname);
>>>+			goto defaults;
>>>+		}
>>
>>So here is the sanity check that I have been talking
>>about. On my hardware since a MODE SENSE 10 was issued,
>>the response is corrupt (actually the response for the
>>corresponding MODE SENSE 6 is returned) so the exercise
>>becomes futile. Note that my hardware complies with
>>the RBC standard in properly supporting MODE SENSE 6.
>>[The RBC standard doesn't say anything about what should
>>happen when MODE SENSE 10 is issued :-)]
>>
>>To work on my hardware the next move would be to
>>"sdev->use_10_for_ms = 0;" and try again (and if
>>that fails give up).
> 
> 
> Well ... what I was wondering is whether to predicate the setting of
> use_10_for_ms in the firewire slave_configure on if (sdev->type !=
> TYPE_RBC).
> 
> However, checking for corrupt mode pages in the routine seems like a
> good idea as well, does the attached work?

Yes.

This is what I saw (with WCD=0):

May 23 10:40:21 frig kernel:     <<< prior to patch >>>
sbp2: $Rev: 1219 $ Ben Collins <bcollins@debian.org>
scsi2 : SCSI emulation for IEEE-1394 SBP-2 Devices
ieee1394: sbp2: Logged into SBP-2 device
   Vendor: ST380011  Model: A                 Rev:
   Type:   Simplified D-A                     ANSI SCSI revision: 06
SCSI device sdb: 156301488 512-byte hdwr sectors (80026 MB)
sdb: got wrong page
sdb: assuming drive cache: write through
SCSI device sdb: 156301488 512-byte hdwr sectors (80026 MB)
sdb: got wrong page
sdb: assuming drive cache: write through
  sdb: sdb1 sdb2 sdb3 sdb4
Attached scsi disk sdb at scsi2, channel 0, id 1, lun 0


May 23 18:00:09 frig kernel:     <<< after patch >>>
sbp2: $Rev: 1219 $ Ben Collins <bcollins@debian.org>
scsi2 : SCSI emulation for IEEE-1394 SBP-2 Devices
ieee1394: sbp2: Logged into SBP-2 device
   Vendor: ST380011  Model: A                 Rev:
   Type:   Simplified D-A                     ANSI SCSI revision: 06
SCSI device sdb: 156301488 512-byte hdwr sectors (80026 MB)
SCSI device sdb: drive cache: write back
SCSI device sdb: 156301488 512-byte hdwr sectors (80026 MB)
SCSI device sdb: drive cache: write back
  sdb: sdb1 sdb2 sdb3 sdb4
Attached scsi disk sdb at scsi2, channel 0, id 1, lun 0


Doug Gilbert

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Stefan Richter]

Al Viro wrote:
> 	a) TYPE_SDAD renamed to TYPE_RBC and taken to scsi.h
> 	b) in sbp2.c remapping of TYPE_RPB to TYPE_DISK turned off
> 	c) relevant places in midlayer and sd.c taught to accept TYPE_RBC
> 	d) sd.c::sd_read_cache_type() looks into page 6 when dealing with
> TYPE_RBC - these guys have writeback cache flag there and are not guaranteed
> to have page 8 at all.
> 	e) sd_read_cache_type() got an extra sanity check - it checks that
> it got the page it asked for before using its contents.  And screams if
> mismatch had happened.  Rationale: there are broken devices out there that
> are "helpful" enough to go for "I don't have a page you've asked for, here,
> have another one".  For example, PL3507 had been caught doing just that...
> 	f) sbp2 sets sdev->use_10_for_rw and sdev->use_10_for_ms instead
> of bothering to remap READ6/WRITE6/MOD_SENSE, so most of the conversions
> in there are gone now.
> 
> 	Incidentally, I wonder if USB storage devices that have no
> mode page 8 are simply RBC ones.  I haven't touched that, but it might
> be interesting to check...
> 	
> Signed-off-by: Al Viro <viro@parcelfarce.linux.theplanet.co.uk>
> ----

While I was testing Al Viro's sbp2 patch from today, I also tested the 
one disk which was broken by this "TYPE_RBC cache fixes" patch back in 
mid 2005. (Release history: The TYPE_RBC patch went into Linus' tree but 
we reverted the sbp2 part of the patch shortly thereafter. The sbp2 part 
was finally merged this January in linux-2.6.16-rc1.)

The device whose support was broken by the patch is a noname (actually 
AVLAB) 2.5" 1394b enclosure based on Initio INIC-2430F, clad in black 
aluminium. http://www.linux1394.org/view_device.php?id=917

On top of an early 2.6.13-rc kernel, this patch caused Linux to *reboot* 
immediately after the disk was attached.

Curiously enough, the device worked again with this patch on top of 
2.6.14.x.

Now I tested the disk again the first time after I moved on to 2.6.15.x 
(again with the "TYPE_RBC cache fixes" patch, which is not available in 
2.6.15 but has been merged in 2.6.16-rc1). Again, the machine 
immediately rebooted when the disk was attached. Like I reported in 
July, the problem seems to arise from the disk's response to 
MODE_SENSE_10. http://marc.theaimsgroup.com/?l=linux-scsi&m=112128914912105

Could the fact that Linux reboots (if sbp2 does not mangle the SCSI 
commands) mean that the SBP-2 target is overwriting memory outside of a 
data buffer? Or does the SCSI stack perform reckless things like jumps 
based on pointer tables, using unchecked data? Or...?

Vanilla 2.6.15.x works with this device. I did not boot into 2.6.16-rcX yet.


> diff -urN RC12-rc4-base/drivers/ieee1394/sbp2.c RC12-rc4-rbc/drivers/ieee1394/sbp2.c
> --- RC12-rc4-base/drivers/ieee1394/sbp2.c	2005-05-07 04:04:56.000000000 -0400
> +++ RC12-rc4-rbc/drivers/ieee1394/sbp2.c	2005-05-15 12:17:39.716295613 -0400
> @@ -1070,7 +1070,7 @@
>  static __inline__ int sbp2_command_conversion_device_type(u8 device_type)
>  {
>  	return (((device_type == TYPE_DISK) ||
> -		 (device_type == TYPE_SDAD) ||
> +		 (device_type == TYPE_RBC) ||
>  		 (device_type == TYPE_ROM)) ? 1:0);
>  }
>  
> @@ -2111,102 +2111,6 @@
>   */
>  static void sbp2_check_sbp2_command(struct scsi_id_instance_data *scsi_id, unchar *cmd)
>  {
> -	unchar new_cmd[16];
> -	u8 device_type = SBP2_DEVICE_TYPE (scsi_id->sbp2_device_type_and_lun);
> -
> -	SBP2_DEBUG("sbp2_check_sbp2_command");
> -
> -	switch (*cmd) {
> -
> -		case READ_6:
> -
> -			if (sbp2_command_conversion_device_type(device_type)) {
> -
> -				SBP2_DEBUG("Convert READ_6 to READ_10");
> -
> -				/*
> -				 * Need to turn read_6 into read_10
> -				 */
> -				new_cmd[0] = 0x28;
> -				new_cmd[1] = (cmd[1] & 0xe0);
> -				new_cmd[2] = 0x0;
> -				new_cmd[3] = (cmd[1] & 0x1f);
> -				new_cmd[4] = cmd[2];
> -				new_cmd[5] = cmd[3];
> -				new_cmd[6] = 0x0;
> -				new_cmd[7] = 0x0;
> -				new_cmd[8] = cmd[4];
> -				new_cmd[9] = cmd[5];
> -
> -				memcpy(cmd, new_cmd, 10);
> -
> -			}
> -
> -			break;
> -
> -		case WRITE_6:
> -
> -			if (sbp2_command_conversion_device_type(device_type)) {
> -
> -				SBP2_DEBUG("Convert WRITE_6 to WRITE_10");
> -
> -				/*
> -				 * Need to turn write_6 into write_10
> -				 */
> -				new_cmd[0] = 0x2a;
> -				new_cmd[1] = (cmd[1] & 0xe0);
> -				new_cmd[2] = 0x0;
> -				new_cmd[3] = (cmd[1] & 0x1f);
> -				new_cmd[4] = cmd[2];
> -				new_cmd[5] = cmd[3];
> -				new_cmd[6] = 0x0;
> -				new_cmd[7] = 0x0;
> -				new_cmd[8] = cmd[4];
> -				new_cmd[9] = cmd[5];
> -
> -				memcpy(cmd, new_cmd, 10);
> -
> -			}
> -
> -			break;
> -
> -		case MODE_SENSE:
> -
> -			if (sbp2_command_conversion_device_type(device_type)) {
> -
> -				SBP2_DEBUG("Convert MODE_SENSE_6 to MODE_SENSE_10");
> -
> -				/*
> -				 * Need to turn mode_sense_6 into mode_sense_10
> -				 */
> -				new_cmd[0] = 0x5a;
> -				new_cmd[1] = cmd[1];
> -				new_cmd[2] = cmd[2];
> -				new_cmd[3] = 0x0;
> -				new_cmd[4] = 0x0;
> -				new_cmd[5] = 0x0;
> -				new_cmd[6] = 0x0;
> -				new_cmd[7] = 0x0;
> -				new_cmd[8] = cmd[4];
> -				new_cmd[9] = cmd[5];
> -
> -				memcpy(cmd, new_cmd, 10);
> -
> -			}
> -
> -			break;
> -
> -		case MODE_SELECT:
> -
> -			/*
> -			 * TODO. Probably need to change mode select to 10 byte version
> -			 */
> -
> -		default:
> -			break;
> -	}
> -
> -	return;
>  }
>  
>  /*
> @@ -2272,14 +2176,6 @@
>  			}
>  
>  			/*
> -			 * Check for Simple Direct Access Device and change it to TYPE_DISK
> -			 */
> -			if ((scsi_buf[0] & 0x1f) == TYPE_SDAD) {
> -				SBP2_DEBUG("Changing TYPE_SDAD to TYPE_DISK");
> -				scsi_buf[0] &= 0xe0;
> -			}
> -
> -			/*
>  			 * Fix ansi revision and response data format
>  			 */
>  			scsi_buf[2] |= 2;
> @@ -2287,27 +2183,6 @@
>  
>  			break;
>  
> -		case MODE_SENSE:
> -
> -			if (sbp2_command_conversion_device_type(device_type)) {
> -
> -				SBP2_DEBUG("Modify mode sense response (10 byte version)");
> -
> -				scsi_buf[0] = scsi_buf[1];	/* Mode data length */
> -				scsi_buf[1] = scsi_buf[2];	/* Medium type */
> -				scsi_buf[2] = scsi_buf[3];	/* Device specific parameter */
> -				scsi_buf[3] = scsi_buf[7];	/* Block descriptor length */
> -				memcpy(scsi_buf + 4, scsi_buf + 8, scsi_buf[0]);
> -			}
> -
> -			break;
> -
> -		case MODE_SELECT:
> -
> -			/*
> -			 * TODO. Probably need to change mode select to 10 byte version
> -			 */
> -
>  		default:
>  			break;
>  	}
> @@ -2690,7 +2565,8 @@
>  static int sbp2scsi_slave_configure (struct scsi_device *sdev)
>  {
>  	blk_queue_dma_alignment(sdev->request_queue, (512 - 1));
> -
> +	sdev->use_10_for_rw = 1;
> +	sdev->use_10_for_ms = 1;
>  	return 0;
>  }
>  
> diff -urN RC12-rc4-base/drivers/ieee1394/sbp2.h RC12-rc4-rbc/drivers/ieee1394/sbp2.h
> --- RC12-rc4-base/drivers/ieee1394/sbp2.h	2005-05-07 04:04:56.000000000 -0400
> +++ RC12-rc4-rbc/drivers/ieee1394/sbp2.h	2005-05-15 12:17:39.716295613 -0400
> @@ -266,10 +266,6 @@
>  #define SBP2_MAX_UDS_PER_NODE		16	/* Maximum scsi devices per node */
>  #define SBP2_MAX_SECTORS		255	/* Max sectors supported */
>  
> -#ifndef TYPE_SDAD
> -#define TYPE_SDAD			0x0e	/* simplified direct access device */
> -#endif
> -
>  /*
>   * SCSI direction table...
>   * (now used as a back-up in case the direction passed down from above is "unknown")
> diff -urN RC12-rc4-base/drivers/scsi/scsi_scan.c RC12-rc4-rbc/drivers/scsi/scsi_scan.c
> --- RC12-rc4-base/drivers/scsi/scsi_scan.c	2005-05-07 04:05:00.000000000 -0400
> +++ RC12-rc4-rbc/drivers/scsi/scsi_scan.c	2005-05-15 12:17:39.717295419 -0400
> @@ -625,6 +625,7 @@
>  	case TYPE_MEDIUM_CHANGER:
>  	case TYPE_ENCLOSURE:
>  	case TYPE_COMM:
> +	case TYPE_RBC:
>  		sdev->writeable = 1;
>  		break;
>  	case TYPE_WORM:
> diff -urN RC12-rc4-base/drivers/scsi/sd.c RC12-rc4-rbc/drivers/scsi/sd.c
> --- RC12-rc4-base/drivers/scsi/sd.c	2005-05-07 04:05:00.000000000 -0400
> +++ RC12-rc4-rbc/drivers/scsi/sd.c	2005-05-15 12:30:38.769387199 -0400
> @@ -1368,17 +1368,26 @@
>   */
>  static void
>  sd_read_cache_type(struct scsi_disk *sdkp, char *diskname,
> -		   struct scsi_request *SRpnt, unsigned char *buffer) {
> +		   struct scsi_request *SRpnt, unsigned char *buffer)
> +{
>  	int len = 0, res;
>  
> -	const int dbd = 0;	   /* DBD */
> -	const int modepage = 0x08; /* current values, cache page */
> +	int dbd;
> +	int modepage;
>  	struct scsi_mode_data data;
>  	struct scsi_sense_hdr sshdr;
>  
>  	if (sdkp->device->skip_ms_page_8)
>  		goto defaults;
>  
> +	if (sdkp->device->type == TYPE_RBC) {
> +		modepage = 6;
> +		dbd = 8;
> +	} else {
> +		modepage = 8;
> +		dbd = 0;
> +	}
> +
>  	/* cautiously ask */
>  	res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer, 4, &data);
>  
> @@ -1409,11 +1418,20 @@
>  			"write back, no read (daft)"
>  		};
>  		int ct = 0;
> -		int offset = data.header_length +
> -			data.block_descriptor_length + 2;
> +		int offset = data.header_length + data.block_descriptor_length;
>  
> -		sdkp->WCE = ((buffer[offset] & 0x04) != 0);
> -		sdkp->RCD = ((buffer[offset] & 0x01) != 0);
> +		if ((buffer[offset] & 0x3f) != modepage) {
> +			printk(KERN_ERR "%s: got wrong page\n", diskname);
> +			goto defaults;
> +		}
> +
> +		if (modepage == 8) {
> +			sdkp->WCE = ((buffer[offset + 2] & 0x04) != 0);
> +			sdkp->RCD = ((buffer[offset + 2] & 0x01) != 0);
> +		} else {
> +			sdkp->WCE = ((buffer[offset + 2] & 0x01) == 0);
> +			sdkp->RCD = 0;
> +		}
>  
>  		ct =  sdkp->RCD + 2*sdkp->WCE;
>  
> @@ -1533,7 +1551,7 @@
>  	int error;
>  
>  	error = -ENODEV;
> -	if ((sdp->type != TYPE_DISK) && (sdp->type != TYPE_MOD))
> +	if (sdp->type != TYPE_DISK && sdp->type != TYPE_MOD && sdp->type != TYPE_RBC)
>  		goto out;
>  
>  	SCSI_LOG_HLQUEUE(3, printk("sd_attach: scsi device: <%d,%d,%d,%d>\n", 
> @@ -1570,7 +1588,7 @@
>  	sdkp->openers = 0;
>  
>  	if (!sdp->timeout) {
> -		if (sdp->type == TYPE_DISK)
> +		if (sdp->type != TYPE_MOD)
>  			sdp->timeout = SD_TIMEOUT;
>  		else
>  			sdp->timeout = SD_MOD_TIMEOUT;
> diff -urN RC12-rc4-base/include/scsi/scsi.h RC12-rc4-rbc/include/scsi/scsi.h
> --- RC12-rc4-base/include/scsi/scsi.h	2005-05-07 04:05:05.000000000 -0400
> +++ RC12-rc4-rbc/include/scsi/scsi.h	2005-05-15 12:17:39.718295225 -0400
> @@ -210,6 +210,7 @@
>  #define TYPE_COMM           0x09    /* Communications device */
>  #define TYPE_ENCLOSURE      0x0d    /* Enclosure Services Device */
>  #define TYPE_RAID           0x0c
> +#define TYPE_RBC	    0x0e
>  #define TYPE_NO_LUN         0x7f
>  
>  /*
> 
> 
> -------------------------------------------------------
> This SF.Net email is sponsored by Oracle Space Sweepstakes
> Want to be the first software developer in space?
> Enter now for the Oracle Space Sweepstakes!
> http://ads.osdn.com/?ad_id=7412&alloc_id=16344&op=click
> _______________________________________________
> mailing list linux1394-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux1394-devel


-- 
Stefan Richter
-=====-=-==- --=- -=--=
http://arcgraph.de/sr/

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Al Viro]

On Thu, Feb 09, 2006 at 12:39:20AM +0100, Stefan Richter wrote:
> Could the fact that Linux reboots (if sbp2 does not mangle the SCSI 
> commands) mean that the SBP-2 target is overwriting memory outside of a 
> data buffer? Or does the SCSI stack perform reckless things like jumps 
> based on pointer tables, using unchecked data? Or...?

Interesting...  What's the last command sent before reboot?  Note that
original driver would remap 6byte commands to 10byte ones, but new one
should not _get_ those commands.  At all.  What happens if you take old
driver, put
        sdev->use_10_for_rw = 1;
        sdev->use_10_for_ms = 1;
into sbp2scsi_slave_configure() and leave remapping code alone?  Then
see if remapper is ever triggered - if it does, we have a problem in
midlayer.  If not...  I'd love to see the last commands.

BTW, I've seen PL3507-based enclosure <spit> with the following lovely
bug: if it _ever_ got INQUIRY (any INQUIRY) other than immediately in
the beginning of session, it got 8 bytes stuck in FIFO.  All subsequeunt
reads got shifted by 8 bytes, no matter what.  With old driver, new driver...
Just scsiinfo -i would be enough to screw it.  With massive fs corruption,
obviously...

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Stefan Richter]

Al Viro wrote on 2006-02-09:
> On Thu, Feb 09, 2006 at 12:39:20AM +0100, Stefan Richter wrote:
>>Could the fact that Linux reboots (if sbp2 does not mangle the SCSI 
>>commands) mean that the SBP-2 target is overwriting memory outside of a 
>>data buffer? Or does the SCSI stack perform reckless things like jumps 
>>based on pointer tables, using unchecked data? Or...?
> 
> Interesting...  What's the last command sent before reboot?  Note that
> original driver would remap 6byte commands to 10byte ones, but new one
> should not _get_ those commands.  At all.  What happens if you take old
> driver, put
>         sdev->use_10_for_rw = 1;
>         sdev->use_10_for_ms = 1;
> into sbp2scsi_slave_configure() and leave remapping code alone?  Then
> see if remapper is ever triggered - if it does, we have a problem in
> midlayer.  If not...  I'd love to see the last commands.

Yes, I will do so as soon I got spare time.
MODE_SENSE_10 seemed to be the trigger, as mentioned in 
http://marc.theaimsgroup.com/?l=linux-scsi&m=112128914912105 . Also note 
that this device reports to implement Direct-Access, unlike most other 
SBP-2 HDDs which pose as Direct-Access-RBC.

I have one other bridge which reports type Direct-Access. It is based on 
the first generation fon TI StorageLynx. This does not work under Linux 
either because it requires the 36 byte inquiry workaround. Sbp2's 
version has become ineffective in recent kernels and I have not yet 
figured out how to enable the SCSI layer's version of this workaround. 
(But that's another issue.)

> BTW, I've seen PL3507-based enclosure <spit> with the following lovely
> bug: if it _ever_ got INQUIRY (any INQUIRY) other than immediately in
> the beginning of session, it got 8 bytes stuck in FIFO.  All subsequeunt
> reads got shifted by 8 bytes, no matter what.  With old driver, new driver...
> Just scsiinfo -i would be enough to screw it.  With massive fs corruption,
> obviously...

Interesting. Many PL3507 come with buggy firmware which is noticed under 
other OSs too. Newer hardware revisions can be reprogrammed but I don't 
know where the latest firmware is available and whether it fixes this 
INQUIRY related bug.

Do you think a workaround (like perhaps "reject all INQUIRY commands 
except the first one after login") would be justified?
-- 
Stefan Richter
-=====-=-==- --=- -=-==
http://arcgraph.de/sr/

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Al Viro]

On Sat, Feb 11, 2006 at 10:50:23AM +0100, Stefan Richter wrote:
> Interesting. Many PL3507 come with buggy firmware which is noticed under 
> other OSs too. Newer hardware revisions can be reprogrammed but I don't 
> know where the latest firmware is available and whether it fixes this 
> INQUIRY related bug.

Newer hardware revisions can be reprogrammed, but it's Windows-only and
price of OXFW911-based enclosure was lower than that of Windows install
media or a new box that would have it preinstalled.  If somebody in nc.us
wants to experiment with that junk, they are welcome to it; I don't have
Windows boxen and have better things to spend time and money on...

> Do you think a workaround (like perhaps "reject all INQUIRY commands 
> except the first one after login") would be justified?

Not really...  Blacklisting that FPOS and recommending to replace it
with real bridge is saner solution, IMO.

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Stefan Richter]

I wrote:
[panic when attaching a INIC-2430F based 1394b SBP-2 disk]
> MODE_SENSE_10 seemed to be the trigger, as mentioned in 
> http://marc.theaimsgroup.com/?l=linux-scsi&m=112128914912105 .

The BLIST_MS_SKIP_PAGE_08 (skip_ms_page_8) flag avoids the panic.
I will continue to investigate...
-- 
Stefan Richter
-=====-=-==- --=- -==-=
http://arcgraph.de/sr/

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Al Viro]

On Sat, Feb 11, 2006 at 10:50:23AM +0100, Stefan Richter wrote:
> Yes, I will do so as soon I got spare time.
> MODE_SENSE_10 seemed to be the trigger, as mentioned in 
> http://marc.theaimsgroup.com/?l=linux-scsi&m=112128914912105 . Also note 
> that this device reports to implement Direct-Access, unlike most other 
> SBP-2 HDDs which pose as Direct-Access-RBC.

OK, I think I've seen one that does it.  Behold the lossage:

	* bugger does, indeed, report itself to be type 0
	* OK, says sd_read_cache_type().  Page 8 for you, then.
	* so called "SCSI device" spits out...
<drumroll> RBC page 6.  Sans mode page headers.

So we see 0x86 0x0b where the data length should've been.  And bytes 3 and 2
of device size where the block descriptors size should've been.  Since sd.c
doesn't expect that level of idiocy (it should, but...) we adjust length
down from ~34000 to 20 _and_ blindly add block descriptors size.  Or what
we assume to be one.

Then we proceed to call scsi_mode_sense() with buffer created by kmalloc(512,
GFP_DMA) and len... well, anywhere up to 64Kb.   One of the first things it
does is memset(buffer, 0, len).


That's an Initio bridge, BTW.  I suspect that the best we can do is to
blacklist the little shit with "don't trust that one, it's really type 14".
If it reacts to request for page 6 in a saner fashion, that is...


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Stefan Richter]

Al Viro wrote:
> OK, I think I've seen one that does it.  Behold the lossage:
> 
> 	* bugger does, indeed, report itself to be type 0
> 	* OK, says sd_read_cache_type().  Page 8 for you, then.
> 	* so called "SCSI device" spits out...
> <drumroll> RBC page 6.  Sans mode page headers.
> 
> So we see 0x86 0x0b where the data length should've been.  And bytes 3 and 2
> of device size where the block descriptors size should've been.  Since sd.c
> doesn't expect that level of idiocy (it should, but...) we adjust length
> down from ~34000 to 20 _and_ blindly add block descriptors size.  Or what
> we assume to be one.
> 
> Then we proceed to call scsi_mode_sense() with buffer created by kmalloc(512,
> GFP_DMA) and len... well, anywhere up to 64Kb.   One of the first things it
> does is memset(buffer, 0, len).

Thanks a lot for the investigation & explanation.

> That's an Initio bridge, BTW.  I suspect that the best we can do is to
> blacklist the little shit with "don't trust that one, it's really type 14".
> If it reacts to request for page 6 in a saner fashion, that is...

2.6.16-rc4-mm1's sbp2 already detects most of the Initio bridges and 
flags them with skip_ms_page_8. It does so based on the 
firmware_revision config ROM entry. This seems to catch most Initio 
bridges but there was at least one report of a different 
firmware_revision key value. However all of them feature "Initio" as 
vendor string. I suppose I should let sbp2_slave_configure check for 
this string in sdev->vendor and for sdev->type == TYPE_DISK, then bend 
sdev->type to TYPE_RBC. (Or set skip_ms_page_8; I will check how my disk 
behaves when forced to TYPE_RBC...)
-- 
Stefan Richter
-=====-=-==- --=- =-=-=
http://arcgraph.de/sr/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Al Viro]

On Tue, Feb 21, 2006 at 08:56:08PM +0100, Stefan Richter wrote:
> 2.6.16-rc4-mm1's sbp2 already detects most of the Initio bridges and 
> flags them with skip_ms_page_8. It does so based on the 
> firmware_revision config ROM entry. This seems to catch most Initio 
> bridges but there was at least one report of a different 
> firmware_revision key value. However all of them feature "Initio" as 
> vendor string. I suppose I should let sbp2_slave_configure check for 
> this string in sdev->vendor and for sdev->type == TYPE_DISK, then bend 
> sdev->type to TYPE_RBC. (Or set skip_ms_page_8; I will check how my disk 
> behaves when forced to TYPE_RBC...)

Same - it still forgets to generate proper header.

See if that helps (might allow killing that skip_ms_page_8 in those, BTW):

--- a/drivers/scsi/scsi_lib.c	2006-02-20 10:02:58.000000000 -0600
+++ b/drivers/scsi/scsi_lib.c	2006-02-21 01:47:18.000000000 -0600
@@ -1892,8 +1892,16 @@
 	}
 
 	if(scsi_status_is_good(result)) {
-		data->header_length = header_length;
-		if(use_10_for_ms) {
+		if (unlikely(buffer[0] == 0x86 && buffer[1] == 0x0b &&
+			     (modepage == 6 || modepage == 8))) {
+			/* Initio breakage? */
+			header_length = 0;
+			data->length = 13;
+			data->medium_type = 0;
+			data->device_specific = 0;
+			data->longlba = 0;
+			data->block_descriptor_length = 0;
+		} else if(use_10_for_ms) {
 			data->length = buffer[0]*256 + buffer[1] + 2;
 			data->medium_type = buffer[2];
 			data->device_specific = buffer[3];
@@ -1906,6 +1914,7 @@
 			data->device_specific = buffer[2];
 			data->block_descriptor_length = buffer[3];
 		}
+		data->header_length = header_length;
 	}
 
 	return result;
--- a/drivers/scsi/sd.c	2006-02-17 16:26:52.000000000 -0600
+++ b/drivers/scsi/sd.c	2006-02-20 18:15:44.000000000 -0600
@@ -1328,6 +1328,12 @@
 	if (!scsi_status_is_good(res))
 		goto bad_sense;
 
+	if (!data.header_length) {
+		modepage = 6;
+		printk(KERN_ERR "%s: missing header in MODE_SENSE response\n",
+		       diskname);
+	}
+
 	/* that went OK, now ask for the proper length */
 	len = data.length;
 
@@ -1342,6 +1348,8 @@
 
 	/* Take headers and block descriptors into account */
 	len += data.header_length + data.block_descriptor_length;
+	if (len > 512)
+		goto bad_sense;
 
 	/* Get the data */
 	res = sd_do_mode_sense(sdp, dbd, modepage, buffer, len, &data, &sshdr);
@@ -1354,8 +1362,15 @@
 		int ct = 0;
 		int offset = data.header_length + data.block_descriptor_length;
 
+		if (offset >= 512 - 2) {
+			printk(KERN_ERR "%s: malformed MODE SENSE response",
+				diskname);
+			goto defaults;
+		}
+
 		if ((buffer[offset] & 0x3f) != modepage) {
-			printk(KERN_ERR "%s: got wrong page\n", diskname);
+			printk(KERN_ERR "%s: got wrong page (%d -> %d)\n",
+				 diskname, modepage, buffer[offset] & 0x3f);
 			goto defaults;
 		}
 

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Stefan Richter]

Al Viro wrote:
> On Tue, Feb 21, 2006 at 08:56:08PM +0100, Stefan Richter wrote:
>>I suppose I should let sbp2_slave_configure check for 
>>this string in sdev->vendor and for sdev->type == TYPE_DISK, then bend 
>>sdev->type to TYPE_RBC. (Or set skip_ms_page_8; I will check how my disk 
>>behaves when forced to TYPE_RBC...)
> 
> 
> Same - it still forgets to generate proper header.
> 
> See if that helps (might allow killing that skip_ms_page_8 in those, BTW):

Yes, this works for me so far.

However someone pointed out to me that he still got memory corruption 
when running the stock driver with the skip_ms_page_8 workaround in 
sbp2, i.e. when sd_read_cache_type skips mode_sense altogether. I will 
proceed to test with DEBUG_SLAB.

> --- a/drivers/scsi/scsi_lib.c	2006-02-20 10:02:58.000000000 -0600
> +++ b/drivers/scsi/scsi_lib.c	2006-02-21 01:47:18.000000000 -0600
> @@ -1892,8 +1892,16 @@
>  	}
>  
>  	if(scsi_status_is_good(result)) {
> -		data->header_length = header_length;
> -		if(use_10_for_ms) {
> +		if (unlikely(buffer[0] == 0x86 && buffer[1] == 0x0b &&
> +			     (modepage == 6 || modepage == 8))) {
> +			/* Initio breakage? */
> +			header_length = 0;
> +			data->length = 13;
> +			data->medium_type = 0;
> +			data->device_specific = 0;
> +			data->longlba = 0;
> +			data->block_descriptor_length = 0;
> +		} else if(use_10_for_ms) {
>  			data->length = buffer[0]*256 + buffer[1] + 2;
>  			data->medium_type = buffer[2];
>  			data->device_specific = buffer[3];
> @@ -1906,6 +1914,7 @@
>  			data->device_specific = buffer[2];
>  			data->block_descriptor_length = buffer[3];
>  		}
> +		data->header_length = header_length;
>  	}
>  
>  	return result;
> --- a/drivers/scsi/sd.c	2006-02-17 16:26:52.000000000 -0600
> +++ b/drivers/scsi/sd.c	2006-02-20 18:15:44.000000000 -0600
> @@ -1328,6 +1328,12 @@
>  	if (!scsi_status_is_good(res))
>  		goto bad_sense;
>  
> +	if (!data.header_length) {
> +		modepage = 6;
> +		printk(KERN_ERR "%s: missing header in MODE_SENSE response\n",
> +		       diskname);
> +	}
> +
>  	/* that went OK, now ask for the proper length */
>  	len = data.length;
>  
> @@ -1342,6 +1348,8 @@
>  
>  	/* Take headers and block descriptors into account */
>  	len += data.header_length + data.block_descriptor_length;
> +	if (len > 512)
> +		goto bad_sense;
>  
>  	/* Get the data */
>  	res = sd_do_mode_sense(sdp, dbd, modepage, buffer, len, &data, &sshdr);
> @@ -1354,8 +1362,15 @@
>  		int ct = 0;
>  		int offset = data.header_length + data.block_descriptor_length;
>  
> +		if (offset >= 512 - 2) {
> +			printk(KERN_ERR "%s: malformed MODE SENSE response",
> +				diskname);
> +			goto defaults;
> +		}
> +
>  		if ((buffer[offset] & 0x3f) != modepage) {
> -			printk(KERN_ERR "%s: got wrong page\n", diskname);
> +			printk(KERN_ERR "%s: got wrong page (%d -> %d)\n",
> +				 diskname, modepage, buffer[offset] & 0x3f);
>  			goto defaults;
>  		}
>  
> 


-- 
Stefan Richter
-=====-=-==- --=- =-=-=
http://arcgraph.de/sr/

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Stefan Richter]

Al Viro wrote:
...

Al, James,

what if we downsize this patch to...

> --- a/drivers/scsi/scsi_lib.c	2006-02-20 10:02:58.000000000 -0600
> +++ b/drivers/scsi/scsi_lib.c	2006-02-21 01:47:18.000000000 -0600
> @@ -1892,8 +1892,16 @@
>  	}
>  
>  	if(scsi_status_is_good(result)) {
> -		data->header_length = header_length;
> -		if(use_10_for_ms) {
> +		if (unlikely(buffer[0] == 0x86 && buffer[1] == 0x0b &&
> +			     (modepage == 6 || modepage == 8))) {
> +			/* Initio breakage? */
> +			header_length = 0;
> +			data->length = 13;
> +			data->medium_type = 0;
> +			data->device_specific = 0;
> +			data->longlba = 0;
> +			data->block_descriptor_length = 0;
> +		} else if(use_10_for_ms) {
>  			data->length = buffer[0]*256 + buffer[1] + 2;
>  			data->medium_type = buffer[2];
>  			data->device_specific = buffer[3];
> @@ -1906,6 +1914,7 @@
>  			data->device_specific = buffer[2];
>  			data->block_descriptor_length = buffer[3];
>  		}
> +		data->header_length = header_length;
>  	}
>  
>  	return result;
> --- a/drivers/scsi/sd.c	2006-02-17 16:26:52.000000000 -0600
> +++ b/drivers/scsi/sd.c	2006-02-20 18:15:44.000000000 -0600
> @@ -1328,6 +1328,12 @@
>  	if (!scsi_status_is_good(res))
>  		goto bad_sense;
>  
> +	if (!data.header_length) {
> +		modepage = 6;
> +		printk(KERN_ERR "%s: missing header in MODE_SENSE response\n",
> +		       diskname);
> +	}
> +
>  	/* that went OK, now ask for the proper length */
>  	len = data.length;
>  
> @@ -1342,6 +1348,8 @@
>  
>  	/* Take headers and block descriptors into account */
>  	len += data.header_length + data.block_descriptor_length;
> +	if (len > 512)
> +		goto bad_sense;

...only these two lines and...

>  	/* Get the data */
>  	res = sd_do_mode_sense(sdp, dbd, modepage, buffer, len, &data, &sshdr);
> @@ -1354,8 +1362,15 @@
>  		int ct = 0;
>  		int offset = data.header_length + data.block_descriptor_length;
>  
> +		if (offset >= 512 - 2) {
> +			printk(KERN_ERR "%s: malformed MODE SENSE response",
> +				diskname);
> +			goto defaults;
> +		}
> +

...these 6 lines here? We would miss what can be extracted from these 
buggy devices but (a) sd_read_cache_type::bad_sense's defaults work IMO 
well enough for the so far reported devices and (b) these are the 
particular checks which protect sd from out-of-bound memory access not 
only in case of the Initio-specific breakage.

>  		if ((buffer[offset] & 0x3f) != modepage) {
> -			printk(KERN_ERR "%s: got wrong page\n", diskname);
> +			printk(KERN_ERR "%s: got wrong page (%d -> %d)\n",
> +				 diskname, modepage, buffer[offset] & 0x3f);
>  			goto defaults;
>  		}
>  
> 

-- 
Stefan Richter
-=====-=-==- --=- =-==-
http://arcgraph.de/sr/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Al Viro]

On Wed, Feb 22, 2006 at 08:08:14AM +0100, Stefan Richter wrote:
> ...these 6 lines here? We would miss what can be extracted from these 
> buggy devices but (a) sd_read_cache_type::bad_sense's defaults work IMO 
> well enough for the so far reported devices

Yeah, right.  The same bridge loses its cache contents on reboot.
I.e. missed cache type => dirty fs on every reboot, with actual
corruption in case if there was any recent activity.  So...  No, thanks.
It might make sense to split that in two patches, but if you really
think that defaults work...  I've seen several enclosures where they
don't (different types, at that).


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642

Re: TYPE_RBC cache fixes (sbp2.c affected)

[Stefan Richter]

Al Viro wrote:
> On Wed, Feb 22, 2006 at 08:08:14AM +0100, Stefan Richter wrote:
>>...these 6 lines here? We would miss what can be extracted from these 
>>buggy devices but (a) sd_read_cache_type::bad_sense's defaults work IMO 
>>well enough for the so far reported devices
> 
> Yeah, right.  The same bridge loses its cache contents on reboot.
> I.e. missed cache type => dirty fs on every reboot, with actual
> corruption in case if there was any recent activity.  So...  No, thanks.
> It might make sense to split that in two patches, but if you really
> think that defaults work...  I've seen several enclosures where they
> don't (different types, at that).

Well, right.

BTW, I never noticed such kind of breakage myself yet. I almost always 
break connection not by reboot but by hot-unplug while sbp2 is still 
logged in. This would corrupt data too --- *if* these devices really had 
a write cache and did not flush it themthelves at some point. Maybe 
self-powered devices actually do this at each FireWire bus reset. But 
bus powered devices cannot.
-- 
Stefan Richter
-=====-=-==- --=- =-==-
http://arcgraph.de/sr/