Re: 2.6.29-rc libata sff 32bit PIO regression

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Hugh Dickins wrote:
> On Mon, 26 Jan 2009, Alan Cox wrote:
>   
>>> [PATCH] libata sff: 32bit PIO use 16bit on slop
>>>
>>> 871af1210f13966ab911ed2166e4ab2ce775b99d libata: Add 32bit PIO support
>>> causes errors on a four-year-old ata_piix Dell Precision 670.  Using
>>> 16bit PIO instead of 32bit PIO on the odd 1, 2 or 3 chars fixes that.
>>>
>>> Signed-off-by: Hugh Dickins <hugh@veritas.com>
>>>       
>> For the 3 bytes of slop it should use a single iowrite32 but otherwise
>> that seems ok. We do need to handle the FIFO setup on the AMD differently
>> if we do this ...
>>     
>
> Sorry, I believe you were waiting on me for this, to accompany your
> AMD and VLB patches.  I'm afraid I don't have any such AMD devices
> to test this along with yours, and the only non-0 slop that I've seen
> in testing has been 2 (about 25% of ops, so I removed the "unlikely").
> But this patch works as well for me as the patch I posted before
> (though much more verbose: please simplify if you see a better way).
>
>
> [PATCH] libata sff: 32bit PIO use 16bit on slop
>
> 871af1210f13966ab911ed2166e4ab2ce775b99d libata: Add 32bit PIO support
> causes errors on a four-year-old ata_piix Dell Precision 670.  Using
> 16bit PIO instead of 32bit PIO on the odd 1 or 2 chars fixes that,
> but Alan Cox indicates that we should still use 32bit for 3 chars.
>
> Signed-off-by: Hugh Dickins <hugh@veritas.com>
> ---
>
>  drivers/ata/libata-sff.c |   33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> --- 2.6.29-rc3/drivers/ata/libata-sff.c	2009-01-29 12:33:28.000000000 +0000
> +++ linux/drivers/ata/libata-sff.c	2009-02-01 20:21:13.000000000 +0000
> @@ -773,18 +773,33 @@ unsigned int ata_sff_data_xfer32(struct
>  	else
>  		iowrite32_rep(data_addr, buf, words);
>  
> -	if (unlikely(slop)) {
> -		__le32 pad;
> -		if (rw == READ) {
> -			pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
> -			memcpy(buf + buflen - slop, &pad, slop);
> +	if (slop) {
> +		unsigned char *trailing_buf = buf + buflen - slop;
>   
> +   
>   
> +		if (slop <= 2) {
> +			__le16 slop_word;
> +			if (rw == READ) {
> +				slop_word = cpu_to_le16(ioread16(data_addr));
> +				memcpy(trailing_buf, &slop_word, slop);
> +			} else {
> +				slop_word = 0;
> +				memcpy(&slop_word, trailing_buf, slop);
> +				iowrite16(le16_to_cpu(slop_word), data_addr);
> +			}
>  		} else {
> -			memcpy(&pad, buf + buflen - slop, slop);
> -			iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
> +			__le32 slop_word;
> +			if (rw == READ) {
> +				slop_word = cpu_to_le32(ioread32(data_addr));
> +				memcpy(trailing_buf, &slop_word, slop);
> +			} else {
> +				slop_word = 0;
> +				memcpy(&slop_word, trailing_buf, slop);
> +				iowrite32(le32_to_cpu(slop_word), data_addr);
> +			}
>   

   How about the following?

		unsigned char *tail = buf + buflen - slop;
		unsigned char pad[4];

		if (rw == READ) {
			if (slop <= 2)
				ioread16_rep(data_addr, pad, 1);
			else
				ioread32_rep(data_addr, pad, 1);
			memcpy(tail, pad, slop);
 		} else {
			memcpy(pad, tail, slop);
			memset(pad + slop, 0, 4 - slop);
			if (slop <= 2)
				iowrite16_rep(data_addr, pad, 1);
			else
				iowrite32_rep(data_addr, pad, 1);
		}

>  	}
> -	return words << 2;
> +
> +	return buflen + (buflen & 1);
>   

    return (buflen + 1) & ~1;

   Well, I guess I could just have posted my own patch... :-)

MBR, Sergei