[BUG](-mm)pci_disable_device function clear bars_enabled element
bibo,mao
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Rajesh Shah
> 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.
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
Grant Grundler
> 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.
Patch below clarifies comments in Documentation/pci.txt.
Greg, can you apply?
(feel free to edit it a bit more)
thanks,
grant
Signed-off-by: Grant Grundler <grun...@parisc-linux.org>
--- a/Documentation/pci.txt
+++ b/Documentation/pci.txt
@@ -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).
Rajesh Shah
> + 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.
> +
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
Kenji Kaneshige
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
Grant Grundler
> On Thu, Jun 01, 2006 at 11:15:59AM -0600, Grant Grundler wrote:
> > + 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
Grant Grundler
...
> 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?
> 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
Kenji Kaneshige
> On Fri, Jun 02, 2006 at 11:57:36AM +0900, Kenji Kaneshige wrote:
> ...
>
>>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.
>>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?
>
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
Rajesh Shah
> 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?
>
Rajesh
Grant Grundler
> 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?
> 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().
I agree. I'll fix that.
Greg KH
> On Fri, Jun 02, 2006 at 04:31:15PM +0900, Kenji Kaneshige wrote:
> > 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
Kenji Kaneshige
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().
Signed-off-by: Kenji Kaneshige <kaneshi...@jp.fujitsu.com>
---
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(-)
Index: linux-2.6.17-rc5-mm3/Documentation/pci.txt
===================================================================
--- linux-2.6.17-rc5-mm3.orig/Documentation/pci.txt 2006-06-05 11:26:27.000000000 +0900
+++ linux-2.6.17-rc5-mm3/Documentation/pci.txt 2006-06-05 18:31:41.000000000 +0900
@@ -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.
Index: linux-2.6.17-rc5-mm3/drivers/net/e1000/e1000_main.c
===================================================================
--- linux-2.6.17-rc5-mm3.orig/drivers/net/e1000/e1000_main.c 2006-06-05 11:26:28.000000000 +0900
+++ linux-2.6.17-rc5-mm3/drivers/net/e1000/e1000_main.c 2006-06-05 18:31:47.000000000 +0900
@@ -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);
Index: linux-2.6.17-rc5-mm3/drivers/pci/pci.c
===================================================================
--- linux-2.6.17-rc5-mm3.orig/drivers/pci/pci.c 2006-06-05 11:26:28.000000000 +0900
+++ linux-2.6.17-rc5-mm3/drivers/pci/pci.c 2006-06-05 18:31:26.000000000 +0900
@@ -648,12 +648,6 @@
{
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 + 1);
- return;
- }
if (pci_resource_flags(pdev, bar) & IORESOURCE_IO)
release_region(pci_resource_start(pdev, bar),
pci_resource_len(pdev, bar));
@@ -680,12 +674,7 @@
{
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 + 1);
- 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))
@@ -710,6 +699,47 @@
return -EBUSY;
}
+/**
+ * pci_release_selected_regions - Release selected PCI I/O and memory resources
+ * @pdev: PCI device whose resources were previously reserved
+ * @bars: Bitmask of BARs to be released
+ *
+ * Release selected PCI I/O and memory resources previously reserved.
+ * Call this function only after all use of the PCI regions has ceased.
+ */
+void pci_release_selected_regions(struct pci_dev *pdev, int bars)
+{
+ int i;
+
+ for (i = 0; i < 6; i++)
+ if (bars & (1 << i))
+ pci_release_region(pdev, i);
+}
+
+/**
+ * pci_request_selected_regions - Reserve selected PCI I/O and memory resources
+ * @pdev: PCI device whose resources are to be reserved
+ * @bars: Bitmask of BARs to be requested
+ * @res_name: Name to be associated with resource
+ */
+int pci_request_selected_regions(struct pci_dev *pdev, int bars,
+ const char *res_name)
+{
+ int i;
+
+ for (i = 0; i < 6; i++)
+ if (bars & (1 << i))
+ if(pci_request_region(pdev, i, res_name))
+ goto err_out;
+ return 0;
+
+err_out:
+ while(--i >= 0)
+ if (bars & (1 << i))
+ pci_release_region(pdev, i);
+
+ return -EBUSY;
+}
/**
* pci_release_regions - Release reserved PCI I/O and memory resources
@@ -722,11 +752,7 @@
void pci_release_regions(struct pci_dev *pdev)
{
- int i;
-
- for (i = 0; i < 6; i++)
- if (pdev->bars_enabled & (1 << i))
- pci_release_region(pdev, i);
+ pci_release_selected_regions(pdev, (1 << 6) - 1);
}
/**
@@ -744,20 +770,7 @@
*/
int pci_request_regions(struct pci_dev *pdev, const char *res_name)
{
- int i;
-
- for (i = 0; i < 6; i++)
- if (pdev->bars_enabled & (1 << i))
- if(pci_request_region(pdev, i, res_name))
- goto err_out;
- return 0;
-
-err_out:
- while(--i >= 0)
- if (pdev->bars_enabled & (1 << i))
- pci_release_region(pdev, i);
-
- return -EBUSY;
+ return pci_request_selected_regions(pdev, ((1 << 6) - 1), res_name);
}
/**
@@ -995,6 +1008,8 @@
EXPORT_SYMBOL(pci_request_regions);
EXPORT_SYMBOL(pci_release_region);
EXPORT_SYMBOL(pci_request_region);
+EXPORT_SYMBOL(pci_release_selected_regions);
+EXPORT_SYMBOL(pci_request_selected_regions);
EXPORT_SYMBOL(pci_set_master);
EXPORT_SYMBOL(pci_set_mwi);
EXPORT_SYMBOL(pci_clear_mwi);
Index: linux-2.6.17-rc5-mm3/drivers/scsi/lpfc/lpfc_init.c
===================================================================
--- linux-2.6.17-rc5-mm3.orig/drivers/scsi/lpfc/lpfc_init.c 2006-06-05 11:26:29.000000000 +0900
+++ linux-2.6.17-rc5-mm3/drivers/scsi/lpfc/lpfc_init.c 2006-06-05 18:31:50.000000000 +0900
@@ -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);
Index: linux-2.6.17-rc5-mm3/include/linux/pci.h
===================================================================
--- linux-2.6.17-rc5-mm3.orig/include/linux/pci.h 2006-06-05 11:26:31.000000000 +0900
+++ linux-2.6.17-rc5-mm3/include/linux/pci.h 2006-06-05 18:31:26.000000000 +0900
@@ -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,
Greg KH
> Hi 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.
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
Kenji Kaneshige
> On Mon, Jun 05, 2006 at 09:40:28PM +0900, Kenji Kaneshige wrote:
>
>>Hi 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.
>
>
> 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
Kenji Kaneshige
> On Mon, Jun 05, 2006 at 09:40:28PM +0900, Kenji Kaneshige wrote:
>
>>Hi 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.
>
>
> 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
Kenji Kaneshige
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().
Signed-off-by: Kenji Kaneshige <kaneshi...@jp.fujitsu.com>
---
drivers/pci/pci-driver.c | 3 +
drivers/pci/pci.c | 86 ++++++++++++++++++++++++++++++++++++-----------
include/linux/pci.h | 4 ++
3 files changed, 73 insertions(+), 20 deletions(-)
Index: linux-2.6.17-rc6/drivers/pci/pci-driver.c
===================================================================
--- linux-2.6.17-rc6.orig/drivers/pci/pci-driver.c 2006-06-06 21:39:11.000000000 +0900
+++ linux-2.6.17-rc6/drivers/pci/pci-driver.c 2006-06-06 21:40:26.000000000 +0900
@@ -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);
Index: linux-2.6.17-rc6/drivers/pci/pci.c
===================================================================
--- linux-2.6.17-rc6.orig/drivers/pci/pci.c 2006-06-06 21:39:11.000000000 +0900
+++ linux-2.6.17-rc6/drivers/pci/pci.c 2006-06-06 21:55:46.000000000 +0900
@@ -490,6 +490,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;
}
@@ -507,8 +510,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;
}
@@ -543,6 +544,7 @@
pcibios_disable_device(dev);
dev->is_enabled = 0;
+ dev->bars_enabled = 0;
}
/**
@@ -651,7 +653,7 @@
{
if (pci_resource_len(pdev, bar) == 0)
return 0;
-
+
if (pci_resource_flags(pdev, bar) & IORESOURCE_IO) {
if (!request_region(pci_resource_start(pdev, bar),
pci_resource_len(pdev, bar), res_name))
@@ -674,6 +676,47 @@
@@ -686,10 +729,7 @@
void pci_release_regions(struct pci_dev *pdev)
{
- int i;
-
- for (i = 0; i < 6; i++)
- pci_release_region(pdev, i);
+ pci_release_selected_regions(pdev, (1 << 6) - 1);
}
/**
@@ -707,18 +747,7 @@
*/
int pci_request_regions(struct pci_dev *pdev, const char *res_name)
{
- int i;
-
- for (i = 0; i < 6; i++)
- if(pci_request_region(pdev, i, res_name))
- goto err_out;
- return 0;
-
-err_out:
- while(--i >= 0)
- pci_release_region(pdev, i);
-
- return -EBUSY;
+ return pci_request_selected_regions(pdev, ((1 << 6) - 1), res_name);
}
/**
@@ -891,6 +920,22 @@
}
#endif
+/**
+ * pci_select_bars - Make BAR mask from the type of resource
+ * @pdev: the PCI device for which BAR mask is made
+ * @flags: resource type mask to be selected
+ *
+ * This helper routine makes bar mask from the type of resource.
+ */
+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;
+}
+
static int __devinit pci_init(void)
{
struct pci_dev *dev = NULL;
@@ -940,6 +985,8 @@
EXPORT_SYMBOL(pci_request_regions);
EXPORT_SYMBOL(pci_release_region);
EXPORT_SYMBOL(pci_request_region);
+EXPORT_SYMBOL(pci_release_selected_regions);
+EXPORT_SYMBOL(pci_request_selected_regions);
EXPORT_SYMBOL(pci_set_master);
EXPORT_SYMBOL(pci_set_mwi);
EXPORT_SYMBOL(pci_clear_mwi);
@@ -948,6 +995,7 @@
EXPORT_SYMBOL(pci_set_consistent_dma_mask);
EXPORT_SYMBOL(pci_assign_resource);
EXPORT_SYMBOL(pci_find_parent_resource);
+EXPORT_SYMBOL(pci_select_bars);
EXPORT_SYMBOL(pci_set_power_state);
EXPORT_SYMBOL(pci_save_state);
Index: linux-2.6.17-rc6/include/linux/pci.h
===================================================================
--- linux-2.6.17-rc6.orig/include/linux/pci.h 2006-06-06 21:39:11.000000000 +0900
+++ linux-2.6.17-rc6/include/linux/pci.h 2006-06-06 21:40:26.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)
@@ -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,
-
Kenji Kaneshige
Documentation/pci.txt.
Signed-off-by: Kenji Kaneshige <kaneshi...@jp.fujitsu.com>
Signed-off-by: Grant Grundler <grun...@parisc-linux.org>
---
Documentation/pci.txt | 70 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 70 insertions(+)
Index: linux-2.6.17-rc6/Documentation/pci.txt
===================================================================
--- linux-2.6.17-rc6.orig/Documentation/pci.txt 2006-06-06 21:39:11.000000000 +0900
+++ linux-2.6.17-rc6/Documentation/pci.txt 2006-06-06 21:56:32.000000000 +0900
@@ -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").
Kenji Kaneshige
Signed-off-by: Kenji Kaneshige <kaneshi...@jp.fujitsu.com>
---
drivers/scsi/lpfc/lpfc_init.c | 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
Index: linux-2.6.17-rc6/drivers/scsi/lpfc/lpfc_init.c
===================================================================
--- linux-2.6.17-rc6.orig/drivers/scsi/lpfc/lpfc_init.c 2006-06-06 21:39:11.000000000 +0900
+++ linux-2.6.17-rc6/drivers/scsi/lpfc/lpfc_init.c 2006-06-06 21:56:56.000000000 +0900
@@ -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);
-
Kenji Kaneshige
Signed-off-by: Kenji Kaneshige <kaneshi...@jp.fujitsu.com>
---
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.h 2006-06-06 21:39:11.000000000 +0900
+++ linux-2.6.17-rc6/drivers/net/e1000/e1000.h 2006-06-06 21:56:41.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;
@@ -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.c 2006-06-06 21:39:11.000000000 +0900
+++ linux-2.6.17-rc6/drivers/net/e1000/e1000_main.c 2006-06-06 21:56:41.000000000 +0900
@@ -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);
Auke Kok
> This patch makes Intel e1000 driver legacy I/O port free.
>
> Signed-off-by: Kenji Kaneshige <kaneshi...@jp.fujitsu.com>
(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
Kenji Kaneshige
> Kenji Kaneshige wrote:
>
>> This patch makes Intel e1000 driver legacy I/O port free.
>>
>> Signed-off-by: Kenji Kaneshige <kaneshi...@jp.fujitsu.com>
>
>
> (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'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
Christoph Hellwig
> This patch makes Emulex lpfc driver legacy I/O port free.
Your interface for this is really horrible ;-)
> + 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.
Kenji Kaneshige
> On Wed, Jun 07, 2006 at 12:15:34PM +0900, Kenji Kaneshige wrote:
>
>>This patch makes Emulex lpfc driver legacy I/O port free.
>
>
> Your interface for this is really horrible ;-)
>
>
>>+ 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.
>
>
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
Christoph Hellwig
> 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.
Kenji Kaneshige
> On Wed, Jun 07, 2006 at 09:23:19PM +0900, Kenji Kaneshige wrote:
>
>>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.
>
>
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
Christoph Hellwig
> I mean the right order is
>
> (1) pci_request_regions()
> (2) pci_enable_device*()
no, pci_enable_device should be first.
Kenji Kaneshige
> On Wed, Jun 07, 2006 at 10:11:12PM +0900, Kenji Kaneshige wrote:
>
>>I mean the right order is
>>
>> (1) pci_request_regions()
>> (2) pci_enable_device*()
>
>
> no, pci_enable_device should 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
Auke Kok
> Auke Kok wrote:
>> Kenji Kaneshige wrote:
>>
>>> This patch makes Intel e1000 driver legacy I/O port free.
>>>
>>> Signed-off-by: Kenji Kaneshige <kaneshi...@jp.fujitsu.com>
>>
>> (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
Christoph Hellwig
> Christoph Hellwig wrote:
> >On Wed, Jun 07, 2006 at 10:11:12PM +0900, Kenji Kaneshige wrote:
> >
> >>I mean the right order is
> >>
> >> (1) pci_request_regions()
> >> (2) pci_enable_device*()
> >
> >
> >no, pci_enable_device should 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.
Rajesh Shah
> On Wed, Jun 07, 2006 at 10:56:07PM +0900, Kenji Kaneshige wrote:
> > Christoph Hellwig wrote:
> > >On Wed, Jun 07, 2006 at 10:11:12PM +0900, Kenji Kaneshige wrote:
> > >
> > >>I mean the right order is
> > >>
> > >> (1) pci_request_regions()
> > >> (2) pci_enable_device*()
> > >
> > >
> > >no, pci_enable_device should 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.
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
Jeff Garzik
Why change all the entries? I would just change the ones with flags...
> @@ -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;
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
Kenji Kaneshige
Thank you for reviewing.
Jeff Garzik wrote:
> Kenji Kaneshige wrote:
>
(snip.)
>>+ 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?
>>@@ -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;
>
>
> 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.
>
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:
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;
}
Please see the following URL about this another patch.
http://www.uwsg.iu.edu/hypermail/linux/kernel/0606.0/1726.html
Thanks,
Kenji Kaneshige
Jeff Garzik
> 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:
>
> 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
Kenji Kaneshige
> Kenji Kaneshige wrote:
>
>>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:
>>
>>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.
>
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