Patchwork cy82c693: fix PCI device selection

login
register
mail settings
Submitter Bartlomiej Zolnierkiewicz
Date Oct. 11, 2011, 5:37 p.m.
Message ID <201110111937.32501.bzolnier@gmail.com>
Download mbox | patch
Permalink /patch/119030/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Bartlomiej Zolnierkiewicz - Oct. 11, 2011, 5:37 p.m.
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
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 <bzolnier@gmail.com>
---

 drivers/ide/cy82c693.c |   15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 11, 2011, 7:20 p.m.
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Tue, 11 Oct 2011 19:37:32 +0200

> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Oct. 12, 2011, 2:52 p.m.
David Miller wrote:

> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Tue, 11 Oct 2011 19:37:32 +0200
> 
> > 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Oct. 12, 2011, 5:58 p.m.
On Wed, 12 Oct 2011 16:52:10 +0200
Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> David Miller wrote:
> 
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Date: Tue, 11 Oct 2011 19:37:32 +0200
> > 
> > > 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 12, 2011, 7:02 p.m.
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Wed, 12 Oct 2011 16:52:10 +0200

> David Miller wrote:
> 
>> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>> Date: Tue, 11 Oct 2011 19:37:32 +0200
>> 
>> > 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,

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.


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

Index: b/drivers/ide/cy82c693.c
===================================================================
--- a/drivers/ide/cy82c693.c
+++ 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)