Is it safe to do DMA directly into scmd->sense_buffer?

Is it safe to do DMA directly into scmd->sense_buffer?

[Alan Stern]

When retrieving sense data for a failed command, is it safe to transfer
the data by DMA directly into scmd->sense_buffer?  My guess is that on
architectures that have non-cache-coherent DMA, it's _not_.

The reason is that there is no guarantee the SCSI core won't touch the
scmd structure while the I/O is taking place.  For instance, should the
abort timer expire, the error-handler thread might very well end up
reading or writing data falling in the same cache line as the
sense_buffer.  That would corrupt the CPU's view of data transferred from
the device.

But maybe there's some provision for taking care of this I'm not aware of.  
If anyone can supply the correct answer, my sense-bounce-buffer and I
would appreciate it.

Alan Stern

Re: Is it safe to do DMA directly into scmd->sense_buffer?

[James Bottomley]

On Sun, 2003-06-22 at 11:09, Alan Stern wrote:
> When retrieving sense data for a failed command, is it safe to transfer
> the data by DMA directly into scmd->sense_buffer?  My guess is that on
> architectures that have non-cache-coherent DMA, it's _not_.
> 
> The reason is that there is no guarantee the SCSI core won't touch the
> scmd structure while the I/O is taking place.  For instance, should the
> abort timer expire, the error-handler thread might very well end up
> reading or writing data falling in the same cache line as the
> sense_buffer.  That would corrupt the CPU's view of data transferred from
> the device.
> 
> But maybe there's some provision for taking care of this I'm not aware of.  
> If anyone can supply the correct answer, my sense-bounce-buffer and I
> would appreciate it.

The answer's yes, it is safe.  This is how it works:

The only way you could get the scenario you describe is if the command
times out while the driver is doing an auto request sense.  This would
be highly unlikely, because usually either we lose the command or we get
sense back quickly (remember, the entire device suspends normal
operations on a contingent allegiance condition, so all it has to do is
service our sense request).

Now, even assuming the above happens, the only problem would be that the
sense data itself could be corrupted by an interfering cache line (since
the DMA direction is to the sense buffer, the current model holds that
you flush and invalidate the cache line before beginning the dma.  Thus,
if the CPU brings the cache line in again the only thing it may miss is
the fact that DMA has altered data it has cached).

This was argued about a long time ago, the upshot being that the pain of
doing it correctly was offset by the fact that it didn't seem to have
any observable consequences.

James

Comments about the __scsi_mode_sense() routine

[Alan Stern]

James:

I've got some comments about your __scsi_mode_sense() routine in
scsi_lib.c.  Some of these may be wrong, and I'd like to get the straight 
story.

	1.  The kerneldoc at the start describes the return value wrongly.  
The actual return value is the request's result code; it has nothing to do 
with header offsets.

	2.  According to my outdated SCSI documentation, the DBD bit
should be set to 0 to allow block descriptors and 1 to forbid them.  This
is contrary to the kerneldoc and also contrary to the use in
sd_read_cache_type() in sd.c.

The exact text from the documentation (8.2.10 MODE SENSE(6) command) is: 

A disable block descriptors (DBD) bit of zero indicates that the target 
may return zero or more block descriptors in the returned MODE SENSE data 
(see 8.3.3), at the target s discretion. A DBD bit of one specifies that 
the target shall not return any block descriptors in the returned MODE 
SENSE data.

	3.  Again according to my SCSI spec, the data->length value value
in the mode parameter header does not include the length value itself.  
So the correct value should be computed as (block[0]*256 + block[1]) + 2
for 10-byte commands and block[0] + 1 for 6-byte commands.

Alan Stern

Re: Comments about the __scsi_mode_sense() routine

[James Bottomley]

On Thu, 2003-07-17 at 10:50, Alan Stern wrote:
> James:
> 
> I've got some comments about your __scsi_mode_sense() routine in
> scsi_lib.c.  Some of these may be wrong, and I'd like to get the straight 
> story.
> 
> 	1.  The kerneldoc at the start describes the return value wrongly.  
> The actual return value is the request's result code; it has nothing to do 
> with header offsets.

Yes, changed the return convention but not the docbook.

> 	2.  According to my outdated SCSI documentation, the DBD bit
> should be set to 0 to allow block descriptors and 1 to forbid them.  This
> is contrary to the kerneldoc and also contrary to the use in
> sd_read_cache_type() in sd.c.

This was commented on a while ago.  Looks like I updated the docbook for
one routine but not the other.

James

Re: Comments about the __scsi_mode_sense() routine

[Alan Stern]

James:

I'm not sure if you're interested in getting a patch, or if this patch 
does exactly the right thing.  But here it is anyway.

Alan Stern


===== scsi_lib.c 1.41 vs edited =====
--- 1.41/drivers/scsi/scsi_lib.c	Tue Jul  1 07:51:39 2003
+++ edited/drivers/scsi/scsi_lib.c	Thu Jul 17 15:48:17 2003
@@ -1356,7 +1356,8 @@
  *	__scsi_mode_sense - issue a mode sense, falling back from 10 to 
  *		six bytes if necessary.
  *	@sreq:	SCSI request to fill in with the MODE_SENSE
- *	@dbd:	set if mode sense will allow block descriptors to be returned
+ *	@dbd:	set the 0x08 bit if mode sense will not allow block
+ *		descriptors to be returned
  *	@modepage: mode page being requested
  *	@buffer: request buffer (may not be smaller than eight bytes)
  *	@len:	length of request buffer.
@@ -1364,9 +1365,7 @@
  *	@retries: number of retries before failing
  *	@data: returns a structure abstracting the mode header data
  *
- *	Returns zero if unsuccessful, or the header offset (either 4
- *	or 8 depending on whether a six or ten byte command was
- *	issued) if successful.
+ *	Returns the result code from the completed request.
  **/
 int
 __scsi_mode_sense(struct scsi_request *sreq, int dbd, int modepage,
@@ -1425,15 +1424,15 @@
 
 	if(scsi_status_is_good(sreq->sr_result)) {
 		data->header_length = header_length;
-		if(use_10_for_ms) {
-			data->length = buffer[0]*256 + buffer[1];
+		if (use_10_for_ms) {
+			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];
 		} else {
-			data->length = buffer[0];
+			data->length = buffer[0] + 1;
 			data->medium_type = buffer[1];
 			data->device_specific = buffer[3];
 			data->block_descriptor_length = buffer[4];

PATCH: (as73) Do a minimal transfer for disk-cache mode-sense page

[Alan Stern]

James:

This patch for 2.6.0 makes 3 changes in the read-write- and cache-detect
mode-sense routines in sd.c:

	It sets the DBD flag to prevent the transfer of unwanted Block 
Descriptors.

	It uses 512 (the actual buffer size) as an upper limit, not 128
as in the current code.

	It transfers only the first 4 bytes of the cache page; the 
information we want is in the 3rd byte.

The last change is the real reason for submitting the patch.  Apparently
the Genesyslogic USB disk drive doesn't like being asked for more than 64
bytes of its cache page.  Doing so crashes the drive's firmware.  Since
the transfer size had to be reduced anyway, I decided to use the minimum
necessary (rounded up to a multiple of 4) -- similar to other changes that
have been made recently in various mode-sense transfers.

This fixes the problem with the Genesyslogic drive, and it ought to work 
correctly with any standards-conformant device.  If it looks okay with 
you, please apply it.

Alan Stern


===== sd.c 1.52 vs edited =====
--- 1.52/drivers/scsi/sd.c	Tue Jul  1 17:54:19 2003
+++ edited/drivers/scsi/sd.c	Wed Jul 30 16:15:46 2003
@@ -1080,6 +1080,7 @@
 sd_read_write_protect_flag(struct scsi_disk *sdkp, char *diskname,
 		   struct scsi_request *SRpnt, unsigned char *buffer) {
 	int res;
+	const int dbd = 0x08;	   /* DBD */
 	struct scsi_mode_data data;
 
 	/*
@@ -1087,7 +1088,7 @@
 	 * We have to start carefully: some devices hang if we ask
 	 * for more than is available.
 	 */
-	res = sd_do_mode_sense(SRpnt, 0, 0x3F, buffer, 4, &data);
+	res = sd_do_mode_sense(SRpnt, dbd, 0x3F, buffer, 4, &data);
 
 	/*
 	 * Second attempt: ask for page 0
@@ -1095,13 +1096,13 @@
 	 * Sense Key 5: Illegal Request, Sense Code 24: Invalid field in CDB.
 	 */
 	if (!scsi_status_is_good(res))
-		res = sd_do_mode_sense(SRpnt, 0, 0, buffer, 4, &data);
+		res = sd_do_mode_sense(SRpnt, dbd, 0, buffer, 4, &data);
 
 	/*
 	 * Third attempt: ask 255 bytes, as we did earlier.
 	 */
 	if (!scsi_status_is_good(res))
-		res = sd_do_mode_sense(SRpnt, 0, 0x3F, buffer, 255, &data);
+		res = sd_do_mode_sense(SRpnt, dbd, 0x3F, buffer, 255, &data);
 
 	if (!scsi_status_is_good(res)) {
 		printk(KERN_WARNING
@@ -1124,7 +1125,7 @@
 		   struct scsi_request *SRpnt, unsigned char *buffer) {
 	int len = 0, res;
 
-	const int dbd = 0;	   /* DBD */
+	const int dbd = 0x08;	   /* DBD */
 	const int modepage = 0x08; /* current values, cache page */
 	struct scsi_mode_data data;
 
@@ -1134,11 +1135,12 @@
 
 	if (scsi_status_is_good(res)) {
 		/* that went OK, now ask for the proper length */
-		len = data.length;
-		if (len > 128)
-			len = 128;
-		res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer,
+		len = data.header_length + data.block_descriptor_length + 4;
+		if (len <= 512)
+			res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer,
 				       len, &data);
+		else
+			res = SAM_STAT_BUSY;
 	}
 
 	if (scsi_status_is_good(res)) {

Re: PATCH: (as73) Do a minimal transfer for disk-cache mode-sense page

[James Bottomley]

On Wed, 2003-07-30 at 15:36, Alan Stern wrote:
> 	It sets the DBD flag to prevent the transfer of unwanted Block 
> Descriptors.

I'm afraid I think we've already been around the houses on this one and
decided that we can't do it.

The problem is that most CD-ROMS and some USB storage devices follow one
of the ATAPI packet specifications which has the DBD bit as reserved,
and someone actually found one which gets annoyed if DBD is set.

> 	It uses 512 (the actual buffer size) as an upper limit, not 128
> as in the current code.

This is fine since the magic buffer size is 512 bytes.

> 	It transfers only the first 4 bytes of the cache page; the 
> information we want is in the 3rd byte.

I didn't actually spot this anywhere in the patch.


> -		len = data.length;
> -		if (len > 128)
> -			len = 128;
> -		res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer,
> +		len = data.header_length + data.block_descriptor_length + 4;
> +		if (len <= 512)
> +			res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer,
>  				       len, &data);
> +		else
> +			res = SAM_STAT_BUSY;

Won't work.  SAM_STAT_BUSY will cause an immediate retry, which still
won't get satisfied.  I think you need to follow the original logic but
change 128 to 512

James

Revised PATCH: (as73b) Do a minimal transfer for disk-cache mode-sense page

[Alan Stern]

On 30 Jul 2003, James Bottomley wrote:

> On Wed, 2003-07-30 at 15:36, Alan Stern wrote:
> > 	It sets the DBD flag to prevent the transfer of unwanted Block 
> > Descriptors.
> 
> I'm afraid I think we've already been around the houses on this one and
> decided that we can't do it.
> 
> The problem is that most CD-ROMS and some USB storage devices follow one
> of the ATAPI packet specifications which has the DBD bit as reserved,
> and someone actually found one which gets annoyed if DBD is set.

Okay.  The revised patch doesn't set DBD and adds an explanatory comment.

> > 	It transfers only the first 4 bytes of the cache page; the 
> > information we want is in the 3rd byte.
> 
> I didn't actually spot this anywhere in the patch.

It's in the line where len is set to the sum of the two headers plus 4.

> Won't work.  SAM_STAT_BUSY will cause an immediate retry, which still
> won't get satisfied.  I think you need to follow the original logic but
> change 128 to 512

I disagree.  All that will happen is that res will fail the 
scsi_status_is_good() test, so the routine will by default assume a 
write-through cache.

However, there would be nothing wrong with changing SAM_STAT_BUSY to a 
different error code, such as SAM_STAT_TASK_SET_FULL.  It doesn't matter 
what the value is, so long as it doesn't pass scsi_status_is_good().

Alan Stern


===== sd.c 1.52 vs edited =====
--- 1.52/drivers/scsi/sd.c	Tue Jul  1 17:54:19 2003
+++ edited/drivers/scsi/sd.c	Thu Jul 31 11:05:01 2003
@@ -1080,6 +1080,7 @@
 sd_read_write_protect_flag(struct scsi_disk *sdkp, char *diskname,
 		   struct scsi_request *SRpnt, unsigned char *buffer) {
 	int res;
+	const int dbd = 0;	   /* Some devices don't like DBD */
 	struct scsi_mode_data data;
 
 	/*
@@ -1087,7 +1088,7 @@
 	 * We have to start carefully: some devices hang if we ask
 	 * for more than is available.
 	 */
-	res = sd_do_mode_sense(SRpnt, 0, 0x3F, buffer, 4, &data);
+	res = sd_do_mode_sense(SRpnt, dbd, 0x3F, buffer, 4, &data);
 
 	/*
 	 * Second attempt: ask for page 0
@@ -1095,13 +1096,13 @@
 	 * Sense Key 5: Illegal Request, Sense Code 24: Invalid field in CDB.
 	 */
 	if (!scsi_status_is_good(res))
-		res = sd_do_mode_sense(SRpnt, 0, 0, buffer, 4, &data);
+		res = sd_do_mode_sense(SRpnt, dbd, 0, buffer, 4, &data);
 
 	/*
 	 * Third attempt: ask 255 bytes, as we did earlier.
 	 */
 	if (!scsi_status_is_good(res))
-		res = sd_do_mode_sense(SRpnt, 0, 0x3F, buffer, 255, &data);
+		res = sd_do_mode_sense(SRpnt, dbd, 0x3F, buffer, 255, &data);
 
 	if (!scsi_status_is_good(res)) {
 		printk(KERN_WARNING
@@ -1124,7 +1125,7 @@
 		   struct scsi_request *SRpnt, unsigned char *buffer) {
 	int len = 0, res;
 
-	const int dbd = 0;	   /* DBD */
+	const int dbd = 0;	   /* Some devices don't like DBD */
 	const int modepage = 0x08; /* current values, cache page */
 	struct scsi_mode_data data;
 
@@ -1134,10 +1135,11 @@
 
 	if (scsi_status_is_good(res)) {
 		/* that went OK, now ask for the proper length */
-		len = data.length;
-		if (len > 128)
-			len = 128;
-		res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer,
+		len = data.header_length + data.block_descriptor_length + 4;
+		if (len > 512)
+			res = SAM_STAT_BUSY;
+		else
+			res = sd_do_mode_sense(SRpnt, dbd, modepage, buffer,
 				       len, &data);
 	}
 

PATCH: (as70b) Update request_bufflen to match this_count

[Alan Stern]

James:

This patch addresses a problem in both sd.c and sr.c.  When a read/write
command is initialized, the routines may reduce this_count (the number of
sectors to transfer) if it exceeds the maximum allowed value (i.e., 0xffff
for READ(10)).  However, the code does not similarly alter
scmd->request_bufflen and scmd->bufflen to match the change in the CDB
value.

scmd->request_bufflen is important for the usb-storage driver, which
requires that it be exactly equal to the number of bytes transferred.  
scmd->bufflen is used in the rw_intr() routines, where it is passed to 
scsi_io_completion() as the number of sectors transferred if no errors 
occur.

Another small change in the patch concerns the code in sr.c that checks 
whether the total of the scatter-gather area lengths matches 
scmd->request_bufflen.  If they don't match, the patch bumps the kernel 
log message level up to KERN_ERR, and it takes the more conservative 
approach of adjusting scmd->request_bufflen only if the s-g length is 
smaller than the request length.

The patch applies to 2.6.0-test2.

Alan Stern

P.S.: I sent essentially this same patch to Jens Axboe a while back, but I 
haven't heard anything about it.  I'm resubmitting to you on the theory 
that you'll have more time to look at it.


# This is a BitKeeper generated patch for the following project:
# Project Name: greg k-h's linux 2.5 USB kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1656  -> 1.1657 
#	   drivers/scsi/sr.c	1.51    -> 1.52   
#	   drivers/scsi/sd.c	1.53    -> 1.54   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/07/31	stern@ida.rowland.org	1.1657
# Update request_bufflen and bufflen to match changes in this_count.
# Fix error handling for when s-g count differs from request_bufflen.
# --------------------------------------------
#
diff -Nru a/drivers/scsi/sd.c b/drivers/scsi/sd.c
--- a/drivers/scsi/sd.c	Thu Jul 31 15:28:30 2003
+++ b/drivers/scsi/sd.c	Thu Jul 31 15:28:30 2003
@@ -308,6 +308,8 @@
 		SCpnt->cmnd[4] = (unsigned char) this_count;
 		SCpnt->cmnd[5] = 0;
 	}
+	SCpnt->request_bufflen = SCpnt->bufflen =
+			this_count * sdp->sector_size;
 
 	/*
 	 * We shouldn't disconnect in the middle of a sector, so with a dumb
diff -Nru a/drivers/scsi/sr.c b/drivers/scsi/sr.c
--- a/drivers/scsi/sr.c	Thu Jul 31 15:28:30 2003
+++ b/drivers/scsi/sr.c	Thu Jul 31 15:28:30 2003
@@ -315,6 +315,20 @@
 		return 0;
 	}
 
+	{
+		struct scatterlist *sg = SCpnt->request_buffer;
+		int i, size = 0;
+		for (i = 0; i < SCpnt->use_sg; i++)
+			size += sg[i].length;
+
+		if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
+			printk(KERN_ERR "sr: mismatch count %d, bytes %d\n",
+					size, SCpnt->request_bufflen);
+			if (SCpnt->request_bufflen > size)
+				SCpnt->request_bufflen = SCpnt->bufflen = size;
+		}
+	}
+
 	/*
 	 * request doesn't start on hw block boundary, add scatter pads
 	 */
@@ -336,8 +350,11 @@
 	SCpnt->cmnd[1] = 0;
 	block = (unsigned int)SCpnt->request->sector / (s_size >> 9);
 
-	if (this_count > 0xffff)
+	if (this_count > 0xffff) {
 		this_count = 0xffff;
+		SCpnt->request_bufflen = SCpnt->bufflen =
+				this_count * s_size;
+	}
 
 	SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
 	SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
@@ -364,18 +381,6 @@
 	 * of capability to this function.
 	 */
 	SCpnt->done = rw_intr;
-
-	{
-		struct scatterlist *sg = SCpnt->request_buffer;
-		int i, size = 0;
-		for (i = 0; i < SCpnt->use_sg; i++)
-			size += sg[i].length;
-
-		if (size != SCpnt->request_bufflen && SCpnt->use_sg) {
-			printk("sr: mismatch count %d, bytes %d\n", size, SCpnt->request_bufflen);
-			SCpnt->request_bufflen = size;
-		}
-	}
 
 	/*
 	 * This indicates that the command is ready from our end to be