[PATCH] libata-core Use more robust parsing for multi_count

[PATCH] libata-core Use more robust parsing for multi_count

[Mark Lord]

Gents,

I am debugging a drive hang issue for a client,
which involves "multiple sector" read/write commands (PIO).

Libata does not appear (as of current #upstream at least)
to correctly implement this functionality.

Specifically, libata doesn't reset dev->multi_count to zero
on drive resets (whereas the drive firmware often *does*),
and libata does not deal well with dubious IDENTIFY data.

So, on resets, we should either clear dev->multi_count,
or re-probe it from the drive, or issue a SET_MULTIPLE command
to restore the old value.

When probing:

If (dev->id[59] == 0xffff), ignore it.
If (dev->id[59] & 0x00ff) is not an even power of two, ignore it.
it should also be ignored (this catches the 0xffff case too).

The powers of two is from the ATA8 standard, which strongly suggests
that only powers of two are valid for use with SET_MULTIPLE.

The probe stuff is trivial to fix (below), but I'm not really
up to speed on figuring out how to get this code re-invoked
after drive resets, and on how to ensure that a SET_MULTIPLE
gets issued to restore the previous working value (with eventual
fallback to not using it after repeated errors).   Tejun?

<--- snip --->

Make libata a little more robust in parsing the multi_count
field from a drive's identify data.  This prevents libata from
attempting to use dubious multi_count values ad infinitum.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- upstream/drivers/ata/libata-core.c	2009-03-18 10:21:07.000000000 -0400
+++ new/drivers/ata/libata-core.c	2009-03-18 10:22:05.000000000 -0400
@@ -2426,9 +2426,20 @@
 
 		dev->n_sectors = ata_id_n_sectors(id);
 
-		if (dev->id[59] & 0x100)
-			dev->multi_count = dev->id[59] & 0xff;
-
+		dev->multi_count = 0;
+		if (dev->id[59] & 0x100) {
+			switch (dev->id[59] & 0xff) {
+			case 1:
+			case 2:
+			case 4:
+			case 8:
+			case 16:
+			case 32:
+			case 64:
+			case 128:
+				dev->multi_count = dev->id[59] & 0xff;
+			}
+		}
 		if (ata_id_has_lba(id)) {
 			const char *lba_desc;
 			char ncq_desc[20];

Re: [PATCH] libata-core Use more robust parsing for multi_count

[Alan Cox]

> +			switch (dev->id[59] & 0xff) {

Umm surely...

	if (n & (n - 1))

otherwise looks good.

Alan

Re: [PATCH] libata-core Use more robust parsing for multi_count

[Mark Lord]

Alan Cox wrote:
>> +			switch (dev->id[59] & 0xff) {
> 
> Umm surely...
> 
> 	if (n & (n - 1))
..

????

Eg. for n = 5:  (5 & 4) == "true", but not a power of two.

????

Re: [PATCH] libata-core Use more robust parsing for multi_count

[Mark Lord]

Mark Lord wrote:
> Alan Cox wrote:
>>> +            switch (dev->id[59] & 0xff) {
>>
>> Umm surely...
>>
>>     if (n & (n - 1))
> ..
> 
> ????
..

Ah.. backwards.  Thanks, Alan!  Revised patch follows:

-----

Make libata a little more robust in parsing the multi_count
field from a drive's identify data.  This prevents libata from
attempting to use dubious multi_count values ad infinitum.

Signed-off-by: Mark Lord <mlord@pobox.com> 

--- upstream/drivers/ata/libata-core.c	2009-03-18 10:21:07.000000000 -0400
+++ new/drivers/ata/libata-core.c	2009-03-18 10:22:05.000000000 -0400
@@ -2426,9 +2426,12 @@
 
 		dev->n_sectors = ata_id_n_sectors(id);
 
-		if (dev->id[59] & 0x100)
-			dev->multi_count = dev->id[59] & 0xff;
-
+		dev->multi_count = 0;
+		if (dev->id[59] & 0x100) {
+			int mc = dev->id[59] & 0xff;
+			if ((mc & (mc - 1)) == 0) /* even power of two? */
+				dev->multi_count = mc;
+		}
 		if (ata_id_has_lba(id)) {
 			const char *lba_desc;
 			char ncq_desc[20];

Re: [PATCH] libata-core Use more robust parsing for multi_count

[Mark Lord]

Mark Lord wrote:
> Gents,
> 
> I am debugging a drive hang issue for a client,
> which involves "multiple sector" read/write commands (PIO).
> 
> Libata does not appear (as of current #upstream at least)
> to correctly implement this functionality.
> 
> Specifically, libata doesn't reset dev->multi_count to zero
> on drive resets (whereas the drive firmware often *does*),
> and libata does not deal well with dubious IDENTIFY data.
> 
> So, on resets, we should either clear dev->multi_count,
> or re-probe it from the drive, or issue a SET_MULTIPLE command
> to restore the old value.
> 
> When probing:
> 
> If (dev->id[59] == 0xffff), ignore it.
> If (dev->id[59] & 0x00ff) is not an even power of two, ignore it.
> it should also be ignored (this catches the 0xffff case too).
> 
> The powers of two is from the ATA8 standard, which strongly suggests
> that only powers of two are valid for use with SET_MULTIPLE.
> 
> The probe stuff is trivial to fix (below), but I'm not really
> up to speed on figuring out how to get this code re-invoked
> after drive resets, and on how to ensure that a SET_MULTIPLE
> gets issued to restore the previous working value (with eventual
> fallback to not using it after repeated errors).   Tejun?
..

Mmm.. we also have to re-check and/or re-set dev->multi_count
after any ata_acpi_on_devcfg() call, because some machines use
ACPI taskfiles to issue a "set multi_count" command to the drive.

This appears to be taken care of already though,
as as ata_dev_configure() is where we now invoke ata_acpi_on_devcfg(),
and this all gets called from ata_dev_revalidate().

So with the patch I provided, newer kernels should then be fine.
But I suppose the patch itself could also be even more finicky
about things, and not bother with R/W multi commands if multi_count==1,
and also validate multi_count against the drive's reported maximum 
from ID word 47.

Alan:  do you reckon we'll be fine enough by just checking
that word on its own, or would its interpretation depend upon
the supported ATA revisions of the device?  If so, then so would
the rest of the mult_count support in libata, right?

I'll re-spin a v3 of the patch to check against word 47.

Cheers

[PATCH] libata-core More robust parsing for multi_count(v3)

[Mark Lord]

Make libata more robust when parsing the multi_count
fields from a drive's identify data.  This prevents us from
attempting to use dubious multi_count values ad infinitum.

Reset dev->multi_count to zero and reprobe it each time
through this routine, as it can change on device reset.

Also ensure that the reported "maximum" value is valid
and is a power of two, and that the reported "count" value
is valid and also a power of two.  And that the "count"
value is not greater than the "maximum" value.

Signed-off-by: Mark Lord <mlord@pobox.com>

--- upstream/drivers/ata/libata-core.c.orig	2009-03-18 11:08:27.000000000 -0400
+++ new/drivers/ata/libata-core.c	2009-03-18 12:09:31.000000000 -0400
@@ -2389,6 +2389,7 @@
 	dev->cylinders = 0;
 	dev->heads = 0;
 	dev->sectors = 0;
+	dev->multi_count = 0;
 
 	/*
 	 * common ATA, ATAPI feature tests
@@ -2426,8 +2427,16 @@
 
 		dev->n_sectors = ata_id_n_sectors(id);
 
-		if (dev->id[59] & 0x100)
-			dev->multi_count = dev->id[59] & 0xff;
+		/* get/validate current R/W Multiple count setting */
+		if ((dev->id[47] >> 8) == 0x80 && (dev->id[59] & 0x100)) {
+			unsigned int max = dev->id[47] & 0xff;
+			unsigned int cnt = dev->id[59] & 0xff;
+			/* only recognize/allow powers of two here */
+			if (cnt && cnt <= max && (max & (max - 1)) == 0) {
+				if ((cnt & (cnt - 1)) == 0)
+					dev->multi_count = cnt;
+			}
+		}
 
 		if (ata_id_has_lba(id)) {
 			const char *lba_desc;

Re: [PATCH] libata-core More robust parsing for multi_count(v3)

[Mark Lord]

Mark Lord wrote:
> Make libata more robust when parsing the multi_count
> fields from a drive's identify data.  This prevents us from
> attempting to use dubious multi_count values ad infinitum.
> 
> Reset dev->multi_count to zero and reprobe it each time
> through this routine, as it can change on device reset.
> 
> Also ensure that the reported "maximum" value is valid
> and is a power of two, and that the reported "count" value
> is valid and also a power of two.  And that the "count"
> value is not greater than the "maximum" value.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> --- upstream/drivers/ata/libata-core.c.orig    2009-03-18 
> 11:08:27.000000000 -0400
> +++ new/drivers/ata/libata-core.c    2009-03-18 12:09:31.000000000 -0400
> @@ -2389,6 +2389,7 @@
>     dev->cylinders = 0;
>     dev->heads = 0;
>     dev->sectors = 0;
> +    dev->multi_count = 0;
> 
>     /*
>      * common ATA, ATAPI feature tests
> @@ -2426,8 +2427,16 @@
> 
>         dev->n_sectors = ata_id_n_sectors(id);
> 
> -        if (dev->id[59] & 0x100)
> -            dev->multi_count = dev->id[59] & 0xff;
> +        /* get/validate current R/W Multiple count setting */
> +        if ((dev->id[47] >> 8) == 0x80 && (dev->id[59] & 0x100)) {
> +            unsigned int max = dev->id[47] & 0xff;
> +            unsigned int cnt = dev->id[59] & 0xff;
> +            /* only recognize/allow powers of two here */
> +            if (cnt && cnt <= max && (max & (max - 1)) == 0) {
> +                if ((cnt & (cnt - 1)) == 0)
> +                    dev->multi_count = cnt;
> +            }
> +        }
> 
>         if (ata_id_has_lba(id)) {
>             const char *lba_desc;
..

Note that this patch still allows (dev->multi_count == 1),
as before, though this may or may not be a good idea.

Right now, we should probably just treat "1" the same as "0",
and not use R/W multi commands with that setting.

But at some point, libata will gain HDIO_SET_MULTCOUNT support
and/or a sysfs attr for it.  At which point there's no reason
to force policy on a multi_count of 1.

Opinions?

We can change that behaviour with a second, follow-up, patch if need be.

Cheers

Re: [PATCH] libata-core Use more robust parsing for multi_count

[Alan Cox]

> > 	if (n & (n - 1))
> ..
> 
> ????
> 
> Eg. for n = 5:  (5 & 4) == "true", but not a power of two.

if (!(...)) rather

Alan

Re: [PATCH] libata-core More robust parsing for multi_count(v3)

[Tejun Heo]

Hello, Mark.

Mark Lord wrote:
> +            if (cnt && cnt <= max && (max & (max - 1)) == 0) {

Can you please include <linux/log2.h> and use is_power_of_2() instead?

Thanks.

-- 
tejun

Re: [PATCH] libata-core More robust parsing for multi_count(v3)

[Tejun Heo]

Tejun Heo wrote:
> Hello, Mark.
> 
> Mark Lord wrote:
>> +            if (cnt && cnt <= max && (max & (max - 1)) == 0) {
> 
> Can you please include <linux/log2.h> and use is_power_of_2() instead?

Please note that is_power_of_2() also checks for zero so the whole
condition can be changed to

	if (is_power_of_2(cnt) && cnt <= max)

Thanks.

-- 
tejun

[PATCH] libata-core More robust parsing for multi_count(v4)

[Mark Lord]

Make libata more robust when parsing the multi_count
field from a drive's identify data.  This prevents us from
attempting to use dubious multi_count values ad infinitum.

Reset dev->multi_count to zero and reprobe it each time
through this routine, as it can change on device reset.

Also ensure that the reported "maximum" value is valid
and is a power of two, and that the reported "count" value
is valid and also a power of two.  And that the "count"
value is not greater than the "maximum" value.

Signed-off-by: Mark Lord <mlord@pobox.com>
---

Updated to use is_power_of_2() as suggested by Tejun.

Make libata more robust when parsing the multi_count
field from a drive's identify data.  This prevents us from
attempting to use dubious multi_count values ad infinitum.

Reset dev->multi_count to zero and reprobe it each time
through this routine, as it can change on device reset.

Also ensure that the reported "maximum" value is valid
and is a power of two, and that the reported "count" value
is valid and also a power of two.  And that the "count"
value is not greater than the "maximum" value.

Signed-off-by: Mark Lord <mlord@pobox.com>
---

Updated to use is_power_of_2() as suggested by Tejun.

--- upstream/drivers/ata/libata-core.c	2009-03-18 11:08:27.000000000 -0400
+++ new/drivers/ata/libata-core.c	2009-03-19 13:21:46.000000000 -0400
@@ -2389,6 +2389,7 @@
 	dev->cylinders = 0;
 	dev->heads = 0;
 	dev->sectors = 0;
+	dev->multi_count = 0;
 
 	/*
 	 * common ATA, ATAPI feature tests
@@ -2426,8 +2427,15 @@
 
 		dev->n_sectors = ata_id_n_sectors(id);
 
-		if (dev->id[59] & 0x100)
-			dev->multi_count = dev->id[59] & 0xff;
+		/* get current R/W Multiple count setting */
+		if ((dev->id[47] >> 8) == 0x80 && (dev->id[59] & 0x100)) {
+			unsigned int max = dev->id[47] & 0xff;
+			unsigned int cnt = dev->id[59] & 0xff;
+			/* only recognize/allow powers of two here */
+			if (is_power_of_2(max) && is_power_of_2(cnt))
+				if (cnt <= max)
+					dev->multi_count = cnt;
+		}
 
 		if (ata_id_has_lba(id)) {
 			const char *lba_desc;
--- libata-dev/drivers/ata/libata-core.c.orig	2009-03-18 11:08:27.000000000 -0400
+++ libata-dev/drivers/ata/libata-core.c	2009-03-19 13:26:37.000000000 -0400
@@ -57,6 +57,7 @@
 #include <linux/scatterlist.h>
 #include <linux/io.h>
 #include <linux/async.h>
+#include <linux/log2.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_host.h>
@@ -2389,6 +2390,7 @@
 	dev->cylinders = 0;
 	dev->heads = 0;
 	dev->sectors = 0;
+	dev->multi_count = 0;
 
 	/*
 	 * common ATA, ATAPI feature tests
@@ -2426,8 +2428,15 @@
 
 		dev->n_sectors = ata_id_n_sectors(id);
 
-		if (dev->id[59] & 0x100)
-			dev->multi_count = dev->id[59] & 0xff;
+		/* get current R/W Multiple count setting */
+		if ((dev->id[47] >> 8) == 0x80 && (dev->id[59] & 0x100)) {
+			unsigned int max = dev->id[47] & 0xff;
+			unsigned int cnt = dev->id[59] & 0xff;
+			/* only recognize/allow powers of two here */
+			if (is_power_of_2(max) && is_power_of_2(cnt))
+				if (cnt <= max)
+					dev->multi_count = cnt;
+		}
 
 		if (ata_id_has_lba(id)) {
 			const char *lba_desc;

[PATCH] libata-core More robust parsing for multi_count(v5)

[Mark Lord]

Make libata more robust when parsing the multi_count
field from a drive's identify data.  This prevents us from
attempting to use dubious multi_count values ad infinitum.

Reset dev->multi_count to zero and reprobe it each time
through this routine, as it can change on device reset.

Also ensure that the reported "maximum" value is valid
and is a power of two, and that the reported "count" value
is valid and also a power of two.  And that the "count"
value is not greater than the "maximum" value.

Signed-off-by: Mark Lord <mlord@pobox.com>
---

Fixed duplicate patch found in v4.
Updated to use is_power_of_2() as suggested by Tejun.

--- old/drivers/ata/libata-core.c	2009-03-18 11:08:27.000000000 -0400
+++ new/drivers/ata/libata-core.c	2009-03-19 13:26:37.000000000 -0400
@@ -57,6 +57,7 @@
 #include <linux/scatterlist.h>
 #include <linux/io.h>
 #include <linux/async.h>
+#include <linux/log2.h>
 #include <scsi/scsi.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_host.h>
@@ -2389,6 +2390,7 @@
 	dev->cylinders = 0;
 	dev->heads = 0;
 	dev->sectors = 0;
+	dev->multi_count = 0;
 
 	/*
 	 * common ATA, ATAPI feature tests
@@ -2426,8 +2428,15 @@
 
 		dev->n_sectors = ata_id_n_sectors(id);
 
-		if (dev->id[59] & 0x100)
-			dev->multi_count = dev->id[59] & 0xff;
+		/* get current R/W Multiple count setting */
+		if ((dev->id[47] >> 8) == 0x80 && (dev->id[59] & 0x100)) {
+			unsigned int max = dev->id[47] & 0xff;
+			unsigned int cnt = dev->id[59] & 0xff;
+			/* only recognize/allow powers of two here */
+			if (is_power_of_2(max) && is_power_of_2(cnt))
+				if (cnt <= max)
+					dev->multi_count = cnt;
+		}
 
 		if (ata_id_has_lba(id)) {
 			const char *lba_desc;

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Tejun Heo]

Hello, Mark.

Mark Lord wrote:
> Make libata more robust when parsing the multi_count
> field from a drive's identify data.  This prevents us from
> attempting to use dubious multi_count values ad infinitum.
> 
> Reset dev->multi_count to zero and reprobe it each time
> through this routine, as it can change on device reset.
> 
> Also ensure that the reported "maximum" value is valid
> and is a power of two, and that the reported "count" value
> is valid and also a power of two.  And that the "count"
> value is not greater than the "maximum" value.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>

Acked-by: Tejun Heo <tj@kernel.org>

The patch looks safe to me although requiring power_of_2 on max seems
a bit too strict.  For configuration, ata_dev_configure() is the right
place.  I think the following should work.

* add dev->multi_limit which is initialized to -1 on ata_dev_init().

* during ata_dev_configure() if multi_limit < 0, set it to device max.

* set dev->multi to rounddown_pow_of_two(multi_limit) and issue
  SET_MULTI.

On EH side, ata_eh_speed_down() is the place to handle it.  It can
probably piggy back on ATA_EH_SPDN_SPEED_DOWN block.  All it has to do
is reducing dev->multi_limit.  dev_config will run later and apply it.
I'm not sure how the speed down strategy should exactly be tho.  It is
distinguisible from regular transfer errors?

Thanks.

-- 
tejun

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Mark Lord]

Tejun Heo wrote:
> Hello, Mark.
> 
> Mark Lord wrote:
>> Make libata more robust when parsing the multi_count
>> field from a drive's identify data.  This prevents us from
>> attempting to use dubious multi_count values ad infinitum.
>>
>> Reset dev->multi_count to zero and reprobe it each time
>> through this routine, as it can change on device reset.
>>
>> Also ensure that the reported "maximum" value is valid
>> and is a power of two, and that the reported "count" value
>> is valid and also a power of two.  And that the "count"
>> value is not greater than the "maximum" value.
>>
>> Signed-off-by: Mark Lord <mlord@pobox.com>
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
> The patch looks safe to me although requiring power_of_2 on max seems
> a bit too strict.
..

Just a safeguard against bad IDENTIFY data.
I've never seen a drive that specified anything there
other than 1, 8, or 16,  and the ATA spec specifically
recommends against using settings that are not powers of 2.

> For configuration, ata_dev_configure() is the right
> place.  I think the following should work.
> 
> * add dev->multi_limit which is initialized to -1 on ata_dev_init().
> 
> * during ata_dev_configure() if multi_limit < 0, set it to device max.
> 
> * set dev->multi to rounddown_pow_of_two(multi_limit) and issue
>   SET_MULTI.
> 
> On EH side, ata_eh_speed_down() is the place to handle it.  It can
> probably piggy back on ATA_EH_SPDN_SPEED_DOWN block.  All it has to do
> is reducing dev->multi_limit.  dev_config will run later and apply it.
..

I won't have time to pursue this in the near/foreseeable future,
so if you feel the urge.. please do it.  Otherwise I might get to
it in a few months from now.

> I'm not sure how the speed down strategy should exactly be tho.  It is
> distinguisible from regular transfer errors?
..

I don't really have a recommendation there, other than to simply stop
using multi_count if there are repeated PIO errors.  Seems like a reasonable
strategy, when all else is failing.

We really need to add SET_MULTIPLE smarts to libata at some point.

Cheers

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Mark Lord]

Tejun,

I need a way to limit the multi_count to a specific maximum
value for sata_mv.  ISTR that some PATA chips may also have
an upper hardware limit on multi_count.

Generally, "8" is safe for all chipsets.  Going above that
requires knowledge of what the chipset can tolerate.

How about a u8 multi_count_max field in the ata_host struct?
Use 0xff for "multi_count not allowed", and anything else
in there as an upper limit for the chipset.

???

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Mark Lord]

Mark Lord wrote:
> Tejun,
> 
> I need a way to limit the multi_count to a specific maximum
> value for sata_mv.  ISTR that some PATA chips may also have
> an upper hardware limit on multi_count.
> 
> Generally, "8" is safe for all chipsets.  Going above that
> requires knowledge of what the chipset can tolerate.
> 
> How about a u8 multi_count_max field in the ata_host struct?
> Use 0xff for "multi_count not allowed", and anything else
> in there as an upper limit for the chipset.
..

Oh, and zero means "don't care".  So it's really easy to add
to libata without having to patch each and every driver.

??

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Alan Cox]

On Fri, 20 Mar 2009 09:13:52 -0400
Mark Lord <liml@rtr.ca> wrote:

> Tejun,
> 
> I need a way to limit the multi_count to a specific maximum
> value for sata_mv.  ISTR that some PATA chips may also have
> an upper hardware limit on multi_count.

Original ST412 depending on the amount of external SRAM, but not I think
anything much newer than that. Would be worth going over the old
drivers/ide code to double check though.

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Alan Cox]

> Oh, and zero means "don't care".  So it's really easy to add
> to libata without having to patch each and every driver.

If its only one driver why not do it the way we do things like transfer
length limits for LBA48 (see pata_it821x and others)

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Mark Lord]

Alan Cox wrote:
>> Oh, and zero means "don't care".  So it's really easy to add
>> to libata without having to patch each and every driver.
> 
> If its only one driver why not do it the way we do things like transfer
> length limits for LBA48 (see pata_it821x and others)
..

Thanks.  I'll do that, and probably go that way for the sata_mv limit.

The bigger issue here is that libata really doesn't support multiple mode.

Sure, it will blindly use it if the drive appears to be already configured
for it at startup.  But I don't see any where that we issue "SET MULTIPLE"
commands or otherwise support turning it on/off or tuning it.

I wonder if we even snoop the SET MULTIPLE command from SG_IO / ATA_{12,16}?

This is more of an infrastructure thing than a driver-specific setting,
so I'm hoping that one of you three (who know the code a lot better
than I do) might think about that for the future.

Thanks again.

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Tejun Heo]

Hello, Mark.

Mark Lord wrote:
> Alan Cox wrote:
>>> Oh, and zero means "don't care".  So it's really easy to add
>>> to libata without having to patch each and every driver.
>>
>> If its only one driver why not do it the way we do things like transfer
>> length limits for LBA48 (see pata_it821x and others)
> ..
> 
> Thanks.  I'll do that, and probably go that way for the sata_mv limit.

Yeap, for the time being, that seems to be the simplest thing to do.

> The bigger issue here is that libata really doesn't support multiple mode.
> 
> Sure, it will blindly use it if the drive appears to be already configured
> for it at startup.  But I don't see any where that we issue "SET MULTIPLE"
> commands or otherwise support turning it on/off or tuning it.
> 
> I wonder if we even snoop the SET MULTIPLE command from SG_IO /
> ATA_{12,16}?
> 
> This is more of an infrastructure thing than a driver-specific setting,
> so I'm hoping that one of you three (who know the code a lot better
> than I do) might think about that for the future.

Right, libata doesn't have any multiple sector related configuration
whatsoever.  Maybe I'll give it a shot.

Thanks.

-- 
tejun

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Jeff Garzik]

Tejun Heo wrote:
> Mark Lord wrote:
>> Sure, it will blindly use it if the drive appears to be already configured
>> for it at startup.  But I don't see any where that we issue "SET MULTIPLE"
>> commands or otherwise support turning it on/off or tuning it.
>>
>> I wonder if we even snoop the SET MULTIPLE command from SG_IO /
>> ATA_{12,16}?
>>
>> This is more of an infrastructure thing than a driver-specific setting,
>> so I'm hoping that one of you three (who know the code a lot better
>> than I do) might think about that for the future.
> 
> Right, libata doesn't have any multiple sector related configuration
> whatsoever.  Maybe I'll give it a shot.


Yeah, that has long been a design decision -- avoid all the mechanics of 
PIO-Multi, simply because we did not have much of a need for it (and 
still don't).

Albert Lee worked on PIO-Multi a while ago, which was mainly to get 
ATA_{12,16} working, IIRC.

But overall, I don't see much of a need to introduce it into the main stack?

	Jeff

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Tejun Heo]

Jeff Garzik wrote:
> Yeah, that has long been a design decision -- avoid all the mechanics of
> PIO-Multi, simply because we did not have much of a need for it (and
> still don't).
> 
> Albert Lee worked on PIO-Multi a while ago, which was mainly to get
> ATA_{12,16} working, IIRC.
> 
> But overall, I don't see much of a need to introduce it into the main
> stack?

Well, there still are awful lot of PIO only devices out in the wild,
so improving PIO support seems like a good idea?

-- 
tejun

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Mark Lord]

Tejun Heo wrote:
> Jeff Garzik wrote:
>> Yeah, that has long been a design decision -- avoid all the mechanics of
>> PIO-Multi, simply because we did not have much of a need for it (and
>> still don't).
>>
>> Albert Lee worked on PIO-Multi a while ago, which was mainly to get
>> ATA_{12,16} working, IIRC.
>>
>> But overall, I don't see much of a need to introduce it into the main
>> stack?
> 
> Well, there still are awful lot of PIO only devices out in the wild,
> so improving PIO support seems like a good idea?
..


Tons of CompactFlash cards are used PIO-only with Linux,
and systems could benefit from the reduced interrupt overhead
that multiple gives.  Especially since those same systems tend
to have rather modest embedded CPUs.

We *already* do multiple-sector I/O in libata.
But it currently works partially by accident,
and probably gets shut off much of the time
whenever the EH code is invoked and resets anything.

Cheers

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Alan Cox]

> Yeah, that has long been a design decision -- avoid all the mechanics of 
> PIO-Multi, simply because we did not have much of a need for it (and 
> still don't).
> 
> Albert Lee worked on PIO-Multi a while ago, which was mainly to get 
> ATA_{12,16} working, IIRC.
> 
> But overall, I don't see much of a need to introduce it into the main stack?

I don't see a need - and if we do its more important IMHO to get the PIO
paths out of the irq handlers before doing so.

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Mark Lord]

Alan Cox wrote:
>> Yeah, that has long been a design decision -- avoid all the mechanics of 
>> PIO-Multi, simply because we did not have much of a need for it (and 
>> still don't).
>>
>> Albert Lee worked on PIO-Multi a while ago, which was mainly to get 
>> ATA_{12,16} working, IIRC.
>>
>> But overall, I don't see much of a need to introduce it into the main stack?
> 
> I don't see a need - and if we do its more important IMHO to get the PIO
> paths out of the irq handlers before doing so.
..

That's not a new concern, given that we *already* perform multi-sector I/O.

And it's not really related to improving the implementation to be better
aware of multi-sector I/O and interaction with EH.

But yeah, a good thing nonetheless.

Cheers

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Jeff Garzik]

Mark Lord wrote:
> Make libata more robust when parsing the multi_count
> field from a drive's identify data.  This prevents us from
> attempting to use dubious multi_count values ad infinitum.
> 
> Reset dev->multi_count to zero and reprobe it each time
> through this routine, as it can change on device reset.
> 
> Also ensure that the reported "maximum" value is valid
> and is a power of two, and that the reported "count" value
> is valid and also a power of two.  And that the "count"
> value is not greater than the "maximum" value.
> 
> Signed-off-by: Mark Lord <mlord@pobox.com>
> ---
> 
> Fixed duplicate patch found in v4.
> Updated to use is_power_of_2() as suggested by Tejun.

applied

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Mark Lord]

Mark Lord wrote:
> Tejun,
> 
> I need a way to limit the multi_count to a specific maximum
> value for sata_mv.  ISTR that some PATA chips may also have
> an upper hardware limit on multi_count.
> 
> Generally, "8" is safe for all chipsets.  Going above that
> requires knowledge of what the chipset can tolerate.
> 
> How about a u8 multi_count_max field in the ata_host struct?
> Use 0xff for "multi_count not allowed", and anything else
> in there as an upper limit for the chipset.
..
Alan Cox wrote:
>
> If its only one driver why not do it the way we do things like transfer
> length limits for LBA48 (see pata_it821x and others)
..

Okay, I've looked at pata_it821x.c now, and I just don't see it.
That particular driver seems to limit *all* transfers to 256 max,
not just those of a particular protocol, like mult-sect I/O.

Am I missing something there?

The limit for sata_mv chipsets seems to actually be 7-sectors or less
for read/write multiple.  Which means a max of 4 in practice.

Similarly, we should also be preventing *any* PIO of more than one DRQ
for sata_mv.  But I don't see a sensible way to do that either.

In practice, that part does seem to work fine with the PIO polling
that sata_mv uses.

But what to do about the read/write multiple issue ?

???

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Alan Cox]

> The limit for sata_mv chipsets seems to actually be 7-sectors or less
> for read/write multiple.  Which means a max of 4 in practice.
> 
> Similarly, we should also be preventing *any* PIO of more than one DRQ
> for sata_mv.  But I don't see a sensible way to do that either.

Ugghhh..

> In practice, that part does seem to work fine with the PIO polling
> that sata_mv uses.
> 
> But what to do about the read/write multiple issue ?

I think the field has to go in but it can default to 255 (unlimited) and
the it821x like hooks can be used for those controllers with limits.

Alan

Re: [PATCH] libata-core More robust parsing for multi_count(v5)

[Jeff Garzik]

Alan Cox wrote:
>> The limit for sata_mv chipsets seems to actually be 7-sectors or less
>> for read/write multiple.  Which means a max of 4 in practice.
>>
>> Similarly, we should also be preventing *any* PIO of more than one DRQ
>> for sata_mv.  But I don't see a sensible way to do that either.
> 
> Ugghhh..

Our infrastructure just flat doesn't deal well with per-xfer-mode sector 
count limitations.  It is quite easy for us to limit per-xfer sector 
count on a __per device__ basis -- both SCSI and block layer handle that 
well.

But deviating from that implies potentially having to re-merge (or 
split) block layer requests -- IOW quickly moves into the non-trivial, 
cross-stack category of problem.

Problem is, of course, sector count limits in ATA can be per-command (or 
per-xfer-mode), not just per-device.

	Jeff