* [PATCH 0/4] PCI legacy I/O port free driver (take4)
@ 2006-03-02 15:12 Kenji Kaneshige
2006-03-02 15:14 ` [PATCH 1/4] PCI legacy I/O port free driver (take4) - Add no_ioport flag into pci_dev Kenji Kaneshige
` (4 more replies)
0 siblings, 5 replies; 24+ messages in thread
From: Kenji Kaneshige @ 2006-03-02 15:12 UTC (permalink / raw)
To: Andrew Morton, Greg KH
Cc: Linux Kernel Mailing List, linux-pci, 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
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH 1/4] PCI legacy I/O port free driver (take4) - Add no_ioport flag into pci_dev 2006-03-02 15:12 [PATCH 0/4] PCI legacy I/O port free driver (take4) Kenji Kaneshige @ 2006-03-02 15:14 ` Kenji Kaneshige 2006-03-02 15:16 ` [PATCH 2/4] PCI legacy I/O port free driver (take4) - Update Documentation/pci.txt Kenji Kaneshige ` (3 subsequent siblings) 4 siblings, 0 replies; 24+ messages in thread From: Kenji Kaneshige @ 2006-03-02 15:14 UTC (permalink / raw) To: Andrew Morton, Greg KH Cc: Kenji Kaneshige, Linux Kernel Mailing List, linux-pci 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; } ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/4] PCI legacy I/O port free driver (take4) - Update Documentation/pci.txt 2006-03-02 15:12 [PATCH 0/4] PCI legacy I/O port free driver (take4) Kenji Kaneshige 2006-03-02 15:14 ` [PATCH 1/4] PCI legacy I/O port free driver (take4) - Add no_ioport flag into pci_dev Kenji Kaneshige @ 2006-03-02 15:16 ` Kenji Kaneshige 2006-03-02 15:18 ` [PATCH 3/4] PCI legacy I/O port free driver (take4) - Make Intel e1000 driver legacy I/O port free Kenji Kaneshige ` (2 subsequent siblings) 4 siblings, 0 replies; 24+ messages in thread From: Kenji Kaneshige @ 2006-03-02 15:16 UTC (permalink / raw) To: Andrew Morton, Greg KH Cc: Kenji Kaneshige, Linux Kernel Mailing List, linux-pci 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"). ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/4] PCI legacy I/O port free driver (take4) - Make Intel e1000 driver legacy I/O port free 2006-03-02 15:12 [PATCH 0/4] PCI legacy I/O port free driver (take4) Kenji Kaneshige 2006-03-02 15:14 ` [PATCH 1/4] PCI legacy I/O port free driver (take4) - Add no_ioport flag into pci_dev Kenji Kaneshige 2006-03-02 15:16 ` [PATCH 2/4] PCI legacy I/O port free driver (take4) - Update Documentation/pci.txt Kenji Kaneshige @ 2006-03-02 15:18 ` Kenji Kaneshige 2006-03-02 15:20 ` [PATCH 4/4] PCI legacy I/O port free driver (take4) - Make Emulex lpfc " Kenji Kaneshige 2006-03-02 15:50 ` [PATCH 0/4] PCI legacy I/O port free driver (take4) Russell King 4 siblings, 0 replies; 24+ messages in thread From: Kenji Kaneshige @ 2006-03-02 15:18 UTC (permalink / raw) To: Andrew Morton, Greg KH Cc: Kenji Kaneshige, Linux Kernel Mailing List, linux-pci 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; + } } } ^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/4] PCI legacy I/O port free driver (take4) - Make Emulex lpfc driver legacy I/O port free 2006-03-02 15:12 [PATCH 0/4] PCI legacy I/O port free driver (take4) Kenji Kaneshige ` (2 preceding siblings ...) 2006-03-02 15:18 ` [PATCH 3/4] PCI legacy I/O port free driver (take4) - Make Intel e1000 driver legacy I/O port free Kenji Kaneshige @ 2006-03-02 15:20 ` Kenji Kaneshige 2006-03-02 15:50 ` [PATCH 0/4] PCI legacy I/O port free driver (take4) Russell King 4 siblings, 0 replies; 24+ messages in thread From: Kenji Kaneshige @ 2006-03-02 15:20 UTC (permalink / raw) To: Andrew Morton, Greg KH Cc: Kenji Kaneshige, Linux Kernel Mailing List, linux-pci 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)) ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-02 15:12 [PATCH 0/4] PCI legacy I/O port free driver (take4) Kenji Kaneshige ` (3 preceding siblings ...) 2006-03-02 15:20 ` [PATCH 4/4] PCI legacy I/O port free driver (take4) - Make Emulex lpfc " Kenji Kaneshige @ 2006-03-02 15:50 ` Russell King 2006-03-02 16:23 ` Kenji Kaneshige 2006-03-02 17:24 ` Grant Grundler 4 siblings, 2 replies; 24+ messages in thread From: Russell King @ 2006-03-02 15:50 UTC (permalink / raw) To: Kenji Kaneshige Cc: Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-02 15:50 ` [PATCH 0/4] PCI legacy I/O port free driver (take4) Russell King @ 2006-03-02 16:23 ` Kenji Kaneshige 2006-03-02 16:41 ` Greg KH 2006-03-02 17:24 ` Grant Grundler 1 sibling, 1 reply; 24+ messages in thread From: Kenji Kaneshige @ 2006-03-02 16:23 UTC (permalink / raw) To: Russell King; +Cc: Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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? > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-02 16:23 ` Kenji Kaneshige @ 2006-03-02 16:41 ` Greg KH 0 siblings, 0 replies; 24+ messages in thread From: Greg KH @ 2006-03-02 16:41 UTC (permalink / raw) To: Kenji Kaneshige Cc: Russell King, Andrew Morton, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-02 15:50 ` [PATCH 0/4] PCI legacy I/O port free driver (take4) Russell King 2006-03-02 16:23 ` Kenji Kaneshige @ 2006-03-02 17:24 ` Grant Grundler 2006-03-02 18:00 ` Russell King 2006-03-02 19:34 ` Russell King 1 sibling, 2 replies; 24+ messages in thread From: Grant Grundler @ 2006-03-02 17:24 UTC (permalink / raw) To: Russell King Cc: Kenji Kaneshige, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-02 17:24 ` Grant Grundler @ 2006-03-02 18:00 ` Russell King 2006-03-02 18:12 ` Jeff Garzik 2006-03-02 19:23 ` Grant Grundler 2006-03-02 19:34 ` Russell King 1 sibling, 2 replies; 24+ messages in thread From: Russell King @ 2006-03-02 18:00 UTC (permalink / raw) To: Grant Grundler Cc: Kenji Kaneshige, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-02 18:00 ` Russell King @ 2006-03-02 18:12 ` Jeff Garzik 2006-03-02 19:13 ` Russell King 2006-03-02 19:23 ` Grant Grundler 1 sibling, 1 reply; 24+ messages in thread From: Jeff Garzik @ 2006-03-02 18:12 UTC (permalink / raw) To: Russell King Cc: Grant Grundler, Kenji Kaneshige, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-02 18:12 ` Jeff Garzik @ 2006-03-02 19:13 ` Russell King 2006-03-02 20:01 ` Jeff Garzik 0 siblings, 1 reply; 24+ messages in thread From: Russell King @ 2006-03-02 19:13 UTC (permalink / raw) To: Jeff Garzik Cc: Grant Grundler, Kenji Kaneshige, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-02 19:13 ` Russell King @ 2006-03-02 20:01 ` Jeff Garzik 0 siblings, 0 replies; 24+ messages in thread From: Jeff Garzik @ 2006-03-02 20:01 UTC (permalink / raw) To: Russell King Cc: Grant Grundler, Kenji Kaneshige, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-02 18:00 ` Russell King 2006-03-02 18:12 ` Jeff Garzik @ 2006-03-02 19:23 ` Grant Grundler 1 sibling, 0 replies; 24+ messages in thread From: Grant Grundler @ 2006-03-02 19:23 UTC (permalink / raw) To: Grant Grundler, Kenji Kaneshige, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-02 17:24 ` Grant Grundler 2006-03-02 18:00 ` Russell King @ 2006-03-02 19:34 ` Russell King 2006-03-02 19:50 ` Roland Dreier ` (2 more replies) 1 sibling, 3 replies; 24+ messages in thread From: Russell King @ 2006-03-02 19:34 UTC (permalink / raw) To: Grant Grundler Cc: Kenji Kaneshige, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-02 19:34 ` Russell King @ 2006-03-02 19:50 ` Roland Dreier 2006-03-03 3:17 ` Kenji Kaneshige 2006-03-10 2:10 ` Adam Belay 2 siblings, 0 replies; 24+ messages in thread From: Roland Dreier @ 2006-03-02 19:50 UTC (permalink / raw) To: Grant Grundler Cc: Kenji Kaneshige, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-02 19:34 ` Russell King 2006-03-02 19:50 ` Roland Dreier @ 2006-03-03 3:17 ` Kenji Kaneshige 2006-03-03 6:59 ` Kenji Kaneshige 2006-03-10 2:10 ` Adam Belay 2 siblings, 1 reply; 24+ messages in thread From: Kenji Kaneshige @ 2006-03-03 3:17 UTC (permalink / raw) To: Russell King Cc: Grant Grundler, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-03 3:17 ` Kenji Kaneshige @ 2006-03-03 6:59 ` Kenji Kaneshige 2006-03-06 1:38 ` Kenji Kaneshige 0 siblings, 1 reply; 24+ messages in thread From: Kenji Kaneshige @ 2006-03-03 6:59 UTC (permalink / raw) To: Russell King, Grant Grundler Cc: Kenji Kaneshige, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-03 6:59 ` Kenji Kaneshige @ 2006-03-06 1:38 ` Kenji Kaneshige 0 siblings, 0 replies; 24+ messages in thread From: Kenji Kaneshige @ 2006-03-06 1:38 UTC (permalink / raw) To: Kenji Kaneshige Cc: Russell King, Grant Grundler, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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. > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-02 19:34 ` Russell King 2006-03-02 19:50 ` Roland Dreier 2006-03-03 3:17 ` Kenji Kaneshige @ 2006-03-10 2:10 ` Adam Belay 2006-03-10 4:10 ` Kenji Kaneshige 2006-03-10 8:33 ` Russell King 2 siblings, 2 replies; 24+ messages in thread From: Adam Belay @ 2006-03-10 2:10 UTC (permalink / raw) To: Russell King Cc: Kenji Kaneshige, Grant Grundler, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-10 2:10 ` Adam Belay @ 2006-03-10 4:10 ` Kenji Kaneshige 2006-03-10 7:49 ` Russell King 2006-03-10 8:33 ` Russell King 1 sibling, 1 reply; 24+ messages in thread From: Kenji Kaneshige @ 2006-03-10 4:10 UTC (permalink / raw) To: Adam Belay Cc: Russell King, Grant Grundler, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-10 4:10 ` Kenji Kaneshige @ 2006-03-10 7:49 ` Russell King 0 siblings, 0 replies; 24+ messages in thread From: Russell King @ 2006-03-10 7:49 UTC (permalink / raw) To: Kenji Kaneshige Cc: Adam Belay, Grant Grundler, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-10 2:10 ` Adam Belay 2006-03-10 4:10 ` Kenji Kaneshige @ 2006-03-10 8:33 ` Russell King 2006-03-13 5:47 ` Kenji Kaneshige 1 sibling, 1 reply; 24+ messages in thread From: Russell King @ 2006-03-10 8:33 UTC (permalink / raw) To: Adam Belay, Kenji Kaneshige, Grant Grundler, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/4] PCI legacy I/O port free driver (take4) 2006-03-10 8:33 ` Russell King @ 2006-03-13 5:47 ` Kenji Kaneshige 0 siblings, 0 replies; 24+ messages in thread From: Kenji Kaneshige @ 2006-03-13 5:47 UTC (permalink / raw) To: Russell King Cc: Adam Belay, Grant Grundler, Andrew Morton, Greg KH, Linux Kernel Mailing List, linux-pci 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. > ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2006-03-13 5:49 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2006-03-02 15:12 [PATCH 0/4] PCI legacy I/O port free driver (take4) Kenji Kaneshige 2006-03-02 15:14 ` [PATCH 1/4] PCI legacy I/O port free driver (take4) - Add no_ioport flag into pci_dev Kenji Kaneshige 2006-03-02 15:16 ` [PATCH 2/4] PCI legacy I/O port free driver (take4) - Update Documentation/pci.txt Kenji Kaneshige 2006-03-02 15:18 ` [PATCH 3/4] PCI legacy I/O port free driver (take4) - Make Intel e1000 driver legacy I/O port free Kenji Kaneshige 2006-03-02 15:20 ` [PATCH 4/4] PCI legacy I/O port free driver (take4) - Make Emulex lpfc " Kenji Kaneshige 2006-03-02 15:50 ` [PATCH 0/4] PCI legacy I/O port free driver (take4) Russell King 2006-03-02 16:23 ` Kenji Kaneshige 2006-03-02 16:41 ` Greg KH 2006-03-02 17:24 ` Grant Grundler 2006-03-02 18:00 ` Russell King 2006-03-02 18:12 ` Jeff Garzik 2006-03-02 19:13 ` Russell King 2006-03-02 20:01 ` Jeff Garzik 2006-03-02 19:23 ` Grant Grundler 2006-03-02 19:34 ` Russell King 2006-03-02 19:50 ` Roland Dreier 2006-03-03 3:17 ` Kenji Kaneshige 2006-03-03 6:59 ` Kenji Kaneshige 2006-03-06 1:38 ` Kenji Kaneshige 2006-03-10 2:10 ` Adam Belay 2006-03-10 4:10 ` Kenji Kaneshige 2006-03-10 7:49 ` Russell King 2006-03-10 8:33 ` Russell King 2006-03-13 5:47 ` Kenji Kaneshige
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).