PATCH: Fix broken PIO with libata

PATCH: Fix broken PIO with libata

[Alan Cox]

The revaldiation in 2.6.17-rc has broken support for PIO only devices.
This is fairly unusual in the SATA world but showed up rather more
promptly with the added PATA drivers.

The patch fixes two specific problem cases

#1	If you issue a DMA command via pass through the libata core blindly
issues a DMA command and calls DMA methods that are not present on PIO
only controllers causing an Oops. The patch does a simple check and
reject of a DMA command in PIO only cases.

#2	The core sets ATA_DFLAG_PIO to indicate PIO commands should be used
on this channel. This same information is available in dev->dma_mode but
for some reason we get two sources of the info. The ATA_DFLAG_PIO is set
once during setup and then cleared but not re-computed by the revalidate
function. This causes DMA commands to be issued when PIO would be and
usually an Oops or hang

Also contains a related bracketing fix


Signed-off-by: Alan Cox <alan@redhat.com>

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17-rc4/drivers/scsi/libata-core.c linux-2.6.17-rc4/drivers/scsi/libata-core.c
--- linux.vanilla-2.6.17-rc4/drivers/scsi/libata-core.c	2006-05-15 15:46:04.000000000 +0100
+++ linux-2.6.17-rc4/drivers/scsi/libata-core.c	2006-05-16 14:52:17.459504416 +0100
@@ -1757,6 +1763,10 @@
 		return rc;
 	}
 
+	/* This is cleared by the revalidation */
+	if (dev->xfer_shift == ATA_SHIFT_PIO)
+		dev->flags |= ATA_DFLAG_PIO;
+
 	DPRINTK("xfer_shift=%u, xfer_mode=0x%x\n",
 		dev->xfer_shift, (int)dev->xfer_mode);
 
diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.17-rc4/drivers/scsi/libata-scsi.c linux-2.6.17-rc4/drivers/scsi/libata-scsi.c
--- linux.vanilla-2.6.17-rc4/drivers/scsi/libata-scsi.c	2006-05-15 15:46:04.000000000 +0100
+++ linux-2.6.17-rc4/drivers/scsi/libata-scsi.c	2006-05-16 12:56:06.212295080 +0100
@@ -1944,7 +1944,7 @@
 		return 0;
 
 	dpofua = 0;
-	if (ata_dev_supports_fua(args->id) && dev->flags & ATA_DFLAG_LBA48 &&
+	if (ata_dev_supports_fua(args->id) && (dev->flags & ATA_DFLAG_LBA48) &&
 	    (!(dev->flags & ATA_DFLAG_PIO) || dev->multi_count))
 		dpofua = 1 << 4;
 
@@ -2414,9 +2414,15 @@
 {
 	struct ata_taskfile *tf = &(qc->tf);
 	struct scsi_cmnd *cmd = qc->scsicmd;
+	struct ata_device *dev = qc->dev;
+	struct ata_port *ap = qc->ap;
 
 	if ((tf->protocol = ata_scsi_map_proto(scsicmd[1])) == ATA_PROT_UNKNOWN)
 		goto invalid_fld;
+		
+	/* We may not issue DMA commands if no DMA mode is set */
+	if (tf->protocol == ATA_PROT_DMA && dev->dma_mode == 0)
+		goto invalid_fld;
 
 	if (scsicmd[1] & 0xe0)
 		/* PIO multi not supported yet */

Re: PATCH: Fix broken PIO with libata

[Kevin Radloff]

On 5/16/06, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> The revaldiation in 2.6.17-rc has broken support for PIO only devices.
> This is fairly unusual in the SATA world but showed up rather more
> promptly with the added PATA drivers.

Excellent; this seems to have solved my oops on CF card insertion problem. :)

However, I still have a problem with pata_pcmcia (that I actually
experienced also with the ide-cs driver) where sustained reading or
writing to the CF card spikes the CPU with nearly 100% system time.
Here's a few seconds of 'vmstat 5' with a 'cat /dev/sdb > /dev/null'
in the middle of it:

procs -----------memory---------- ---swap-- -----io---- --system-- ----cpu----
 r  b   swpd   free   buff  cache   si   so    bi    bo   in    cs us sy id wa
 1  1      0 502608  13304 254740    0    0   164    42 1089   618  7  2 85  5
 1  1      0 495440  20472 254740    0    0  1434     7 1078  1079  1 95  0  4
 1  1      0 487264  28664 254740    0    0  1638     8 1073  1063  1 95  0  4
 1  1      0 479940  35832 254740    0    0  1434    50 1077  1087  1 95  0  4
 1  1      0 472320  43000 254740    0    0  1434    15 1078  1093 10 86  0  4
 1  1      0 465104  50168 254740    0    0  1434    56 1077  1053  6 90  0  4
 1  1      0 456944  58360 254740    0    0  1638     8 1072  1063  1 94  0  5
 1  1      0 449744  65528 254740    0    0  1434     3 1073  1069  1 94  0  5
 1  1      0 441600  73720 254740    0    0  1638     5 1072  1071  1 95  0  4
 1  1      0 434020  80888 254740    0    0  1434     0 1074  1087 11 85  0  4
 0  0      0 428896  86008 254740    0    0  1024     3 1184  1313  4 83 11  3
 0  0      0 428896  86008 254740    0    0     0     5 1105  1057  1  0 98  0


-- 
Kevin 'radsaq' Radloff
radsaq@gmail.com
http://thesaq.com/

Re: PATCH: Fix broken PIO with libata

[Alan Cox]

On Maw, 2006-05-16 at 11:33 -0400, Kevin Radloff wrote:
> However, I still have a problem with pata_pcmcia (that I actually
> experienced also with the ide-cs driver) where sustained reading or
> writing to the CF card spikes the CPU with nearly 100% system time.

That is normal. The PCMCIA devices don't support DMA. As a result of
this the processor has to fetch each byte itself over the ISA speed
PCMCIA bus link.

Alan

Re: PATCH: Fix broken PIO with libata

[Kevin Radloff]

On 5/16/06, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Maw, 2006-05-16 at 11:33 -0400, Kevin Radloff wrote:
> > However, I still have a problem with pata_pcmcia (that I actually
> > experienced also with the ide-cs driver) where sustained reading or
> > writing to the CF card spikes the CPU with nearly 100% system time.
>
> That is normal. The PCMCIA devices don't support DMA. As a result of
> this the processor has to fetch each byte itself over the ISA speed
> PCMCIA bus link.

Hrm, as I recall that only started happening with ide-cs sometime in
the single digits of 2.6.x.. And note that it's only maxing out at
about 1.5MB/s. Should that saturate my laptop's 1.1GHz Pentium M
processor?

-- 
Kevin 'radsaq' Radloff
radsaq@gmail.com
http://thesaq.com/

Re: PATCH: Fix broken PIO with libata

[Jeff Garzik]

Kevin Radloff wrote:
> On 5/16/06, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> On Maw, 2006-05-16 at 11:33 -0400, Kevin Radloff wrote:
>> > However, I still have a problem with pata_pcmcia (that I actually
>> > experienced also with the ide-cs driver) where sustained reading or
>> > writing to the CF card spikes the CPU with nearly 100% system time.
>>
>> That is normal. The PCMCIA devices don't support DMA. As a result of
>> this the processor has to fetch each byte itself over the ISA speed
>> PCMCIA bus link.
> 
> Hrm, as I recall that only started happening with ide-cs sometime in
> the single digits of 2.6.x.. And note that it's only maxing out at
> about 1.5MB/s. Should that saturate my laptop's 1.1GHz Pentium M
> processor?

Doing data xfer using PIO rather than DMA definitely eats tons of CPU 
cycles.

	Jeff

Re: PATCH: Fix broken PIO with libata

[Tejun Heo]

Jeff Garzik wrote:
> Kevin Radloff wrote:
>> On 5/16/06, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>>> On Maw, 2006-05-16 at 11:33 -0400, Kevin Radloff wrote:
>>> > However, I still have a problem with pata_pcmcia (that I actually
>>> > experienced also with the ide-cs driver) where sustained reading or
>>> > writing to the CF card spikes the CPU with nearly 100% system time.
>>>
>>> That is normal. The PCMCIA devices don't support DMA. As a result of
>>> this the processor has to fetch each byte itself over the ISA speed
>>> PCMCIA bus link.
>>
>> Hrm, as I recall that only started happening with ide-cs sometime in
>> the single digits of 2.6.x.. And note that it's only maxing out at
>> about 1.5MB/s. Should that saturate my laptop's 1.1GHz Pentium M
>> processor?
> 
> Doing data xfer using PIO rather than DMA definitely eats tons of CPU 
> cycles.

Yeap, in addition, if doing real PIO (unbuffered by the HBA), the time 
it takes is soley determined by what PIO mode is in use.  It doesn't 
matter how fast the CPU is.  Faster CPUs only end up wasting more 
cycles.  :-(

-- 
tejun

Re: PATCH: Fix broken PIO with libata

[Kevin Radloff]

On 5/16/06, Tejun Heo <htejun@gmail.com> wrote:
> Jeff Garzik wrote:
> > Kevin Radloff wrote:
> >> On 5/16/06, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >>> On Maw, 2006-05-16 at 11:33 -0400, Kevin Radloff wrote:
> >>> > However, I still have a problem with pata_pcmcia (that I actually
> >>> > experienced also with the ide-cs driver) where sustained reading or
> >>> > writing to the CF card spikes the CPU with nearly 100% system time.
> >>>
> >>> That is normal. The PCMCIA devices don't support DMA. As a result of
> >>> this the processor has to fetch each byte itself over the ISA speed
> >>> PCMCIA bus link.
> >>
> >> Hrm, as I recall that only started happening with ide-cs sometime in
> >> the single digits of 2.6.x.. And note that it's only maxing out at
> >> about 1.5MB/s. Should that saturate my laptop's 1.1GHz Pentium M
> >> processor?
> >
> > Doing data xfer using PIO rather than DMA definitely eats tons of CPU
> > cycles.
>
> Yeap, in addition, if doing real PIO (unbuffered by the HBA), the time
> it takes is soley determined by what PIO mode is in use.  It doesn't
> matter how fast the CPU is.  Faster CPUs only end up wasting more
> cycles.  :-(

(oops, hit 'reply', but given the incredible importance of my response... ;P)

Ah, well then never mind. ;) I just have a dim memory of it being
different a long time ago. At least it works now. :D

-- 
Kevin 'radsaq' Radloff
radsaq@gmail.com
http://thesaq.com/

Re: PATCH: Fix broken PIO with libata

[Alan Cox]

On Maw, 2006-05-16 at 13:19 -0400, Kevin Radloff wrote:
> Hrm, as I recall that only started happening with ide-cs sometime in
> the single digits of 2.6.x.. And note that it's only maxing out at
> about 1.5MB/s. Should that saturate my laptop's 1.1GHz Pentium M
> processor?

Yes. It is possible that adding 32bit I/O support will boost this a
little but not much, and it may not work on all cards. The PCMCIA bus is
ISA speed, 1.5MB/sec is pretty much flat out


Alan

Re: PATCH: Fix broken PIO with libata

[Tejun Heo]

Alan Cox wrote:
> #2	The core sets ATA_DFLAG_PIO to indicate PIO commands should be used
> on this channel. This same information is available in dev->dma_mode but
> for some reason we get two sources of the info. The ATA_DFLAG_PIO is set
> once during setup and then cleared but not re-computed by the revalidate
> function. This causes DMA commands to be issued when PIO would be and
> usually an Oops or hang

Hmmm... I tried to fix this problem in the following commit.  With it,
ATA_DFLAG_PIO isn't cleared over ata_dev_configure().  Only
ata_dev_set_mode() is allowed to diddle with it and does about the same
thing as your patch does.

diff-tree ea1dd4e13010eb9dd5ffb4bfabbb472bc238bebb (from
198e0fed9e59461fc1890dd
Author: Tejun Heo <htejun@gmail.com>
Date:   Sun Apr 2 18:51:53 2006 +0900

    [PATCH] libata: clear only affected flags during ata_dev_configure()

    ata_dev_configure() should not clear dynamic device flags determined
    elsewhere.  Lower eight bits are reserved for feature flags, define
    ATA_DFLAG_CFG_MASK and clear only those bits before configuring
    device.  Without this patch, ATA_DFLAG_PIO gets turned off during
    revalidation making PIO mode unuseable.

    Signed-off-by: Tejun Heo <htejun@gmail.com>
    Signed-off-by: Jeff Garzik <jeff@garzik.org>

> Also contains a related bracketing fix

Is this agreed upon?  I tend to omit almost all unnecessary (by operator
precedence) parenthesis, so in new EH and all other stuff, the "a && b &
c" sort of lines are abundant.  If this is something that's agreed upon,
I can do a clean sweep over those.

-- 
tejun

Re: PATCH: Fix broken PIO with libata

[Jeff Garzik]

Tejun Heo wrote:
> Is this agreed upon?  I tend to omit almost all unnecessary (by operator
> precedence) parenthesis, so in new EH and all other stuff, the "a && b &
> c" sort of lines are abundant.  If this is something that's agreed upon,
> I can do a clean sweep over those.


More parens == easier to review.  So
	a && b & c
should be
	a && (b & c)

to clearly delineate the separate expressions to the human eye, and also
make it clear to the reader that the '&' is intended, and not a typo
that should have been '&&'.

Anytime you see a long string of 'if' conditions, and the operators
vary, add parents for readability.

	Jeff

Re: PATCH: Fix broken PIO with libata

[Tejun Heo]

Jeff Garzik wrote:
> Tejun Heo wrote:
>> Is this agreed upon?  I tend to omit almost all unnecessary (by operator
>> precedence) parenthesis, so in new EH and all other stuff, the "a && b &
>> c" sort of lines are abundant.  If this is something that's agreed upon,
>> I can do a clean sweep over those.
> 
> 
> More parens == easier to review.  So
> 	a && b & c
> should be
> 	a && (b & c)

Understood.  Usually, my rule is something like doing the least
maximizes consistency (style-wise) thus increasing readability in the
end, but rules usually suck, don't they?  What fits the most eyes is the
best, I guess.

> to clearly delineate the separate expressions to the human eye, and also
> make it clear to the reader that the '&' is intended, and not a typo
> that should have been '&&'.
> 
> Anytime you see a long string of 'if' conditions, and the operators
> vary, add parents for readability.
> 

Yeap, will do, from now on.

-- 
tejun

Re: PATCH: Fix broken PIO with libata

[Paul Mackerras]

Jeff Garzik writes:

> More parens == easier to review.  So

Well... mostly, but not always.  I agree about a && (b & c), but I
find the parens in ((a == b) && (c != d)) distracting.  And once we
get to three or more parens in a row, I have to start counting
carefully to see where they match up.

Paul.

Re: PATCH: Fix broken PIO with libata

[Jeff Garzik]

Tejun Heo wrote:
> Alan Cox wrote:
>> #2	The core sets ATA_DFLAG_PIO to indicate PIO commands should be used
>> on this channel. This same information is available in dev->dma_mode but
>> for some reason we get two sources of the info. The ATA_DFLAG_PIO is set
>> once during setup and then cleared but not re-computed by the revalidate
>> function. This causes DMA commands to be issued when PIO would be and
>> usually an Oops or hang
> 
> Hmmm... I tried to fix this problem in the following commit.  With it,
> ATA_DFLAG_PIO isn't cleared over ata_dev_configure().  Only
> ata_dev_set_mode() is allowed to diddle with it and does about the same
> thing as your patch does.

I presume he's looking at what users will hit when 2.6.17 is released...

	Jeff