Patchwork [1/1] scsi: retrieve cache mode using ATA_16 if normal routine fails

login
register
mail settings
Submitter Amit Sahrawat
Date Feb. 3, 2012, 12:59 p.m.
Message ID <1328273954-5010-1-git-send-email-amit.sahrawat83@gmail.com>
Download mbox | patch
Permalink /patch/139364/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Amit Sahrawat - Feb. 3, 2012, 12:59 p.m.
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).
This results in a number of Filesystem corruptions, when the device
is unplugged abruptly.

So, in order to identify the devices correctly - give it
a last try using ATA_16 after failure from normal routine.

Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>

---
 drivers/ata/libata-scsi.c |   51 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/sd.c         |   17 +++++++++++++++
 include/linux/libata.h    |    3 ++
 3 files changed, 71 insertions(+), 0 deletions(-)
Sergei Shtylyov - Feb. 5, 2012, 12:05 p.m.
Hello.

On 03-02-2012 16:59, Amit Sahrawat wrote:

> It has been observed that a number of USB HDD's do not respond correctly

    Why are you working around this in libata-scsi.c then if it's USB HDD?

> 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).
> This results in a number of Filesystem corruptions, when the device
> is unplugged abruptly.

> So, in order to identify the devices correctly - give it
> a last try using ATA_16 after failure from normal routine.

    Shouldn't this be done in drivers/usb/storage somewhere?

> Signed-off-by: Amit Sahrawat<a.sahrawat@samsung.com>
> Signed-off-by: Namjae Jeon<namjae.jeon@samsung.com>

[...]

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 508a60b..d5b00e6 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -562,6 +562,57 @@ error:
>   }
>
>   /**
> + *      ata_get_cachestatus - Handler for to get WriteCache Status
> + *                      using ATA_16 scsi command
> + *      @scsidev: Device to which we are issuing command
> + *
> + *      LOCKING:
> + *      Defined by the SCSI layer.  We don't really care.
> + *
> + *      RETURNS:
> + *      '0' for Write Cache Disabled and on any error.
> + *      '1' for Write Cache enabled
> + */
> +int ata_get_cachestatus(struct scsi_device *scsidev)
> +{
> +	int rc = 0;
> +	u8 scsi_cmd[MAX_COMMAND_SIZE] = {0};
> +	u8 *sensebuf = NULL, *argbuf = NULL;

    No need to initialize them.

> +	enum dma_data_direction data_dir;
> +
> +	sensebuf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
> +	if (!sensebuf)
> +		return rc;
> +
> +	argbuf = kmalloc(ATA_SECT_SIZE, GFP_KERNEL);
> +	if (argbuf == NULL)

    Could you use an uniform NULL-check, either '!x' or 'x == NULL'?

> +		goto error;
> +
> +	scsi_cmd[0] = ATA_16;
> +	scsi_cmd[1]  = (4<<  1); /* PIO Data-in */
> +	scsi_cmd[2]  = 0x0e;     /* no off.line or cc, read from dev,
> +				block count in sector count field */
> +	data_dir = DMA_FROM_DEVICE;
> +	scsi_cmd[14] = ATA_IDENTIFY_DEV;
> +
> +	if (!scsi_execute(scsidev, scsi_cmd, data_dir, argbuf, ATA_SECT_SIZE,
> +				sensebuf, (10*HZ), 5, 0, NULL)) {
> +		/*
> +		 * '6th' Bit in Word 85 Corresponds to Write Cache

    No need for apostrophes here.

> +		 * being Enabled/disabled, Word 85 represnets the
> +		 * features supported
> +		 */
> +		if (le16_to_cpu(((unsigned short *)argbuf)[85])&  0x20)
> +			rc = 1;
> +	}
> +
> +error:
> +	kfree(sensebuf);
> +	kfree(argbuf);
> +	return rc;
> +}
> +
> +/**
>    *	ata_task_ioctl - Handler for HDIO_DRIVE_TASK ioctl
>    *	@scsidev: Device to which we are issuing command
>    *	@arg: User provided data for issuing command
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index c691fb5..a6b887d 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -50,6 +50,11 @@
>   #include<linux/string_helpers.h>
>   #include<linux/async.h>
>   #include<linux/slab.h>
> +
> +#ifdef CONFIG_ATA

    #ifdef not necessary here.

> +#include<linux/libata.h>
> +#endif
> +
>   #include<linux/pm_runtime.h>
>   #include<asm/uaccess.h>
>   #include<asm/unaligned.h>
> @@ -2129,7 +2134,11 @@ sd_read_cache_type(struct scsi_disk *sdkp, unsigned char *buffer)
>   		if (modepage == 0x3F) {
>   			sd_printk(KERN_ERR, sdkp, "No Caching mode page "
>   				  "present\n");
> +#ifdef CONFIG_ATA
> +			goto WCE_USING_ATA;

    Yikes.

> +#else
>   			goto defaults;
> +#endif
>   		} else if ((buffer[offset]&  0x3f) != modepage) {
>   			sd_printk(KERN_ERR, sdkp, "Got wrong page\n");
>   			goto defaults;
> @@ -2149,6 +2158,14 @@ sd_read_cache_type(struct scsi_disk *sdkp, unsigned char *buffer)
>   				  "Uses READ/WRITE(6), disabling FUA\n");
>   			sdkp->DPOFUA = 0;
>   		}
> +#ifdef CONFIG_ATA
> +WCE_USING_ATA:
> +		if (!sdp->removable && !sdkp->WCE) {
> +			sd_printk(KERN_NOTICE, sdkp, "Try to check write cache "
> +				"enable/disable using ATA command\n");
> +			sdkp->WCE = ata_get_cachestatus(sdp);
> +		}
> +#endif
>
>   		if (sdkp->first_scan || old_wce != sdkp->WCE ||
>   		    old_rcd != sdkp->RCD || old_dpofua != sdkp->DPOFUA)
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index cafc09a..33fc73f 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -84,6 +84,8 @@
>   	}							\
>   })
>
> +#define ATA_IDENTIFY_DEV	0xEC
> +

    This is alerady #defined in <linux/ata.h> as ATA_CMD_ID_ATA.

MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amit Sahrawat - Feb. 6, 2012, 5:40 a.m.
Hi Sergei,
Thanks for your review comments.

On Sun, Feb 5, 2012 at 5:35 PM, Sergei Shtylyov <sshtylyov@mvista.com> wrote:
> Hello.
>
>
> On 03-02-2012 16:59, Amit Sahrawat wrote:
>
>> It has been observed that a number of USB HDD's do not respond correctly
>
>
>   Why are you working around this in libata-scsi.c then if it's USB HDD?
There has been a history to this problem. It has been a long pending
problem with SCSI layer and USB HDD. We observed when we encountered
various Filesystem corruption issues on USB HDD and on analysis - we
found that there is no SYNCHRONIZE_CACHE command going to a number of
USB HDD even though - they do seem to possess WRITE CACHE.
So, drivers/scsi/sd.c - was where we found that WCE is being set for
these kind of devices which will in-turn take care of setting the
correct 'cache_type - Writeback' and also take care of proper working
(by managing correct order in requests queue).
We first did a change by introducing  - mechanism to retrieve cache
bit in sd_read_cache_type, alongwith this we provided an interface
using 'syfs' if we can change the cache type from that place.
But, we received review comments in this regards - that the layer on
which we are doing changes is not the proper place(and also ATA_16
seems to crash some old HDD's, and were advised to look out for
changes in libata-scsi  instead).(Original link -
http://lkml.org/lkml/2011/12/12/164)

So, we went for changes in libata-scsi.

Please let me know in case more information is needed in this regards,
or may be if you could provide more inputs to these sort of changes.
>
>
>> 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).
>> This results in a number of Filesystem corruptions, when the device
>> is unplugged abruptly.
>
>
>> So, in order to identify the devices correctly - give it
>> a last try using ATA_16 after failure from normal routine.
>
>
>   Shouldn't this be done in drivers/usb/storage somewhere?
>
>
>> Signed-off-by: Amit Sahrawat<a.sahrawat@samsung.com>
>> Signed-off-by: Namjae Jeon<namjae.jeon@samsung.com>
>
>
> [...]
>
>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 508a60b..d5b00e6 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -562,6 +562,57 @@ error:
>>  }
>>
>>  /**
>> + *      ata_get_cachestatus - Handler for to get WriteCache Status
>> + *                      using ATA_16 scsi command
>> + *      @scsidev: Device to which we are issuing command
>> + *
>> + *      LOCKING:
>> + *      Defined by the SCSI layer.  We don't really care.
>> + *
>> + *      RETURNS:
>> + *      '0' for Write Cache Disabled and on any error.
>> + *      '1' for Write Cache enabled
>> + */
>> +int ata_get_cachestatus(struct scsi_device *scsidev)
>> +{
>> +       int rc = 0;
>> +       u8 scsi_cmd[MAX_COMMAND_SIZE] = {0};
>> +       u8 *sensebuf = NULL, *argbuf = NULL;
>
>
>   No need to initialize them.
Tried to maintain the uniformity in the code for this file, at other
places it is being done like.
>
>
>> +       enum dma_data_direction data_dir;
>> +
>> +       sensebuf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
>> +       if (!sensebuf)
>> +               return rc;
>> +
>> +       argbuf = kmalloc(ATA_SECT_SIZE, GFP_KERNEL);
>> +       if (argbuf == NULL)
>
>
>   Could you use an uniform NULL-check, either '!x' or 'x == NULL'?
Agreed, need to stick with one type of comparision.
>
>
>> +               goto error;
>> +
>> +       scsi_cmd[0] = ATA_16;
>> +       scsi_cmd[1]  = (4<<  1); /* PIO Data-in */
>> +       scsi_cmd[2]  = 0x0e;     /* no off.line or cc, read from dev,
>> +                               block count in sector count field */
>> +       data_dir = DMA_FROM_DEVICE;
>> +       scsi_cmd[14] = ATA_IDENTIFY_DEV;
>> +
>> +       if (!scsi_execute(scsidev, scsi_cmd, data_dir, argbuf,
>> ATA_SECT_SIZE,
>> +                               sensebuf, (10*HZ), 5, 0, NULL)) {
>> +               /*
>> +                * '6th' Bit in Word 85 Corresponds to Write Cache
>
>
>   No need for apostrophes here.
Tried to highlight the 6th bit, if this is not the convention, I will
remove this.
>
>> +                * being Enabled/disabled, Word 85 represnets the
>> +                * features supported
>> +                */
>> +               if (le16_to_cpu(((unsigned short *)argbuf)[85])&  0x20)
>>
>> +                       rc = 1;
>> +       }
>> +
>> +error:
>> +       kfree(sensebuf);
>> +       kfree(argbuf);
>> +       return rc;
>> +}
>> +
>> +/**
>>   *    ata_task_ioctl - Handler for HDIO_DRIVE_TASK ioctl
>>   *    @scsidev: Device to which we are issuing command
>>   *    @arg: User provided data for issuing command
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index c691fb5..a6b887d 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -50,6 +50,11 @@
>>  #include<linux/string_helpers.h>
>>  #include<linux/async.h>
>>  #include<linux/slab.h>
>> +
>> +#ifdef CONFIG_ATA
>
>
>   #ifdef not necessary here.
Yes, but just wanted to specifiy for code readability, as there is no
purpose to include - libata header in sd.c
>
>
>> +#include<linux/libata.h>
>> +#endif
>> +
>>  #include<linux/pm_runtime.h>
>>  #include<asm/uaccess.h>
>>  #include<asm/unaligned.h>
>> @@ -2129,7 +2134,11 @@ sd_read_cache_type(struct scsi_disk *sdkp, unsigned
>> char *buffer)
>>                if (modepage == 0x3F) {
>>                        sd_printk(KERN_ERR, sdkp, "No Caching mode page "
>>                                  "present\n");
>> +#ifdef CONFIG_ATA
>> +                       goto WCE_USING_ATA;
>
>
>   Yikes.
>
>
>> +#else
>>                        goto defaults;
>> +#endif
>>                } else if ((buffer[offset]&  0x3f) != modepage) {
>>                        sd_printk(KERN_ERR, sdkp, "Got wrong page\n");
>>                        goto defaults;
>> @@ -2149,6 +2158,14 @@ sd_read_cache_type(struct scsi_disk *sdkp, unsigned
>> char *buffer)
>>                                  "Uses READ/WRITE(6), disabling FUA\n");
>>                        sdkp->DPOFUA = 0;
>>                }
>> +#ifdef CONFIG_ATA
>> +WCE_USING_ATA:
>> +               if (!sdp->removable && !sdkp->WCE) {
>> +                       sd_printk(KERN_NOTICE, sdkp, "Try to check write
>> cache "
>> +                               "enable/disable using ATA command\n");
>> +                       sdkp->WCE = ata_get_cachestatus(sdp);
>> +               }
>> +#endif
>>
>>                if (sdkp->first_scan || old_wce != sdkp->WCE ||
>>                    old_rcd != sdkp->RCD || old_dpofua != sdkp->DPOFUA)
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index cafc09a..33fc73f 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -84,6 +84,8 @@
>>        }                                                       \
>>  })
>>
>> +#define ATA_IDENTIFY_DEV       0xEC
>> +
>
>
>   This is alerady #defined in <linux/ata.h> as ATA_CMD_ID_ATA.
Yes, will include that instead of redifining.
>
> MBR, Sergei

Please share your inputs to this.

Thanks & Regards,
Amit Sahrawat
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 508a60b..d5b00e6 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -562,6 +562,57 @@  error:
 }
 
 /**
+ *      ata_get_cachestatus - Handler for to get WriteCache Status
+ *                      using ATA_16 scsi command
+ *      @scsidev: Device to which we are issuing command
+ *
+ *      LOCKING:
+ *      Defined by the SCSI layer.  We don't really care.
+ *
+ *      RETURNS:
+ *      '0' for Write Cache Disabled and on any error.
+ *      '1' for Write Cache enabled
+ */
+int ata_get_cachestatus(struct scsi_device *scsidev)
+{
+	int rc = 0;
+	u8 scsi_cmd[MAX_COMMAND_SIZE] = {0};
+	u8 *sensebuf = NULL, *argbuf = NULL;
+	enum dma_data_direction data_dir;
+
+	sensebuf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
+	if (!sensebuf)
+		return rc;
+
+	argbuf = kmalloc(ATA_SECT_SIZE, GFP_KERNEL);
+	if (argbuf == NULL)
+		goto error;
+
+	scsi_cmd[0] = ATA_16;
+	scsi_cmd[1]  = (4 << 1); /* PIO Data-in */
+	scsi_cmd[2]  = 0x0e;     /* no off.line or cc, read from dev,
+				block count in sector count field */
+	data_dir = DMA_FROM_DEVICE;
+	scsi_cmd[14] = ATA_IDENTIFY_DEV;
+
+	if (!scsi_execute(scsidev, scsi_cmd, data_dir, argbuf, ATA_SECT_SIZE,
+				sensebuf, (10*HZ), 5, 0, NULL)) {
+		/*
+		 * '6th' Bit in Word 85 Corresponds to Write Cache
+		 * being Enabled/disabled, Word 85 represnets the
+		 * features supported
+		 */
+		if (le16_to_cpu(((unsigned short *)argbuf)[85]) & 0x20)
+			rc = 1;
+	}
+
+error:
+	kfree(sensebuf);
+	kfree(argbuf);
+	return rc;
+}
+
+/**
  *	ata_task_ioctl - Handler for HDIO_DRIVE_TASK ioctl
  *	@scsidev: Device to which we are issuing command
  *	@arg: User provided data for issuing command
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index c691fb5..a6b887d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -50,6 +50,11 @@ 
 #include <linux/string_helpers.h>
 #include <linux/async.h>
 #include <linux/slab.h>
+
+#ifdef CONFIG_ATA
+#include <linux/libata.h>
+#endif
+
 #include <linux/pm_runtime.h>
 #include <asm/uaccess.h>
 #include <asm/unaligned.h>
@@ -2129,7 +2134,11 @@  sd_read_cache_type(struct scsi_disk *sdkp, unsigned char *buffer)
 		if (modepage == 0x3F) {
 			sd_printk(KERN_ERR, sdkp, "No Caching mode page "
 				  "present\n");
+#ifdef CONFIG_ATA
+			goto WCE_USING_ATA;
+#else
 			goto defaults;
+#endif
 		} else if ((buffer[offset] & 0x3f) != modepage) {
 			sd_printk(KERN_ERR, sdkp, "Got wrong page\n");
 			goto defaults;
@@ -2149,6 +2158,14 @@  sd_read_cache_type(struct scsi_disk *sdkp, unsigned char *buffer)
 				  "Uses READ/WRITE(6), disabling FUA\n");
 			sdkp->DPOFUA = 0;
 		}
+#ifdef CONFIG_ATA
+WCE_USING_ATA:
+		if (!sdp->removable && !sdkp->WCE) {
+			sd_printk(KERN_NOTICE, sdkp, "Try to check write cache "
+				"enable/disable using ATA command\n");
+			sdkp->WCE = ata_get_cachestatus(sdp);
+		}
+#endif
 
 		if (sdkp->first_scan || old_wce != sdkp->WCE ||
 		    old_rcd != sdkp->RCD || old_dpofua != sdkp->DPOFUA)
diff --git a/include/linux/libata.h b/include/linux/libata.h
index cafc09a..33fc73f 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -84,6 +84,8 @@ 
 	}							\
 })
 
+#define ATA_IDENTIFY_DEV	0xEC
+
 /* NEW: debug levels */
 #define HAVE_LIBATA_MSG 1
 
@@ -990,6 +992,7 @@  extern void ata_host_init(struct ata_host *, struct device *,
 			  unsigned long, struct ata_port_operations *);
 extern int ata_scsi_detect(struct scsi_host_template *sht);
 extern int ata_scsi_ioctl(struct scsi_device *dev, int cmd, void __user *arg);
+extern int ata_get_cachestatus(struct scsi_device *scsidev);
 extern int ata_scsi_queuecmd(struct Scsi_Host *h, struct scsi_cmnd *cmd);
 extern int ata_sas_scsi_ioctl(struct ata_port *ap, struct scsi_device *dev,
 			    int cmd, void __user *arg);