Patchwork Serialize CMD643 and CMD646 to fix a hardware bug with SSD

login
register
mail settings
Submitter Mikulas Patocka
Date Oct. 21, 2009, 6:55 p.m.
Message ID <Pine.LNX.4.64.0910211452160.17897@hs20-bc2-1.build.redhat.com>
Download mbox | patch
Permalink /patch/36615/
State Accepted
Delegated to: David Miller
Headers show

Comments

Mikulas Patocka - Oct. 21, 2009, 6:55 p.m.
Hi

This patch fixes a data corruption when SSD is connected to Ultra 5.

Mikulas

--

Serialize CMD643 and CMD646 to fix a hardware bug with SSD

CMD646 corrupts data on concurrent transfers on both channels when IDE SSD is
connected to one of the channels.

Setup that demonstrates this hardware bug: Ultra 5, onboard CMD646, rev 3.
/dev/hda is 8GB Seagate ST38410A in MWDMA2
/dev/hdd is 32GB SSD SiliconHardDisk in MWDMA2

- When reading /dev/hdd (for example with dd or fsck), reads from /dev/hda
  are corrupted, there are twiddled single bits 1->0 and some full 32-bit
  words corrupted, sometimes commands fail (which switches /dev/hda to
  PIO mode but the corruptions happen even in PIO).
- Reads from /dev/hdd don't seem to be corrupted (i.e. fsck passes fine).
- When I connected normal rotating harddisk to /dev/hdd, there was no
  corruption, so the corruption is something specific to SSD.
- I tried the same setup on a PCI card with CMD649 and saw no corruption.

This patch serializes the operation for CMD646 and 643 (I didn't test
CMD643 but it may have the same hw bug too because it's earlier design).
CMD649 is good. I don't know anything about CMD 648.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/ide/cmd64x.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikael Pettersson - Oct. 21, 2009, 7:34 p.m.
Mikulas Patocka writes:
 > Hi
 > 
 > This patch fixes a data corruption when SSD is connected to Ultra 5.
 > 
 > Mikulas
 > 
 > --
 > 
 > Serialize CMD643 and CMD646 to fix a hardware bug with SSD
 > 
 > CMD646 corrupts data on concurrent transfers on both channels when IDE SSD is
 > connected to one of the channels.
 > 
 > Setup that demonstrates this hardware bug: Ultra 5, onboard CMD646, rev 3.
 > /dev/hda is 8GB Seagate ST38410A in MWDMA2
 > /dev/hdd is 32GB SSD SiliconHardDisk in MWDMA2
 > 
 > - When reading /dev/hdd (for example with dd or fsck), reads from /dev/hda
 >   are corrupted, there are twiddled single bits 1->0 and some full 32-bit
 >   words corrupted, sometimes commands fail (which switches /dev/hda to
 >   PIO mode but the corruptions happen even in PIO).
 > - Reads from /dev/hdd don't seem to be corrupted (i.e. fsck passes fine).
 > - When I connected normal rotating harddisk to /dev/hdd, there was no
 >   corruption, so the corruption is something specific to SSD.
 > - I tried the same setup on a PCI card with CMD649 and saw no corruption.
 > 
 > This patch serializes the operation for CMD646 and 643 (I didn't test
 > CMD643 but it may have the same hw bug too because it's earlier design).
 > CMD649 is good. I don't know anything about CMD 648.

Have you checked if the libata pata_cmd64x driver also has this
problem?  I know that it works fine on the Ultra 5 with just a
single pata drive, but the pata+ssd combo may not have been tested.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Oct. 21, 2009, 7:39 p.m.
On Wednesday 21 October 2009 20:55:28 Mikulas Patocka wrote:
> Hi
> 
> This patch fixes a data corruption when SSD is connected to Ultra 5.
> 
> Mikulas
> 
> --
> 
> Serialize CMD643 and CMD646 to fix a hardware bug with SSD
> 
> CMD646 corrupts data on concurrent transfers on both channels when IDE SSD is
> connected to one of the channels.
> 
> Setup that demonstrates this hardware bug: Ultra 5, onboard CMD646, rev 3.
> /dev/hda is 8GB Seagate ST38410A in MWDMA2
> /dev/hdd is 32GB SSD SiliconHardDisk in MWDMA2
> 
> - When reading /dev/hdd (for example with dd or fsck), reads from /dev/hda
>   are corrupted, there are twiddled single bits 1->0 and some full 32-bit
>   words corrupted, sometimes commands fail (which switches /dev/hda to
>   PIO mode but the corruptions happen even in PIO).
> - Reads from /dev/hdd don't seem to be corrupted (i.e. fsck passes fine).
> - When I connected normal rotating harddisk to /dev/hdd, there was no
>   corruption, so the corruption is something specific to SSD.
> - I tried the same setup on a PCI card with CMD649 and saw no corruption.
> 
> This patch serializes the operation for CMD646 and 643 (I didn't test
> CMD643 but it may have the same hw bug too because it's earlier design).
> CMD649 is good. I don't know anything about CMD 648.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

Seems like SSD (simply by being faster) triggers some race condition that
hardware has tolerated in the past and since we used to always serialize
operation for CMD646 before:

commit e01698aed04811b9a9c4f8d54b73cb182757063d           
Author: David S. Miller <davem@davemloft.net>             
Date:   Sun Jun 21 22:48:03 2009 -0700

    ide cmd64x: Remove serialize setting.

it went undetected until now..
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikulas Patocka - Oct. 21, 2009, 11:01 p.m.
>  > This patch serializes the operation for CMD646 and 643 (I didn't test
>  > CMD643 but it may have the same hw bug too because it's earlier design).
>  > CMD649 is good. I don't know anything about CMD 648.
> 
> Have you checked if the libata pata_cmd64x driver also has this
> problem?  I know that it works fine on the Ultra 5 with just a
> single pata drive, but the pata+ssd combo may not have been tested.

I have tried libata driver now --- I see no data corruption but I've got 
this error under the same operation (both channels reading):

ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
ata1.00: BMDMA stat 0x24
ata1.00: cmd c8/00:10:10:c8:d9/00:00:00:00:00/e0 tag 0 dma 8192 in
         res 50/00:00:1f:c8:d9/00:00:00:00:00/e0 Emask 0x2 (HSM violation)
ata1.00: status: { DRDY }
pata_cmd64x: active 10 recovery 10 setup 3.
pata_cmd64x: active 10 recovery 10 setup 3.
ata1: soft resetting link
pata_cmd64x: active 3 recovery 1 setup 1.
pata_cmd64x: active 3 recovery 1 setup 1.
ata1.00: configured for MWDMA2
ata1: EH complete

(ata1 is the primary channel)
--- I'm wondering, what does it mean? status 0x50 is OK. DMA status 0x24 
is also OK. What was the problem there?

Mikulas
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 22, 2009, 12:41 a.m.
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Wed, 21 Oct 2009 21:39:24 +0200

> Seems like SSD (simply by being faster) triggers some race condition that
> hardware has tolerated in the past and since we used to always serialize
> operation for CMD646 before:

Yes, and technically we only did the synchronization for one
of the two chips mpatocka is adding the serialize setting to.

> commit e01698aed04811b9a9c4f8d54b73cb182757063d           
> Author: David S. Miller <davem@davemloft.net>             
> Date:   Sun Jun 21 22:48:03 2009 -0700
> 
>     ide cmd64x: Remove serialize setting.
> 
> it went undetected until now..

Right, and see also:

commit 6b5cde3629701258004b94cde75dd1089b556b02
Author: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date:   Mon Dec 29 20:27:32 2008 +0100

    cmd64x: set IDE_HFLAG_SERIALIZE explictly for CMD646

Which is how we got there.

The most conservative thing to do is to set the flag as
is done by mpatocka's patch but I'd like Frans to regression
test that on his ultra5.

Frans can you do that?

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Oct. 22, 2009, 9:44 a.m.
On Thursday 22 October 2009 02:41:55 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Wed, 21 Oct 2009 21:39:24 +0200
> 
> > Seems like SSD (simply by being faster) triggers some race condition that
> > hardware has tolerated in the past and since we used to always serialize
> > operation for CMD646 before:
> 
> Yes, and technically we only did the synchronization for one
> of the two chips mpatocka is adding the serialize setting to.
> 
> > commit e01698aed04811b9a9c4f8d54b73cb182757063d           
> > Author: David S. Miller <davem@davemloft.net>             
> > Date:   Sun Jun 21 22:48:03 2009 -0700
> > 
> >     ide cmd64x: Remove serialize setting.
> > 
> > it went undetected until now..
> 
> Right, and see also:
> 
> commit 6b5cde3629701258004b94cde75dd1089b556b02
> Author: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date:   Mon Dec 29 20:27:32 2008 +0100
> 
>     cmd64x: set IDE_HFLAG_SERIALIZE explictly for CMD646
> 
> Which is how we got there.

We are through this the second time and you're still not willing
neither to listen nor to read the code.  We always did serialization
for CMD646, we just used  hwif->chipset == ide_cmd646 (without using
IDE_HFLAG_SERIALIZE flag):

1061                 if (h && h->hwgroup) {  /* scan only initialized ports */
1062                         if (hwif->irq == h->irq) {
1063                                 hwif->sharing_irq = h->sharing_irq = 1;
1064                                 if (hwif->chipset != ide_pci ||
1065                                     h->chipset != ide_pci) {
1066                                         save_match(hwif, h, &match);
1067                                 }
1068                         }

so the code was using the same serialized hwgroup for CMD646 (which always
uses shared PCI IRQ AFAIK) because of hwif->chipset == ide_cmd646.  My patch
only made this explicit in preparation for other changes (one of such other
changes resulted later in uncovering unexpected IRQ problem on Ultra 5).

> The most conservative thing to do is to set the flag as
> is done by mpatocka's patch but I'd like Frans to regression
> test that on his ultra5.

Agreed, though I wonder whether we should also provide module parameter to
disable serializing on those chipsets for people not using SSDs...
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 22, 2009, 11 a.m.
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Thu, 22 Oct 2009 11:44:04 +0200

> We are through this the second time and you're still not willing
> neither to listen nor to read the code.

I can understand why you might attribute malice and ignorance to my
actions by default, but it doesn't apply here.

> We always did serialization for CMD646, we just used hwif->chipset
> == ide_cmd646 (without using IDE_HFLAG_SERIALIZE flag):

I fully acknowledge that we always serialized, sorry for not
being explicit.

I was just showing where the serialize IRQ flag got added
(and again, it was a correct change).
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Oct. 22, 2009, 11:15 a.m.
On Thursday 22 October 2009 13:00:49 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Thu, 22 Oct 2009 11:44:04 +0200
> 
> > We always did serialization for CMD646, we just used hwif->chipset
> > == ide_cmd646 (without using IDE_HFLAG_SERIALIZE flag):
> 
> I fully acknowledge that we always serialized, sorry for not
> being explicit.
> 
> I was just showing where the serialize IRQ flag got added
> (and again, it was a correct change).

What for?  The only patch changing behavior was yours and it seems
your luck is much worse than mine when it comes to unexpected side
effects.. ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 22, 2009, 11:20 a.m.
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Thu, 22 Oct 2009 13:15:59 +0200

> On Thursday 22 October 2009 13:00:49 David Miller wrote:
>> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>> Date: Thu, 22 Oct 2009 11:44:04 +0200
>> 
>> > We always did serialization for CMD646, we just used hwif->chipset
>> > == ide_cmd646 (without using IDE_HFLAG_SERIALIZE flag):
>> 
>> I fully acknowledge that we always serialized, sorry for not
>> being explicit.
>> 
>> I was just showing where the serialize IRQ flag got added
>> (and again, it was a correct change).
> 
> What for?  The only patch changing behavior was yours and it seems
> your luck is much worse than mine when it comes to unexpected side
> effects.. ;)

Like I said, you can attribute my actions to malice or ignorance
if you want, but it simply isn't there.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Oct. 22, 2009, 1:56 p.m.
I doubt there is any hardware bug with the SSD. Some CMD64x hardware
certainly has errata and as the data sheets were published your starting
point would be the data sheets for the various chip versions. Does the
sparc use the same off the shelf part as the PC people or does it have
different glue or chip versions (eg as a macro cell in something else ?)

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 23, 2009, 1:30 a.m.
From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Thu, 22 Oct 2009 14:56:13 +0100

> I doubt there is any hardware bug with the SSD. Some CMD64x hardware
> certainly has errata and as the data sheets were published your starting
> point would be the data sheets for the various chip versions. Does the
> sparc use the same off the shelf part as the PC people or does it have
> different glue or chip versions (eg as a macro cell in something else ?)

The sparc machines use the standard cmd64x chips without any special
glue logic.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikulas Patocka - Oct. 23, 2009, 2:29 p.m.
> We are through this the second time and you're still not willing
> neither to listen nor to read the code.  We always did serialization
> for CMD646, we just used  hwif->chipset == ide_cmd646 (without using
> IDE_HFLAG_SERIALIZE flag):

> Agreed, though I wonder whether we should also provide module parameter to
> disable serializing on those chipsets for people not using SSDs...

Don't do it --- disks have cache and reading from the cache is as fast as 
reading from SSD (or even faster), so if there is some speed-race in the 
chip, there is a possibility that the data corruption happens with disks 
too --- just with lower probability.

If we don't know why the chip corrupts data, we must treat it as always 
corrupting data.

Mikulas
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 23, 2009, 2:31 p.m.
From: Mikulas Patocka <mpatocka@redhat.com>
Date: Fri, 23 Oct 2009 10:29:16 -0400 (EDT)

> If we don't know why the chip corrupts data, we must treat it as always 
> corrupting data.

Completely agreed.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Oct. 23, 2009, 2:44 p.m.
On Friday 23 October 2009 16:29:16 Mikulas Patocka wrote:
> > We are through this the second time and you're still not willing
> > neither to listen nor to read the code.  We always did serialization
> > for CMD646, we just used  hwif->chipset == ide_cmd646 (without using
> > IDE_HFLAG_SERIALIZE flag):
> 
> > Agreed, though I wonder whether we should also provide module parameter to
> > disable serializing on those chipsets for people not using SSDs...
> 
> Don't do it --- disks have cache and reading from the cache is as fast as 
> reading from SSD (or even faster), so if there is some speed-race in the 
> chip, there is a possibility that the data corruption happens with disks 
> too --- just with lower probability.
> 
> If we don't know why the chip corrupts data, we must treat it as always 
> corrupting data.

I actually suspect that it is device/chipset specific interaction and not
generic problem so adding a fallback option (w/ BIG FAT WARNING) seem to
make sense..  Especially since we have never serialized on CMD643 and your
patch will be adding performance regression without even verifying that
the issue also affects this chipset..
Mikulas Patocka - Oct. 23, 2009, 2:50 p.m.
> Right, and see also:
> 
> commit 6b5cde3629701258004b94cde75dd1089b556b02
> Author: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date:   Mon Dec 29 20:27:32 2008 +0100
> 
>     cmd64x: set IDE_HFLAG_SERIALIZE explictly for CMD646
> 
> Which is how we got there.
> 
> The most conservative thing to do is to set the flag as
> is done by mpatocka's patch but I'd like Frans to regression
> test that on his ultra5.
> 
> Frans can you do that?
> 
> Thanks.

I read the thread about wild interrupts that Frans was observing and that 
led to disabling the serialization.

The thing is tricky --- if we read status register on interrupt, we break 
the serialization principle and introduce potential data corruption.

If we don't read the status register, the wild interrupt kills the whole 
interrupt line.

I think the interrupt handler should read the BM-status register on both 
channels (it reflects the interrupt state even in PIO mode) to test if 
there is an unexpected interrupt on the inactive channel --- this should 
be much more safe than reading the status register. If there is an 
interrupt, then
--- read the status of the inactive channel? (potential data corruption, 
but it is reported to happen only on boot).
--- Or can the interrupt be acknowledged only in BM-status without 
touching the device? I believe yes, it shoud shut the PCI interrupt but it 
would leave the IDE interrupt line on (should be cleared on next command).

Mikulas
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikulas Patocka - Oct. 23, 2009, 2:55 p.m.
On Fri, 23 Oct 2009, Bartlomiej Zolnierkiewicz wrote:

> On Friday 23 October 2009 16:29:16 Mikulas Patocka wrote:
> > > We are through this the second time and you're still not willing
> > > neither to listen nor to read the code.  We always did serialization
> > > for CMD646, we just used  hwif->chipset == ide_cmd646 (without using
> > > IDE_HFLAG_SERIALIZE flag):
> > 
> > > Agreed, though I wonder whether we should also provide module parameter to
> > > disable serializing on those chipsets for people not using SSDs...
> > 
> > Don't do it --- disks have cache and reading from the cache is as fast as 
> > reading from SSD (or even faster), so if there is some speed-race in the 
> > chip, there is a possibility that the data corruption happens with disks 
> > too --- just with lower probability.
> > 
> > If we don't know why the chip corrupts data, we must treat it as always 
> > corrupting data.
> 
> I actually suspect that it is device/chipset specific interaction and not
> generic problem so adding a fallback option (w/ BIG FAT WARNING) seem to
> make sense..

So, why was it serialized before? I assume that either someone noticed the 
corruption or someone read some datasheet noting the corruption or someone 
reverse engineered some other driver and saw the serialization.

> Especially since we have never serialized on CMD643 and your patch will 
> be adding performance regression without even verifying that the issue 
> also affects this chipset..

Do you have this chip? If you were IDE maintainer, did you collect cards 
with IDE chips?

Mikulas

> -- 
> Bartlomiej Zolnierkiewicz
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Oct. 23, 2009, 3:03 p.m.
On Friday 23 October 2009 16:55:59 Mikulas Patocka wrote:
> 
> On Fri, 23 Oct 2009, Bartlomiej Zolnierkiewicz wrote:
> 
> > On Friday 23 October 2009 16:29:16 Mikulas Patocka wrote:
> > > > We are through this the second time and you're still not willing
> > > > neither to listen nor to read the code.  We always did serialization
> > > > for CMD646, we just used  hwif->chipset == ide_cmd646 (without using
> > > > IDE_HFLAG_SERIALIZE flag):
> > > 
> > > > Agreed, though I wonder whether we should also provide module parameter to
> > > > disable serializing on those chipsets for people not using SSDs...
> > > 
> > > Don't do it --- disks have cache and reading from the cache is as fast as 
> > > reading from SSD (or even faster), so if there is some speed-race in the 
> > > chip, there is a possibility that the data corruption happens with disks 
> > > too --- just with lower probability.
> > > 
> > > If we don't know why the chip corrupts data, we must treat it as always 
> > > corrupting data.
> > 
> > I actually suspect that it is device/chipset specific interaction and not
> > generic problem so adding a fallback option (w/ BIG FAT WARNING) seem to
> > make sense..
> 
> So, why was it serialized before? I assume that either someone noticed the 
> corruption or someone read some datasheet noting the corruption or someone 
> reverse engineered some other driver and saw the serialization.

Serialization is usually needed in case of chipset not handling concurrent
data transfers on both ports.  Unfortunately I don't know details for CMD646.

> > Especially since we have never serialized on CMD643 and your patch will 
> > be adding performance regression without even verifying that the issue 
> > also affects this chipset..
> 
> Do you have this chip? If you were IDE maintainer, did you collect cards 
> with IDE chips?

I recall having CMD648 and CMD649 cards somewhere, however not earlier
chipsets.
Daniela Engert - Oct. 23, 2009, 3:18 p.m.
Bartlomiej Zolnierkiewicz schrieb:
> On Friday 23 October 2009 16:55:59 Mikulas Patocka wrote:
>    
>> On Fri, 23 Oct 2009, Bartlomiej Zolnierkiewicz wrote:
>>
>>      
>>> On Friday 23 October 2009 16:29:16 Mikulas Patocka wrote:
>>>        
>>>>> We are through this the second time and you're still not willing
>>>>> neither to listen nor to read the code.  We always did serialization
>>>>> for CMD646, we just used  hwif->chipset == ide_cmd646 (without using
>>>>> IDE_HFLAG_SERIALIZE flag):
>>>>>            
>>>>          
>>>>> Agreed, though I wonder whether we should also provide module parameter to
>>>>> disable serializing on those chipsets for people not using SSDs...
>>>>>            
>>>> Don't do it --- disks have cache and reading from the cache is as fast as
>>>> reading from SSD (or even faster), so if there is some speed-race in the
>>>> chip, there is a possibility that the data corruption happens with disks
>>>> too --- just with lower probability.
>>>>
>>>> If we don't know why the chip corrupts data, we must treat it as always
>>>> corrupting data.
>>>>          
>>> I actually suspect that it is device/chipset specific interaction and not
>>> generic problem so adding a fallback option (w/ BIG FAT WARNING) seem to
>>> make sense..
>>>        
>> So, why was it serialized before? I assume that either someone noticed the
>> corruption or someone read some datasheet noting the corruption or someone
>> reverse engineered some other driver and saw the serialization.
>>      
> Serialization is usually needed in case of chipset not handling concurrent
> data transfers on both ports.  Unfortunately I don't know details for CMD646.
>    
Guys, this is old news. CMD643 and CMD646 have a shared data path from 
the PCI interface to the ATA channel ports. In a document issued by CMD, 
they explain the requirement for serialization. This issue is rectified 
with CMD648 and later chips.
>    
>>> Especially since we have never serialized on CMD643 and your patch will
>>> be adding performance regression without even verifying that the issue
>>> also affects this chipset..
>>>        
>> Do you have this chip? If you were IDE maintainer, did you collect cards
>> with IDE chips?
>>      
> I recall having CMD648 and CMD649 cards somewhere, however not earlier
> chipsets.
>
>    

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Oct. 23, 2009, 4:51 p.m.
> So, why was it serialized before? I assume that either someone noticed the 
> corruption or someone read some datasheet noting the corruption or someone 
> reverse engineered some other driver and saw the serialization.

The data sheet has some things to say for the 643 abd 646

In PIO mode it says you must check

CFR bit 2   (indicates primary interrupt)
ARTIM23 bit 4  (secondary interrupt)

Both bits can be written 1 to clear the interrupt. The doc doesn't say
anything about not touching status but it also uses the word "must" about
the other bits so make what you will of it.

In DMA mode I would read the BMDMA status in preference to status but the
doc doesn't specifically say to do so and simply say it works to the
spec. it isn't clear if ARTIM23 and CFR work for reporting/clearing a DMA
interrupt. You'd have to try it.

ARTIM23 may well need some locking care


The 646U2 adds the MRDMODE register so you can check the bits more sanely


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Oct. 23, 2009, 5:15 p.m.
> Don't do it --- disks have cache and reading from the cache is as fast as 
> reading from SSD (or even faster), so if there is some speed-race in the 
> chip, there is a possibility that the data corruption happens with disks 
> too --- just with lower probability.
> 
> If we don't know why the chip corrupts data, we must treat it as always 
> corrupting data.

In which case we should delete the driver because serializing it is
probably not sufficient. There is a proper way to deal with IDE problems
and randomly turning things on and off isn't the solution from experience.

So
- It would be useful to get the data sheet that Daniela refers to
- If there is some kind of data path issue then serializing probably isn't
  enough on its own as has been said

and we need to understand why the libata case doesn't show corruption but
clearly shows it is unhappy. Libata doesn't serialize the device and
doesn't do fancy IRQ checking either.

Just serializing is likely to make it worse - it may well become a rare
deeply obscure corruption rather than a nice visible one like we have now
- and that is far far nastier. Most likely libata also needs work.




--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - Oct. 23, 2009, 5:27 p.m.
Hello.

Alan Cox wrote:

>>So, why was it serialized before? I assume that either someone noticed the 
>>corruption or someone read some datasheet noting the corruption or someone 
>>reverse engineered some other driver and saw the serialization.

> The data sheet has some things to say for the 643 abd 646

> In PIO mode it says you must check

> CFR bit 2   (indicates primary interrupt)
> ARTIM23 bit 4  (secondary interrupt)

> Both bits can be written 1 to clear the interrupt.

    Not in the original PCI0646 datasheet -- it says that reading the 
register clears the interrupt bit. PCI0646U datashett however started 
talking about writing one to clear the bit.

> The doc doesn't say
> anything about not touching status but it also uses the word "must" about
> the other bits so make what you will of it.

    I think they use must in the sense that if the driver *really* needs to 
know from which channel the interrupt has come. Since most of the chip don't 
provide that capability, the real world Linux drivers, both old and new, had 
to get along without such knowledge. The IDE driver does read and clear the 
interrupt bits now though, in both PIO abnd DMA modes.

> In DMA mode I would read the BMDMA status in preference to status but the
> doc doesn't specifically say to do so and simply say it works to the
> spec. it isn't clear if ARTIM23 and CFR work for reporting/clearing a DMA
> interrupt. You'd have to try it.

    Earlier the IDE driver did chec CFR/ARTTIM23 bits before the BMIDE 
status interrupt bit in dma_test_irq() method and that used to work.  Now it 
also does so, via the new test_irq() method.

> ARTIM23 may well need some locking care

    Locking WRT what? It's not shared between channels...

> The 646U2 adds the MRDMODE register so you can check the bits more sanely

    Yeah. The libata driver however doesn't make use of that register, 
unlike  cmd64x. It doesn't even touch the interrupt bits on PCI0646, only 
manipulation the "old" bits on PCI-64[89].
    It seems that I need to sync up these two, pata_cmd64x.c seems to be 
lagging behind the changes in cmd64x.c. Well, not only this one, at least 
pata_hpt* drivers are lagging behind too...

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Oct. 23, 2009, 6:22 p.m.
>     Yeah. The libata driver however doesn't make use of that register, 
> unlike  cmd64x. It doesn't even touch the interrupt bits on PCI0646, only 
> manipulation the "old" bits on PCI-64[89].

Yes I purposefully left out all the complexity because when I tested the
cards I had it simply wasn't needed. I'm also not sure we should merge
anything from the old to the new one until we know why the old one
corrupts and the new one merely gets upset. Accidentally porting over a
corruptor would be very bad indeed.

>     It seems that I need to sync up these two, pata_cmd64x.c seems to be 
> lagging behind the changes in cmd64x.c. Well, not only this one, at least 
> pata_hpt* drivers are lagging behind too...

That would be useful - although several of the differences are because
the pata_hpt driver took from the vendor supplied reference code not the
old IDE code. It also seems more stable for having done that.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Oct. 23, 2009, 6:52 p.m.
On Friday 23 October 2009 20:22:50 Alan Cox wrote:
> >     Yeah. The libata driver however doesn't make use of that register, 
> > unlike  cmd64x. It doesn't even touch the interrupt bits on PCI0646, only 
> > manipulation the "old" bits on PCI-64[89].
> 
> Yes I purposefully left out all the complexity because when I tested the
> cards I had it simply wasn't needed. I'm also not sure we should merge
> anything from the old to the new one until we know why the old one
> corrupts and the new one merely gets upset. Accidentally porting over a
> corruptor would be very bad indeed.

Oh, we know that.

"Old" one corrupts because somebody thought it would be a smart move to remove
serialized flag instead of debugging certain problem properly..

"New" one gets serialized at the command issue level (like all other new shiny
libata PATA stuff) and this may as well explain the difference..

> >     It seems that I need to sync up these two, pata_cmd64x.c seems to be 
> > lagging behind the changes in cmd64x.c. Well, not only this one, at least 
> > pata_hpt* drivers are lagging behind too...

Just from the memory:

pata_sis, pata_atiixp, pata_it8213, pata_cs5536, pata_amd..

So, gentlemen, when are you planning to remove IDE (not from your playground
Linux distribution but from kernel.org)?

I've heard that it will happen "soon" 4 years ago... :)

> That would be useful - although several of the differences are because
> the pata_hpt driver took from the vendor supplied reference code not the
> old IDE code. It also seems more stable for having done that.

Yeah, right..

This reminds me of that hpt366 blacklist that somebody forgot to port over
initially and which resulted in real world data corruptions..
Sergei Shtylyov - Oct. 23, 2009, 8:50 p.m.
Hello.

Mikulas Patocka wrote:

>> Right, and see also:
>>
>> commit 6b5cde3629701258004b94cde75dd1089b556b02
>> Author: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>> Date:   Mon Dec 29 20:27:32 2008 +0100
>>
>>     cmd64x: set IDE_HFLAG_SERIALIZE explictly for CMD646
>>
>> Which is how we got there.
>>
>> The most conservative thing to do is to set the flag as
>> is done by mpatocka's patch but I'd like Frans to regression
>> test that on his ultra5.
>>
>> Frans can you do that?
>>
>> Thanks.
>>     
>
> I read the thread about wild interrupts that Frans was observing and that 
> led to disabling the serialization.
>
> The thing is tricky --- if we read status register on interrupt, we break 
> the serialization principle and introduce potential data corruption.
>
> If we don't read the status register, the wild interrupt kills the whole 
> interrupt line.
>
> I think the interrupt handler should read the BM-status register on both 
> channels (it reflects the interrupt state even in PIO mode) to test if 
>   

   Are you sure? Anyway, there's no need as we're reading the interrupt 
bits CFR/ARTTIM23 registers first (at least in the IDE code). Look at 
the cmd*_test_irq() methods and ide_intr().

> there is an unexpected interrupt on the inactive channel --- this should 
> be much more safe than reading the status register. If there is an 
> interrupt, then
> --- read the status of the inactive channel? (potential data corruption, 
> but it is reported to happen only on boot).
> --- Or can the interrupt be acknowledged only in BM-status without 
> touching the device? I believe yes,

   And I believe no. BMIDE status bit doesn't acknoledge (clear, to be 
precise) the IDE interrupts, only the status register read does.


> it shoud shut the PCI interrupt but it 
> would leave the IDE interrupt line on (should be cleared on next command).
>   

    I think the negated wired-OR of both INTRQ signals serves as an 
-INTA source, not the BMIDE status bits. At least in the general case, 
where the BMIDE status doesn't reflect PIO mode interrupts.

> Mikulas

WBR, Sergei


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 24, 2009, 3:24 a.m.
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Fri, 23 Oct 2009 20:52:47 +0200

> "Old" one corrupts because somebody thought it would be a smart move
> to remove serialized flag instead of debugging certain problem
> properly..

Bart, I fear you may live your entire life always with some
axe to grind with someone.

It makes it impossible to work with you effectively.

I've tried to start taking the personal attacks out of my
interactions with you, but you seem to be completely unable
to drop it.

If you're constantly bitter about this or that, nobody will
want to work with you.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frans Pop - Oct. 24, 2009, 11:28 a.m.
On Thursday 22 October 2009, David Miller wrote:
> The most conservative thing to do is to set the flag as
> is done by mpatocka's patch but I'd like Frans to regression
> test that on his ultra5.
>
> Frans can you do that?

Sure, if you send me the patch (or a link to it). May take a few days.

Also, is it still needed given the whole discussion that happened after 
this mail?

Cheers,
FJP
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 24, 2009, 11:31 a.m.
From: Frans Pop <elendil@planet.nl>
Date: Sat, 24 Oct 2009 13:28:39 +0200

> On Thursday 22 October 2009, David Miller wrote:
>> The most conservative thing to do is to set the flag as
>> is done by mpatocka's patch but I'd like Frans to regression
>> test that on his ultra5.
>>
>> Frans can you do that?
> 
> Sure, if you send me the patch (or a link to it). May take a few days.

It's here:

http://patchwork.ozlabs.org/patch/36615/

Also, all pending IDE patches can always be found at:

http://patchwork.ozlabs.org/project/linux-ide/list/

> Also, is it still needed given the whole discussion that happened after 
> this mail?

Yes, I still fully intend to apply mpatocka's patch, as-is.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Oct. 24, 2009, 12:38 p.m.
On Saturday 24 October 2009 05:24:47 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Fri, 23 Oct 2009 20:52:47 +0200
> 
> > "Old" one corrupts because somebody thought it would be a smart move
> > to remove serialized flag instead of debugging certain problem
> > properly..
> 
> Bart, I fear you may live your entire life always with some
> axe to grind with someone.
> 
> It makes it impossible to work with you effectively.

You mean that I don't enjoy the idea of "just a managers" trying to
"manage" my free time?  Well, I don't.

I also don't buy the definition of "working with" presented by you.

> I've tried to start taking the personal attacks out of my
> interactions with you, but you seem to be completely unable
> to drop it.
> 
> If you're constantly bitter about this or that, nobody will
> want to work with you.

I'm not bitter, I just stick to the facts.

My mails may sound rude sometimes but they _always_ have the backing
in facts (and if I'm wrong I have no problem with saying "sorry, I was
wrong").

If you want to see how (unprovoked) personal attacks look like please
go re-read your own mails from few months ago...

It is all here:

	http://patchwork.ozlabs.org/patch/28945/

Please start with:

| Unlike the commit log message states, I suspect this change
| "introduces" incorrect handling of unexpected IRQs rather than
| "fixing".  I suspect the problem arises when the controller

and

| That's why all the IDE drivers are constantly breaking on sparc.

parts.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 24, 2009, 12:58 p.m.
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Sat, 24 Oct 2009 14:38:00 +0200

> My mails may sound rude sometimes but they _always_ have the backing
> in facts (and if I'm wrong I have no problem with saying "sorry, I was
> wrong").

Being rude is not justified by being right.

> If you want to see how (unprovoked) personal attacks look like please
> go re-read your own mails from few months ago...

Yes, I am a complete asshole sometimes, and I need to improve
upon that.

Are you possesing such a level of willingness to change too?
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Oct. 24, 2009, 1:13 p.m.
On Saturday 24 October 2009 14:58:21 David Miller wrote:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> > If you want to see how (unprovoked) personal attacks look like please
> > go re-read your own mails from few months ago...
> 
> Yes, I am a complete asshole sometimes, and I need to improve
> upon that.

I also try to keep working on this but reality keeps standing in my way. ;)

> Are you possesing such a level of willingness to change too?

Lets just put all past misunderstandings behind and start over..
David Miller - Oct. 24, 2009, 1:20 p.m.
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Sat, 24 Oct 2009 15:13:12 +0200

> On Saturday 24 October 2009 14:58:21 David Miller wrote:
>> Are you possesing such a level of willingness to change too?
> 
> Lets just put all past misunderstandings behind and start over..

Sounds like a good idea to me :-)
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frans Pop - Oct. 25, 2009, 2:48 a.m.
On Saturday 24 October 2009, David Miller wrote:
> > On Thursday 22 October 2009, David Miller wrote:
> >> The most conservative thing to do is to set the flag as
> >> is done by mpatocka's patch but I'd like Frans to regression
> >> test that on his ultra5.
> >>
> >> Frans can you do that?
> >
> > Sure, if you send me the patch (or a link to it). May take a few days.
>
> It's here:
> http://patchwork.ozlabs.org/patch/36615/

Looks to work OK on my system. dmesg shows ide0 and ide1 are now 
serialized.

Cheers,
FJP
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikulas Patocka - Oct. 26, 2009, 11:30 a.m.
>   Are you sure? Anyway, there's no need as we're reading the interrupt bits
> CFR/ARTTIM23 registers first (at least in the IDE code). Look at the
> cmd*_test_irq() methods and ide_intr().

Maybe the BMIDE status bit is just the same as CFR/ARTTIM23 interrupt bit.

> > there is an unexpected interrupt on the inactive channel --- this should be
> > much more safe than reading the status register. If there is an interrupt,
> > then
> > --- read the status of the inactive channel? (potential data corruption, but
> > it is reported to happen only on boot).
> > --- Or can the interrupt be acknowledged only in BM-status without touching
> > the device? I believe yes,
> 
>   And I believe no. BMIDE status bit doesn't acknoledge (clear, to be precise)
> the IDE interrupts, only the status register read does.

There are two things: IDE interrupt line set by the device (BMIDE status 
doesn't do anything with it) and chipset's INT[A-D] interrupt line --- and 
BMIDE status should clear it, at least for some chipsets.

Some chipset documentation (not for CMD64x) thatI have says that BMIDE 
irq status is set on any interrupt regardless if it's DMA or NONDMA.

On ICH SATA (in legacy non-AHCI mode), it is even required to acknowledge 
PIO interrupts with BMIDE status, otherwise the interrupt stays pending.

> > it shoud shut the PCI interrupt but it would leave the IDE interrupt line on
> > (should be cleared on next command).
> >   
> 
>    I think the negated wired-OR of both INTRQ signals serves as an -INTA
> source, not the BMIDE status bits. At least in the general case, where the
> BMIDE status doesn't reflect PIO mode interrupts.

It is not as simple, INTA and BMIDE status must be postponed until the 
chip flushes its buffers and writes the DMA last byte to the memory.

I agree with you that for some chipsets BMIDE doesn't have to be signalled 
in PIO mode --- but remember that here we are talking about dealing with 
broken devices that set the interrupt line spuriously and about 
serializing chipsets --- not about all chipsets and all devices.

So the best that can be done for such broken devices is to try to shut the 
interrupt in BMIDE register (or PCI registers in CMD64x). There is nothing 
better to do. If you have serializing chipset that doesn't let you shut 
interrupt and the inactive device fires spuriously --- there is absolutely 
nothing that can be done about it.

Mikulas
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikulas Patocka - Oct. 26, 2009, 11:36 a.m.
On Fri, 23 Oct 2009, Alan Cox wrote:

> >     Yeah. The libata driver however doesn't make use of that register, 
> > unlike  cmd64x. It doesn't even touch the interrupt bits on PCI0646, only 
> > manipulation the "old" bits on PCI-64[89].
> 
> Yes I purposefully left out all the complexity because when I tested the
> cards I had it simply wasn't needed. I'm also not sure we should merge
> anything from the old to the new one until we know why the old one
> corrupts and the new one merely gets upset. Accidentally porting over a
> corruptor would be very bad indeed.

Well, if Daniela showed that cmd64[36] docs say it needs serialization, 
then just add it there. Don't rely on the fact that the corruption didn't 
happen in my case.

Mikulas
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Oct. 26, 2009, 12:18 p.m.
On Mon, 26 Oct 2009 07:36:48 -0400 (EDT)
Mikulas Patocka <mpatocka@redhat.com> wrote:

> On Fri, 23 Oct 2009, Alan Cox wrote:
> 
> > >     Yeah. The libata driver however doesn't make use of that register, 
> > > unlike  cmd64x. It doesn't even touch the interrupt bits on PCI0646, only 
> > > manipulation the "old" bits on PCI-64[89].
> > 
> > Yes I purposefully left out all the complexity because when I tested the
> > cards I had it simply wasn't needed. I'm also not sure we should merge
> > anything from the old to the new one until we know why the old one
> > corrupts and the new one merely gets upset. Accidentally porting over a
> > corruptor would be very bad indeed.
> 
> Well, if Daniela showed that cmd64[36] docs say it needs serialization, 
> then just add it there. Don't rely on the fact that the corruption didn't 
> happen in my case.

Daniela sent me the relevant document - it doesn't exactly match what is
being described here as it relates to motherboards improperly timing out
PCI access requests.

Basically the fix is "don't touch the task file on one channel while DMA
is live on the other". That means that for libata serialize is not
sufficient on a shared IRQ set up as we'll touch ALTSTATUS. Plus it is
overkill as you can have parallel issue PIO commands.

For libata the best way to fix it seems to be to avoid parallel issuing
commands with an ATA_PROT_DMA/ATAPI_PROT_DMA command outstanding, and to
check the IRQ bits. Patch to follow when I get a moment.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - Oct. 26, 2009, 6:20 p.m.
Hello.

Mikulas Patocka wrote:

>>  Are you sure? Anyway, there's no need as we're reading the interrupt bits
>>CFR/ARTTIM23 registers first (at least in the IDE code). Look at the
>>cmd*_test_irq() methods and ide_intr().

> Maybe the BMIDE status bit is just the same as CFR/ARTTIM23 interrupt bit.

    Maybe -- if it indeed gets set in PIO mode as well. In this case though, 
there's quite little sense in having that bit mirrored (even twice with the 
newer controllers).

>>>there is an unexpected interrupt on the inactive channel --- this should be
>>>much more safe than reading the status register. If there is an interrupt,
>>>then
>>>--- read the status of the inactive channel? (potential data corruption, but
>>>it is reported to happen only on boot).
>>>--- Or can the interrupt be acknowledged only in BM-status without touching
>>>the device? I believe yes,

>>  And I believe no. BMIDE status bit doesn't acknoledge (clear, to be precise)
>>the IDE interrupts, only the status register read does.

> There are two things: IDE interrupt line set by the device (BMIDE status 
> doesn't do anything with it) and chipset's INT[A-D] interrupt line --- and 
> BMIDE status should clear it, at least for some chipsets.

> Some chipset documentation (not for CMD64x) thatI have says that BMIDE 
> irq status is set on any interrupt regardless if it's DMA or NONDMA.

    That is rather untypical behavior although some chipsets like Intel ICH 
are known to do it.

> On ICH SATA (in legacy non-AHCI mode), it is even required to acknowledge 
> PIO interrupts with BMIDE status, otherwise the interrupt stays pending.

>>>it shoud shut the PCI interrupt but it would leave the IDE interrupt line on
>>>(should be cleared on next command).

>>   I think the negated wired-OR of both INTRQ signals serves as an -INTA
>>source, not the BMIDE status bits. At least in the general case, where the
>>BMIDE status doesn't reflect PIO mode interrupts.

> It is not as simple, INTA and BMIDE status must be postponed until the 
> chip flushes its buffers and writes the DMA last byte to the memory.

    I know. The delay logic only acts in the DMA case. And it doesn't have 
to delay the interrupt itself, only the BMIDE status read with bit 2 set -- 
which is achievable by retrying the I/O transaction on PCI until the DMA 
actually completes.

> I agree with you that for some chipsets BMIDE doesn't have to be signalled 
> in PIO mode --- but remember that here we are talking about dealing with 
> broken devices that set the interrupt line spuriously and about 
> serializing chipsets --- not about all chipsets and all devices.

> So the best that can be done for such broken devices is to try to shut the 
> interrupt in BMIDE register (or PCI registers in CMD64x). There is nothing 
> better to do.

    And we're doing it, now for the PIO case also.

> If you have serializing chipset that doesn't let you shut 
> interrupt and the inactive device fires spuriously --- there is absolutely 
> nothing that can be done about it.

    Yes, seems so from ide_intr()...

> Mikulas

WBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 29, 2009, 10:02 a.m.
From: Frans Pop <elendil@planet.nl>
Date: Sun, 25 Oct 2009 03:48:32 +0100

> On Saturday 24 October 2009, David Miller wrote:
>> > On Thursday 22 October 2009, David Miller wrote:
>> >> The most conservative thing to do is to set the flag as
>> >> is done by mpatocka's patch but I'd like Frans to regression
>> >> test that on his ultra5.
>> >>
>> >> Frans can you do that?
>> >
>> > Sure, if you send me the patch (or a link to it). May take a few days.
>>
>> It's here:
>> http://patchwork.ozlabs.org/patch/36615/
> 
> Looks to work OK on my system. dmesg shows ide0 and ide1 are now 
> serialized.

Thanks a lot for testing Frans, I've applied Mikulas's patch
to ide-2.6
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

Index: linux-2.6.31-fast/drivers/ide/cmd64x.c
===================================================================
--- linux-2.6.31-fast.orig/drivers/ide/cmd64x.c	2009-10-21 06:08:42.000000000 +0200
+++ linux-2.6.31-fast/drivers/ide/cmd64x.c	2009-10-21 06:09:09.000000000 +0200
@@ -379,7 +379,8 @@  static const struct ide_port_info cmd64x
 		.enablebits	= {{0x00,0x00,0x00}, {0x51,0x08,0x08}},
 		.port_ops	= &cmd64x_port_ops,
 		.host_flags	= IDE_HFLAG_CLEAR_SIMPLEX |
-				  IDE_HFLAG_ABUSE_PREFETCH,
+				  IDE_HFLAG_ABUSE_PREFETCH |
+				  IDE_HFLAG_SERIALIZE,
 		.pio_mask	= ATA_PIO5,
 		.mwdma_mask	= ATA_MWDMA2,
 		.udma_mask	= 0x00, /* no udma */
@@ -389,7 +390,8 @@  static const struct ide_port_info cmd64x
 		.init_chipset	= init_chipset_cmd64x,
 		.enablebits	= {{0x51,0x04,0x04}, {0x51,0x08,0x08}},
 		.port_ops	= &cmd648_port_ops,
-		.host_flags	= IDE_HFLAG_ABUSE_PREFETCH,
+		.host_flags	= IDE_HFLAG_ABUSE_PREFETCH |
+				  IDE_HFLAG_SERIALIZE,
 		.pio_mask	= ATA_PIO5,
 		.mwdma_mask	= ATA_MWDMA2,
 		.udma_mask	= ATA_UDMA2,