[PATCH 0/4] PCI legacy I/O port free driver (take4)

[PATCH 0/4] PCI legacy I/O port free driver (take4)

[Kenji Kaneshige]

Hi Andrew, Greg,

Here is an updated set of patches for PCI legacy I/O port free
driver. It incorporates all of the feedbacks. I rebased it against the
latest -mm kernel (2.6.16-rc5-mm1).

Could you consider applying this to -mm tree?

Thanks,
Kenji Kaneshige

[PATCH 1/4] PCI legacy I/O port free driver (take4) - Add no_ioport flag into pci_dev

[Kenji Kaneshige]

This patch adds the no_ioport field into struct pci_dev, which is used
to tell the kernel not to touch any I/O port regions.

Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>

 drivers/pci/pci.c   |   39 ++++++++++++++++++++++++++++++++-------
 include/linux/pci.h |    1 +
 2 files changed, 33 insertions(+), 7 deletions(-)

Index: linux-2.6.16-rc5-mm1/include/linux/pci.h
===================================================================
--- linux-2.6.16-rc5-mm1.orig/include/linux/pci.h	2006-03-01 13:56:06.000000000 +0900
+++ linux-2.6.16-rc5-mm1/include/linux/pci.h	2006-03-01 16:30:56.000000000 +0900
@@ -163,6 +163,7 @@
 	unsigned int	is_busmaster:1; /* device is busmaster */
 	unsigned int	no_msi:1;	/* device may not use msi */
 	unsigned int	block_ucfg_access:1;	/* userspace config space access is blocked */
+	unsigned int	no_ioport:1;	/* device may not use ioport */
 
 	u32		saved_config_space[16]; /* config space saved at suspend time */
 	struct hlist_head saved_cap_space;
Index: linux-2.6.16-rc5-mm1/drivers/pci/pci.c
===================================================================
--- linux-2.6.16-rc5-mm1.orig/drivers/pci/pci.c	2006-03-01 13:56:04.000000000 +0900
+++ linux-2.6.16-rc5-mm1/drivers/pci/pci.c	2006-03-01 16:35:00.000000000 +0900
@@ -507,7 +507,14 @@
 int
 pci_enable_device(struct pci_dev *dev)
 {
-	int err = pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1);
+	int i, err, bars = (1 << PCI_NUM_RESOURCES) - 1;
+
+	if (dev->no_ioport)
+		for (i = 0; i < PCI_NUM_RESOURCES; i++)
+			if (pci_resource_flags(dev, i) & IORESOURCE_IO)
+				bars &= ~(1 << i);
+
+	err = pci_enable_device_bars(dev, bars);
 	if (err)
 		return err;
 	pci_fixup_device(pci_fixup_enable, dev);
@@ -628,9 +635,14 @@
 {
 	if (pci_resource_len(pdev, bar) == 0)
 		return;
-	if (pci_resource_flags(pdev, bar) & IORESOURCE_IO)
+	if (pci_resource_flags(pdev, bar) & IORESOURCE_IO) {
+		if (pdev->no_ioport)
+			dev_warn(&pdev->dev,
+				 "Trying to release PCI I/O region #%d for "
+				 "the device marked I/O resource free\n", bar);
 		release_region(pci_resource_start(pdev, bar),
 				pci_resource_len(pdev, bar));
+	}
 	else if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM)
 		release_mem_region(pci_resource_start(pdev, bar),
 				pci_resource_len(pdev, bar));
@@ -656,6 +668,10 @@
 		return 0;
 		
 	if (pci_resource_flags(pdev, bar) & IORESOURCE_IO) {
+		if (pdev->no_ioport)
+			dev_warn(&pdev->dev,
+				 "Trying to request PCI I/O region #%d for "
+				 "the device marked I/O resource free\n", bar);
 		if (!request_region(pci_resource_start(pdev, bar),
 			    pci_resource_len(pdev, bar), res_name))
 			goto err_out;
@@ -689,10 +705,13 @@
 
 void pci_release_regions(struct pci_dev *pdev)
 {
-	int i;
+	int i, no_ioport = pdev->no_ioport;
 	
-	for (i = 0; i < 6; i++)
+	for (i = 0; i < 6; i++) {
+		if (no_ioport && (pci_resource_flags(pdev, i) & IORESOURCE_IO))
+			continue;
 		pci_release_region(pdev, i);
+	}
 }
 
 /**
@@ -710,16 +729,22 @@
  */
 int pci_request_regions(struct pci_dev *pdev, char *res_name)
 {
-	int i;
+	int i, no_ioport = pdev->no_ioport;
 	
-	for (i = 0; i < 6; i++)
+	for (i = 0; i < 6; i++) {
+		if (no_ioport && (pci_resource_flags(pdev, i) & IORESOURCE_IO))
+			continue;
 		if(pci_request_region(pdev, i, res_name))
 			goto err_out;
+	}
 	return 0;
 
 err_out:
-	while(--i >= 0)
+	while(--i >= 0) {
+		if (no_ioport && (pci_resource_flags(pdev, i) & IORESOURCE_IO))
+			continue;
 		pci_release_region(pdev, i);
+	}
 		
 	return -EBUSY;
 }

[PATCH 2/4] PCI legacy I/O port free driver (take4) - Update Documentation/pci.txt

[Kenji Kaneshige]

This patch adds the description about legacy I/O port free driver into
Documentation/pci.txt.

Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>

 Documentation/pci.txt |   68 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 68 insertions(+)

Index: linux-2.6.16-rc5-mm1/Documentation/pci.txt
===================================================================
--- linux-2.6.16-rc5-mm1.orig/Documentation/pci.txt	2006-01-03 12:21:10.000000000 +0900
+++ linux-2.6.16-rc5-mm1/Documentation/pci.txt	2006-03-01 16:36:11.000000000 +0900
@@ -269,3 +269,71 @@
 pci_find_device()		Superseded by pci_get_device()
 pci_find_subsys()		Superseded by pci_get_subsys()
 pci_find_slot()			Superseded by pci_get_slot()
+
+
+9. Legacy I/O port free driver
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Large servers may not be able to provide I/O port resources to all PCI
+devices. I/O Port space is only 64KB on Intel Architecture[1] and is
+likely also fragmented since the I/O base register of PCI-to-PCI
+bridge will usually be aligned to a 4KB boundary[2]. On such systems,
+pci_enable_device() and pci_request_regions() will fail when
+attempting to enable I/O Port regions that don't have I/O Port
+resources assigned.
+
+Fortunately, many PCI devices which request I/O Port resources also
+provide access to the same registers via MMIO BARs. These devices can
+be handled without using I/O port space and the drivers typically
+offer a CONFIG_ option to only use MMIO regions
+(e.g. CONFIG_TULIP_MMIO). PCI devices typically provide I/O port
+interface for legacy OSs and will work when I/O port resources are not
+assigned. The "PCI Local Bus Specification Revision 3.0" discusses
+this on p.44, "IMPLEMENTATION NOTE".
+
+If your PCI device driver doesn't need I/O port resources assigned to
+I/O Port BARs, set the no_ioport flag in struct pci_dev before calling
+pci_enable_device() and pci_request_regions(). If the no_ioport flag
+is set, generic PCI support will ignore I/O port regions for the
+corresponding devices.
+
+[1] Some systems support 64KB I/O port space per PCI segment.
+[2] Some PCI-to-PCI bridges support optional 1KB aligned I/O base.
+
+
+10. MMIO Space and "Write Posting"
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Converting a driver from using I/O Port space to using MMIO space
+often requires some additional changes. Specifically, "write posting"
+needs to be handled. Most drivers (e.g. tg3, acenic, sym53c8xx_2)
+already do. I/O Port space guarantees write transactions reach the PCI
+device before the CPU can continue. Writes to MMIO space allow to CPU
+continue before the transaction reaches the PCI device. HW weenies
+call this "Write Posting" because the write completion is "posted" to
+the CPU before the transaction has reached it's destination.
+
+Thus, timing sensitive code should add readl() where the CPU is
+expected to wait before doing other work.  The classic "bit banging"
+sequence works fine for I/O Port space:
+
+	for (i=8; --i; val >>= 1) {
+		outb(val & 1, ioport_reg);	/* write bit */
+		udelay(10);
+	}
+
+The same sequence for MMIO space should be:
+
+	for (i=8; --i; val >>= 1) {
+		writeb(val & 1, mmio_reg);	/* write bit */
+		readb(safe_mmio_reg);		/* flush posted write */
+		udelay(10);
+	}
+
+It is important that "safe_mmio_reg" not have any side effects that
+interferes with the correct operation of the device.
+
+Another case to watch out for is when resetting a PCI device. Use PCI
+Configuration space reads to flush the writel(). This will gracefully
+handle the PCI master abort on all platforms if the PCI device is
+expected to not respond to a readl().  Most x86 platforms will allow
+MMIO reads to master abort (aka "Soft Fail") and return garbage
+(e.g. ~0). But many RISC platforms will crash (aka "Hard Fail").

[PATCH 3/4] PCI legacy I/O port free driver (take4) - Make Intel e1000 driver legacy I/O port free

[Kenji Kaneshige]

This patch makes Intel e1000 driver legacy I/O port free.

Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>

---
 drivers/net/e1000/e1000.h      |    5 +
 drivers/net/e1000/e1000_main.c |  109 +++++++++++++++++++++--------------------
 2 files changed, 61 insertions(+), 53 deletions(-)

Index: linux-2.6.16-rc5-mm1/drivers/net/e1000/e1000.h
===================================================================
--- linux-2.6.16-rc5-mm1.orig/drivers/net/e1000/e1000.h	2006-03-01 13:56:04.000000000 +0900
+++ linux-2.6.16-rc5-mm1/drivers/net/e1000/e1000.h	2006-03-01 16:36:34.000000000 +0900
@@ -77,8 +77,9 @@
 #define BAR_1		1
 #define BAR_5		5
 
-#define INTEL_E1000_ETHERNET_DEVICE(device_id) {\
-	PCI_DEVICE(PCI_VENDOR_ID_INTEL, device_id)}
+#define E1000_NO_IOPORT	(1 << 0)
+#define INTEL_E1000_ETHERNET_DEVICE(device_id, flags) {\
+	PCI_DEVICE(PCI_VENDOR_ID_INTEL, device_id), .driver_data = flags}
 
 struct e1000_adapter;
 
Index: linux-2.6.16-rc5-mm1/drivers/net/e1000/e1000_main.c
===================================================================
--- linux-2.6.16-rc5-mm1.orig/drivers/net/e1000/e1000_main.c	2006-03-01 13:56:04.000000000 +0900
+++ linux-2.6.16-rc5-mm1/drivers/net/e1000/e1000_main.c	2006-03-01 16:36:34.000000000 +0900
@@ -115,51 +115,51 @@
  *   {PCI_DEVICE(PCI_VENDOR_ID_INTEL, device_id)}
  */
 static struct pci_device_id e1000_pci_tbl[] = {
-	INTEL_E1000_ETHERNET_DEVICE(0x1000),
-	INTEL_E1000_ETHERNET_DEVICE(0x1001),
-	INTEL_E1000_ETHERNET_DEVICE(0x1004),
-	INTEL_E1000_ETHERNET_DEVICE(0x1008),
-	INTEL_E1000_ETHERNET_DEVICE(0x1009),
-	INTEL_E1000_ETHERNET_DEVICE(0x100C),
-	INTEL_E1000_ETHERNET_DEVICE(0x100D),
-	INTEL_E1000_ETHERNET_DEVICE(0x100E),
-	INTEL_E1000_ETHERNET_DEVICE(0x100F),
-	INTEL_E1000_ETHERNET_DEVICE(0x1010),
-	INTEL_E1000_ETHERNET_DEVICE(0x1011),
-	INTEL_E1000_ETHERNET_DEVICE(0x1012),
-	INTEL_E1000_ETHERNET_DEVICE(0x1013),
-	INTEL_E1000_ETHERNET_DEVICE(0x1014),
-	INTEL_E1000_ETHERNET_DEVICE(0x1015),
-	INTEL_E1000_ETHERNET_DEVICE(0x1016),
-	INTEL_E1000_ETHERNET_DEVICE(0x1017),
-	INTEL_E1000_ETHERNET_DEVICE(0x1018),
-	INTEL_E1000_ETHERNET_DEVICE(0x1019),
-	INTEL_E1000_ETHERNET_DEVICE(0x101A),
-	INTEL_E1000_ETHERNET_DEVICE(0x101D),
-	INTEL_E1000_ETHERNET_DEVICE(0x101E),
-	INTEL_E1000_ETHERNET_DEVICE(0x1026),
-	INTEL_E1000_ETHERNET_DEVICE(0x1027),
-	INTEL_E1000_ETHERNET_DEVICE(0x1028),
-	INTEL_E1000_ETHERNET_DEVICE(0x105E),
-	INTEL_E1000_ETHERNET_DEVICE(0x105F),
-	INTEL_E1000_ETHERNET_DEVICE(0x1060),
-	INTEL_E1000_ETHERNET_DEVICE(0x1075),
-	INTEL_E1000_ETHERNET_DEVICE(0x1076),
-	INTEL_E1000_ETHERNET_DEVICE(0x1077),
-	INTEL_E1000_ETHERNET_DEVICE(0x1078),
-	INTEL_E1000_ETHERNET_DEVICE(0x1079),
-	INTEL_E1000_ETHERNET_DEVICE(0x107A),
-	INTEL_E1000_ETHERNET_DEVICE(0x107B),
-	INTEL_E1000_ETHERNET_DEVICE(0x107C),
-	INTEL_E1000_ETHERNET_DEVICE(0x107D),
-	INTEL_E1000_ETHERNET_DEVICE(0x107E),
-	INTEL_E1000_ETHERNET_DEVICE(0x107F),
-	INTEL_E1000_ETHERNET_DEVICE(0x108A),
-	INTEL_E1000_ETHERNET_DEVICE(0x108B),
-	INTEL_E1000_ETHERNET_DEVICE(0x108C),
-	INTEL_E1000_ETHERNET_DEVICE(0x1099),
-	INTEL_E1000_ETHERNET_DEVICE(0x109A),
-	INTEL_E1000_ETHERNET_DEVICE(0x10B5),
+	INTEL_E1000_ETHERNET_DEVICE(0x1000, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1001, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1004, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1008, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1009, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x100C, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x100D, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x100E, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x100F, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1010, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1011, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1012, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1013, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1014, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1015, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1016, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1017, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1018, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1019, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x101A, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x101D, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x101E, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1026, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1027, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1028, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x105E, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x105F, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1060, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1075, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1076, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1077, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1078, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1079, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x107A, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x107B, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x107C, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x107D, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x107E, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x107F, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x108A, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x108B, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x108C, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1099, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x109A, E1000_NO_IOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x10B5, E1000_NO_IOPORT),
 	/* required last entry */
 	{0,}
 };
@@ -652,6 +652,10 @@
 	int i, err, pci_using_dac;
 	uint16_t eeprom_data;
 	uint16_t eeprom_apme_mask = E1000_EEPROM_APME;
+
+	/* See if the device needs I/O port */
+	pdev->no_ioport = !!(ent->driver_data & E1000_NO_IOPORT);
+
 	if ((err = pci_enable_device(pdev)))
 		return err;
 
@@ -695,12 +699,15 @@
 		goto err_ioremap;
 	}
 
-	for (i = BAR_1; i <= BAR_5; i++) {
-		if (pci_resource_len(pdev, i) == 0)
-			continue;
-		if (pci_resource_flags(pdev, i) & IORESOURCE_IO) {
-			adapter->hw.io_base = pci_resource_start(pdev, i);
-			break;
+	if (!(ent->driver_data & E1000_NO_IOPORT)) {
+		for (i = BAR_1; i <= BAR_5; i++) {
+			if (pci_resource_len(pdev, i) == 0)
+				continue;
+			if (pci_resource_flags(pdev, i) & IORESOURCE_IO) {
+				adapter->hw.io_base =
+					pci_resource_start(pdev, i);
+				break;
+			}
 		}
 	}
 

[PATCH 4/4] PCI legacy I/O port free driver (take4) - Make Emulex lpfc driver legacy I/O port free

[Kenji Kaneshige]

This patch makes Emulex lpfc driver legacy I/O port free.

Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>

---
 drivers/scsi/lpfc/lpfc_init.c |    3 +++
 1 files changed, 3 insertions(+)

Index: linux-2.6.16-rc5-mm1/drivers/scsi/lpfc/lpfc_init.c
===================================================================
--- linux-2.6.16-rc5-mm1.orig/drivers/scsi/lpfc/lpfc_init.c	2006-03-01 13:53:27.000000000 +0900
+++ linux-2.6.16-rc5-mm1/drivers/scsi/lpfc/lpfc_init.c	2006-03-01 16:37:09.000000000 +0900
@@ -1368,6 +1368,9 @@
 	int i;
 	uint16_t iotag;
 
+	/* Don't need to use I/O port regions */
+	pdev->no_ioport = 1;
+
 	if (pci_enable_device(pdev))
 		goto out;
 	if (pci_request_regions(pdev, LPFC_DRIVER_NAME))

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Russell King]

On Fri, Mar 03, 2006 at 12:12:34AM +0900, Kenji Kaneshige wrote:
> Hi Andrew, Greg,
> 
> Here is an updated set of patches for PCI legacy I/O port free
> driver. It incorporates all of the feedbacks. I rebased it against the
> latest -mm kernel (2.6.16-rc5-mm1).
> 
> Could you consider applying this to -mm tree?

I've been wondering whether this "no_ioport" flag is the correct approach,
or whether it's adding to complexity when it isn't really required.

In the non-Intel world, the kernel itself sets up the PCI bus mappings,
and any IO bars which it can't satisfy might also need to be gracefully
handled.  Currently, we just go 'printk("whoops, didn't allocate
resource")' and leave the BAR containing whatever random junk it
contained before, along with the resource containing whatever random
junk pci_bus_alloc_resource() decided to leave in it.

In such cases, I would suggest that the method of signalling that IO
should not be used is to have the IO resource structures cleared out -
if the IO resources aren't valid, they should not contain something
which could be interpreted as valid.

Maybe something like this should be done for the "legacy IO port" case
as well?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Kenji Kaneshige]

Hi,

It seems we need a little more discussion.
Please don't apply my patches >> Andrew, Greg

Russel, I'm sorry, but could you wait some hours for my reply?
I noticed the email from you when I was about to go to bed...

Thanks,
Kenji Kaneshige


Russell King wrote:
> On Fri, Mar 03, 2006 at 12:12:34AM +0900, Kenji Kaneshige wrote:
> 
>>Hi Andrew, Greg,
>>
>>Here is an updated set of patches for PCI legacy I/O port free
>>driver. It incorporates all of the feedbacks. I rebased it against the
>>latest -mm kernel (2.6.16-rc5-mm1).
>>
>>Could you consider applying this to -mm tree?
> 
> 
> I've been wondering whether this "no_ioport" flag is the correct approach,
> or whether it's adding to complexity when it isn't really required.
> 
> In the non-Intel world, the kernel itself sets up the PCI bus mappings,
> and any IO bars which it can't satisfy might also need to be gracefully
> handled.  Currently, we just go 'printk("whoops, didn't allocate
> resource")' and leave the BAR containing whatever random junk it
> contained before, along with the resource containing whatever random
> junk pci_bus_alloc_resource() decided to leave in it.
> 
> In such cases, I would suggest that the method of signalling that IO
> should not be used is to have the IO resource structures cleared out -
> if the IO resources aren't valid, they should not contain something
> which could be interpreted as valid.
> 
> Maybe something like this should be done for the "legacy IO port" case
> as well?
> 

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Greg KH]

On Fri, Mar 03, 2006 at 01:23:51AM +0900, Kenji Kaneshige wrote:
> Hi,
> 
> It seems we need a little more discussion.
> Please don't apply my patches >> Andrew, Greg

Don't worry, once all of the discussion settles down, I'll add this to
my tree, and then Andrew will pick it up from there :)

thanks,

greg k-h

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Grant Grundler]

On Thu, Mar 02, 2006 at 03:50:57PM +0000, Russell King wrote:
> I've been wondering whether this "no_ioport" flag is the correct approach,
> or whether it's adding to complexity when it isn't really required.

I think it's the simplest solution to allowing a driver
to indicate which resources it wants to use. It solves
the problem of I/O Port resource allocation sufficiently
well.

> In the non-Intel world, the kernel itself sets up the PCI bus mappings,

mappings, yes. But many of the architectures depend on (or assume)
firmware will assign appropriate resources. The problem is firmware
runs out of I/O Port resources.

> and any IO bars which it can't satisfy might also need to be gracefully
> handled.  Currently, we just go 'printk("whoops, didn't allocate
> resource")' and leave the BAR containing whatever random junk it
> contained before, along with the resource containing whatever random
> junk pci_bus_alloc_resource() decided to leave in it.
> 
> In such cases, I would suggest that the method of signalling that IO
> should not be used is to have the IO resource structures cleared out -
> if the IO resources aren't valid, they should not contain something
> which could be interpreted as valid.

You want the arch PCI support to clobber the I/O port space resources?
How will the arch PCI support know that a particular device's driver
only uses MMIO resources?

> Maybe something like this should be done for the "legacy IO port" case
> as well?

Several people have already agreed the driver needs to indicate which
resource it wants to work with. I don't see how the arch PCI support
can provide that "knowledge".

hth,
grant

> 
> -- 
> Russell King
>  Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
>  maintainer of:  2.6 Serial core

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Russell King]

On Thu, Mar 02, 2006 at 10:24:36AM -0700, Grant Grundler wrote:
> On Thu, Mar 02, 2006 at 03:50:57PM +0000, Russell King wrote:
> > I've been wondering whether this "no_ioport" flag is the correct approach,
> > or whether it's adding to complexity when it isn't really required.
> 
> I think it's the simplest solution to allowing a driver
> to indicate which resources it wants to use. It solves
> the problem of I/O Port resource allocation sufficiently
> well.

It's not really "I/O port resource allocation" though - the resources
have already been allocated and potentially programmed into the BARs
well before the driver gets anywhere near the device.

> > In the non-Intel world, the kernel itself sets up the PCI bus mappings,
> 
> mappings, yes. But many of the architectures depend on (or assume)
> firmware will assign appropriate resources. The problem is firmware
> runs out of I/O Port resources.

Are you implying that somehow resources are allocated at pci_enable_device
time?  If so, shouldn't we be thinking of moving completely to that model
rather than having yet-another-pci-setup-model.

Let's see - we have the i386 method, the drivers/pci/setup-* method, and
it sounds like some pci_enable_device() method now.

The problem is that the "no_ioport" method is completely useless for the
drivers/pci/setup-* method since that information from the drivers comes
well after the PCI setup code has been run.

> Several people have already agreed the driver needs to indicate which
> resource it wants to work with. I don't see how the arch PCI support
> can provide that "knowledge".

What I'm saying is that we need to unify this stuff, rather than keep
bluring and overloading the interfaces.  Otherwise we'll end up with
drivers developed on one platform which have one expectation from the
PCI API, which could potentially be different from drivers developed
on some other platform.

Let's have some uniformity - and a solution which can fit everyone -
that's all I'm asking.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Jeff Garzik]

Russell King wrote:
> It's not really "I/O port resource allocation" though - the resources
> have already been allocated and potentially programmed into the BARs
> well before the driver gets anywhere near the device.
[...]
> Are you implying that somehow resources are allocated at pci_enable_device
> time?  If so, shouldn't we be thinking of moving completely to that model
> rather than having yet-another-pci-setup-model.

Actually, that's has been the rule ever since the cardbus days: 
resources -- bars and irqs -- should not be considered allocated until 
after pci_enable_device().

Documentation/pci.txt reflects this reality as well:

> 3. Enabling and disabling devices
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>    Before you do anything with the device you've found, you need to enable
> it by calling pci_enable_device() which enables I/O and memory regions of
> the device, allocates an IRQ if necessary, assigns missing resources if
> needed and wakes up the device if it was in suspended state. Please note
> that this function can fail.

Any PCI driver that presumes -anything- about resources before calling 
pci_enable_device() is buggy, and that's been the case for many years. 
Some platform-specific PCI drivers violate this with special knowledge, 
but overall that's the rule.

Regards,

	Jeff

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Russell King]

On Thu, Mar 02, 2006 at 01:12:35PM -0500, Jeff Garzik wrote:
> Russell King wrote:
> >It's not really "I/O port resource allocation" though - the resources
> >have already been allocated and potentially programmed into the BARs
> >well before the driver gets anywhere near the device.
> [...]
> >Are you implying that somehow resources are allocated at pci_enable_device
> >time?  If so, shouldn't we be thinking of moving completely to that model
> >rather than having yet-another-pci-setup-model.
> 
> Actually, that's has been the rule ever since the cardbus days: 
> resources -- bars and irqs -- should not be considered allocated until 
> after pci_enable_device().

However, practically, cardbus have always had and continue to have their
resources setup prior to driver probe.

> Documentation/pci.txt reflects this reality as well:
> 
> >3. Enabling and disabling devices
> >~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >   Before you do anything with the device you've found, you need to enable
> >it by calling pci_enable_device() which enables I/O and memory regions of
> >the device, allocates an IRQ if necessary, assigns missing resources if
> >needed and wakes up the device if it was in suspended state. Please note
> >that this function can fail.
> 
> Any PCI driver that presumes -anything- about resources before calling 
> pci_enable_device() is buggy, and that's been the case for many years. 
> Some platform-specific PCI drivers violate this with special knowledge, 
> but overall that's the rule.

Nevertheless, my basic point that the no_ioport solution only addresses
one problem area, while being far too late for the other methods still
stands.

Maybe the setup-* stuff needs to be rewritten (in which case cardbus
will also convert to the "new" system automatically)?

Given the problems of allocating resources for bridges inside cardbus
cards, it would be nice if this could happen, so the limited IO range
provided to cardbus could be more efficiently (and selectively) used.

Wouldn't you think that's a sane idea?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Grant Grundler]

On Thu, Mar 02, 2006 at 06:00:25PM +0000, Russell King wrote:
> It's not really "I/O port resource allocation" though - the resources
> have already been allocated and potentially programmed into the BARs
> well before the driver gets anywhere near the device.

Agreed.

> Are you implying that somehow resources are allocated at pci_enable_device
> time?

No. The driver just wants to ignore those BARs regardless of whether
they have been programmed or not.

>   If so, shouldn't we be thinking of moving completely to that model
> rather than having yet-another-pci-setup-model.
> 
> Let's see - we have the i386 method, the drivers/pci/setup-* method, and
> it sounds like some pci_enable_device() method now.

It does give the OS the opportunity to "recycle" the I/O Port resources
at pci_enable_device() time. But this patch doesn't try to do that.

> The problem is that the "no_ioport" method is completely useless for the
> drivers/pci/setup-* method since that information from the drivers comes
> well after the PCI setup code has been run.

Agreed.

> 
> > Several people have already agreed the driver needs to indicate which
> > resource it wants to work with. I don't see how the arch PCI support
> > can provide that "knowledge".
> 
> What I'm saying is that we need to unify this stuff, rather than keep
> bluring and overloading the interfaces.  Otherwise we'll end up with
> drivers developed on one platform which have one expectation from the
> PCI API, which could potentially be different from drivers developed
> on some other platform.

*nod*. Have pci resource allocation take place at any time *after*
the PCI bus walks has it's own set of challenges. In particular,
changing bridge windows or reassigning value to devices which
have already been enabled. Dealing with PCI quirks is another
can of worms.

> Let's have some uniformity - and a solution which can fit everyone -
> that's all I'm asking.

Since the changes are primarily to generic PCI code,
how is this not uniform across arches?
Or am I thinking about this the wrong way?

thanks,
grant

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Russell King]

On Thu, Mar 02, 2006 at 10:24:36AM -0700, Grant Grundler wrote:
> On Thu, Mar 02, 2006 at 03:50:57PM +0000, Russell King wrote:
> > I've been wondering whether this "no_ioport" flag is the correct approach,
> > or whether it's adding to complexity when it isn't really required.
> 
> I think it's the simplest solution to allowing a driver
> to indicate which resources it wants to use. It solves
> the problem of I/O Port resource allocation sufficiently
> well.

I have another question (brought up by someone working on a series of
ARM machines which make heavy use of MMIO.)

Why isn't pci_enable_device_bars() sufficient - why do we have to
have another interface to say "we don't want BARs XXX" ?

Let's say that we have a device driver which does this sequence (with,
of course, error checking):

	pci_enable_device_bars(dev, 1<<1);
	pci_request_regions(dev);

(a) should PCI remember that only BAR 1 has been requested to be enabled,
    and as such shouldn't pci_request_regions() ignore BAR 0?

(b) should the PCI driver pass into pci_request_regions() (or even
    pci_request_regions_bars()) a bitmask of the BARs it wants to have
    requested, and similarly for pci_release_regions().

Basically, if BAR0 hasn't been enabled, has pci_request_regions() got
any business requesting it from the resource tree?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Roland Dreier]

    Russell> Why isn't pci_enable_device_bars() sufficient - why do we
    Russell> have to have another interface to say "we don't want BARs
    Russell> XXX" ?

That's an excellent point.  It does look like fixing
pci_enable_device_bars() would be a reasonable interface as well.
Currently, pci_enable_device() calls pci_enable_device_bars() on all
resources, and then does two more things:

    - It calls pci_fixup_device() on the device
    - It sets dev->is_enabled

Both of these things look like they could just be moved into
pci_enable_device_bars(), leaving pci_enable_device() as a
compatibility wrapper.

Along with a fix for pci_request_regions(), the legacy-free patch
would just modify drivers for devices that know they don't need a
particular BAR, without adding an extra flag to the pci device struct.

 - R.

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Jeff Garzik]

Russell King wrote:
> Nevertheless, my basic point that the no_ioport solution only addresses
> one problem area, while being far too late for the other methods still
> stands.

Agreed.

FWIW, as a PCI driver writer I've always wanted a one-call-does-it-all 
interface, where you pass in a struct that describes
* what BARs to map (pci_iomap/ioremap, pci_request_regions)
* irq handler (request_irq)
* enable bus mastering, MWI (pci_set_master, pci_set_mwi)

to collapse the billion-and-one pci_xxx calls into a single one that 
replaces pci_enable_device(dev) with pci_up(dev, &info).

	Jeff

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Kenji Kaneshige]

Russell King wrote:
> On Thu, Mar 02, 2006 at 10:24:36AM -0700, Grant Grundler wrote:
> 
>>On Thu, Mar 02, 2006 at 03:50:57PM +0000, Russell King wrote:
>>
>>>I've been wondering whether this "no_ioport" flag is the correct approach,
>>>or whether it's adding to complexity when it isn't really required.
>>
>>I think it's the simplest solution to allowing a driver
>>to indicate which resources it wants to use. It solves
>>the problem of I/O Port resource allocation sufficiently
>>well.
> 
> 
> I have another question (brought up by someone working on a series of
> ARM machines which make heavy use of MMIO.)
> 
> Why isn't pci_enable_device_bars() sufficient - why do we have to
> have another interface to say "we don't want BARs XXX" ?
> 
> Let's say that we have a device driver which does this sequence (with,
> of course, error checking):
> 
> 	pci_enable_device_bars(dev, 1<<1);
> 	pci_request_regions(dev);
> 
> (a) should PCI remember that only BAR 1 has been requested to be enabled,
>     and as such shouldn't pci_request_regions() ignore BAR 0?
> 
> (b) should the PCI driver pass into pci_request_regions() (or even
>     pci_request_regions_bars()) a bitmask of the BARs it wants to have
>     requested, and similarly for pci_release_regions().
> 
> Basically, if BAR0 hasn't been enabled, has pci_request_regions() got
> any business requesting it from the resource tree?
> 

Hmm, pci_enable_device_bars() approach looks better and better to me.
If we use this approach, we need to consider which (a) and (b) we
should use, as you said.

The (b) option looks natural to me because pci_enable_device_bars() and
pci_request_regions_bars() becomes a pair. But the (b) option would have
a problem at resume time. If the driver doesn't have .resume() method,
default resume routine (i.e. pci_default_resume()) will be called, and
it calls pci_enable_device().

The (a) option doesn't have this kind of problem. But it looks a little
strange to me that we use pci_enable_device_bars() at probe time while
we use pci_enable_device() at resume time, though I might be too anxious.

To tell the truth, I've considered this before. I could not come up with
which is better between (a) and (b), and then I posted another approach
which introduce a new API. Now I came back to this approach again. Maybe
I'm being caught by the endless loop...

I would very much appreciate giving me any comments about (a) and (b).

Thanks,
Kenji Kaneshige

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Kenji Kaneshige]

Kenji Kaneshige wrote:
> Russell King wrote:

(snip.)

>>
>> (a) should PCI remember that only BAR 1 has been requested to be enabled,
>>     and as such shouldn't pci_request_regions() ignore BAR 0?
>>
>> (b) should the PCI driver pass into pci_request_regions() (or even
>>     pci_request_regions_bars()) a bitmask of the BARs it wants to have
>>     requested, and similarly for pci_release_regions().
>>
>> Basically, if BAR0 hasn't been enabled, has pci_request_regions() got
>> any business requesting it from the resource tree?
>>
> 

(snip.)

> 
> The (a) option doesn't have this kind of problem. But it looks a little
> strange to me that we use pci_enable_device_bars() at probe time while
> we use pci_enable_device() at resume time, though I might be too anxious.
> 

I noticed this is not a problem. All we need to do is just replacing
pci_enable_device() with pci_enable_device_bars() in pci_default_resume().
Sorry for the noise.

BTW, I'm attaching the sample patch which implements option (a), though
I've not tested it at all. It looks good to me. How does it looks?

Thanks,
Kenji Kaneshige


---
 drivers/pci/pci-driver.c |    3 ++-
 drivers/pci/pci.c        |   30 +++++++++++++++++++++++-------
 include/linux/pci.h      |   12 ++++++++++++
 3 files changed, 37 insertions(+), 8 deletions(-)

Index: linux-2.6.16-rc5-mm1/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.16-rc5-mm1.orig/drivers/pci/pci-driver.c	2006-03-01 13:56:04.000000000 +0900
+++ linux-2.6.16-rc5-mm1/drivers/pci/pci-driver.c	2006-03-03 14:39:57.000000000 +0900
@@ -294,7 +294,8 @@
 	pci_restore_state(pci_dev);
 	/* if the device was enabled before suspend, reenable */
 	if (pci_dev->is_enabled)
-		retval = pci_enable_device(pci_dev);
+		retval = pci_enable_device_bars(pci_dev,
+						pci_dev->bars_enabled);
 	/* if the device was busmaster before the suspend, make it busmaster again */
 	if (pci_dev->is_busmaster)
 		pci_set_master(pci_dev);
Index: linux-2.6.16-rc5-mm1/drivers/pci/pci.c
===================================================================
--- linux-2.6.16-rc5-mm1.orig/drivers/pci/pci.c	2006-03-01 13:56:04.000000000 +0900
+++ linux-2.6.16-rc5-mm1/drivers/pci/pci.c	2006-03-03 15:46:12.000000000 +0900
@@ -493,6 +493,9 @@
 	err = pcibios_enable_device(dev, bars);
 	if (err < 0)
 		return err;
+	pci_fixup_device(pci_fixup_enable, dev);
+	dev->is_enabled = 1;
+	dev->bars_enabled = bars;
 	return 0;
 }
 
@@ -510,8 +513,6 @@
 	int err = pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1);
 	if (err)
 		return err;
-	pci_fixup_device(pci_fixup_enable, dev);
-	dev->is_enabled = 1;
 	return 0;
 }
 
@@ -546,6 +547,7 @@
 
 	pcibios_disable_device(dev);
 	dev->is_enabled = 0;
+	dev->bars_enabled = 0;
 }
 
 /**
@@ -628,6 +630,12 @@
 {
 	if (pci_resource_len(pdev, bar) == 0)
 		return;
+	if (pdev->bars_enabled & (1 << bar)) {
+		dev_warn(&pdev->dev,
+			 "Trying to release region #%d that is not enabled\n",
+			 bar);
+		return;
+	}
 	if (pci_resource_flags(pdev, bar) & IORESOURCE_IO)
 		release_region(pci_resource_start(pdev, bar),
 				pci_resource_len(pdev, bar));
@@ -654,7 +662,12 @@
 {
 	if (pci_resource_len(pdev, bar) == 0)
 		return 0;
-		
+	if (pdev->bars_enabled & (1 << bar)) {
+		dev_warn(&pdev->dev,
+			 "Trying to request region #%d that is not enabled\n",
+			 bar);
+		goto err_out;
+	}
 	if (pci_resource_flags(pdev, bar) & IORESOURCE_IO) {
 		if (!request_region(pci_resource_start(pdev, bar),
 			    pci_resource_len(pdev, bar), res_name))
@@ -692,7 +705,8 @@
 	int i;
 	
 	for (i = 0; i < 6; i++)
-		pci_release_region(pdev, i);
+		if (pdev->bars_enabled & (1 << i))
+			pci_release_region(pdev, i);
 }
 
 /**
@@ -713,13 +727,15 @@
 	int i;
 	
 	for (i = 0; i < 6; i++)
-		if(pci_request_region(pdev, i, res_name))
-			goto err_out;
+		if (pdev->bars_enabled & (1 << i))
+			if(pci_request_region(pdev, i, res_name))
+				goto err_out;
 	return 0;
 
 err_out:
 	while(--i >= 0)
-		pci_release_region(pdev, i);
+		if (pdev->bars_enabled & (1 << i))
+			pci_release_region(pdev, i);
 		
 	return -EBUSY;
 }
Index: linux-2.6.16-rc5-mm1/include/linux/pci.h
===================================================================
--- linux-2.6.16-rc5-mm1.orig/include/linux/pci.h	2006-03-01 13:56:06.000000000 +0900
+++ linux-2.6.16-rc5-mm1/include/linux/pci.h	2006-03-03 15:31:58.000000000 +0900
@@ -169,6 +169,7 @@
 	struct bin_attribute *rom_attr; /* attribute descriptor for sysfs ROM entry */
 	int rom_attr_enabled;		/* has display of the rom attribute been enabled? */
 	struct bin_attribute *res_attr[DEVICE_COUNT_RESOURCE]; /* sysfs file for resources */
+	int	bars_enabled;		/* BARs enabled */
 };
 
 #define pci_dev_g(n) list_entry(n, struct pci_dev, global_list)
@@ -732,6 +733,17 @@
 }
 #endif /* HAVE_ARCH_PCI_RESOURCE_TO_USER */
 
+/*
+ * This helper routine makes bar mask from the type of resource.
+ */
+static inline int pci_select_bars(struct pci_dev *dev, unsigned long flags)
+{
+	int i, bars = 0;
+	for (i = 0; i < PCI_NUM_RESOURCES; i++)
+		if (pci_resource_flags(dev, i) & flags)
+			bars |= (1 << i);
+	return bars;
+}
 
 /*
  *  The world is not perfect and supplies us with broken PCI devices.

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Kenji Kaneshige]

Hi,

If there are no objections to the pci_enable_device_bars()
approach for PCI legacy ioport free drivers, I'd start to
reimplement the patches based on this approach.
Does anybody have any objections?

The summary of changes in this approach is as follows:

    - Add new field 'bars_enabled' into struct pci_dev to
      remember which BARs already enabled. This new field
      is initialized at pci_enable_device_bars() time and
      cleared pci_disable_device() time.

    - Change pci_request_regions()/pci_release_regions() to
      requests only the BARs which have already been enabled.

    - For converting drivers to legacy I/O port free, change
      drivers to use pci_enable_device_bars() instead of
      pci_enable_device() in order not to enable ioport regions.
      The helper routine to make proper BAR mask from the specified
      resource type would be very helpful (please see the sample
      patche attached in the previous e-mail below).

Thanks,
Kenji Kaneshige


Kenji Kaneshige wrote:
> Kenji Kaneshige wrote:
> 
>> Russell King wrote:
> 
> 
> (snip.)
> 
>>>
>>> (a) should PCI remember that only BAR 1 has been requested to be 
>>> enabled,
>>>     and as such shouldn't pci_request_regions() ignore BAR 0?
>>>
>>> (b) should the PCI driver pass into pci_request_regions() (or even
>>>     pci_request_regions_bars()) a bitmask of the BARs it wants to have
>>>     requested, and similarly for pci_release_regions().
>>>
>>> Basically, if BAR0 hasn't been enabled, has pci_request_regions() got
>>> any business requesting it from the resource tree?
>>>
>>
> 
> (snip.)
> 
>>
>> The (a) option doesn't have this kind of problem. But it looks a little
>> strange to me that we use pci_enable_device_bars() at probe time while
>> we use pci_enable_device() at resume time, though I might be too anxious.
>>
> 
> I noticed this is not a problem. All we need to do is just replacing
> pci_enable_device() with pci_enable_device_bars() in pci_default_resume().
> Sorry for the noise.
> 
> BTW, I'm attaching the sample patch which implements option (a), though
> I've not tested it at all. It looks good to me. How does it looks?
> 
> Thanks,
> Kenji Kaneshige
> 
> 
> ---
> drivers/pci/pci-driver.c |    3 ++-
> drivers/pci/pci.c        |   30 +++++++++++++++++++++++-------
> include/linux/pci.h      |   12 ++++++++++++
> 3 files changed, 37 insertions(+), 8 deletions(-)
> 
> Index: linux-2.6.16-rc5-mm1/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-2.6.16-rc5-mm1.orig/drivers/pci/pci-driver.c    2006-03-01 
> 13:56:04.000000000 +0900
> +++ linux-2.6.16-rc5-mm1/drivers/pci/pci-driver.c    2006-03-03 
> 14:39:57.000000000 +0900
> @@ -294,7 +294,8 @@
>     pci_restore_state(pci_dev);
>     /* if the device was enabled before suspend, reenable */
>     if (pci_dev->is_enabled)
> -        retval = pci_enable_device(pci_dev);
> +        retval = pci_enable_device_bars(pci_dev,
> +                        pci_dev->bars_enabled);
>     /* if the device was busmaster before the suspend, make it busmaster 
> again */
>     if (pci_dev->is_busmaster)
>         pci_set_master(pci_dev);
> Index: linux-2.6.16-rc5-mm1/drivers/pci/pci.c
> ===================================================================
> --- linux-2.6.16-rc5-mm1.orig/drivers/pci/pci.c    2006-03-01 
> 13:56:04.000000000 +0900
> +++ linux-2.6.16-rc5-mm1/drivers/pci/pci.c    2006-03-03 
> 15:46:12.000000000 +0900
> @@ -493,6 +493,9 @@
>     err = pcibios_enable_device(dev, bars);
>     if (err < 0)
>         return err;
> +    pci_fixup_device(pci_fixup_enable, dev);
> +    dev->is_enabled = 1;
> +    dev->bars_enabled = bars;
>     return 0;
> }
> 
> @@ -510,8 +513,6 @@
>     int err = pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1);
>     if (err)
>         return err;
> -    pci_fixup_device(pci_fixup_enable, dev);
> -    dev->is_enabled = 1;
>     return 0;
> }
> 
> @@ -546,6 +547,7 @@
> 
>     pcibios_disable_device(dev);
>     dev->is_enabled = 0;
> +    dev->bars_enabled = 0;
> }
> 
> /**
> @@ -628,6 +630,12 @@
> {
>     if (pci_resource_len(pdev, bar) == 0)
>         return;
> +    if (pdev->bars_enabled & (1 << bar)) {
> +        dev_warn(&pdev->dev,
> +             "Trying to release region #%d that is not enabled\n",
> +             bar);
> +        return;
> +    }
>     if (pci_resource_flags(pdev, bar) & IORESOURCE_IO)
>         release_region(pci_resource_start(pdev, bar),
>                 pci_resource_len(pdev, bar));
> @@ -654,7 +662,12 @@
> {
>     if (pci_resource_len(pdev, bar) == 0)
>         return 0;
> -       
> +    if (pdev->bars_enabled & (1 << bar)) {
> +        dev_warn(&pdev->dev,
> +             "Trying to request region #%d that is not enabled\n",
> +             bar);
> +        goto err_out;
> +    }
>     if (pci_resource_flags(pdev, bar) & IORESOURCE_IO) {
>         if (!request_region(pci_resource_start(pdev, bar),
>                 pci_resource_len(pdev, bar), res_name))
> @@ -692,7 +705,8 @@
>     int i;
>     
>     for (i = 0; i < 6; i++)
> -        pci_release_region(pdev, i);
> +        if (pdev->bars_enabled & (1 << i))
> +            pci_release_region(pdev, i);
> }
> 
> /**
> @@ -713,13 +727,15 @@
>     int i;
>     
>     for (i = 0; i < 6; i++)
> -        if(pci_request_region(pdev, i, res_name))
> -            goto err_out;
> +        if (pdev->bars_enabled & (1 << i))
> +            if(pci_request_region(pdev, i, res_name))
> +                goto err_out;
>     return 0;
> 
> err_out:
>     while(--i >= 0)
> -        pci_release_region(pdev, i);
> +        if (pdev->bars_enabled & (1 << i))
> +            pci_release_region(pdev, i);
>        
>     return -EBUSY;
> }
> Index: linux-2.6.16-rc5-mm1/include/linux/pci.h
> ===================================================================
> --- linux-2.6.16-rc5-mm1.orig/include/linux/pci.h    2006-03-01 
> 13:56:06.000000000 +0900
> +++ linux-2.6.16-rc5-mm1/include/linux/pci.h    2006-03-03 
> 15:31:58.000000000 +0900
> @@ -169,6 +169,7 @@
>     struct bin_attribute *rom_attr; /* attribute descriptor for sysfs 
> ROM entry */
>     int rom_attr_enabled;        /* has display of the rom attribute 
> been enabled? */
>     struct bin_attribute *res_attr[DEVICE_COUNT_RESOURCE]; /* sysfs file 
> for resources */
> +    int    bars_enabled;        /* BARs enabled */
> };
> 
> #define pci_dev_g(n) list_entry(n, struct pci_dev, global_list)
> @@ -732,6 +733,17 @@
> }
> #endif /* HAVE_ARCH_PCI_RESOURCE_TO_USER */
> 
> +/*
> + * This helper routine makes bar mask from the type of resource.
> + */
> +static inline int pci_select_bars(struct pci_dev *dev, unsigned long 
> flags)
> +{
> +    int i, bars = 0;
> +    for (i = 0; i < PCI_NUM_RESOURCES; i++)
> +        if (pci_resource_flags(dev, i) & flags)
> +            bars |= (1 << i);
> +    return bars;
> +}
> 
> /*
>  *  The world is not perfect and supplies us with broken PCI devices.
> 

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Adam Belay]

On Thu, Mar 02, 2006 at 07:34:41PM +0000, Russell King wrote:
> On Thu, Mar 02, 2006 at 10:24:36AM -0700, Grant Grundler wrote:
> > On Thu, Mar 02, 2006 at 03:50:57PM +0000, Russell King wrote:
> > > I've been wondering whether this "no_ioport" flag is the correct approach,
> > > or whether it's adding to complexity when it isn't really required.
> > 
> > I think it's the simplest solution to allowing a driver
> > to indicate which resources it wants to use. It solves
> > the problem of I/O Port resource allocation sufficiently
> > well.
> 
> I have another question (brought up by someone working on a series of
> ARM machines which make heavy use of MMIO.)
> 
> Why isn't pci_enable_device_bars() sufficient - why do we have to
> have another interface to say "we don't want BARs XXX" ?
> 
> Let's say that we have a device driver which does this sequence (with,
> of course, error checking):
> 
> 	pci_enable_device_bars(dev, 1<<1);
> 	pci_request_regions(dev);
> 
> (a) should PCI remember that only BAR 1 has been requested to be enabled,
>     and as such shouldn't pci_request_regions() ignore BAR 0?
> 
> (b) should the PCI driver pass into pci_request_regions() (or even
>     pci_request_regions_bars()) a bitmask of the BARs it wants to have
>     requested, and similarly for pci_release_regions().
> 
> Basically, if BAR0 hasn't been enabled, has pci_request_regions() got
> any business requesting it from the resource tree?

I understand the point you're making, but I think this misrepresents what
is actually happening.  From my understanding of the spec, it's not possible
to disable individual bars (with the exception of the expansion ROM).  Rather
there is one bit for IO enable and one bit for IOMMU enable.  Therefore, we
can enable or disable all I/O ports, but there's really no in between.  If
the device uses even one I/O port, it's still a huge loss because of the
potential bridge window dependency.  Also, if a device has several I/O ports
but the driver only wants to use one, all of the others must still be
assigned.

Thanks,
Adam

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Kenji Kaneshige]

Adam Belay wrote:
> On Thu, Mar 02, 2006 at 07:34:41PM +0000, Russell King wrote:
> 
>>On Thu, Mar 02, 2006 at 10:24:36AM -0700, Grant Grundler wrote:
>>
>>>On Thu, Mar 02, 2006 at 03:50:57PM +0000, Russell King wrote:
>>>
>>>>I've been wondering whether this "no_ioport" flag is the correct approach,
>>>>or whether it's adding to complexity when it isn't really required.
>>>
>>>I think it's the simplest solution to allowing a driver
>>>to indicate which resources it wants to use. It solves
>>>the problem of I/O Port resource allocation sufficiently
>>>well.
>>
>>I have another question (brought up by someone working on a series of
>>ARM machines which make heavy use of MMIO.)
>>
>>Why isn't pci_enable_device_bars() sufficient - why do we have to
>>have another interface to say "we don't want BARs XXX" ?
>>
>>Let's say that we have a device driver which does this sequence (with,
>>of course, error checking):
>>
>>	pci_enable_device_bars(dev, 1<<1);
>>	pci_request_regions(dev);
>>
>>(a) should PCI remember that only BAR 1 has been requested to be enabled,
>>    and as such shouldn't pci_request_regions() ignore BAR 0?
>>
>>(b) should the PCI driver pass into pci_request_regions() (or even
>>    pci_request_regions_bars()) a bitmask of the BARs it wants to have
>>    requested, and similarly for pci_release_regions().
>>
>>Basically, if BAR0 hasn't been enabled, has pci_request_regions() got
>>any business requesting it from the resource tree?
> 
> 
> I understand the point you're making, but I think this misrepresents what
> is actually happening.  From my understanding of the spec, it's not possible
> to disable individual bars (with the exception of the expansion ROM).  Rather
> there is one bit for IO enable and one bit for IOMMU enable.  Therefore, we
> can enable or disable all I/O ports, but there's really no in between.  If
> the device uses even one I/O port, it's still a huge loss because of the
> potential bridge window dependency.  Also, if a device has several I/O ports
> but the driver only wants to use one, all of the others must still be
> assigned.
> 

I see. I think you are right.

In addition to the fact that there is one bit for IO enable and one
bit for MMIO enable, I think we should not enable I/O port (or MMIO)
of the device if not all the I/O port (or MMIO) regions are assigned
to the device because we must build a consistent address mapping
before enabling it.

It seems that using pci_enable_device_bars() is not a good idea.
If there is no objection, I'll design and implement take6 again.

Any comments?

Thanks,
Kenji Kaneshige

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Russell King]

On Fri, Mar 10, 2006 at 01:10:41PM +0900, Kenji Kaneshige wrote:
> Adam Belay wrote:
> >On Thu, Mar 02, 2006 at 07:34:41PM +0000, Russell King wrote:
> >>Why isn't pci_enable_device_bars() sufficient - why do we have to
> >>have another interface to say "we don't want BARs XXX" ?
> >>
> >>Let's say that we have a device driver which does this sequence (with,
> >>of course, error checking):
> >>
> >>	pci_enable_device_bars(dev, 1<<1);
> >>	pci_request_regions(dev);
> >>
> >>(a) should PCI remember that only BAR 1 has been requested to be enabled,
> >>   and as such shouldn't pci_request_regions() ignore BAR 0?
> >>
> >>(b) should the PCI driver pass into pci_request_regions() (or even
> >>   pci_request_regions_bars()) a bitmask of the BARs it wants to have
> >>   requested, and similarly for pci_release_regions().
> >>
> >>Basically, if BAR0 hasn't been enabled, has pci_request_regions() got
> >>any business requesting it from the resource tree?
> >
> >
> >I understand the point you're making, but I think this misrepresents what
> >is actually happening.  From my understanding of the spec, it's not 
> >possible
> >to disable individual bars (with the exception of the expansion ROM).  
> >Rather
> >there is one bit for IO enable and one bit for IOMMU enable.  Therefore, we
> >can enable or disable all I/O ports, but there's really no in between.  If
> >the device uses even one I/O port, it's still a huge loss because of the
> >potential bridge window dependency.  Also, if a device has several I/O 
> >ports
> >but the driver only wants to use one, all of the others must still be
> >assigned.
> >
> 
> I see. I think you are right.
> 
> In addition to the fact that there is one bit for IO enable and one
> bit for MMIO enable, I think we should not enable I/O port (or MMIO)
> of the device if not all the I/O port (or MMIO) regions are assigned
> to the device because we must build a consistent address mapping
> before enabling it.
> 
> It seems that using pci_enable_device_bars() is not a good idea.
> If there is no objection, I'll design and implement take6 again.

TBH, I don't think that your original approach is any better.  Maybe
Adam has a better idea how to solve this problem?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Russell King]

On Thu, Mar 09, 2006 at 09:10:10PM -0500, Adam Belay wrote:
> On Thu, Mar 02, 2006 at 07:34:41PM +0000, Russell King wrote:
> > I have another question (brought up by someone working on a series of
> > ARM machines which make heavy use of MMIO.)
> > 
> > Why isn't pci_enable_device_bars() sufficient - why do we have to
> > have another interface to say "we don't want BARs XXX" ?
> > 
> > Let's say that we have a device driver which does this sequence (with,
> > of course, error checking):
> > 
> > 	pci_enable_device_bars(dev, 1<<1);
> > 	pci_request_regions(dev);
> > 
> > (a) should PCI remember that only BAR 1 has been requested to be enabled,
> >     and as such shouldn't pci_request_regions() ignore BAR 0?
> > 
> > (b) should the PCI driver pass into pci_request_regions() (or even
> >     pci_request_regions_bars()) a bitmask of the BARs it wants to have
> >     requested, and similarly for pci_release_regions().
> > 
> > Basically, if BAR0 hasn't been enabled, has pci_request_regions() got
> > any business requesting it from the resource tree?
> 
> I understand the point you're making, but I think this misrepresents what
> is actually happening.  From my understanding of the spec, it's not possible
> to disable individual bars (with the exception of the expansion ROM).  Rather
> there is one bit for IO enable and one bit for IOMMU enable.  Therefore, we
> can enable or disable all I/O ports, but there's really no in between.  If
> the device uses even one I/O port, it's still a huge loss because of the
> potential bridge window dependency.  Also, if a device has several I/O ports
> but the driver only wants to use one, all of the others must still be
> assigned.

Agreed, but despite claiming that you understand my point, I don't
think you actually do.

1. It is already the case that drivers need to know the type of each BAR
   which the device presents - eg, most, if not all drivers take the start
   address of BAR0, throw that directly at ioremap, or use inb etc on it
   directly without first checking whether it's a MMIO or IO resource.

   So, if a driver knows that BAR0 is IO and BAR1 is MMIO, it can use
   pci_enable_device_bars(dev, BAR1) to only enable MMIO access.

2. pci_enable_device_bars() (which pre-exists for dealing with IDE) when
   passed the appropriate BAR mask, can be used to "enable" (or setup,
   program, or whatever, see below) only all MMIO or all IO BARs.

3. It is defined by the PCI subsystem that, for drivers, PCI resources
   are not valid prior to pci_enable_device*() being called, and are
   valid immediately after this call returns - pci_enable_device*()
   might be used by a PCI subsystem implementation to assign or
   reassign, and program PCI resources.

   Therefore, if you request pci_enable_device() (or pci_enable_device_bars
   with a full-bar mask) it is expected that _all_ BARs (or those
   requested) will become valid.  Adding this "no_io" device flag
   breaks this expectation.

4. I'm not suggesting that pci_enable_device_bars() should be (or can be)
   used to "selectively enable BARs" which I think is how you're reading
   this.  Yes, it does apparantly have the capacity to do this (and is
   used like this for IDE) but inappropriate use like that is a driver
   bug, and is already a driver bug *today*.

I'm merely suggesting using an established interface which has some
agreed appropriate driver expectations for the purpose of solving
this new problem.  It fits perfectly, it's clean, it doesn't add lots
of complexity, and there's no reason what so ever not to use it.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: [PATCH 0/4] PCI legacy I/O port free driver (take4)

[Kenji Kaneshige]

Russell King wrote:
> On Thu, Mar 09, 2006 at 09:10:10PM -0500, Adam Belay wrote:
> 
>>On Thu, Mar 02, 2006 at 07:34:41PM +0000, Russell King wrote:
>>
>>>I have another question (brought up by someone working on a series of
>>>ARM machines which make heavy use of MMIO.)
>>>
>>>Why isn't pci_enable_device_bars() sufficient - why do we have to
>>>have another interface to say "we don't want BARs XXX" ?
>>>
>>>Let's say that we have a device driver which does this sequence (with,
>>>of course, error checking):
>>>
>>>	pci_enable_device_bars(dev, 1<<1);
>>>	pci_request_regions(dev);
>>>
>>>(a) should PCI remember that only BAR 1 has been requested to be enabled,
>>>    and as such shouldn't pci_request_regions() ignore BAR 0?
>>>
>>>(b) should the PCI driver pass into pci_request_regions() (or even
>>>    pci_request_regions_bars()) a bitmask of the BARs it wants to have
>>>    requested, and similarly for pci_release_regions().
>>>
>>>Basically, if BAR0 hasn't been enabled, has pci_request_regions() got
>>>any business requesting it from the resource tree?
>>
>>I understand the point you're making, but I think this misrepresents what
>>is actually happening.  From my understanding of the spec, it's not possible
>>to disable individual bars (with the exception of the expansion ROM).  Rather
>>there is one bit for IO enable and one bit for IOMMU enable.  Therefore, we
>>can enable or disable all I/O ports, but there's really no in between.  If
>>the device uses even one I/O port, it's still a huge loss because of the
>>potential bridge window dependency.  Also, if a device has several I/O ports
>>but the driver only wants to use one, all of the others must still be
>>assigned.
> 
> 
> Agreed, but despite claiming that you understand my point, I don't
> think you actually do.
> 
> 1. It is already the case that drivers need to know the type of each BAR
>    which the device presents - eg, most, if not all drivers take the start
>    address of BAR0, throw that directly at ioremap, or use inb etc on it
>    directly without first checking whether it's a MMIO or IO resource.
> 
>    So, if a driver knows that BAR0 is IO and BAR1 is MMIO, it can use
>    pci_enable_device_bars(dev, BAR1) to only enable MMIO access.
>
> 2. pci_enable_device_bars() (which pre-exists for dealing with IDE) when
>    passed the appropriate BAR mask, can be used to "enable" (or setup,
>    program, or whatever, see below) only all MMIO or all IO BARs.
> 
> 3. It is defined by the PCI subsystem that, for drivers, PCI resources
>    are not valid prior to pci_enable_device*() being called, and are
>    valid immediately after this call returns - pci_enable_device*()
>    might be used by a PCI subsystem implementation to assign or
>    reassign, and program PCI resources.
> 
>    Therefore, if you request pci_enable_device() (or pci_enable_device_bars
>    with a full-bar mask) it is expected that _all_ BARs (or those
>    requested) will become valid.  Adding this "no_io" device flag
>    breaks this expectation.
> 
> 4. I'm not suggesting that pci_enable_device_bars() should be (or can be)
>    used to "selectively enable BARs" which I think is how you're reading
>    this.  Yes, it does apparantly have the capacity to do this (and is
>    used like this for IDE) but inappropriate use like that is a driver
>    bug, and is already a driver bug *today*.
> 

If we can consider it as a driver bug, I have no objection to use
pci_enable_device_bars(). I've added a new helper routine to make
proper BARs mask from resource type mask (pci_select_bars()) into
my take5 patch. So there would be no problems as long as drivers
use it when converting drivers to legacy I/O port free.

Thanks,
Kenji Kaneshige


> I'm merely suggesting using an established interface which has some
> agreed appropriate driver expectations for the purpose of solving
> this new problem.  It fits perfectly, it's clean, it doesn't add lots
> of complexity, and there's no reason what so ever not to use it.
>