RE: ata: HDIO_DRIVE_* ioctl() Linux 3.9 regression

RE: ata: HDIO_DRIVE_* ioctl() Linux 3.9 regression

[Ronald]

In reply to [1]: I have the same issue. Git bisect took 50+ rebuilds xD

Smartd does not work anymore since 84a9a8cd9 ([libata] Set proper SK
when CK_COND is set.).

I hope I'm not stepping on anyone's toe's by chosing the same title.
I'm not subscribed to this list.

Just wanted to add a 'me2'

[1] http://www.spinics.net/lists/linux-ide/msg45268.html

Re: ata: HDIO_DRIVE_* ioctl() Linux 3.9 regression

[Krzysztof Mazur]

On Mon, Mar 25, 2013 at 06:26:50PM +0100, Ronald wrote:
> In reply to [1]: I have the same issue. Git bisect took 50+ rebuilds xD
> 
> Smartd does not work anymore since 84a9a8cd9 ([libata] Set proper SK
> when CK_COND is set.).
> 
> I hope I'm not stepping on anyone's toe's by chosing the same title.
> I'm not subscribed to this list.
> 
> Just wanted to add a 'me2'
> 
> [1] http://www.spinics.net/lists/linux-ide/msg45268.html

It seems that the SAM_STAT_CHECK_CONDITION is not cleared
causing -EIO, because that patch modified sensebuf and
the check for clearing SAM_STAT_CHECK_CONDITION is no longer valid.

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 318b413..ff44787 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -532,8 +532,8 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 			struct scsi_sense_hdr sshdr;
 			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
 					     &sshdr);
-			if (sshdr.sense_key == 0 &&
-			    sshdr.asc == 0 && sshdr.ascq == 0)
+			if (sshdr.sense_key == RECOVERED_ERROR &&
+			    sshdr.asc == 0 && sshdr.ascq == 0x1d)
 				cmd_result &= ~SAM_STAT_CHECK_CONDITION;
 		}
 
@@ -618,8 +618,8 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
 			struct scsi_sense_hdr sshdr;
 			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
 						&sshdr);
-			if (sshdr.sense_key == 0 &&
-				sshdr.asc == 0 && sshdr.ascq == 0)
+			if (sshdr.sense_key == RECOVERED_ERROR &&
+			    sshdr.asc == 0 && sshdr.ascq == 0x1d)
 				cmd_result &= ~SAM_STAT_CHECK_CONDITION;
 		}
 
Krzysiek

Re: ata: HDIO_DRIVE_* ioctl() Linux 3.9 regression

[Jeff Garzik]

On 03/27/2013 08:51 AM, Krzysztof Mazur wrote:
> On Mon, Mar 25, 2013 at 06:26:50PM +0100, Ronald wrote:
>> In reply to [1]: I have the same issue. Git bisect took 50+ rebuilds xD
>>
>> Smartd does not work anymore since 84a9a8cd9 ([libata] Set proper SK
>> when CK_COND is set.).
>>
>> I hope I'm not stepping on anyone's toe's by chosing the same title.
>> I'm not subscribed to this list.
>>
>> Just wanted to add a 'me2'
>>
>> [1] http://www.spinics.net/lists/linux-ide/msg45268.html
>
> It seems that the SAM_STAT_CHECK_CONDITION is not cleared
> causing -EIO, because that patch modified sensebuf and
> the check for clearing SAM_STAT_CHECK_CONDITION is no longer valid.
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 318b413..ff44787 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -532,8 +532,8 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
>   			struct scsi_sense_hdr sshdr;
>   			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
>   					     &sshdr);
> -			if (sshdr.sense_key == 0 &&
> -			    sshdr.asc == 0 && sshdr.ascq == 0)
> +			if (sshdr.sense_key == RECOVERED_ERROR &&
> +			    sshdr.asc == 0 && sshdr.ascq == 0x1d)
>   				cmd_result &= ~SAM_STAT_CHECK_CONDITION;
>   		}
>
> @@ -618,8 +618,8 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
>   			struct scsi_sense_hdr sshdr;
>   			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
>   						&sshdr);
> -			if (sshdr.sense_key == 0 &&
> -				sshdr.asc == 0 && sshdr.ascq == 0)
> +			if (sshdr.sense_key == RECOVERED_ERROR &&
> +			    sshdr.asc == 0 && sshdr.ascq == 0x1d)
>   				cmd_result &= ~SAM_STAT_CHECK_CONDITION;

applied

[PATCH] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Gwendal Grignou]

commit 84a9a8cd9d0aa93c17e5815ab8a9cc4c0a765c63 changed the sense key
used for returning task registers, but HDIO_DRIVE_CMD ioctl was
not changed accordingly.

Tested: check that SMART ENABLE sent using HDIO_DRIVE_CMD returns 0
instead of EIO.

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 318b413..e05bf4c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -532,8 +532,8 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 			struct scsi_sense_hdr sshdr;
 			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
 					     &sshdr);
-			if (sshdr.sense_key == 0 &&
-			    sshdr.asc == 0 && sshdr.ascq == 0)
+			if (sshdr.sense_key == RECOVERED_ERROR &&
+			    sshdr.asc == 0 && sshdr.ascq == 0x1D)
 				cmd_result &= ~SAM_STAT_CHECK_CONDITION;
 		}
 
-- 
1.8.1.3

Re: [PATCH] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Krzysztof Mazur]

On Fri, Mar 29, 2013 at 08:26:41AM -0700, Gwendal Grignou wrote:
> commit 84a9a8cd9d0aa93c17e5815ab8a9cc4c0a765c63 changed the sense key
> used for returning task registers, but HDIO_DRIVE_CMD ioctl was
> not changed accordingly.
> 
> Tested: check that SMART ENABLE sent using HDIO_DRIVE_CMD returns 0
> instead of EIO.
> 
> Signed-off-by: Gwendal Grignou <gwendal@google.com>
> ---
>  drivers/ata/libata-scsi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 318b413..e05bf4c 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -532,8 +532,8 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
>  			struct scsi_sense_hdr sshdr;
>  			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
>  					     &sshdr);
> -			if (sshdr.sense_key == 0 &&
> -			    sshdr.asc == 0 && sshdr.ascq == 0)
> +			if (sshdr.sense_key == RECOVERED_ERROR &&
> +			    sshdr.asc == 0 && sshdr.ascq == 0x1D)
>  				cmd_result &= ~SAM_STAT_CHECK_CONDITION;
>  		}
>  
> -- 
> 1.8.1.3

I did not test your patch, but I think that you missed the second
test in ata_task_ioctl() and the kernel will still return -EIO
in case of HDIO_DRIVE_TASK (0x31e) ioctl.
See http://marc.info/?l=linux-kernel&m=136438868606981&w=2.
The version I've sent fixes the problem for me.

Krzysiek

Re: [PATCH] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Gwendal Grignou]

Yours work.

On Fri, Mar 29, 2013 at 9:10 AM, Krzysztof Mazur <krzysiek@podlesie.net> wrote:
> On Fri, Mar 29, 2013 at 08:26:41AM -0700, Gwendal Grignou wrote:
>> commit 84a9a8cd9d0aa93c17e5815ab8a9cc4c0a765c63 changed the sense key
>> used for returning task registers, but HDIO_DRIVE_CMD ioctl was
>> not changed accordingly.
>>
>> Tested: check that SMART ENABLE sent using HDIO_DRIVE_CMD returns 0
>> instead of EIO.
>>
>> Signed-off-by: Gwendal Grignou <gwendal@google.com>
>> ---
>>  drivers/ata/libata-scsi.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 318b413..e05bf4c 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -532,8 +532,8 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
>>                       struct scsi_sense_hdr sshdr;
>>                       scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
>>                                            &sshdr);
>> -                     if (sshdr.sense_key == 0 &&
>> -                         sshdr.asc == 0 && sshdr.ascq == 0)
>> +                     if (sshdr.sense_key == RECOVERED_ERROR &&
>> +                         sshdr.asc == 0 && sshdr.ascq == 0x1D)
>>                               cmd_result &= ~SAM_STAT_CHECK_CONDITION;
>>               }
>>
>> --
>> 1.8.1.3
>
> I did not test your patch, but I think that you missed the second
> test in ata_task_ioctl() and the kernel will still return -EIO
> in case of HDIO_DRIVE_TASK (0x31e) ioctl.
> See http://marc.info/?l=linux-kernel&m=136438868606981&w=2.
> The version I've sent fixes the problem for me.
>
> Krzysiek

[PATCH v2] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Gwendal Grignou]

commit 84a9a8cd9d0aa93c17e5815ab8a9cc4c0a765c63 changed the sense key
used for returning task registers, but HDIO_DRIVE_CMD ioctl was
not changed accordingly.

Tested: check that SMART ENABLE sent using HDIO_DRIVE_CMD returns 0
instead of EIO.

Change-Id: I84dccd3febb0467a83a39e55ecfdaaa9686332cd

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-scsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 318b413..5eae74b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -532,8 +532,8 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 			struct scsi_sense_hdr sshdr;
 			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
 					     &sshdr);
-			if (sshdr.sense_key == 0 &&
-			    sshdr.asc == 0 && sshdr.ascq == 0)
+			if (sshdr.sense_key == RECOVERED_ERROR &&
+			    sshdr.asc == 0 && sshdr.ascq == 0x1D)
 				cmd_result &= ~SAM_STAT_CHECK_CONDITION;
 		}
 
@@ -618,8 +618,8 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
 			struct scsi_sense_hdr sshdr;
 			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
 						&sshdr);
-			if (sshdr.sense_key == 0 &&
-				sshdr.asc == 0 && sshdr.ascq == 0)
+			if (sshdr.sense_key == RECOVERED_ERROR &&
+			    sshdr.asc == 0 && sshdr.ascq == 0x1D)
 				cmd_result &= ~SAM_STAT_CHECK_CONDITION;
 		}
 
-- 
1.8.1.3

[PATCH v2] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Gwendal Grignou]

commit 84a9a8cd9d0aa93c17e5815ab8a9cc4c0a765c63 changed the sense key
used for returning task registers, but HDIO_DRIVE_CMD ioctl was
not changed accordingly.

Tested: check that SMART ENABLE sent using HDIO_DRIVE_CMD returns 0
instead of EIO.

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-scsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 318b413..5eae74b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -532,8 +532,8 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 			struct scsi_sense_hdr sshdr;
 			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
 					     &sshdr);
-			if (sshdr.sense_key == 0 &&
-			    sshdr.asc == 0 && sshdr.ascq == 0)
+			if (sshdr.sense_key == RECOVERED_ERROR &&
+			    sshdr.asc == 0 && sshdr.ascq == 0x1D)
 				cmd_result &= ~SAM_STAT_CHECK_CONDITION;
 		}
 
@@ -618,8 +618,8 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
 			struct scsi_sense_hdr sshdr;
 			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
 						&sshdr);
-			if (sshdr.sense_key == 0 &&
-				sshdr.asc == 0 && sshdr.ascq == 0)
+			if (sshdr.sense_key == RECOVERED_ERROR &&
+			    sshdr.asc == 0 && sshdr.ascq == 0x1D)
 				cmd_result &= ~SAM_STAT_CHECK_CONDITION;
 		}
 
-- 
1.8.1.3

Re: [PATCH v2] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Krzysztof Mazur]

On Fri, Mar 29, 2013 at 10:12:46AM -0700, Gwendal Grignou wrote:
> commit 84a9a8cd9d0aa93c17e5815ab8a9cc4c0a765c63 changed the sense key
> used for returning task registers, but HDIO_DRIVE_CMD ioctl was
> not changed accordingly.
> 
> Tested: check that SMART ENABLE sent using HDIO_DRIVE_CMD returns 0
> instead of EIO.
> 
> Signed-off-by: Gwendal Grignou <gwendal@google.com>

Works for me. If you like you can add:

Reported-and-tested-by: Krzysztof Mazur <krzysiek@podlesie.net>

Krzysiek

Re: [PATCH v2] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Sergei Shtylyov]

Hello.

On 03/29/2013 08:12 PM, Gwendal Grignou wrote:

> commit 84a9a8cd9d0aa93c17e5815ab8a9cc4c0a765c63

    Please, also specify that commit's summary line in parens
(or however you like).

>   changed the sense key
> used for returning task registers, but HDIO_DRIVE_CMD ioctl was
> not changed accordingly.
>
> Tested: check that SMART ENABLE sent using HDIO_DRIVE_CMD returns 0
> instead of EIO.
>
> Signed-off-by: Gwendal Grignou <gwendal@google.com>
>

MBR, Sergei

[PATCH v3] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Gwendal Grignou]

commit 84a9a8 "Set proper SK when CK_COND is set" changed the sense key
used for returning task registers, but HDIO_DRIVE_CMD ioctl was
not changed accordingly.

Tested: check that SMART ENABLE sent using HDIO_DRIVE_CMD returns 0
instead of EIO.

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-scsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 318b413..5eae74b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -532,8 +532,8 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 			struct scsi_sense_hdr sshdr;
 			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
 					     &sshdr);
-			if (sshdr.sense_key == 0 &&
-			    sshdr.asc == 0 && sshdr.ascq == 0)
+			if (sshdr.sense_key == RECOVERED_ERROR &&
+			    sshdr.asc == 0 && sshdr.ascq == 0x1D)
 				cmd_result &= ~SAM_STAT_CHECK_CONDITION;
 		}
 
@@ -618,8 +618,8 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
 			struct scsi_sense_hdr sshdr;
 			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
 						&sshdr);
-			if (sshdr.sense_key == 0 &&
-				sshdr.asc == 0 && sshdr.ascq == 0)
+			if (sshdr.sense_key == RECOVERED_ERROR &&
+			    sshdr.asc == 0 && sshdr.ascq == 0x1D)
 				cmd_result &= ~SAM_STAT_CHECK_CONDITION;
 		}
 
-- 
1.8.1.3

Re: ata: HDIO_DRIVE_* ioctl() Linux 3.9 regression

[Gwendal Grignou]

Ronald [and other],
Sorry for the bug I introduced. I just send a mail with a patch
"[PATCH] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check" that
addresses the problem.

Gwendal.

On Mon, Mar 25, 2013 at 10:26 AM, Ronald <ronald645@gmail.com> wrote:
> In reply to [1]: I have the same issue. Git bisect took 50+ rebuilds xD
>
> Smartd does not work anymore since 84a9a8cd9 ([libata] Set proper SK
> when CK_COND is set.).
>
> I hope I'm not stepping on anyone's toe's by chosing the same title.
> I'm not subscribed to this list.
>
> Just wanted to add a 'me2'
>
> [1] http://www.spinics.net/lists/linux-ide/msg45268.html
> --
> 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

Re: smartd broken in 3.9.0-rc4 : bisected

[Gwendal Grignou]

My mistake.
In ata_cmd_ioctl(), we clean the results and remove CHECK_CONDITION
when necessary. I did not update that code for the new Sense Key:
                if (cmd_result & SAM_STAT_CHECK_CONDITION) {
                        struct scsi_sense_hdr sshdr;
                        scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
                                             &sshdr);
                        if (sshdr.sense_key == 0 &&
                            sshdr.asc == 0 && sshdr.ascq == 0)
                                cmd_result &= ~SAM_STAT_CHECK_CONDITION;
                }

A patch in progress.
Gwendal.

On Thu, Mar 28, 2013 at 1:59 PM, Ken Moffat <zarniwhoop@ntlworld.com> wrote:
> On Thu, Mar 28, 2013 at 01:01:48AM +0000, Ken Moffat wrote:
>
> Adding Cc:s, further details at the end.
>> Hi,
>>
>>  just tested my first 3.9 kernel today.  During boot, smartd (from
>> smartmontools-6.0) fails to start.  Works fine in 3.8.4.
>>
>>  In 3.8.4 I get messages like this :
>>
>> Mar 27 22:02:02 ac4tv smartd[3981]: smartd 6.0 2012-10-10 r3643
>> [x86_64-linux-3.8.4] (local build)
>> Mar 27 22:02:02 ac4tv smartd[3981]: Copyright (C) 2002-12, Bruce
>> Allen, Christian Franke, www.smartmontools.org
>> Mar 27 22:02:02 ac4tv smartd[3981]: Opened configuration file
>> /etc/smartd.conf
>> Mar 27 22:02:02 ac4tv smartd[3981]: Configuration file
>> /etc/smartd.conf parsed.
>> Mar 27 22:02:02 ac4tv smartd[3981]: Device: /dev/sda, opened
>> Mar 27 22:02:02 ac4tv smartd[3981]: Device: /dev/sda, WDC
>> WD5000AAKX-001CA0, S/N:WD-WMAYU6818967, WWN:5-0014ee-0ad9caed4,
>> FW:15.01H15, 500 GB
>> Mar 27 22:02:02 ac4tv smartd[3981]: Device: /dev/sda, found in
>> smartd database: Western Digital Caviar Blue Serial ATA
>> Mar 27 22:02:02 ac4tv smartd[3981]: Device: /dev/sda, enabled SMART
>> Automatic Offline Testing.
>> Mar 27 22:02:02 ac4tv smartd[3981]: Device: /dev/sda, is SMART
>> capable. Adding to "monitor" list.
>> Mar 27 22:02:02 ac4tv smartd[3981]: Monitoring 1 ATA and 0 SCSI
>> devices
>>
>>  but in 3.9.0-rc4 all I get is
>> Mar 28 00:39:32 ac4tv smartd[2487]: Copyright (C) 2002-12, Bruce
>> Allen, Christian Franke, www.smartmontools.org
>> Mar 28 00:39:32 ac4tv smartd[2487]: Opened configuration file
>> /etc/smartd.conf
>> Mar 28 00:39:32 ac4tv smartd[2487]: Configuration file
>> /etc/smartd.conf parsed.
>> Mar 28 00:39:32 ac4tv smartd[2487]: Device: /dev/sda, opened
>> Mar 28 00:39:32 ac4tv smartd[2487]: Device: /dev/sda, WDC
>> WD5000AAKX-001CA0, S/N:WD-WMAYU6818967, WWN:5-0014ee-0ad9caed4,
>> FW:15.01H15, 500 GB
>> Mar 28 00:39:32 ac4tv smartd[2487]: Device: /dev/sda, found in
>> smartd database: Western Digital Caviar Blue Serial ATA
>> Mar 28 00:39:32 ac4tv smartd[2487]: Device: /dev/sda, could not
>> enable SMART capability
>> Mar 28 00:39:32 ac4tv smartd[2487]: Unable to register ATA device
>> /dev/sda at line 1 of file /etc/smartd.conf
>> Mar 28 00:39:32 ac4tv smartd[2487]: Unable to register device
>> /dev/sda (no Directive -d removable). Exiting.
>>
>>  Using strace, in the failing version I get
>> 2643  ioctl(3, HDIO_DRIVE_CMD, 0x7fff53288a60) = -1 EIO (Input/output error)
>>
>> instead of the normal
>> 3981  ioctl(3, HDIO_DRIVE_CMD, 0x7fff04437340) = 0
>> 3981  ioctl(3, HDIO_DRIVE_TASK, 0x7fff04437350) = 0
>> 3981  ioctl(3, HDIO_DRIVE_CMD, 0x7fff04437330) = 0
>> 3981  ioctl(3, HDIO_DRIVE_CMD, 0x7fff04437330) = 0
>> 3981  ioctl(3, HDIO_DRIVE_TASK, 0x7fff04437340) = 0
>>
>>  Looks like I'll be bisecting.
>>
>> ken
>  Bisection blames :
>
> commit 84a9a8cd9d0aa93c17e5815ab8a9cc4c0a765c63
> Author: Gwendal Grignou <gwendal@google.com>
> Date:   Fri Jan 18 10:56:43 2013 -0800
>
>     [libata] Set proper SK when CK_COND is set.
>
>     When the user application sends a ATA_12 or ATA_16 PASSTHROUGH
>     scsi command, put the task file register in the sense data with the
>     proper Sense Key. Instead of NO SENSE, set RECOVERED, as
>     specified in [SAT2]12.2.5 Table 92.
>
>  That reverts cleanly from 3.9.0-rc4, and with it reverted smartd
> works again.  Obviously that does nothing to fix whatever problem
> this was supposed to fix.  I can test any follow-up patches if
> needed.
>
> ken
> --
> das eine Mal als Tragödie, das andere Mal als Farce

[PATCH] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Gwendal Grignou]

commit 84a9a8cd9d0aa93c17e5815ab8a9cc4c0a765c63 changed the sense key
used for returning task registers, but HDIO_DRIVE_CMD ioctl was
not changed accordingly.

Tested: check that SMART ENABLE sent using HDIO_DRIVE_CMD returns 0
instead of EIO.

Signed-off-by: Gwendal Grignou <gwendal@google.com>
---
 drivers/ata/libata-scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 318b413..e05bf4c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -532,8 +532,8 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 			struct scsi_sense_hdr sshdr;
 			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
 					     &sshdr);
-			if (sshdr.sense_key == 0 &&
-			    sshdr.asc == 0 && sshdr.ascq == 0)
+			if (sshdr.sense_key == RECOVERED_ERROR &&
+			    sshdr.asc == 0 && sshdr.ascq == 0x1D)
 				cmd_result &= ~SAM_STAT_CHECK_CONDITION;
 		}
 
-- 
1.8.1.3

Re: [PATCH] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Ken Moffat]

On Thu, Mar 28, 2013 at 10:56:49PM -0700, Gwendal Grignou wrote:

 Hmm, not sure.  Smartd started and was happy to monitor the disk,
but I got two new messages between 'found in smartd database' and
'is SMART capable. Adding to "monitor" list' -

Mar 29 17:26:42 ac4tv smartd[2481]: Device: /dev/sda, not capable of
SMART Health Status check
Mar 29 17:26:42 ac4tv smartd[2481]: Device: /dev/sda, enable SMART
Automatic Offline Testing failed.

 I've seen the first (intermittently) when a drive was starting to
fail, and apparently there was a taskfile issue in the days of 2.6.22
which also caused it to appear.  I don't think I've seen the second
of these before.

 After going back and forth between the kernel where I reverted your
original patch, and regular rc4 plus this new patch the output from
running smartctl as root all seems to be consistent (including
'Passed' for the health check).

 I'm now running with the patch again, and I've started a manual
'long' test (which will take 85 minutes, the default 'offline' is
about 150 minutes).

> commit 84a9a8cd9d0aa93c17e5815ab8a9cc4c0a765c63 changed the sense key
> used for returning task registers, but HDIO_DRIVE_CMD ioctl was
> not changed accordingly.
> 
> Tested: check that SMART ENABLE sent using HDIO_DRIVE_CMD returns 0
> instead of EIO.
> 
> Signed-off-by: Gwendal Grignou <gwendal@google.com>
> ---
>  drivers/ata/libata-scsi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 318b413..e05bf4c 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -532,8 +532,8 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
>  			struct scsi_sense_hdr sshdr;
>  			scsi_normalize_sense(sensebuf, SCSI_SENSE_BUFFERSIZE,
>  					     &sshdr);
> -			if (sshdr.sense_key == 0 &&
> -			    sshdr.asc == 0 && sshdr.ascq == 0)
> +			if (sshdr.sense_key == RECOVERED_ERROR &&
> +			    sshdr.asc == 0 && sshdr.ascq == 0x1D)
>  				cmd_result &= ~SAM_STAT_CHECK_CONDITION;
>  		}
>  
> -- 
> 1.8.1.3

-- 
das eine Mal als Tragödie, das andere Mal als Farce

Re: [PATCH] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Ken Moffat]

On Fri, Mar 29, 2013 at 06:31:03PM +0000, Ken Moffat wrote:
> On Thu, Mar 28, 2013 at 10:56:49PM -0700, Gwendal Grignou wrote:
> 
>  Hmm, not sure.  Smartd started and was happy to monitor the disk,
> but I got two new messages between 'found in smartd database' and
> 'is SMART capable. Adding to "monitor" list' -
> 
> Mar 29 17:26:42 ac4tv smartd[2481]: Device: /dev/sda, not capable of
> SMART Health Status check
> Mar 29 17:26:42 ac4tv smartd[2481]: Device: /dev/sda, enable SMART
> Automatic Offline Testing failed.
> 
>  I've seen the first (intermittently) when a drive was starting to
> fail, and apparently there was a taskfile issue in the days of 2.6.22
> which also caused it to appear.  I don't think I've seen the second
> of these before.
> 
>  After going back and forth between the kernel where I reverted your
> original patch, and regular rc4 plus this new patch the output from
> running smartctl as root all seems to be consistent (including
> 'Passed' for the health check).
> 
>  I'm now running with the patch again, and I've started a manual
> 'long' test (which will take 85 minutes, the default 'offline' is
> about 150 minutes).
> 

 Looks like the problem is confined to smartd, smartctl is
different and working fine.  The new messages only come from smartd.cpp :
(sorry, long lines to avoid word wrapping)

  // capability check: SMART status
  if (cfg.smartcheck && ataSmartStatus2(atadev) == -1) {
    PrintOut(LOG_INFO,"Device: %s, not capable of SMART Health Status check\n",name);
    cfg.smartcheck = false;
  }

and

  // enable/disable automatic on-line testing
  if (cfg.autoofflinetest) {
    // is this an enable or disable request?
    const char *what=(cfg.autoofflinetest==1)?"disable":"enable";
    if (!smart_val_ok)
      PrintOut(LOG_INFO,"Device: %s, could not %s SMART Automatic Offline Testing.\n",name, what);
    else {
      // if command appears unsupported, issue a warning...
      if (!isSupportAutomaticTimer(&state.smartval))
        PrintOut(LOG_INFO,"Device: %s, SMART Automatic Offline Testing unsupported...\n",name);
      // ... but then try anyway
      if ((cfg.autoofflinetest==1)?ataDisableAutoOffline(atadev):ataEnableAutoOffline(atadev))
        PrintOut(LOG_INFO,"Device: %s, %s SMART Automatic Offline Testing failed.\n", name, what);
      else
        PrintOut(LOG_INFO,"Device: %s, %sd SMART Automatic Offline Testing.\n", name, what);
    }
  }

 I've no idea about the details, but it looks to me as if smartd is
still getting different values returned to it.  The capability check
normally was ok (silent), the automatic testing normally showed as
'enabled'd.

ken
-- 
das eine Mal als Tragödie, das andere Mal als Farce

Re: [PATCH] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Ken Moffat]

On Fri, Mar 29, 2013 at 08:34:43PM +0000, Ken Moffat wrote:
> On Fri, Mar 29, 2013 at 06:31:03PM +0000, Ken Moffat wrote:

>  I've no idea about the details, but it looks to me as if smartd is
> still getting different values returned to it.  The capability check
> normally was ok (silent), the automatic testing normally showed as
> 'enabled'd.
> 
 After that I found today's posts in Krzystof Mazur's earlier
thread.  When I first read that I thought it was for a specific
drive, and didn't remember it when I had time to try rc4.

 The v2 of the patch (with two hunks) fixes it for me too.

ken
-- 
das eine Mal als Tragödie, das andere Mal als Farce

Re: [PATCH] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Jeff Garzik]

On 03/29/2013 01:56 AM, Gwendal Grignou wrote:
> commit 84a9a8cd9d0aa93c17e5815ab8a9cc4c0a765c63 changed the sense key
> used for returning task registers, but HDIO_DRIVE_CMD ioctl was
> not changed accordingly.
>
> Tested: check that SMART ENABLE sent using HDIO_DRIVE_CMD returns 0
> instead of EIO.
>
> Signed-off-by: Gwendal Grignou <gwendal@google.com>
> ---
>   drivers/ata/libata-scsi.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

applied the version from Krzysztof Mazur, which covered both cases

Re: [PATCH] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Gwendal Grignou]

Thank you.
Gwendal.

On Wed, Apr 3, 2013 at 4:49 PM, Jeff Garzik <jgarzik@pobox.com> wrote:
> On 03/29/2013 01:56 AM, Gwendal Grignou wrote:
>>
>> commit 84a9a8cd9d0aa93c17e5815ab8a9cc4c0a765c63 changed the sense key
>> used for returning task registers, but HDIO_DRIVE_CMD ioctl was
>> not changed accordingly.
>>
>> Tested: check that SMART ENABLE sent using HDIO_DRIVE_CMD returns 0
>> instead of EIO.
>>
>> Signed-off-by: Gwendal Grignou <gwendal@google.com>
>> ---
>>   drivers/ata/libata-scsi.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>
>
> applied the version from Krzysztof Mazur, which covered both cases
>
>
>

Re: [PATCH] [libata] Fix HDIO_DRIVE_CMD ioctl sense data check

[Ken Moffat]

On Wed, Apr 03, 2013 at 07:49:48PM -0400, Jeff Garzik wrote:
> 
> applied the version from Krzysztof Mazur, which covered both cases
> 
> 
 According to linus's changelog for -rc6, this doesn't seem to have
been included ?

ken
-- 
das eine Mal als Tragödie, das andere Mal als Farce