Messages in this thread | | | Date | Mon, 02 Feb 2009 14:48:57 +0300 | From | Sergei Shtylyov <> | Subject | Re: 2.6.29-rc libata sff 32bit PIO regression |
| |
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
| |