[Patch] SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails

[Patch] SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails

[Amit Sahrawat]

SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails

It has been observed that a number of USB HDD's do not respond correctly
to SCSI mode sense command(retrieve caching pages) which results in their
Write Cache being discarded by queue requests i.e., WCE if left set to
'0'(disabled). So, in order to identify the devices correctly - give it
a last try using SG_ATA_16 after failure from normal routine.

Signed-off-by: Amit Sahrawat <amit.sahrawat83@gmail.com>

diff -Nurp linux-Orig/drivers/scsi/sd.c linux-Updated/drivers/scsi/sd.c
--- linux-Orig/drivers/scsi/sd.c	2011-10-11 11:02:48.000000000 +0530
+++ linux-Updated/drivers/scsi/sd.c	2011-10-11 11:10:09.000000000 +0530
@@ -56,6 +56,7 @@
 #include <asm/uaccess.h>
 #include <asm/unaligned.h>

+#include <scsi/sg.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_dbg.h>
@@ -134,6 +135,58 @@ static const char *sd_cache_types[] = {
 	"write back, no read (daft)"
 };

+/* Relevant Structure and Function to be used to Retrieve
+ * Caching Information from USB HDD - this is picked from
+ * source code of 'hdparm'
+ *
+ *
+ * Definitions and structures for use with SG_IO + ATA_16:
+ * */
+#define SG_ATA_16               0x85
+#define SG_ATA_16_LEN           16
+
+#define ATA_OP_IDENTIFY		0xec
+
+/*
+ * Some useful ATA register bits
+ */
+enum {
+        ATA_USING_LBA           = (1 << 6),
+        ATA_STAT_DRQ            = (1 << 3),
+        ATA_STAT_ERR            = (1 << 0),
+};
+
+struct ata_lba_regs {
+        __u8    feat;
+        __u8    nsect;
+        __u8    lbal;
+        __u8    lbam;
+        __u8    lbah;
+};
+struct ata_tf {
+        __u8                    dev;
+        __u8                    command;
+        __u8                    error;
+        __u8                    status;
+        __u8                    is_lba48;
+        struct ata_lba_regs     lob;
+        struct ata_lba_regs     hob;
+};
+
+__u64 tf_to_lba (struct ata_tf *tf)
+{
+        __u32 lba24, lbah;
+        __u64 lba64;
+
+        lba24 = (tf->lob.lbah << 16) | (tf->lob.lbam << 8) | (tf->lob.lbal);
+        if (tf->is_lba48)
+                lbah = (tf->hob.lbah << 16) | (tf->hob.lbam << 8) |
(tf->hob.lbal);
+        else
+                lbah = (tf->dev & 0x0f);
+        lba64 = (((__u64)lbah) << 24) | (__u64)lba24;
+        return lba64;
+}
+
 static ssize_t
 sd_store_cache_type(struct device *dev, struct device_attribute *attr,
 		    const char *buf, size_t count)
@@ -1839,6 +1892,18 @@ sd_read_cache_type(struct scsi_disk *sdk
 	int old_rcd = sdkp->RCD;
 	int old_dpofua = sdkp->DPOFUA;

+	struct ata_tf tf;
+	struct sg_io_hdr io_hdr;
+	unsigned char cdb[SG_ATA_16_LEN] = {0};
+	unsigned char sb[32] = {0};
+	unsigned char buf[512] = {0};
+	unsigned short wce_word = 0;
+	void *data_cmd = buf;
+
+	memset(cdb, 0, SG_ATA_16_LEN);
+	memset(&tf, 0, sizeof(struct ata_tf));
+	memset(&io_hdr, 0, sizeof(struct sg_io_hdr));
+
        first_len = 4;
        if (sdp->skip_ms_page_8) {
                if (sdp->type == TYPE_RBC)
@@ -1961,7 +2026,6 @@ Page_found:
 				  sdkp->DPOFUA ? "supports DPO and FUA"
 				  : "doesn't support DPO or FUA");

-		return;
 	}

 bad_sense:
@@ -1974,8 +2038,64 @@ bad_sense:
 		sd_printk(KERN_ERR, sdkp, "Asking for cache data failed\n");

 defaults:
-	sd_printk(KERN_ERR, sdkp, "Assuming drive cache: write through\n");
-	sdkp->WCE = 0;
+	if (sdkp->WCE)
+		return;
+	else {
+		sd_printk(KERN_ERR, sdkp, "Normal Routine Failed: Trying ATA_16\n");
+		
+		/* The below are necessary parameters which are to set - in order
+		to make use of ATA_OP_IDENTIFY command */
+		tf.lob.lbal = 0;
+	        tf.lob.lbam = 0;
+	        tf.lob.lbah = 0;
+	        tf.lob.nsect = 1; //Number of Sectors to Read
+	        tf.lob.feat = 0;
+
+		/* Command Descriptor Block For SCSI 	*/
+	        cdb[0] = SG_ATA_16;
+	        cdb[1] = 0x08;
+	        cdb[2] = 0x0e;
+	        cdb[6] = 0x01; //No. of Sectors To Read
+	        cdb[13] = ATA_USING_LBA;
+	        cdb[14] = ATA_OP_IDENTIFY;
+
+	        io_hdr.cmd_len = SG_ATA_16_LEN;
+	        io_hdr.interface_id = SG_INTERFACE_ID_ORIG;
+	        io_hdr.mx_sb_len= sizeof(sb);
+	        io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
+	        io_hdr.dxfer_len = sizeof(buf);
+	        io_hdr.dxferp = data_cmd;
+	        io_hdr.cmdp = cdb;
+	        io_hdr.sbp = sb;
+	        io_hdr.pack_id = tf_to_lba(&tf);
+        	io_hdr.timeout = 0;
+
+	        if (!scsi_cmd_ioctl(sdkp->disk->queue, sdkp->disk,
O_RDONLY|O_NONBLOCK, SG_IO, &io_hdr))
+        	{
+#if 0
+#define DUMP_BYTES_BUFFER(x...) printk( x )
+			int i;
+			for (i = 0; i < 32; i++)
+        	                DUMP_BYTES_BUFFER(" %02x", sb[i]);
+	                DUMP_BYTES_BUFFER("\n");
+	                for (i = 0; i < 512; i++)
+        	                DUMP_BYTES_BUFFER(" %02x", buf[i]);
+	                DUMP_BYTES_BUFFER("\n");
+        	        printk(KERN_ERR"82 - [0x%x], 85 -
[0x%x]\n",((unsigned short*)data_cmd)[82],((unsigned
short*)data_cmd)[85]);
+#endif
+			/* '6th' Bit in Word 85 Corresponds to Write Cache being Enabled/disabled*/
+                	wce_word = le16_to_cpu(((unsigned short*)data_cmd)[85]);
+	                if (wce_word & 0x20) {
+        	                sdkp->WCE = 1;
+                		sd_printk(KERN_NOTICE, sdkp, "Write Cache Enabled
after ATA_16 response\n");
+			} else
+				goto write_through;
+	        } else {
+write_through:
+	        	sd_printk(KERN_ERR, sdkp, "Assuming drive cache: write through\n");
+		        sdkp->WCE = 0;
+        	}
+	}
 	sdkp->RCD = 0;
 	sdkp->DPOFUA = 0;
 }

Re: [Patch] SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails

[Hannes Reinecke]

On 10/11/2011 10:26 AM, Amit Sahrawat wrote:
> SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails
>
> It has been observed that a number of USB HDD's do not respond correctly
> to SCSI mode sense command(retrieve caching pages) which results in their
> Write Cache being discarded by queue requests i.e., WCE if left set to
> '0'(disabled). So, in order to identify the devices correctly - give it
> a last try using SG_ATA_16 after failure from normal routine.
>
I'd be _VERY_ cautious when trying something like this.

ATA_16 is a rather recent command, and most older drives simply 
can't cope with this. Instead they just kick the error handler and 
go offline eventually.

Shouldn't we rather use a sensible default, ie assume WCE is enabled?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare-l3A5Bk7waGM@public.gmane.org			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch] SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails

[Amit Sahrawat]

On Tue, Oct 11, 2011 at 4:11 PM, Hannes Reinecke <hare@suse.de> wrote:
> On 10/11/2011 10:26 AM, Amit Sahrawat wrote:
>>
>> SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails
>>
>> It has been observed that a number of USB HDD's do not respond correctly
>> to SCSI mode sense command(retrieve caching pages) which results in their
>> Write Cache being discarded by queue requests i.e., WCE if left set to
>> '0'(disabled). So, in order to identify the devices correctly - give it
>> a last try using SG_ATA_16 after failure from normal routine.
>>
> I'd be _VERY_ cautious when trying something like this.
>
> ATA_16 is a rather recent command, and most older drives simply can't cope
> with this. Instead they just kick the error handler and go offline
> eventually.
I tried with a number of USB HDD(around 6 with different
manufacturer's, 1 USB SSD and pen drives) - they are responding
correctly with this.
>
> Shouldn't we rather use a sensible default, ie assume WCE is enabled?
If WCE is enabled by default - that will mark QUEUE ORDERING to
QUEUE_ORDERED_DRAIN_FLUSH(Preflush-Postflush)(ie., flushing will occur
for all the Mass Storage devices - which in turn will keep on calling
SYNC_CACHE for these devices(which do not have write cache)) - ie., an
extra overhead for the devices which do not have this routine(Pen
drives being widely used among these)
Is there any identification(unique information) for "older drives" ?
Please share your opinion more.

>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke                   zSeries & Storage
> hare@suse.de                          +49 911 74053 688
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
>
Regards,
Amit Sahrawat

Re: [Patch] SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails

[NamJae Jeon]

2011/10/11 Amit Sahrawat <amit.sahrawat83@gmail.com>:
> On Tue, Oct 11, 2011 at 4:11 PM, Hannes Reinecke <hare@suse.de> wrote:
>> On 10/11/2011 10:26 AM, Amit Sahrawat wrote:
>>>
>>> SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails
>>>
>>> It has been observed that a number of USB HDD's do not respond correctly
>>> to SCSI mode sense command(retrieve caching pages) which results in their
>>> Write Cache being discarded by queue requests i.e., WCE if left set to
>>> '0'(disabled). So, in order to identify the devices correctly - give it
>>> a last try using SG_ATA_16 after failure from normal routine.
>>>
>> I'd be _VERY_ cautious when trying something like this.
>>
>> ATA_16 is a rather recent command, and most older drives simply can't cope
>> with this. Instead they just kick the error handler and go offline
>> eventually.
> I tried with a number of USB HDD(around 6 with different
> manufacturer's, 1 USB SSD and pen drives) - they are responding
> correctly with this.
>>
>> Shouldn't we rather use a sensible default, ie assume WCE is enabled?
> If WCE is enabled by default - that will mark QUEUE ORDERING to
> QUEUE_ORDERED_DRAIN_FLUSH(Preflush-Postflush)(ie., flushing will occur
> for all the Mass Storage devices - which in turn will keep on calling
> SYNC_CACHE for these devices(which do not have write cache)) - ie., an
> extra overhead for the devices which do not have this routine(Pen
> drives being widely used among these)
> Is there any identification(unique information) for "older drives" ?
> Please share your opinion more.
I think that it is good approach. Have you tested journaling test on XFS ?
For example, while tiny several files is written, Is there corruption
fail when suddenly unplugging usb HDD ?
And you should check write performance.
If you change a little, It will be better~ let me check more.
>
>>
>> Cheers,
>>
>> Hannes
>> --
>> Dr. Hannes Reinecke                   zSeries & Storage
>> hare@suse.de                          +49 911 74053 688
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
>> GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
>>
> Regards,
> Amit Sahrawat
>

Re: [Patch] SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails

[Jeff Garzik]

On 10/11/2011 06:41 AM, Hannes Reinecke wrote:
> ATA_16 is a rather recent command, and most older drives simply can't
> cope with this. Instead they just kick the error handler and go offline
> eventually.

I tend to agree...  there are a great many USB drives out there, and 
this has not really been tested with a large test selection, has it?

	Jeff

Re: [Patch] SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails

[James Bottomley]

On Tue, 2011-10-11 at 13:56 +0530, Amit Sahrawat wrote:
> SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails
> 
> It has been observed that a number of USB HDD's do not respond correctly
> to SCSI mode sense command(retrieve caching pages) which results in their
> Write Cache being discarded by queue requests i.e., WCE if left set to
> '0'(disabled). So, in order to identify the devices correctly - give it
> a last try using SG_ATA_16 after failure from normal routine.

This is a non-starter, as I've said before.  Apart from the layering
violation of trying to make sd ATA aware, ATA_16 is known to crash some
USB devices (the bug reports are mostly about smartctl which can be made
to use ATA_16 failing).

The correct way to implement this is to have a user visible WCE variable
that can be written to in the scsi_disk class sysfs files so the user
can alter the caching type on the fly.

James

Re: [Patch] SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails

[Alan Stern]

On Tue, 11 Oct 2011, Amit Sahrawat wrote:

> SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails
> 
> It has been observed that a number of USB HDD's do not respond correctly
> to SCSI mode sense command(retrieve caching pages) which results in their
> Write Cache being discarded by queue requests i.e., WCE if left set to
> '0'(disabled). So, in order to identify the devices correctly - give it
> a last try using SG_ATA_16 after failure from normal routine.
> 
> Signed-off-by: Amit Sahrawat <amit.sahrawat83@gmail.com>
> 
> diff -Nurp linux-Orig/drivers/scsi/sd.c linux-Updated/drivers/scsi/sd.c
> --- linux-Orig/drivers/scsi/sd.c	2011-10-11 11:02:48.000000000 +0530
> +++ linux-Updated/drivers/scsi/sd.c	2011-10-11 11:10:09.000000000 +0530
> @@ -56,6 +56,7 @@
>  #include <asm/uaccess.h>
>  #include <asm/unaligned.h>
> 
> +#include <scsi/sg.h>
>  #include <scsi/scsi.h>
>  #include <scsi/scsi_cmnd.h>
>  #include <scsi/scsi_dbg.h>
> @@ -134,6 +135,58 @@ static const char *sd_cache_types[] = {
>  	"write back, no read (daft)"
>  };
> 
> +/* Relevant Structure and Function to be used to Retrieve
> + * Caching Information from USB HDD - this is picked from
> + * source code of 'hdparm'
> + *
> + *
> + * Definitions and structures for use with SG_IO + ATA_16:
> + * */
> +#define SG_ATA_16               0x85
> +#define SG_ATA_16_LEN           16
> +
> +#define ATA_OP_IDENTIFY		0xec
> +
> +/*
> + * Some useful ATA register bits
> + */
> +enum {
> +        ATA_USING_LBA           = (1 << 6),
> +        ATA_STAT_DRQ            = (1 << 3),
> +        ATA_STAT_ERR            = (1 << 0),
> +};
> +
> +struct ata_lba_regs {
> +        __u8    feat;
> +        __u8    nsect;
> +        __u8    lbal;
> +        __u8    lbam;
> +        __u8    lbah;
> +};
> +struct ata_tf {
> +        __u8                    dev;
> +        __u8                    command;
> +        __u8                    error;
> +        __u8                    status;
> +        __u8                    is_lba48;
> +        struct ata_lba_regs     lob;
> +        struct ata_lba_regs     hob;
> +};

Don't these things already exist in some standard header file?  If not, 
shouldn't they be added in a more central location?

> +__u64 tf_to_lba (struct ata_tf *tf)
> +{
> +        __u32 lba24, lbah;
> +        __u64 lba64;
> +
> +        lba24 = (tf->lob.lbah << 16) | (tf->lob.lbam << 8) | (tf->lob.lbal);
> +        if (tf->is_lba48)
> +                lbah = (tf->hob.lbah << 16) | (tf->hob.lbam << 8) |
> (tf->hob.lbal);
> +        else
> +                lbah = (tf->dev & 0x0f);
> +        lba64 = (((__u64)lbah) << 24) | (__u64)lba24;
> +        return lba64;
> +}
> +
>  static ssize_t
>  sd_store_cache_type(struct device *dev, struct device_attribute *attr,
>  		    const char *buf, size_t count)
> @@ -1839,6 +1892,18 @@ sd_read_cache_type(struct scsi_disk *sdk
>  	int old_rcd = sdkp->RCD;
>  	int old_dpofua = sdkp->DPOFUA;
> 
> +	struct ata_tf tf;
> +	struct sg_io_hdr io_hdr;
> +	unsigned char cdb[SG_ATA_16_LEN] = {0};
> +	unsigned char sb[32] = {0};
> +	unsigned char buf[512] = {0};

Arrays generally should not have static initializers.  Also, a 512-byte
array is too large to allocate on the stack.  And there's already a
512-byte array available -- it's named "buffer".

> +	unsigned short wce_word = 0;

There's no reason to initialize this variable.

> +	void *data_cmd = buf;

Why do you need to alias a perfectly good variable?

> +
> +	memset(cdb, 0, SG_ATA_16_LEN);
> +	memset(&tf, 0, sizeof(struct ata_tf));
> +	memset(&io_hdr, 0, sizeof(struct sg_io_hdr));

There's no point initializing these things before you know that they 
will be used.

> +
>         first_len = 4;
>         if (sdp->skip_ms_page_8) {
>                 if (sdp->type == TYPE_RBC)
> @@ -1961,7 +2026,6 @@ Page_found:
>  				  sdkp->DPOFUA ? "supports DPO and FUA"
>  				  : "doesn't support DPO or FUA");
> 
> -		return;
>  	}
> 
>  bad_sense:
> @@ -1974,8 +2038,64 @@ bad_sense:
>  		sd_printk(KERN_ERR, sdkp, "Asking for cache data failed\n");
> 
>  defaults:
> -	sd_printk(KERN_ERR, sdkp, "Assuming drive cache: write through\n");
> -	sdkp->WCE = 0;
> +	if (sdkp->WCE)
> +		return;
> +	else {
> +		sd_printk(KERN_ERR, sdkp, "Normal Routine Failed: Trying ATA_16\n");

Instead of adding an awful lot of code inside an "else" clause, it 
would be better to put this into its own subroutine.

> +		
> +		/* The below are necessary parameters which are to set - in order
> +		to make use of ATA_OP_IDENTIFY command */
> +		tf.lob.lbal = 0;
> +	        tf.lob.lbam = 0;
> +	        tf.lob.lbah = 0;
> +	        tf.lob.nsect = 1; //Number of Sectors to Read
> +	        tf.lob.feat = 0;
> +
> +		/* Command Descriptor Block For SCSI 	*/
> +	        cdb[0] = SG_ATA_16;
> +	        cdb[1] = 0x08;
> +	        cdb[2] = 0x0e;
> +	        cdb[6] = 0x01; //No. of Sectors To Read
> +	        cdb[13] = ATA_USING_LBA;
> +	        cdb[14] = ATA_OP_IDENTIFY;
> +
> +	        io_hdr.cmd_len = SG_ATA_16_LEN;
> +	        io_hdr.interface_id = SG_INTERFACE_ID_ORIG;
> +	        io_hdr.mx_sb_len= sizeof(sb);
> +	        io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
> +	        io_hdr.dxfer_len = sizeof(buf);
> +	        io_hdr.dxferp = data_cmd;
> +	        io_hdr.cmdp = cdb;
> +	        io_hdr.sbp = sb;
> +	        io_hdr.pack_id = tf_to_lba(&tf);
> +        	io_hdr.timeout = 0;
> +
> +	        if (!scsi_cmd_ioctl(sdkp->disk->queue, sdkp->disk,
> O_RDONLY|O_NONBLOCK, SG_IO, &io_hdr))

Do you really need to do an ioctl?  Why not call scsi_execute_req() 
directly?

> +        	{
> +#if 0
> +#define DUMP_BYTES_BUFFER(x...) printk( x )
> +			int i;
> +			for (i = 0; i < 32; i++)
> +        	                DUMP_BYTES_BUFFER(" %02x", sb[i]);
> +	                DUMP_BYTES_BUFFER("\n");
> +	                for (i = 0; i < 512; i++)
> +        	                DUMP_BYTES_BUFFER(" %02x", buf[i]);
> +	                DUMP_BYTES_BUFFER("\n");
> +        	        printk(KERN_ERR"82 - [0x%x], 85 -
> [0x%x]\n",((unsigned short*)data_cmd)[82],((unsigned
> short*)data_cmd)[85]);
> +#endif

For the final patch submission, of course this section should be 
removed.

> +			/* '6th' Bit in Word 85 Corresponds to Write Cache being Enabled/disabled*/
> +                	wce_word = le16_to_cpu(((unsigned short*)data_cmd)[85]);
> +	                if (wce_word & 0x20) {
> +        	                sdkp->WCE = 1;
> +                		sd_printk(KERN_NOTICE, sdkp, "Write Cache Enabled
> after ATA_16 response\n");
> +			} else
> +				goto write_through;
> +	        } else {
> +write_through:
> +	        	sd_printk(KERN_ERR, sdkp, "Assuming drive cache: write through\n");
> +		        sdkp->WCE = 0;
> +        	}
> +	}
>  	sdkp->RCD = 0;
>  	sdkp->DPOFUA = 0;
>  }

Besides the potential problems raised by other people, these structural
weaknesses in the patch should be fixed.

Alan Stern

Re: [Patch] SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails

[Amit Sahrawat]

On Tue, Oct 11, 2011 at 7:35 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Tue, 2011-10-11 at 13:56 +0530, Amit Sahrawat wrote:
>> SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails
>>
>> It has been observed that a number of USB HDD's do not respond correctly
>> to SCSI mode sense command(retrieve caching pages) which results in their
>> Write Cache being discarded by queue requests i.e., WCE if left set to
>> '0'(disabled). So, in order to identify the devices correctly - give it
>> a last try using SG_ATA_16 after failure from normal routine.
>
> This is a non-starter, as I've said before.  Apart from the layering
> violation of trying to make sd ATA aware, ATA_16 is known to crash some
> USB devices (the bug reports are mostly about smartctl which can be made
> to use ATA_16 failing).
I agree that there might devices which crash on this response.
>
> The correct way to implement this is to have a user visible WCE variable
> that can be written to in the scsi_disk class sysfs files so the user
> can alter the caching type on the fly.
This is OK to add WCE variable, but again this "on the fly" is
confusing. I expect this to be initialized the moment I plug-in the
USB. If the user has to take this decision - then again there has to
be a supporting fact so as to change the "cache type" from sysfs.
Again, the point would be how to get this if the device do not respond
correctly. sd_store_cache_type() - is this main code point which needs
to be thoroughly checked for this? especially the scsi_mode_select()?
Please correct me if I am wrong.
>
> James
>
>
Thanks & Regards,
Amit Sahrawat

Re: [Patch] SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails

[Amit Sahrawat]

On Tue, Oct 11, 2011 at 7:49 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 11 Oct 2011, Amit Sahrawat wrote:
>
>> SCSI: Retrieve Cache Mode Using SG_ATA_16 if normal routine fails
>>
>> It has been observed that a number of USB HDD's do not respond correctly
>> to SCSI mode sense command(retrieve caching pages) which results in their
>> Write Cache being discarded by queue requests i.e., WCE if left set to
>> '0'(disabled). So, in order to identify the devices correctly - give it
>> a last try using SG_ATA_16 after failure from normal routine.
>>
>> Signed-off-by: Amit Sahrawat <amit.sahrawat83@gmail.com>
>>
>> diff -Nurp linux-Orig/drivers/scsi/sd.c linux-Updated/drivers/scsi/sd.c
>> --- linux-Orig/drivers/scsi/sd.c      2011-10-11 11:02:48.000000000 +0530
>> +++ linux-Updated/drivers/scsi/sd.c   2011-10-11 11:10:09.000000000 +0530
>> @@ -56,6 +56,7 @@
>>  #include <asm/uaccess.h>
>>  #include <asm/unaligned.h>
>>
>> +#include <scsi/sg.h>
>>  #include <scsi/scsi.h>
>>  #include <scsi/scsi_cmnd.h>
>>  #include <scsi/scsi_dbg.h>
>> @@ -134,6 +135,58 @@ static const char *sd_cache_types[] = {
>>       "write back, no read (daft)"
>>  };
>>
>> +/* Relevant Structure and Function to be used to Retrieve
>> + * Caching Information from USB HDD - this is picked from
>> + * source code of 'hdparm'
>> + *
>> + *
>> + * Definitions and structures for use with SG_IO + ATA_16:
>> + * */
>> +#define SG_ATA_16               0x85
>> +#define SG_ATA_16_LEN           16
>> +
>> +#define ATA_OP_IDENTIFY              0xec
>> +
>> +/*
>> + * Some useful ATA register bits
>> + */
>> +enum {
>> +        ATA_USING_LBA           = (1 << 6),
>> +        ATA_STAT_DRQ            = (1 << 3),
>> +        ATA_STAT_ERR            = (1 << 0),
>> +};
>> +
>> +struct ata_lba_regs {
>> +        __u8    feat;
>> +        __u8    nsect;
>> +        __u8    lbal;
>> +        __u8    lbam;
>> +        __u8    lbah;
>> +};
>> +struct ata_tf {
>> +        __u8                    dev;
>> +        __u8                    command;
>> +        __u8                    error;
>> +        __u8                    status;
>> +        __u8                    is_lba48;
>> +        struct ata_lba_regs     lob;
>> +        struct ata_lba_regs     hob;
>> +};
>
> Don't these things already exist in some standard header file?  If not,
> shouldn't they be added in a more central location?
>
>> +__u64 tf_to_lba (struct ata_tf *tf)
>> +{
>> +        __u32 lba24, lbah;
>> +        __u64 lba64;
>> +
>> +        lba24 = (tf->lob.lbah << 16) | (tf->lob.lbam << 8) | (tf->lob.lbal);
>> +        if (tf->is_lba48)
>> +                lbah = (tf->hob.lbah << 16) | (tf->hob.lbam << 8) |
>> (tf->hob.lbal);
>> +        else
>> +                lbah = (tf->dev & 0x0f);
>> +        lba64 = (((__u64)lbah) << 24) | (__u64)lba24;
>> +        return lba64;
>> +}
>> +
>>  static ssize_t
>>  sd_store_cache_type(struct device *dev, struct device_attribute *attr,
>>                   const char *buf, size_t count)
>> @@ -1839,6 +1892,18 @@ sd_read_cache_type(struct scsi_disk *sdk
>>       int old_rcd = sdkp->RCD;
>>       int old_dpofua = sdkp->DPOFUA;
>>
>> +     struct ata_tf tf;
>> +     struct sg_io_hdr io_hdr;
>> +     unsigned char cdb[SG_ATA_16_LEN] = {0};
>> +     unsigned char sb[32] = {0};
>> +     unsigned char buf[512] = {0};
>
> Arrays generally should not have static initializers.  Also, a 512-byte
> array is too large to allocate on the stack.  And there's already a
> 512-byte array available -- it's named "buffer".
>
>> +     unsigned short wce_word = 0;
>
> There's no reason to initialize this variable.
>
>> +     void *data_cmd = buf;
>
> Why do you need to alias a perfectly good variable?
>
>> +
>> +     memset(cdb, 0, SG_ATA_16_LEN);
>> +     memset(&tf, 0, sizeof(struct ata_tf));
>> +     memset(&io_hdr, 0, sizeof(struct sg_io_hdr));
>
> There's no point initializing these things before you know that they
> will be used.
>
>> +
>>         first_len = 4;
>>         if (sdp->skip_ms_page_8) {
>>                 if (sdp->type == TYPE_RBC)
>> @@ -1961,7 +2026,6 @@ Page_found:
>>                                 sdkp->DPOFUA ? "supports DPO and FUA"
>>                                 : "doesn't support DPO or FUA");
>>
>> -             return;
>>       }
>>
>>  bad_sense:
>> @@ -1974,8 +2038,64 @@ bad_sense:
>>               sd_printk(KERN_ERR, sdkp, "Asking for cache data failed\n");
>>
>>  defaults:
>> -     sd_printk(KERN_ERR, sdkp, "Assuming drive cache: write through\n");
>> -     sdkp->WCE = 0;
>> +     if (sdkp->WCE)
>> +             return;
>> +     else {
>> +             sd_printk(KERN_ERR, sdkp, "Normal Routine Failed: Trying ATA_16\n");
>
> Instead of adding an awful lot of code inside an "else" clause, it
> would be better to put this into its own subroutine.
>
>> +
>> +             /* The below are necessary parameters which are to set - in order
>> +             to make use of ATA_OP_IDENTIFY command */
>> +             tf.lob.lbal = 0;
>> +             tf.lob.lbam = 0;
>> +             tf.lob.lbah = 0;
>> +             tf.lob.nsect = 1; //Number of Sectors to Read
>> +             tf.lob.feat = 0;
>> +
>> +             /* Command Descriptor Block For SCSI    */
>> +             cdb[0] = SG_ATA_16;
>> +             cdb[1] = 0x08;
>> +             cdb[2] = 0x0e;
>> +             cdb[6] = 0x01; //No. of Sectors To Read
>> +             cdb[13] = ATA_USING_LBA;
>> +             cdb[14] = ATA_OP_IDENTIFY;
>> +
>> +             io_hdr.cmd_len = SG_ATA_16_LEN;
>> +             io_hdr.interface_id = SG_INTERFACE_ID_ORIG;
>> +             io_hdr.mx_sb_len= sizeof(sb);
>> +             io_hdr.dxfer_direction = SG_DXFER_FROM_DEV;
>> +             io_hdr.dxfer_len = sizeof(buf);
>> +             io_hdr.dxferp = data_cmd;
>> +             io_hdr.cmdp = cdb;
>> +             io_hdr.sbp = sb;
>> +             io_hdr.pack_id = tf_to_lba(&tf);
>> +             io_hdr.timeout = 0;
>> +
>> +             if (!scsi_cmd_ioctl(sdkp->disk->queue, sdkp->disk,
>> O_RDONLY|O_NONBLOCK, SG_IO, &io_hdr))
>
> Do you really need to do an ioctl?  Why not call scsi_execute_req()
> directly?
>
>> +             {
>> +#if 0
>> +#define DUMP_BYTES_BUFFER(x...) printk( x )
>> +                     int i;
>> +                     for (i = 0; i < 32; i++)
>> +                             DUMP_BYTES_BUFFER(" %02x", sb[i]);
>> +                     DUMP_BYTES_BUFFER("\n");
>> +                     for (i = 0; i < 512; i++)
>> +                             DUMP_BYTES_BUFFER(" %02x", buf[i]);
>> +                     DUMP_BYTES_BUFFER("\n");
>> +                     printk(KERN_ERR"82 - [0x%x], 85 -
>> [0x%x]\n",((unsigned short*)data_cmd)[82],((unsigned
>> short*)data_cmd)[85]);
>> +#endif
>
> For the final patch submission, of course this section should be
> removed.
>
>> +                     /* '6th' Bit in Word 85 Corresponds to Write Cache being Enabled/disabled*/
>> +                     wce_word = le16_to_cpu(((unsigned short*)data_cmd)[85]);
>> +                     if (wce_word & 0x20) {
>> +                             sdkp->WCE = 1;
>> +                             sd_printk(KERN_NOTICE, sdkp, "Write Cache Enabled
>> after ATA_16 response\n");
>> +                     } else
>> +                             goto write_through;
>> +             } else {
>> +write_through:
>> +                     sd_printk(KERN_ERR, sdkp, "Assuming drive cache: write through\n");
>> +                     sdkp->WCE = 0;
>> +             }
>> +     }
>>       sdkp->RCD = 0;
>>       sdkp->DPOFUA = 0;
>>  }
>
> Besides the potential problems raised by other people, these structural
> weaknesses in the patch should be fixed.
>
> Alan Stern
>
>

Thanks Alan for the thorough review, I will consider all your points.
Also, will try and make use of scsi_execute_req() instead of ioctl().

Regards,
Amit Sahrawat