Re: TYPE_RBC cache fixes (sbp2.c affected)

[Stefan Richter <stefanr@xxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> ----

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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/linux1394-devel


--
Stefan Richter
-=====-=-==- --=- -=--=
http://arcgraph.de/sr/
-
: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html