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

ata: HDIO_DRIVE_* ioctl() Linux 3.9 regression

[Krzysztof Mazur]

Hi,

commit 84a9a8cd9d0aa93c17e5815ab8a9cc4c0a765c63
("[libata] Set proper SK when CK_COND is set") breaks
"SMART Status command" ATA command issued by
smartctl -d ata -a /dev/sda at least on FUJITSU MHV2060AH
on ICH6 IDE Controller. The kernel returns -EIO on
HDIO_DRIVE_TASK (0x31e) or HDIO_DRIVE_CMD (0x31f) ioctl,
probably due to RECOVERED_ERROR translated to -EIO.

The regression still exists in v3.9-rc3-218-g0a7e453.

Ancient smartctl like 5.33 give up after such error:

  smartctl version 5.33 [i686-pc-linux-gnu] Copyright (C) 2002-4 Bruce Allen
  Home page is http://smartmontools.sourceforge.net/
  
  === START OF INFORMATION SECTION ===
  Device Model:     FUJITSU MHV2060AH
  Serial Number:    NT29T5B2D23C
  Firmware Version: 00830096
  User Capacity:    60,011,642,880 bytes
  Device is:        Not in smartctl database [for details use: -P showall]
  ATA Version is:   6
  ATA Standard is:  ATA/ATAPI-6 T13 1410D revision 3a
  Local Time is:    Sat Mar 23 01:55:27 2013 CET
  SMART support is: Available - device has SMART capability.
  SMART support is: Enabled
  
  SMART Disabled. Use option -s with argument 'on' to enable it.

 # strace -x smartctl-5.33 -d ata -a /dev/sda 2>&1 | tail -n 10
  write(1, "SMART support is: Available - de"..., 59SMART support is: Available - device has SMART capability.
  ) = 59
  write(1, "SMART support is: Enabled\n", 26SMART support is: Enabled
  ) = 26
  write(1, "\n", 1
  )                       = 1
  ioctl(3, 0x31f, 0x7f98a240)             = -1 EIO (Input/output error)
  write(1, "SMART Disabled. Use option -s wi"..., 63SMART Disabled. Use option -s with argument 'on' to enable it.
  ) = 63
  exit_group(0)                           = ?


Newer smartctl versions (at least 6.1) just reports an error in such
case and continue:

  Error SMART Status command failed: Input/output error

 # strace -x smartctl-6.1 -d ata -a /dev/sda 2>&1
  ...
  ioctl(3, 0x31e, 0x7fac798c)             = -1 EIO (Input/output error)
  write(1, "Error SMART Status command faile"..., 54Error SMART Status command failed: Input/output error
  ...

On older kernels ioctl() returns 0 instead of -EIO.

Thanks,
Krzysiek