Skip to contentSkip to conversation

cy82c693: fix PCI device selection

PATCH Applied Bartlomiej Zolnierkiewicz 3 participants 7 messages Oct 11, 2011 – Oct 13, 2011 libata

lore
Share & tools
lore

Bartlomiej Zolnierkiewicz

author
#0 lore raw

PATCH 1/1 cy82c693: fix PCI device selection

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] cy82c693: fix PCI device selection

Wrong PCI device may be selected by cy82c693_set_pio_mode() if modular IDE host drivers are used and there are additional IDE PCI devices installed in the system. Fix it.

While at it remove redundant pci_get_slot() call as cy82c693_init_one() already takes care of keeping the reference on the second port's PCI device.

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

---

 drivers/ide/cy82c693.c |   15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)
drivers/ide/cy82c693.c +1 -14
Index: b/drivers/ide/cy82c693.c
===================================================================
@@ -82,24 +82,13 @@ static void cy82c693_set_dma_mode(ide_hw
 
 static void cy82c693_set_pio_mode(ide_hwif_t *hwif, ide_drive_t *drive)
 {
-	struct pci_dev *dev = to_pci_dev(hwif->dev);
+	struct pci_dev *dev = to_pci_dev(hwif->host->dev[drive->dn / 2]);
 	int bus_speed = ide_pci_clk ? ide_pci_clk : 33;
 	const unsigned long T = 1000000 / bus_speed;
 	unsigned int addrCtrl;
 	struct ide_timing t;
 	u8 time_16, time_8;
 
-	/* select primary or secondary channel */
-	if (hwif->index > 0) {  /* drive is on the secondary channel */
-		dev = pci_get_slot(dev->bus, dev->devfn+1);
-		if (!dev) {
-			printk(KERN_ERR "%s: tune_drive: "
-				"Cannot find secondary interface!\n",
-				drive->name);
-			return;
-		}
-	}
-
 	ide_timing_compute(drive, drive->pio_mode, &t, T, 1);
 
 	time_16 = clamp_val(t.recover - 1, 0, 15) |

@@ -141,8 +130,6 @@ static void cy82c693_set_pio_mode(ide_hw
 		pci_write_config_byte(dev, CY82_IDE_SLAVE_IOW, time_16);
 		pci_write_config_byte(dev, CY82_IDE_SLAVE_8BIT, time_8);
 	}
-	if (hwif->index > 0)
-		pci_dev_put(dev);
 }
 
 static void __devinit init_iops_cy82c693(ide_hwif_t *hwif)
#1 lore raw

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 11 Oct 2011 19:37:32 +0200

Quoted 3 lines

While at it remove redundant pci_get_slot() call as cy82c693_init_one() already takes care of keeping the reference on the second port's PCI device.

Please do not submit unrelated changes with a bug fix, and as IDE is in long-term maintainence I would not accept a risky refinement like this anyways.

author
#2 lore raw
Quoted 10 lines · David Miller

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 11 Oct 2011 19:37:32 +0200

Quoted 3 lines

While at it remove redundant pci_get_slot() call as cy82c693_init_one() already takes care of keeping the reference on the second port's PCI device.

Please do not submit unrelated changes with a bug fix, and as IDE is in long-term maintainence I would not accept a risky refinement like this anyways.

Removed code is just bogus, we cannot fail in ->set_pio_mode method.

Please take a look at the code:

- /* select primary or secondary channel */
- if (hwif->index > 0) { /* drive is on the secondary channel */
- dev = pci_get_slot(dev->bus, dev->devfn+1);
- if (!dev) {
- printk(KERN_ERR "%s: tune_drive: "
- "Cannot find secondary interface!\n",
- drive->name);
- return;
- }
- }

Please apply the patch, libata's pata_cypress doesn't support secondary port currently and cy82c693 driver is the only way to use this hardware.

#3 lore raw
Quoted 28 lines · On Wed, 12 Oct 2011 16:52:10 +0200 Bartlomiej Zolnierkiewicz…
Quoted 10 lines · David Miller

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 11 Oct 2011 19:37:32 +0200

Quoted 3 lines

While at it remove redundant pci_get_slot() call as cy82c693_init_one() already takes care of keeping the reference on the second port's PCI device.

Please do not submit unrelated changes with a bug fix, and as IDE is in long-term maintainence I would not accept a risky refinement like this anyways.

Removed code is just bogus, we cannot fail in ->set_pio_mode method.

Please take a look at the code:

drivers/ide/cy82c693.c +0 -10
-	/* select primary or secondary channel */
-	if (hwif->index > 0) {  /* drive is on the secondary channel */
-		dev = pci_get_slot(dev->bus, dev->devfn+1);
-		if (!dev) {
-			printk(KERN_ERR "%s: tune_drive: "
-				"Cannot find secondary interface!\n",
-				drive->name);
-			return;
-		}
-	}
Quoted 2 lines

Please apply the patch, libata's pata_cypress doesn't support secondary port currently and cy82c693 driver is the only way to use this hardware.

I've not found anyone using an Alpha 164SX in years so it's not been possible to do the updates and test them. And no ... do not offer to send me an Alpha, donate it to a museum.

Alan

#4 lore raw
In reply to Bartlomiej Zolnierkiewicz

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Wed, 12 Oct 2011 16:52:10 +0200

Quoted 28 lines
Quoted 10 lines · David Miller

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 11 Oct 2011 19:37:32 +0200

Quoted 3 lines

While at it remove redundant pci_get_slot() call as cy82c693_init_one() already takes care of keeping the reference on the second port's PCI device.

Please do not submit unrelated changes with a bug fix, and as IDE is in long-term maintainence I would not accept a risky refinement like this anyways.

Removed code is just bogus, we cannot fail in ->set_pio_mode method.

Please take a look at the code:

drivers/ide/cy82c693.c +0 -10
-	/* select primary or secondary channel */
-	if (hwif->index > 0) {  /* drive is on the secondary channel */
-		dev = pci_get_slot(dev->bus, dev->devfn+1);
-		if (!dev) {
-			printk(KERN_ERR "%s: tune_drive: "
-				"Cannot find secondary interface!\n",
-				drive->name);
-			return;
-		}
-	}
Quoted 1 line

Please apply the patch,

Sorry, I will not do that, please respin the patch with the unrelated pieces removed. I don't care how obvious it is to you.

The IDE layer is not a place for refinements or simplifications any longer, I'm sorry if that isn't clear to you.

author
#5 lore raw
Quoted 31 lines · David Miller

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Wed, 12 Oct 2011 16:52:10 +0200

Quoted 28 lines
Quoted 10 lines · David Miller

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 11 Oct 2011 19:37:32 +0200

Quoted 3 lines

While at it remove redundant pci_get_slot() call as cy82c693_init_one() already takes care of keeping the reference on the second port's PCI device.

Please do not submit unrelated changes with a bug fix, and as IDE is in long-term maintainence I would not accept a risky refinement like this anyways.

Removed code is just bogus, we cannot fail in ->set_pio_mode method.

Please take a look at the code:

(quoted diff) +0 -10
-	/* select primary or secondary channel */
-	if (hwif->index > 0) {  /* drive is on the secondary channel */
-		dev = pci_get_slot(dev->bus, dev->devfn+1);
-		if (!dev) {
-			printk(KERN_ERR "%s: tune_drive: "
-				"Cannot find secondary interface!\n",
-				drive->name);
-			return;
-		}
-	}
Quoted 7 lines
Quoted 1 line

Please apply the patch,

Sorry, I will not do that, please respin the patch with the unrelated pieces removed. I don't care how obvious it is to you.

The IDE layer is not a place for refinements or simplifications any longer, I'm sorry if that isn't clear to you.

Calling pci_get_slot() inside ->set_pio_mode is a bug since this method cannot fail but if you want to leave it as it is here is an updated patch:

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH v2] cy82c693: fix PCI device selection

Wrong PCI device may be selected by cy82c693_set_pio_mode() if modular IDE host drivers are used and there are additional IDE PCI devices installed in the system. Fix it.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/cy82c693.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/ide/cy82c693.c +3 -3
Index: b/drivers/ide/cy82c693.c
===================================================================
@@ -1,7 +1,7 @@
 /*
  *  Copyright (C) 1998-2000 Andreas S. Krebs ([email protected]), Maintainer
  *  Copyright (C) 1998-2002 Andre Hedrick <[email protected]>, Integrator
- *  Copyright (C) 2007-2010 Bartlomiej Zolnierkiewicz
+ *  Copyright (C) 2007-2011 Bartlomiej Zolnierkiewicz
  *
  * CYPRESS CY82C693 chipset IDE controller
  *

@@ -90,7 +90,7 @@ static void cy82c693_set_pio_mode(ide_hw
 	u8 time_16, time_8;
 
 	/* select primary or secondary channel */
-	if (hwif->index > 0) {  /* drive is on the secondary channel */
+	if (drive->dn > 1) {  /* drive is on the secondary channel */
 		dev = pci_get_slot(dev->bus, dev->devfn+1);
 		if (!dev) {
 			printk(KERN_ERR "%s: tune_drive: "

@@ -141,7 +141,7 @@ static void cy82c693_set_pio_mode(ide_hw
 		pci_write_config_byte(dev, CY82_IDE_SLAVE_IOW, time_16);
 		pci_write_config_byte(dev, CY82_IDE_SLAVE_8BIT, time_8);
 	}
-	if (hwif->index > 0)
+	if (drive->dn > 1)
 		pci_dev_put(dev);
 }
Applied
#6 lore raw

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Thu, 13 Oct 2011 12:28:54 +0200

Quoted 8 lines

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH v2] cy82c693: fix PCI device selection

Wrong PCI device may be selected by cy82c693_set_pio_mode() if modular IDE host drivers are used and there are additional IDE PCI devices installed in the system. Fix it.

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

Applied.