| Message ID | Pine.LNX.4.64.0910211452160.17897@hs20-bc2-1.build.redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
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
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
> > 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
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
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
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
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
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
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
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
> 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
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
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..
> 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
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
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.
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
> 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
> 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
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
> 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
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..
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
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
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
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
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
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
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..
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
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
> 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
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
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
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
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
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,
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