Mailing List Archive: Linux: Kernel

PATCH: Fix broken PIO with libata

Index | Next | Previous | View Threaded

alan at lxorguk

May 16, 2006, 7:39 AM

Post #1 of 13 (818 views)
Permalink

PATCH: Fix broken PIO with libata

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 [at] redhat>

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 */

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

radsaq at gmail

May 16, 2006, 8:33 AM

Post #2 of 13 (794 views)
Permalink

Re: PATCH: Fix broken PIO with libata [In reply to]

On 5/16/06, Alan Cox <alan [at] lxorguk> 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 [at] gmail
http://thesaq.com/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

htejun at gmail

May 16, 2006, 8:36 AM

Post #3 of 13 (792 views)
Permalink

Re: PATCH: Fix broken PIO with libata [In reply to]

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 [at] gmail>
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 [at] gmail>
Signed-off-by: Jeff Garzik <jeff [at] garzik>

> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

jgarzik at pobox

May 16, 2006, 8:48 AM

Post #4 of 13 (795 views)
Permalink

Re: PATCH: Fix broken PIO with libata [In reply to]

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

alan at lxorguk

May 16, 2006, 8:53 AM

Post #5 of 13 (802 views)
Permalink

Re: PATCH: Fix broken PIO with libata [In reply to]

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

htejun at gmail

May 16, 2006, 8:57 AM

Post #6 of 13 (792 views)
Permalink

Re: PATCH: Fix broken PIO with libata [In reply to]

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

jgarzik at pobox

May 16, 2006, 10:10 AM

Post #7 of 13 (802 views)
Permalink

Re: PATCH: Fix broken PIO with libata [In reply to]

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

radsaq at gmail

May 16, 2006, 10:19 AM

Post #8 of 13 (788 views)
Permalink

Re: PATCH: Fix broken PIO with libata [In reply to]

On 5/16/06, Alan Cox <alan [at] lxorguk> 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 [at] gmail
http://thesaq.com/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

jeff at garzik

May 16, 2006, 10:27 AM

Post #9 of 13 (792 views)
Permalink

Re: PATCH: Fix broken PIO with libata [In reply to]

Kevin Radloff wrote:
> On 5/16/06, Alan Cox <alan [at] lxorguk> 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

htejun at gmail

May 16, 2006, 10:39 AM

Post #10 of 13 (781 views)
Permalink

Re: PATCH: Fix broken PIO with libata [In reply to]

Jeff Garzik wrote:
> Kevin Radloff wrote:
>> On 5/16/06, Alan Cox <alan [at] lxorguk> 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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

radsaq at gmail

May 16, 2006, 11:13 AM

Post #11 of 13 (804 views)
Permalink

Re: PATCH: Fix broken PIO with libata [In reply to]

On 5/16/06, Tejun Heo <htejun [at] gmail> wrote:
> Jeff Garzik wrote:
> > Kevin Radloff wrote:
> >> On 5/16/06, Alan Cox <alan [at] lxorguk> 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 [at] gmail
http://thesaq.com/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

alan at lxorguk

May 16, 2006, 3:51 PM

Post #12 of 13 (794 views)
Permalink

Re: PATCH: Fix broken PIO with libata [In reply to]

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

paulus at samba

May 16, 2006, 4:00 PM

Post #13 of 13 (796 views)
Permalink

Re: PATCH: Fix broken PIO with libata [In reply to]

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads