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

[Mark Lord <liml@rtr.ca>]

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