[PATCH] libata-sff: fix 32-bit PIO regression

[PATCH] libata-sff: fix 32-bit PIO regression

[Sergei Shtylyov]

Commit 871af1210f13966ab911ed2166e4ab2ce775b99d (libata: Add 32bit PIO support)
caused all kind of errors on the ATAPI devices, so it's been empirically proven
that one shouldn't read/write an extra data word when a device isn't expecting
it already. "Don't do it then"; however still taking a chance to use 32-bit I/O
one last  time when there are exactly 3 trailing bytes.  Oh, and stop pointless
swapping bytes to and fro as well by using io*_rep() which shouldn't byte-swap.

This should fix the kernel.org bug #12609.

---
This is hopefully better replacement for Hugh Dickins most recent patch
(http://marc.info/?l=linux-ide&m=123352294619281)...

 drivers/ata/libata-sff.c |   23 ++++++++++++++++-------
 1 files changed, 16 insertions(+), 7 deletions(-)

Index: linux-2.6/drivers/ata/libata-sff.c
===================================================================
--- linux-2.6.orig/drivers/ata/libata-sff.c
+++ linux-2.6/drivers/ata/libata-sff.c
@@ -773,18 +773,27 @@ unsigned int ata_sff_data_xfer32(struct 
 	else
 		iowrite32_rep(data_addr, buf, words);
 
+	/* Transfer trailing bytes, if any */
 	if (unlikely(slop)) {
-		__le32 pad;
+		unsigned char pad[4];
+
+		/* Point buf to the tail of buffer */
+		buf += buflen - slop;
 		if (rw == READ) {
-			pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
-			memcpy(buf + buflen - slop, &pad, slop);
+			if (slop < 3)
+				ioread16_rep(data_addr, pad, 1);
+			else
+				ioread32_rep(data_addr, pad, 1);
+			memcpy(buf, pad, slop);
 		} else {
-			memcpy(&pad, buf + buflen - slop, slop);
-			iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
+			memcpy(pad, buf, slop);
+			if (slop < 3)
+				iowrite16_rep(data_addr, pad, 1);
+			else
+				iowrite32_rep(data_addr, pad, 1);
 		}
-		words++;
 	}
-	return words << 2;
+	return (buflen + 1) & ~1;
 }
 EXPORT_SYMBOL_GPL(ata_sff_data_xfer32);
 

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Alan Cox]

On Sun, 8 Feb 2009 23:53:07 +0400
Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:

> one last  time when there are exactly 3 trailing bytes.  Oh, and stop pointless
> swapping bytes to and fro as well by using io*_rep() which shouldn't byte-swap.

Nice - thats a very sneaky and clean way to do it but probably should be
commented or someone will "optimise" it one day ..

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Hugh Dickins]

On Sun, 8 Feb 2009, Sergei Shtylyov wrote:

> Commit 871af1210f13966ab911ed2166e4ab2ce775b99d (libata: Add 32bit PIO support)
> caused all kind of errors on the ATAPI devices, so it's been empirically proven
> that one shouldn't read/write an extra data word when a device isn't expecting
> it already. "Don't do it then"; however still taking a chance to use 32-bit I/O
> one last  time when there are exactly 3 trailing bytes.  Oh, and stop pointless
> swapping bytes to and fro as well by using io*_rep() which shouldn't byte-swap.
> 
> This should fix the kernel.org bug #12609.
> 
> ---
> This is hopefully better replacement for Hugh Dickins most recent patch
> (http://marc.info/?l=linux-ide&m=123352294619281)...

Yes, looks nice, and works for me.  My only criticism would be,
minor issue unchanged by your patch, that actually "slop" isn't
unlikely enough to deserve an "unlikely" - slop of 1 or 3 is
unlikely, but slop of 2 is quite common.

Hugh

> 
>  drivers/ata/libata-sff.c |   23 ++++++++++++++++-------
>  1 files changed, 16 insertions(+), 7 deletions(-)
> 
> Index: linux-2.6/drivers/ata/libata-sff.c
> ===================================================================
> --- linux-2.6.orig/drivers/ata/libata-sff.c
> +++ linux-2.6/drivers/ata/libata-sff.c
> @@ -773,18 +773,27 @@ unsigned int ata_sff_data_xfer32(struct 
>  	else
>  		iowrite32_rep(data_addr, buf, words);
>  
> +	/* Transfer trailing bytes, if any */
>  	if (unlikely(slop)) {
> -		__le32 pad;
> +		unsigned char pad[4];
> +
> +		/* Point buf to the tail of buffer */
> +		buf += buflen - slop;
>  		if (rw == READ) {
> -			pad = cpu_to_le32(ioread32(ap->ioaddr.data_addr));
> -			memcpy(buf + buflen - slop, &pad, slop);
> +			if (slop < 3)
> +				ioread16_rep(data_addr, pad, 1);
> +			else
> +				ioread32_rep(data_addr, pad, 1);
> +			memcpy(buf, pad, slop);
>  		} else {
> -			memcpy(&pad, buf + buflen - slop, slop);
> -			iowrite32(le32_to_cpu(pad), ap->ioaddr.data_addr);
> +			memcpy(pad, buf, slop);
> +			if (slop < 3)
> +				iowrite16_rep(data_addr, pad, 1);
> +			else
> +				iowrite32_rep(data_addr, pad, 1);
>  		}
> -		words++;
>  	}
> -	return words << 2;
> +	return (buflen + 1) & ~1;
>  }
>  EXPORT_SYMBOL_GPL(ata_sff_data_xfer32);
>  

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>> swapping bytes to and fro as well by using io*_rep() which shouldn't byte-swap.
>>     
>
> Nice - thats a very sneaky and clean way to do it but probably should be
> commented or someone will "optimise" it one day ..
>   

   OK, I'll recast and repost it -- along with the patch cleaning up the 
corresponding fragment of the 16-bit I/O method.

MBR, Sergei

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Sergei Shtylyov]

Hello.

Hugh Dickins wrote:

>> Commit 871af1210f13966ab911ed2166e4ab2ce775b99d (libata: Add 32bit PIO support)
>> caused all kind of errors on the ATAPI devices, so it's been empirically proven
>> that one shouldn't read/write an extra data word when a device isn't expecting
>> it already. "Don't do it then"; however still taking a chance to use 32-bit I/O
>> one last  time when there are exactly 3 trailing bytes.  Oh, and stop pointless
>> swapping bytes to and fro as well by using io*_rep() which shouldn't byte-swap.
>>
>> This should fix the kernel.org bug #12609.
>>
>> ---
>> This is hopefully better replacement for Hugh Dickins most recent patch
>> (http://marc.info/?l=linux-ide&m=123352294619281)...
>>     
>
> Yes, looks nice, and works for me.  My only criticism would be,
> minor issue unchanged by your patch, that actually "slop" isn't
> unlikely enough to deserve an "unlikely" - slop of 1 or 3 is
> unlikely, but slop of 2 is quite common.
>   

   I guess that's the case only for the ATAPI devices, so I'm going to 
keep it.

MBR, Sergei

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Mikael Pettersson]

Sergei Shtylyov writes:
 > Hello.
 > 
 > Hugh Dickins wrote:
 > 
 > >> Commit 871af1210f13966ab911ed2166e4ab2ce775b99d (libata: Add 32bit PIO support)
 > >> caused all kind of errors on the ATAPI devices, so it's been empirically proven
 > >> that one shouldn't read/write an extra data word when a device isn't expecting
 > >> it already. "Don't do it then"; however still taking a chance to use 32-bit I/O
 > >> one last  time when there are exactly 3 trailing bytes.  Oh, and stop pointless
 > >> swapping bytes to and fro as well by using io*_rep() which shouldn't byte-swap.
 > >>
 > >> This should fix the kernel.org bug #12609.
 > >>
 > >> ---
 > >> This is hopefully better replacement for Hugh Dickins most recent patch
 > >> (http://marc.info/?l=linux-ide&m=123352294619281)...
 > >>     
 > >
 > > Yes, looks nice, and works for me.  My only criticism would be,
 > > minor issue unchanged by your patch, that actually "slop" isn't
 > > unlikely enough to deserve an "unlikely" - slop of 1 or 3 is
 > > unlikely, but slop of 2 is quite common.
 > >   
 > 
 >    I guess that's the case only for the ATAPI devices, so I'm going to 
 > keep it.

I really don't think it's a good idea to play micro-optimisation
unlikely() tricks when the likelihood depends on what kind of device
the driver is talking to. In this case unlikely() is clearly wrong.

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Sergei Shtylyov]

Hello

Mikael Pettersson wrote:
> Sergei Shtylyov writes:
>  > Hello.
>  > 
>  > Hugh Dickins wrote:
>  > 
>  > >> Commit 871af1210f13966ab911ed2166e4ab2ce775b99d (libata: Add 32bit PIO support)
>  > >> caused all kind of errors on the ATAPI devices, so it's been empirically proven
>  > >> that one shouldn't read/write an extra data word when a device isn't expecting
>  > >> it already. "Don't do it then"; however still taking a chance to use 32-bit I/O
>  > >> one last  time when there are exactly 3 trailing bytes.  Oh, and stop pointless
>  > >> swapping bytes to and fro as well by using io*_rep() which shouldn't byte-swap.
>  > >>
>  > >> This should fix the kernel.org bug #12609.
>  > >>
>  > >> ---
>  > >> This is hopefully better replacement for Hugh Dickins most recent patch
>  > >> (http://marc.info/?l=linux-ide&m=123352294619281)...
>  > >>     
>  > >
>  > > Yes, looks nice, and works for me.  My only criticism would be,
>  > > minor issue unchanged by your patch, that actually "slop" isn't
>  > > unlikely enough to deserve an "unlikely" - slop of 1 or 3 is
>  > > unlikely, but slop of 2 is quite common.
>  > >   
>  > 
>  >    I guess that's the case only for the ATAPI devices, so I'm going to 
>  > keep it.
>
> I really don't think it's a good idea to play micro-optimisation
> unlikely() tricks when the likelihood depends on what kind of device
> the driver is talking to. In this case unlikely() is clearly wrong.
>   

   Clearly?! :-O
   Do you really think that the transfers having lengths non-divisible 
by 4 make any *significant* percentage even on the ATAPI devices? I 
think it's you who is really wrong.

MBR, Sergei

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Sergei Shtylyov]

Hello.

Hugh Dickins wrote:

>> Commit 871af1210f13966ab911ed2166e4ab2ce775b99d (libata: Add 32bit PIO support)
>> caused all kind of errors on the ATAPI devices, so it's been empirically proven
>> that one shouldn't read/write an extra data word when a device isn't expecting
>> it already. "Don't do it then"; however still taking a chance to use 32-bit I/O
>> one last  time when there are exactly 3 trailing bytes.  Oh, and stop pointless
>> swapping bytes to and fro as well by using io*_rep() which shouldn't byte-swap.
>>
>> This should fix the kernel.org bug #12609.
>>
>> ---
>> This is hopefully better replacement for Hugh Dickins most recent patch
>> (http://marc.info/?l=linux-ide&m=123352294619281)...
>>     
>
> Yes, looks nice, and works for me.  My only criticism would be,
> minor issue unchanged by your patch, that actually "slop" isn't
> unlikely enough to deserve an "unlikely" - slop of 1 or 3 is
> unlikely, but slop of 2 is quite common.
>   

  Common on what types of ATAPI commands? I can hardly believe that they 
are common on the block I/O...

MBR, Sergei

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Jeff Garzik]

Sergei Shtylyov wrote:
>   Do you really think that the transfers having lengths non-divisible by 
> 4 make any *significant* percentage even on the ATAPI devices? I think 
> it's you who is really wrong.

The answer depends on workload.  Though rare, workloads do exist that 
involve a lot of oddball querying via weird, vendor-specific SCSI[-ish] 
commands.

Moreover, the likelihood and cost of a branch mispredict are both low in 
this case, IMO.

Or a more human version of the rule:  if you have to have a long email 
thread about unlikely() placement, it is best just to avoid using 
unlikely() in that case at all.  Branch prediction units in modern CPUs 
are damned good anyways, and there is always the likelihood that a 
human-placed unlikely() becomes wrong in the future.

Plus the code is more readable without unlikely(), IMO.

	Jeff

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Hugh Dickins]

On Mon, 9 Feb 2009, Sergei Shtylyov wrote:
> Hugh Dickins wrote:
> 
> > Yes, looks nice, and works for me.  My only criticism would be,
> > minor issue unchanged by your patch, that actually "slop" isn't
> > unlikely enough to deserve an "unlikely" - slop of 1 or 3 is
> > unlikely, but slop of 2 is quite common.
> 
>  Common on what types of ATAPI commands? I can hardly believe that they are
> common on the block I/O...

When I checked (mounting and listing a CD), about 34% of
commands had slop 2.  But you're right, now I try copying
a large file, there's no slop involved in that at all.

Hugh

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Sergei Shtylyov]

Hello.

Jeff Garzik wrote:

>>   Do you really think that the transfers having lengths non-divisible 
>> by 4 make any *significant* percentage even on the ATAPI devices? I 
>> think it's you who is really wrong.
>
> The answer depends on workload.  Though rare, workloads do exist that 
> involve a lot of oddball querying via weird, vendor-specific 
> SCSI[-ish] commands.

   Can you give an example of a *continous* querying with the data 
transferring commands?
   Hm, it just occured to me that the typical ATAPI command packet is 12 
bytes long.

> Moreover, the likelihood and cost of a branch mispredict are both low 
> in this case, IMO.
>
> Or a more human version of the rule:  if you have to have a long email 
> thread about unlikely() placement, it is best just to avoid using 
> unlikely() in that case at all.  Branch prediction units in modern 
> CPUs are damned good anyways, and there is always the likelihood that 
> a human-placed unlikely() becomes wrong in the future.

   There are still CPUs without the branch prediction, you know -- Linux 
runs not only on x86.

> Plus the code is more readable without unlikely(), IMO.

   I tend to disagree. However, the packet command transfer is not 
unlikely at all, so I'll remove that unlikely() in the respun patch.

>     Jeff

MBR, Sergei

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Sergei Shtylyov]

Hello, I wrote:

>> The answer depends on workload.  Though rare, workloads do exist that 
>> involve a lot of oddball querying via weird, vendor-specific 
>> SCSI[-ish] commands.
>   Do you really think that the transfers having lengths non-divisible 
> by 4 make any *significant* percentage even on the ATAPI devices? I 
> think it's you who is really wrong.
>
>   Can you give an example of a *continous* querying with the data 
> transferring commands?
>   Hm, it just occured to me that the typical ATAPI command packet is 
> 12 bytes long.

   Haha, I even can't count! 12 divides by 4, of course. :-D

>> Or a more human version of the rule:  if you have to have a long 
>> email thread about unlikely() placement, it is best just to avoid 
>> using unlikely() in that case at all.  Branch prediction units in 
>> modern CPUs are damned good anyways, and there is always the 
>> likelihood that a human-placed unlikely() becomes wrong in the 
>> future. Moreover, the likelihood and cost of a branch mispredict are 
>> both low in this case, IMO.
>
>   There are still CPUs without the branch prediction, you know -- 
> Linux runs not only on x86.
>
>> Plus the code is more readable without unlikely(), IMO.
>
>   I tend to disagree. However, the packet command transfer is not 
> unlikely at all, so I'll remove that unlikely() in the respun patch.

   No, I'll keep it now. This case is indeed unlikely.

>>     Jeff

MBR, Sergei

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Sergei Shtylyov]

Hugh Dickins wrote:

>>>Yes, looks nice, and works for me.  My only criticism would be,
>>>minor issue unchanged by your patch, that actually "slop" isn't
>>>unlikely enough to deserve an "unlikely" - slop of 1 or 3 is
>>>unlikely, but slop of 2 is quite common.

>> Common on what types of ATAPI commands? I can hardly believe that they are
>>common on the block I/O...

> When I checked (mounting and listing a CD), about 34% of
> commands had slop 2.

    Hm, that probably involved the raw block reads. I don't remember the raw 
sector length off-hand but it may well be just even, not divisble by 4.

> But you're right, now I try copying
> a large file, there's no slop involved in that at all.

> Hugh

MBR, Sergei

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Sergei Shtylyov]

Hello, I wrote:

>>>> Yes, looks nice, and works for me.  My only criticism would be,
>>>> minor issue unchanged by your patch, that actually "slop" isn't
>>>> unlikely enough to deserve an "unlikely" - slop of 1 or 3 is
>>>> unlikely, but slop of 2 is quite common.

>>> Common on what types of ATAPI commands? I can hardly believe that 
>>> they are
>>> common on the block I/O...

>> When I checked (mounting and listing a CD), about 34% of
>> commands had slop 2.

>    Hm, that probably involved the raw block reads. I don't remember the 
> raw sector length off-hand but it may well be just even, not divisble by 4.

    Oops, I read "listening" instead of "listing". :-/
    But I thought that listing shouldn't involve anything other than block 
reads. OTOH, I'm not familiar enough with ISO9660... What commands are you 
seeing (if you have them dumped)?

>> But you're right, now I try copying
>> a large file, there's no slop involved in that at all.


>> Hugh

MBR, Sergei

Re: [PATCH] libata-sff: fix 32-bit PIO regression

[Hugh Dickins]

On Mon, 9 Feb 2009, Sergei Shtylyov wrote:
> Hello, I wrote:
> 
> > > When I checked (mounting and listing a CD), about 34% of
> > > commands had slop 2.
> 
> >    Hm, that probably involved the raw block reads. I don't remember the raw
> > sector length off-hand but it may well be just even, not divisble by 4.
> 
>    Oops, I read "listening" instead of "listing". :-/
>    But I thought that listing shouldn't involve anything other than block 
> reads. OTOH, I'm not familiar enough with ISO9660... What commands are you
> seeing (if you have them dumped)?

No, I didn't dump out the commands, just added temporary slop stats
into the xfer32 function, to check what paths were being tested.

That machine is busy running tests now: I could try again when it's
done, what would you recommend for dumping out the commands (remember,
I'm not an ATA developer)?

Though, to be honest, I'd prefer to just let this unlikely subject drop!

Hugh