libata-core Use more robust parsing for multi_count

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.

@@ -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];

..

????

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

????

..

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.

@@ -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];

if (!(...)) rather

Alan

..

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

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.

@@ -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;

Hello, Mark.

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

Thanks.

tejun

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

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.

---

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.

---

Updated to use is_power_of_2() as suggested by Tejun.

@@ -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;

@@ -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;

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.

---

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

@@ -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;

Hello, Mark.

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

..

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.

..

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 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

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.

??

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.

Hello, Mark.

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

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

Thanks.

tejun

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

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

tejun

..

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

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

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.

..

..

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 ?

???

Ugghhh..

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

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

applied