From: Jeff Garzik <jgarzik@pobox.com>
To: Linux Kernel <linux-kernel@vger.kernel.org>
Cc: Andrew Morton <akpm@osdl.org>, Greg KH <greg@kroah.com>,
Alan Cox <alan@lxorguk.ukuu.org.uk>
Subject: avoiding pci_disable_device()...
Date: Sun, 13 Feb 2005 20:42:55 -0500 [thread overview]
Message-ID: <4210021F.7060401@pobox.com> (raw)
[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]
Currently, in almost every PCI driver, if pci_request_regions() fails --
indicating another driver is using the hardware -- then
pci_disable_device() is called on the error path, disabling a device
that another driver is using
To call this "rather rude" is an understatement :)
Fortunately, the ugliness is mitigated in large part by the PCI layer
helping to make sure that no two drivers bind to the same PCI device.
Thus, in the vast majority of cases, pci_request_regions() -should- be
guaranteed to succeed.
However, there are oddball cases like mixed PCI/ISA devices (hello IDE)
or cases where a driver refers a pci_dev other than the primary, where
pci_request_regions() and request_regions() still matter.
As a result, I have committed the attached patch to libata-2.6. In many
cases, it is a "semantic fix", addressing the case
* pci_request_regions() indicates hardware is in use
* we rudely disable the in-use hardware
that would not occur in practice.
But better safe than sorry. Code cuts cut-n-pasted all over the place.
I'm hoping one or two things will happen now:
* janitors fix up the other PCI drivers along these lines
* improve the PCI API so that pci_request_regions() is axiomatic
[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 8901 bytes --]
#
# ChangeSet
# 2005/02/13 19:58:07-05:00 jgarzik@pobox.com
# [libata] do not call pci_disable_device() for certain errors
#
# If PCI request regions fails, then someone else is using the
# hardware we wish to use. For that one case, calling pci_disable_device()
# is rather rude.
#
diff -Nru a/drivers/scsi/ahci.c b/drivers/scsi/ahci.c
--- a/drivers/scsi/ahci.c 2005-02-13 19:58:50 -05:00
+++ b/drivers/scsi/ahci.c 2005-02-13 19:58:50 -05:00
@@ -940,6 +940,7 @@
unsigned long base;
void *mmio_base;
unsigned int board_idx = (unsigned int) ent->driver_data;
+ int pci_dev_busy = 0;
int rc;
VPRINTK("ENTER\n");
@@ -952,8 +953,10 @@
return rc;
rc = pci_request_regions(pdev, DRV_NAME);
- if (rc)
+ if (rc) {
+ pci_dev_busy = 1;
goto err_out;
+ }
pci_enable_intx(pdev);
@@ -1015,7 +1018,8 @@
err_out_regions:
pci_release_regions(pdev);
err_out:
- pci_disable_device(pdev);
+ if (!pci_dev_busy)
+ pci_disable_device(pdev);
return rc;
}
diff -Nru a/drivers/scsi/libata-core.c b/drivers/scsi/libata-core.c
--- a/drivers/scsi/libata-core.c 2005-02-13 19:58:50 -05:00
+++ b/drivers/scsi/libata-core.c 2005-02-13 19:58:50 -05:00
@@ -3656,6 +3656,7 @@
struct ata_port_info *port[2];
u8 tmp8, mask;
unsigned int legacy_mode = 0;
+ int disable_dev_on_err = 1;
int rc;
DPRINTK("ENTER\n");
@@ -3686,8 +3687,10 @@
return rc;
rc = pci_request_regions(pdev, DRV_NAME);
- if (rc)
+ if (rc) {
+ disable_dev_on_err = 0;
goto err_out;
+ }
if (legacy_mode) {
if (!request_region(0x1f0, 8, "libata")) {
@@ -3697,8 +3700,10 @@
conflict = ____request_resource(&ioport_resource, &res);
if (!strcmp(conflict->name, "libata"))
legacy_mode |= (1 << 0);
- else
+ else {
+ disable_dev_on_err = 0;
printk(KERN_WARNING "ata: 0x1f0 IDE port busy\n");
+ }
} else
legacy_mode |= (1 << 0);
@@ -3709,8 +3714,10 @@
conflict = ____request_resource(&ioport_resource, &res);
if (!strcmp(conflict->name, "libata"))
legacy_mode |= (1 << 1);
- else
+ else {
+ disable_dev_on_err = 0;
printk(KERN_WARNING "ata: 0x170 IDE port busy\n");
+ }
} else
legacy_mode |= (1 << 1);
}
@@ -3763,7 +3770,8 @@
release_region(0x170, 8);
pci_release_regions(pdev);
err_out:
- pci_disable_device(pdev);
+ if (disable_dev_on_err)
+ pci_disable_device(pdev);
return rc;
}
diff -Nru a/drivers/scsi/sata_nv.c b/drivers/scsi/sata_nv.c
--- a/drivers/scsi/sata_nv.c 2005-02-13 19:58:50 -05:00
+++ b/drivers/scsi/sata_nv.c 2005-02-13 19:58:50 -05:00
@@ -332,6 +332,7 @@
struct nv_host *host;
struct ata_port_info *ppi;
struct ata_probe_ent *probe_ent;
+ int pci_dev_busy = 0;
int rc;
u32 bar;
@@ -350,8 +351,10 @@
goto err_out;
rc = pci_request_regions(pdev, DRV_NAME);
- if (rc)
+ if (rc) {
+ pci_dev_busy = 1;
goto err_out_disable;
+ }
rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
if (rc)
@@ -427,7 +430,8 @@
err_out_regions:
pci_release_regions(pdev);
err_out_disable:
- pci_disable_device(pdev);
+ if (!pci_dev_busy)
+ pci_disable_device(pdev);
err_out:
return rc;
}
diff -Nru a/drivers/scsi/sata_promise.c b/drivers/scsi/sata_promise.c
--- a/drivers/scsi/sata_promise.c 2005-02-13 19:58:50 -05:00
+++ b/drivers/scsi/sata_promise.c 2005-02-13 19:58:50 -05:00
@@ -556,6 +556,7 @@
unsigned long base;
void *mmio_base;
unsigned int board_idx = (unsigned int) ent->driver_data;
+ int pci_dev_busy = 0;
int rc;
if (!printed_version++)
@@ -570,8 +571,10 @@
return rc;
rc = pci_request_regions(pdev, DRV_NAME);
- if (rc)
+ if (rc) {
+ pci_dev_busy = 1;
goto err_out;
+ }
rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
if (rc)
@@ -650,7 +653,8 @@
err_out_regions:
pci_release_regions(pdev);
err_out:
- pci_disable_device(pdev);
+ if (!pci_dev_busy)
+ pci_disable_device(pdev);
return rc;
}
diff -Nru a/drivers/scsi/sata_sil.c b/drivers/scsi/sata_sil.c
--- a/drivers/scsi/sata_sil.c 2005-02-13 19:58:50 -05:00
+++ b/drivers/scsi/sata_sil.c 2005-02-13 19:58:50 -05:00
@@ -336,6 +336,7 @@
void *mmio_base;
int rc;
unsigned int i;
+ int pci_dev_busy = 0;
u32 tmp, irq_mask;
if (!printed_version++)
@@ -350,8 +351,10 @@
return rc;
rc = pci_request_regions(pdev, DRV_NAME);
- if (rc)
+ if (rc) {
+ pci_dev_busy = 1;
goto err_out;
+ }
rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
if (rc)
@@ -438,7 +441,8 @@
err_out_regions:
pci_release_regions(pdev);
err_out:
- pci_disable_device(pdev);
+ if (!pci_dev_busy)
+ pci_disable_device(pdev);
return rc;
}
diff -Nru a/drivers/scsi/sata_sis.c b/drivers/scsi/sata_sis.c
--- a/drivers/scsi/sata_sis.c 2005-02-13 19:58:50 -05:00
+++ b/drivers/scsi/sata_sis.c 2005-02-13 19:58:50 -05:00
@@ -200,14 +200,17 @@
int rc;
u32 genctl;
struct ata_port_info *ppi;
+ int pci_dev_busy = 0;
rc = pci_enable_device(pdev);
if (rc)
return rc;
rc = pci_request_regions(pdev, DRV_NAME);
- if (rc)
+ if (rc) {
+ pci_dev_busy = 1;
goto err_out;
+ }
rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
if (rc)
@@ -259,7 +262,8 @@
pci_release_regions(pdev);
err_out:
- pci_disable_device(pdev);
+ if (!pci_dev_busy)
+ pci_disable_device(pdev);
return rc;
}
diff -Nru a/drivers/scsi/sata_svw.c b/drivers/scsi/sata_svw.c
--- a/drivers/scsi/sata_svw.c 2005-02-13 19:58:50 -05:00
+++ b/drivers/scsi/sata_svw.c 2005-02-13 19:58:50 -05:00
@@ -338,6 +338,7 @@
struct ata_probe_ent *probe_ent = NULL;
unsigned long base;
void *mmio_base;
+ int pci_dev_busy = 0;
int rc;
if (!printed_version++)
@@ -359,8 +360,10 @@
/* Request PCI regions */
rc = pci_request_regions(pdev, DRV_NAME);
- if (rc)
+ if (rc) {
+ pci_dev_busy = 1;
goto err_out;
+ }
rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
if (rc)
@@ -433,7 +436,8 @@
err_out_regions:
pci_release_regions(pdev);
err_out:
- pci_disable_device(pdev);
+ if (!pci_dev_busy)
+ pci_disable_device(pdev);
return rc;
}
diff -Nru a/drivers/scsi/sata_sx4.c b/drivers/scsi/sata_sx4.c
--- a/drivers/scsi/sata_sx4.c 2005-02-13 19:58:50 -05:00
+++ b/drivers/scsi/sata_sx4.c 2005-02-13 19:58:50 -05:00
@@ -1366,6 +1366,7 @@
void *mmio_base, *dimm_mmio = NULL;
struct pdc_host_priv *hpriv = NULL;
unsigned int board_idx = (unsigned int) ent->driver_data;
+ int pci_dev_busy = 0;
int rc;
if (!printed_version++)
@@ -1380,8 +1381,10 @@
return rc;
rc = pci_request_regions(pdev, DRV_NAME);
- if (rc)
+ if (rc) {
+ pci_dev_busy = 1;
goto err_out;
+ }
rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
if (rc)
@@ -1471,7 +1474,8 @@
err_out_regions:
pci_release_regions(pdev);
err_out:
- pci_disable_device(pdev);
+ if (!pci_dev_busy)
+ pci_disable_device(pdev);
return rc;
}
diff -Nru a/drivers/scsi/sata_uli.c b/drivers/scsi/sata_uli.c
--- a/drivers/scsi/sata_uli.c 2005-02-13 19:58:50 -05:00
+++ b/drivers/scsi/sata_uli.c 2005-02-13 19:58:50 -05:00
@@ -185,14 +185,17 @@
struct ata_port_info *ppi;
int rc;
unsigned int board_idx = (unsigned int) ent->driver_data;
+ int pci_dev_busy = 0;
rc = pci_enable_device(pdev);
if (rc)
return rc;
rc = pci_request_regions(pdev, DRV_NAME);
- if (rc)
+ if (rc) {
+ pci_dev_busy = 1;
goto err_out;
+ }
rc = pci_set_dma_mask(pdev, ATA_DMA_MASK);
if (rc)
@@ -260,7 +263,8 @@
pci_release_regions(pdev);
err_out:
- pci_disable_device(pdev);
+ if (!pci_dev_busy)
+ pci_disable_device(pdev);
return rc;
}
diff -Nru a/drivers/scsi/sata_via.c b/drivers/scsi/sata_via.c
--- a/drivers/scsi/sata_via.c 2005-02-13 19:58:50 -05:00
+++ b/drivers/scsi/sata_via.c 2005-02-13 19:58:50 -05:00
@@ -290,6 +290,7 @@
struct ata_probe_ent *probe_ent;
int board_id = (int) ent->driver_data;
const int *bar_sizes;
+ int pci_dev_busy = 0;
u8 tmp8;
if (!printed_version++)
@@ -300,8 +301,10 @@
return rc;
rc = pci_request_regions(pdev, DRV_NAME);
- if (rc)
+ if (rc) {
+ pci_dev_busy = 1;
goto err_out;
+ }
if (board_id == vt6420) {
pci_read_config_byte(pdev, SATA_PATA_SHARING, &tmp8);
@@ -360,7 +363,8 @@
err_out_regions:
pci_release_regions(pdev);
err_out:
- pci_disable_device(pdev);
+ if (!pci_dev_busy)
+ pci_disable_device(pdev);
return rc;
}
diff -Nru a/drivers/scsi/sata_vsc.c b/drivers/scsi/sata_vsc.c
--- a/drivers/scsi/sata_vsc.c 2005-02-13 19:58:50 -05:00
+++ b/drivers/scsi/sata_vsc.c 2005-02-13 19:58:50 -05:00
@@ -255,6 +255,7 @@
static int printed_version;
struct ata_probe_ent *probe_ent = NULL;
unsigned long base;
+ int pci_dev_busy = 0;
void *mmio_base;
int rc;
@@ -274,8 +275,10 @@
}
rc = pci_request_regions(pdev, DRV_NAME);
- if (rc)
+ if (rc) {
+ pci_dev_busy = 1;
goto err_out;
+ }
/*
* Use 32 bit DMA mask, because 64 bit address support is poor.
@@ -352,7 +355,8 @@
err_out_regions:
pci_release_regions(pdev);
err_out:
- pci_disable_device(pdev);
+ if (!pci_dev_busy)
+ pci_disable_device(pdev);
return rc;
}
next reply other threads:[~2005-02-14 1:43 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2005-02-14 1:42 Jeff Garzik [this message]
2005-02-14 19:06 ` avoiding pci_disable_device() Greg KH
2005-02-14 18:08 ` Alan Cox
2005-02-14 19:24 ` Takashi Iwai
2005-02-14 19:34 ` Greg KH
2005-02-14 19:50 ` Takashi Iwai
2005-02-14 19:54 ` Jeff Garzik
2005-02-14 19:51 ` Jeff Garzik
2005-02-14 19:58 ` Roland Dreier
2005-02-14 20:00 ` Jeff Garzik
2005-02-14 21:42 ` Roland Dreier
2005-02-14 22:25 ` Jeff Garzik
2005-02-14 22:46 ` Roland Dreier
2005-02-17 23:07 ` Greg KH
2005-02-14 20:02 ` Arjan van de Ven
2005-02-15 2:05 ` Jeff Garzik
2005-02-16 11:27 ` Takashi Iwai
2005-02-16 13:44 ` Alan Cox
-- strict thread matches above, loose matches on Subject: below --
2005-02-14 10:43 Michal Rokos
2005-02-14 11:08 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4210021F.7060401@pobox.com \
--to=jgarzik@pobox.com \
--cc=akpm@osdl.org \
--cc=alan@lxorguk.ukuu.org.uk \
--cc=greg@kroah.com \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox