libata dev_config call order wrong.

libata dev_config call order wrong.

[Brad Campbell]

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

Well, well, well - Three holes in the ground.

My bridge boards *are* choking on > 200 sector transfers.

I patched sata_via in the same way sata_sil was patched to tell the controller to use 200 as 
max_sectors the same way sata_sil uses 15 as max_sectors on the seagates.

The problem is that ata_scsi_slave_config in libata-scsi.c is getting called *after* .dev_config in 
sata_via and resetting my value of 200 to 2048.

If I hard code 200 into ata_scsi_slave_config then this issue never raises it's ugly head and I 
can't get the bridge boards to crash.

I wonder if this might be happening in sata_sil also?

I added a simple printk in ata_scsi_slave_config to print "Slave Config!!!"
See attached log. I have also attached my patch to sata_via for sanity checking, in case I did 
something obviously wrong.

ata9: no device found (phy stat 00000000)
scsi8 : sata_promise
ata10: no device found (phy stat 00000000)
scsi9 : sata_promise
ata11: no device found (phy stat 00000000)
scsi10 : sata_promise
ata12: no device found (phy stat 00000000)
scsi11 : sata_promise
ACPI: PCI interrupt 0000:00:0f.0[B] -> GSI 20 (level, low) -> IRQ 20
sata_via(0000:00:0f.0): routed to hard irq line 0
ata13: SATA max UDMA/133 cmd 0x8800 ctl 0x8402 bmdma 0x7400 irq 20
ata14: SATA max UDMA/133 cmd 0x8000 ctl 0x7802 bmdma 0x7408 irq 20
ata13: dev 0 ATA, max UDMA/100, 390622887 sectors: lba48

ata13(0): applying WD errata fix

ata13: dev 0 configured for UDMA/100
scsi12 : sata_via
ata14: no device found (phy stat 00000000)
scsi13 : sata_via
   Vendor: ATA       Model: WDC WD2000JB-00D  Rev: 02.1
   Type:   Direct-Access                      ANSI SCSI revision: 05

Slave Config!!!

SCSI device sda: 390622887 512-byte hdwr sectors (199999 MB)
SCSI device sda: drive cache: write back
  /dev/scsi/host12/bus0/target0/lun0: unknown partition table
Attached scsi disk sda at scsi12, channel 0, id 0, lun 0
Attached scsi generic sg0 at scsi12, channel 0, id 0, lun 0,  type 0

Regards,
Brad

[-- Attachment #2: sata.patch --]
[-- Type: text/plain, Size: 2503 bytes --]

--- orig/linux-2.6.0/drivers/scsi/sata_via.c	2004-08-26 22:21:56.000000000 +0400
+++ linux-2.6.9-rc1/drivers/scsi/sata_via.c	2004-08-26 22:14:19.000000000 +0400
@@ -59,10 +59,13 @@
 
 	SATA_EXT_PHY		= (1 << 6), /* 0==use PATA, 1==ext phy */
 	SATA_2DEV		= (1 << 5), /* SATA is master/slave */
+	SVIA_QUIRK_200SECTS	= (1 << 0),
+
 };
 
 static int svia_init_one (struct pci_dev *pdev, const struct pci_device_id *ent);
 static u32 svia_scr_read (struct ata_port *ap, unsigned int sc_reg);
+static void svia_dev_config(struct ata_port *ap, struct ata_device *dev);
 static void svia_scr_write (struct ata_port *ap, unsigned int sc_reg, u32 val);
 
 static struct pci_device_id svia_pci_tbl[] = {
@@ -71,6 +74,14 @@
 	{ }	/* terminate list */
 };
 
+struct svia_drivelist {
+        const char * product;
+        unsigned int quirk;
+} svia_blacklist [] = {
+        { "WDC WD2000JB",         SVIA_QUIRK_200SECTS },
+        { }
+};
+
 static struct pci_driver svia_pci_driver = {
 	.name			= DRV_NAME,
 	.id_table		= svia_pci_tbl,
@@ -122,6 +133,8 @@
 
 	.port_start		= ata_port_start,
 	.port_stop		= ata_port_stop,
+	.dev_config		= svia_dev_config,
+
 };
 
 MODULE_AUTHOR("Jeff Garzik");
@@ -283,6 +296,40 @@
 	return rc;
 }
 
+static void svia_dev_config(struct ata_port *ap, struct ata_device *dev)
+{
+        unsigned int n, quirks = 0;
+        unsigned char model_num[40];
+        const char *s;
+        unsigned int len;
+
+        ata_dev_id_string(dev, model_num, ATA_ID_PROD_OFS,
+                          sizeof(model_num));
+        s = &model_num[0];
+        len = strnlen(s, sizeof(model_num));
+
+        /* ATAPI specifies that empty space is blank-filled; remove blanks */
+        while ((len > 0) && (s[len - 1] == ' '))
+                len--;
+
+        for (n = 0; svia_blacklist[n].product; n++) 
+                if (!memcmp(svia_blacklist[n].product, s,
+                            strlen(svia_blacklist[n].product))) {
+                        quirks = svia_blacklist[n].quirk;
+                        break;
+                }
+        
+        /* limit requests to 200 sectors */
+        if (quirks & SVIA_QUIRK_200SECTS) {
+                printk(KERN_INFO "ata%u(%u): applying WD errata fix\n",
+                       ap->id, dev->devno);
+                ap->host->max_sectors = 200;
+                ap->host->hostt->max_sectors = 200;
+                return;
+        }
+
+}
+
 static int __init svia_init(void)
 {
 	return pci_module_init(&svia_pci_driver);

Re: libata dev_config call order wrong.

[Jeff Garzik]

Brad Campbell wrote:
> Well, well, well - Three holes in the ground.
> 
> My bridge boards *are* choking on > 200 sector transfers.
> 
> I patched sata_via in the same way sata_sil was patched to tell the 
> controller to use 200 as max_sectors the same way sata_sil uses 15 as 
> max_sectors on the seagates.
> 
> The problem is that ata_scsi_slave_config in libata-scsi.c is getting 
> called *after* .dev_config in sata_via and resetting my value of 200 to 
> 2048.

Take a look at what ATA_DFLAG_LOCK_SECTORS does ;-)  Here's hoping 
that's the last piece of the puzzle...

BTW I just sent email to SiI to see if there is a way to detect a 
PATA->SATA bridge.

	Jeff

Re: libata dev_config call order wrong.

[Brad Campbell]

Jeff Garzik wrote:
> 
> Take a look at what ATA_DFLAG_LOCK_SECTORS does ;-)  Here's hoping 
> that's the last piece of the puzzle...

HAHA! He says. That was not in 2.6.8.1! That solves that one.

Frankly, now I'm running a WD2500BB on one of those converters on a RAID-5 I'm just going to leave 
the 200 sector hack in my kernel until we figure out a way to detect the bridges. I can't see any 
other clean way of doing it.

> BTW I just sent email to SiI to see if there is a way to detect a 
> PATA->SATA bridge.

Man I hope so.. I don't want to resort to having to throw these board away or constantly patch my 
kernel! I could just go and buy some more SATA drives I guess, but where would the fun be in that!

Oh well. I'm far more familiar with the libata, scsi and block layer internals now. And I can muddle 
my way through bk. *PLUS* I did not actually lose any data, so it's all been good really.

I'm almost tempted to buy some of the other bridges on the market to see if the exhibit the same 
behaviour. The data I can find on these Sil chips makes no mention of any limitations (mind you the 
data I can find is pretty poor all round)

Cheers for all the encouragement.

If your ever flying through Dubai, stop in for a beer :p)

Regards,
Brad

Re: libata dev_config call order wrong.

[Jeff Garzik]

Brad Campbell wrote:
> Jeff Garzik wrote:
> 
>>
>> Take a look at what ATA_DFLAG_LOCK_SECTORS does ;-)  Here's hoping 
>> that's the last piece of the puzzle...
> 
> 
> HAHA! He says. That was not in 2.6.8.1! That solves that one.
> 
> Frankly, now I'm running a WD2500BB on one of those converters on a 
> RAID-5 I'm just going to leave the 200 sector hack in my kernel until we 
> figure out a way to detect the bridges. I can't see any other clean way 
> of doing it.

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.

Honestly I would prefer to apply some "if it's PATA-over-SATA bridge" 
rules, namely,
* limit to udma5 (udma/100)
* limit to 256 sectors [actually 200 in libata]


>> BTW I just sent email to SiI to see if there is a way to detect a 
>> PATA->SATA bridge.
> 
> 
> Man I hope so.. I don't want to resort to having to throw these board 
> away or constantly patch my kernel! I could just go and buy some more 
> SATA drives I guess, but where would the fun be in that!

hehe

Another option is to have a "no large transfers" module option that 
users could specify; that way you wouldn't have to patch the kernel, and 
it would work for all drivers without having to maintain a blacklist.

	Jeff

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