avoiding pci_disable_device()...

avoiding pci_disable_device()...

[Jeff Garzik]

[-- 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;
 }
 

Re: avoiding pci_disable_device()...

[Greg KH]

On Sun, Feb 13, 2005 at 08:42:55PM -0500, Jeff Garzik wrote:
> 
> 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.

But this is a very small subset of pci devices, correct?

> 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

Do you have any suggestions for how to do this?

thanks,

greg k-h

Re: avoiding pci_disable_device()...

[Alan Cox]

> > 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
> 
> Do you have any suggestions for how to do this?

One would be to keep an "enabler" count.

If the device is enabled at boot then set it to one (video, legacy IDE
etc) and it never gets back to zero. Otherwise set it to zero and it
goes up and down with the last [ab]user clearing it to zero and turning
it off. That also deals with the "who disables" question for power
mismanagement where the same problem occurs

Alan

Re: avoiding pci_disable_device()...

[Takashi Iwai]

At Mon, 14 Feb 2005 11:06:19 -0800,
Greg KH wrote:
> 
> > 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
> 
> Do you have any suggestions for how to do this?

How about to add an exclusiveness check in pci_enable_device()?
Most drivers suppose that the given pci resources are exclusively
available.


Takashi

Re: avoiding pci_disable_device()...

[Greg KH]

On Mon, Feb 14, 2005 at 08:24:29PM +0100, Takashi Iwai wrote:
> At Mon, 14 Feb 2005 11:06:19 -0800,
> Greg KH wrote:
> > 
> > > 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
> > 
> > Do you have any suggestions for how to do this?
> 
> How about to add an exclusiveness check in pci_enable_device()?
> Most drivers suppose that the given pci resources are exclusively
> available.

You mean only allow pci_enable_device() to work for the first caller of
it?  I don't see how that would help this issue out.

thanks,

greg k-h

Re: avoiding pci_disable_device()...

[Takashi Iwai]

At Mon, 14 Feb 2005 11:34:13 -0800,
Greg KH wrote:
> 
> On Mon, Feb 14, 2005 at 08:24:29PM +0100, Takashi Iwai wrote:
> > At Mon, 14 Feb 2005 11:06:19 -0800,
> > Greg KH wrote:
> > > 
> > > > 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
> > > 
> > > Do you have any suggestions for how to do this?
> > 
> > How about to add an exclusiveness check in pci_enable_device()?
> > Most drivers suppose that the given pci resources are exclusively
> > available.
> 
> You mean only allow pci_enable_device() to work for the first caller of
> it?  I don't see how that would help this issue out.

Well, for example, add a new pointer to indicate the driver accessing
exclusively.  And pci_enable_device() (maybe a new variant would be
better for compatibility) checks whether this is free.

The second caller wouldn't reach even to pci_request_regions() because
of this check.  So, no side-effect of pci_disable_device() in the
error path.


Takashi

Re: avoiding pci_disable_device()...

[Jeff Garzik]

Takashi Iwai wrote:
> At Mon, 14 Feb 2005 11:34:13 -0800,
> Greg KH wrote:
> 
>>On Mon, Feb 14, 2005 at 08:24:29PM +0100, Takashi Iwai wrote:
>>
>>>At Mon, 14 Feb 2005 11:06:19 -0800,
>>>Greg KH wrote:
>>>
>>>>>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
>>>>
>>>>Do you have any suggestions for how to do this?
>>>
>>>How about to add an exclusiveness check in pci_enable_device()?
>>>Most drivers suppose that the given pci resources are exclusively
>>>available.
>>
>>You mean only allow pci_enable_device() to work for the first caller of
>>it?  I don't see how that would help this issue out.
> 
> 
> Well, for example, add a new pointer to indicate the driver accessing
> exclusively.  And pci_enable_device() (maybe a new variant would be
> better for compatibility) checks whether this is free.
> 
> The second caller wouldn't reach even to pci_request_regions() because
> of this check.  So, no side-effect of pci_disable_device() in the
> error path.

This doesn't work with a driver that is properly using 
request_resource(), but not using the PCI API.

	Jeff

Re: avoiding pci_disable_device()...

[Jeff Garzik]

Greg KH wrote:
> On Sun, Feb 13, 2005 at 08:42:55PM -0500, Jeff Garzik wrote:
> 
>>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.
> 
> 
> But this is a very small subset of pci devices, correct?

No.  You also need to consider situations such as out-of-tree drivers 
for the same hardware (might not use PCI API), and situations where you 
have peer devices discovered and used (PCI API doesn't have "hey, <this> 
device is associated with <current driver>, too" capability)


>>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
> 
> 
> Do you have any suggestions for how to do this?

I'm glad you asked ;-)  As the author of pci_disable_device() and 
pci_request_regions(), I recognized their inadequacy almost immediately.

There are some fundamental flaws in the API that need correcting:

* pci_disable_device() should perform exactly the opposite of 
pci_enable_device(), no more, no less.   It should NOT unconditionally 
disable the device, but instead restore the hardware to the state it was 
in prior to pci_enable_device().

* pci_request_regions() should be axiomatic.  By that I mean, 
pci_enable_device() should
	(a) handle pci_request_regions() completely
	(b) fail if regions are not available

* pci_enable_device() may touch the hardware when it should not.  In an 
ideal world, pci_enable_device() would
	* assign resources to device, if necessary
	* request_resource()s [aka pci_request_regions()]
	* enable device by setting bits in PCI_COMMAND, etc.
but since the request-resource step is assumed to occur after 
pci_enable_device() returns to the driver, this is impossible.


The solution?  I am still thinking.  My gut feeling is that we want a 
slightly higher level PCI API for drivers.  Drivers pass in an 'info' 
structure to pci_up().  pci_up() enables the device, requests resources 
(not just irq), maps resources as necessary, enables irqs and/or MSI as 
necessary, and similar housekeeping.  pci_down() does the precise 
opposite. Essentially, pci_up() is a lib function that kills a ton of 
duplicate code from the vast majority of PCI drivers.


OTOH, Alan's suggestion seems sane and a lot more simple, but doesn't 
address the flaws in the API.

	Jeff

Re: avoiding pci_disable_device()...

[Roland Dreier]

    Jeff> * pci_request_regions() should be axiomatic.  By that I mean,
    Jeff> pci_enable_device() should
    Jeff> 	(a) handle pci_request_regions() completely
    Jeff> 	(b) fail if regions are not available

There's one pitfall here: for a device using MSI-X, the MSI-X table is
going to be somewhere in one of the device's BARs.  When the device
driver does pci_enable_msix(), drivers/pci/msi.c will do
request_region() on this table.  If the device driver has already done
pci_request_regions(), then this will fail and the driver won't be
able to use MSI-X.  The current solution is for the driver to avoid
requesting the whole BAR where the MSI-X table is.

 - R.

Re: avoiding pci_disable_device()...

[Jeff Garzik]

On Mon, Feb 14, 2005 at 11:58:38AM -0800, Roland Dreier wrote:
>     Jeff> * pci_request_regions() should be axiomatic.  By that I mean,
>     Jeff> pci_enable_device() should
>     Jeff> 	(a) handle pci_request_regions() completely
>     Jeff> 	(b) fail if regions are not available
> 
> There's one pitfall here: for a device using MSI-X, the MSI-X table is
> going to be somewhere in one of the device's BARs.  When the device
> driver does pci_enable_msix(), drivers/pci/msi.c will do
> request_region() on this table.  If the device driver has already done
> pci_request_regions(), then this will fail and the driver won't be
> able to use MSI-X.  The current solution is for the driver to avoid
> requesting the whole BAR where the MSI-X table is.


That's an MSI bug.

A current PCI driver -should- be using pci_request_regions().

	Jeff

Re: avoiding pci_disable_device()...

[Roland Dreier]

    Jeff> That's an MSI bug.

    Jeff> A current PCI driver -should- be using pci_request_regions().

Hmm... I'm not sure everyone would agree with that.  It does make
sense that the MSI-X core wants to make sure that it owns the MSI-X
table without having someone else stomp on it.

 - R.

Re: avoiding pci_disable_device()...

[Jeff Garzik]

Roland Dreier wrote:
>     Jeff> That's an MSI bug.
> 
>     Jeff> A current PCI driver -should- be using pci_request_regions().
> 
> Hmm... I'm not sure everyone would agree with that.  It does make
> sense that the MSI-X core wants to make sure that it owns the MSI-X
> table without having someone else stomp on it.

The idea is right but the implementation is definitely incorrect, for 
multiple reasons:

* Every PCI driver should call pci_request_regions(), because there 
should only be one pci_driver associated with that set of resources.

* Therefore, any _addition_ to the PCI API that prevents a driver from 
using pci_request_regions() causes multiple problems, by virtue of 
diverging from the other 98% of the kernel's PCI drivers, which use 
pci_request_regions().

IOW, MSI-X's implementation incorrectly constrains PCI driver authors.

* Programmer over-protection.  Who is attempting to stomp on MSI-X 
tables?  If a driver is reading an MSI-X bar and doing something without 
consulting the kernel MSI-X code, that's a bug and most likely a 
violation of the PCI spec.  Fix the driver.

In Linux we don't over-protect programmers with a zillion sanity checks 
for every internal API call; that leads to bloat, and papers over bugs 
that should be fixed.

	Jeff

Re: avoiding pci_disable_device()...

[Roland Dreier]

OK, I'm happy to go along with that (it definitely simplifies my
driver code).  Here's the patch.


Remove the call to request_mem_region() in msix_capability_init() to
grab the MSI-X vector table.  Drivers should be using
pci_request_regions() so that they own all of the PCI BARs, and the
MSI-X core should trust it's being called by a correct driver.

Signed-off-by: Roland Dreier <roland@topspin.com>

--- linux-orig/drivers/pci/msi.c	(revision 26881)
+++ linux/drivers/pci/msi.c	(working copy)
@@ -616,15 +616,10 @@ static int msix_capability_init(struct p
 	bir = (u8)(table_offset & PCI_MSIX_FLAGS_BIRMASK);
 	phys_addr = pci_resource_start (dev, bir);
 	phys_addr += (u32)(table_offset & ~PCI_MSIX_FLAGS_BIRMASK);
-	if (!request_mem_region(phys_addr,
-		nr_entries * PCI_MSIX_ENTRY_SIZE,
-		"MSI-X vector table"))
-		return -ENOMEM;
 	base = ioremap_nocache(phys_addr, nr_entries * PCI_MSIX_ENTRY_SIZE);
-	if (base == NULL) {
-		release_mem_region(phys_addr, nr_entries * PCI_MSIX_ENTRY_SIZE);
+	if (base == NULL)
 		return -ENOMEM;
-	}
+
 	/* MSI-X Table Initialization */
 	for (i = 0; i < nvec; i++) {
 		entry = alloc_msi_entry();
@@ -859,8 +854,6 @@ static int msi_free_vector(struct pci_de
 			phys_addr += (u32)(table_offset &
 				~PCI_MSIX_FLAGS_BIRMASK);
 			iounmap(base);
-			release_mem_region(phys_addr,
-				nr_entries * PCI_MSIX_ENTRY_SIZE);
 		}
 	}
 
@@ -1133,8 +1126,6 @@ void msi_remove_pci_irq_vectors(struct p
 			phys_addr += (u32)(table_offset &
 				~PCI_MSIX_FLAGS_BIRMASK);
 			iounmap(base);
-			release_mem_region(phys_addr, PCI_MSIX_ENTRY_SIZE *
-				multi_msix_capable(control));
 			printk(KERN_WARNING "PCI: %s: msi_remove_pci_irq_vectors() "
 			       "called without free_irq() on all MSI-X vectors\n",
 			       pci_name(dev));

Re: avoiding pci_disable_device()...

[Greg KH]

On Mon, Feb 14, 2005 at 02:46:38PM -0800, Roland Dreier wrote:
> OK, I'm happy to go along with that (it definitely simplifies my
> driver code).  Here's the patch.
> 
> 
> Remove the call to request_mem_region() in msix_capability_init() to
> grab the MSI-X vector table.  Drivers should be using
> pci_request_regions() so that they own all of the PCI BARs, and the
> MSI-X core should trust it's being called by a correct driver.
> 
> Signed-off-by: Roland Dreier <roland@topspin.com>

Applied, thanks,

greg k-h

Re: avoiding pci_disable_device()...

[Arjan van de Ven]

> No.  You also need to consider situations such as out-of-tree drivers 
> for the same hardware (might not use PCI API), and situations where you 
> have peer devices discovered and used (PCI API doesn't have "hey, <this> 
> device is associated with <current driver>, too" capability)


there's not a lot you or anyone else can do about such broken (and often
proprietary) drivers.... if a device doesn't use the kernel API's its
end of game basically. Adding more new API's isn't going to help you ...

Re: avoiding pci_disable_device()...

[Jeff Garzik]

Arjan van de Ven wrote:
>>No.  You also need to consider situations such as out-of-tree drivers 
>>for the same hardware (might not use PCI API), and situations where you 
>>have peer devices discovered and used (PCI API doesn't have "hey, <this> 
>>device is associated with <current driver>, too" capability)
> 
> 
> 
> there's not a lot you or anyone else can do about such broken (and often
> proprietary) drivers.... if a device doesn't use the kernel API's its
> end of game basically. Adding more new API's isn't going to help you ...


This specific instance isn't about adding a new API, but using an 
existing one correctly.

If pci_request_regions() fails, that implies another driver is using the 
kernel API to let you know the region is unavailable.  You should honor 
that, by not disabling the hardware in that case.

	Jeff

Re: avoiding pci_disable_device()...

[Takashi Iwai]

At Mon, 14 Feb 2005 21:05:58 -0500,
Jeff Garzik wrote:
> 
> Arjan van de Ven wrote:
> >>No.  You also need to consider situations such as out-of-tree drivers 
> >>for the same hardware (might not use PCI API), and situations where you 
> >>have peer devices discovered and used (PCI API doesn't have "hey, <this> 
> >>device is associated with <current driver>, too" capability)
> > 
> > 
> > 
> > there's not a lot you or anyone else can do about such broken (and often
> > proprietary) drivers.... if a device doesn't use the kernel API's its
> > end of game basically. Adding more new API's isn't going to help you ...
> 
> 
> This specific instance isn't about adding a new API, but using an 
> existing one correctly.
> 
> If pci_request_regions() fails, that implies another driver is using the 
> kernel API to let you know the region is unavailable.  You should honor 
> that, by not disabling the hardware in that case.

I guess an enable counter as Alan proposed would fix this problem
well, except for the case that an out-of-tree driver allocates the
resource without calling pci_enable_device().

OTOH this will introduce more buglets to broken drivers which don't
call pci_disable_device() properly.  Consequently, the ad hoc fix to
each driver like Jeff's patch might be most practical...


Takashi

Re: avoiding pci_disable_device()...

[Alan Cox]

> OTOH this will introduce more buglets to broken drivers which don't
> call pci_disable_device() properly.  Consequently, the ad hoc fix to
> each driver like Jeff's patch might be most practical...

This is true but it does provide the mechanism to fix such devices. It
also fails safe because a driver that fails to disable leaves the device
always enabled.

With the ability to mark the specific awkward cases as "enabled on boot"
we can remove some of the horrible special case issues like IDE
controllers using BARs of the northbridge.

Alan

Re: avoiding pci_disable_device()...

[Michal Rokos]

Hello,

> 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 :)

I believe this is needed for natsemi to be inline with $SUBJ.

Signed-off-by: Michal Rokos <michal@rokos.info>

--- linux-2.6/drivers/net/natsemi.c     2005-02-14 11:34:53.000000000 +0100
+++ linux-2.6-mr/drivers/net/natsemi.c  2005-02-14 11:36:00.000000000 +0100
@@ -811,6 +811,7 @@
        void __iomem *ioaddr;
        const int pcibar = 1; /* PCI base address register */
        int prev_eedata;
+       int pci_dev_busy = 0;
        u32 tmp;

 /* when built into the kernel, we only print version if device is found */
@@ -821,7 +822,13 @@
 #endif

        i = pci_enable_device(pdev);
-       if (i) return i;
+       if (i) goto out;
+
+       i = pci_request_regions(pdev, DRV_NAME);
+       if (i) {
+               pci_dev_busy = 1;
+               goto err_pci_request_regions;
+       }

        /* natsemi has a non-standard PM control register
         * in PCI config space.  Some boards apparently need
@@ -843,15 +850,13 @@
                pci_set_master(pdev);

        dev = alloc_etherdev(sizeof (struct netdev_private));
-       if (!dev)
-               return -ENOMEM;
+       if (!dev) {
+               i = -ENOMEM;
+               goto err_alloc_etherdev;
+       }
        SET_MODULE_OWNER(dev);
        SET_NETDEV_DEV(dev, &pdev->dev);

-       i = pci_request_regions(pdev, DRV_NAME);
-       if (i)
-               goto err_pci_request_regions;
-
        ioaddr = ioremap(iostart, iosize);
        if (!ioaddr) {
                i = -ENOMEM;
@@ -992,15 +997,20 @@
        }
        return 0;

- err_register_netdev:
+err_register_netdev:
        iounmap(ioaddr);

- err_ioremap:
-       pci_release_regions(pdev);
+err_ioremap:
        pci_set_drvdata(pdev, NULL);
-
- err_pci_request_regions:
        free_netdev(dev);
+
+err_alloc_etherdev:
+       pci_release_regions(pdev);
+
+err_pci_request_regions:
+       if (!pci_dev_busy)
+               pci_disable_device(pdev);
+out:
        return i;
 }

Re: avoiding pci_disable_device()...

[Christoph Hellwig]

On Mon, Feb 14, 2005 at 11:43:02AM +0100, Michal Rokos wrote:
> Hello,
> 
> > 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 :)
> 
> I believe this is needed for natsemi to be inline with $SUBJ.

Why?  I don't think there's any old-style driver for natsemi.  And if
there was please switch it to use modern pci probing or kill it.