[PATCH 0/6] PCI legacy I/O port free driver (take2)

[PATCH 0/6] PCI legacy I/O port free driver (take2)

[Kenji Kaneshige]

Hi,

Here is an updated set of patches for PCI legacy I/O port free drivers
which incorporates feedbacks. Summary of changes from the previous
version are:

   - Added the no_ioport field into struct pci_dev.
   - Added the device_flags field into struct pci_device_id, which is
     used to pass the flags to the kernel through ID table.
   - Removed pci_set_bar_mask() which was introduced in the previous
     version of patch.
   - Removed the bar_mask field from struct pci_dev which was
     introduced in the previous version of patch.
   - Updated the document.
   - Updated the patch for e1000 and lpfc in order to follow the
     above-mentioned change.

I'm attaching the following six patches:

    [patch 1/6] Add no_ioport flag into pci_dev
    [patch 2/6] Fix minor bug in store_new_id()
    [patch 3/6] Add device_flags into pci_device_id
    [patch 4/6] Update Documentation/pci.txt
    [patch 5/6] Make Intel e1000 driver legacy I/O port free
    [patch 6/6] Make Emulex lpfc driver legacy I/O port free

I'm attaching the brief description below about what the problem I'm
trying to solve is.

Thanks,
Kenji Kaneshige


Brief description of the problem
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
I encountered a problem that some PCI devices don't work on my system
which have huge number of PCI devices.

It is mandatory for all PCI device drivers to enable the device by
calling pci_enable_device() which enables all regions probed from the
device's BARs. If pci_enable_device() failes to enable any regions
probed from BARs, it returns as error. On the large servers, I/O port
resource could not be assigned to all PCI devices because it is
limited (64KB on Intel Architecture[1]) and it would be fragmented
(I/O base register of PCI-to-PCI bridge will usually be aligned to a
4KB boundary[2]). In this case, the devices which have no I/O port
resource assigned don't work because pci_enable_device() for those
devices failes. This is what happened on my machine.
---
[1]: Some machines support 64KB I/O port space per PCI segment.
[2]: Some P2P bridges support optional 1KB aligned I/O base.

Here, there are many PCI devices that provide both I/O port and MMIO
interface, and some of those devices can be handled without using I/O
port interface. The reason why such devices provide I/O port interface
is for compatibility to legacy OSs. So this kind of devices should
work even if enough I/O port resources are not assigned. The "PCI
Local Bus Specification Revision 3.0" also mentions about this topic
(Please see p.44, "IMPLEMENTATION NOTE"). On the current linux,
unfortunately, this kind of devices don't work if I/O port resources
are not assigned, because pci_enable_device() for those devices fails.

[PATCH 1/6] PCI legacy I/O port free driver (take2) - 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   |   33 +++++++++++++++++++++++++--------
 include/linux/pci.h |    1 +
 2 files changed, 26 insertions(+), 8 deletions(-)

Index: linux-2.6.16-rc4/include/linux/pci.h
===================================================================
--- linux-2.6.16-rc4.orig/include/linux/pci.h	2006-02-21 14:40:46.000000000 +0900
+++ linux-2.6.16-rc4/include/linux/pci.h	2006-02-21 14:40:54.000000000 +0900
@@ -152,6 +152,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 bin_attribute *rom_attr; /* attribute descriptor for sysfs ROM entry */
Index: linux-2.6.16-rc4/drivers/pci/pci.c
===================================================================
--- linux-2.6.16-rc4.orig/drivers/pci/pci.c	2006-02-21 14:40:46.000000000 +0900
+++ linux-2.6.16-rc4/drivers/pci/pci.c	2006-02-21 14:40:54.000000000 +0900
@@ -495,9 +495,14 @@
 int
 pci_enable_device(struct pci_dev *dev)
 {
-	int err;
+	int err, i, bars = (1 << PCI_NUM_RESOURCES) - 1;
 
-	if ((err = pci_enable_device_bars(dev, (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);
+
+	if ((err = pci_enable_device_bars(dev, bars)))
 		return err;
 	pci_fixup_device(pci_fixup_enable, dev);
 	dev->is_enabled = 1;
@@ -617,9 +622,11 @@
 {
 	if (pci_resource_len(pdev, bar) == 0)
 		return;
-	if (pci_resource_flags(pdev, bar) & IORESOURCE_IO)
+	if (pci_resource_flags(pdev, bar) & IORESOURCE_IO) {
+		WARN_ON(pdev->no_ioport);
 		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));
@@ -645,6 +652,7 @@
 		return 0;
 		
 	if (pci_resource_flags(pdev, bar) & IORESOURCE_IO) {
+		WARN_ON(pdev->no_ioport);
 		if (!request_region(pci_resource_start(pdev, bar),
 			    pci_resource_len(pdev, bar), res_name))
 			goto err_out;
@@ -678,10 +686,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);
+	}
 }
 
 /**
@@ -699,16 +710,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/6] PCI legacy I/O port free driver (take2) - Fix minor bug in store_new_id()

[Kenji Kaneshige]

This patch fixes invalid use of sscanf() in store_new_id().

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

---
 drivers/pci/pci-driver.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.16-rc4/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.16-rc4.orig/drivers/pci/pci-driver.c	2006-02-21 14:40:46.000000000 +0900
+++ linux-2.6.16-rc4/drivers/pci/pci-driver.c	2006-02-21 14:40:55.000000000 +0900
@@ -47,7 +47,7 @@
 	unsigned long driver_data=0;
 	int fields=0;
 
-	fields = sscanf(buf, "%x %x %x %x %x %x %lux",
+	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
 			&vendor, &device, &subvendor, &subdevice,
 			&class, &class_mask, &driver_data);
 	if (fields < 0)

[PATCH 3/6] PCI legacy I/O port free driver (take2) - Add device_flags into pci_device_id

[Kenji Kaneshige]

This patch adds the device_flags field into struct pci_device_id to
enables pci device drivers to pass per device ID flags to the
kernel. This patch also defines the PCI_DEVICE_ID_FLAG_NOIOPOT flag of
the device_flags field which is used to tell the kernel whether the
driver need to use I/O port regions to handle the device.

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

---
 drivers/pci/pci-driver.c        |   14 +++++++++++---
 include/linux/mod_devicetable.h |    3 +++
 2 files changed, 14 insertions(+), 3 deletions(-)

Index: linux-2.6.16-rc4/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.16-rc4.orig/drivers/pci/pci-driver.c	2006-02-21 14:40:55.000000000 +0900
+++ linux-2.6.16-rc4/drivers/pci/pci-driver.c	2006-02-21 14:40:55.000000000 +0900
@@ -43,13 +43,13 @@
 	struct pci_dynid *dynid;
 	struct pci_driver *pdrv = to_pci_driver(driver);
 	__u32 vendor=PCI_ANY_ID, device=PCI_ANY_ID, subvendor=PCI_ANY_ID,
-		subdevice=PCI_ANY_ID, class=0, class_mask=0;
+		subdevice=PCI_ANY_ID, class=0, class_mask=0, device_flags=0;
 	unsigned long driver_data=0;
 	int fields=0;
 
-	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
+	fields = sscanf(buf, "%x %x %x %x %x %x %lx %x",
 			&vendor, &device, &subvendor, &subdevice,
-			&class, &class_mask, &driver_data);
+			&class, &class_mask, &driver_data, &device_flags);
 	if (fields < 0)
 		return -EINVAL;
 
@@ -67,6 +67,7 @@
 	dynid->id.class_mask = class_mask;
 	dynid->id.driver_data = pdrv->dynids.use_driver_data ?
 		driver_data : 0UL;
+	dynid->id.device_flags = device_flags;
 
 	spin_lock(&pdrv->dynids.lock);
 	list_add_tail(&pdrv->dynids.list, &dynid->node);
@@ -170,6 +171,12 @@
 	return NULL;
 }
 
+static inline void pci_extract_per_id_flags(struct pci_dev *dev,
+					    const struct pci_device_id *id)
+{
+	dev->no_ioport = !!(id->device_flags & PCI_DEVICE_ID_FLAG_NOIOPORT);
+}
+
 static int pci_call_probe(struct pci_driver *drv, struct pci_dev *dev,
 			  const struct pci_device_id *id)
 {
@@ -189,6 +196,7 @@
 	current->mempolicy = &default_policy;
 	mpol_get(current->mempolicy);
 #endif
+	pci_extract_per_id_flags(dev, id);
 	error = drv->probe(dev, id);
 #ifdef CONFIG_NUMA
 	set_cpus_allowed(current, oldmask);
Index: linux-2.6.16-rc4/include/linux/mod_devicetable.h
===================================================================
--- linux-2.6.16-rc4.orig/include/linux/mod_devicetable.h	2006-02-21 14:40:46.000000000 +0900
+++ linux-2.6.16-rc4/include/linux/mod_devicetable.h	2006-02-21 14:40:55.000000000 +0900
@@ -19,8 +19,11 @@
 	__u32 subvendor, subdevice;	/* Subsystem ID's or PCI_ANY_ID */
 	__u32 class, class_mask;	/* (class,subclass,prog-if) triplet */
 	kernel_ulong_t driver_data;	/* Data private to the driver */
+	__u32 device_flags;		/* Per device ID flags (See below) */
 };
 
+/* Per PCI device ID flags */
+#define PCI_DEVICE_ID_FLAG_NOIOPORT	(1<<0)	/* Don't need I/O port */
 
 #define IEEE1394_MATCH_VENDOR_ID	0x0001
 #define IEEE1394_MATCH_MODEL_ID		0x0002

[PATCH 4/6] PCI legacy I/O port free driver (take2) - 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 |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 50 insertions(+)

Index: linux-2.6.16-rc4/Documentation/pci.txt
===================================================================
--- linux-2.6.16-rc4.orig/Documentation/pci.txt	2006-02-21 14:40:46.000000000 +0900
+++ linux-2.6.16-rc4/Documentation/pci.txt	2006-02-21 14:40:56.000000000 +0900
@@ -81,6 +81,8 @@
 	class,		Device class to match. The class_mask tells which bits
 	class_mask	of the class are honored during the comparison.
 	driver_data	Data private to the driver.
+	device_flags	Per device id flags. See mod_devicetable.h for
+			specific.
 
 Most drivers don't need to use the driver_data field.  Best practice
 for use of driver_data is to use it as an index into a static list of
@@ -269,3 +271,51 @@
 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
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+On the large servers, I/O port resources could not be assigned to all
+PCI devices because it is limited (64KB on Intel Architecture[1]) and
+it would be fragmented (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() for those devices will
+fail because those functions try to enable all the regions. However,
+it is a problem for some PCI devices which provide both I/O port and
+MMIO interface because some of them can be handled without using I/O
+port interface. The reason why such devices provide I/O port interface
+is for compatibility to legacy OSs. So this kind of devices should
+work even if enough I/O port resources are not assigned. The "PCI
+Local Bus Specification Revision 3.0" also mentions about this topic
+(Please see p.44, "IMPLEMENTATION NOTE").
+
+This problem is solved by telling the kernel if your driver needs to
+use I/O port to handle the device. If your driver doesn't need any I/O
+port regions to handle the device, you can tell it to the kernel by
+setting PCI_DEVICE_ID_FLAG_NOIOPORT flag in the ID table like below:
+
+	struct pci_device_id your_id_table {
+		...,
+		{
+			...,
+			.device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT,
+			...,
+		},
+		...,
+	}
+
+If the PCI_DEVICE_ID_FLAG_NOIOPORT flag is set, kernel will never
+touch the I/O port regions for the corresponding devices.
+
+By using ID table, you can tell the kernel whether to use I/O port by
+per device ID basis. However, some drivers might need to check other
+information than in table ID (e.g. revision ID) to see if they need to
+use I/O port. In this case, you can use the no_ioport flag in struct
+pci_dev. If the no_ioport flag is set, kernel will never touch I/O
+port regions for the device. You would check some information to see
+if your device needs I/O port, and you would set the no_ioport flag as
+necessary. Please note that you need to set the no_ioport flag before
+calling pci_enable_device() and pci_request_regions().
+
+[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.

[PATCH 5/6] PCI legacy I/O port free driver (take2) - Make Intel e1000 driver legacy I/O port free

[Kenji Kaneshige]

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

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

 drivers/net/e1000/e1000.h      |    4 -
 drivers/net/e1000/e1000_main.c |  105 +++++++++++++++++++++--------------------
 2 files changed, 56 insertions(+), 53 deletions(-)

Index: linux-2.6.16-rc4/drivers/net/e1000/e1000.h
===================================================================
--- linux-2.6.16-rc4.orig/drivers/net/e1000/e1000.h	2006-02-21 14:40:46.000000000 +0900
+++ linux-2.6.16-rc4/drivers/net/e1000/e1000.h	2006-02-21 14:40:57.000000000 +0900
@@ -77,8 +77,8 @@
 #define BAR_1		1
 #define BAR_5		5
 
-#define INTEL_E1000_ETHERNET_DEVICE(device_id) {\
-	PCI_DEVICE(PCI_VENDOR_ID_INTEL, device_id)}
+#define INTEL_E1000_ETHERNET_DEVICE(device_id, flags) {\
+	PCI_DEVICE(PCI_VENDOR_ID_INTEL, device_id), .device_flags = flags}
 
 struct e1000_adapter;
 
Index: linux-2.6.16-rc4/drivers/net/e1000/e1000_main.c
===================================================================
--- linux-2.6.16-rc4.orig/drivers/net/e1000/e1000_main.c	2006-02-21 14:40:46.000000000 +0900
+++ linux-2.6.16-rc4/drivers/net/e1000/e1000_main.c	2006-02-21 14:40:57.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, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1001, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1004, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	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, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	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, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x101A, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x101D, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x101E, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1026, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1027, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1028, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x105E, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x105F, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1060, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1075, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1076, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1077, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1078, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x1079, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x107A, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x107B, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x107C, 0),
+	INTEL_E1000_ETHERNET_DEVICE(0x107D, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x107E, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x107F, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x108A, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x108B, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x108C, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x1099, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x109A, PCI_DEVICE_ID_FLAG_NOIOPORT),
+	INTEL_E1000_ETHERNET_DEVICE(0x10B5, PCI_DEVICE_ID_FLAG_NOIOPORT),
 	/* required last entry */
 	{0,}
 };
@@ -709,12 +709,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->device_flags & PCI_DEVICE_ID_FLAG_NOIOPORT)) {
+		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 6/6] PCI legacy I/O port free driver (take2) - Make Emulex lpfc driver legacy I/O port free

[Kenji Kaneshige]

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

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

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

Index: linux-2.6.16-rc4/drivers/scsi/lpfc/lpfc_init.c
===================================================================
--- linux-2.6.16-rc4.orig/drivers/scsi/lpfc/lpfc_init.c	2006-02-21 14:40:03.000000000 +0900
+++ linux-2.6.16-rc4/drivers/scsi/lpfc/lpfc_init.c	2006-02-21 14:40:57.000000000 +0900
@@ -1715,59 +1715,59 @@
 
 static struct pci_device_id lpfc_id_table[] = {
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_VIPER,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_FIREFLY,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_THOR,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_PEGASUS,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_CENTAUR,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_DRAGONFLY,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_SUPERFLY,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_RFLY,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_PFLY,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_NEPTUNE,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_NEPTUNE_SCSP,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_NEPTUNE_DCSP,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_HELIOS,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_HELIOS_SCSP,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_HELIOS_DCSP,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_BMID,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_BSMB,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_ZEPHYR,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_ZEPHYR_SCSP,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_ZEPHYR_DCSP,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_ZMID,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_ZSMB,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_TFLY,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_LP101,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_LP10000S,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_LP11000S,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_LPE11000S,
-		PCI_ANY_ID, PCI_ANY_ID, },
+	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},
 	{ 0 }
 };
 

Re: [PATCH 3/6] PCI legacy I/O port free driver (take2) - Add device_flags into pci_device_id

[Andi Kleen]

Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> writes:

> This patch adds the device_flags field into struct pci_device_id to
> enables pci device drivers to pass per device ID flags to the
> kernel. This patch also defines the PCI_DEVICE_ID_FLAG_NOIOPOT flag of
> the device_flags field which is used to tell the kernel whether the
> driver need to use I/O port regions to handle the device.


Thanks. I actually meant to use the existing driver_data field for it,
but on second thought using a new field like you did makes sense
because we could use that to easily enable MSI and possibly other
advanced features in the future too.

Only thing I would double check is if the generation of modules.pcimap
(that is used by distribution installers to load the right drivers
automatically) still works correctly.

-Andi

Re: [PATCH 6/6] PCI legacy I/O port free driver (take2) - Make Emulex lpfc driver legacy I/O port free

[Greg KH]

On Tue, Feb 21, 2006 at 03:33:40PM +0900, Kenji Kaneshige wrote:
> This patch makes lpfc driver legacy I/O port free.
> 
> Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> 
>  drivers/scsi/lpfc/lpfc_init.c |   54 +++++++++++++++++++++---------------------
>  1 files changed, 27 insertions(+), 27 deletions(-)
> 
> Index: linux-2.6.16-rc4/drivers/scsi/lpfc/lpfc_init.c
> ===================================================================
> --- linux-2.6.16-rc4.orig/drivers/scsi/lpfc/lpfc_init.c	2006-02-21 14:40:03.000000000 +0900
> +++ linux-2.6.16-rc4/drivers/scsi/lpfc/lpfc_init.c	2006-02-21 14:40:57.000000000 +0900
> @@ -1715,59 +1715,59 @@
>  
>  static struct pci_device_id lpfc_id_table[] = {
>  	{PCI_VENDOR_ID_EMULEX, PCI_DEVICE_ID_VIPER,
> -		PCI_ANY_ID, PCI_ANY_ID, },
> +	 PCI_ANY_ID, PCI_ANY_ID, .device_flags = PCI_DEVICE_ID_FLAG_NOIOPORT},

Might just be easier to use PCI_DEVICE() here instead, right?

thanks,

greg k-h

Re: [PATCH 3/6] PCI legacy I/O port free driver (take2) - Add device_flags into pci_device_id

[Greg KH]

On Tue, Feb 21, 2006 at 03:30:13PM +0900, Kenji Kaneshige wrote:
> This patch adds the device_flags field into struct pci_device_id to
> enables pci device drivers to pass per device ID flags to the
> kernel. This patch also defines the PCI_DEVICE_ID_FLAG_NOIOPOT flag of
> the device_flags field which is used to tell the kernel whether the
> driver need to use I/O port regions to handle the device.
> 
> Signed-off-by: Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>
> 
> ---
>  drivers/pci/pci-driver.c        |   14 +++++++++++---
>  include/linux/mod_devicetable.h |    3 +++
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> Index: linux-2.6.16-rc4/drivers/pci/pci-driver.c
> ===================================================================
> --- linux-2.6.16-rc4.orig/drivers/pci/pci-driver.c	2006-02-21 14:40:55.000000000 +0900
> +++ linux-2.6.16-rc4/drivers/pci/pci-driver.c	2006-02-21 14:40:55.000000000 +0900
> @@ -43,13 +43,13 @@
>  	struct pci_dynid *dynid;
>  	struct pci_driver *pdrv = to_pci_driver(driver);
>  	__u32 vendor=PCI_ANY_ID, device=PCI_ANY_ID, subvendor=PCI_ANY_ID,
> -		subdevice=PCI_ANY_ID, class=0, class_mask=0;
> +		subdevice=PCI_ANY_ID, class=0, class_mask=0, device_flags=0;
>  	unsigned long driver_data=0;
>  	int fields=0;
>  
> -	fields = sscanf(buf, "%x %x %x %x %x %x %lx",
> +	fields = sscanf(buf, "%x %x %x %x %x %x %lx %x",
>  			&vendor, &device, &subvendor, &subdevice,
> -			&class, &class_mask, &driver_data);
> +			&class, &class_mask, &driver_data, &device_flags);
>  	if (fields < 0)
>  		return -EINVAL;
>  
> @@ -67,6 +67,7 @@
>  	dynid->id.class_mask = class_mask;
>  	dynid->id.driver_data = pdrv->dynids.use_driver_data ?
>  		driver_data : 0UL;
> +	dynid->id.device_flags = device_flags;
>  
>  	spin_lock(&pdrv->dynids.lock);
>  	list_add_tail(&pdrv->dynids.list, &dynid->node);
> @@ -170,6 +171,12 @@
>  	return NULL;
>  }
>  
> +static inline void pci_extract_per_id_flags(struct pci_dev *dev,
> +					    const struct pci_device_id *id)
> +{
> +	dev->no_ioport = !!(id->device_flags & PCI_DEVICE_ID_FLAG_NOIOPORT);
> +}
> +
>  static int pci_call_probe(struct pci_driver *drv, struct pci_dev *dev,
>  			  const struct pci_device_id *id)
>  {
> @@ -189,6 +196,7 @@
>  	current->mempolicy = &default_policy;
>  	mpol_get(current->mempolicy);
>  #endif
> +	pci_extract_per_id_flags(dev, id);
>  	error = drv->probe(dev, id);
>  #ifdef CONFIG_NUMA
>  	set_cpus_allowed(current, oldmask);
> Index: linux-2.6.16-rc4/include/linux/mod_devicetable.h
> ===================================================================
> --- linux-2.6.16-rc4.orig/include/linux/mod_devicetable.h	2006-02-21 14:40:46.000000000 +0900
> +++ linux-2.6.16-rc4/include/linux/mod_devicetable.h	2006-02-21 14:40:55.000000000 +0900
> @@ -19,8 +19,11 @@
>  	__u32 subvendor, subdevice;	/* Subsystem ID's or PCI_ANY_ID */
>  	__u32 class, class_mask;	/* (class,subclass,prog-if) triplet */
>  	kernel_ulong_t driver_data;	/* Data private to the driver */
> +	__u32 device_flags;		/* Per device ID flags (See below) */

I don't think you can add fields here, after the driver_data field.  It
might mess up userspace tools a lot, as you are changing a userspace
api.

Do you _really_ need to pass this information back from userspace to the
driver in this manner?

thanks,

greg k-h

Re: [PATCH 3/6] PCI legacy I/O port free driver (take2) - Add device_flags into pci_device_id

[Andi Kleen]

On Tuesday 21 February 2006 21:56, Greg KH wrote:

> I don't think you can add fields here, after the driver_data field.  It
> might mess up userspace tools a lot, as you are changing a userspace
> api.

User space should look at the ASCII files (modules.*), not the binary
As long as the code to generate these files still works it should be ok.

> Do you _really_ need to pass this information back from userspace to the
> driver in this manner?

Well driver_data wouldn't be needed then either. Obviously it's for more
than just userspace.

-Andi

 

Re: [PATCH 1/6] PCI legacy I/O port free driver (take2) - Add no_ioport flag into pci_dev

[Greg KH]

On Tue, Feb 21, 2006 at 03:28:01PM +0900, Kenji Kaneshige wrote:
> -	if (pci_resource_flags(pdev, bar) & IORESOURCE_IO)
> +	if (pci_resource_flags(pdev, bar) & IORESOURCE_IO) {
> +		WARN_ON(pdev->no_ioport);

You might want to just print out a nicer warning to the user through the
syslog, using dev_warn() otherwise they are not going to know which
device and driver are having problems.

Also, people see the output of this, and think their kernel just died,
which is not the case here.

thanks,

greg k-h

Re: [PATCH 3/6] PCI legacy I/O port free driver (take2) - Add device_flags into pci_device_id

[Greg KH]

On Tue, Feb 21, 2006 at 09:59:51PM +0100, Andi Kleen wrote:
> On Tuesday 21 February 2006 21:56, Greg KH wrote:
> 
> > I don't think you can add fields here, after the driver_data field.  It
> > might mess up userspace tools a lot, as you are changing a userspace
> > api.
> 
> User space should look at the ASCII files (modules.*), not the binary
> As long as the code to generate these files still works it should be ok.

Does it?  Shouldn't the tools export this information too, if it really
should belong in the pci_id structure?

So, is _every_ pci driver going to have to be modified to support this
new field if they are supposed to work on this kind of hardware?  If so,
that doesn't sound like a good idea.  Any way we can just set the bit in
the pci arch specific code for the devices instead?

> > Do you _really_ need to pass this information back from userspace to the
> > driver in this manner?
> 
> Well driver_data wouldn't be needed then either. Obviously it's for more
> than just userspace.

For some drivers, they need that driver_data field to be set.  Just
wanting to make sure that this also is needed.

thanks,

greg k-h

Re: [PATCH 3/6] PCI legacy I/O port free driver (take2) - Add device_flags into pci_device_id

[Andi Kleen]

On Tuesday 21 February 2006 22:10, Greg KH wrote:
> On Tue, Feb 21, 2006 at 09:59:51PM +0100, Andi Kleen wrote:
> > On Tuesday 21 February 2006 21:56, Greg KH wrote:
> > 
> > > I don't think you can add fields here, after the driver_data field.  It
> > > might mess up userspace tools a lot, as you are changing a userspace
> > > api.
> > 
> > User space should look at the ASCII files (modules.*), not the binary
> > As long as the code to generate these files still works it should be ok.
> 
> Does it?  

I assume Kenji-San tested that.

> Shouldn't the tools export this information too, if it really 
> should belong in the pci_id structure?

No - is driver_data exported? 
 
> So, is _every_ pci driver going to have to be modified to support this
> new field if they are supposed to work on this kind of hardware? 

There is 100% source compatibility because fields are only added at the
end of the structure.

And the drivers will still work on this hardware even without modification.

It's only an optimization that can be added to selected drivers.

-Andi

Re: [PATCH 3/6] PCI legacy I/O port free driver (take2) - Add device_flags into pci_device_id

[Jeff Garzik]

Andi Kleen wrote:
> On Tuesday 21 February 2006 22:10, Greg KH wrote:
> 
>>On Tue, Feb 21, 2006 at 09:59:51PM +0100, Andi Kleen wrote:
>>
>>>On Tuesday 21 February 2006 21:56, Greg KH wrote:
>>>
>>>
>>>>I don't think you can add fields here, after the driver_data field.  It
>>>>might mess up userspace tools a lot, as you are changing a userspace
>>>>api.
>>>
>>>User space should look at the ASCII files (modules.*), not the binary
>>>As long as the code to generate these files still works it should be ok.
>>
>>Does it?  
> 
> 
> I assume Kenji-San tested that.
> 
> 
>>Shouldn't the tools export this information too, if it really 
>>should belong in the pci_id structure?
> 
> 
> No - is driver_data exported? 
>  
> 
>>So, is _every_ pci driver going to have to be modified to support this
>>new field if they are supposed to work on this kind of hardware? 
> 
> 
> There is 100% source compatibility because fields are only added at the
> end of the structure.
> 
> And the drivers will still work on this hardware even without modification.
> 
> It's only an optimization that can be added to selected drivers.

It doesn't matter how easily its added, it is the wrong place to add 
such things.

This is what the various functions called during pci_driver::probe() do...

	Jeff

Re: [PATCH 3/6] PCI legacy I/O port free driver (take2) - Add device_flags into pci_device_id

[Andi Kleen]

On Tuesday 21 February 2006 22:55, Jeff Garzik wrote:

> It doesn't matter how easily its added, it is the wrong place to add 
> such things.
> 
> This is what the various functions called during pci_driver::probe() do...

The problem is that at least on the e1000 it only applies to some of the 
many PCI-IDs it supports. So the original patch had an long ugly switch
with PCI IDs to check it. I suggested to use driver_data for it then,
but Kenji-San ended up with this new field.  I actually like the idea
of the new field because it would allow to add such things very easily
without adding lots of code.

it's not an uncommon situation. e.g. consider driver A which supports
a lot of PCI-IDs but MSI only works on a few of them. How do you
handle this? Add an ugly switch that will bitrot? Or put all the 
information into a single place which is the pci_device_id array.

I prefer the later solution. 

Of course there are some boundaries that shouldn't be exceeded here.
It probably only makes sense for parameters that are simple boolean,
nothing more complicated. But for that it's quite nice.

-Andi

Re: [PATCH 3/6] PCI legacy I/O port free driver (take2) - Add device_flags into pci_device_id

[Jeff Garzik]

Andi Kleen wrote:
> On Tuesday 21 February 2006 22:55, Jeff Garzik wrote:
> 
> 
>>It doesn't matter how easily its added, it is the wrong place to add 
>>such things.
>>
>>This is what the various functions called during pci_driver::probe() do...
> 
> 
> The problem is that at least on the e1000 it only applies to some of the 
> many PCI-IDs it supports. So the original patch had an long ugly switch
> with PCI IDs to check it. I suggested to use driver_data for it then,
> but Kenji-San ended up with this new field.  I actually like the idea
> of the new field because it would allow to add such things very easily
> without adding lots of code.
> 
> it's not an uncommon situation. e.g. consider driver A which supports
> a lot of PCI-IDs but MSI only works on a few of them. How do you
> handle this? Add an ugly switch that will bitrot? Or put all the 
> information into a single place which is the pci_device_id array.

You do what tons of other drivers do, and indicate this via driver_data. 
  An enumerated type in driver_data can be used to uniquely identify any 
device or set of devices.

No need to add anything.

	Jeff

Re: [PATCH 3/6] PCI legacy I/O port free driver (take2) - Add device_flags into pci_device_id

[Greg KH]

On Tue, Feb 21, 2006 at 07:09:08PM -0500, Jeff Garzik wrote:
> Andi Kleen wrote:
> >On Tuesday 21 February 2006 22:55, Jeff Garzik wrote:
> >
> >
> >>It doesn't matter how easily its added, it is the wrong place to add 
> >>such things.
> >>
> >>This is what the various functions called during pci_driver::probe() do...
> >
> >
> >The problem is that at least on the e1000 it only applies to some of the 
> >many PCI-IDs it supports. So the original patch had an long ugly switch
> >with PCI IDs to check it. I suggested to use driver_data for it then,
> >but Kenji-San ended up with this new field.  I actually like the idea
> >of the new field because it would allow to add such things very easily
> >without adding lots of code.
> >
> >it's not an uncommon situation. e.g. consider driver A which supports
> >a lot of PCI-IDs but MSI only works on a few of them. How do you
> >handle this? Add an ugly switch that will bitrot? Or put all the 
> >information into a single place which is the pci_device_id array.
> 
> You do what tons of other drivers do, and indicate this via driver_data. 
>  An enumerated type in driver_data can be used to uniquely identify any 
> device or set of devices.
> 
> No need to add anything.

Yes, I agree, use driver_data, it's simpler, and keeps the PCI core
clean.

thanks,

greg k-h

Re: [PATCH 3/6] PCI legacy I/O port free driver (take2) - Add device_flags into pci_device_id

[Kenji Kaneshige]

Greg KH wrote:
> On Tue, Feb 21, 2006 at 07:09:08PM -0500, Jeff Garzik wrote:
> 
>>Andi Kleen wrote:
>>
>>>On Tuesday 21 February 2006 22:55, Jeff Garzik wrote:
>>>
>>>
>>>
>>>>It doesn't matter how easily its added, it is the wrong place to add 
>>>>such things.
>>>>
>>>>This is what the various functions called during pci_driver::probe() do...
>>>
>>>
>>>The problem is that at least on the e1000 it only applies to some of the 
>>>many PCI-IDs it supports. So the original patch had an long ugly switch
>>>with PCI IDs to check it. I suggested to use driver_data for it then,
>>>but Kenji-San ended up with this new field.  I actually like the idea
>>>of the new field because it would allow to add such things very easily
>>>without adding lots of code.
>>>
>>>it's not an uncommon situation. e.g. consider driver A which supports
>>>a lot of PCI-IDs but MSI only works on a few of them. How do you
>>>handle this? Add an ugly switch that will bitrot? Or put all the 
>>>information into a single place which is the pci_device_id array.
>>
>>You do what tons of other drivers do, and indicate this via driver_data. 
>> An enumerated type in driver_data can be used to uniquely identify any 
>>device or set of devices.
>>
>>No need to add anything.
> 
> 
> Yes, I agree, use driver_data, it's simpler, and keeps the PCI core
> clean.
> 
> thanks,
> 
> greg k-h
> 

Hi,

Thank you very much for all of the comments.

The reason why I added the new field into struct pci_device_id is
as follows, though these are already explained completely by Andi.

    - By adding the new field instead of using driver_data, drivers
      can pass the flags to the kernel directly. That is, drivers
      don't need to call any new APIs to tell something to the kernel
      after checking their own driver_data in their .probe().

    - This new field can be used not only for ioport but also
      other purpose (e.g. MSI).

But according to the discussion, it doesn't look a good idea to add
the new field into pci_device_id this time. So I'll update my patches
to use the driver_data field instead.

Thanks,
Kenji Kaneshige

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

[Benjamin Herrenschmidt]

> Here, there are many PCI devices that provide both I/O port and MMIO
> interface, and some of those devices can be handled without using I/O
> port interface. The reason why such devices provide I/O port interface
> is for compatibility to legacy OSs. So this kind of devices should
> work even if enough I/O port resources are not assigned. The "PCI
> Local Bus Specification Revision 3.0" also mentions about this topic
> (Please see p.44, "IMPLEMENTATION NOTE"). On the current linux,
> unfortunately, this kind of devices don't work if I/O port resources
> are not assigned, because pci_enable_device() for those devices fails.

Which is where the real problem is ... I'm afraid you are doing a
workaround for the wrong issue... do we really need to assign all
resources to the device at pci_enable_device() time ? Yeah, I know, that
sounds gross... but think about it... doesn't pci_request_region(s) look
like a better spot ? Or maybe we should change pci_enable_device()
itself to take a mask of BARs that are relevant. That would help dealing
with a couple of other cases of devices where some BARs really need to
be ignored...

The later is probably the best approach without breaking everything, by
having a new pci_enable_resources(mask) that would take a mask of BARs
to enable, with pci_enable_device() becoming just a call to the former
for all BARs ....

Ben.

Re: [PATCH 3/6] PCI legacy I/O port free driver (take2) - Add device_flags into pci_device_id

[Benjamin Herrenschmidt]

On Tue, 2006-02-21 at 13:10 -0800, Greg KH wrote:
> On Tue, Feb 21, 2006 at 09:59:51PM +0100, Andi Kleen wrote:
> > On Tuesday 21 February 2006 21:56, Greg KH wrote:
> > 
> > > I don't think you can add fields here, after the driver_data field.  It
> > > might mess up userspace tools a lot, as you are changing a userspace
> > > api.
> > 
> > User space should look at the ASCII files (modules.*), not the binary
> > As long as the code to generate these files still works it should be ok.
> 
> Does it?  Shouldn't the tools export this information too, if it really
> should belong in the pci_id structure?
> 
> So, is _every_ pci driver going to have to be modified to support this
> new field if they are supposed to work on this kind of hardware?  If so,
> that doesn't sound like a good idea.  Any way we can just set the bit in
> the pci arch specific code for the devices instead?

I think the right approach is to not change driver_data but instead to
add a new version of pci_enable_device() (I call it
pci_enable_resources() but you are welcome to find something more fancy)
to enable a selected set of resources with the old pci_enable_device()
just calling the new one with a full mask set.

I don't like the driver_data approach. I don't like the static table
approach in fact. Drivers may "know" wether they need to enable/disable
given resources based on other things like revision, etc... Some drivers
may want to enable only one BAR, access some registers to properly
figure out what rev of a device they are talking to, then selectively
enable other BARs and/or MSIs etc...

I think the driver should be in control... flags in a static table
aren't flexible enough.

Ben.

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

[Kenji Kaneshige]

Benjamin Herrenschmidt wrote:
>>Here, there are many PCI devices that provide both I/O port and MMIO
>>interface, and some of those devices can be handled without using I/O
>>port interface. The reason why such devices provide I/O port interface
>>is for compatibility to legacy OSs. So this kind of devices should
>>work even if enough I/O port resources are not assigned. The "PCI
>>Local Bus Specification Revision 3.0" also mentions about this topic
>>(Please see p.44, "IMPLEMENTATION NOTE"). On the current linux,
>>unfortunately, this kind of devices don't work if I/O port resources
>>are not assigned, because pci_enable_device() for those devices fails.
> 
> 
> Which is where the real problem is ... I'm afraid you are doing a
> workaround for the wrong issue... do we really need to assign all
> resources to the device at pci_enable_device() time ? Yeah, I know, that
> sounds gross... but think about it... doesn't pci_request_region(s) look
> like a better spot ? Or maybe we should change pci_enable_device()
> itself to take a mask of BARs that are relevant. That would help dealing
> with a couple of other cases of devices where some BARs really need to
> be ignored...
> 
> The later is probably the best approach without breaking everything, by
> having a new pci_enable_resources(mask) that would take a mask of BARs
> to enable, with pci_enable_device() becoming just a call to the former
> for all BARs ....
> 
> Ben.
> 

I guess the existing pci_enable_device_bars() is very similar to
your pci_enable_resources(). We already discussed it at:

http://marc.theaimsgroup.com/?l=linux-kernel&m=114000705026791&w=2

Thanks,
Kenji Kaneshige

Re: [PATCH 3/6] PCI legacy I/O port free driver (take2) - Add device_flags into pci_device_id

[Kenji Kaneshige]

Benjamin Herrenschmidt wrote:
> On Tue, 2006-02-21 at 13:10 -0800, Greg KH wrote:
> 
>>On Tue, Feb 21, 2006 at 09:59:51PM +0100, Andi Kleen wrote:
>>
>>>On Tuesday 21 February 2006 21:56, Greg KH wrote:
>>>
>>>
>>>>I don't think you can add fields here, after the driver_data field.  It
>>>>might mess up userspace tools a lot, as you are changing a userspace
>>>>api.
>>>
>>>User space should look at the ASCII files (modules.*), not the binary
>>>As long as the code to generate these files still works it should be ok.
>>
>>Does it?  Shouldn't the tools export this information too, if it really
>>should belong in the pci_id structure?
>>
>>So, is _every_ pci driver going to have to be modified to support this
>>new field if they are supposed to work on this kind of hardware?  If so,
>>that doesn't sound like a good idea.  Any way we can just set the bit in
>>the pci arch specific code for the devices instead?
> 
> 
> I think the right approach is to not change driver_data but instead to
> add a new version of pci_enable_device() (I call it
> pci_enable_resources() but you are welcome to find something more fancy)
> to enable a selected set of resources with the old pci_enable_device()
> just calling the new one with a full mask set.
>

Using driver_data is one method to check if the device needs I/O
port or not. So whether to use driver_data depends on the design
of each driver.


> I don't like the driver_data approach. I don't like the static table
> approach in fact. Drivers may "know" wether they need to enable/disable
> given resources based on other things like revision, etc... Some drivers
> may want to enable only one BAR, access some registers to properly
> figure out what rev of a device they are talking to, then selectively
> enable other BARs and/or MSIs etc...
> 

Exactly. I already mentioned about that in Documentation/pci.txt
in my second patch.

Thanks,
Kenji Kaneshige