libata bridge limits
Hi,
Jeff, a long time ago we talked about the bridge limits in libata core. Back then it was my T43 notebook, but now I'm running into the same problem. The Mtron 70xx series SSD drives sit behind a PATA-SATA bridge and thus runs into the bridging limits in libata. To put things into perspective, the difference between UDMA5 and UDMA6 modes on this drive is 85 vs 115Mb/sec or 77 vs 106Mb/sec (depends on the host controller I tested), so it's quite a lot.
There are several ways to solve this:
a) Why was this limit put in there? It limits both transfer speed and request size. If it's due to some dodgy drive/bridge, perhaps we should just check for that and only apply the transfer limits when detected (or blacklisted). On the bridge setups I've seen, I've never had problems with killing the limit.
b) Put in a whitelist, easy to do for these Mtron drives.
c) Add a parameter to turn it on (or off, depending on the default) for a specific drive.
I'm in favor of a) personally, but I'd like to hear why the check was added originally first. Dropping 20-30% of the throughput performance on the floor without option seems like a really bad choice.
Signature · 1 line
Jens Axboe
Quoted 5 lines
a) Why was this limit put in there? It limits both transfer speed and request size. If it's due to some dodgy drive/bridge, perhaps we should just check for that and only apply the transfer limits when detected (or blacklisted). On the bridge setups I've seen, I've never had problems with killing the limit.
Various old bridges need it - and you can't detect the bridge type.
Quoted 9 lines
b) Put in a whitelist, easy to do for these Mtron drives.
c) Add a parameter to turn it on (or off, depending on the default) for a specific drive.
I'm in favor of a) personally, but I'd like to hear why the check was added originally first. Dropping 20-30% of the throughput performance on the floor without option seems like a really bad choice.
Can I suggest
d) Assume the bridge is ok but teach the SATA error handling code that if there is a timeout immediately with such a bridge then to flip down to UDMA5 and knobble the transfer length.
Quoted 7 lines · Alan Cox
Quoted 5 lines
a) Why was this limit put in there? It limits both transfer speed and request size. If it's due to some dodgy drive/bridge, perhaps we should just check for that and only apply the transfer limits when detected (or blacklisted). On the bridge setups I've seen, I've never had problems with killing the limit.
Various old bridges need it - and you can't detect the bridge type.
Not generically, but for some devices (like the Mtron) we can.
Quoted 9 lines
Quoted 8 lines
b) Put in a whitelist, easy to do for these Mtron drives.
c) Add a parameter to turn it on (or off, depending on the default) for a specific drive.
I'm in favor of a) personally, but I'd like to hear why the check was added originally first. Dropping 20-30% of the throughput performance on the floor without option seems like a really bad choice.
Quoted 5 lines
Can I suggest
d) Assume the bridge is ok but teach the SATA error handling code that if there is a timeout immediately with such a bridge then to flip down to UDMA5 and knobble the transfer length.
That would be nice, assuming that we can rely on safe behaviour (eg not data corrupting badness).
Signature · 1 line
Jens Axboe
(cc'ing Mark Lord, Kay Sievers and Gwendal Grignou)
Quoted 6 lines · Jens Axboe
Quoted 3 lines
d) Assume the bridge is ok but teach the SATA error handling code that if there is a timeout immediately with such a bridge then to flip down to UDMA5 and knobble the transfer length.
That would be nice, assuming that we can rely on safe behaviour (eg not data corrupting badness).
Obstacles to such approaches are...
* The current IO timeouts are too long. It's not like reducing this is difficult. The only reason why we haven't reduced it yet is because we haven't been able to agree on what's the proper timeout value. According to Mark, 8 secs should be fine (Windows uses it) for read/writes but there seem to be some corner cases.
* Some rare controllers fail miserably after a timeout but this is pretty rare and getting rarer. I don't think we need to consider this the main deciding factor.
* Currently, the transfer speed setting reached by EH actions is not persistent. On the next boot, the device would have to go through the same thing all over again, which isn't too pleasant. It would be great if we can make this setting persistent. Maybe this can be done libata sysfs and udev?
Thanks.
Signature · 1 line
tejun
Quoted 2 lines
* The current IO timeouts are too long. It's not like reducing this is difficult. The only reason why we haven't reduced it yet is because we
They are too short.
Quoted 3 lines
haven't been able to agree on what's the proper timeout value. According to Mark, 8 secs should be fine (Windows uses it) for read/writes but there seem to be some corner cases.
The worst case I've seen on bad blocks is up over sixty seconds and as a result of our underlength timeouts we get continuous retries and mode changedowns in response to this not a proper error and raid failover. The worst case on cache flush is even longer!
We have the same problem with CD devices.
Now we should probably have a shorter timeout where we then check the status bits for BUSY so we can quickly catch lost interrupts or commands but that is quite different.
Quoted 3 lines
* Some rare controllers fail miserably after a timeout but this is pretty rare and getting rarer. I don't think we need to consider this the main deciding factor.
Several require resets, the driver should be doing this work. Again the poll on timeout to check if we just lost the IRQ would improve this also but is only done by old IDE right now.
Quoted 5 lines
* Currently, the transfer speed setting reached by EH actions is not persistent. On the next boot, the device would have to go through the same thing all over again, which isn't too pleasant. It would be great if we can make this setting persistent. Maybe this can be done libata sysfs and udev?
How ? you've no idea what device combination will appear next boot ?
Alan
Quoted 5 lines · Alan Cox
Quoted 2 lines
* The current IO timeouts are too long. It's not like reducing this is difficult. The only reason why we haven't reduced it yet is because we
They are too short.
Quoted 8 lines
Quoted 3 lines
haven't been able to agree on what's the proper timeout value. According to Mark, 8 secs should be fine (Windows uses it) for read/writes but there seem to be some corner cases.
The worst case I've seen on bad blocks is up over sixty seconds and as a result of our underlength timeouts we get continuous retries and mode changedowns in response to this not a proper error and raid failover. The worst case on cache flush is even longer!
We probably needs to extend timeout for flush or at least when retrying flushes. As for reads and writes, I really think we should move more towards shorter timeout as timeouts are not too rare in SATA at least (and undistinguishible using BSY or other methods). Those long delays are very rare and maybe having control knobs is the best way to deal with them.
Quoted 5 lines
We have the same problem with CD devices.
Now we should probably have a shorter timeout where we then check the status bits for BUSY so we can quickly catch lost interrupts or commands but that is quite different.
Yeah, we need to check for lost interrupts and dead IRQ due to screaming IRQ. Maybe we can do some of that in interrupt core.
Quoted 7 lines
Quoted 3 lines
* Some rare controllers fail miserably after a timeout but this is pretty rare and getting rarer. I don't think we need to consider this the main deciding factor.
Several require resets, the driver should be doing this work. Again the poll on timeout to check if we just lost the IRQ would improve this also but is only done by old IDE right now.
The few I was talking about just freezes the whole machine after a timeout. Dunno whether the lowlevel driver needs to do EH differently or the controller is just built that way tho.
Quoted 7 lines
Quoted 5 lines
* Currently, the transfer speed setting reached by EH actions is not persistent. On the next boot, the device would have to go through the same thing all over again, which isn't too pleasant. It would be great if we can make this setting persistent. Maybe this can be done libata sysfs and udev?
How ? you've no idea what device combination will appear next boot ?
udev and hal know a lot about the system configuration including connection topology and many ways to id each device. Saving limit configuration using combination of topology and device ID should be pretty safe. I think more difficult problem is how to notify the user that such persistent auto-configuration happened and provide a convenient way to undo.
Thanks.
Signature · 1 line
tejun
Quoted 6 lines
Quoted 3 lines
Now we should probably have a shorter timeout where we then check the status bits for BUSY so we can quickly catch lost interrupts or commands but that is quite different.
Yeah, we need to check for lost interrupts and dead IRQ due to screaming IRQ. Maybe we can do some of that in interrupt core.
For SFF at least we can read altstatus and check for BUSY. If BUSY is not set then something is up, either we have an error we didn't get an IRQ for or the status is successful.
Quoted 3 lines
The few I was talking about just freezes the whole machine after a timeout. Dunno whether the lowlevel driver needs to do EH differently or the controller is just built that way tho.
Some lock the box solid if you don't reset the controller before you touch any registers on a timeout. I thought we had them all covered - do you know which controllers are still showing this ?
Alan
Quoted 10 lines · Alan Cox
Quoted 5 lines
Quoted 3 lines
Now we should probably have a shorter timeout where we then check the status bits for BUSY so we can quickly catch lost interrupts or commands but that is quite different.
Yeah, we need to check for lost interrupts and dead IRQ due to screaming IRQ. Maybe we can do some of that in interrupt core.
For SFF at least we can read altstatus and check for BUSY. If BUSY is not set then something is up, either we have an error we didn't get an IRQ for or the status is successful.
Quoted 7 lines
Quoted 3 lines
The few I was talking about just freezes the whole machine after a timeout. Dunno whether the lowlevel driver needs to do EH differently or the controller is just built that way tho.
Some lock the box solid if you don't reset the controller before you touch any registers on a timeout. I thought we had them all covered - do you know which controllers are still showing this ?
IIRC some very early via SATAs lock up the machine solid no matter what you do unless a command is completed by the device side. Maybe it requires special sequence to abort in-flight commands but simple SRST wasn't enough. :-(
Signature · 1 line
tejun
The current patch to implement libata-sysfs floating in the mailing list is read-only.
For write we must currently go through SCSI midlayer ioctls and use sat. To send commands directly within libata[-sysfs] - for instance writing SControl register of a SATA PM - I need to either implement a queue mechanism over ata_exec_internal() or schedule eh and piggy back on it.
Once implemented, we could definitely use it in conjunction with udev to tune ATA devices.
Gwendal.
Quoted 33 lines · Tejun Heo
(cc'ing Mark Lord, Kay Sievers and Gwendal Grignou)
Quoted 6 lines · Jens Axboe
Quoted 3 lines
d) Assume the bridge is ok but teach the SATA error handling code that if there is a timeout immediately with such a bridge then to flip down to UDMA5 and knobble the transfer length.
That would be nice, assuming that we can rely on safe behaviour (eg not data corrupting badness).
Obstacles to such approaches are...
* The current IO timeouts are too long. It's not like reducing this is difficult. The only reason why we haven't reduced it yet is because we haven't been able to agree on what's the proper timeout value. According to Mark, 8 secs should be fine (Windows uses it) for read/writes but there seem to be some corner cases.
* Some rare controllers fail miserably after a timeout but this is pretty rare and getting rarer. I don't think we need to consider this the main deciding factor.
* Currently, the transfer speed setting reached by EH actions is not persistent. On the next boot, the device would have to go through the same thing all over again, which isn't too pleasant. It would be great if we can make this setting persistent. Maybe this can be done libata sysfs and udev?
Thanks.
--
tejun
Quoted 7 lines · Gwendal Grignou
The current patch to implement libata-sysfs floating in the mailing list is read-only.
For write we must currently go through SCSI midlayer ioctls and use sat. To send commands directly within libata[-sysfs] - for instance writing SControl register of a SATA PM - I need to either implement a queue mechanism overata_exec_internal()or schedule eh and piggy back on it.
That's not how the other transport classes work, why does the libata one need to go through the mid-layer (well, except for the necessary command issue stuff, of course). The design of the transport classes was specifically to take all transport knowledge out of the mid layer and connect the requisite controls in sysfs directly to the transport actions.
The (semi)direct analogue of writing SControl of a SATA PM is writing a phy control parameter of a SAS expander ... that's all accomplished in libsas/scsi_transport_sata without ever going via the mid-layer.
Quoted 2 lines
Once implemented, we could definitely use it in conjunction with udev to tune ATA devices.
James
Quoted 9 lines · James Bottomley
Quoted 7 lines · Gwendal Grignou
The current patch to implement libata-sysfs floating in the mailing list is read-only.
For write we must currently go through SCSI midlayer ioctls and use sat. To send commands directly within libata[-sysfs] - for instance writing SControl register of a SATA PM - I need to either implement a queue mechanism overata_exec_internal()or schedule eh and piggy back on it.
Quoted 6 lines
That's not how the other transport classes work, why does the libata one need to go through the mid-layer (well, except for the necessary command issue stuff, of course). The design of the transport classes was specifically to take all transport knowledge out of the mid layer and connect the requisite controls in sysfs directly to the transport actions.
I agree. I don't want to use the mid-layer either. I just mentioned that if I want to send a raw taskfile to a ATA device today, the only way I found is using ata_task_ioctl() which relies on the mid-layer to schedule the request; the direct way - through ata_exec_internal() - assumes only one outstanding command at a time. I need to implement something else for the accessing ATA devices in libata-sysfs similar to smp_execute_task() in libsas.
Quoted 4 lines
The (semi)direct analogue of writing SControl of a SATA PM is writing a phy control parameter of a SAS expander ... that's all accomplished in libsas/scsi_transport_sata without ever going via the mid-layer.
Agree.
Gwendal.
Quoted 8 lines
Quoted 3 lines
Once implemented, we could definitely use it in conjunction with udev to tune ATA devices.
James
Quoted 17 lines · Gwendal Grignou
Quoted 9 lines · James Bottomley
Quoted 7 lines · Gwendal Grignou
The current patch to implement libata-sysfs floating in the mailing list is read-only.
For write we must currently go through SCSI midlayer ioctls and use sat. To send commands directly within libata[-sysfs] - for instance writing SControl register of a SATA PM - I need to either implement a queue mechanism overata_exec_internal()or schedule eh and piggy back on it.Quoted 6 lines
That's not how the other transport classes work, why does the libata one need to go through the mid-layer (well, except for the necessary command issue stuff, of course). The design of the transport classes was specifically to take all transport knowledge out of the mid layer and connect the requisite controls in sysfs directly to the transport actions.
Quoted 7 lines
I agree. I don't want to use the mid-layer either. I just mentioned that if I want to send a raw taskfile to a ATA device today, the only way I found is using
ata_task_ioctl()which relies on the mid-layer to schedule the request; the direct way - throughata_exec_internal()- assumes only one outstanding command at a time. I need to implement something else for the accessing ATA devices in libata-sysfs similar tosmp_execute_task()in libsas.
Actually, no ... if you're sending a SATA task, then you're right, you need to use the mid-layer inputs. scsi_execute() is the way transport classes use this (see scsi_transport_spi ... it does exactly this for domain validation). ata_task_ioctl() does it exactly this way.
You don't want something like smp_execute_task because that's an internal libsas API for doing out of band management tasks ... ATA has no concept of anything like this. Even our publicly exposed frame in/frame out interfaces for performing SMP tasks go via a special API stream in the transport class framework so they can be issued on all SAS hosts. Mechanistically, they're different from normal SCSI commands because they don't count against the queuing quota or have ordinary tags. If your commands come out again as standard ATA commands, I suspect they have to go through the standard ATA quota machinery.
SAS is very different from SATA in that we have tons of addressable ports sitting off the devices, each addressable by predefined protocols, so what smp_execute_task is doing is addressing the SMP port ... this is completely out of band as far as the normal SSP/STP protocols see things. With port multipliers, you're more or less stuck with an in-band type of packet.
James
Quoted 18 lines · Jens 2 wrote:
Quoted 6 lines · Alan Cox
Quoted 5 lines
a) Why was this limit put in there? It limits both transfer speed and request size. If it's due to some dodgy drive/bridge, perhaps we should just check for that and only apply the transfer limits when detected (or blacklisted). On the bridge setups I've seen, I've never had problems with killing the limit.
Various old bridges need it - and you can't detect the bridge type.
Not generically, but for some devices (like the Mtron) we can.
Quoted 8 lines
Quoted 8 lines
b) Put in a whitelist, easy to do for these Mtron drives.
c) Add a parameter to turn it on (or off, depending on the default) for a specific drive.
I'm in favor of a) personally, but I'd like to hear why the check was added originally first. Dropping 20-30% of the throughput performance on the floor without option seems like a really bad choice.
The check was originally put there as some nasty bridges caused all sorts of errors when these limits were exceeded, including some random data corruption from memory.
Those particular bridges were/are still widely available an can't be detected / identified using any other means.
Quoted 9 lines
Quoted 5 lines
Can I suggest
d) Assume the bridge is ok but teach the SATA error handling code that if there is a timeout immediately with such a bridge then to flip down to UDMA5 and knobble the transfer length.
That would be nice, assuming that we can rely on safe behaviour (eg not data corrupting badness).
I was responsible for that original bridge knobble stuff and fortunately I still have the bridges, disks and controllers that triggered the original faults. If someone wants to cook up some code for testing I'd be more than happy to stick this stuff on the bench and beat on it for regression purposes.
Brad
Signature · 3 lines
Dolphins are so intelligent that within a few weeks they can train Americans to stand at the edge of the pool and throw them fish.
Quoted 20 lines · Brad Campbell
Quoted 18 lines · Jens 2 wrote:
Quoted 6 lines · Alan Cox
Quoted 5 lines
a) Why was this limit put in there? It limits both transfer speed and request size. If it's due to some dodgy drive/bridge, perhaps we should just check for that and only apply the transfer limits when detected (or blacklisted). On the bridge setups I've seen, I've never had problems with killing the limit.
Various old bridges need it - and you can't detect the bridge type.
Not generically, but for some devices (like the Mtron) we can.
Quoted 8 lines
Quoted 8 lines
b) Put in a whitelist, easy to do for these Mtron drives.
c) Add a parameter to turn it on (or off, depending on the default) for a specific drive.
I'm in favor of a) personally, but I'd like to hear why the check was added originally first. Dropping 20-30% of the throughput performance on the floor without option seems like a really bad choice.
Quoted 6 lines
The check was originally put there as some nasty bridges caused all sorts of errors when these limits were exceeded, including some random data corruption from memory.
Those particular bridges were/are still widely available an can't be detected / identified using any other means.
That's a worry, since I don't think we can dynamically switch back in that case if it has potential data corruption problems.
Quoted 10 lines
Quoted 9 lines
Quoted 5 lines
Can I suggest
d) Assume the bridge is ok but teach the SATA error handling code that if there is a timeout immediately with such a bridge then to flip down to UDMA5 and knobble the transfer length.
That would be nice, assuming that we can rely on safe behaviour (eg not data corrupting badness).
Quoted 5 lines
I was responsible for that original bridge knobble stuff and fortunately I still have the bridges, disks and controllers that triggered the original faults. If someone wants to cook up some code for testing I'd be more than happy to stick this stuff on the bench and beat on it for regression purposes.
Given that this problem should be going away and that it only really matters on very select devices (like this SSD), I think we should just add a quick white list for the bridge limits.
Below is a quick'n dirty for that...
drivers/ata/libata-core.c +61 -29
@@ -2097,9 +2097,70 @@ retry:return rc; } +struct ata_blacklist_entry { +const char *model_num; +const char *model_rev; +unsigned long horkage; +}; + +static const struct ata_blacklist_entry ata_bridge_whitelist[] = { +/* +* The following devices sit behind a bridge, but don't need +* transfer rate or size limits applied. +*/ + {"Mtron" , }, + +/* End Marker */ + { } +}; + +static int strn_pattern_cmp (const char *patt,const char *name,int wildchar) +{ +const char *p; +int len; + +/* +* check for trailing wildcard: *\0 +*/ + p =strchr (patt, wildchar); +if (p && ((*(p +1 )) ==0 )) + len = p - patt; +else { + len =strlen (name); +if (!len) { +if (!*patt) +return 0 ; +return -1 ; + } + } + +return strncmp (patt, name, len); +} + +static unsigned int ata_dev_bridge_whitelisted (const struct ata_device *dev) +{ +unsigned char model_num[ATA_ID_PROD_LEN +1 ]; +unsigned char model_rev[ATA_ID_FW_REV_LEN +1 ]; +const struct ata_blacklist_entry *ad = ata_bridge_whitelist; + +ata_id_c_string (dev->id, model_num,ATA_ID_PROD ,sizeof (model_num)); +ata_id_c_string (dev->id, model_rev,ATA_ID_FW_REV ,sizeof (model_rev)); + +while (ad->model_num) { +if (!strn_pattern_cmp (ad->model_num, model_num,'*' )) +return 1 ; + ad++; + } +return 0 ; +} +static inline u8 ata_dev_knobble (struct ata_device *dev) {struct ata_port *ap = dev->link->ap; + +if (ata_dev_bridge_whitelisted (dev)) +return 0 ; +return ((ap->cbl ==ATA_CBL_SATA ) && (!ata_id_is_sata (dev->id))); }
@@ -3913,12 +3974,6 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,return rc; } -struct ata_blacklist_entry { -const char *model_num; -const char *model_rev; -unsigned long horkage; -}; -static const struct ata_blacklist_entry ata_device_blacklist [] = {/* Devices with DMA related problems under Linux */ {"WDC AC11000H" ,NULL ,ATA_HORKAGE_NODMA },
@@ -4002,29 +4057,6 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = { { } }; -static int strn_pattern_cmp (const char *patt,const char *name,int wildchar) -{ -const char *p; -int len; - -/* -* check for trailing wildcard: *\0 -*/ - p =strchr (patt, wildchar); -if (p && ((*(p +1 )) ==0 )) - len = p - patt; -else { - len =strlen (name); -if (!len) { -if (!*patt) -return 0 ; -return -1 ; - } - } - -return strncmp (patt, name, len); -} -static unsigned long ata_dev_blacklisted (const struct ata_device *dev) {unsigned char model_num[ATA_ID_PROD_LEN +1 ];
Signature · 1 line
Jens Axboe
Quoted 3 lines · Jens Axboe
Given that this problem should be going away and that it only really matters on very select devices (like this SSD), I think we should just add a quick white list for the bridge limits.
Yeah, it sucks that up & coming SSDs are still using PATA-SATA bridges.
The expectation when adding the wildcard limitation was that those P/S
bridges are not gonna be around for too long and the limit is most likely not be an actual problem. Oh well...
Quoted 16 lines
Below is a quick'n dirty for that...
drivers/ata/libata-core.c +6
-0@@ -2097,9 +2097,70 @@ retry:return rc; } +struct ata_blacklist_entry { +const char *model_num; +const char *model_rev; +unsigned long horkage; +}; +
drivers/ata/libata-core.c retry: · line 2115
Quoted 10 lines
drivers/ata/libata-core.c +10
-0+static const struct ata_blacklist_entry ata_bridge_whitelist[] = { +/* +* The following devices sit behind a bridge, but don't need +* transfer rate or size limits applied. +*/ + {"Mtron" , }, + +/* End Marker */ + { } +};
Any reason this can't be part of the existing blacklist? It already supports wildcard matching and all.
Thanks.
Signature · 1 line
tejun
Quoted 9 lines · Tejun Heo
Quoted 3 lines · Jens Axboe
Given that this problem should be going away and that it only really matters on very select devices (like this SSD), I think we should just add a quick white list for the bridge limits.
Yeah, it sucks that up & coming SSDs are still using PATA-SATA bridges.
The expectation when adding the wildcard limitation was that those P/S
bridges are not gonna be around for too long and the limit is most likely not be an actual problem. Oh well...
I would hope that only the current generation do that, and to be honest I'm still pretty baffled that they exist :-)
Quoted 21 lines
Quoted 21 lines
Below is a quick'n dirty for that...
drivers/ata/libata-core.c +11
-0@@ -2097,9 +2097,70 @@ retry:return rc; } +struct ata_blacklist_entry { +const char *model_num; +const char *model_rev; +unsigned long horkage; +}; + +static const struct ata_blacklist_entry ata_bridge_whitelist[] = { +/* +* The following devices sit behind a bridge, but don't need +* transfer rate or size limits applied. +*/
Quoted 8 lines
drivers/ata/libata-core.c retry: · line 2115
Quoted 5 lines
drivers/ata/libata-core.c +5
-0+ { "Mtron", }, + +/* End Marker */ + { } +};Any reason this can't be part of the existing blacklist? It already supports wildcard matching and all.
Sure, just with an inverse flag, I'll do that.
Signature · 1 line
Jens Axboe
Quoted 44 lines · Jens Axboe
Quoted 9 lines · Tejun Heo
Quoted 3 lines · Jens Axboe
Given that this problem should be going away and that it only really matters on very select devices (like this SSD), I think we should just add a quick white list for the bridge limits.
Yeah, it sucks that up & coming SSDs are still using PATA-SATA bridges.
The expectation when adding the wildcard limitation was that those P/S
bridges are not gonna be around for too long and the limit is most likely not be an actual problem. Oh well...I would hope that only the current generation do that, and to be honest I'm still pretty baffled that they exist :-)
Quoted 21 lines
Quoted 21 lines
Below is a quick'n dirty for that...
drivers/ata/libata-core.c +11
-0@@ -2097,9 +2097,70 @@ retry:return rc; } +struct ata_blacklist_entry { +const char *model_num; +const char *model_rev; +unsigned long horkage; +}; + +static const struct ata_blacklist_entry ata_bridge_whitelist[] = { +/* +* The following devices sit behind a bridge, but don't need +* transfer rate or size limits applied. +*/ Quoted 8 lines
drivers/ata/libata-core.c retry: · line 2115
Quoted 5 lines
drivers/ata/libata-core.c +5
-0+ { "Mtron", }, + +/* End Marker */ + { } +};Any reason this can't be part of the existing blacklist? It already supports wildcard matching and all.
Quoted 1 line
Sure, just with an inverse flag, I'll do that.
OK, something like this. Jeff, if you think this is fine, let me know and I'll submit a proper patch with description and so on.
drivers/ata/libata-core.c +7 -0
@@ -2100,6 +2100,10 @@ retry:static inline u8 ata_dev_knobble (struct ata_device *dev) {struct ata_port *ap = dev->link->ap; + +if (ata_dev_blacklisted (dev) &ATA_HORKAGE_BRIDGE_OK ) +return 0 ; +return ((ap->cbl ==ATA_CBL_SATA ) && (!ata_id_is_sata (dev->id))); }
@@ -3998,6 +4002,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = { { "TSSTcorp CDDVDW SH-S202N", "SB00", ATA_HORKAGE_IVB, }, { "TSSTcorp CDDVDW SH-S202N", "SB01", ATA_HORKAGE_IVB, }, + /* Devices that do not need bridging limits applied */ + { "Mtron", NULL, 0, }, + /* End Marker */ { } };
include/linux/libata.h +1 -0
@@ -364,6 +364,7 @@ enum {ATA_HORKAGE_IPM = (1 <<7 ),/* Link PM problems */ ATA_HORKAGE_IVB = (1 <<8 ),/* cbl det validity bit bugs */ ATA_HORKAGE_STUCK_ERR = (1 <<9 ),/* stuck ERR on next PACKET */ + ATA_HORKAGE_BRIDGE_OK = (1 << 10), /* no bridge limits *//* DMA mask for user DMA control: User visible values; DO NOT renumber */
Signature · 1 line
Jens Axboe
Quoted 2 lines · Jens Axboe
OK, something like this. Jeff, if you think this is fine, let me know and I'll submit a proper patch with description and so on.
FWIW, looks good to me. Just one small concern below...
Quoted 16 lines
drivers/ata/libata-core.c +4
-0@@ -2100,6 +2100,10 @@ retry:static inline u8 ata_dev_knobble (struct ata_device *dev) {struct ata_port *ap = dev->link->ap; + +if (ata_dev_blacklisted (dev) &ATA_HORKAGE_BRIDGE_OK ) +return 0 ; +return ((ap->cbl ==ATA_CBL_SATA ) && (!ata_id_is_sata (dev->id))); }
drivers/ata/libata-core.c static const struct ata_blacklist_entry ata_device_blacklist [] = { · line 4006
Quoted 5 lines
drivers/ata/libata-core.c +2
-0@@ -3998,6 +4002,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = { { "TSSTcorp CDDVDW SH-S202N", "SB00", ATA_HORKAGE_IVB, }, { "TSSTcorp CDDVDW SH-S202N", "SB01", ATA_HORKAGE_IVB, }, + /* Devices that do not need bridging limits applied */ + { "Mtron", NULL, 0, },
"Mtron" looks like a broad match but then again for some reason many non-traditional ATA vendors don't like to give descriptive identifications to their devices. Any chance there is something more to match? :-(
Thanks.
Signature · 1 line
tejun
Quoted 25 lines · Tejun Heo
Quoted 2 lines · Jens Axboe
OK, something like this. Jeff, if you think this is fine, let me know and I'll submit a proper patch with description and so on.
FWIW, looks good to me. Just one small concern below...
Quoted 19 lines
drivers/ata/libata-core.c +4
-0@@ -2100,6 +2100,10 @@ retry:static inline u8 ata_dev_knobble (struct ata_device *dev) {struct ata_port *ap = dev->link->ap; + +if (ata_dev_blacklisted (dev) &ATA_HORKAGE_BRIDGE_OK ) +return 0 ; +return ((ap->cbl ==ATA_CBL_SATA ) && (!ata_id_is_sata (dev->id))); }@@ -3998,6 +4002,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = { { "TSSTcorp CDDVDW SH-S202N", "SB00", ATA_HORKAGE_IVB, }, { "TSSTcorp CDDVDW SH-S202N", "SB01", ATA_HORKAGE_IVB, },
Quoted 7 lines
drivers/ata/libata-core.c static const struct ata_blacklist_entry ata_device_blacklist [] = { · line 4006
Quoted 2 lines
drivers/ata/libata-core.c +2
-0+/* Devices that do not need bridging limits applied */ + { "Mtron", NULL, 0, },"Mtron" looks like a broad match but then again for some reason many non-traditional ATA vendors don't like to give descriptive identifications to their devices. Any chance there is something more to match? :-(
We could make it "MTRON MSP", I think that covers both the 70xx and 75xx range.
Signature · 1 line
Jens Axboe
Quoted 45 lines · Jens Axboe
Quoted 44 lines · Jens Axboe
Quoted 9 lines · Tejun Heo
Quoted 3 lines · Jens Axboe
Given that this problem should be going away and that it only really matters on very select devices (like this SSD), I think we should just add a quick white list for the bridge limits.
Yeah, it sucks that up & coming SSDs are still using PATA-SATA bridges.
The expectation when adding the wildcard limitation was that those P/S
bridges are not gonna be around for too long and the limit is most likely not be an actual problem. Oh well...I would hope that only the current generation do that, and to be honest I'm still pretty baffled that they exist :-)
Quoted 29 lines
Quoted 16 lines
Below is a quick'n dirty for that...
drivers/ata/libata-core.c +6
-0@@ -2097,9 +2097,70 @@ retry:return rc; } +struct ata_blacklist_entry { +const char *model_num; +const char *model_rev; +unsigned long horkage; +}; +drivers/ata/libata-core.c retry: · line 2115
Quoted 10 lines
drivers/ata/libata-core.c +10
-0+static const struct ata_blacklist_entry ata_bridge_whitelist[] = { +/* +* The following devices sit behind a bridge, but don't need +* transfer rate or size limits applied. +*/ + {"Mtron" , }, + +/* End Marker */ + { } +};Any reason this can't be part of the existing blacklist? It already supports wildcard matching and all.
Quoted 4 lines
Quoted 1 line
Sure, just with an inverse flag, I'll do that.
OK, something like this. Jeff, if you think this is fine, let me know and I'll submit a proper patch with description and so on.
This one has a better chance of actually working...
drivers/ata/libata-core.c +7 -0
@@ -2100,6 +2100,10 @@ retry:static inline u8 ata_dev_knobble (struct ata_device *dev) {struct ata_port *ap = dev->link->ap; + +if (ata_dev_blacklisted (dev) &ATA_HORKAGE_BRIDGE_OK ) +return 0 ; +return ((ap->cbl ==ATA_CBL_SATA ) && (!ata_id_is_sata (dev->id))); }
@@ -3998,6 +4002,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = { { "TSSTcorp CDDVDW SH-S202N", "SB00", ATA_HORKAGE_IVB, }, { "TSSTcorp CDDVDW SH-S202N", "SB01", ATA_HORKAGE_IVB, }, + /* Devices that do not need bridging limits applied */ + { "MTRON", NULL, ATA_HORKAGE_BRIDGE_OK, }, + /* End Marker */ { } };
include/linux/libata.h +1 -0
@@ -364,6 +364,7 @@ enum {ATA_HORKAGE_IPM = (1 <<7 ),/* Link PM problems */ ATA_HORKAGE_IVB = (1 <<8 ),/* cbl det validity bit bugs */ ATA_HORKAGE_STUCK_ERR = (1 <<9 ),/* stuck ERR on next PACKET */ + ATA_HORKAGE_BRIDGE_OK = (1 << 10), /* no bridge limits *//* DMA mask for user DMA control: User visible values; DO NOT renumber */
Signature · 1 line
Jens Axboe
Quoted 33 lines · Jens Axboe
OK, something like this. Jeff, if you think this is fine, let me know and I'll submit a proper patch with description and so on.
drivers/ata/libata-core.c +7
-0@@ -2100,6 +2100,10 @@ retry:static inline u8 ata_dev_knobble (struct ata_device *dev) {struct ata_port *ap = dev->link->ap; + +if (ata_dev_blacklisted (dev) &ATA_HORKAGE_BRIDGE_OK ) +return 0 ; +return ((ap->cbl ==ATA_CBL_SATA ) && (!ata_id_is_sata (dev->id))); }@@ -3998,6 +4002,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = { { "TSSTcorp CDDVDW SH-S202N", "SB00", ATA_HORKAGE_IVB, }, { "TSSTcorp CDDVDW SH-S202N", "SB01", ATA_HORKAGE_IVB, }, + /* Devices that do not need bridging limits applied */ + { "Mtron", NULL, 0, }, + /* End Marker */ { } };include/linux/libata.h +0
-0
include/linux/libata.h enum { · line 370
Quoted 7 lines
include/linux/libata.h +1
-0@@ -364,6 +364,7 @@ enum {ATA_HORKAGE_IPM = (1 <<7 ),/* Link PM problems */ ATA_HORKAGE_IVB = (1 <<8 ),/* cbl det validity bit bugs */ ATA_HORKAGE_STUCK_ERR = (1 <<9 ),/* stuck ERR on next PACKET */ + ATA_HORKAGE_BRIDGE_OK = (1 << 10), /* no bridge limits *//* DMA mask for user DMA control: User visible values; DO NOT renumber */
This one seems fine to me...
Jeff
Quoted 19 lines · Jeff Garzik
Quoted 18 lines · Jens Axboe
OK, something like this. Jeff, if you think this is fine, let me know and I'll submit a proper patch with description and so on.
drivers/ata/libata-core.c +4
-0@@ -2100,6 +2100,10 @@ retry:static inline u8 ata_dev_knobble (struct ata_device *dev) {struct ata_port *ap = dev->link->ap; + +if (ata_dev_blacklisted (dev) &ATA_HORKAGE_BRIDGE_OK ) +return 0 ; +return ((ap->cbl ==ATA_CBL_SATA ) && (!ata_id_is_sata (dev->id))); }
Quoted 7 lines
drivers/ata/libata-core.c static const struct ata_blacklist_entry · line 4007
Quoted 7 lines
drivers/ata/libata-core.c +2
-0@@ -3998,6 +4002,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = { { "TSSTcorp CDDVDW SH-S202N", "SB00", ATA_HORKAGE_IVB, }, { "TSSTcorp CDDVDW SH-S202N", "SB01", ATA_HORKAGE_IVB, }, + /* Devices that do not need bridging limits applied */ + { "Mtron", NULL, 0, },
Err.. shouldn't there be an ATA_HORKAGE_BRIDGE_OK on that line? Or did Jens already repost a corrected patch..
Quoted 21 lines
Quoted 16 lines
drivers/ata/libata-core.c +1
-0+/* End Marker */ { }};
include/linux/libata.h +0
-0@@ -364,6 +364,7 @@ enum {ATA_HORKAGE_IPM = (1 <<7 ),/* Link PM problems */ ATA_HORKAGE_IVB = (1 << 8), /* cbl det validity bitbugs */
ATA_HORKAGE_STUCK_ERR= (1 << 9), /* stuck ERR on next
PACKET */
+ATA_HORKAGE_BRIDGE_OK= (1 << 10), /* no bridge limits */Quoted 2 lines
/* DMA mask for user DMA control: User visible values; DO NOT renumber */This one seems fine to me...
..
Quoted 42 lines · Jeff Garzik
Quoted 41 lines · Jens Axboe
OK, something like this. Jeff, if you think this is fine, let me know and I'll submit a proper patch with description and so on.
drivers/ata/libata-core.c +7
-0@@ -2100,6 +2100,10 @@ retry:static inline u8 ata_dev_knobble (struct ata_device *dev) {struct ata_port *ap = dev->link->ap; + +if (ata_dev_blacklisted (dev) &ATA_HORKAGE_BRIDGE_OK ) +return 0 ; +return ((ap->cbl ==ATA_CBL_SATA ) && (!ata_id_is_sata (dev->id))); }@@ -3998,6 +4002,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = { { "TSSTcorp CDDVDW SH-S202N", "SB00", ATA_HORKAGE_IVB, }, { "TSSTcorp CDDVDW SH-S202N", "SB01", ATA_HORKAGE_IVB, }, + /* Devices that do not need bridging limits applied */ + { "Mtron", NULL, 0, }, + /* End Marker */ { }};
include/linux/libata.h +1
-0@@ -364,6 +364,7 @@ enum {ATA_HORKAGE_IPM = (1 <<7 ),/* Link PM problems */ ATA_HORKAGE_IVB = (1 <<8 ),/* cbl det validity bit bugs */ ATA_HORKAGE_STUCK_ERR = (1 <<9 ),/* stuck ERR on next PACKET */ + ATA_HORKAGE_BRIDGE_OK = (1 << 10), /* no bridge limits */
Quoted 5 lines
Quoted 2 lines
/* DMA mask for user DMA control: User visible values; DO NOT renumber */This one seems fine to me...
OK, so here's a version that I also tested. I applied the bridge ok flag to MTRON MSP-SATA, that is the closest match to the 70xx and 75xx series.
---
From: Jens Axboe <[email protected]>
Subject: [PATCH] libata: add whitelist for devices with known good pata-sata bridges
libata currently imposes a UDMA5 max transfer rate and 200 sector max transfer size for SATA devices that sit behind a pata-sata bridge. Lots of devices have known good bridges that don't need this limit applied. The MTRON SSD disks are such devices. Transfer rates are increased by 20-30% with the restriction removed.
So add a "blacklist" entry for the MTRON devices, with a flag indicating that the bridge is known good.
drivers/ata/libata-core.c +7 -0
@@ -2100,6 +2100,10 @@ retry:static inline u8 ata_dev_knobble (struct ata_device *dev) {struct ata_port *ap = dev->link->ap; + +if (ata_dev_blacklisted (dev) &ATA_HORKAGE_BRIDGE_OK ) +return 0 ; +return ((ap->cbl ==ATA_CBL_SATA ) && (!ata_id_is_sata (dev->id))); }
@@ -3998,6 +4002,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = { { "TSSTcorp CDDVDW SH-S202N", "SB00", ATA_HORKAGE_IVB, }, { "TSSTcorp CDDVDW SH-S202N", "SB01", ATA_HORKAGE_IVB, }, + /* Devices that do not need bridging limits applied */ + { "MTRON MSP-SATA*", NULL, ATA_HORKAGE_BRIDGE_OK, }, + /* End Marker */ { } };
include/linux/libata.h +1 -0
@@ -364,6 +364,7 @@ enum {ATA_HORKAGE_IPM = (1 <<7 ),/* Link PM problems */ ATA_HORKAGE_IVB = (1 <<8 ),/* cbl det validity bit bugs */ ATA_HORKAGE_STUCK_ERR = (1 <<9 ),/* stuck ERR on next PACKET */ + ATA_HORKAGE_BRIDGE_OK = (1 << 10), /* no bridge limits *//* DMA mask for user DMA control: User visible values; DO NOT renumber */
Signature · 1 line
Jens Axboe
Jeff Garzik
AppliedQuoted 57 lines · Jens Axboe
Quoted 42 lines · Jeff Garzik
Quoted 41 lines · Jens Axboe
OK, something like this. Jeff, if you think this is fine, let me know and I'll submit a proper patch with description and so on.
drivers/ata/libata-core.c +7
-0@@ -2100,6 +2100,10 @@ retry:static inline u8 ata_dev_knobble (struct ata_device *dev) {struct ata_port *ap = dev->link->ap; + +if (ata_dev_blacklisted (dev) &ATA_HORKAGE_BRIDGE_OK ) +return 0 ; +return ((ap->cbl ==ATA_CBL_SATA ) && (!ata_id_is_sata (dev->id))); }@@ -3998,6 +4002,9 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = { { "TSSTcorp CDDVDW SH-S202N", "SB00", ATA_HORKAGE_IVB, }, { "TSSTcorp CDDVDW SH-S202N", "SB01", ATA_HORKAGE_IVB, }, + /* Devices that do not need bridging limits applied */ + { "Mtron", NULL, 0, }, + /* End Marker */ { }};
include/linux/libata.h +1
-0@@ -364,6 +364,7 @@ enum {ATA_HORKAGE_IPM = (1 <<7 ),/* Link PM problems */ ATA_HORKAGE_IVB = (1 <<8 ),/* cbl det validity bit bugs */ ATA_HORKAGE_STUCK_ERR = (1 <<9 ),/* stuck ERR on next PACKET */ + ATA_HORKAGE_BRIDGE_OK = (1 << 10), /* no bridge limits */Quoted 4 lines
Quoted 2 lines
/* DMA mask for user DMA control: User visible values; DO NOT renumber */This one seems fine to me...
OK, so here's a version that I also tested. I applied the bridge ok flag to MTRON MSP-SATA, that is the closest match to the 70xx and 75xx series.
---
From: Jens Axboe <[email protected]>
Subject: [PATCH] libata: add whitelist for devices with known good pata-sata bridges
Quoted 8 lines
libata currently imposes a UDMA5 max transfer rate and 200 sector max transfer size for SATA devices that sit behind a pata-sata bridge. Lots of devices have known good bridges that don't need this limit applied. The MTRON SSD disks are such devices. Transfer rates are increased by 20-30% with the restriction removed.
So add a "blacklist" entry for the MTRON devices, with a flag indicating that the bridge is known good.
applied