Re: libata dev_config call order wrong.

Re: libata dev_config call order wrong.

[Jeff Garzik]

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

Jeff Garzik wrote:
> me either, unless I can figure out a way to detect that a disk is PATA 
> not SATA.  Nothing is obvious from the specs on www.t13.org, but who knows.

According to the Serial ATA docs, IDENTIFY DEVICE word 93 will be zero 
if it's Serial ATA.  Who knows if that's true, given the wierd wild 
world of ATA devices.

So, given the attached patch, you could try and create a generic 
ata_dev_config that all drivers call, that does something like

	/* limit bridge transfers to udma5, 200 sectors */
	if ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(dev))) {
                 printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
                        ap->id, dev->devno);
		ap->udma_mask &= ATA_UDMA5;
                 ap->host->max_sectors = ATA_MAX_SECTORS;
                 ap->host->hostt->max_sectors = ATA_MAX_SECTORS;
                 dev->flags |= ATA_DFLAG_LOCK_SECTORS;
	}

Regards,

	Jeff



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

===== include/linux/ata.h 1.17 vs edited =====
--- 1.17/include/linux/ata.h	2004-08-18 01:47:22 -04:00
+++ edited/include/linux/ata.h	2004-08-29 14:18:02 -04:00
@@ -218,6 +218,7 @@
 };
 
 #define ata_id_is_ata(dev)	(((dev)->id[0] & (1 << 15)) == 0)
+#define ata_id_is_sata(dev)	((dev)->id[93] == 0)
 #define ata_id_rahead_enabled(dev) ((dev)->id[85] & (1 << 6))
 #define ata_id_wcache_enabled(dev) ((dev)->id[85] & (1 << 5))
 #define ata_id_has_flush(dev) ((dev)->id[83] & (1 << 12))

Re: libata dev_config call order wrong.

[Alan Cox]

On Sul, 2004-08-29 at 19:25, Jeff Garzik wrote:
> According to the Serial ATA docs, IDENTIFY DEVICE word 93 will be zero 
> if it's Serial ATA.  Who knows if that's true, given the wierd wild 
> world of ATA devices.

You need to check if word 93 is valid first. Same with things like the
cache control word - its value is only meaningful if the drive says the
word is meaningful.

Re: libata dev_config call order wrong.

[Brad Campbell]

Alan Cox wrote:
> On Sul, 2004-08-29 at 19:25, Jeff Garzik wrote:
> 
>>According to the Serial ATA docs, IDENTIFY DEVICE word 93 will be zero 
>>if it's Serial ATA.  Who knows if that's true, given the wierd wild 
>>world of ATA devices.
> 
> 
> You need to check if word 93 is valid first. Same with things like the
> cache control word - its value is only meaningful if the drive says the
> word is meaningful.

I'm making some assumptions here based on information I could scrape up off the net.

IDENTIFY DEVICE Word 93 support has been mandatory at least since ATA-5.

ATA-5 did not have lba48 or > udma/66.

SATA->PATA bridge boards support > udma/33 and thus must emulate an 80 conductor cable.

Thus, any device capable of lba48 (and these are the ones that trigger the > 200 sector problem) 
must (according to the ATA-5 and up standard) support correct use of the IDENTIFY DEVICE word 93 
register.

Given that the SATA->PATA bridge boards support 80 pin detection, then bit 13 of word 93 must be 
high on any drive that supports lba48, and given the *current* sata spec states that word 93 must be 
zero, we should be able to use this detection method.

Now, remember I have been an "ATA researcher" for about 4 hours now, please feel free to belt me 
with the wet salmon of enlightenment and point out the flaw in my logic. Otherwise, when I get home 
this evening I'm going to have a crack and getting this working.

Regards,
Brad

Re: libata dev_config call order wrong.

[Alan Cox]

On Llu, 2004-08-30 at 10:12, Brad Campbell wrote:
> Given that the SATA->PATA bridge boards support 80 pin detection, then bit 13 of word 93 must be 
> high on any drive that supports lba48, and given the *current* sata spec states that word 93 must be 
> zero, we should be able to use this detection method.

Word 53 is the important one and essentially tells you what else to
believe later on in the configuration.

Re: libata dev_config call order wrong.

[Brad Campbell]

Alan Cox wrote:
> On Llu, 2004-08-30 at 10:12, Brad Campbell wrote:
> 
>>Given that the SATA->PATA bridge boards support 80 pin detection, then bit 13 of word 93 must be 
>>high on any drive that supports lba48, and given the *current* sata spec states that word 93 must be 
>>zero, we should be able to use this detection method.
> 
> 
> Word 53 is the important one and essentially tells you what else to
> believe later on in the configuration.

Ok, well acording to my copy of the ATA-6 spec, word 53 mentions nothing about word 93. From 
everything I have read in ATA-5 -> ATA-7 (Draft) word 93 appears to be pretty fixed.
It certainly works with my limited test set (all WD unfortunately)

[PATCH] libata ATA vs SATA detection and workaround.

[Brad Campbell]

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

Jeff Garzik wrote:
> Jeff Garzik wrote:
> 
>> me either, unless I can figure out a way to detect that a disk is PATA 
>> not SATA.  Nothing is obvious from the specs on www.t13.org, but who 
>> knows.
> 
> 
> According to the Serial ATA docs, IDENTIFY DEVICE word 93 will be zero 
> if it's Serial ATA.  Who knows if that's true, given the wierd wild 
> world of ATA devices.
> 

I'm assuming here that if the driver uses sata_phy_reset in libata-core then it's actually a SATA 
controller.

Patch attached, tested and verified.
SATA drives (of which I only have one brand) report IDENTIFY DEVICE word 93 as zero, ATA devices 
behind the bridge (Of which I only have one brand) report at a minimum bit 13 to report an 80 
conductor cable.

Like I said before, my assumptions are

A) IDENTIFY DEVICE and the associated Word 93 register have been *Mandatory* at least since ATA-5

B) ATA-5 maxed out at < udma/100 and did not contain lba48

C) The SATA->PATA bridge looks like an 80 conductor cable and thus on any drive that supports lba48 
will always present a non-zero value in IDENTIFY DEVICE Word 93.

It works here anyway.
Patch against vanilla 2.6.9-rc1 attached (including your ata.h patch)

Thanks again for all the help. I'll see if I can get some other older ATA drives to test with (I 
might have to gut all the machines in the office for the weekend)

Regards,
Brad

[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 1520 bytes --]

diff -ur orig/linux-2.6.0/drivers/scsi/libata-core.c linux-2.6.9-rc1/drivers/scsi/libata-core.c
--- orig/linux-2.6.0/drivers/scsi/libata-core.c	2004-08-30 18:31:31.000000000 +0400
+++ linux-2.6.9-rc1/drivers/scsi/libata-core.c	2004-08-30 18:22:05.000000000 +0400
@@ -1152,6 +1152,16 @@
 			found = 1;
 			if (ap->ops->dev_config)
 				ap->ops->dev_config(ap, &ap->device[i]);
+			/* limit bridge transfers to udma5, 200 sectors */
+			printk("Value is %u\n",ap->cbl==ATA_CBL_SATA);
+			if ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(ap->device))) {
+				printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
+					ap->id, ap->device->devno);
+				ap->udma_mask &= ATA_UDMA5;
+				ap->host->max_sectors = ATA_MAX_SECTORS;
+				ap->host->hostt->max_sectors = ATA_MAX_SECTORS;
+				ap->device->flags |= ATA_DFLAG_LOCK_SECTORS;
+			}
 		}
 	}
 
@@ -1226,7 +1236,7 @@
 		ata_port_disable(ap);
 		return;
 	}
-
+	ap->cbl = ATA_CBL_SATA;
 	ata_bus_reset(ap);
 }
 
diff -ur orig/linux-2.6.0/include/linux/ata.h linux-2.6.9-rc1/include/linux/ata.h
--- orig/linux-2.6.0/include/linux/ata.h	2004-08-30 18:31:32.000000000 +0400
+++ linux-2.6.9-rc1/include/linux/ata.h	2004-08-30 17:56:53.000000000 +0400
@@ -218,6 +218,7 @@
 };
 
 #define ata_id_is_ata(dev)	(((dev)->id[0] & (1 << 15)) == 0)
+#define ata_id_is_sata(dev)	((dev)->id[93] == 0)
 #define ata_id_rahead_enabled(dev) ((dev)->id[85] & (1 << 6))
 #define ata_id_wcache_enabled(dev) ((dev)->id[85] & (1 << 5))
 #define ata_id_has_flush(dev) ((dev)->id[83] & (1 << 12))

Re: [PATCH] libata ATA vs SATA detection and workaround.

[Brad Campbell]

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

Brad Campbell wrote:
> 
> It works here anyway.
> Patch against vanilla 2.6.9-rc1 attached (including your ata.h patch)
> 

Lets try this one that won't undo the work that the sata_sil Seagate/Maxtor blacklist does!

Regards,
Brad.

[-- Attachment #2: diff2 --]
[-- Type: text/plain, Size: 1509 bytes --]

diff -ur orig/linux/drivers/scsi/libata-core.c linux-2.6.9-rc1/drivers/scsi/libata-core.c
--- orig/linux/drivers/scsi/libata-core.c	2004-08-30 18:52:54.000000000 +0400
+++ linux-2.6.9-rc1/drivers/scsi/libata-core.c	2004-08-30 18:53:42.000000000 +0400
@@ -1150,6 +1150,15 @@
 		ata_dev_identify(ap, i);
 		if (ata_dev_present(&ap->device[i])) {
 			found = 1;
+			/* limit bridge transfers to udma5, 200 sectors */
+			if ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(ap->device))) {
+				printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
+					ap->id, ap->device->devno);
+				ap->udma_mask &= ATA_UDMA5;
+				ap->host->max_sectors = ATA_MAX_SECTORS;
+				ap->host->hostt->max_sectors = ATA_MAX_SECTORS;
+				ap->device->flags |= ATA_DFLAG_LOCK_SECTORS;
+			}
 			if (ap->ops->dev_config)
 				ap->ops->dev_config(ap, &ap->device[i]);
 		}
@@ -1226,7 +1235,7 @@
 		ata_port_disable(ap);
 		return;
 	}
-
+	ap->cbl = ATA_CBL_SATA;
 	ata_bus_reset(ap);
 }
 
diff -ur orig/linux/include/linux/ata.h linux-2.6.9-rc1/include/linux/ata.h
--- orig/linux/include/linux/ata.h	2004-08-30 18:52:54.000000000 +0400
+++ linux-2.6.9-rc1/include/linux/ata.h	2004-08-30 18:42:41.000000000 +0400
@@ -218,6 +218,7 @@
 };
 
 #define ata_id_is_ata(dev)	(((dev)->id[0] & (1 << 15)) == 0)
+#define ata_id_is_sata(dev)	((dev)->id[93] == 0)
 #define ata_id_rahead_enabled(dev) ((dev)->id[85] & (1 << 6))
 #define ata_id_wcache_enabled(dev) ((dev)->id[85] & (1 << 5))
 #define ata_id_has_flush(dev) ((dev)->id[83] & (1 << 12))

Re: [PATCH] libata ATA vs SATA detection and workaround.

[Jeff Garzik]

Brad Campbell wrote:
> +			/* limit bridge transfers to udma5, 200 sectors */
> +			if ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(ap->device))) {
> +				printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
> +					ap->id, ap->device->devno);
> +				ap->udma_mask &= ATA_UDMA5;
> +				ap->host->max_sectors = ATA_MAX_SECTORS;
> +				ap->host->hostt->max_sectors = ATA_MAX_SECTORS;
> +				ap->device->flags |= ATA_DFLAG_LOCK_SECTORS;
> +			}
>  			if (ap->ops->dev_config)
>  				ap->ops->dev_config(ap, &ap->device[i]);


Close!  Please move the entire quoted section, including the two lines 
of code calling ->dev_config(), into a new function 'ata_dev_config'. 
Export it (bottom of libata-core.c) and prototype it (libata.h) as well.

I'm still pondering what Alan was hinting at, a bit.  You (Brad) are 
correct in pointing out that this code should only trigger for the 
correct situations (lba48, etc.) which are only present on modern 
drives, but...  there is still a chance that word 93 will be zero on 
some weird (probably non-compliant) device.

However, Alan's comment is actually more relevant for unrelated sections 
of libata.  Whenever we test a feature bit in words 82-87, we should 
check for "word != 0 && word != 0xffff" which is how one knows the word 
is implemented.  There are no feature bits indicating that feature bits 
exist :)

	Jeff

Re: [PATCH] libata ATA vs SATA detection and workaround.

[Brad Campbell]

> I'm still pondering what Alan was hinting at, a bit.  You (Brad) are 
> correct in pointing out that this code should only trigger for the 
> correct situations (lba48, etc.) which are only present on modern 
> drives, but...  there is still a chance that word 93 will be zero on 
> some weird (probably non-compliant) device.

I agree completely, though my feeling is that if someone plugs a device that broken into a SATA 
controller via a bridge then there "aint nuffin we can do about it" anyway and if it breaks it 
breaks. I guess we could offer the option you suggested before where we load the individual drivers 
as modules and provide a "knobble" module parm that will limit max_sectors to 200 and udma_mask to 
udma/100.
Then we get the hassle if someone wants to use it as the root device, but I guess then you move to 
an initrd and load the module from there.

How far do we want to take it?

Regards,
Brad

Re: [PATCH] libata ATA vs SATA detection and workaround.

[Jeff Garzik]

Brad Campbell wrote:
> 
>> I'm still pondering what Alan was hinting at, a bit.  You (Brad) are 
>> correct in pointing out that this code should only trigger for the 
>> correct situations (lba48, etc.) which are only present on modern 
>> drives, but...  there is still a chance that word 93 will be zero on 
>> some weird (probably non-compliant) device.
> 
> 
> I agree completely, though my feeling is that if someone plugs a device 
> that broken into a SATA controller via a bridge then there "aint nuffin 
> we can do about it" anyway and if it breaks it breaks. I guess we could 
> offer the option you suggested before where we load the individual 
> drivers as modules and provide a "knobble" module parm that will limit 
> max_sectors to 200 and udma_mask to udma/100.
> Then we get the hassle if someone wants to use it as the root device, 
> but I guess then you move to an initrd and load the module from there.
> 
> How far do we want to take it?

For now I think moving your code to ata_dev_config() function is 
sufficient, with one modification:

Move the
	((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(ap->device)))
test into a separate function all its own, "ata_knobble_device" or 
somesuch.  static inline if you wish.

Then it will be trivial to add a 'knobble' module parm later on, by 
simply modifying ata_knobble_device() to also check the module parameter 
in addition to the existing tests.

	Jeff

Re: [PATCH] libata ATA vs SATA detection and workaround.

[Brad Campbell]

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

Jeff Garzik wrote:

> For now I think moving your code to ata_dev_config() function is 
> sufficient, with one modification:
> 
> Move the
>     ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(ap->device)))
> test into a separate function all its own, "ata_knobble_device" or 
> somesuch.  static inline if you wish.
> 
> Then it will be trivial to add a 'knobble' module parm later on, by 
> simply modifying ata_knobble_device() to also check the module parameter 
> in addition to the existing tests.

Pretty new at kernel code. (And C for that matter)
I did note that it appears it's not going to do the right thing if we have more than one device per 
host, but I guess thats not going to be an issue for SATA for the near future anyway.

How's this grab you?

Regards,
Brad

[-- Attachment #2: diff3 --]
[-- Type: text/plain, Size: 2939 bytes --]

diff -ur orig/linux-2.6.0/drivers/scsi/libata-core.c linux-2.6.9-rc1/drivers/scsi/libata-core.c
--- orig/linux-2.6.0/drivers/scsi/libata-core.c	2004-08-30 20:48:35.000000000 +0400
+++ linux-2.6.9-rc1/drivers/scsi/libata-core.c	2004-08-30 21:02:42.000000000 +0400
@@ -1128,6 +1128,37 @@
 	DPRINTK("EXIT, err\n");
 }
 
+
+static inline u8 ata_dev_knobble(struct ata_port *ap)
+{
+	return ((ap->cbl == ATA_CBL_SATA) && (!ata_id_is_sata(ap->device)));
+}
+
+/**
+ * 	ata_dev_config - Run device specific handlers and check for
+ * 			 SATA->PATA bridges
+ * 	@ap: Bus 
+ * 	@i:  Device
+ *
+ * 	LOCKING:
+ */
+ 
+void ata_dev_config(struct ata_port *ap, unsigned int i)
+{
+	/* limit bridge transfers to udma5, 200 sectors */
+	if (ata_dev_knobble(ap)) {
+		printk(KERN_INFO "ata%u(%u): applying bridge limits\n",
+			ap->id, ap->device->devno);
+		ap->udma_mask &= ATA_UDMA5;
+		ap->host->max_sectors = ATA_MAX_SECTORS;
+		ap->host->hostt->max_sectors = ATA_MAX_SECTORS;
+		ap->device->flags |= ATA_DFLAG_LOCK_SECTORS;
+	}
+
+	if (ap->ops->dev_config)
+		ap->ops->dev_config(ap, &ap->device[i]);
+}
+
 /**
  *	ata_bus_probe - Reset and probe ATA bus
  *	@ap: Bus to probe
@@ -1150,8 +1181,7 @@
 		ata_dev_identify(ap, i);
 		if (ata_dev_present(&ap->device[i])) {
 			found = 1;
-			if (ap->ops->dev_config)
-				ap->ops->dev_config(ap, &ap->device[i]);
+			ata_dev_config(ap,i);
 		}
 	}
 
@@ -1226,7 +1256,7 @@
 		ata_port_disable(ap);
 		return;
 	}
-
+	ap->cbl = ATA_CBL_SATA;
 	ata_bus_reset(ap);
 }
 
@@ -3567,3 +3597,4 @@
 EXPORT_SYMBOL_GPL(ata_scsi_release);
 EXPORT_SYMBOL_GPL(ata_host_intr);
 EXPORT_SYMBOL_GPL(ata_dev_id_string);
+EXPORT_SYMBOL_GPL(ata_dev_config);
nly in linux-2.6.9-rc1/include: config
diff -ur orig/linux-2.6.0/include/linux/ata.h linux-2.6.9-rc1/include/linux/ata.h
--- orig/linux-2.6.0/include/linux/ata.h	2004-08-30 20:48:35.000000000 +0400
+++ linux-2.6.9-rc1/include/linux/ata.h	2004-08-30 18:42:41.000000000 +0400
@@ -218,6 +218,7 @@
 };
 
 #define ata_id_is_ata(dev)	(((dev)->id[0] & (1 << 15)) == 0)
+#define ata_id_is_sata(dev)	((dev)->id[93] == 0)
 #define ata_id_rahead_enabled(dev) ((dev)->id[85] & (1 << 6))
 #define ata_id_wcache_enabled(dev) ((dev)->id[85] & (1 << 5))
 #define ata_id_has_flush(dev) ((dev)->id[83] & (1 << 12))
diff -ur orig/linux-2.6.0/include/linux/libata.h linux-2.6.9-rc1/include/linux/libata.h
--- orig/linux-2.6.0/include/linux/libata.h	2004-08-30 20:48:35.000000000 +0400
+++ linux-2.6.9-rc1/include/linux/libata.h	2004-08-30 20:42:05.000000000 +0400
@@ -400,6 +400,7 @@
 		 unsigned int n_elem);
 extern void ata_dev_id_string(struct ata_device *dev, unsigned char *s,
 			      unsigned int ofs, unsigned int len);
+extern void ata_dev_config(struct ata_port *ap, unsigned int i);
 extern void ata_bmdma_setup_mmio (struct ata_queued_cmd *qc);
 extern void ata_bmdma_start_mmio (struct ata_queued_cmd *qc);
 extern void ata_bmdma_setup_pio (struct ata_queued_cmd *qc);

Re: [PATCH] libata ATA vs SATA detection and workaround.

[Jeff Garzik]

Brad Campbell wrote:
> Pretty new at kernel code. (And C for that matter)

Since you've been so helpful, I choose to disbelieve this ;-)


> I did note that it appears it's not going to do the right thing if we 
> have more than one device per host, but I guess thats not going to be an 
> issue for SATA for the near future anyway.
> 
> How's this grab you?

Looks perfect.

Confirm for me that it solves your problem, and I'll push it upstream.

	Jeff

Re: [PATCH] libata ATA vs SATA detection and workaround.

[Brad Campbell]

Jeff Garzik wrote:
>>
>> How's this grab you?
> 
> 
> Looks perfect.
> 
> Confirm for me that it solves your problem, and I'll push it upstream.
> 

Well, I have done numerous reboots and beat it hard and it has not failed *yet*. Never say never.
So yes it works for me.

Regards,
Brad