Skip to contentSkip to conversation

ide: PCI BMDMA initialization fixes

PATCH NAK Bartlomiej Zolnierkiewicz 2 participants 4 messages Oct 1, 2007 – Oct 24, 2007 libata

lore
Share & tools
lore

Bartlomiej Zolnierkiewicz

author
#0 lore raw

PATCH 11/15 ide: PCI BMDMA initialization fixes

* Set hwif->dma_base only if allocation of extra ports succeeds.

* Allocate hwif->dmatable_cpu before reserving I/O ports.

While at it:

* Move setting of hwif->dma_{base,master} from ide_{mapped_mmio,iomio}_dma() to ide_setup_dma().

* Rename 'dma_base' argument to 'base' in ide_setup_dma().

* Remove stale ide_setup_dma() comment.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-dma.c |   30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)
drivers/ide/ide-dma.c +10 -20
Index: b/drivers/ide/ide-dma.c
===================================================================
@@ -956,12 +956,6 @@ static int ide_mapped_mmio_dma(ide_hwif_
 {
 	printk(KERN_INFO "    %s: MMIO-DMA ", hwif->name);
 
- 	hwif->dma_base = base;
-
-	if(hwif->mate)
-		hwif->dma_master = (hwif->channel) ? hwif->mate->dma_base : base;
-	else
-		hwif->dma_master = base;
 	return 0;
 }
 

@@ -975,8 +969,6 @@ static int ide_iomio_dma(ide_hwif_t *hwi
 		return 1;
 	}
 
-	hwif->dma_base = base;
-
 	if (hwif->cds->extra) {
 		hwif->extra_base = base + (hwif->channel ? 8 : 16);
 

@@ -991,10 +983,6 @@ static int ide_iomio_dma(ide_hwif_t *hwi
 		}
 	}
 
-	if(hwif->mate)
-		hwif->dma_master = (hwif->channel) ? hwif->mate->dma_base:base;
-	else
-		hwif->dma_master = base;
 	return 0;
 }
 

@@ -1006,18 +994,20 @@ static int ide_dma_iobase(ide_hwif_t *hw
 	return ide_iomio_dma(hwif, base, ports);
 }
 
-/*
- * This can be called for a dynamically installed interface. Don't __init it
- */
-void ide_setup_dma (ide_hwif_t *hwif, unsigned long dma_base, unsigned int num_ports)
+void ide_setup_dma(ide_hwif_t *hwif, unsigned long base, unsigned num_ports)
 {
-	if (ide_dma_iobase(hwif, dma_base, num_ports))
+	if (ide_allocate_dma_engine(hwif))
 		return;
 
-	if (ide_allocate_dma_engine(hwif)) {
-		ide_release_dma(hwif);
+	if (ide_dma_iobase(hwif, base, num_ports))
 		return;
-	}
+
+	hwif->dma_base = base;
+
+	if (hwif->mate)
+		hwif->dma_master = hwif->channel ? hwif->mate->dma_base : base;
+	else
+		hwif->dma_master = base;
 
 	if (!(hwif->dma_command))
 		hwif->dma_command	= hwif->dma_base;
#1 lore raw

Hello.

Quoted 3 lines · Bartlomiej Zolnierkiewicz

* Set hwif->dma_base only if allocation of extra ports succeeds.

* Allocate hwif->dmatable_cpu before reserving I/O ports.

Looks like a bit of teardown code was missed with that change...

Quoted 6 lines

While at it:

* Move setting of hwif->dma_{base,master} from ide_{mapped_mmio,iomio}_dma() to ide_setup_dma().

* Rename 'dma_base' argument to 'base' in ide_setup_dma().

I'm not sure what that achieves...

Quoted 11 lines

* Remove stale ide_setup_dma() comment.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
drivers/ide/ide-dma.c +0 -3
@@ -1006,18 +994,20 @@ static int ide_dma_iobase(ide_hwif_t *hw
 	return ide_iomio_dma(hwif, base, ports);
 }
 
-/*
- * This can be called for a dynamically installed interface. Don't __init it
- */

drivers/ide/ide-dma.c static int ide_dma_iobase(ide_hwif_t *hw · line 1002

Quoted 10 lines
drivers/ide/ide-dma.c +3 -4
-void ide_setup_dma (ide_hwif_t *hwif, unsigned long dma_base, unsigned int num_ports)
+void ide_setup_dma(ide_hwif_t *hwif, unsigned long base, unsigned num_ports)
 {
-	if (ide_dma_iobase(hwif, dma_base, num_ports))
+	if (ide_allocate_dma_engine(hwif))
 		return;
 
-	if (ide_allocate_dma_engine(hwif)) {
-		ide_release_dma(hwif);
+	if (ide_dma_iobase(hwif, base, num_ports))

Why no ide_release_dma_engine() call here?

MBR, Sergei

author
#2 lore raw
Quoted 9 lines · Sergei Shtylyov

Hello.

Quoted 3 lines · Bartlomiej Zolnierkiewicz

* Set hwif->dma_base only if allocation of extra ports succeeds.

* Allocate hwif->dmatable_cpu before reserving I/O ports.

Looks like a bit of teardown code was missed with that change...

Thanks for catching this!

Actually it turns out that the whole change was based on a thinko. :)

Quoted 8 lines
Quoted 6 lines

While at it:

* Move setting of hwif->dma_{base,master} from ide_{mapped_mmio,iomio}_dma() to ide_setup_dma().

* Rename 'dma_base' argument to 'base' in ide_setup_dma().

I'm not sure what that achieves...

Makes the code obey 80-columns limit and increases its readability :)

Revised patch follows...

[PATCH] ide: PCI BMDMA initialization fixes (take 2)

* Set hwif->dma_base only if allocation of extra ports succeeds.

While at it:

* Move setting of hwif->dma_{base,master} from ide_{mapped_mmio,iomio}_dma() to ide_setup_dma().

* Rename 'dma_base' argument to 'base' in ide_setup_dma() (to make the code obey 80-columns limit and increase its readability).

* Remove stale ide_setup_dma() comment.

v2:
* Change to allocate hwif->dmatable_cpu before reserving I/O ports missed teardown code (spotted by Sergei). On the second thought this change is actually unnecessary so revert it in v2.

* Make ide_release_dma_engine() void and remove needless comment.

Cc Sergei Shtylyov <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-dma.c |   32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)
drivers/ide/ide-dma.c +10 -22
Index: b/drivers/ide/ide-dma.c
===================================================================
@@ -901,10 +901,7 @@ void ide_dma_timeout (ide_drive_t *drive
 
 EXPORT_SYMBOL(ide_dma_timeout);
 
-/*
- * Needed for allowing full modular support of ide-driver
- */
-static int ide_release_dma_engine(ide_hwif_t *hwif)
+static void ide_release_dma_engine(ide_hwif_t *hwif)
 {
 	if (hwif->dmatable_cpu) {
 		pci_free_consistent(hwif->pci_dev,

@@ -913,7 +910,6 @@ static int ide_release_dma_engine(ide_hw
 				    hwif->dmatable_dma);
 		hwif->dmatable_cpu = NULL;
 	}
-	return 1;
 }
 
 static int ide_release_iomio_dma(ide_hwif_t *hwif)

@@ -956,12 +952,6 @@ static int ide_mapped_mmio_dma(ide_hwif_
 {
 	printk(KERN_INFO "    %s: MMIO-DMA ", hwif->name);
 
- 	hwif->dma_base = base;
-
-	if(hwif->mate)
-		hwif->dma_master = (hwif->channel) ? hwif->mate->dma_base : base;
-	else
-		hwif->dma_master = base;
 	return 0;
 }
 

@@ -975,8 +965,6 @@ static int ide_iomio_dma(ide_hwif_t *hwi
 		return 1;
 	}
 
-	hwif->dma_base = base;
-
 	if (hwif->cds->extra) {
 		hwif->extra_base = base + (hwif->channel ? 8 : 16);
 

@@ -991,10 +979,6 @@ static int ide_iomio_dma(ide_hwif_t *hwi
 		}
 	}
 
-	if(hwif->mate)
-		hwif->dma_master = (hwif->channel) ? hwif->mate->dma_base:base;
-	else
-		hwif->dma_master = base;
 	return 0;
 }
 

@@ -1006,12 +990,9 @@ static int ide_dma_iobase(ide_hwif_t *hw
 	return ide_iomio_dma(hwif, base, ports);
 }
 
-/*
- * This can be called for a dynamically installed interface. Don't __init it
- */
-void ide_setup_dma (ide_hwif_t *hwif, unsigned long dma_base, unsigned int num_ports)
+void ide_setup_dma(ide_hwif_t *hwif, unsigned long base, unsigned num_ports)
 {
-	if (ide_dma_iobase(hwif, dma_base, num_ports))
+	if (ide_dma_iobase(hwif, base, num_ports))
 		return;
 
 	if (ide_allocate_dma_engine(hwif)) {

@@ -1019,6 +1000,13 @@ void ide_setup_dma (ide_hwif_t *hwif, un
 		return;
 	}
 
+	hwif->dma_base = base;
+
+	if (hwif->mate)
+		hwif->dma_master = hwif->channel ? hwif->mate->dma_base : base;
+	else
+		hwif->dma_master = base;
+
 	if (!(hwif->dma_command))
 		hwif->dma_command	= hwif->dma_base;
 	if (!(hwif->dma_vendor1))
Acked-by
#3 lore raw

Hello.

Quoted 14 lines · Bartlomiej Zolnierkiewicz

[PATCH] ide: PCI BMDMA initialization fixes (take 2)

* Set hwif->dma_base only if allocation of extra ports succeeds.

While at it:

* Move setting of hwif->dma_{base,master} from ide_{mapped_mmio,iomio}_dma() to ide_setup_dma().

* Rename 'dma_base' argument to 'base' in ide_setup_dma() (to make the code obey 80-columns limit and increase its readability).

* Remove stale ide_setup_dma() comment.

Quoted 8 lines

v2:
* Change to allocate hwif->dmatable_cpu before reserving I/O ports missed teardown code (spotted by Sergei). On the second thought this change is actually unnecessary so revert it in v2.

* Make ide_release_dma_engine() void and remove needless comment.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
Acked-by Sergei Shtylyov <[email protected]>

MBR, Sergei