Skip to contentSkip to conversation

ide: remove ->dma_master field from ide_hwif_t

PATCH NAK Bartlomiej Zolnierkiewicz 3 participants 12 messages Oct 1, 2007 – Oct 25, 2007 libata

lore
Share & tools
lore

Bartlomiej Zolnierkiewicz

author
#0 lore raw

PATCH 12/15 ide: remove ->dma_master field from ide_hwif_t

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

* Remove no longer needed ->dma_master field from ide_hwif_t.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-dma.c          |    7 -------
 drivers/ide/ide.c              |    2 --
 drivers/ide/pci/cmd64x.c       |    8 +++++---
 drivers/ide/pci/hpt366.c       |   21 +++++++++++----------
 drivers/ide/pci/pdc202xx_old.c |   12 ++++++------
 include/linux/ide.h            |    1 -
 6 files changed, 22 insertions(+), 29 deletions(-)
drivers/ide/ide-dma.c +0 -7
Index: b/drivers/ide/ide-dma.c
===================================================================
@@ -1004,11 +1004,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
 
 	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))

@@ -1050,8 +1045,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
 		       hwif->drives[1].name, (dma_stat & 0x40) ? "DMA" : "pio");
 	}
 	printk("\n");
-
-	BUG_ON(!hwif->dma_master);
 }
 
 EXPORT_SYMBOL_GPL(ide_setup_dma);
drivers/ide/ide.c +0 -2
Index: b/drivers/ide/ide.c
===================================================================
@@ -470,7 +470,6 @@ static void ide_hwif_restore(ide_hwif_t 
 #endif
 
 	hwif->dma_base			= tmp_hwif->dma_base;
-	hwif->dma_master		= tmp_hwif->dma_master;
 	hwif->dma_command		= tmp_hwif->dma_command;
 	hwif->dma_vendor1		= tmp_hwif->dma_vendor1;
 	hwif->dma_status		= tmp_hwif->dma_status;

@@ -604,7 +603,6 @@ void ide_unregister(unsigned int index)
 		(void) ide_release_dma(hwif);
 
 		hwif->dma_base = 0;
-		hwif->dma_master = 0;
 		hwif->dma_command = 0;
 		hwif->dma_vendor1 = 0;
 		hwif->dma_status = 0;
drivers/ide/pci/cmd64x.c +5 -3
Index: b/drivers/ide/pci/cmd64x.c
===================================================================
@@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
 static int cmd648_ide_dma_end (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long addr	= pci_resource_start(hwif->pci_dev, 4) + 0x01;
 	int err			= __ide_dma_end(drive);
 	u8  irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
-	u8  mrdmode		= inb(hwif->dma_master + 0x01);
+	u8  mrdmode		= inb(addr);
 
 	/* clear the interrupt bit */
-	outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+	outb(mrdmode | irq_mask, addr);
 
 	return err;
 }

@@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
 static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long addr	= pci_resource_start(hwif->pci_dev, 4) + 0x01;
 	u8 irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
 	u8 dma_stat		= inb(hwif->dma_status);
-	u8 mrdmode		= inb(hwif->dma_master + 0x01);
+	u8 mrdmode		= inb(addr);
 
 #ifdef DEBUG
 	printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
drivers/ide/pci/hpt366.c +11 -10
Index: b/drivers/ide/pci/hpt366.c
===================================================================
@@ -827,32 +827,33 @@ static int hpt374_ide_dma_end(ide_drive_
 
 static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
 {
-	u8 scr2 = inb(hwif->dma_master + 0x7b);
+	unsigned long addr = pci_resource_start(hwif->pci_dev, 4);
+	u8 scr2 = inb(addr + 0x7b);
 
 	if ((scr2 & 0x7f) == mode)
 		return;
 
 	/* Tristate the bus */
-	outb(0x80, hwif->dma_master + 0x73);
-	outb(0x80, hwif->dma_master + 0x77);
+	outb(0x80, addr + 0x73);
+	outb(0x80, addr + 0x77);
 
 	/* Switch clock and reset channels */
-	outb(mode, hwif->dma_master + 0x7b);
-	outb(0xc0, hwif->dma_master + 0x79);
+	outb(mode, addr + 0x7b);
+	outb(0xc0, addr + 0x79);
 
 	/*
 	 * Reset the state machines.
 	 * NOTE: avoid accidentally enabling the disabled channels.
 	 */
-	outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
-	outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
+	outb(inb(addr + 0x70) | 0x32, addr + 0x70);
+	outb(inb(addr + 0x74) | 0x32, addr + 0x74);
 
 	/* Complete reset */
-	outb(0x00, hwif->dma_master + 0x79);
+	outb(0x00, addr + 0x79);
 
 	/* Reconnect channels to bus */
-	outb(0x00, hwif->dma_master + 0x73);
-	outb(0x00, hwif->dma_master + 0x77);
+	outb(0x00, addr + 0x73);
+	outb(0x00, addr + 0x77);
 }
 
 /**
drivers/ide/pci/pdc202xx_old.c +6 -6
Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
@@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide
  */
 static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;
+	unsigned long clock_reg = pci_resource_start(hwif->pci_dev, 4) + 0x11;
 	u8 clock = inb(clock_reg);
 
 	outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
 
 static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;
+	unsigned long clock_reg = pci_resource_start(hwif->pci_dev, 4) + 0x11;
 	u8 clock = inb(clock_reg);
 
 	outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		struct request *rq	= HWGROUP(drive)->rq;
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16   = hwif->dma_master;
+		unsigned long high_16	= pci_resource_start(hwif->pci_dev, 4);
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u32 word_count	= 0;
 		u8 clock = inb(high_16 + 0x11);

@@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 {
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16	= hwif->dma_master;
+		unsigned long high_16	= pci_resource_start(hwif->pci_dev, 4);
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u8 clock		= 0;
 

@@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= pci_resource_start(hwif->pci_dev, 4);
 	u8 dma_stat		= inb(hwif->dma_status);
 	u8 sc1d			= inb(high_16 + 0x001d);
 

@@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
 
 static void pdc202xx_reset_host (ide_hwif_t *hwif)
 {
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= pci_resource_start(hwif->pci_dev, 4);
 	u8 udma_speed_flag	= inb(high_16 | 0x001f);
 
 	outb(udma_speed_flag | 0x10, high_16 | 0x001f);
include/linux/ide.h +0 -1
Index: b/include/linux/ide.h
===================================================================
@@ -764,7 +764,6 @@ typedef struct hwif_s {
 	int		rqsize;		/* max sectors per request */
 	int		irq;		/* our irq number */
 
-	unsigned long	dma_master;	/* reference base addr dmabase */
 	unsigned long	dma_base;	/* base addr for dma ports */
 	unsigned long	dma_command;	/* dma command register */
 	unsigned long	dma_vendor1;	/* dma vendor 1 register */
#1 lore raw
Quoted 2 lines · Bartlomiej Zolnierkiewicz

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

Before using pci_resource_start(), the code should check pci_resource_len() to ensure it is the appropriate size.

It would be very wise to ensure that pci_resource_flags()&IORESOURCE_IO is true, too.

Jeff

author
#2 lore raw
Quoted 9 lines · Jeff Garzik
Quoted 2 lines · Bartlomiej Zolnierkiewicz

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

Before using pci_resource_start(), the code should check pci_resource_len() to ensure it is the appropriate size.

It would be very wise to ensure that pci_resource_flags()&IORESOURCE_IO is true, too.

Sure but since the old code hasn't been doing these checks (old code just did pci_resource_start() -> hwif->dma_base -> hwif->dma_master) this is a separate issue from what the patch tries to achieve.

Bart

#3 lore raw
Quoted 11 lines · Bartlomiej Zolnierkiewicz
Quoted 6 lines · Jeff Garzik
Quoted 2 lines · Bartlomiej Zolnierkiewicz

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

Before using pci_resource_start(), the code should check pci_resource_len() to ensure it is the appropriate size.

It would be very wise to ensure that pci_resource_flags()&IORESOURCE_IO is true, too.

Sure but since the old code hasn't been doing these checks (old code just did pci_resource_start() -> hwif->dma_base -> hwif->dma_master) this is a separate issue from what the patch tries to achieve.

Nonetheless this is duplicating an existing bug $N times... not ideal :)

Jeff

author
#4 lore raw
Quoted 8 lines · Jeff Garzik
Quoted 7 lines · Bartlomiej Zolnierkiewicz
Quoted 6 lines · Jeff Garzik
Quoted 2 lines · Bartlomiej Zolnierkiewicz

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

Before using pci_resource_start(), the code should check pci_resource_len() to ensure it is the appropriate size.

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

It would be very wise to ensure that pci_resource_flags()&IORESOURCE_IO is true, too.

Sure but since the old code hasn't been doing these checks (old code just did pci_resource_start() -> hwif->dma_base -> hwif->dma_master) this is a separate issue from what the patch tries to achieve.

Nonetheless this is duplicating an existing bug $N times... not ideal :)

The existing bug is that the methods converted to use pci_resource_start() may be set in hwif instance and used even if the BAR4 is invalid. Therefore the above patch is correct and it isn't duplicating the existing bug. :)

However I agree that all these pci_resource_start()s could be a bit confusing so here is 'take 2'.

[PATCH] ide: remove ->dma_master field from ide_hwif_t (take 2)

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

* Remove no longer needed ->dma_master field from ide_hwif_t.

v2:
* Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of pci_resource_start(hwif->pci_dev, 4).

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-dma.c          |    7 -------
 drivers/ide/ide.c              |    2 --
 drivers/ide/pci/cmd64x.c       |    8 +++++---
 drivers/ide/pci/hpt366.c       |   21 +++++++++++----------
 drivers/ide/pci/pdc202xx_old.c |   12 ++++++------
 include/linux/ide.h            |    1 -
 6 files changed, 22 insertions(+), 29 deletions(-)
drivers/ide/ide-dma.c +0 -7
Index: b/drivers/ide/ide-dma.c
===================================================================
@@ -1006,11 +1006,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
 
 	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))

@@ -1052,8 +1047,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
 		       hwif->drives[1].name, (dma_stat & 0x40) ? "DMA" : "pio");
 	}
 	printk("\n");
-
-	BUG_ON(!hwif->dma_master);
 }
 
 EXPORT_SYMBOL_GPL(ide_setup_dma);
drivers/ide/ide.c +0 -2
Index: b/drivers/ide/ide.c
===================================================================
@@ -470,7 +470,6 @@ static void ide_hwif_restore(ide_hwif_t 
 #endif
 
 	hwif->dma_base			= tmp_hwif->dma_base;
-	hwif->dma_master		= tmp_hwif->dma_master;
 	hwif->dma_command		= tmp_hwif->dma_command;
 	hwif->dma_vendor1		= tmp_hwif->dma_vendor1;
 	hwif->dma_status		= tmp_hwif->dma_status;

@@ -604,7 +603,6 @@ void ide_unregister(unsigned int index)
 		(void) ide_release_dma(hwif);
 
 		hwif->dma_base = 0;
-		hwif->dma_master = 0;
 		hwif->dma_command = 0;
 		hwif->dma_vendor1 = 0;
 		hwif->dma_status = 0;
drivers/ide/pci/cmd64x.c +5 -3
Index: b/drivers/ide/pci/cmd64x.c
===================================================================
@@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
 static int cmd648_ide_dma_end (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long addr	= hwif->dma_base - (hwif->channel * 8) + 0x01;
 	int err			= __ide_dma_end(drive);
 	u8  irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
-	u8  mrdmode		= inb(hwif->dma_master + 0x01);
+	u8  mrdmode		= inb(addr);
 
 	/* clear the interrupt bit */
-	outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+	outb(mrdmode | irq_mask, addr);
 
 	return err;
 }

@@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
 static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long addr	= hwif->dma_base - (hwif->channel * 8) + 0x01;
 	u8 irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
 	u8 dma_stat		= inb(hwif->dma_status);
-	u8 mrdmode		= inb(hwif->dma_master + 0x01);
+	u8 mrdmode		= inb(addr);
 
 #ifdef DEBUG
 	printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
drivers/ide/pci/hpt366.c +11 -10
Index: b/drivers/ide/pci/hpt366.c
===================================================================
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
 
 static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
 {
-	u8 scr2 = inb(hwif->dma_master + 0x7b);
+	unsigned long addr = hwif->dma_base - (hwif->channel * 8);
+	u8 scr2 = inb(addr + 0x7b);
 
 	if ((scr2 & 0x7f) == mode)
 		return;
 
 	/* Tristate the bus */
-	outb(0x80, hwif->dma_master + 0x73);
-	outb(0x80, hwif->dma_master + 0x77);
+	outb(0x80, addr + 0x73);
+	outb(0x80, addr + 0x77);
 
 	/* Switch clock and reset channels */
-	outb(mode, hwif->dma_master + 0x7b);
-	outb(0xc0, hwif->dma_master + 0x79);
+	outb(mode, addr + 0x7b);
+	outb(0xc0, addr + 0x79);
 
 	/*
 	 * Reset the state machines.
 	 * NOTE: avoid accidentally enabling the disabled channels.
 	 */
-	outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
-	outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
+	outb(inb(addr + 0x70) | 0x32, addr + 0x70);
+	outb(inb(addr + 0x74) | 0x32, addr + 0x74);
 
 	/* Complete reset */
-	outb(0x00, hwif->dma_master + 0x79);
+	outb(0x00, addr + 0x79);
 
 	/* Reconnect channels to bus */
-	outb(0x00, hwif->dma_master + 0x73);
-	outb(0x00, hwif->dma_master + 0x77);
+	outb(0x00, addr + 0x73);
+	outb(0x00, addr + 0x77);
 }
 
 /**
drivers/ide/pci/pdc202xx_old.c +6 -6
Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
@@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide
  */
 static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;
+	unsigned long clock_reg = hwif->dma_base - (hwif->channel * 8) + 0x11;
 	u8 clock = inb(clock_reg);
 
 	outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
 
 static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;
+	unsigned long clock_reg = hwif->dma_base - (hwif->channel * 8) + 0x11;
 	u8 clock = inb(clock_reg);
 
 	outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		struct request *rq	= HWGROUP(drive)->rq;
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16   = hwif->dma_master;
+		unsigned long high_16	= hwif->dma_base - (hwif->channel * 8);
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u32 word_count	= 0;
 		u8 clock = inb(high_16 + 0x11);

@@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 {
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16	= hwif->dma_master;
+		unsigned long high_16	= hwif->dma_base - (hwif->channel * 8);
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u8 clock		= 0;
 

@@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= hwif->dma_base - (hwif->channel * 8);
 	u8 dma_stat		= inb(hwif->dma_status);
 	u8 sc1d			= inb(high_16 + 0x001d);
 

@@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
 
 static void pdc202xx_reset_host (ide_hwif_t *hwif)
 {
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= hwif->dma_base - (hwif->channel * 8);
 	u8 udma_speed_flag	= inb(high_16 | 0x001f);
 
 	outb(udma_speed_flag | 0x10, high_16 | 0x001f);
include/linux/ide.h +0 -1
Index: b/include/linux/ide.h
===================================================================
@@ -764,7 +764,6 @@ typedef struct hwif_s {
 	int		rqsize;		/* max sectors per request */
 	int		irq;		/* our irq number */
 
-	unsigned long	dma_master;	/* reference base addr dmabase */
 	unsigned long	dma_base;	/* base addr for dma ports */
 	unsigned long	dma_command;	/* dma command register */
 	unsigned long	dma_vendor1;	/* dma vendor 1 register */
#5 lore raw

Hello.

Quoted 11 lines · Bartlomiej Zolnierkiewicz
Quoted 10 lines
Quoted 10 lines
Quoted 6 lines
Quoted 2 lines

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

Before using pci_resource_start(), the code should check pci_resource_len() to ensure it is the appropriate size.

It would be very wise to ensure that pci_resource_flags()&IORESOURCE_IO is true, too.

Sure but since the old code hasn't been doing these checks (old code just did pci_resource_start() -> hwif->dma_base -> hwif->dma_master) this is a separate issue from what the patch tries to achieve.

Quoted 8 lines
Quoted 1 line

Nonetheless this is duplicating an existing bug $N times... not ideal :)

The existing bug is that the methods converted to use pci_resource_start() may be set in hwif instance and used even if the BAR4 is invalid. Therefore the above patch is correct and it isn't duplicating the existing bug. :)

However I agree that all these pci_resource_start()s could be a bit confusing so here is 'take 2'.

Yeah, they are. Actually, I was thinking about such patch ~1.5 years ago
*but* using a different approach -- namely, hwif->extra_base.

Quoted 10 lines

[PATCH] ide: remove ->dma_master field from ide_hwif_t (take 2)

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

* Remove no longer needed ->dma_master field from ide_hwif_t.

v2:
* Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of pci_resource_start(hwif->pci_dev, 4).

s/readable/clumsy/? ;-)

Quoted 8 lines
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

Index: b/drivers/ide/ide-dma.c
===================================================================

drivers/ide/ide-dma.c +0 -0
@@ -1006,11 +1006,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns

drivers/ide/ide-dma.c void ide_setup_dma(ide_hwif_t *hwif, uns · line 1011

Quoted 10 lines
drivers/ide/ide-dma.c +0 -5
 	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))

Ugh, these extra parens... :-)

[...]

drivers/ide/pci/cmd64x.c static void cmd64x_set_dma_mode(ide_driv · line 336

Quoted 9 lines

Index: b/drivers/ide/pci/cmd64x.c
===================================================================

drivers/ide/pci/cmd64x.c +1 -0
@@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
 static int cmd648_ide_dma_end (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long addr	= hwif->dma_base - (hwif->channel * 8) + 0x01;

Dunno... pci_resource_start() looked better to my taste. Except I'd
probably called the variable 'base' and wouldn't + 0x01 right away.

Quoted 13 lines
drivers/ide/pci/cmd64x.c +2 -2
 	int err			= __ide_dma_end(drive);
 	u8  irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
-	u8  mrdmode		= inb(hwif->dma_master + 0x01);
+	u8  mrdmode		= inb(addr);
 
 	/* clear the interrupt bit */
-	outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+	outb(mrdmode | irq_mask, addr);
 
 	return err;
 }

drivers/ide/pci/cmd64x.c static int cmd64x_ide_dma_end (ide_drive · line 368

Quoted 4 lines
drivers/ide/pci/cmd64x.c +1 -0
@@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
 static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long addr	= hwif->dma_base - (hwif->channel * 8) + 0x01;

Same comment here...

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_

Quoted 13 lines
drivers/ide/pci/cmd64x.c +1 -1
 	u8 irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
 	u8 dma_stat		= inb(hwif->dma_status);
-	u8 mrdmode		= inb(hwif->dma_master + 0x01);
+	u8 mrdmode		= inb(addr);
 
 #ifdef DEBUG
 	printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",

Index: b/drivers/ide/pci/hpt366.c
===================================================================

drivers/ide/pci/hpt366.c +0 -0
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_

This just asks to be:

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_ · line 847

Quoted 2 lines
drivers/ide/pci/hpt366.c +0 -0
 static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
 {

	unsigned long base = hwif->extra_base;
	u8 scr2 = inb(base + 0x5b);

if ((scr2 & 0x7f) == mode)
return;

/* Tristate the bus */

	outb(0x80, base + 0x53);
	outb(0x80, base + 0x57);

/* Switch clock and reset channels */

	outb(mode, base + 0x5b);
	outb(0xc0, base + 0x59);

/*
* Reset the state machines.
* NOTE: avoid accidentally enabling the disabled channels. */

	outb(inb(base + 0x50) | 0x32, base + 0x50);
	outb(inb(base + 0x54) | 0x32, base + 0x54);

/* Complete reset */

	outb(0x00, base + 0x59);

	/* Reconnect channels to bus */
	outb(0x00, base + 0x53);
	outb(0x00, base + 0x57);
}

drivers/ide/pci/pdc202xx_old.c static u8 pdc202xx_old_cable_detect (ide

Quoted 5 lines

Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================

drivers/ide/pci/pdc202xx_old.c +0 -0
@@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide

And this asks to be:

drivers/ide/pci/pdc202xx_old.c static u8 pdc202xx_old_cable_detect (ide · line 165

Quoted 4 lines
drivers/ide/pci/pdc202xx_old.c +0 -1
  */
 static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;

+ unsigned long clock_reg = hwif->extra_base + 0x01;

drivers/ide/pci/pdc202xx_old.c static void pdc_old_enable_66MHz_clock(i · line 173

Quoted 8 lines
drivers/ide/pci/pdc202xx_old.c +0 -1
 	u8 clock = inb(clock_reg);
 
 	outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
 
 static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;

+ unsigned long clock_reg = hwif->extra_base + 0x01;

drivers/ide/pci/pdc202xx_old.c static void pdc202xx_old_ide_dma_start(i · line 196

Quoted 8 lines
drivers/ide/pci/pdc202xx_old.c +0 -1
 	u8 clock = inb(clock_reg);
 
 	outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		struct request *rq	= HWGROUP(drive)->rq;
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16   = hwif->dma_master;

+ unsigned long high_16 = hwif->extra_base;

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_old_ide_dma_end(ide_ · line 215

Quoted 8 lines
drivers/ide/pci/pdc202xx_old.c +0 -1
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u32 word_count	= 0;
 		u8 clock = inb(high_16 + 0x11);

@@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 {
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16	= hwif->dma_master;

+ unsigned long high_16 = hwif->extra_base;

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_old_ide_dma_end(ide_ · line 231

Quoted 8 lines
drivers/ide/pci/pdc202xx_old.c +0 -1
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u8 clock		= 0;
 

@@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
-	unsigned long high_16	= hwif->dma_master;

+ unsigned long high_16 = hwif->extra_base;

drivers/ide/pci/pdc202xx_old.c static void pdc202xx_dma_timeout(ide_dri · line 274

Quoted 9 lines
drivers/ide/pci/pdc202xx_old.c +1 -1
 	u8 dma_stat		= inb(hwif->dma_status);
 	u8 sc1d			= inb(high_16 + 0x001d);
 

@@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
 
 static void pdc202xx_reset_host (ide_hwif_t *hwif)
 {
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= hwif->dma_base - (hwif->channel * 8);

	unsigned long high_16	= hwif->extra_base;

drivers/ide/pci/pdc202xx_old.c static void pdc202xx_dma_timeout(ide_dri · line 275

Quoted 1 line
drivers/ide/pci/pdc202xx_old.c +0 -0
 	u8 udma_speed_flag	= inb(high_16 | 0x001f);

MBR, Sergei

author
#6 lore raw
Quoted 23 lines · Sergei Shtylyov

Hello.

Quoted 18 lines · Bartlomiej Zolnierkiewicz
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

Before using pci_resource_start(), the code should check pci_resource_len() to ensure it is the appropriate size.

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

It would be very wise to ensure that pci_resource_flags()&IORESOURCE_IO is true, too.

Sure but since the old code hasn't been doing these checks (old code just did pci_resource_start() -> hwif->dma_base -> hwif->dma_master) this is a separate issue from what the patch tries to achieve.

Nonetheless this is duplicating an existing bug $N times... not ideal :)

The existing bug is that the methods converted to use pci_resource_start() may be set in hwif instance and used even if the BAR4 is invalid. Therefore the above patch is correct and it isn't duplicating the existing bug. :)

Quoted 5 lines
Quoted 2 lines

However I agree that all these pci_resource_start()s could be a bit confusing so here is 'take 2'.

Yeah, they are. Actually, I was thinking about such patch ~1.5 years ago
*but* using a different approach -- namely, hwif->extra_base.

good idea!

Quoted 13 lines

[...]

drivers/ide/pci/cmd64x.c static void cmd64x_set_dma_mode(ide_driv · line 336

Quoted 9 lines

Index: b/drivers/ide/pci/cmd64x.c
===================================================================

drivers/ide/pci/cmd64x.c +1 -0
@@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
 static int cmd648_ide_dma_end (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long addr	= hwif->dma_base - (hwif->channel * 8) + 0x01;

Dunno... pci_resource_start() looked better to my taste. Except I'd
probably called the variable 'base' and wouldn't + 0x01 right away.

fixed

Quoted 10 lines
Quoted 10 lines
drivers/ide/pci/cmd64x.c +2 -2
 	int err			= __ide_dma_end(drive);
 	u8  irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
-	u8  mrdmode		= inb(hwif->dma_master + 0x01);
+	u8  mrdmode		= inb(addr);
 
 	/* clear the interrupt bit */
-	outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+	outb(mrdmode | irq_mask, addr);
Quoted 9 lines

drivers/ide/pci/cmd64x.c static int cmd64x_ide_dma_end (ide_drive · line 368

Quoted 7 lines
drivers/ide/pci/cmd64x.c +1 -0
 	return err;
 }

@@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
 static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long addr	= hwif->dma_base - (hwif->channel * 8) + 0x01;

Same comment here...

ditto

Quoted 39 lines

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_

Quoted 13 lines
drivers/ide/pci/cmd64x.c +1 -1
 	u8 irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
 	u8 dma_stat		= inb(hwif->dma_status);
-	u8 mrdmode		= inb(hwif->dma_master + 0x01);
+	u8 mrdmode		= inb(addr);
 
 #ifdef DEBUG
 	printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",

Index: b/drivers/ide/pci/hpt366.c
===================================================================

drivers/ide/pci/hpt366.c +0 -0
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_

This just asks to be:

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_ · line 846

Quoted 2 lines
drivers/ide/pci/hpt366.c +0 -0
 static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
 {

	unsigned long base = hwif->extra_base;
	u8 scr2 = inb(base + 0x5b);

if ((scr2 & 0x7f) == mode)
return;

/* Tristate the bus */

	outb(0x80, base + 0x53);
	outb(0x80, base + 0x57);

/* Switch clock and reset channels */

	outb(mode, base + 0x5b);
	outb(0xc0, base + 0x59);

/*
* Reset the state machines.
* NOTE: avoid accidentally enabling the disabled channels. */

	outb(inb(base + 0x50) | 0x32, base + 0x50);
	outb(inb(base + 0x54) | 0x32, base + 0x54);
Quoted 7 lines

/* Complete reset */

	outb(0x00, base + 0x59);

	/* Reconnect channels to bus */
	outb(0x00, base + 0x53);
	outb(0x00, base + 0x57);
}

ditto

Review

Quoted 13 lines

drivers/ide/pci/pdc202xx_old.c static u8 pdc202xx_old_cable_detect (ide

Quoted 5 lines

Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================

drivers/ide/pci/pdc202xx_old.c +0 -0
@@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide

And this asks to be:

Quoted 4 lines
drivers/ide/pci/pdc202xx_old.c +0 -1
  */
 static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;
(quoted diff) +1 -0
+	unsigned long clock_reg = hwif->extra_base + 0x01;

ditto

Review

Quoted 10 lines
Quoted 8 lines
drivers/ide/pci/pdc202xx_old.c +0 -1
 	u8 clock = inb(clock_reg);
 
 	outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
 
 static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;
(quoted diff) +1 -0
+	unsigned long clock_reg = hwif->extra_base + 0x01;

ditto

Review

Quoted 10 lines
Quoted 8 lines
drivers/ide/pci/pdc202xx_old.c +0 -1
 	u8 clock = inb(clock_reg);
 
 	outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		struct request *rq	= HWGROUP(drive)->rq;
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16   = hwif->dma_master;
(quoted diff) +1 -0
+		unsigned long high_16	= hwif->extra_base;

fixed but with s/hwif->extra_base/hwif->extra_base - 16/

Review

Quoted 10 lines
Quoted 8 lines
drivers/ide/pci/pdc202xx_old.c +0 -1
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u32 word_count	= 0;
 		u8 clock = inb(high_16 + 0x11);

@@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 {
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16	= hwif->dma_master;
(quoted diff) +1 -0
+		unsigned long high_16	= hwif->extra_base;

ditto

Review

Quoted 10 lines
Quoted 8 lines
drivers/ide/pci/pdc202xx_old.c +0 -1
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u8 clock		= 0;
 

@@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
-	unsigned long high_16	= hwif->dma_master;
(quoted diff) +1 -0
+	unsigned long high_16	= hwif->extra_base;

ditto

Quoted 11 lines

drivers/ide/pci/pdc202xx_old.c static void pdc202xx_dma_timeout(ide_dri · line 274

Quoted 9 lines
drivers/ide/pci/pdc202xx_old.c +1 -1
 	u8 dma_stat		= inb(hwif->dma_status);
 	u8 sc1d			= inb(high_16 + 0x001d);
 

@@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
 
 static void pdc202xx_reset_host (ide_hwif_t *hwif)
 {
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= hwif->dma_base - (hwif->channel * 8);

	unsigned long high_16	= hwif->extra_base;

ditto

thanks for all hints, new version of the patch below...

[PATCH] ide: remove ->dma_master field from ide_hwif_t (take 3)

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

* Remove no longer needed ->dma_master field from ide_hwif_t.

v2:
* Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of pci_resource_start(hwif->pci_dev, 4).

v3:
* Use hwif->extra_base in hpt366/pdc20xx_old + some cosmetic fixups over v2 (suggested by Sergei).

Cc Sergei Shtylyov <[email protected]>
Cc Jeff Garzik <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-dma.c          |    7 -------
 drivers/ide/ide.c              |    2 --
 drivers/ide/pci/cmd64x.c       |    8 +++++---
 drivers/ide/pci/hpt366.c       |   21 +++++++++++----------
 drivers/ide/pci/pdc202xx_old.c |   12 ++++++------
 include/linux/ide.h            |    1 -
 6 files changed, 22 insertions(+), 29 deletions(-)
drivers/ide/ide-dma.c +0 -7
Index: b/drivers/ide/ide-dma.c
===================================================================
@@ -1002,11 +1002,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
 
 	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))

@@ -1048,8 +1043,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
 		       hwif->drives[1].name, (dma_stat & 0x40) ? "DMA" : "pio");
 	}
 	printk("\n");
-
-	BUG_ON(!hwif->dma_master);
 }
 
 EXPORT_SYMBOL_GPL(ide_setup_dma);
drivers/ide/ide.c +0 -2
Index: b/drivers/ide/ide.c
===================================================================
@@ -468,7 +468,6 @@ static void ide_hwif_restore(ide_hwif_t 
 #endif
 
 	hwif->dma_base			= tmp_hwif->dma_base;
-	hwif->dma_master		= tmp_hwif->dma_master;
 	hwif->dma_command		= tmp_hwif->dma_command;
 	hwif->dma_vendor1		= tmp_hwif->dma_vendor1;
 	hwif->dma_status		= tmp_hwif->dma_status;

@@ -602,7 +601,6 @@ void ide_unregister(unsigned int index)
 		(void) ide_release_dma(hwif);
 
 		hwif->dma_base = 0;
-		hwif->dma_master = 0;
 		hwif->dma_command = 0;
 		hwif->dma_vendor1 = 0;
 		hwif->dma_status = 0;
drivers/ide/pci/cmd64x.c +5 -3
Index: b/drivers/ide/pci/cmd64x.c
===================================================================
@@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
 static int cmd648_ide_dma_end (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long base	= hwif->dma_base - (hwif->channel * 8);
 	int err			= __ide_dma_end(drive);
 	u8  irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
-	u8  mrdmode		= inb(hwif->dma_master + 0x01);
+	u8  mrdmode		= inb(base + 1);
 
 	/* clear the interrupt bit */
-	outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+	outb(mrdmode | irq_mask, base + 1);
 
 	return err;
 }

@@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
 static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long base	= hwif->dma_base - (hwif->channel * 8);
 	u8 irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
 	u8 dma_stat		= inb(hwif->dma_status);
-	u8 mrdmode		= inb(hwif->dma_master + 0x01);
+	u8 mrdmode		= inb(base + 1);
 
 #ifdef DEBUG
 	printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
drivers/ide/pci/hpt366.c +11 -10
Index: b/drivers/ide/pci/hpt366.c
===================================================================
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
 
 static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
 {
-	u8 scr2 = inb(hwif->dma_master + 0x7b);
+	unsigned long base = hwif->dma_base - (hwif->channel * 8);
+	u8 scr2 = inb(base + 0x7b);
 
 	if ((scr2 & 0x7f) == mode)
 		return;
 
 	/* Tristate the bus */
-	outb(0x80, hwif->dma_master + 0x73);
-	outb(0x80, hwif->dma_master + 0x77);
+	outb(0x80, base + 0x73);
+	outb(0x80, base + 0x77);
 
 	/* Switch clock and reset channels */
-	outb(mode, hwif->dma_master + 0x7b);
-	outb(0xc0, hwif->dma_master + 0x79);
+	outb(mode, base + 0x7b);
+	outb(0xc0, base + 0x79);
 
 	/*
 	 * Reset the state machines.
 	 * NOTE: avoid accidentally enabling the disabled channels.
 	 */
-	outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
-	outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
+	outb(inb(base + 0x70) | 0x32, base + 0x70);
+	outb(inb(base + 0x74) | 0x32, base + 0x74);
 
 	/* Complete reset */
-	outb(0x00, hwif->dma_master + 0x79);
+	outb(0x00, base + 0x79);
 
 	/* Reconnect channels to bus */
-	outb(0x00, hwif->dma_master + 0x73);
-	outb(0x00, hwif->dma_master + 0x77);
+	outb(0x00, base + 0x73);
+	outb(0x00, base + 0x77);
 }
 
 /**
drivers/ide/pci/pdc202xx_old.c +6 -6
Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
@@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide
  */
 static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;
+	unsigned long clock_reg = hwif->extra_base + 0x01;
 	u8 clock = inb(clock_reg);
 
 	outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
 
 static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;
+	unsigned long clock_reg = hwif->extra_base + 0x01;
 	u8 clock = inb(clock_reg);
 
 	outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		struct request *rq	= HWGROUP(drive)->rq;
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16   = hwif->dma_master;
+		unsigned long high_16	= hwif->extra_base - 16;
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u32 word_count	= 0;
 		u8 clock = inb(high_16 + 0x11);

@@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 {
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16	= hwif->dma_master;
+		unsigned long high_16	= hwif->extra_base - 16;
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u8 clock		= 0;
 

@@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= hwif->extra_base - 16;
 	u8 dma_stat		= inb(hwif->dma_status);
 	u8 sc1d			= inb(high_16 + 0x001d);
 

@@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
 
 static void pdc202xx_reset_host (ide_hwif_t *hwif)
 {
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= hwif->extra_base - 16;
 	u8 udma_speed_flag	= inb(high_16 | 0x001f);
 
 	outb(udma_speed_flag | 0x10, high_16 | 0x001f);
include/linux/ide.h +0 -1
Index: b/include/linux/ide.h
===================================================================
@@ -764,7 +764,6 @@ typedef struct hwif_s {
 	int		rqsize;		/* max sectors per request */
 	int		irq;		/* our irq number */
 
-	unsigned long	dma_master;	/* reference base addr dmabase */
 	unsigned long	dma_base;	/* base addr for dma ports */
 	unsigned long	dma_command;	/* dma command register */
 	unsigned long	dma_vendor1;	/* dma vendor 1 register */
#7 lore raw
In reply to Sergei Shtylyov
Quoted 17 lines · Hello, I wrote:
Quoted 16 lines
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

Before using pci_resource_start(), the code should check pci_resource_len() to ensure it is the appropriate size.

Quoted 7 lines
Quoted 5 lines
Quoted 1 line

It would be very wise to ensure that pci_resource_flags()&IORESOURCE_IO is true, too.

Sure but since the old code hasn't been doing these checks (old code just did pci_resource_start() -> hwif->dma_base -> hwif->dma_master) this is a separate issue from what the patch tries to achieve.

Nonetheless this is duplicating an existing bug $N times... not ideal :)

The existing bug is that the methods converted to use pci_resource_start() may be set in hwif instance and used even if the BAR4 is invalid. Therefore the above patch is correct and it isn't duplicating the existing bug. :)

Quoted 5 lines
Quoted 2 lines

However I agree that all these pci_resource_start()s could be a bit confusing so here is 'take 2'.

Yeah, they are. Actually, I was thinking about such patch ~1.5 years
ago *but* using a different approach -- namely, hwif->extra_base.

Yes, the code introducing this field was written with replacing dma_master
by it in mind... It's a pity I haven't gotten around to it.

[...]

Quoted 7 lines

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_

Quoted 5 lines

Index: b/drivers/ide/pci/hpt366.c
===================================================================

drivers/ide/pci/hpt366.c +0 -0
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_

This just asks to be:

No, it doesn't cause that way one would only break it.

Quoted 3 lines

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_ · line 847

Quoted 2 lines
drivers/ide/pci/hpt366.c +0 -0
 static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
 {

    unsigned long base = hwif->extra_base;

Sorry for a thinko. All offsets below shoud be really 0x6x, not 0x5x...

Quoted 20 lines
    u8 scr2 = inb(base + 0x5b);

     if ((scr2 & 0x7f) == mode)
         return;

     /* Tristate the bus */
    outb(0x80, base + 0x53);
    outb(0x80, base + 0x57);

     /* Switch clock and reset channels */
    outb(mode, base + 0x5b);
    outb(0xc0, base + 0x59);

     /*
      * Reset the state machines.
       * NOTE: avoid accidentally enabling the disabled channels.
      */
    outb(inb(base + 0x50) | 0x32, base + 0x50);
    outb(inb(base + 0x54) | 0x32, base + 0x54);
Quoted 7 lines
     /* Complete reset */
    outb(0x00, base + 0x59);

    /* Reconnect channels to bus */
    outb(0x00, base + 0x53);
    outb(0x00, base + 0x57);
}

MBR, Sergei

author
#8 lore raw

Hi,

Quoted 10 lines · Sergei Shtylyov

[...]

Quoted 7 lines

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_

Quoted 5 lines

Index: b/drivers/ide/pci/hpt366.c
===================================================================

drivers/ide/pci/hpt366.c +0 -0
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_

This just asks to be:

Quoted 7 lines

No, it doesn't cause that way one would only break it.

Quoted 3 lines

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_ · line 846

Quoted 2 lines
drivers/ide/pci/hpt366.c +0 -0
 static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
 {

    unsigned long base = hwif->extra_base;
    Sorry for a thinko. All offsets below shoud be really 0x6x, not 0x5x...

Here we go again... :)

[PATCH] ide: remove ->dma_master field from ide_hwif_t (take 4)

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

* Remove no longer needed ->dma_master field from ide_hwif_t.

v2:
* Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of pci_resource_start(hwif->pci_dev, 4).

v3:
* Use hwif->extra_base in hpt366/pdc20xx_old + some cosmetic fixups over v2 (suggested by Sergei).

v4:
* Correct offsets in hpt3xxn_set_clock().

Cc Sergei Shtylyov <[email protected]>
Cc Jeff Garzik <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-dma.c          |    7 -------
 drivers/ide/ide.c              |    2 --
 drivers/ide/pci/cmd64x.c       |    8 +++++---
 drivers/ide/pci/hpt366.c       |   21 +++++++++++----------
 drivers/ide/pci/pdc202xx_old.c |   12 ++++++------
 include/linux/ide.h            |    1 -
 6 files changed, 22 insertions(+), 29 deletions(-)
drivers/ide/ide-dma.c +0 -7
Index: b/drivers/ide/ide-dma.c
===================================================================
@@ -1002,11 +1002,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
 
 	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))

@@ -1048,8 +1043,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
 		       hwif->drives[1].name, (dma_stat & 0x40) ? "DMA" : "pio");
 	}
 	printk("\n");
-
-	BUG_ON(!hwif->dma_master);
 }
 
 EXPORT_SYMBOL_GPL(ide_setup_dma);
drivers/ide/ide.c +0 -2
Index: b/drivers/ide/ide.c
===================================================================
@@ -468,7 +468,6 @@ static void ide_hwif_restore(ide_hwif_t 
 #endif
 
 	hwif->dma_base			= tmp_hwif->dma_base;
-	hwif->dma_master		= tmp_hwif->dma_master;
 	hwif->dma_command		= tmp_hwif->dma_command;
 	hwif->dma_vendor1		= tmp_hwif->dma_vendor1;
 	hwif->dma_status		= tmp_hwif->dma_status;

@@ -602,7 +601,6 @@ void ide_unregister(unsigned int index)
 		(void) ide_release_dma(hwif);
 
 		hwif->dma_base = 0;
-		hwif->dma_master = 0;
 		hwif->dma_command = 0;
 		hwif->dma_vendor1 = 0;
 		hwif->dma_status = 0;
drivers/ide/pci/cmd64x.c +5 -3
Index: b/drivers/ide/pci/cmd64x.c
===================================================================
@@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
 static int cmd648_ide_dma_end (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long base	= hwif->dma_base - (hwif->channel * 8);
 	int err			= __ide_dma_end(drive);
 	u8  irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
-	u8  mrdmode		= inb(hwif->dma_master + 0x01);
+	u8  mrdmode		= inb(base + 1);
 
 	/* clear the interrupt bit */
-	outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+	outb(mrdmode | irq_mask, base + 1);
 
 	return err;
 }

@@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
 static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long base	= hwif->dma_base - (hwif->channel * 8);
 	u8 irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
 	u8 dma_stat		= inb(hwif->dma_status);
-	u8 mrdmode		= inb(hwif->dma_master + 0x01);
+	u8 mrdmode		= inb(base + 1);
 
 #ifdef DEBUG
 	printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
drivers/ide/pci/hpt366.c +11 -10
Index: b/drivers/ide/pci/hpt366.c
===================================================================
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
 
 static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
 {
-	u8 scr2 = inb(hwif->dma_master + 0x7b);
+	unsigned long base = hwif->dma_base - (hwif->channel * 8);
+	u8 scr2 = inb(base + 0x6b);
 
 	if ((scr2 & 0x7f) == mode)
 		return;
 
 	/* Tristate the bus */
-	outb(0x80, hwif->dma_master + 0x73);
-	outb(0x80, hwif->dma_master + 0x77);
+	outb(0x80, base + 0x63);
+	outb(0x80, base + 0x67);
 
 	/* Switch clock and reset channels */
-	outb(mode, hwif->dma_master + 0x7b);
-	outb(0xc0, hwif->dma_master + 0x79);
+	outb(mode, base + 0x6b);
+	outb(0xc0, base + 0x69);
 
 	/*
 	 * Reset the state machines.
 	 * NOTE: avoid accidentally enabling the disabled channels.
 	 */
-	outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
-	outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
+	outb(inb(base + 0x60) | 0x32, base + 0x60);
+	outb(inb(base + 0x64) | 0x32, base + 0x64);
 
 	/* Complete reset */
-	outb(0x00, hwif->dma_master + 0x79);
+	outb(0x00, base + 0x69);
 
 	/* Reconnect channels to bus */
-	outb(0x00, hwif->dma_master + 0x73);
-	outb(0x00, hwif->dma_master + 0x77);
+	outb(0x00, base + 0x63);
+	outb(0x00, base + 0x67);
 }
 
 /**
drivers/ide/pci/pdc202xx_old.c +6 -6
Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
@@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide
  */
 static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;
+	unsigned long clock_reg = hwif->extra_base + 0x01;
 	u8 clock = inb(clock_reg);
 
 	outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
 
 static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;
+	unsigned long clock_reg = hwif->extra_base + 0x01;
 	u8 clock = inb(clock_reg);
 
 	outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		struct request *rq	= HWGROUP(drive)->rq;
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16   = hwif->dma_master;
+		unsigned long high_16	= hwif->extra_base - 16;
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u32 word_count	= 0;
 		u8 clock = inb(high_16 + 0x11);

@@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 {
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16	= hwif->dma_master;
+		unsigned long high_16	= hwif->extra_base - 16;
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u8 clock		= 0;
 

@@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= hwif->extra_base - 16;
 	u8 dma_stat		= inb(hwif->dma_status);
 	u8 sc1d			= inb(high_16 + 0x001d);
 

@@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
 
 static void pdc202xx_reset_host (ide_hwif_t *hwif)
 {
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= hwif->extra_base - 16;
 	u8 udma_speed_flag	= inb(high_16 | 0x001f);
 
 	outb(udma_speed_flag | 0x10, high_16 | 0x001f);
include/linux/ide.h +0 -1
Index: b/include/linux/ide.h
===================================================================
@@ -764,7 +764,6 @@ typedef struct hwif_s {
 	int		rqsize;		/* max sectors per request */
 	int		irq;		/* our irq number */
 
-	unsigned long	dma_master;	/* reference base addr dmabase */
 	unsigned long	dma_base;	/* base addr for dma ports */
 	unsigned long	dma_command;	/* dma command register */
 	unsigned long	dma_vendor1;	/* dma vendor 1 register */
#9 lore raw

Hello.

Quoted 26 lines · Bartlomiej Zolnierkiewicz
Quoted 8 lines
Quoted 7 lines

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_

Quoted 5 lines

Index: b/drivers/ide/pci/hpt366.c
===================================================================

drivers/ide/pci/hpt366.c +0 -0
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_

This just asks to be:

Quoted 8 lines

No, it doesn't cause that way one would only break it.

Quoted 4 lines

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_ · line 847

Quoted 2 lines
drivers/ide/pci/hpt366.c +0 -0
 static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
 {

   unsigned long base = hwif->extra_base;

Sorry for a thinko. All offsets below shoud be really 0x6x, not 0x5x...

Here we go again... :)

[PATCH] ide: remove ->dma_master field from ide_hwif_t (take 4)

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

* Remove no longer needed ->dma_master field from ide_hwif_t.

Quoted 7 lines

v2:
* Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of pci_resource_start(hwif->pci_dev, 4).

v3:
* Use hwif->extra_base in hpt366/pdc20xx_old + some cosmetic fixups over v2 (suggested by Sergei).

Huh? I don't see where you're using extra_base in hpt366.c

Quoted 4 lines

v4:
* Correct offsets in hpt3xxn_set_clock().

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

Unfortunately, I have to NAK the patch... :-/

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_ · line 848

Quoted 10 lines

Index: b/drivers/ide/pci/hpt366.c
===================================================================

drivers/ide/pci/hpt366.c +1 -1
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
 
 static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
 {
-	u8 scr2 = inb(hwif->dma_master + 0x7b);
+	unsigned long base = hwif->dma_base - (hwif->channel * 8);

Should be hwif->extra_base - 0x10 or else the following code will touch
all the wrong/reserved registers; one also can use hwif->extra_base ISO 'base' and change all the offsets to 0x6x...

Quoted 26 lines
drivers/ide/pci/hpt366.c +7 -6
+	u8 scr2 = inb(base + 0x6b);
 
 	if ((scr2 & 0x7f) == mode)
 		return;
 
 	/* Tristate the bus */
-	outb(0x80, hwif->dma_master + 0x73);
-	outb(0x80, hwif->dma_master + 0x77);
+	outb(0x80, base + 0x63);
+	outb(0x80, base + 0x67);
 
 	/* Switch clock and reset channels */
-	outb(mode, hwif->dma_master + 0x7b);
-	outb(0xc0, hwif->dma_master + 0x79);
+	outb(mode, base + 0x6b);
+	outb(0xc0, base + 0x69);
 
 	/*
 	 * Reset the state machines.
 	 * NOTE: avoid accidentally enabling the disabled channels.
 	 */
-	outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
-	outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
+	outb(inb(base + 0x60) | 0x32, base + 0x60);
+	outb(inb(base + 0x64) | 0x32, base + 0x64);

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_ · line 875

Quoted 10 lines
drivers/ide/pci/hpt366.c +3 -3
 	/* Complete reset */
-	outb(0x00, hwif->dma_master + 0x79);
+	outb(0x00, base + 0x69);
 
 	/* Reconnect channels to bus */
-	outb(0x00, hwif->dma_master + 0x73);
-	outb(0x00, hwif->dma_master + 0x77);
+	outb(0x00, base + 0x63);
+	outb(0x00, base + 0x67);
 }

Huh? That's wrong -- if you're setting base to the value of the former
value of hwof->dma_master, the register offsets shouldn't change!

drivers/ide/pci/pdc202xx_old.c static void pdc202xx_old_ide_dma_start(i · line 197

Quoted 11 lines

Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================

drivers/ide/pci/pdc202xx_old.c +1 -1
@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		struct request *rq	= HWGROUP(drive)->rq;
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16   = hwif->dma_master;
+		unsigned long high_16	= hwif->extra_base - 16;
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);

That 'high_16' is *not* a good name in this case -- it would've been if we
didn't subtract 16. So, I'd wrote:

unsigned long atapi_reg = hwif->extra_base +

					 (hwif->channel ? 0x14 : 0x10);

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_old_ide_dma_end(ide_ · line 217

Quoted 8 lines
drivers/ide/pci/pdc202xx_old.c +1 -1
@@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 {
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16	= hwif->dma_master;
+		unsigned long high_16	= hwif->extra_base - 16;
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u8 clock		= 0;

Same here...

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_old_ide_dma_end(ide_ · line 233

Quoted 8 lines
drivers/ide/pci/pdc202xx_old.c +1 -1
@@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= hwif->extra_base - 16;
 	u8 dma_stat		= inb(hwif->dma_status);
 	u8 sc1d			= inb(high_16 + 0x001d);

... and here...

drivers/ide/pci/pdc202xx_old.c static void pdc202xx_dma_timeout(ide_dri · line 275

Quoted 7 lines
drivers/ide/pci/pdc202xx_old.c +1 -1
@@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
 
 static void pdc202xx_reset_host (ide_hwif_t *hwif)
 {
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= hwif->extra_base - 16;
 	u8 udma_speed_flag	= inb(high_16 | 0x001f);

... and here too.

drivers/ide/pci/pdc202xx_old.c static void pdc202xx_dma_timeout(ide_dri · line 277

Quoted 1 line
drivers/ide/pci/pdc202xx_old.c +0 -0
 	outb(udma_speed_flag | 0x10, high_16 | 0x001f);

MBR, Sergei

author
#10 lore raw

Hi,

Quoted 34 lines · Sergei Shtylyov

Hello.

Quoted 29 lines · Bartlomiej Zolnierkiewicz
Quoted 8 lines
Quoted 7 lines

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_

Quoted 5 lines

Index: b/drivers/ide/pci/hpt366.c
===================================================================

drivers/ide/pci/hpt366.c +0 -0
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_

This just asks to be:

Quoted 8 lines

No, it doesn't cause that way one would only break it.

Quoted 4 lines

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_ · line 846

Quoted 2 lines
drivers/ide/pci/hpt366.c +0 -0
 static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
 {

   unsigned long base = hwif->extra_base;

Sorry for a thinko. All offsets below shoud be really 0x6x, not 0x5x...

Here we go again... :)

[PATCH] ide: remove ->dma_master field from ide_hwif_t (take 4)

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

* Remove no longer needed ->dma_master field from ide_hwif_t.

v2:
* Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of pci_resource_start(hwif->pci_dev, 4).

Quoted 5 lines
Quoted 3 lines

v3:
* Use hwif->extra_base in hpt366/pdc20xx_old + some cosmetic fixups over v2 (suggested by Sergei).

Huh? I don't see where you're using extra_base in hpt366.c

Doh, somebody hand me brown paper bag, please...

Quoted 13 lines
Quoted 4 lines

v4:
* Correct offsets in hpt3xxn_set_clock().

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

Unfortunately, I have to NAK the patch... :-/

Quoted 6 lines

Index: b/drivers/ide/pci/hpt366.c
===================================================================

drivers/ide/pci/hpt366.c +0 -0
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
Quoted 8 lines

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_ · line 848

Quoted 4 lines
drivers/ide/pci/hpt366.c +1 -1
 static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
 {
-	u8 scr2 = inb(hwif->dma_master + 0x7b);
+	unsigned long base = hwif->dma_base - (hwif->channel * 8);

    Should be hwif->extra_base - 0x10 or else the following code will touch

all the wrong/reserved registers; one also can use hwif->extra_base ISO 'base' and change all the offsets to 0x6x...

Well, I updated all offsets but I somehow forgot about updating 'base'. :)

Quoted 55 lines
Quoted 26 lines
drivers/ide/pci/hpt366.c +7 -6
+	u8 scr2 = inb(base + 0x6b);
 
 	if ((scr2 & 0x7f) == mode)
 		return;
 
 	/* Tristate the bus */
-	outb(0x80, hwif->dma_master + 0x73);
-	outb(0x80, hwif->dma_master + 0x77);
+	outb(0x80, base + 0x63);
+	outb(0x80, base + 0x67);
 
 	/* Switch clock and reset channels */
-	outb(mode, hwif->dma_master + 0x7b);
-	outb(0xc0, hwif->dma_master + 0x79);
+	outb(mode, base + 0x6b);
+	outb(0xc0, base + 0x69);
 
 	/*
 	 * Reset the state machines.
 	 * NOTE: avoid accidentally enabling the disabled channels.
 	 */
-	outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
-	outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
+	outb(inb(base + 0x60) | 0x32, base + 0x60);
+	outb(inb(base + 0x64) | 0x32, base + 0x64);

drivers/ide/pci/hpt366.c static int hpt374_ide_dma_end(ide_drive_ · line 875

Quoted 10 lines
drivers/ide/pci/hpt366.c +3 -3
 	/* Complete reset */
-	outb(0x00, hwif->dma_master + 0x79);
+	outb(0x00, base + 0x69);
 
 	/* Reconnect channels to bus */
-	outb(0x00, hwif->dma_master + 0x73);
-	outb(0x00, hwif->dma_master + 0x77);
+	outb(0x00, base + 0x63);
+	outb(0x00, base + 0x67);
 }

Huh? That's wrong -- if you're setting base to the value of the former
value of hwof->dma_master, the register offsets shouldn't change!

drivers/ide/pci/pdc202xx_old.c static void pdc202xx_old_ide_dma_start(i · line 197

Quoted 11 lines

Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================

drivers/ide/pci/pdc202xx_old.c +1 -1
@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		struct request *rq	= HWGROUP(drive)->rq;
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16   = hwif->dma_master;
+		unsigned long high_16	= hwif->extra_base - 16;
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);

That 'high_16' is *not* a good name in this case -- it would've been if we
didn't subtract 16. So, I'd wrote:

Quoted 2 lines

unsigned long atapi_reg = hwif->extra_base +

					 (hwif->channel ? 0x14 : 0x10);

'high_16' is used for more than 'atapi_reg' (same is true for other places defining 'high_16' local variable). With five revisions of this patch needed to hopefully, finally get it right I prefer to defer futher cleanups to the incremental patches (*hint*).

[PATCH] ide: remove ->dma_master field from ide_hwif_t (take 5)

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

* Remove no longer needed ->dma_master field from ide_hwif_t.

v2:
* Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of pci_resource_start(hwif->pci_dev, 4).

v3:
* Use hwif->extra_base in hpt366/pdc20xx_old + some cosmetic fixups over v2 (suggested by Sergei).

v4:
* Correct offsets in hpt3xxn_set_clock().

v5:
* Use hwif->extra_base in hpt366 for _real_ this time. (Noticed by Sergei)

Cc Sergei Shtylyov <[email protected]>
Cc Jeff Garzik <[email protected]>
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/ide-dma.c          |    7 -------
 drivers/ide/ide.c              |    2 --
 drivers/ide/pci/cmd64x.c       |    8 +++++---
 drivers/ide/pci/hpt366.c       |   21 +++++++++++----------
 drivers/ide/pci/pdc202xx_old.c |   12 ++++++------
 include/linux/ide.h            |    1 -
 6 files changed, 22 insertions(+), 29 deletions(-)
drivers/ide/ide-dma.c +0 -7
Index: b/drivers/ide/ide-dma.c
===================================================================
@@ -1002,11 +1002,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
 
 	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))

@@ -1048,8 +1043,6 @@ void ide_setup_dma(ide_hwif_t *hwif, uns
 		       hwif->drives[1].name, (dma_stat & 0x40) ? "DMA" : "pio");
 	}
 	printk("\n");
-
-	BUG_ON(!hwif->dma_master);
 }
 
 EXPORT_SYMBOL_GPL(ide_setup_dma);
drivers/ide/ide.c +0 -2
Index: b/drivers/ide/ide.c
===================================================================
@@ -468,7 +468,6 @@ static void ide_hwif_restore(ide_hwif_t 
 #endif
 
 	hwif->dma_base			= tmp_hwif->dma_base;
-	hwif->dma_master		= tmp_hwif->dma_master;
 	hwif->dma_command		= tmp_hwif->dma_command;
 	hwif->dma_vendor1		= tmp_hwif->dma_vendor1;
 	hwif->dma_status		= tmp_hwif->dma_status;

@@ -602,7 +601,6 @@ void ide_unregister(unsigned int index)
 		(void) ide_release_dma(hwif);
 
 		hwif->dma_base = 0;
-		hwif->dma_master = 0;
 		hwif->dma_command = 0;
 		hwif->dma_vendor1 = 0;
 		hwif->dma_status = 0;
drivers/ide/pci/cmd64x.c +5 -3
Index: b/drivers/ide/pci/cmd64x.c
===================================================================
@@ -333,13 +333,14 @@ static void cmd64x_set_dma_mode(ide_driv
 static int cmd648_ide_dma_end (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long base	= hwif->dma_base - (hwif->channel * 8);
 	int err			= __ide_dma_end(drive);
 	u8  irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
-	u8  mrdmode		= inb(hwif->dma_master + 0x01);
+	u8  mrdmode		= inb(base + 1);
 
 	/* clear the interrupt bit */
-	outb(mrdmode | irq_mask, hwif->dma_master + 0x01);
+	outb(mrdmode | irq_mask, base + 1);
 
 	return err;
 }

@@ -364,10 +365,11 @@ static int cmd64x_ide_dma_end (ide_drive
 static int cmd648_ide_dma_test_irq (ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
+	unsigned long base	= hwif->dma_base - (hwif->channel * 8);
 	u8 irq_mask		= hwif->channel ? MRDMODE_INTR_CH1 :
 						  MRDMODE_INTR_CH0;
 	u8 dma_stat		= inb(hwif->dma_status);
-	u8 mrdmode		= inb(hwif->dma_master + 0x01);
+	u8 mrdmode		= inb(base + 1);
 
 #ifdef DEBUG
 	printk("%s: dma_stat: 0x%02x mrdmode: 0x%02x irq_mask: 0x%02x\n",
drivers/ide/pci/hpt366.c +11 -10
Index: b/drivers/ide/pci/hpt366.c
===================================================================
@@ -845,32 +845,33 @@ static int hpt374_ide_dma_end(ide_drive_
 
 static void hpt3xxn_set_clock(ide_hwif_t *hwif, u8 mode)
 {
-	u8 scr2 = inb(hwif->dma_master + 0x7b);
+	unsigned long base = hwif->extra_base;
+	u8 scr2 = inb(base + 0x6b);
 
 	if ((scr2 & 0x7f) == mode)
 		return;
 
 	/* Tristate the bus */
-	outb(0x80, hwif->dma_master + 0x73);
-	outb(0x80, hwif->dma_master + 0x77);
+	outb(0x80, base + 0x63);
+	outb(0x80, base + 0x67);
 
 	/* Switch clock and reset channels */
-	outb(mode, hwif->dma_master + 0x7b);
-	outb(0xc0, hwif->dma_master + 0x79);
+	outb(mode, base + 0x6b);
+	outb(0xc0, base + 0x69);
 
 	/*
 	 * Reset the state machines.
 	 * NOTE: avoid accidentally enabling the disabled channels.
 	 */
-	outb(inb(hwif->dma_master + 0x70) | 0x32, hwif->dma_master + 0x70);
-	outb(inb(hwif->dma_master + 0x74) | 0x32, hwif->dma_master + 0x74);
+	outb(inb(base + 0x60) | 0x32, base + 0x60);
+	outb(inb(base + 0x64) | 0x32, base + 0x64);
 
 	/* Complete reset */
-	outb(0x00, hwif->dma_master + 0x79);
+	outb(0x00, base + 0x69);
 
 	/* Reconnect channels to bus */
-	outb(0x00, hwif->dma_master + 0x73);
-	outb(0x00, hwif->dma_master + 0x77);
+	outb(0x00, base + 0x63);
+	outb(0x00, base + 0x67);
 }
 
 /**
drivers/ide/pci/pdc202xx_old.c +6 -6
Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
@@ -162,7 +162,7 @@ static u8 pdc202xx_old_cable_detect (ide
  */
 static void pdc_old_enable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;
+	unsigned long clock_reg = hwif->extra_base + 0x01;
 	u8 clock = inb(clock_reg);
 
 	outb(clock | (hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -170,7 +170,7 @@ static void pdc_old_enable_66MHz_clock(i
 
 static void pdc_old_disable_66MHz_clock(ide_hwif_t *hwif)
 {
-	unsigned long clock_reg = hwif->dma_master + 0x11;
+	unsigned long clock_reg = hwif->extra_base + 0x01;
 	u8 clock = inb(clock_reg);
 
 	outb(clock & ~(hwif->channel ? 0x08 : 0x02), clock_reg);

@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		struct request *rq	= HWGROUP(drive)->rq;
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16   = hwif->dma_master;
+		unsigned long high_16	= hwif->extra_base - 16;
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u32 word_count	= 0;
 		u8 clock = inb(high_16 + 0x11);

@@ -212,7 +212,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 {
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16	= hwif->dma_master;
+		unsigned long high_16	= hwif->extra_base - 16;
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);
 		u8 clock		= 0;
 

@@ -228,7 +228,7 @@ static int pdc202xx_old_ide_dma_end(ide_
 static int pdc202xx_old_ide_dma_test_irq(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif	= HWIF(drive);
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= hwif->extra_base - 16;
 	u8 dma_stat		= inb(hwif->dma_status);
 	u8 sc1d			= inb(high_16 + 0x001d);
 

@@ -271,7 +271,7 @@ static void pdc202xx_dma_timeout(ide_dri
 
 static void pdc202xx_reset_host (ide_hwif_t *hwif)
 {
-	unsigned long high_16	= hwif->dma_master;
+	unsigned long high_16	= hwif->extra_base - 16;
 	u8 udma_speed_flag	= inb(high_16 | 0x001f);
 
 	outb(udma_speed_flag | 0x10, high_16 | 0x001f);
include/linux/ide.h +0 -1
Index: b/include/linux/ide.h
===================================================================
@@ -766,7 +766,6 @@ typedef struct hwif_s {
 	int		rqsize;		/* max sectors per request */
 	int		irq;		/* our irq number */
 
-	unsigned long	dma_master;	/* reference base addr dmabase */
 	unsigned long	dma_base;	/* base addr for dma ports */
 	unsigned long	dma_command;	/* dma command register */
 	unsigned long	dma_vendor1;	/* dma vendor 1 register */
Acked-by
#11 lore raw

Hello.

Quoted 5 lines · Bartlomiej Zolnierkiewicz
Quoted 5 lines
Quoted 5 lines

Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================

drivers/ide/pci/pdc202xx_old.c +0 -0
@@ -193,7 +193,7 @@ static void pdc202xx_old_ide_dma_start(i
Quoted 8 lines
Quoted 8 lines

drivers/ide/pci/pdc202xx_old.c static void pdc202xx_old_ide_dma_start(i · line 197

Quoted 6 lines
drivers/ide/pci/pdc202xx_old.c +1 -1
 	if (drive->media != ide_disk || drive->addressing == 1) {
 		struct request *rq	= HWGROUP(drive)->rq;
 		ide_hwif_t *hwif	= HWIF(drive);
-		unsigned long high_16   = hwif->dma_master;
+		unsigned long high_16	= hwif->extra_base - 16;
 		unsigned long atapi_reg	= high_16 + (hwif->channel ? 0x24 : 0x20);

That 'high_16' is *not* a good name in this case --

It's not a good name in any case, because there's more than 16 extra
registers.

Quoted 9 lines
Quoted 4 lines

it would've been if we didn't subtract 16. So, I'd wrote:

unsigned long atapi_reg = hwif->extra_base +

					 (hwif->channel ? 0x14 : 0x10);

'high_16' is used for more than 'atapi_reg' (same is true for other places defining 'high_16' local variable). With five revisions of this patch needed to hopefully, finally get it right I prefer to defer futher cleanups to the incremental patches (*hint*).

Hint gotten. I'm still neck deep in other stuff but the IDE cleanups can
wait anyway.

Quoted 15 lines

[PATCH] ide: remove ->dma_master field from ide_hwif_t (take 5)

* Convert cmd64x, hpt366 and pdc202xx_old host drivers to use pci_resource_start(hwif->pci_dev, 4) instead of hwif->dma_master.

* Remove no longer needed ->dma_master field from ide_hwif_t.

v2:
* Use the more readable 'hwif->dma_base - (hwif->channel * 8)' instead of pci_resource_start(hwif->pci_dev, 4).

v3:
* Use hwif->extra_base in hpt366/pdc20xx_old + some cosmetic fixups over v2 (suggested by Sergei).

Quoted 7 lines

v4:
* Correct offsets in hpt3xxn_set_clock().

v5:
* Use hwif->extra_base in hpt366 for _real_ this time. (Noticed by Sergei)

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

MBR, Sergei