Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Quel Qun]

 -------------- Original message ----------------------
From: Tejun Heo <htejun@gmail.com>
> Luiz Fernando N. Capitulino wrote:
> > Em Wed, 08 Aug 2007 11:21:08 +0900
> > Tejun Heo <htejun@gmail.com> escreveu:
> > 
> > | Alan Cox wrote:
> > | >>> I'd rather know what is going on here. A drive can legitimately
> > | >>> support LBA48 and HPA and refuse READ_NATIVE_MAX_EXT.
> > | >> READ_NATIVE_MAX_EXT is mandatory if HPA && LBA48, no
> > | > 
> > | > Ok the report in that thread is different. The offending Maxtor simply
> > | > aborts the read_native_max_ext
> > | 
> > | I'll resend sans ata_id_has_hpa() change.  Does that sound okay?  I
> > | don't really think we can do anything other than blacklisting it.
> > 
> >  Does that make your patch invalid or is it still ok?
> 
> You controller being ICH ahci, I don't think the chance of the
> controller messing things up is pretty slim but, just in case, do you
> have another SATA controller to cross check?  If not, can you put the
> controller into IDE mode so that ata_piix can drive it and test whether
> the problem persists?
>
Hi, 
Unfortunately, I do not have any machine with a different controller. Whichever 
I pick in the BIOS between ATA and AHCI, it seems that the drive is always 
handled by the ahci module, and the boot process goes the exact same way.
Is there a way to force the IDE mode?
--
kk1

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Luiz Fernando N. Capitulino]

Em Wed, 08 Aug 2007 16:50:39 +0000
kelk1@comcast.net (Quel Qun) escreveu:

|  -------------- Original message ----------------------
| From: Tejun Heo <htejun@gmail.com>
| > Luiz Fernando N. Capitulino wrote:
| > > Em Wed, 08 Aug 2007 11:21:08 +0900
| > > Tejun Heo <htejun@gmail.com> escreveu:
| > > 
| > > | Alan Cox wrote:
| > > | >>> I'd rather know what is going on here. A drive can legitimately
| > > | >>> support LBA48 and HPA and refuse READ_NATIVE_MAX_EXT.
| > > | >> READ_NATIVE_MAX_EXT is mandatory if HPA && LBA48, no
| > > | > 
| > > | > Ok the report in that thread is different. The offending Maxtor simply
| > > | > aborts the read_native_max_ext
| > > | 
| > > | I'll resend sans ata_id_has_hpa() change.  Does that sound okay?  I
| > > | don't really think we can do anything other than blacklisting it.
| > > 
| > >  Does that make your patch invalid or is it still ok?
| > 
| > You controller being ICH ahci, I don't think the chance of the
| > controller messing things up is pretty slim but, just in case, do you
| > have another SATA controller to cross check?  If not, can you put the
| > controller into IDE mode so that ata_piix can drive it and test whether
| > the problem persists?
| >
| Hi, 
| Unfortunately, I do not have any machine with a different controller. Whichever 
| I pick in the BIOS between ATA and AHCI, it seems that the drive is always 
| handled by the ahci module, and the boot process goes the exact same way.
| Is there a way to force the IDE mode?

 I can try it here, but maybe not today.

-- 
Luiz Fernando N. Capitulino

[PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Tejun Heo]

HDS724040KLSA80 reports that it supports HPA && LBA48 but craps itself
on READ_NATIVE_MAX_EXT.  Implement BROKEN_HPA horkage and apply it to
the drive.  If the horkage is set, all HPA operations are skipped.

While at it, make HPA test a bit more reliable by also checking
ata_id_has_hpa().

Signed-off-by: Tejun Heo <htejun@gmail.com>
Cc: Quel Qun <kelk1@comcast.net>
Cc: Luiz Fernando N. Capitulino <lcapitulino@mandriva.com.br>
---
The drive worked fine before HPA support was added and thus this is a
regression fix.  Please consider for -stable.

Thanks.

 drivers/ata/libata-core.c |    4 +++-
 include/linux/libata.h    |    1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 60e78be..7158a06 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1911,7 +1911,8 @@ int ata_dev_configure(struct ata_device *dev)
 					dev->flags |= ATA_DFLAG_FLUSH_EXT;
 			}
 
-			if (ata_id_hpa_enabled(dev->id))
+			if (!(dev->horkage & ATA_HORKAGE_BROKEN_HPA) &&
+			    ata_id_has_hpa(id) && ata_id_hpa_enabled(dev->id))
 				dev->n_sectors = ata_hpa_resize(dev);
 
 			/* config NCQ */
@@ -3789,6 +3790,7 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
 	{ "FUJITSU MHV2080BH",	"00840028",	ATA_HORKAGE_NONCQ, },
 	{ "ST9160821AS",	"3.CLF",	ATA_HORKAGE_NONCQ, },
 	{ "SAMSUNG HD401LJ",	"ZZ100-15",	ATA_HORKAGE_NONCQ, },
+	{ "HDS724040KLSA80",	"KFAOA20N",	ATA_HORKAGE_BROKEN_HPA, },
 
 	/* Devices with NCQ limits */
 
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 41978a5..a67bb90 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -303,6 +303,7 @@ enum {
 	ATA_HORKAGE_NODMA	= (1 << 1),	/* DMA problems */
 	ATA_HORKAGE_NONCQ	= (1 << 2),	/* Don't use NCQ */
 	ATA_HORKAGE_MAX_SEC_128	= (1 << 3),	/* Limit max sects to 128 */
+	ATA_HORKAGE_BROKEN_HPA	= (1 << 4),	/* Broken HPA */
 };
 
 enum hsm_task_states {

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Luiz Fernando N. Capitulino]

Em Tue, 7 Aug 2007 14:42:50 +0900
Tejun Heo <htejun@gmail.com> escreveu:

| HDS724040KLSA80 reports that it supports HPA && LBA48 but craps itself
| on READ_NATIVE_MAX_EXT.  Implement BROKEN_HPA horkage and apply it to
| the drive.  If the horkage is set, all HPA operations are skipped.
| 
| While at it, make HPA test a bit more reliable by also checking
| ata_id_has_hpa().

 Tejun, is the following patch ok for 2.6.22.1?

--- linux-2.6.22.orig/drivers/ata/libata-core.c
+++ linux-2.6.22/drivers/ata/libata-core.c
@@ -1939,7 +1939,8 @@ int ata_dev_configure(struct ata_device 
 					dev->flags |= ATA_DFLAG_FLUSH_EXT;
 			}
 
-			if (ata_id_hpa_enabled(dev->id))
+			if (!(dev->horkage & ATA_HORKAGE_BROKEN_HPA) &&
+			    ata_id_has_hpa(id) && ata_id_hpa_enabled(dev->id))
 				dev->n_sectors = ata_hpa_resize(dev);
 
 			/* config NCQ */
@@ -3800,6 +3801,7 @@ static const struct ata_blacklist_entry 
 	{ "HTS541612J9SA00",	"SBDIC7JP",	ATA_HORKAGE_NONCQ, },
 	{ "Hitachi HTS541616J9SA00", "SB4OC70P", ATA_HORKAGE_NONCQ, },
 	{ "WDC WD740ADFD-00NLR1", NULL,		ATA_HORKAGE_NONCQ, },
+	{ "HDS724040KLSA80",    "KFAOA20N",     ATA_HORKAGE_BROKEN_HPA, },
 
 	/* Devices with NCQ limits */
 
--- linux-2.6.22.orig/include/linux/libata.h
+++ linux-2.6.22/include/linux/libata.h
@@ -298,6 +298,7 @@ enum {
 	ATA_HORKAGE_NODMA	= (1 << 1),	/* DMA problems */
 	ATA_HORKAGE_NONCQ	= (1 << 2),	/* Don't use NCQ */
 	ATA_HORKAGE_MAX_SEC_128	= (1 << 3),	/* Limit max sects to 128 */
+	ATA_HORKAGE_BROKEN_HPA	= (1 << 4),	/* Broken HPA */
 };
 
 enum hsm_task_states {


-- 
Luiz Fernando N. Capitulino

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Tejun Heo]

Luiz Fernando N. Capitulino wrote:
> Em Tue, 7 Aug 2007 14:42:50 +0900
> Tejun Heo <htejun@gmail.com> escreveu:
> 
> | HDS724040KLSA80 reports that it supports HPA && LBA48 but craps itself
> | on READ_NATIVE_MAX_EXT.  Implement BROKEN_HPA horkage and apply it to
> | the drive.  If the horkage is set, all HPA operations are skipped.
> | 
> | While at it, make HPA test a bit more reliable by also checking
> | ata_id_has_hpa().
> 
>  Tejun, is the following patch ok for 2.6.22.1?

Yeah, looks okay to me but -stable needs to wait for upstream merge.

Thanks.

-- 
tejun

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Luiz Fernando N. Capitulino]

Em Wed, 08 Aug 2007 00:00:28 +0900
Tejun Heo <htejun@gmail.com> escreveu:

| Luiz Fernando N. Capitulino wrote:
| > Em Tue, 7 Aug 2007 14:42:50 +0900
| > Tejun Heo <htejun@gmail.com> escreveu:
| > 
| > | HDS724040KLSA80 reports that it supports HPA && LBA48 but craps itself
| > | on READ_NATIVE_MAX_EXT.  Implement BROKEN_HPA horkage and apply it to
| > | the drive.  If the horkage is set, all HPA operations are skipped.
| > | 
| > | While at it, make HPA test a bit more reliable by also checking
| > | ata_id_has_hpa().
| > 
| >  Tejun, is the following patch ok for 2.6.22.1?
| 
| Yeah, looks okay to me but -stable needs to wait for upstream merge.

 Okay, I'll that version in Mandriva.

 Thanks a lot, Tejun.

-- 
Luiz Fernando N. Capitulino

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Alan Cox]

On Tue, 7 Aug 2007 14:42:50 +0900
Tejun Heo <htejun@gmail.com> wrote:

> HDS724040KLSA80 reports that it supports HPA && LBA48 but craps itself
> on READ_NATIVE_MAX_EXT.  Implement BROKEN_HPA horkage and apply it to
> the drive.  If the horkage is set, all HPA operations are skipped.

I'd rather know what is going on here. A drive can legitimately
support LBA48 and HPA and refuse READ_NATIVE_MAX_EXT. In addition just
about every OS I know queries the HPA which means that something other
than HPA support being broken is probably at the root. We thus want to
find the real fix.

What do the actual traces look like ?

Does it crap out of it gets READ_NATIVE_MAX ?

> 
> While at it, make HPA test a bit more reliable by also checking
> ata_id_has_hpa().

>  
> -			if (ata_id_hpa_enabled(dev->id))
> +			if (!(dev->horkage & ATA_HORKAGE_BROKEN_HPA) &&
> +			    ata_id_has_hpa(id) && ata_id_hpa_enabled(dev->id))
>  				dev->n_sectors = ata_hpa_resize(dev);

A drive cannot set hpa_enabled without setting has_hpa so this does
nothing and I've never seen a drive d what you try to cover here. I'd
like to see actual evidence.

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Tejun Heo]

Alan Cox wrote:
> On Tue, 7 Aug 2007 14:42:50 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> HDS724040KLSA80 reports that it supports HPA && LBA48 but craps itself
>> on READ_NATIVE_MAX_EXT.  Implement BROKEN_HPA horkage and apply it to
>> the drive.  If the horkage is set, all HPA operations are skipped.
> 
> I'd rather know what is going on here. A drive can legitimately
> support LBA48 and HPA and refuse READ_NATIVE_MAX_EXT.

READ_NATIVE_MAX_EXT is mandatory if HPA && LBA48, no?

> In addition just
> about every OS I know queries the HPA which means that something other
> than HPA support being broken is probably at the root. We thus want to
> find the real fix.
> 
> What do the actual traces look like ?
> 
> Does it crap out of it gets READ_NATIVE_MAX ?

Haven't tried that but the problem is that the drive times out
READ_NATIVE_MAX_EXT so it doesn't really matter whether the drive
succeeds READ_NATIVE_MAX or not.  For more detail, please read the
following thread.

  http://thread.gmane.org/gmane.linux.ide/21322

>> While at it, make HPA test a bit more reliable by also checking
>> ata_id_has_hpa().
> 
>>  
>> -			if (ata_id_hpa_enabled(dev->id))
>> +			if (!(dev->horkage & ATA_HORKAGE_BROKEN_HPA) &&
>> +			    ata_id_has_hpa(id) && ata_id_hpa_enabled(dev->id))
>>  				dev->n_sectors = ata_hpa_resize(dev);
> 
> A drive cannot set hpa_enabled without setting has_hpa so this does
> nothing and I've never seen a drive d what you try to cover here. I'd
> like to see actual evidence.

Well, it's what the ide driver does.  BTW, according to the spec, we
need to test bit 14 and 15 of word 87 before trusting any value the
device reports in words 85-87 and 120, which libata currently doesn't
do.  Are we leaving this out intentionally (for broken devices) or just
did we just miss it?

Thanks.

-- 
tejun

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 210 bytes --]

People should check out Ben C's HPA fixes and cleanups, too. (attached)

I was hoping one of the original libata HPA authors would review that in 
depth and integrate.  (it no longer applies cleanly)

	Jeff




[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 7066 bytes --]


From: Ben Collins <ben.collins@ubuntu.com>

The original HPA patch that Kyle worked on has gone into current git
without some fixes that we worked through late in the Ubuntu feisty
release.  Here's the main copy of the notes I sent to Alan a few weeks ago
in regards to the original patch, and a repatch against current git to fix
things up.  Note we have released feisty with the patch attached (albeit we
have HPA enabled by default), and we have not had any reports directly
attributed to it.  However, in gutsy (devel for next release, based on
current stock linux-2.6.git), we are already seeing reports of the same
issues we already fixed.

The issues we saw were mainly that some controllers didn't return the
correct size from the SET_MAX command (sata_nv is a good example).  So I
added a re IDENTIFY after the SET_MAX, and compared all the numbers.  If
re-id was correct, but return value from SET_MAX wasn't we print a warning
and use the IDENTIFY value regardless (since that's what the device is
going to use).

ata_hpa_resize() was changed to handle everything in a single call (checks
for HPA support of the device, and whether ignore_hpa is set or not), and
it also sets dev->n_sectors before returning.

So far with this patch, we were able to fix all the problems we were seeing
with it (except the sata_nv issue where we had to revert to not using adma
for NO_DATA transactions, already reported to libata-dev).  We've not had
any reports of further problems.

Cc: Kyle McMartin <kyle@ubuntu.com>
Signed-off-by: Ben Collins <bcollins@ubuntu.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/ata/libata-core.c |  140 +++++++++++++++++++++++++-----------
 1 files changed, 100 insertions(+), 40 deletions(-)

diff -puN drivers/ata/libata-core.c~cleanup-libata-hpa-support drivers/ata/libata-core.c
--- a/drivers/ata/libata-core.c~cleanup-libata-hpa-support
+++ a/drivers/ata/libata-core.c
@@ -819,16 +819,19 @@ void ata_id_c_string(const u16 *id, unsi
 
 static u64 ata_tf_to_lba48(struct ata_taskfile *tf)
 {
-	u64 sectors = 0;
+	u64 sectors;
+	u32 high, low;
 
-	sectors |= ((u64)(tf->hob_lbah & 0xff)) << 40;
-	sectors |= ((u64)(tf->hob_lbam & 0xff)) << 32;
-	sectors |= (tf->hob_lbal & 0xff) << 24;
-	sectors |= (tf->lbah & 0xff) << 16;
-	sectors |= (tf->lbam & 0xff) << 8;
-	sectors |= (tf->lbal & 0xff);
+	high = (tf->hob_lbah << 16) |
+	       (tf->hob_lbam <<  8) |
+		tf->hob_lbal;
+	low  = (tf->lbah << 16) |
+	       (tf->lbam <<  8) |
+		tf->lbal;
 
-	return ++sectors;
+	sectors = ((u64)high << 24) | low;
+
+	return sectors + 1;
 }
 
 static u64 ata_tf_to_lba(struct ata_taskfile *tf)
@@ -970,52 +973,107 @@ static u64 ata_set_native_max_address(st
 }
 
 /**
+ *	ata_hpa_get_native_size	-	Get the native size of a disk
+ *	@dev: Device to get the size for
+ *
+ *	Read the size of an LBA28 or LBA48 disk with HPA features and
+ *	return the native size. Caller must check that the drive has HPA
+ *	feature set enabled.
+ */
+static u64 ata_hpa_get_native_size(struct ata_device *dev)
+{
+	if (ata_id_has_lba48(dev->id))
+		return ata_read_native_max_address_ext(dev);
+	else
+		return ata_read_native_max_address(dev);
+}
+
+
+static u64 ata_hpa_set_native_size(struct ata_device *dev, u64 new_sectors)
+{
+	if (ata_id_has_lba48(dev->id))
+		return ata_set_native_max_address_ext(dev, new_sectors);
+	else
+		return ata_set_native_max_address(dev, new_sectors);
+}
+
+static unsigned long long sectors_to_MB(unsigned long long n)
+{
+	n <<= 9;		/* make it bytes */
+	do_div(n, 1000000);	/* make it MB */
+	return n;
+}
+
+static u64 ata_id_n_sectors(const u16 *id);
+
+/**
  *	ata_hpa_resize		-	Resize a device with an HPA set
  *	@dev: Device to resize
  *
  *	Read the size of an LBA28 or LBA48 disk with HPA features and resize
- *	it if required to the full size of the media. The caller must check
- *	the drive has the HPA feature set enabled.
+ *	it if required to the full size of the media.
  */
 
-static u64 ata_hpa_resize(struct ata_device *dev)
+static int ata_hpa_resize(struct ata_device *dev)
 {
-	u64 sectors = dev->n_sectors;
 	u64 hpa_sectors;
 
-	if (ata_id_has_lba48(dev->id))
-		hpa_sectors = ata_read_native_max_address_ext(dev);
-	else
-		hpa_sectors = ata_read_native_max_address(dev);
+	if (!ata_id_hpa_enabled(dev->id))
+		return 0;
+
+	hpa_sectors = ata_hpa_get_native_size(dev);
 
-	/* if no hpa, both should be equal */
-	ata_dev_printk(dev, KERN_INFO, "%s 1: sectors = %lld, "
-				"hpa_sectors = %lld\n",
-		__FUNCTION__, (long long)sectors, (long long)hpa_sectors);
+	ata_dev_printk(dev, KERN_INFO, "%s: sectors = %lld, hpa_sectors = %lld\n",
+		__FUNCTION__, dev->n_sectors, hpa_sectors);
+
+	if (ata_ignore_hpa && hpa_sectors > dev->n_sectors) {
+		u64 ret_sectors;
 
-	if (hpa_sectors > sectors) {
 		ata_dev_printk(dev, KERN_INFO,
-			"Host Protected Area detected:\n"
-			"\tcurrent size: %lld sectors\n"
-			"\tnative size: %lld sectors\n",
-			(long long)sectors, (long long)hpa_sectors);
-
-		if (ata_ignore_hpa) {
-			if (ata_id_has_lba48(dev->id))
-				hpa_sectors = ata_set_native_max_address_ext(dev, hpa_sectors);
-			else
-				hpa_sectors = ata_set_native_max_address(dev,
-								hpa_sectors);
+			"Host Protected Area detected.\n"
+			"     current size : %llu sectors (%llu MB)\n"
+			"     native  size : %llu sectors (%llu MB)\n",
+			dev->n_sectors, sectors_to_MB(dev->n_sectors),
+			hpa_sectors, sectors_to_MB(hpa_sectors));
+
+		ret_sectors = ata_hpa_set_native_size(dev, hpa_sectors);
+		if (ret_sectors != hpa_sectors) {
+			ata_dev_printk(dev, KERN_ERR,
+				"SET of native returned %llu, expected %llu\n",
+				ret_sectors, hpa_sectors);
+		}
+
+		if (ret_sectors) {
+			u16 *id = (void *)dev->ap->sector_buf;
+			int rc;
+
+			/* Re read the IDENTITY to see if the set took */
+			rc = ata_dev_read_id(dev, &dev->class,
+					     ATA_READID_POSTRESET, id);
 
-			if (hpa_sectors) {
-				ata_dev_printk(dev, KERN_INFO, "native size "
-					"increased to %lld sectors\n",
-					(long long)hpa_sectors);
-				return hpa_sectors;
+			if (rc) {
+				ata_dev_printk(dev, KERN_ERR,
+					"Failed to reread ID after SET_MAX (%d)\n",
+					rc);
+				return -EIO;
+			}
+
+			memcpy(dev->id, id, sizeof(id[0]) * ATA_ID_WORDS);
+			dev->n_sectors = ata_id_n_sectors(dev->id);
+
+			if (dev->n_sectors == hpa_sectors) {
+				ata_dev_printk(dev, KERN_INFO,
+					"Native size increased to %llu sectors\n",
+					hpa_sectors);
+			} else {
+				ata_dev_printk(dev, KERN_ERR,
+					"Failed to set native size, "
+					"drive reports %llu sectors\n",
+					dev->n_sectors);
 			}
 		}
 	}
-	return sectors;
+	return 0;
 }
 
 static u64 ata_id_n_sectors(const u16 *id)
@@ -1914,8 +1972,10 @@ int ata_dev_configure(struct ata_device 
 					dev->flags |= ATA_DFLAG_FLUSH_EXT;
 			}
 
-			if (ata_id_hpa_enabled(dev->id))
-				dev->n_sectors = ata_hpa_resize(dev);
+			/* Check for HPA, and resize if requested */
+			rc = ata_hpa_resize(dev);
+			if (rc)
+				goto err_out_nosup;
 
 			/* config NCQ */
 			ata_dev_config_ncq(dev, ncq_desc, sizeof(ncq_desc));
_

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Alan Cox]

On Tue, 07 Aug 2007 11:47:38 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> People should check out Ben C's HPA fixes and cleanups, too. (attached)
> 
> I was hoping one of the original libata HPA authors would review that in 
> depth and integrate.  (it no longer applies cleanly)

Looks basically sound but we should still probably move over this bit of
the old IDE paranoia as well:

/*
 * Bits 10 of command_set_1 and cfs_enable_1 must be equal,
 * so on non-buggy drives we need test only one.
 * However, we should also check whether these fields are valid.  
 */
static inline int idedisk_supports_hpa(const struct hd_driveid *id)
{
        return (id->command_set_1 & 0x0400) && (id->cfs_enable_1 &0x0400); }


---

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Jeff Garzik]

Alan Cox wrote:
> On Tue, 07 Aug 2007 11:47:38 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
> 
>> People should check out Ben C's HPA fixes and cleanups, too. (attached)
>>
>> I was hoping one of the original libata HPA authors would review that in 
>> depth and integrate.  (it no longer applies cleanly)
> 
> Looks basically sound but we should still probably move over this bit of
> the old IDE paranoia as well:

Oh quite agreed...

	Jeff

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Alan Cox]

> > I'd rather know what is going on here. A drive can legitimately
> > support LBA48 and HPA and refuse READ_NATIVE_MAX_EXT.
> 
> READ_NATIVE_MAX_EXT is mandatory if HPA && LBA48, no?

No - and we hit this specific case in old IDE with some Maxtor drives.

> Haven't tried that but the problem is that the drive times out
> READ_NATIVE_MAX_EXT so it doesn't really matter whether the drive
> succeeds READ_NATIVE_MAX or not.  For more detail, please read the
> following thread.
> 
>   http://thread.gmane.org/gmane.linux.ide/21322

Thanks will do:

> device reports in words 85-87 and 120, which libata currently doesn't
> do.  Are we leaving this out intentionally (for broken devices) or just
> did we just miss it?

We missed it. I assume the drive in the blacklist sets it correctly
however ?

Alan

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Tejun Heo]

Alan Cox wrote:
>>> I'd rather know what is going on here. A drive can legitimately
>>> support LBA48 and HPA and refuse READ_NATIVE_MAX_EXT.
>> READ_NATIVE_MAX_EXT is mandatory if HPA && LBA48, no?
>
> No - and we hit this specific case in old IDE with some Maxtor drives.

Hmmm... Looking up the spec...  This is from ATA8-ACS 4.11.1.

  A device that implements the Host Protected Area feature set and
  supports the 48-bit Address feature set shall implement the following
  additional set of commands:
      a) READ NATIVE MAX ADDRESS EXT
      b) SET MAX ADDRESS EXT
  Devices supporting this feature set shall set IDENTIFY DEVICE data
  word 82 bit 10 to one.

Did older specs specify it differently?

>> Haven't tried that but the problem is that the drive times out
>> READ_NATIVE_MAX_EXT so it doesn't really matter whether the drive
>> succeeds READ_NATIVE_MAX or not.  For more detail, please read the
>> following thread.
>>
>>   http://thread.gmane.org/gmane.linux.ide/21322
>
> Thanks will do:
>
>> device reports in words 85-87 and 120, which libata currently doesn't
>> do.  Are we leaving this out intentionally (for broken devices) or just
>> did we just miss it?
>
> We missed it. I assume the drive in the blacklist sets it correctly
> however ?

Yes, it does.

-- 
tejun

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Alan Cox]

> > I'd rather know what is going on here. A drive can legitimately
> > support LBA48 and HPA and refuse READ_NATIVE_MAX_EXT.
> 
> READ_NATIVE_MAX_EXT is mandatory if HPA && LBA48, no

Ok the report in that thread is different. The offending Maxtor simply
aborts the read_native_max_ext

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Tejun Heo]

Alan Cox wrote:
>>> I'd rather know what is going on here. A drive can legitimately
>>> support LBA48 and HPA and refuse READ_NATIVE_MAX_EXT.
>> READ_NATIVE_MAX_EXT is mandatory if HPA && LBA48, no
> 
> Ok the report in that thread is different. The offending Maxtor simply
> aborts the read_native_max_ext

I'll resend sans ata_id_has_hpa() change.  Does that sound okay?  I
don't really think we can do anything other than blacklisting it.

-- 
tejun

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Alan Cox]

On Wed, 08 Aug 2007 11:21:08 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Alan Cox wrote:
> >>> I'd rather know what is going on here. A drive can legitimately
> >>> support LBA48 and HPA and refuse READ_NATIVE_MAX_EXT.
> >> READ_NATIVE_MAX_EXT is mandatory if HPA && LBA48, no
> > 
> > Ok the report in that thread is different. The offending Maxtor simply
> > aborts the read_native_max_ext
> 
> I'll resend sans ata_id_has_hpa() change.  Does that sound okay?  I
> don't really think we can do anything other than blacklisting it.

Other than finding someone who can test it with a different controller
(especially SI3112 and old-ide) I agree

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Luiz Fernando N. Capitulino]

Em Wed, 08 Aug 2007 11:21:08 +0900
Tejun Heo <htejun@gmail.com> escreveu:

| Alan Cox wrote:
| >>> I'd rather know what is going on here. A drive can legitimately
| >>> support LBA48 and HPA and refuse READ_NATIVE_MAX_EXT.
| >> READ_NATIVE_MAX_EXT is mandatory if HPA && LBA48, no
| > 
| > Ok the report in that thread is different. The offending Maxtor simply
| > aborts the read_native_max_ext
| 
| I'll resend sans ata_id_has_hpa() change.  Does that sound okay?  I
| don't really think we can do anything other than blacklisting it.

 Does that make your patch invalid or is it still ok?

-- 
Luiz Fernando N. Capitulino

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Tejun Heo]

Luiz Fernando N. Capitulino wrote:
> Em Wed, 08 Aug 2007 11:21:08 +0900
> Tejun Heo <htejun@gmail.com> escreveu:
> 
> | Alan Cox wrote:
> | >>> I'd rather know what is going on here. A drive can legitimately
> | >>> support LBA48 and HPA and refuse READ_NATIVE_MAX_EXT.
> | >> READ_NATIVE_MAX_EXT is mandatory if HPA && LBA48, no
> | > 
> | > Ok the report in that thread is different. The offending Maxtor simply
> | > aborts the read_native_max_ext
> | 
> | I'll resend sans ata_id_has_hpa() change.  Does that sound okay?  I
> | don't really think we can do anything other than blacklisting it.
> 
>  Does that make your patch invalid or is it still ok?

You controller being ICH ahci, I don't think the chance of the
controller messing things up is pretty slim but, just in case, do you
have another SATA controller to cross check?  If not, can you put the
controller into IDE mode so that ata_piix can drive it and test whether
the problem persists?

Thanks.

-- 
tejun

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Chuck Ebbert]

On 08/07/2007 11:36 AM, Tejun Heo wrote:
> Alan Cox wrote:
>> On Tue, 7 Aug 2007 14:42:50 +0900
>> Tejun Heo <htejun@gmail.com> wrote:
>>
>>> HDS724040KLSA80 reports that it supports HPA && LBA48 but craps itself
>>> on READ_NATIVE_MAX_EXT.  Implement BROKEN_HPA horkage and apply it to
>>> the drive.  If the horkage is set, all HPA operations are skipped.
>> I'd rather know what is going on here. A drive can legitimately
>> support LBA48 and HPA and refuse READ_NATIVE_MAX_EXT.
> 
> READ_NATIVE_MAX_EXT is mandatory if HPA && LBA48, no?
> 
>> In addition just
>> about every OS I know queries the HPA which means that something other
>> than HPA support being broken is probably at the root. We thus want to
>> find the real fix.
>>
>> What do the actual traces look like ?
>>
>> Does it crap out of it gets READ_NATIVE_MAX ?
> 
> Haven't tried that but the problem is that the drive times out
> READ_NATIVE_MAX_EXT so it doesn't really matter whether the drive
> succeeds READ_NATIVE_MAX or not.  For more detail, please read the
> following thread.
> 
>   http://thread.gmane.org/gmane.linux.ide/21322
> 

There's also this Fedora bug:

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=251047#c2

...where after an error on the slave device the master starts throwing
HPA errors after the port is reset. Don't know if it's related or
not...

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Alan Cox]

> There's also this Fedora bug:
> 
> https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=251047#c2
> 
> ...where after an error on the slave device the master starts throwing
> HPA errors after the port is reset. Don't know if it's related or
> not...

Looks unrelated. You get a timeout

ata2.01: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x2 frozen
ata2.01: cmd a0/00:00:00:00:20/00:00:00:00:00/b0 tag 0 cdb 0x43 data 12 in
         res 40/00:03:00:00:20/00:00:00:00:00/b0 Emask 0x4 (timeout)

and then the entire system shits itself

ata2.00: qc timeout (cmd 0xf8)
ata2.00: failed to set xfermode (err_mask=0x40)

Looks like the bus didn't come back from whatever broke it.

Re: [PATCH] libata: implement BROKEN_HPA horkage and apply it to HDS724040KLSA80

[Alan Cox]

> Well, it's what the ide driver does.  BTW, according to the spec, we
> need to test bit 14 and 15 of word 87 before trusting any value the
> device reports in words 85-87 and 120, which libata currently doesn't
> do.  Are we leaving this out intentionally (for broken devices) or just
> did we just miss it?

Much of our testing was short checks on validity bits [although thats
really all paranoia]. Fixed in my tree and also made the hpa check match
drivers/ide and check both.

Verified the rest, fixed the dword_io check.


Think our flush_cache handling is still buggy in both old and new IDE btw.