diff mbox

libata: check SATA_SETTINGS log with HW Feature Ctrl

Message ID 1353185081-11349-1-git-send-email-shane.huang@amd.com
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Shane Huang Nov. 17, 2012, 8:44 p.m. UTC
NCQ capability was used to check availability of SATA Settings page
from Identify Device Data Log, which contains DevSlp timing variables.
It does not work on some HDDs and leads to error messages.
IDENTIFY word 78 bit 5(Hardware Feature Control) should be used.

Quoting SATA spec 3.1:
If Hardware Feature Control is supported, then:
a) IDENTIFY DEVICE data word 78 bit 5 (see 13.2.1.18) shall be
set to one;
b) the SET FEATURES Select Hardware Feature Control subcommand
shall be supported (see 13.3.8);
c) page 08h of the Identify Device Data log (see 13.7.7) shall
be supported;

This patch is not tested on SATA HDD with DevSlp supported.

Reported-by: Borislav Petkov <bp@amd64.org>
Signed-off-by: Shane Huang <shane.huang@amd.com>
---
 drivers/ata/libata-core.c |    3 +--
 include/linux/ata.h       |    1 +
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Jeff Garzik Dec. 3, 2012, 9:58 a.m. UTC | #1
On 11/17/2012 03:44 PM, Shane Huang wrote:
> NCQ capability was used to check availability of SATA Settings page
> from Identify Device Data Log, which contains DevSlp timing variables.
> It does not work on some HDDs and leads to error messages.
> IDENTIFY word 78 bit 5(Hardware Feature Control) should be used.
>
> Quoting SATA spec 3.1:
> If Hardware Feature Control is supported, then:
> a) IDENTIFY DEVICE data word 78 bit 5 (see 13.2.1.18) shall be
> set to one;
> b) the SET FEATURES Select Hardware Feature Control subcommand
> shall be supported (see 13.3.8);
> c) page 08h of the Identify Device Data log (see 13.7.7) shall
> be supported;
>
> This patch is not tested on SATA HDD with DevSlp supported.
>
> Reported-by: Borislav Petkov <bp@amd64.org>
> Signed-off-by: Shane Huang <shane.huang@amd.com>
> ---
>   drivers/ata/libata-core.c |    3 +--
>   include/linux/ata.h       |    1 +
>   2 files changed, 2 insertions(+), 2 deletions(-)

applied



--
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
Shane Huang Dec. 11, 2012, 9:25 a.m. UTC | #2
Jeff,

> > This patch is not tested on SATA HDD with DevSlp supported.
> >
> > Reported-by: Borislav Petkov <bp@amd64.org>
> > Signed-off-by: Shane Huang <shane.huang@amd.com>
> > ---
> >   drivers/ata/libata-core.c |    3 +--
> >   include/linux/ata.h       |    1 +
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> applied

Please suspend this patch because I just received two new
DevSlp drives but found word 78 bit 5 is _not_ set.

I'm checking with the drive vendor whether he gave me
the wrong information. If bit 5 is not the necessary and
sufficient condition, I will implement another patch to
replace ata_device->sata_settings into ->devslp_timing.

Thanks,
Shane

--
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
Jeff Garzik Dec. 14, 2012, 2:37 p.m. UTC | #3
On 12/11/2012 04:25 AM, Huang, Shane wrote:
> Jeff,
>
>>> This patch is not tested on SATA HDD with DevSlp supported.
>>>
>>> Reported-by: Borislav Petkov <bp@amd64.org>
>>> Signed-off-by: Shane Huang <shane.huang@amd.com>
>>> ---
>>>    drivers/ata/libata-core.c |    3 +--
>>>    include/linux/ata.h       |    1 +
>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> applied
>
> Please suspend this patch because I just received two new
> DevSlp drives but found word 78 bit 5 is _not_ set.
>
> I'm checking with the drive vendor whether he gave me
> the wrong information. If bit 5 is not the necessary and
> sufficient condition, I will implement another patch to
> replace ata_device->sata_settings into ->devslp_timing.

reverted



--
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
diff mbox

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3cc7096..4c9a58b 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2330,9 +2330,8 @@  int ata_dev_configure(struct ata_device *dev)
 
 		/* Obtain SATA Settings page from Identify Device Data Log,
 		 * which contains DevSlp timing variables etc.
-		 * Exclude old devices with ata_id_has_ncq()
 		 */
-		if (ata_id_has_ncq(dev->id)) {
+		if (ata_id_has_hw_feature_ctrl(dev->id)) {
 			err_mask = ata_read_log_page(dev,
 						     ATA_LOG_SATA_ID_DEV_DATA,
 						     ATA_LOG_SATA_SETTINGS,
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 408da95..18cbb93 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -593,6 +593,7 @@  static inline int ata_is_data(u8 prot)
 #define ata_id_cdb_intr(id)	(((id)[ATA_ID_CONFIG] & 0x60) == 0x20)
 #define ata_id_has_da(id)	((id)[ATA_ID_SATA_CAPABILITY_2] & (1 << 4))
 #define ata_id_has_devslp(id)	((id)[ATA_ID_FEATURE_SUPP] & (1 << 8))
+#define ata_id_has_hw_feature_ctrl(id)	((id)[ATA_ID_FEATURE_SUPP] & (1 << 5))
 
 static inline bool ata_id_has_hipm(const u16 *id)
 {