(-mm)pci_disable_device function clear bars_enabled element
Subsystems (6)
Hi,
I found that in -mm tree, function pci_disable_device() clears bars_enabled variable, so that pci_release_regions can not release reserved PCI I/O and memory resource. Some device driver programs in kernel tree call pci_release_regions function after pci_disable_device(), that will cause some problem. And I do not know whether pci_disable_device() function should be modified or device drivers should be adjusted.
Thanks
bibo,mao
Quoted 5 lines · bibo,mao
I found that in -mm tree, function
pci_disable_device()clearsbars_enabledvariable, so thatpci_release_regionscan not release reserved PCI I/O and memory resource. Some device driver programs in kernel tree callpci_release_regionsfunction afterpci_disable_device(), that will cause some problem.
It's coming from Kaneshige-san's patch:
pci-legacy-i-o-port-free-driver-changes-to-generic-pci-code.patch
This patch assumes that pci_request_region() will always be called after pci_enable_device() and pci_release_region() will always be called before pci_disable_device(). We cannot make this assumption,since it's perfectly legal to disable a device first and then release it's regions. So, I think that patch needs to change.
thanks,
Rajesh
Quoted 6 lines · Rajesh Shah
This patch assumes that
pci_request_region()will always be called afterpci_enable_device()andpci_release_region()will always be called beforepci_disable_device(). We cannot make this assumption,since it's perfectly legal to disable a device first and then release it's regions. So, I think that patch needs to change.
Patch below clarifies comments in Documentation/pci.txt. Greg, can you apply?
(feel free to edit it a bit more)
thanks,
grant
Documentation/pci.txt +11 -3
@@ -213,9 +213,17 @@ have been remapped by the kernel. See Documentation/IO-mapping.txt for how to access device memory. - You still need to call request_region() for I/O regions and -request_mem_region() for memory regions to make sure nobody else is using the -same device. + The device driver needs to call pci_request_region() to make sure +no other device is already using the same resource. The driver is expected +to determine MMIO and IO Port resource availability _before_ calling +pci_enable_device(). Conversely, drivers should call pci_release_region() +_after_ calling pci_disable_device(). The idea is to prevent two devices +colliding on the same address range. + +Generic flavors of pci_request_region() are request_mem_region() +(for MMIO ranges) and request_region() (for IO Port ranges). +Use these for address resources that are not described by "normal" PCI +interfaces (e.g. BAR). All interrupt handlers should be registered with SA_SHIRQ and use the devid to map IRQs to devices (remember that all PCI interrupts are shared).
Documentation/pci.txt have been remapped by the kernel. · line 222
Quoted 7 lines · Grant Grundler
Documentation/pci.txt +7
-0+ The device driver needs to call pci_request_region() to make sure +no other device is already using the same resource. The driver is expected +to determine MMIO and IO Port resource availability _before_ calling +pci_enable_device(). Conversely, drivers should call pci_release_region() +_after_ calling pci_disable_device(). The idea is to prevent two devices +colliding on the same address range. +
A quick look in the drivers directory shows that _lots_ of drivers violate this rule. In fact, I suspect Kaneshige-san made the original incorrect assumption since there were so many drivers out there which do it in the wrong order.
Rajesh
Hi,
Sorry for replying late.
I understand that I had a woring assumption and I need to change my patch. I'll post the fixed one as soon as possible.
As Rajesh pointed out, there are many drivers which initialize the device with the wrong order. They should be fixed. I would like to confirm the correct order to initialize the device again. Is the following correct order?
(1) pci_request_regions()
(2) pci_enable_device()
(3) request_irq()
(4) free_irq()
(5) pci_disable_device()
(6) pci_release_regions()
Thanks,
Kenji Kaneshige
Quoted 17 lines · Rajesh Shah
Documentation/pci.txt have been remapped by the kernel. · line 222
Quoted 7 lines · Grant Grundler
Documentation/pci.txt +7
-0+ The device driver needs to call pci_request_region() to make sure +no other device is already using the same resource. The driver is expected +to determine MMIO and IO Port resource availability _before_ calling +pci_enable_device(). Conversely, drivers should call pci_release_region() +_after_ calling pci_disable_device(). The idea is to prevent two devices +colliding on the same address range. +A quick look in the drivers directory shows that _lots_ of drivers violate this rule. In fact, I suspect Kaneshige-san made the original incorrect assumption since there were so many drivers out there which do it in the wrong order.
Rajesh
On Fri, Jun 02, 2006 at 11:57:36AM +0900, Kenji Kaneshige wrote:
...
Quoted 2 lines
As Rajesh pointed out, there are many drivers which initialize the device with the wrong order. They should be fixed.
Then you also agree with the patch to pci.txt?
Quoted 10 lines
I would like to
confirm the correct order to initialize the device again. Is the following correct order?(1) pci_request_regions() (2) pci_enable_device() (3) request_irq() (4) free_irq() (5) pci_disable_device() (6) pci_release_regions()
Yes, that's what I would prefer and would like to see reccomended. Would you like to see that order listed (like you have above) in the pci.txt file?
A less precise list is in the first section of Documentation/pci.txt.
[ TODO: Can someone define which kernel versions implement "new style"? ]
There's more to this list unfortunately:
DMA mask settings, MSI support, power state
And probably a few more that I'm not thinking of right now.
Restructing the document to list the steps, indicate which are optional, and describe each step in order is more than I can deal with right now. Section 3 and 5 cover most of the material but aren't as clear as Kenji's list.
thanks,
grant
Quoted 8 lines · Grant Grundler
On Fri, Jun 02, 2006 at 11:57:36AM +0900, Kenji Kaneshige wrote:
...Quoted 2 lines
As Rajesh pointed out, there are many drivers which initialize the device with the wrong order. They should be fixed.
Then you also agree with the patch to pci.txt?
Yes. I agree with you.
Quoted 12 lines
Quoted 10 lines
I would like to
confirm the correct order to initialize the device again. Is the following correct order?(1)
pci_request_regions()
(2)pci_enable_device()
(3)request_irq()
(4)free_irq()
(5)pci_disable_device()
(6)pci_release_regions()
Quoted 4 lines
Yes, that's what I would prefer and would like to see reccomended. Would you like to see that order listed (like you have above) in the pci.txt file?
I think that list would be very useful. But as you said, there are other steps remaining than ones I came up with at once. I can't deal with the steps of all of them...
BTW, Section 3 says "Before you do anything with the device you've found, you need to enable it by calling pci_enable_device()...". I think it would be one of the causes of misunderstanding the order between pci_request_regions() and pci_enable_device().
Thanks,
Kenji Kaneshige
Quoted 17 lines
A less precise list is in the first section of
Documentation/pci.txt.[ TODO: Can someone define which kernel versions implement "new style"? ]
There's more to this list unfortunately:
DMA mask settings, MSI support, power stateAnd probably a few more that I'm not thinking of right now.
Restructing the document to list the steps, indicate which are optional, and describe each step in order is more than I can deal with right now. Section 3 and 5 cover most of the material but aren't as clear as Kenji's list.
thanks,
grant
Quoted 3 lines · Kenji Kaneshige
I think that list would be very useful. But as you said, there are other steps remaining than ones I came up with at once. I can't deal with the steps of all of them...
Ok. I'm motivated to clean up/rewrite that file... Greg, you want that peice meal or all in one patch?
Quoted 4 lines
BTW, Section 3 says "Before you do anything with the device you've found, you need to enable it by calling
pci_enable_device()...". I think it would be one of the causes of misunderstanding the order betweenpci_request_regions()andpci_enable_device().
I agree. I'll fix that.
thanks,
grant
Quoted 7 lines · Grant Grundler
Quoted 3 lines · Kenji Kaneshige
I think that list would be very useful. But as you said, there are other steps remaining than ones I came up with at once. I can't deal with the steps of all of them...
Ok. I'm motivated to clean up/rewrite that file... Greg, you want that peice meal or all in one patch?
All in one is probably easier, unless you think you can break it up into logicial pieces.
thanks,
greg k-h
PATCH 1/1 bugfix: PCI legacy I/O port free driver
In reply to Kenji KaneshigeHi Andrew, Greg,
Here is a patche to fix a bug that pci_request_regions() doesn't work when it is called after pci_disable_device(), which was reported at:
http://marc.theaimsgroup.com/?l=linux-kernel&m=114914585213991&w=2
This bug is introduced by the following "PCI legacy I/O port free driver" patches currently in 2.6.17-rc5-mm3.
o gregkh-pci-pci-legacy-i-o-port-free-driver-changes-to-generic-pci-code.patch
o gregkh-pci-pci-legacy-i-o-port-free-driver-make-emulex-lpfc-driver-legacy-i-o-port-free.patch
o gregkh-pci-pci-legacy-i-o-port-free-driver-make-intel-e1000-driver-legacy-i-o-port-free.patch
o gregkh-pci-pci-legacy-i-o-port-free-driver-update-documentation-pci_txt.patch
This patch is against 2.6.17-rc5-mm3. Please see the header of the patch about details.
If reposting the fixed version of PCI legacy I/O port free driver patches against the latest Linus tree are preferred rather than this patch, please let me know.
Thanks,
Kenji Kaneshige
This patch fixes the bug that pci_release_regions() doesn't work when it is called after pci_disable_device(), which is reported at:
http://marc.theaimsgroup.com/?l=linux-kernel&m=114914585213991&w=2
The root cause of this bug is "PCI legacy I/O port free driver" patches had been implemented with a wrong assumption that pci_release_regions() is called before pci_disable_device(), while it should be called _after_ pci_disable_device().
To fix the bug, this patch makes the follwoing changes:
o Changed pci_request_regions()/pci_release_regions() not to refer bars_enabled bitmask.
o Introduce new pci_request_selected_regions() and pci_release_selected_regions() for PCI legacy I/O port free drivers to request/release only the selected regions.
o Added the description about pci_request_selected_regions() and pci_release_selected_regions().
o Changed intel e1000 driver to use pci_request_selected_regions() and pci_release_selected_regions().
o Changed Emulex lpfc driver to use pci_request_selected_regions() and pci_release_selected_regions().
--- Documentation/pci.txt | 5 ++ drivers/net/e1000/e1000_main.c | 7 ++- drivers/pci/pci.c | 77 ++++++++++++++++++++++++----------------- drivers/scsi/lpfc/lpfc_init.c | 7 ++- include/linux/pci.h | 2 + 5 files changed, 60 insertions(+), 38 deletions(-)
Documentation/pci.txt +4 -1
@@ -311,7 +311,10 @@ If your PCI device driver doesn't need I/O port resources assigned to I/O Port BARs, you should use pci_enable_device_bars() instead of pci_enable_device() in order not to enable I/O port regions for the -corresponding devices. +corresponding devices. In addition, you should use +pci_request_selected_regions()/pci_release_selected_regions() instead +of pci_request_regions()/pci_release_regions() in order not to +request/release 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.
drivers/net/e1000/e1000_main.c +4 -3
@@ -616,7 +616,8 @@ pci_using_dac =0 ; } -if ((err =pci_request_regions (pdev, e1000_driver_name))) + err =pci_request_selected_regions (pdev, bars, e1000_driver_name); +if (err)return err;pci_set_master (pdev);
@@ -866,7 +867,7 @@ err_ioremap:free_netdev (netdev); err_alloc_etherdev: -pci_release_regions (pdev); +pci_release_selected_regions (pdev, bars);return err; }
@@ -921,7 +922,7 @@#endif iounmap (adapter->hw.hw_addr); -pci_release_regions (pdev); +pci_release_selected_regions (pdev, adapter->bars);free_netdev (netdev);
drivers/pci/pci.c +46 -31
113 lines not on this page · raw message
drivers/scsi/lpfc/lpfc_init.c +4 -3
@@ -1429,7 +1429,7 @@if (pci_enable_device_bars (pdev, bars)) goto out; -if (pci_request_regions (pdev,LPFC_DRIVER_NAME )) +if (pci_request_selected_regions (pdev, bars,LPFC_DRIVER_NAME )) goto out_disable_device; host =scsi_host_alloc (&lpfc_template,sizeof (struct lpfc_hba ));
@@ -1721,7 +1721,7 @@ phba->host =NULL ;scsi_host_put (host); out_release_regions: -pci_release_regions (pdev); +pci_release_selected_regions (pdev, bars); out_disable_device:pci_disable_device (pdev); out:
@@ -1735,6 +1735,7 @@struct Scsi_Host *host =pci_get_drvdata (pdev);struct lpfc_hba *phba = (struct lpfc_hba *)host->hostdata;unsigned long iflag; +int bars =pci_select_bars (pdev,IORESOURCE_MEM );lpfc_free_sysfs_attr (phba);
@@ -1778,7 +1779,7 @@iounmap (phba->ctrl_regs_memmap_p);iounmap (phba->slim_memmap_p); -pci_release_regions (phba->pcidev); +pci_release_selected_regions (phba->pcidev, bars);pci_disable_device (phba->pcidev);idr_remove (&lpfc_hba_index, phba->brd_no);
include/linux/pci.h +2 -0
@@ -531,6 +531,8 @@void pci_release_regions (struct pci_dev *);int pci_request_region (struct pci_dev *,int ,const char *);void pci_release_region (struct pci_dev *,int ); +int pci_request_selected_regions (struct pci_dev *,int ,const char *); +void pci_release_selected_regions (struct pci_dev *,int );/* drivers/pci/bus.c */ int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
Re: [BUG][PATCH 2.6.17-rc5-mm3] bugfix: PCI legacy I/O port free driver
Quoted 15 lines · Kenji Kaneshige
Hi Andrew, Greg,
Here is a patche to fix a bug that
pci_request_regions()doesn't work when it is called afterpci_disable_device(), which was reported at:http://marc.theaimsgroup.com/?l=linux-kernel&m=114914585213991&w=2This bug is introduced by the following "PCI legacy I/O port free driver" patches currently in 2.6.17-rc5-mm3.
o gregkh-pci-pci-legacy-i-o-port-free-driver-changes-to-generic-pci-code.patch
o gregkh-pci-pci-legacy-i-o-port-free-driver-make-emulex-lpfc-driver-legacy-i-o-port-free.patch
o gregkh-pci-pci-legacy-i-o-port-free-driver-make-intel-e1000-driver-legacy-i-o-port-free.patch
o gregkh-pci-pci-legacy-i-o-port-free-driver-update-documentation-pci_txt.patch
Quoted 6 lines
This patch is against 2.6.17-rc5-mm3. Please see the header of the patch about details.
If reposting the fixed version of PCI legacy I/O port free driver patches against the latest Linus tree are preferred rather than this patch, please let me know.
I think a new set of patches would be the best, as that way when I apply them to Linus's tree, there is no point in the patch history that has a problem that people might hit.
So, care to just resend the above 4 patches with your fix included? Is that easy to do?
thanks,
greg k-h
Re: [BUG][PATCH 2.6.17-rc5-mm3] bugfix: PCI legacy I/O port free driver
Quoted 25 lines · Greg KH
Quoted 15 lines · Kenji Kaneshige
Hi Andrew, Greg,
Here is a patche to fix a bug that
pci_request_regions()doesn't work when it is called afterpci_disable_device(), which was reported at:http://marc.theaimsgroup.com/?l=linux-kernel&m=114914585213991&w=2This bug is introduced by the following "PCI legacy I/O port free driver" patches currently in 2.6.17-rc5-mm3.
o gregkh-pci-pci-legacy-i-o-port-free-driver-changes-to-generic-pci-code.patch
o gregkh-pci-pci-legacy-i-o-port-free-driver-make-emulex-lpfc-driver-legacy-i-o-port-free.patch
o gregkh-pci-pci-legacy-i-o-port-free-driver-make-intel-e1000-driver-legacy-i-o-port-free.patch
o gregkh-pci-pci-legacy-i-o-port-free-driver-update-documentation-pci_txt.patchQuoted 6 lines
This patch is against 2.6.17-rc5-mm3. Please see the header of the patch about details.
If reposting the fixed version of PCI legacy I/O port free driver patches against the latest Linus tree are preferred rather than this patch, please let me know.
Quoted 7 lines
I think a new set of patches would be the best, as that way when I apply them to Linus's tree, there is no point in the patch history that has a problem that people might hit.
So, care to just resend the above 4 patches with your fix included? Is that easy to do?
Sure.
I've already finished those patches and I'm checking them now. I'll resend them soon after some tests.
Thanks,
Kenji Kaneshige
Re: [BUG][PATCH 2.6.17-rc5-mm3] bugfix: PCI legacy I/O port free driver
In reply to Greg KHQuoted 25 lines · Greg KH
Quoted 15 lines · Kenji Kaneshige
Hi Andrew, Greg,
Here is a patche to fix a bug that
pci_request_regions()doesn't work when it is called afterpci_disable_device(), which was reported at:http://marc.theaimsgroup.com/?l=linux-kernel&m=114914585213991&w=2This bug is introduced by the following "PCI legacy I/O port free driver" patches currently in 2.6.17-rc5-mm3.
o gregkh-pci-pci-legacy-i-o-port-free-driver-changes-to-generic-pci-code.patch
o gregkh-pci-pci-legacy-i-o-port-free-driver-make-emulex-lpfc-driver-legacy-i-o-port-free.patch
o gregkh-pci-pci-legacy-i-o-port-free-driver-make-intel-e1000-driver-legacy-i-o-port-free.patch
o gregkh-pci-pci-legacy-i-o-port-free-driver-update-documentation-pci_txt.patchQuoted 6 lines
This patch is against 2.6.17-rc5-mm3. Please see the header of the patch about details.
If reposting the fixed version of PCI legacy I/O port free driver patches against the latest Linus tree are preferred rather than this patch, please let me know.
Quoted 7 lines
I think a new set of patches would be the best, as that way when I apply them to Linus's tree, there is no point in the patch history that has a problem that people might hit.
So, care to just resend the above 4 patches with your fix included? Is that easy to do?
Hi Greg,
Here is a updated set of patches for PCI legacy I/O port free driver patches. It is against 2.6.17-rc6. It contains the following patches.
o [PATCH 1/4] Changes to generic pci code
o [PATCH 2/4] Update Documentation/pci.txt
o [PATCH 3/4] Make Intel e1000 driver legacy I/O port free
o [PATCH 4/4] Make Emulex lpfc driver legacy I/O port free
Each of them are corresponding to the following patches in your tree.
o pci-legacy-i-o-port-free-driver-changes-to-generic-pci-code.patch
o pci-legacy-i-o-port-free-driver-update-documentation-pci_txt.patch
o pci-legacy-i-o-port-free-driver-make-intel-e1000-driver-legacy-i-o-port-free.patch
o pci-legacy-i-o-port-free-driver-make-emulex-lpfc-driver-legacy-i-o-port-free.patch
Thanks,
Kenji Kaneshige
PATCH 1/4 Changes to generic pci code
This patch adds the following changes into generic PCI code especially for PCI legacy I/O port free drivers.
- Moved the following two things from pci_enable_device() into pci_enable_device_bars(). By this change, we can use pci_enable_device_bars() to enable only the specific regions.
o Call pci_fixup_device() on the device
o Set dev->is_enabled
- Added new field 'bars_enabled' into struct pci_device to remember which BARs already enabled. This new field is initialized at pci_enable_device_bars() time and cleared at pci_disable_device() time. This is needed for pci_default_resume() to enable appropriate BARs.
- Added new pci_request_selected_regions() and pci_release_selected_regions() for PCI legacy I/O port free drivers in order to request/release only the selected regions.
- Added helper routine pci_select_bars() which makes proper mask of BARs from the specified resource type. This would be very helpful for users of pci_enable_device_bars().
--- drivers/pci/pci-driver.c | 3 + drivers/pci/pci.c | 86 ++++++++++++++++++++++++++++++++++++----------- include/linux/pci.h | 4 ++ 3 files changed, 73 insertions(+), 20 deletions(-)
drivers/pci/pci-driver.c +2 -1
@@ -293,7 +293,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);
drivers/pci/pci.c +67 -19
146 lines not on this page · raw message
include/linux/pci.h +4 -0
@@ -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)
@@ -497,6 +498,7 @@void pci_update_resource (struct pci_dev *dev,struct resource *res,int resno);int pci_assign_resource (struct pci_dev *dev,int i);void pci_restore_bars (struct pci_dev *dev); +int pci_select_bars (struct pci_dev *dev,unsigned long flags);/* ROM control related routines */ void __iomem __must_check *pci_map_rom (struct pci_dev *pdev,size_t *size);
@@ -525,6 +527,8 @@void pci_release_regions (struct pci_dev *);int pci_request_region (struct pci_dev *,int ,const char *);void pci_release_region (struct pci_dev *,int ); +int pci_request_selected_regions (struct pci_dev *,int ,const char *); +void pci_release_selected_regions (struct pci_dev *,int );/* drivers/pci/bus.c */ int pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
PATCH 2/4 Update Documentation/pci.txt
In reply to Kenji KaneshigeThis patch adds the description about legacy I/O port free driver into Documentation/pci.txt.
--- Documentation/pci.txt | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 70 insertions(+)
Documentation/pci.txt +70 -0
@@ -279,3 +279,73 @@ 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, you should use pci_enable_device_bars() instead of +pci_enable_device() in order not to enable I/O port regions for the +corresponding devices. In addition, you should use +pci_request_selected_regions()/pci_release_selected_regions() instead +of pci_request_regions()/pci_release_regions() in order not to +request/release I/O port regions for the corresponding devices. + +[1] Some systems support 64KB I/O port space per PCI segment. +[2] Some PCI-to-PCI bridges support optional 1KB aligned I/O base. + + +10. MMIO Space and "Write Posting" +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +Converting a driver from using I/O Port space to using MMIO space +often requires some additional changes. Specifically, "write posting" +needs to be handled. Most drivers (e.g. tg3, acenic, sym53c8xx_2) +already do. I/O Port space guarantees write transactions reach the PCI +device before the CPU can continue. Writes to MMIO space allow to CPU +continue before the transaction reaches the PCI device. HW weenies +call this "Write Posting" because the write completion is "posted" to +the CPU before the transaction has reached it's destination. + +Thus, timing sensitive code should add readl() where the CPU is +expected to wait before doing other work. The classic "bit banging" +sequence works fine for I/O Port space: + + for (i=8; --i; val >>= 1) { + outb(val & 1, ioport_reg); /* write bit */ + udelay(10); + } + +The same sequence for MMIO space should be: + + for (i=8; --i; val >>= 1) { + writeb(val & 1, mmio_reg); /* write bit */ + readb(safe_mmio_reg); /* flush posted write */ + udelay(10); + } + +It is important that "safe_mmio_reg" not have any side effects that +interferes with the correct operation of the device. + +Another case to watch out for is when resetting a PCI device. Use PCI +Configuration space reads to flush the writel(). This will gracefully +handle the PCI master abort on all platforms if the PCI device is +expected to not respond to a readl(). Most x86 platforms will allow +MMIO reads to master abort (aka "Soft Fail") and return garbage +(e.g. ~0). But many RISC platforms will crash (aka "Hard Fail").
PATCH 3/4 Make Intel e1000 driver legacy I/O port free
In reply to Kenji KaneshigeThis patch makes Intel e1000 driver legacy I/O port free.
--- drivers/net/e1000/e1000.h | 6 + drivers/net/e1000/e1000_main.c | 130 ++++++++++++++++++++++------------------- 2 files changed, 75 insertions(+), 61 deletions(-)
drivers/net/e1000/e1000.h +4 -2
@@ -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 ;
@@ -338,6 +339,7 @@#ifdef NETIF_F_TSO boolean_t tso_force;#endif +int bars;/* BARs to be enabled */ };
drivers/net/e1000/e1000_main.c +71 -59
178 lines not on this page · raw message
Re: [PATCH 3/4] Make Intel e1000 driver legacy I/O port free
Quoted 3 lines · Kenji Kaneshige
This patch makes Intel e1000 driver legacy I/O port free.
Signed-off-by Kenji Kaneshige <[email protected]>
(adding netdev and the other e1000 maintainers to cc:)
without sending this to any of the listed e1000 maintainers???? *and* not even including netdev???
I'm going to take a look at it first, and have some other colleagues look at this and the impact, which is unclear to me at the moment. Please don't commit
this like this.
Auke
Quoted 226 lines
--- drivers/net/e1000/e1000.h | 6 + drivers/net/e1000/e1000_main.c | 130 ++++++++++++++++++++++------------------- 2 files changed, 75 insertions(+), 61 deletions(-)Index:
linux-2.6.17-rc6/drivers/net/e1000/e1000.h
===================================================================drivers/net/e1000/e1000.h +4
-2@@ -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 ;@@ -338,6 +339,7 @@#ifdef NETIF_F_TSO boolean_t tso_force;#endif +int bars;/* BARs to be enabled */ };Index:
linux-2.6.17-rc6/drivers/net/e1000/e1000_main.c
===================================================================drivers/net/e1000/e1000_main.c +71
-60@@ -86,54 +86,54 @@* {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 (0x1096 ), -INTEL_E1000_ETHERNET_DEVICE (0x1098 ), -INTEL_E1000_ETHERNET_DEVICE (0x1099 ), -INTEL_E1000_ETHERNET_DEVICE (0x109A ), -INTEL_E1000_ETHERNET_DEVICE (0x10B5 ), -INTEL_E1000_ETHERNET_DEVICE (0x10B9 ), +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 (0x1096 ,0 ), +INTEL_E1000_ETHERNET_DEVICE (0x1098 ,0 ), +INTEL_E1000_ETHERNET_DEVICE (0x1099 ,E1000_NO_IOPORT ), +INTEL_E1000_ETHERNET_DEVICE (0x109A ,E1000_NO_IOPORT ), +INTEL_E1000_ETHERNET_DEVICE (0x10B5 ,E1000_NO_IOPORT ), +INTEL_E1000_ETHERNET_DEVICE (0x10B9 ,0 ),/* required last entry */ {0 ,} };@@ -621,7 +621,14 @@int i, err, pci_using_dac;uint16_t eeprom_data;uint16_t eeprom_apme_mask =E1000_EEPROM_APME ; -if ((err =pci_enable_device (pdev))) +int bars; + +if (ent->driver_data &E1000_NO_IOPORT ) + bars =pci_select_bars (pdev,IORESOURCE_MEM ); +else + bars =pci_select_bars (pdev,IORESOURCE_MEM |IORESOURCE_IO ); + +if ((err =pci_enable_device_bars (pdev, bars)))return err;if (!(err =pci_set_dma_mask (pdev,DMA_64BIT_MASK ))) {@@ -634,7 +641,8 @@ pci_using_dac =0 ; } -if ((err =pci_request_regions (pdev, e1000_driver_name))) + err =pci_request_selected_regions (pdev, bars, e1000_driver_name); +if (err)return err;pci_set_master (pdev);@@ -654,6 +662,7 @@ adapter->pdev = pdev; adapter->hw.back = adapter; adapter->msg_enable = (1 << debug) -1 ; + adapter->bars = bars; mmio_start =pci_resource_start (pdev,BAR_0 ); mmio_len =pci_resource_len (pdev,BAR_0 );@@ -664,12 +673,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 ; + } } }@@ -880,7 +892,7 @@ err_ioremap:free_netdev (netdev); err_alloc_etherdev: -pci_release_regions (pdev); +pci_release_selected_regions (pdev, bars);return err; }@@ -935,7 +947,7 @@#endif iounmap (adapter->hw.hw_addr); -pci_release_regions (pdev); +pci_release_selected_regions (pdev, adapter->bars);free_netdev (netdev);@@ -4577,7 +4589,7 @@if (retval)DPRINTK (PROBE ,ERR ,"Error in setting power state\n" );e1000_pci_restore_state (adapter); - ret_val =pci_enable_device (pdev); + ret_val =pci_enable_device_bars (pdev, adapter->bars);pci_set_master (pdev); retval =pci_enable_wake (pdev, PCI_D3hot,0 ); -To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] Make Intel e1000 driver legacy I/O port free
Quoted 11 lines · Auke Kok
Quoted 3 lines · Kenji Kaneshige
This patch makes Intel e1000 driver legacy I/O port free.
Signed-off-by Kenji Kaneshige <[email protected]>(adding netdev and the other e1000 maintainers to cc:)
without sending this to any of the listed e1000 maintainers???? *and* not even including netdev???
I'm sorry about that.
Quoted 1 line
I'm going to take a look at it first, and have some other colleagues look at this and the impact, which is unclear to me at the moment.
Thank you for reviewing.
What this patch try to do is not to request/enable I/O port regions if the device can be handled without using I/O port so that the device can work even if I/O port resource is not assigned to it on the large servers. Please see the "[PATCH 2/4] Update Document/pci.txt" about details.
Thanks,
Kenji Kaneshige
Quoted 239 lines
Please don't commit this like this.
Auke
Quoted 231 lines
--- drivers/net/e1000/e1000.h | 6 + drivers/net/e1000/e1000_main.c | 130 ++++++++++++++++++++++------------------- 2 files changed, 75 insertions(+), 61 deletions(-)Index:
linux-2.6.17-rc6/drivers/net/e1000/e1000.h
===================================================================
---linux-2.6.17-rc6.orig/drivers/net/e1000/e1000.h2006-06-06
21:39:11.000000000 +0900
+++linux-2.6.17-rc6/drivers/net/e1000/e1000.h2006-06-06
21:56:41.000000000 +0900drivers/net/e1000/e1000.h +4
-2@@ -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 ;@@ -338,6 +339,7 @@#ifdef NETIF_F_TSO boolean_t tso_force;#endif +int bars;/* BARs to be enabled */ };Index:
linux-2.6.17-rc6/drivers/net/e1000/e1000_main.c
===================================================================
---linux-2.6.17-rc6.orig/drivers/net/e1000/e1000_main.c2006-06-06
21:39:11.000000000 +0900
+++linux-2.6.17-rc6/drivers/net/e1000/e1000_main.c2006-06-06
21:56:41.000000000 +0900drivers/net/e1000/e1000_main.c +71
-60@@ -86,54 +86,54 @@* {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 (0x1096 ), -INTEL_E1000_ETHERNET_DEVICE (0x1098 ), -INTEL_E1000_ETHERNET_DEVICE (0x1099 ), -INTEL_E1000_ETHERNET_DEVICE (0x109A ), -INTEL_E1000_ETHERNET_DEVICE (0x10B5 ), -INTEL_E1000_ETHERNET_DEVICE (0x10B9 ), +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 (0x1096 ,0 ), +INTEL_E1000_ETHERNET_DEVICE (0x1098 ,0 ), +INTEL_E1000_ETHERNET_DEVICE (0x1099 ,E1000_NO_IOPORT ), +INTEL_E1000_ETHERNET_DEVICE (0x109A ,E1000_NO_IOPORT ), +INTEL_E1000_ETHERNET_DEVICE (0x10B5 ,E1000_NO_IOPORT ), +INTEL_E1000_ETHERNET_DEVICE (0x10B9 ,0 ),/* required last entry */ {0 ,} };@@ -621,7 +621,14 @@int i, err, pci_using_dac;uint16_t eeprom_data;uint16_t eeprom_apme_mask =E1000_EEPROM_APME ; -if ((err =pci_enable_device (pdev))) +int bars; + +if (ent->driver_data &E1000_NO_IOPORT ) + bars =pci_select_bars (pdev,IORESOURCE_MEM ); +else + bars =pci_select_bars (pdev,IORESOURCE_MEM |IORESOURCE_IO ); + +if ((err =pci_enable_device_bars (pdev, bars)))return err;if (!(err =pci_set_dma_mask (pdev,DMA_64BIT_MASK ))) {@@ -634,7 +641,8 @@ pci_using_dac =0 ; } -if ((err =pci_request_regions (pdev, e1000_driver_name))) + err =pci_request_selected_regions (pdev, bars, e1000_driver_name); +if (err)return err;pci_set_master (pdev);@@ -654,6 +662,7 @@ adapter->pdev = pdev; adapter->hw.back = adapter; adapter->msg_enable = (1 << debug) -1 ; + adapter->bars = bars; mmio_start =pci_resource_start (pdev,BAR_0 ); mmio_len =pci_resource_len (pdev,BAR_0 );@@ -664,12 +673,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 ; + } } }@@ -880,7 +892,7 @@ err_ioremap:free_netdev (netdev); err_alloc_etherdev: -pci_release_regions (pdev); +pci_release_selected_regions (pdev, bars);return err; }@@ -935,7 +947,7 @@#endif iounmap (adapter->hw.hw_addr); -pci_release_regions (pdev); +pci_release_selected_regions (pdev, adapter->bars);free_netdev (netdev);@@ -4577,7 +4589,7 @@if (retval)DPRINTK (PROBE ,ERR ,"Error in setting power state\n" );e1000_pci_restore_state (adapter); - ret_val =pci_enable_device (pdev); + ret_val =pci_enable_device_bars (pdev, adapter->bars);pci_set_master (pdev); retval =pci_enable_wake (pdev, PCI_D3hot,0 ); -To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] Make Intel e1000 driver legacy I/O port free
Quoted 5 lines · Kenji Kaneshige
Quoted 4 lines · Auke Kok
Quoted 2 lines · Kenji Kaneshige
This patch makes Intel e1000 driver legacy I/O port free.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Signed-off-by Kenji Kaneshige <[email protected]>(adding netdev and the other e1000 maintainers to cc:)
without sending this to any of the listed e1000 maintainers???? *and* not even including netdev???
I'm sorry about that.
I also didn't see that you were sending this to Greg-KH. I think I got thrown off by that as I wasn't following lkml until yesterday in the first place. I'll toss the patches around over here and see what comes up.
Cheers,
Auke
Jeff Garzik
NAKRe: [PATCH 3/4] Make Intel e1000 driver legacy I/O port free
In reply to Kenji KaneshigeQuoted 90 lines · Kenji Kaneshige
static struct
pci_device_ide1000_pci_tbl[] = {drivers/net/e1000/e1000_main.c +41
-48-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 (0x1096 ), -INTEL_E1000_ETHERNET_DEVICE (0x1098 ), -INTEL_E1000_ETHERNET_DEVICE (0x1099 ), -INTEL_E1000_ETHERNET_DEVICE (0x109A ), -INTEL_E1000_ETHERNET_DEVICE (0x10B5 ), - INTEL_E1000_ETHERNET_DEVICE(0x10B9), +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),
drivers/net/e1000/e1000_main.c line 139
Quoted 10 lines
drivers/net/e1000/e1000_main.c +7
-0+INTEL_E1000_ETHERNET_DEVICE (0x108C ,E1000_NO_IOPORT ), +INTEL_E1000_ETHERNET_DEVICE (0x1096 ,0 ), +INTEL_E1000_ETHERNET_DEVICE (0x1098 ,0 ), +INTEL_E1000_ETHERNET_DEVICE (0x1099 ,E1000_NO_IOPORT ), +INTEL_E1000_ETHERNET_DEVICE (0x109A ,E1000_NO_IOPORT ), +INTEL_E1000_ETHERNET_DEVICE (0x10B5 ,E1000_NO_IOPORT ), +INTEL_E1000_ETHERNET_DEVICE (0x10B9 ,0 ),/* required last entry */ {0,} };
Why change all the entries? I would just change the ones with flags...
Quoted 4 lines
drivers/net/e1000/e1000_main.c +0
-0@@ -621,7 +621,14 @@int i, err, pci_using_dac;uint16_t eeprom_data;uint16_t eeprom_apme_mask =E1000_EEPROM_APME ;
drivers/net/e1000/e1000_main.c line 632
Quoted 10 lines
drivers/net/e1000/e1000_main.c +8
-1-if ((err =pci_enable_device (pdev))) +int bars; + +if (ent->driver_data &E1000_NO_IOPORT ) + bars =pci_select_bars (pdev,IORESOURCE_MEM ); +else + bars =pci_select_bars (pdev,IORESOURCE_MEM |IORESOURCE_IO ); + +if ((err =pci_enable_device_bars (pdev, bars)))return err;
NAK, this is an obvious regression.
pci_enable_device() also powers up the device, and enables irq delivery (on e.g. cardbus), and is allowed to do other platform-specific device bring-up tasks.
Jeff
Re: [PATCH 3/4] Make Intel e1000 driver legacy I/O port free
Hi Jeff,
Thank you for reviewing.
Quoted 2 lines · Jeff Garzik
Kenji Kaneshige wrote:
(snip.)
Quoted 11 lines
Quoted 7 lines
drivers/net/e1000/e1000_main.c +4
-0+INTEL_E1000_ETHERNET_DEVICE (0x1099 ,E1000_NO_IOPORT ), +INTEL_E1000_ETHERNET_DEVICE (0x109A ,E1000_NO_IOPORT ), +INTEL_E1000_ETHERNET_DEVICE (0x10B5 ,E1000_NO_IOPORT ), + INTEL_E1000_ETHERNET_DEVICE(0x10B9, 0),/* required last entry */ {0,} };Why change all the entries? I would just change the ones with flags...
I'm sorry. I don't understand what you mean. Could you tell me how should I change?
Quoted 18 lines
Quoted 4 lines
drivers/net/e1000/e1000_main.c +0
-0@@ -621,7 +621,14 @@int i, err, pci_using_dac;uint16_t eeprom_data;uint16_t eeprom_apme_mask =E1000_EEPROM_APME ;drivers/net/e1000/e1000_main.c line 632
Quoted 10 lines
drivers/net/e1000/e1000_main.c +8
-1-if ((err =pci_enable_device (pdev))) +int bars; + +if (ent->driver_data &E1000_NO_IOPORT ) + bars =pci_select_bars (pdev,IORESOURCE_MEM ); +else + bars =pci_select_bars (pdev,IORESOURCE_MEM |IORESOURCE_IO ); + +if ((err =pci_enable_device_bars (pdev, bars)))return err;NAK, this is an obvious regression.
Quoted 4 lines
pci_enable_device()also powers up the device, and enables irq delivery (on e.g. cardbus), and is allowed to do other platform-specific device bring-up tasks.
No, those tasks are done through pci_enable_device_bars() called from pci_enable_device() actually. In addition, I made small changes to pci_enable_device() and pci_enable_device_bars() in another patch ([PATCH 1/4]). Now pci_enable_device_bars() just call pci_enable_device_bars() like below:
intpci_enable_device(struct pci_dev *dev)
{
int err = pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1);
if (err)
return err;
return 0;
}Please see the following URL about this another patch.
http://www.uwsg.iu.edu/hypermail/linux/kernel/0606.0/1726.html
Thanks,
Kenji Kaneshige
Re: [PATCH 3/4] Make Intel e1000 driver legacy I/O port free
Quoted 5 lines · Kenji Kaneshige
No, those tasks are done through
pci_enable_device_bars()called frompci_enable_device()actually. In addition, I made small changes topci_enable_device()andpci_enable_device_bars()in another patch ([PATCH 1/4]). Nowpci_enable_device_bars()just callpci_enable_device_bars()like below:
Quoted 8 lines
int
pci_enable_device(struct pci_dev *dev){ int err = pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1); if (err) return err; return 0; }
You'll likely break IDE with such a change, which I also NAK. You can't just blindly do everything that pci_enable_device() does in IDE, which was the entire reason why pci_enable_device_bars() was added by Alan in the first place.
Jeff
Re: [PATCH 3/4] Make Intel e1000 driver legacy I/O port free
Quoted 17 lines · Jeff Garzik
Quoted 5 lines · Kenji Kaneshige
No, those tasks are done through
pci_enable_device_bars()called frompci_enable_device()actually. In addition, I made small changes topci_enable_device()andpci_enable_device_bars()in another patch ([PATCH 1/4]). Nowpci_enable_device_bars()just callpci_enable_device_bars()like below:Quoted 8 lines
int
pci_enable_device(struct pci_dev *dev){ int err = pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1); if (err) return err; return 0; }
Quoted 5 lines
You'll likely break IDE with such a change, which I also NAK. You can't just blindly do everything that
pci_enable_device()does in IDE, which was the entire reason whypci_enable_device_bars()was added by Alan in the first place.
Could you tell me what will be broken by my change? I only moved the following two lines from pci_enable_device() to pci_enable_device_bars().
pci_fixup_device(pci_fixup_enable, dev);
dev->is_enabled = 1;Thanks,
Kenji Kaneshige
PATCH 4/4 Make Emulex lpfc driver legacy I/O port free
In reply to Kenji KaneshigeThis patch makes Emulex lpfc driver legacy I/O port free.
--- drivers/scsi/lpfc/lpfc_init.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)
drivers/scsi/lpfc/lpfc_init.c +6 -4
@@ -1425,10 +1425,11 @@int error = -ENODEV , retval;int i;uint16_t iotag; +int bars =pci_select_bars (pdev,IORESOURCE_MEM ); -if (pci_enable_device (pdev)) +if (pci_enable_device_bars (pdev, bars)) goto out; -if (pci_request_regions (pdev,LPFC_DRIVER_NAME )) +if (pci_request_selected_regions (pdev, bars,LPFC_DRIVER_NAME )) goto out_disable_device; host =scsi_host_alloc (&lpfc_template,sizeof (struct lpfc_hba ));
@@ -1720,7 +1721,7 @@ phba->host =NULL ;scsi_host_put (host); out_release_regions: -pci_release_regions (pdev); +pci_release_selected_regions (pdev, bars); out_disable_device:pci_disable_device (pdev); out:
@@ -1734,6 +1735,7 @@struct Scsi_Host *host =pci_get_drvdata (pdev);struct lpfc_hba *phba = (struct lpfc_hba *)host->hostdata;unsigned long iflag; +int bars =pci_select_bars (pdev,IORESOURCE_MEM );lpfc_free_sysfs_attr (phba);
@@ -1777,7 +1779,7 @@iounmap (phba->ctrl_regs_memmap_p);iounmap (phba->slim_memmap_p); -pci_release_regions (phba->pcidev); +pci_release_selected_regions (phba->pcidev, bars);pci_disable_device (phba->pcidev);idr_remove (&lpfc_hba_index, phba->brd_no);
Re: [PATCH 4/4] Make Emulex lpfc driver legacy I/O port free
Quoted 1 line · Kenji Kaneshige
This patch makes Emulex lpfc driver legacy I/O port free.
Your interface for this is really horrible ;-)
drivers/scsi/lpfc/lpfc_init.c line 1433
Quoted 8 lines
drivers/scsi/lpfc/lpfc_init.c +3
-2+int bars =pci_select_bars (pdev,IORESOURCE_MEM ); -if (pci_enable_device (pdev)) +if (pci_enable_device_bars (pdev, bars)) goto out; -if (pci_request_regions (pdev,LPFC_DRIVER_NAME )) +if (pci_request_selected_regions (pdev, bars,LPFC_DRIVER_NAME )) goto out_disable_device;
Please make this something like:
if (pci_enable_device_noioport(pdev)) goto out; if (pci_request_regions(pdev, LPFC_DRIVER_NAME)) goto out_disable_device;
as in:
- get rid of this awkward pci_select_bars function, the pci_enable* function should do all the work and add a flag to struct pci_dev so that all other functions do the right thing.
Re: [PATCH 4/4] Make Emulex lpfc driver legacy I/O port free
Quoted 25 lines · Christoph Hellwig
Quoted 1 line · Kenji Kaneshige
This patch makes Emulex lpfc driver legacy I/O port free.
Your interface for this is really horrible ;-)
Quoted 8 lines
drivers/scsi/lpfc/lpfc_init.c +2
-1+int bars =pci_select_bars (pdev,IORESOURCE_MEM ); -if (pci_enable_device (pdev)) +if (pci_enable_device_bars (pdev, bars))goto out;drivers/scsi/lpfc/lpfc_init.c +1
-1-if (pci_request_regions (pdev,LPFC_DRIVER_NAME )) +if (pci_request_selected_regions (pdev, bars,LPFC_DRIVER_NAME ))goto out_disable_device;Please make this something like:
if (pci_enable_device_noioport(pdev)) goto out; if (pci_request_regions(pdev, LPFC_DRIVER_NAME)) goto out_disable_device;
Quoted 7 lines
as in:
- get rid of this awkward
pci_select_barsfunction, the pci_enable* function should do all the work and add a flag to structpci_devso that all other functions do the right thing.
No. Your idea is very similar to the idea of previous version of my patche which had a bug. The problem is that it doesn't work if pci_request_regions() is called before pci_enable_device*() (This is the correct order, though so many drivers breaks this rule).
Thanks,
Kenji Kaneshige
Re: [PATCH 4/4] Make Emulex lpfc driver legacy I/O port free
Quoted 5 lines · Kenji Kaneshige
No. Your idea is very similar to the idea of previous version of my patche which had a bug. The problem is that it doesn't work if
pci_request_regions()
is called before pci_enable_device*() (This is the correct order, though so many drivers breaks this rule).
Doesn't matter. Drivers not using pci_enable_device_noioport should be unaffect. Any any driver you convert should be fixed to do thing in the right order.
Re: [PATCH 4/4] Make Emulex lpfc driver legacy I/O port free
Quoted 8 lines · Christoph Hellwig
Quoted 4 lines · Kenji Kaneshige
No. Your idea is very similar to the idea of previous version of my patche which had a bug. The problem is that it doesn't work if
pci_request_regions()is called before pci_enable_device*() (This is the correct order, though so many drivers breaks this rule).
Quoted 5 lines
Doesn't matter. Drivers not using
pci_enable_device_noioportshould be unaffect. Any any driver you convert should be fixed to do thing in the right order.
I mean the right order is
(1) pci_request_regions()
(2) pci_enable_device*()
So there are no chance to set the flag for pci_request_regions() to know which regions should be requested.
Thanks,
Kenji Kaneshige
Re: [PATCH 4/4] Make Emulex lpfc driver legacy I/O port free
Quoted 4 lines · Kenji Kaneshige
I mean the right order is
(1) pci_request_regions() (2) pci_enable_device*()
no, pci_enable_device should be first.
Re: [PATCH 4/4] Make Emulex lpfc driver legacy I/O port free
Quoted 11 lines · Christoph Hellwig
Quoted 4 lines · Kenji Kaneshige
I mean the right order is
(1)
pci_request_regions()
(2) pci_enable_device*()no,
pci_enable_deviceshould be first.
I had the same wrong assumption before. But to prevent two devices colliding on the same address range, pci_request_regions() should be called first. Please see the following discussions:
http://www.uwsg.iu.edu/hypermail/linux/kernel/0606.0/0076.html
http://www.uwsg.iu.edu/hypermail/linux/kernel/0606.0/0187.html
http://www.uwsg.iu.edu/hypermail/linux/kernel/0606.0/0212.html
http://www.uwsg.iu.edu/hypermail/linux/kernel/0606.0/0369.html
http://www.uwsg.iu.edu/hypermail/linux/kernel/0606.0/0401.html
http://www.uwsg.iu.edu/hypermail/linux/kernel/0606.0/0431.html
http://www.uwsg.iu.edu/hypermail/linux/kernel/0606.0/0839.html
Thanks,
Kenji Kaneshige
Re: [PATCH 4/4] Make Emulex lpfc driver legacy I/O port free
Quoted 9 lines · Kenji Kaneshige
Quoted 8 lines · Christoph Hellwig
Quoted 4 lines · Kenji Kaneshige
I mean the right order is
(1)
pci_request_regions()
(2) pci_enable_device*()
Quoted 7 lines
Quoted 3 lines
no,
pci_enable_deviceshould be first.I had the same wrong assumption before. But to prevent two devices colliding on the same address range,
pci_request_regions()should be called first. Please see the following discussions:
No. That's what the pci_driver matching is for. Without pci_enable_device pci_request_regions could do the wrong thing when fixups aren't run.
Re: [PATCH 4/4] Make Emulex lpfc driver legacy I/O port free
Quoted 14 lines · Christoph Hellwig
Quoted 13 lines · Kenji Kaneshige
Quoted 11 lines · Christoph Hellwig
Quoted 4 lines · Kenji Kaneshige
I mean the right order is
(1)
pci_request_regions()
(2) pci_enable_device*()no,
pci_enable_deviceshould be first.
Quoted 6 lines
Quoted 3 lines
I had the same wrong assumption before. But to prevent two devices colliding on the same address range,
pci_request_regions()should be called first. Please see the following discussions:No. That's what the
pci_drivermatching is for. Withoutpci_enable_devicepci_request_regionscould do the wrong thing when fixups aren't run.
I don't see how driver matching will help here. It seems wrong to enable a device first, and then check if the resources it is decoding actually conflict with some other device's resources. Regarding quirks, all the ones that mess around with resources are marked as HEADER quirks. So they should be called right after a device is probed and before its driver can call pci_request_regions(). What problems do you see if regions are requested before the device is enabled?
Rajesh
Quoted 12 lines · Rajesh Shah
Documentation/pci.txt have been remapped by the kernel. · line 222
Quoted 7 lines · Grant Grundler
Documentation/pci.txt +7
-0+ The device driver needs to call pci_request_region() to make sure +no other device is already using the same resource. The driver is expected +to determine MMIO and IO Port resource availability _before_ calling +pci_enable_device(). Conversely, drivers should call pci_release_region() +_after_ calling pci_disable_device(). The idea is to prevent two devices +colliding on the same address range. +A quick look in the drivers directory shows that _lots_ of drivers violate this rule. In fact, I suspect Kaneshige-san made the original incorrect assumption since there were so many drivers out there which do it in the wrong order.
That's ok. It's not a big deal normally.
It's just when BIOS is broken and assigns overlapping ranges to multiple devices or mis-routes MMIO address space that we need this to work right. The vast majority of BIOS's do get it right and that's why I'm not terribly worried.
If there is consensus the drivers are wrong, then it's pretty easy to fix and we don't have to panic.
Do you agree with the change in the text?
thanks,
grant
Quoted 8 lines · Grant Grundler
this to work right. The vast majority of BIOS's do get it right and that's why I'm not terribly worried.
If there is consensus the drivers are wrong, then it's pretty easy to fix and we don't have to panic.
Do you agree with the change in the text?
Yeah, I agree that's the right order.
Rajesh