2.6.15-rc1: IDE: fix potential data corruption with SL82C105 interfaces

2.6.15-rc1: IDE: fix potential data corruption with SL82C105 interfaces

[Russell King]

We must _never_ _ever_ on pain of death enable IDE DMA on SL82C105
chipsets where the southbridge revision is <= 5, otherwise data
corruption will occur.

Strangely this used to work, but something has changed in the upper
echelons of the IDE layer to break the hosts decision to deny DMA.
Let's make it crystal clear to the IDE layer that we know best.

Note: due to the urgency of this fix, I will be applying this to the
ARM tree.  Any comments/criticisms can be dealt with further patches.


diff -up -x BitKeeper -x ChangeSet -x SCCS -x _xlk -x *.orig -x *.rej -x .git linus/drivers/ide/pci/sl82c105.c linux/drivers/ide/pci/sl82c105.c
--- linus/drivers/ide/pci/sl82c105.c	Sun Nov  6 22:16:04 2005
+++ linux/drivers/ide/pci/sl82c105.c	Sat Nov 12 16:50:20 2005
@@ -399,34 +399,6 @@ static unsigned int __devinit init_chips
 	return dev->irq;
 }
 
-static void __devinit init_dma_sl82c105(ide_hwif_t *hwif, unsigned long dma_base)
-{
-	unsigned int rev;
-	u8 dma_state;
-
-	DBG(("init_dma_sl82c105(hwif: ide%d, dma_base: 0x%08x)\n", hwif->index, dma_base));
-
-	hwif->autodma = 0;
-
-	if (!dma_base)
-		return;
-
-	dma_state = hwif->INB(dma_base + 2);
-	rev = sl82c105_bridge_revision(hwif->pci_dev);
-	if (rev <= 5) {
-		printk("    %s: Winbond 553 bridge revision %d, BM-DMA disabled\n",
-		       hwif->name, rev);
-		dma_state &= ~0x60;
-	} else {
-		dma_state |= 0x60;
-		if (!noautodma)
-			hwif->autodma = 1;
-	}
-	hwif->OUTB(dma_state, dma_base + 2);
-
-	ide_setup_dma(hwif, dma_base, 8);
-}
-
 /*
  * Initialise the chip
  */
@@ -434,6 +406,8 @@ static void __devinit init_dma_sl82c105(
 static void __devinit init_hwif_sl82c105(ide_hwif_t *hwif)
 {
 	struct pci_dev *dev = hwif->pci_dev;
+	unsigned int rev;
+	u8 dma_state;
 	u32 val;
 	
 	DBG(("init_hwif_sl82c105(hwif: ide%d)\n", hwif->index));
@@ -455,33 +429,51 @@ static void __devinit init_hwif_sl82c105
 	pci_read_config_dword(dev, 0x40, &val);
 	*((u32 *)&hwif->hwif_data) = val;
 	
+	hwif->atapi_dma = 0;
+	hwif->mwdma_mask = 0;
+	hwif->swdma_mask = 0;
+	hwif->autodma = 0;
+
 	if (!hwif->dma_base)
 		return;
 
-	hwif->atapi_dma = 1;
-	hwif->mwdma_mask = 0x07;
-	hwif->swdma_mask = 0x07;
-
+	dma_state = hwif->INB(hwif->dma_base + 2) & ~0x60;
+	rev = sl82c105_bridge_revision(hwif->pci_dev);
+	if (rev <= 5) {
+		/*
+		 * Never ever EVER under any circumstances enable
+		 * DMA when the bridge is this old.
+		 */
+		printk("    %s: Winbond 553 bridge revision %d, BM-DMA disabled\n",
+		       hwif->name, rev);
+	} else {
 #ifdef CONFIG_BLK_DEV_IDEDMA
-	hwif->ide_dma_check = &sl82c105_check_drive;
-	hwif->ide_dma_on = &sl82c105_ide_dma_on;
-	hwif->ide_dma_off_quietly = &sl82c105_ide_dma_off_quietly;
-	hwif->ide_dma_lostirq = &sl82c105_ide_dma_lost_irq;
-	hwif->dma_start = &sl82c105_ide_dma_start;
-	hwif->ide_dma_timeout = &sl82c105_ide_dma_timeout;
-
-	if (!noautodma)
-		hwif->autodma = 1;
-	hwif->drives[0].autodma = hwif->autodma;
-	hwif->drives[1].autodma = hwif->autodma;
+		dma_state |= 0x60;
+
+		hwif->atapi_dma = 1;
+		hwif->mwdma_mask = 0x07;
+		hwif->swdma_mask = 0x07;
+
+		hwif->ide_dma_check = &sl82c105_check_drive;
+		hwif->ide_dma_on = &sl82c105_ide_dma_on;
+		hwif->ide_dma_off_quietly = &sl82c105_ide_dma_off_quietly;
+		hwif->ide_dma_lostirq = &sl82c105_ide_dma_lost_irq;
+		hwif->dma_start = &sl82c105_ide_dma_start;
+		hwif->ide_dma_timeout = &sl82c105_ide_dma_timeout;
+
+		if (!noautodma)
+			hwif->autodma = 1;
+		hwif->drives[0].autodma = hwif->autodma;
+		hwif->drives[1].autodma = hwif->autodma;
 #endif /* CONFIG_BLK_DEV_IDEDMA */
+	}
+	hwif->OUTB(dma_state, hwif->dma_base + 2);
 }
 
 static ide_pci_device_t sl82c105_chipset __devinitdata = {
 	.name		= "W82C105",
 	.init_chipset	= init_chipset_sl82c105,
 	.init_hwif	= init_hwif_sl82c105,
-	.init_dma	= init_dma_sl82c105,
 	.channels	= 2,
 	.autodma	= NOAUTODMA,
 	.enablebits	= {{0x40,0x01,0x01}, {0x40,0x10,0x10}},


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: 2.6.15-rc1: IDE: fix potential data corruption with SL82C105 interfaces

[Alan Cox]

On Sad, 2005-11-12 at 16:55 +0000, Russell King wrote:
> We must _never_ _ever_ on pain of death enable IDE DMA on SL82C105
> chipsets where the southbridge revision is <= 5, otherwise data
> corruption will occur.


If you are fixing this driver also set ->serialize = 1; before someone
with dual channel device gets burned.

Re: 2.6.15-rc1: IDE: fix potential data corruption with SL82C105 interfaces

[Russell King]

On Sat, Nov 12, 2005 at 06:03:34PM +0000, Alan Cox wrote:
> On Sad, 2005-11-12 at 16:55 +0000, Russell King wrote:
> > We must _never_ _ever_ on pain of death enable IDE DMA on SL82C105
> > chipsets where the southbridge revision is <= 5, otherwise data
> > corruption will occur.
> 
> If you are fixing this driver also set ->serialize = 1; before someone
> with dual channel device gets burned.

Thanks for reminding me - I've included that as well.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: 2.6.15-rc1: IDE: fix potential data corruption with SL82C105 interfaces

[Sergei Shtylyov]

Hello.

Alan Cox wrote:
 > On Sad, 2005-11-12 at 16:55 +0000, Russell King wrote:
 >
 >>We must _never_ _ever_ on pain of death enable IDE DMA on SL82C105
 >>chipsets where the southbridge revision is <= 5, otherwise data
 >>corruption will occur.

 > If you are fixing this driver also set ->serialize = 1; before someone
 > with dual channel device gets burned.

    Heh, this driver also tries to cache the single PCI register per-channel 
like hpt366.c... This buglet concerns using fast PIO timings and is probably 
harmless though but needs to be fixed -- I'll send a patch soon...
    I wonder what is otherwise wrong with using 2 channels concurrently?

MBR, Sergei

Re: 2.6.15-rc1: IDE: fix potential data corruption with SL82C105 interfaces

[Alan Cox]

On Llu, 2006-05-15 at 18:32 +0400, Sergei Shtylyov wrote:
>     Heh, this driver also tries to cache the single PCI register per-channel 
> like hpt366.c... This buglet concerns using fast PIO timings and is probably 
> harmless though but needs to be fixed -- I'll send a patch soon...
>     I wonder what is otherwise wrong with using 2 channels concurrently?

I've not got any dual channel devices to test, and in fact I couldn't
find anyone with dual channel stuff at all. The caching is one bug, the
fact the reset hits both channels is the other I know about.

Otherwise the libata driver is fairly similar although the timing is
pre-computed from the documentation for the DMA modes.

Alan

Re: 2.6.15-rc1: IDE: fix potential data corruption with SL82C105 interfaces

[Sergei Shtylyov]

Hello.

Alan Cox wrote:
> On Llu, 2006-05-15 at 18:32 +0400, Sergei Shtylyov wrote:

>>    Heh, this driver also tries to cache the single PCI register per-channel 
>>like hpt366.c... This buglet concerns using fast PIO timings and is probably 
>>harmless though but needs to be fixed -- I'll send a patch soon...
>>    I wonder what is otherwise wrong with using 2 channels concurrently?

> I've not got any dual channel devices to test, and in fact I couldn't
> find anyone with dual channel stuff at all.

    Hm, I thought they're all dual channel, at least from W83C553F docs. We 
have this chip on several embedded boards -- I'll try to gain access to one of 
them when I get to this driver...

> The caching is one bug, the
> fact the reset hits both channels is the other I know about.

    Ah, that register 0x7E reset? Strangely, W83C55[34]F datasheets don't even 
mention it. :-/

> Otherwise the libata driver is fairly similar

    Found it, looking...

> although the timing is
> pre-computed from the documentation for the DMA modes.

    As these chips lack 66 MHz PCI support, this should be quite enough, I 
think... :-)

MBR, Sergei

Re: 2.6.15-rc1: IDE: fix potential data corruption with SL82C105 interfaces

[Alan Cox]

On Llu, 2006-05-15 at 20:05 +0400, Sergei Shtylyov wrote:

The chips may be dual channel, none of the users I have access to are
using the second channel ..

>     Ah, that register 0x7E reset? Strangely, W83C55[34]F datasheets don't even 
> mention it. :-/

Its in the errata document for the ones I have. You want "W83C553F-G
Engineering Notice Nov 19 2001"

Re: 2.6.15-rc1: IDE: fix potential data corruption with SL82C105 interfaces

[Russell King]

On Mon, May 15, 2006 at 08:05:54PM +0400, Sergei Shtylyov wrote:
> >The caching is one bug, the
> >fact the reset hits both channels is the other I know about.
> 
>    Ah, that register 0x7E reset? Strangely, W83C55[34]F datasheets don't 
>    even mention it. :-/

They don't - it's an undocumented register as far as the data sheets go,
but if you're lucky enough to have the chip errata, you'll be told to
use it explicitly to work around bugs.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: 2.6.15-rc1: IDE: fix potential data corruption with SL82C105 interfaces

[Bartlomiej Zolnierkiewicz]

On 11/12/05, Russell King <rmk+lkml@arm.linux.org.uk> wrote:
> We must _never_ _ever_ on pain of death enable IDE DMA on SL82C105
> chipsets where the southbridge revision is <= 5, otherwise data
> corruption will occur.
>
> Strangely this used to work, but something has changed in the upper
> echelons of the IDE layer to break the hosts decision to deny DMA.
> Let's make it crystal clear to the IDE layer that we know best.

Has it changed recently?

AFAICS this bug was introduced long time ago in the sl82c105
driver itself by setting hwif->autodma in init_hwif_sl82c105()
without checking for bridge revision:

http://linux.bkbits.net:8080/linux-2.6/patch@1.497.94.23?nav=index.html|src/|src/drivers|src/drivers/ide|src/drivers/ide/pci|related/drivers/ide/pci/sl82c105.c|cset@1.497.94.23

> Note: due to the urgency of this fix, I will be applying this to the
> ARM tree.  Any comments/criticisms can be dealt with further patches.

Fine with me.

Bartlomiej