[PATCH 1/3] scsi: set to WCE if usb cache quirk is present.

[PATCH 1/3] scsi: set to WCE if usb cache quirk is present.

[Namjae Jeon]

Make use of USB quirk method to identify such HDD while reading the cache status in sd_probe(). If cache quirk is present for the HDD, lets assume that cache is enabled and make WCE bit equal to 1.

Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
Signed-off-by: Pankaj Kumar <pankaj.km@samsung.com>
Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
---
 drivers/scsi/sd.c          |    9 +++++++--
 include/scsi/scsi_device.h |    1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6f0a4c6..33faf6d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2259,8 +2259,13 @@ 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 (sdp->wce_quirk) {
+		sdkp->WCE = 1;
+		sd_printk(KERN_NOTICE, sdkp, "Assuming drive cache write back\n");
+	} else {
+		sd_printk(KERN_ERR, sdkp, "Assuming drive cache: write through\n");
+		sdkp->WCE = 0;
+	}
 	sdkp->RCD = 0;
 	sdkp->DPOFUA = 0;
 }
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 6efb2e1..8be0247 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -152,6 +152,7 @@ struct scsi_device {
 	unsigned no_read_disc_info:1;	/* Avoid READ_DISC_INFO cmds */
 	unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
 	unsigned is_visible:1;	/* is the device visible in sysfs */
+	unsigned wce_quirk:1; /* cache read page is not available */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */
-- 
1.7.9.5

Re: [PATCH 1/3] scsi: set to WCE if usb cache quirk is present.

[Alan Stern]

On Wed, 6 Jun 2012, Namjae Jeon wrote:

> Make use of USB quirk method to identify such HDD while reading the cache status in sd_probe(). If cache quirk is present for the HDD, lets assume that cache is enabled and make WCE bit equal to 1.

Your lines should be wrapped after 72 columns or so.

Calling this a "quirk" sounds strange.  Really, your new flag indicates 
that WCE should default to "on" instead of "off".  How about calling it 
"wce_default_on" instead of "wce_quirk"?

> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -152,6 +152,7 @@ struct scsi_device {
>  	unsigned no_read_disc_info:1;	/* Avoid READ_DISC_INFO cmds */
>  	unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
>  	unsigned is_visible:1;	/* is the device visible in sysfs */
> +	unsigned wce_quirk:1; /* cache read page is not available */

The comment is wrong.  There's no need for a quirk to indicate the
cache page is unavailable; the driver already knows when the page is
unavailable.  Rather, the quirk indicates that WCE should default to
"on".

Alan Stern

Re: [PATCH 1/3] scsi: set to WCE if usb cache quirk is present.

[Namjae Jeon]

Hi Alan.

I will send patch v2 after chaing code.
Thanks for your review.

2012/6/7, Alan Stern <stern@rowland.harvard.edu>:
> On Wed, 6 Jun 2012, Namjae Jeon wrote:
>
>> Make use of USB quirk method to identify such HDD while reading the cache
>> status in sd_probe(). If cache quirk is present for the HDD, lets assume
>> that cache is enabled and make WCE bit equal to 1.
>
> Your lines should be wrapped after 72 columns or so.
>
> Calling this a "quirk" sounds strange.  Really, your new flag indicates
> that WCE should default to "on" instead of "off".  How about calling it
> "wce_default_on" instead of "wce_quirk"?
>
>> --- a/include/scsi/scsi_device.h
>> +++ b/include/scsi/scsi_device.h
>> @@ -152,6 +152,7 @@ struct scsi_device {
>>  	unsigned no_read_disc_info:1;	/* Avoid READ_DISC_INFO cmds */
>>  	unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
>>  	unsigned is_visible:1;	/* is the device visible in sysfs */
>> +	unsigned wce_quirk:1; /* cache read page is not available */
>
> The comment is wrong.  There's no need for a quirk to indicate the
> cache page is unavailable; the driver already knows when the page is
> unavailable.  Rather, the quirk indicates that WCE should default to
> "on".
>
> Alan Stern
>
>

Re: [PATCH 1/3] scsi: set to WCE if usb cache quirk is present.

[Sergei Shtylyov]

Hello.

On 06-06-2012 16:20, Namjae Jeon wrote:

> Make use of USB quirk method to identify such HDD while reading the cache status in sd_probe(). If cache quirk is present for the HDD, lets assume that cache is enabled and make WCE bit equal to 1.

> Signed-off-by: Namjae Jeon<namjae.jeon@samsung.com>
> Signed-off-by: Pankaj Kumar<pankaj.km@samsung.com>
> Signed-off-by: Amit Sahrawat<a.sahrawat@samsung.com>
> ---
>   drivers/scsi/sd.c          |    9 +++++++--
>   include/scsi/scsi_device.h |    1 +
>   2 files changed, 8 insertions(+), 2 deletions(-)

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 6f0a4c6..33faf6d 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2259,8 +2259,13 @@ 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 (sdp->wce_quirk) {
> +		sdkp->WCE = 1;
> +		sd_printk(KERN_NOTICE, sdkp, "Assuming drive cache write back\n");

    You forgot colon after "cache".

> +	} else {
> +		sd_printk(KERN_ERR, sdkp, "Assuming drive cache: write through\n");
> +		sdkp->WCE = 0;

    It makes sense to do assignments and printk() in the same order in both 
branches.

> +	}
>   	sdkp->RCD = 0;
>   	sdkp->DPOFUA = 0;
>   }

WBR, Sergei

Re: [PATCH 1/3] scsi: set to WCE if usb cache quirk is present.

[Namjae Jeon]

Hi. Sergei.

Good point. I will update.
Thanks for your review.

2012/6/7, Sergei Shtylyov <sshtylyov@mvista.com>:
> Hello.
>
> On 06-06-2012 16:20, Namjae Jeon wrote:
>
>> Make use of USB quirk method to identify such HDD while reading the cache
>> status in sd_probe(). If cache quirk is present for the HDD, lets assume
>> that cache is enabled and make WCE bit equal to 1.
>
>> Signed-off-by: Namjae Jeon<namjae.jeon@samsung.com>
>> Signed-off-by: Pankaj Kumar<pankaj.km@samsung.com>
>> Signed-off-by: Amit Sahrawat<a.sahrawat@samsung.com>
>> ---
>>   drivers/scsi/sd.c          |    9 +++++++--
>>   include/scsi/scsi_device.h |    1 +
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 6f0a4c6..33faf6d 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -2259,8 +2259,13 @@ 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 (sdp->wce_quirk) {
>> +		sdkp->WCE = 1;
>> +		sd_printk(KERN_NOTICE, sdkp, "Assuming drive cache write back\n");
>
>     You forgot colon after "cache".
>
>> +	} else {
>> +		sd_printk(KERN_ERR, sdkp, "Assuming drive cache: write through\n");
>> +		sdkp->WCE = 0;
>
>     It makes sense to do assignments and printk() in the same order in both
>
> branches.
>
>> +	}
>>   	sdkp->RCD = 0;
>>   	sdkp->DPOFUA = 0;
>>   }
>
> WBR, Sergei
>