The pci_enable_device_bars() interface isn't well suited to PCI
because you can't actually enable/disable BARs individually on
a device. So for example, if a device has 2 memory BARs 0 and 1,
and one of them (let's say 1) has not been successfully allocated
by the firmware or the kernel, then enabling memory decoding
shouldn't be permitted for the entire device since it will decode
whatever random address is still in that BAR 1.
So a device must be either fully enabled for IO, for Memory, or
for both. Not on a per-BAR basis.
This provides two new functions, pci_enable_device_io() and
pci_enable_device_mem() to replace pci_enable_device_bars(). The
implementation internally builds a BAR mask in order to be able
to use existing arch infrastructure.
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
---
drivers/pci/pci.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/pci.h | 2 +
2 files changed, 56 insertions(+)
--- linux-work.orig/drivers/pci/pci.c 2007-12-18 09:37:56.000000000 +1100
+++ linux-work/drivers/pci/pci.c 2007-12-18 09:38:25.000000000 +1100
@@ -737,6 +737,58 @@ pci_enable_device_bars(struct pci_dev *d
}
/**
+ * pci_enable_device_io - Initialize a device for use with IO space
+ * @dev: PCI device to be initialized
+ *
+ * Initialize device before it's used by a driver. Ask low-level code
+ * to enable I/O resources. Wake up the device if it was suspended.
+ * Beware, this function can fail.
+ */
+int pci_enable_device_io(struct pci_dev *dev)
+{
+ int err;
+ int i, bars = 0;
+
+ if (atomic_add_return(1, &dev->enable_cnt) > 1)
+ return 0; /* already enabled */
+
+ for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
+ if (dev->resource[i].flags & IORESOURCE_IO)
+ bars |= (1 << i);
+
+ err = do_pci_enable_device(dev, bars);
+ if (err < 0)
+ atomic_dec(&dev->enable_cnt);
+ return err;
+}
+
+/**
+ * pci_enable_device_mem - Initialize a device for use with Memory space
+ * @dev: PCI device to be initialized
+ *
+ * Initialize device before it's used by a driver. Ask low-level code
+ * to enable Memory resources. Wake up the device if it was suspended.
+ * Beware, this function can fail.
+ */
+int pci_enable_device_mem(struct pci_dev *dev)
+{
+ int err;
+ int i, bars = 0;
+
+ if (atomic_add_return(1, &dev->enable_cnt) > 1)
+ return 0; /* already enabled */
+
+ for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
+ if (dev->resource[i].flags & IORESOURCE_MEM)
+ bars |= (1 << i);
+
+ err = do_pci_enable_device(dev, bars);
+ if (err < 0)
+ atomic_dec(&dev->enable_cnt);
+ return err;
+}
+
+/**
* pci_enable_device - Initialize device before it's used by a driver.
* @dev: PCI device to be initialized
*
@@ -1621,6 +1673,8 @@ device_initcall(pci_init);
EXPORT_SYMBOL_GPL(pci_restore_bars);
EXPORT_SYMBOL(pci_reenable_device);
EXPORT_SYMBOL(pci_enable_device_bars);
+EXPORT_SYMBOL(pci_enable_device_io);
+EXPORT_SYMBOL(pci_enable_device_mem);
EXPORT_SYMBOL(pci_enable_device);
EXPORT_SYMBOL(pcim_enable_device);
EXPORT_SYMBOL(pcim_pin_device);
Index: linux-work/include/linux/pci.h
===================================================================
--- linux-work.orig/include/linux/pci.h 2007-12-18 09:38:36.000000000 +1100
+++ linux-work/include/linux/pci.h 2007-12-18 09:38:53.000000000 +1100
@@ -548,6 +548,8 @@ static inline int pci_write_config_dword
int __must_check pci_enable_device(struct pci_dev *dev);
int __must_check pci_enable_device_bars(struct pci_dev *dev, int mask);
+int __must_check pci_enable_device_io(struct pci_dev *dev);
+int __must_check pci_enable_device_mem(struct pci_dev *dev);
int __must_check pci_reenable_device(struct pci_dev *);
int __must_check pcim_enable_device(struct pci_dev *pdev);
void pcim_pin_device(struct pci_dev *pdev);
This patch converts users of pci_enable_device_bars() to the new
pci_enable_device_{io,mem} interface.
The new API fits nicely, except maybe for the QLA case where a bit of
code re-organization might be a good idea but I prefer sticking to the
simple patch as I don't have hardware to test on.
I'll also need some feedback on the cs5520 change.
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
---
drivers/ata/pata_cs5520.c | 2 +-
drivers/i2c/busses/scx200_acb.c | 2 +-
drivers/ide/pci/cs5520.c | 10 ++++++++--
drivers/ide/setup-pci.c | 6 ++++--
drivers/scsi/lpfc/lpfc_init.c | 3 +--
drivers/scsi/qla2xxx/qla_os.c | 12 +++++++++---
6 files changed, 24 insertions(+), 11 deletions(-)
--- linux-work.orig/drivers/ata/pata_cs5520.c 2007-12-18 09:37:55.000000000 +1100
+++ linux-work/drivers/ata/pata_cs5520.c 2007-12-18 09:39:03.000000000 +1100
@@ -229,7 +229,7 @@ static int __devinit cs5520_init_one(str
return -ENOMEM;
/* Perform set up for DMA */
- if (pci_enable_device_bars(pdev, 1<<2)) {
+ if (pci_enable_device_io(pdev)) {
printk(KERN_ERR DRV_NAME ": unable to configure BAR2.\n");
return -ENODEV;
}
Index: linux-work/drivers/i2c/busses/scx200_acb.c
===================================================================
--- linux-work.orig/drivers/i2c/busses/scx200_acb.c 2007-12-18 09:37:55.000000000 +1100
+++ linux-work/drivers/i2c/busses/scx200_acb.c 2007-12-18 09:39:03.000000000 +1100
@@ -492,7 +492,7 @@ static __init int scx200_create_pci(cons
iface->pdev = pdev;
iface->bar = bar;
- rc = pci_enable_device_bars(iface->pdev, 1 << iface->bar);
+ rc = pci_enable_device_io(iface->pdev);
if (rc)
goto errout_free;
Index: linux-work/drivers/ide/pci/cs5520.c
===================================================================
--- linux-work.orig/drivers/ide/pci/cs5520.c 2007-12-18 09:37:55.000000000 +1100
+++ linux-work/drivers/ide/pci/cs5520.c 2007-12-18 09:39:03.000000000 +1100
@@ -160,8 +160,14 @@ static int __devinit cs5520_init_one(str
ide_setup_pci_noise(dev, d);
/* We must not grab the entire device, it has 'ISA' space in its
- BARS too and we will freak out other bits of the kernel */
- if (pci_enable_device_bars(dev, 1<<2)) {
+ * BARS too and we will freak out other bits of the kernel
+ *
+ * pci_enable_device_bars() is going away. I replaced it with
+ * IO only enable for now but I'll need confirmation this is
+ * allright for that device. If not, it will need some kind of
+ * quirk. --BenH.
+ */
+ if (pci_enable_device_io(dev)) {
printk(KERN_WARNING "%s: Unable to enable 55x0.\n", d->name);
return -ENODEV;
}
Index: linux-work/drivers/ide/setup-pci.c
===================================================================
--- linux-work.orig/drivers/ide/setup-pci.c 2007-12-18 09:37:55.000000000 +1100
+++ linux-work/drivers/ide/setup-pci.c 2007-12-18 09:40:07.000000000 +1100
@@ -236,7 +236,9 @@ EXPORT_SYMBOL_GPL(ide_setup_pci_noise);
* @d: IDE port info
*
* Enable the IDE PCI device. We attempt to enable the device in full
- * but if that fails then we only need BAR4 so we will enable that.
+ * but if that fails then we only need IO space. The PCI code should
+ * have setup the proper resources for us already for controllers in
+ * legacy mode.
*
* Returns zero on success or an error code
*/
@@ -246,7 +248,7 @@ static int ide_pci_enable(struct pci_dev
int ret;
if (pci_enable_device(dev)) {
- ret = pci_enable_device_bars(dev, 1 << 4);
+ ret = pci_enable_device_io(dev);
if (ret < 0) {
printk(KERN_WARNING "%s: (ide_setup_pci_device:) "
"Could not enable device.\n", d->name);
Index: linux-work/drivers/scsi/lpfc/lpfc_init.c
===================================================================
--- linux-work.orig/drivers/scsi/lpfc/lpfc_init.c 2007-12-18 09:37:55.000000000 +1100
+++ linux-work/drivers/scsi/lpfc/lpfc_init.c 2007-12-18 09:39:03.000000000 +1100
@@ -2100,10 +2100,9 @@ static pci_ers_result_t lpfc_io_slot_res
struct Scsi_Host *shost = pci_get_drvdata(pdev);
struct lpfc_hba *phba = ((struct lpfc_vport *)shost->hostdata)->phba;
struct lpfc_sli *psli = &phba->sli;
- int bars = pci_select_bars(pdev, IORESOURCE_MEM);
dev_printk(KERN_INFO, &pdev->dev, "recovering from a slot reset.\n");
- if (pci_enable_device_bars(pdev, bars)) {
+ if (pci_enable_device_mem(pdev)) {
printk(KERN_ERR "lpfc: Cannot re-enable "
"PCI device after reset.\n");
return PCI_ERS_RESULT_DISCONNECT;
Index: linux-work/drivers/scsi/qla2xxx/qla_os.c
===================================================================
--- linux-work.orig/drivers/scsi/qla2xxx/qla_os.c 2007-12-18 09:37:55.000000000 +1100
+++ linux-work/drivers/scsi/qla2xxx/qla_os.c 2007-12-18 09:39:03.000000000 +1100
@@ -1583,7 +1583,7 @@ qla2x00_probe_one(struct pci_dev *pdev,
char pci_info[30];
char fw_str[30];
struct scsi_host_template *sht;
- int bars;
+ int bars, mem_only = 0;
bars = pci_select_bars(pdev, IORESOURCE_MEM | IORESOURCE_IO);
sht = &qla2x00_driver_template;
@@ -1594,10 +1594,16 @@ qla2x00_probe_one(struct pci_dev *pdev,
pdev->device == PCI_DEVICE_ID_QLOGIC_ISP2532) {
bars = pci_select_bars(pdev, IORESOURCE_MEM);
sht = &qla24xx_driver_template;
+ mem_only = 1;
}
- if (pci_enable_device_bars(pdev, bars))
- goto probe_out;
+ if (mem_only) {
+ if (pci_enable_device_mem(pdev))
+ goto probe_out;
+ } else {
+ if (pci_enable_device(pdev))
+ goto probe_out;
+ }
if (pci_find_aer_capability(pdev))
if (pci_enable_pcie_error_reporting(pdev))
This patch changes the PowerPC PCI code to disable IO and/or Memory
decoding on a PCI device when a resource of that type failed to be
allocated. This is done to avoid having unallocated dangling BARs enabled
that might try to decode on top of other devices.
If a proper resource is assigned later on, then pci_enable_device{,_io,_mem}
will take care of re-enabling decoding.
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
---
NOTE: This patch doesn't apply on current upstream, but rather on top
of a serie that merges 32 and 64 bits PowerPC PCI resource handling, and
which will be in 2.6.25. I post it here mostly to show what I think should
be done. A similar patch will have to be done for other architectures.
arch/powerpc/kernel/pci-common.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
--- linux-work.orig/arch/powerpc/kernel/pci-common.c 2007-12-18 09:37:52.000000000 +1100
+++ linux-work/arch/powerpc/kernel/pci-common.c 2007-12-18 09:39:27.000000000 +1100
@@ -1016,7 +1016,7 @@ static void __init pcibios_allocate_bus_
}
}
-static inline void __devinit alloc_resource(struct pci_dev *dev, int idx)
+static inline int __devinit alloc_resource(struct pci_dev *dev, int idx)
{
struct resource *pr, *r = &dev->resource[idx];
@@ -1040,7 +1040,10 @@ static inline void __devinit alloc_resou
r->flags |= IORESOURCE_UNSET;
r->end -= r->start;
r->start = 0;
+
+ return -EBUSY;
}
+ return 0;
}
static void __init pcibios_allocate_resources(int pass)
@@ -1062,8 +1065,12 @@ static void __init pcibios_allocate_reso
disabled = !(command & PCI_COMMAND_IO);
else
disabled = !(command & PCI_COMMAND_MEMORY);
- if (pass == disabled)
- alloc_resource(dev, idx);
+ if (pass == disabled && alloc_resource(dev, idx)) {
+ command &= ~(r->flags & (IORESOURCE_IO |
+ IORESOURCE_MEM));
+ pci_write_config_word(dev,
+ PCI_COMMAND, command);
+ }
}
if (pass)
continue;
Now that all in-tree users are gone, this removes pci_enable_device_bars()
completely.
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
---
drivers/pci/pci.c | 32 +++++++-------------------------
include/linux/pci.h | 1 -
2 files changed, 7 insertions(+), 26 deletions(-)
--- linux-work.orig/drivers/pci/pci.c 2007-12-18 09:38:25.000000000 +1100
+++ linux-work/drivers/pci/pci.c 2007-12-18 09:39:04.000000000 +1100
@@ -714,29 +714,6 @@ int pci_reenable_device(struct pci_dev *
}
/**
- * pci_enable_device_bars - Initialize some of a device for use
- * @dev: PCI device to be initialized
- * @bars: bitmask of BAR's that must be configured
- *
- * Initialize device before it's used by a driver. Ask low-level code
- * to enable selected I/O and memory resources. Wake up the device if it
- * was suspended. Beware, this function can fail.
- */
-int
-pci_enable_device_bars(struct pci_dev *dev, int bars)
-{
- int err;
-
- if (atomic_add_return(1, &dev->enable_cnt) > 1)
- return 0; /* already enabled */
-
- err = do_pci_enable_device(dev, bars);
- if (err < 0)
- atomic_dec(&dev->enable_cnt);
- return err;
-}
-
-/**
* pci_enable_device_io - Initialize a device for use with IO space
* @dev: PCI device to be initialized
*
@@ -801,7 +778,13 @@ int pci_enable_device_mem(struct pci_dev
*/
int pci_enable_device(struct pci_dev *dev)
{
- return pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1);
+ if (atomic_add_return(1, &dev->enable_cnt) > 1)
+ return 0; /* already enabled */
+
+ err = do_pci_enable_device(dev, (1 << PCI_NUM_RESOURCES) - 1);
+ if (err < 0)
+ atomic_dec(&dev->enable_cnt);
+ return err;
}
/*
@@ -1672,7 +1655,6 @@ device_initcall(pci_init);
EXPORT_SYMBOL_GPL(pci_restore_bars);
EXPORT_SYMBOL(pci_reenable_device);
-EXPORT_SYMBOL(pci_enable_device_bars);
EXPORT_SYMBOL(pci_enable_device_io);
EXPORT_SYMBOL(pci_enable_device_mem);
EXPORT_SYMBOL(pci_enable_device);
Index: linux-work/include/linux/pci.h
===================================================================
--- linux-work.orig/include/linux/pci.h 2007-12-18 09:39:20.000000000 +1100
+++ linux-work/include/linux/pci.h 2007-12-18 09:39:22.000000000 +1100
@@ -547,7 +547,6 @@ static inline int pci_write_config_dword
}
int __must_check pci_enable_device(struct pci_dev *dev);
-int __must_check pci_enable_device_bars(struct pci_dev *dev, int mask);
int __must_check pci_enable_device_io(struct pci_dev *dev);
int __must_check pci_enable_device_mem(struct pci_dev *dev);
int __must_check pci_reenable_device(struct pci_dev *);
Hi Benjamin,
Benjamin Herrenschmidt <[email protected]> writes:
> --- linux-work.orig/drivers/pci/pci.c 2007-12-18 09:37:56.000000000 +1100
> +++ linux-work/drivers/pci/pci.c 2007-12-18 09:38:25.000000000 +1100
> [...]
> +int pci_enable_device_io(struct pci_dev *dev)
> +{
> + int err;
> + int i, bars = 0;
> +
> + if (atomic_add_return(1, &dev->enable_cnt) > 1)
> + return 0; /* already enabled */
> +
> + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
> + if (dev->resource[i].flags & IORESOURCE_IO)
> + bars |= (1 << i);
> +
> + err = do_pci_enable_device(dev, bars);
> + if (err < 0)
> + atomic_dec(&dev->enable_cnt);
> + return err;
> +}
> [...]
> +int pci_enable_device_mem(struct pci_dev *dev)
> +{
> + int err;
> + int i, bars = 0;
> +
> + if (atomic_add_return(1, &dev->enable_cnt) > 1)
> + return 0; /* already enabled */
> +
> + for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
> + if (dev->resource[i].flags & IORESOURCE_MEM)
> + bars |= (1 << i);
> +
> + err = do_pci_enable_device(dev, bars);
> + if (err < 0)
> + atomic_dec(&dev->enable_cnt);
> + return err;
> +}
These two functions should be refactored, the only difference is the
flag checking.
Hannes
On Tue, 2007-12-18 at 00:49 +0100, Johannes Weiner wrote:
>
> These two functions should be refactored, the only difference is the
> flag checking.
Yeah, I somewhat though about it... anyway, it's not important right
now, I'm waiting for feedback on the approach itself. I'll send a new
patch with a nicer implementation later.
Cheers,
Ben.
On Tue, 2007-12-18 at 00:49 +0100, Johannes Weiner wrote:
> These two functions should be refactored, the only difference is the
> flag checking.
FYI. Here's what it looks like in my next version of the patch:
Index: linux-work/drivers/pci/pci.c
===================================================================
--- linux-work.orig/drivers/pci/pci.c 2007-10-15 11:19:38.000000000 +1000
+++ linux-work/drivers/pci/pci.c 2007-12-18 11:01:20.000000000 +1100
@@ -736,6 +736,51 @@ pci_enable_device_bars(struct pci_dev *d
return err;
}
+static int __pci_enable_device_flags(struct pci_dev *dev,
+ resource_size_t flags)
+{
+ int err;
+ int i, bars = 0;
+
+ if (atomic_add_return(1, &dev->enable_cnt) > 1)
+ return 0; /* already enabled */
+
+ for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
+ if (dev->resource[i].flags & flags)
+ bars |= (1 << i);
+
+ err = do_pci_enable_device(dev, bars);
+ if (err < 0)
+ atomic_dec(&dev->enable_cnt);
+ return err;
+}
+
+/**
+ * pci_enable_device_io - Initialize a device for use with IO space
+ * @dev: PCI device to be initialized
+ *
+ * Initialize device before it's used by a driver. Ask low-level code
+ * to enable I/O resources. Wake up the device if it was suspended.
+ * Beware, this function can fail.
+ */
+int pci_enable_device_io(struct pci_dev *dev)
+{
+ return __pci_enable_device_flags(dev, IORESOURCE_IO);
+}
+
+/**
+ * pci_enable_device_mem - Initialize a device for use with Memory space
+ * @dev: PCI device to be initialized
+ *
+ * Initialize device before it's used by a driver. Ask low-level code
+ * to enable Memory resources. Wake up the device if it was suspended.
+ * Beware, this function can fail.
+ */
+int pci_enable_device_mem(struct pci_dev *dev)
+{
+ return __pci_enable_device_flags(dev, IORESOURCE_MEM);
+}
+
/**
* pci_enable_device - Initialize device before it's used by a driver.
* @dev: PCI device to be initialized
@@ -749,7 +794,7 @@ pci_enable_device_bars(struct pci_dev *d
*/
int pci_enable_device(struct pci_dev *dev)
{
- return pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1);
+ return __pci_enable_device_flags(dev, IORESOURCE_MEM | IORESOURCE_IO);
}
/*
@@ -1621,6 +1666,8 @@ device_initcall(pci_init);
EXPORT_SYMBOL_GPL(pci_restore_bars);
EXPORT_SYMBOL(pci_reenable_device);
EXPORT_SYMBOL(pci_enable_device_bars);
+EXPORT_SYMBOL(pci_enable_device_io);
+EXPORT_SYMBOL(pci_enable_device_mem);
EXPORT_SYMBOL(pci_enable_device);
EXPORT_SYMBOL(pcim_enable_device);
EXPORT_SYMBOL(pcim_pin_device);
Index: linux-work/include/linux/pci.h
===================================================================
--- linux-work.orig/include/linux/pci.h 2007-12-18 11:00:32.000000000 +1100
+++ linux-work/include/linux/pci.h 2007-12-18 11:00:33.000000000 +1100
@@ -548,6 +548,8 @@ static inline int pci_write_config_dword
int __must_check pci_enable_device(struct pci_dev *dev);
int __must_check pci_enable_device_bars(struct pci_dev *dev, int mask);
+int __must_check pci_enable_device_io(struct pci_dev *dev);
+int __must_check pci_enable_device_mem(struct pci_dev *dev);
int __must_check pci_reenable_device(struct pci_dev *);
int __must_check pcim_enable_device(struct pci_dev *pdev);
void pcim_pin_device(struct pci_dev *pdev);
On Tue, 18 Dec 2007 10:01:14 +1100
Benjamin Herrenschmidt <[email protected]> wrote:
> This patch converts users of pci_enable_device_bars() to the new
> pci_enable_device_{io,mem} interface.
>
> The new API fits nicely, except maybe for the QLA case where a bit of
> code re-organization might be a good idea but I prefer sticking to the
> simple patch as I don't have hardware to test on.
>
> I'll also need some feedback on the cs5520 change
5520 in fact is always enabled as it is the host bridge.
pci_enable_device_io will do just fine. The 5520 fun is if you disable it
the system hangs.
On Tue, 2007-12-18 at 00:07 +0000, Alan Cox wrote:
> On Tue, 18 Dec 2007 10:01:14 +1100
> Benjamin Herrenschmidt <[email protected]> wrote:
>
> > This patch converts users of pci_enable_device_bars() to the new
> > pci_enable_device_{io,mem} interface.
> >
> > The new API fits nicely, except maybe for the QLA case where a bit of
> > code re-organization might be a good idea but I prefer sticking to the
> > simple patch as I don't have hardware to test on.
> >
> > I'll also need some feedback on the cs5520 change
>
> 5520 in fact is always enabled as it is the host bridge.
> pci_enable_device_io will do just fine. The 5520 fun is if you disable it
> the system hangs.
Ok, thanks, that's not my idea of fun though :-)
Cheers,
Ben.
On Tue, Dec 18, 2007 at 11:02:37AM +1100, Benjamin Herrenschmidt wrote:
> +EXPORT_SYMBOL(pci_enable_device_io);
> +EXPORT_SYMBOL(pci_enable_device_mem);
> EXPORT_SYMBOL(pci_enable_device);
Wouldn't it be better to export only the pci_enable_device_flags()
and make these three just "static inline" wrappers in pci.h?
> EXPORT_SYMBOL(pcim_enable_device);
Looks like many SATA drivers would benefit from pcim_* equivalents
of pci_enable_device_{io,mem}, as they could happily work with just
a single MMIO BAR... Alan?
Ivan.
On Tue, 2007-12-18 at 12:37 +0300, Ivan Kokshaysky wrote:
> On Tue, Dec 18, 2007 at 11:02:37AM +1100, Benjamin Herrenschmidt wrote:
> > +EXPORT_SYMBOL(pci_enable_device_io);
> > +EXPORT_SYMBOL(pci_enable_device_mem);
> > EXPORT_SYMBOL(pci_enable_device);
>
> Wouldn't it be better to export only the pci_enable_device_flags()
> and make these three just "static inline" wrappers in pci.h?
Been thinking about it yeah. I don't want drivers to start using
the _flags version directly tho, which is why I preferred going that
way but if people prefer the inline version, I'll do that.
> > EXPORT_SYMBOL(pcim_enable_device);
>
> Looks like many SATA drivers would benefit from pcim_* equivalents
> of pci_enable_device_{io,mem}, as they could happily work with just
> a single MMIO BAR... Alan?
I have no experience with the pcim stuff, I would appreciate if somebody
took care of that part.
Cheers,
Ben.
On Tue, Dec 18, 2007 at 10:01:15AM +1100, Benjamin Herrenschmidt wrote:
> @@ -1040,7 +1040,10 @@ static inline void __devinit alloc_resou
> r->flags |= IORESOURCE_UNSET;
> r->end -= r->start;
> r->start = 0;
Perhaps we should use IORESOURCE_UNSET universally... It's a lot better
than clearing r->start which is in fact architecture dependent thing
and in the end just destroys information for no purpose.
Ivan.
On Tue, 2007-12-18 at 12:56 +0300, Ivan Kokshaysky wrote:
> On Tue, Dec 18, 2007 at 10:01:15AM +1100, Benjamin Herrenschmidt wrote:
> > @@ -1040,7 +1040,10 @@ static inline void __devinit alloc_resou
> > r->flags |= IORESOURCE_UNSET;
> > r->end -= r->start;
> > r->start = 0;
>
> Perhaps we should use IORESOURCE_UNSET universally... It's a lot better
> than clearing r->start which is in fact architecture dependent thing
> and in the end just destroys information for no purpose.
I'm not totally sure. I was actually tempted to switch powerpc to get
rid of it lately :-)
The problem is that a resource is never "unset" on PCI... the BAR always
contains something... the question is whether that something is going to
be problematic or not, or rather, whether that something can be
"allocated" (ie. fitted into the resource tree in a free spot or not).
And we have a way of knowing that already which is ... resource->parent
being NULL or not. That also happens to be what
pci_assign_unassigned_resources() uses and I think the various arch
pcibios_enable_device() should use (in fact I have a fix to make PowerPC
use that) instead of testing res->start or even IORESOURCE_UNSET.
Anything that isn't in the resource tree is potentially stale and thus
mustn't be enabled (ie. IO/MEM decoding must not be enabled on a device
that has a BAR whose res->parent is NULL).
The reason why we introduced use of IORESOURCE_UNSET on powerpc goes
back from when we had our own resource assignment code there that was
somewhat a half-assed version of what pci_assign_unassigned_resources().
We decided to introduce IORESOURCE_UNSET rather than testing res->start
because it could cope with a value of 0 being valid, and on machines
with no legacy ISA stuff at all, 0 is a valid BAR value for some things,
and we weren't smart enough to reassign those things in all
circumstances.
Nowadays, that doesn't seem necessary anymore, and in fact, I think we
can still support those valid 0 BARs -and- not have IORESOURCE_UNSET by
standardizing on the idea that res->parent is the only indicator of a
resource validity... unless I'm missing something which is quite
possible :-)
Cheers,
Ben.
This patch changes the x86 PCI code to disable IO and/or Memory
decoding on a PCI device when a resource of that type failed to
be allocated.
This is done to avoid having unallocated dangling BARs enabled
that might try to decode on top of other devices.
If a proper resource is assigned later on, then
pci_enable_device{,_io,_mem} will take care of re-enabling
decoding.
Signed-off-by: Benjamin Herrenschmidt <[email protected]>
---
On top of my previous 4 patches to replace pci_enable_device_bars()
I don't have any x86 at hand to test it... this will probably want to
simmer a little bit in -mm, though it shouldn't have any problem of
user setups that don't already have dodgy resource setup. In fact, it
might turn weird latent bugs into right away visible ones.
I'll wait for more comments today and post the whole 5 again tomorrow
as official candidates for inclusion :-) (BTW. What is people general
feeling about inline vs. non inline for the functions in pci.c ?)
Index: linux-merge/arch/x86/pci/i386.c
===================================================================
--- linux-merge.orig/arch/x86/pci/i386.c 2007-12-19 16:00:31.000000000 +1100
+++ linux-merge/arch/x86/pci/i386.c 2007-12-19 16:02:20.000000000 +1100
@@ -143,12 +143,32 @@ static void __init pcibios_allocate_bus_
}
}
+static inline int __init pcibios_alloc_resource(struct pci_dev *dev, int idx)
+{
+ struct resource *pr, *r = &dev->resource[idx];
+
+ DBG("PCI: Resource %08lx-%08lx (f=%lx, d=%d, p=%d)\n",
+ r->start, r->end, r->flags, disabled, pass);
+
+ pr = pci_find_parent_resource(dev, r);
+ if (!pr || request_resource(pr, r) < 0) {
+ printk(KERN_ERR "PCI: Cannot allocate "
+ "resource region %d of device %s\n",
+ idx, pci_name(dev));
+ /* We'll assign a new address later */
+ r->end -= r->start;
+ r->start = 0;
+ return -EBUSY;
+ }
+ return 0;
+}
+
static void __init pcibios_allocate_resources(int pass)
{
struct pci_dev *dev = NULL;
int idx, disabled;
u16 command;
- struct resource *r, *pr;
+ struct resource *r;
for_each_pci_dev(dev) {
pci_read_config_word(dev, PCI_COMMAND, &command);
@@ -162,20 +182,11 @@ static void __init pcibios_allocate_reso
disabled = !(command & PCI_COMMAND_IO);
else
disabled = !(command & PCI_COMMAND_MEMORY);
- if (pass == disabled) {
- DBG("PCI: Resource %08lx-%08lx "
- "(f=%lx, d=%d, p=%d)\n",
- r->start, r->end, r->flags, disabled, pass);
- pr = pci_find_parent_resource(dev, r);
- if (!pr || request_resource(pr, r) < 0) {
- printk(KERN_ERR "PCI: Cannot allocate "
- "resource region %d "
- "of device %s\n",
- idx, pci_name(dev));
- /* We'll assign a new address later */
- r->end -= r->start;
- r->start = 0;
- }
+ if (pass == disabled && pcibios_alloc_resource(dev, idx)) {
+ command &= ~(r->flags & (IORESOURCE_IO |
+ IORESOURCE_MEM));
+ pci_write_config_word(dev,
+ PCI_COMMAND, command);
}
}
if (!pass) {
On Wed, Dec 19, 2007 at 04:10:19PM +1100, Benjamin Herrenschmidt wrote:
> This patch changes the x86 PCI code to disable IO and/or Memory
> decoding on a PCI device when a resource of that type failed to
> be allocated.
Oh, this opens up a can of worms ;-)
In ideal world, the patch would be perfectly valid. But on x86 we have
at least two firmware layers (E820 and ACPI), each with its own (often
totally crazy) view on system resources. OTOH, we cannot completely ignore
the firmware - otherwise the resource allocator could step onto some
hidden/undocumented system decode ranges...
One of the typical reasons for "dangling BAR" on x86 is that a resource
allocation failed because BIOS has reserved address region for the
very same BAR ;-)
Perhaps you've seen most recent illustration of x86 resource allocation
problems:
http://lkml.org/lkml/2007/12/17/429
Plus there are lots of x86 hardware like host bridges with a BAR
representing the system RAM, IOAPIC BARs with some high order bits
hardwired to "1" and so on. Which also doesn't make life any easier.
That's why I still think that res->parent check is not enough for
x86 and we need some resource flag that tells generic PCI code
"We failed to allocate this resource, but please don't touch it!".
Some code is already using IORESOURCE_PCI_FIXED for that purpose, so it
would suffice.
On the other hand, with that flag we can move all those horrible
exceptions like PCI_CLASS_BRIDGE_HOST (which nearly breaks alpha, BTW)
and PCI_CLASS_SYSTEM_PIC from generic code to arch/x86 where it belongs.
> I'll wait for more comments today and post the whole 5 again tomorrow
> as official candidates for inclusion :-) (BTW. What is people general
> feeling about inline vs. non inline for the functions in pci.c ?)
I think inlines are prettier, but not allowing direct use of the _flag
function is a valid argument too. So I'm fine with both.
Ivan.
On Wed, 2007-12-19 at 16:43 +0300, Ivan Kokshaysky wrote:
> On Wed, Dec 19, 2007 at 04:10:19PM +1100, Benjamin Herrenschmidt wrote:
> > This patch changes the x86 PCI code to disable IO and/or Memory
> > decoding on a PCI device when a resource of that type failed to
> > be allocated.
>
> Oh, this opens up a can of worms ;-)
>
> In ideal world, the patch would be perfectly valid. But on x86 we have
> at least two firmware layers (E820 and ACPI), each with its own (often
> totally crazy) view on system resources. OTOH, we cannot completely ignore
> the firmware - otherwise the resource allocator could step onto some
> hidden/undocumented system decode ranges...
> One of the typical reasons for "dangling BAR" on x86 is that a resource
> allocation failed because BIOS has reserved address region for the
> very same BAR ;-)
>
> Perhaps you've seen most recent illustration of x86 resource allocation
> problems:
> http://lkml.org/lkml/2007/12/17/429
Yeah, gives me headaches.
> Plus there are lots of x86 hardware like host bridges with a BAR
> representing the system RAM, IOAPIC BARs with some high order bits
> hardwired to "1" and so on. Which also doesn't make life any easier.
Ok. We tent do have quirks for those on powerpc.
> That's why I still think that res->parent check is not enough for
> x86 and we need some resource flag that tells generic PCI code
> "We failed to allocate this resource, but please don't touch it!".
> Some code is already using IORESOURCE_PCI_FIXED for that purpose, so it
> would suffice.
Yup, possibly. On the other hand, it's also used for other things. It
normally means a fixed decode resource, such as IDE in legacy mode. If
that conflicts for real, then the only option -is- to disable the
device.
The problem on x86 seems to be to differenciate what conflicts for real
and what is just this resource management crackpot.
> On the other hand, with that flag we can move all those horrible
> exceptions like PCI_CLASS_BRIDGE_HOST (which nearly breaks alpha, BTW)
> and PCI_CLASS_SYSTEM_PIC from generic code to arch/x86 where it belongs.
Heh, possibly yeah :-)
> > I'll wait for more comments today and post the whole 5 again tomorrow
> > as official candidates for inclusion :-) (BTW. What is people general
> > feeling about inline vs. non inline for the functions in pci.c ?)
>
> I think inlines are prettier, but not allowing direct use of the _flag
> function is a valid argument too. So I'm fine with both.
Ok. I'll keep the x86 patch out for now though. I'll let others sort out
what happens on this platform.
Cheers,
Ben.
On Tue, Dec 18, 2007 at 10:01:14AM +1100, Benjamin Herrenschmidt wrote:
> Now that all in-tree users are gone, this removes pci_enable_device_bars()
> completely.
Almost completely.
Patch below removes pci_enable_device_bars() from Documentation/pci.txt .
(And I'd be happy to review patches to pci.txt that add
pci_enable_device_io() and _mmio() as well. )
Signed-off-by: Grant Grundler <[email protected]>
diff --git a/Documentation/pci.txt b/Documentation/pci.txt
index 7754f5a..72b20c6 100644
--- a/Documentation/pci.txt
+++ b/Documentation/pci.txt
@@ -274,8 +274,6 @@ the PCI device by calling pci_enable_device(). This will:
o allocate an IRQ (if BIOS did not).
NOTE: pci_enable_device() can fail! Check the return value.
-NOTE2: Also see pci_enable_device_bars() below. Drivers can
- attempt to enable only a subset of BARs they need.
[ OS BUG: we don't check resource allocations before enabling those
resources. The sequence would make more sense if we called
@@ -605,40 +603,7 @@ device lists. This is still possible but discouraged.
-10. pci_enable_device_bars() and Legacy I/O Port space
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-
-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_region() 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 OSes 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() and 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.
-
-
-
-11. MMIO Space and "Write Posting"
+10. MMIO Space and "Write Posting"
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Converting a driver from using I/O Port space to using MMIO space
Just a style nit...
On Tue, Dec 18, 2007 at 10:01:14AM +1100, Benjamin Herrenschmidt wrote:
> This patch converts users of pci_enable_device_bars() to the new
> pci_enable_device_{io,mem} interface.
>
> The new API fits nicely, except maybe for the QLA case where a bit of
> code re-organization might be a good idea but I prefer sticking to the
> simple patch as I don't have hardware to test on.
>
> I'll also need some feedback on the cs5520 change.
>
> Signed-off-by: Benjamin Herrenschmidt <[email protected]>
....
> /* We must not grab the entire device, it has 'ISA' space in its
> - BARS too and we will freak out other bits of the kernel */
> - if (pci_enable_device_bars(dev, 1<<2)) {
> + * BARS too and we will freak out other bits of the kernel
> + *
> + * pci_enable_device_bars() is going away. I replaced it with
> + * IO only enable for now but I'll need confirmation this is
> + * allright for that device. If not, it will need some kind of
> + * quirk. --BenH.
> + */
This comment doesn't really belong in the code. It should be part of
the commit log or email RFC.
cheers,
grant
On Tue, Dec 18, 2007 at 10:01:15AM +1100, Benjamin Herrenschmidt wrote:
> This patch changes the PowerPC PCI code to disable IO and/or Memory
> decoding on a PCI device when a resource of that type failed to be
> allocated. This is done to avoid having unallocated dangling BARs enabled
> that might try to decode on top of other devices.
>
> If a proper resource is assigned later on, then pci_enable_device{,_io,_mem}
> will take care of re-enabling decoding.
>
> Signed-off-by: Benjamin Herrenschmidt <[email protected]>
....
> @@ -1062,8 +1065,12 @@ static void __init pcibios_allocate_reso
> disabled = !(command & PCI_COMMAND_IO);
> else
> disabled = !(command & PCI_COMMAND_MEMORY);
> - if (pass == disabled)
> - alloc_resource(dev, idx);
> + if (pass == disabled && alloc_resource(dev, idx)) {
> + command &= ~(r->flags & (IORESOURCE_IO |
> + IORESOURCE_MEM));
While this may be ok for PPC, in general, wouldn't we want to only disable
which ever type of resource that couldn't be allocated?
ie make two calls: alloc_resource_io() and alloc_resource_mem() and disable
the respective flag if the alloc call fails?
Thus a device which was enable and programmed by BIOS could remain functional
despite one resource not being allocated.
(e.g. MMIO was allocated successfully while IO Port space was not)
Again, this is just a hypothetical question since I have no example (yet)
off hand where this is true. ISTR, the original discussion around
pci_enable_device_bars() suggested some machines already have this issue.
cheers,
grant
> + pci_write_config_word(dev,
> + PCI_COMMAND, command);
> + }
> }
> if (pass)
> continue;
On Mon, 2007-12-24 at 00:08 -0700, Grant Grundler wrote:
> On Tue, Dec 18, 2007 at 10:01:14AM +1100, Benjamin Herrenschmidt wrote:
> > Now that all in-tree users are gone, this removes pci_enable_device_bars()
> > completely.
>
> Almost completely.
> Patch below removes pci_enable_device_bars() from Documentation/pci.txt .
>
> (And I'd be happy to review patches to pci.txt that add
> pci_enable_device_io() and _mmio() as well. )
>
> Signed-off-by: Grant Grundler <[email protected]>
Will do.
Thanks !
Ben.
On Mon, 2007-12-24 at 00:23 -0700, Grant Grundler wrote:
> On Tue, Dec 18, 2007 at 10:01:15AM +1100, Benjamin Herrenschmidt wrote:
> > This patch changes the PowerPC PCI code to disable IO and/or Memory
> > decoding on a PCI device when a resource of that type failed to be
> > allocated. This is done to avoid having unallocated dangling BARs enabled
> > that might try to decode on top of other devices.
> >
> > If a proper resource is assigned later on, then pci_enable_device{,_io,_mem}
> > will take care of re-enabling decoding.
> >
> > Signed-off-by: Benjamin Herrenschmidt <[email protected]>
> ....
> > @@ -1062,8 +1065,12 @@ static void __init pcibios_allocate_reso
> > disabled = !(command & PCI_COMMAND_IO);
> > else
> > disabled = !(command & PCI_COMMAND_MEMORY);
> > - if (pass == disabled)
> > - alloc_resource(dev, idx);
> > + if (pass == disabled && alloc_resource(dev, idx)) {
> > + command &= ~(r->flags & (IORESOURCE_IO |
> > + IORESOURCE_MEM));
>
> While this may be ok for PPC, in general, wouldn't we want to only disable
> which ever type of resource that couldn't be allocated?
This is exactly what's supposed to be happening, but the code is buggy
and nobody noticed :-) (I'm mixing up IORESOURCE_* flags and
PCI_COMMAND_* flags). Thanks for reviewing !
> ie make two calls: alloc_resource_io() and alloc_resource_mem() and disable
> the respective flag if the alloc call fails?
No need for 2 calls, just disable whatever type the resource is, but
using the right bits instead of what my code incorrectly does.
> Thus a device which was enable and programmed by BIOS could remain functional
> despite one resource not being allocated.
Yes, that's what intended by the above code, if I didn't have mixed up
the flags.
Ben.
> This is exactly what's supposed to be happening, but the code is buggy
> and nobody noticed :-) (I'm mixing up IORESOURCE_* flags and
> PCI_COMMAND_* flags). Thanks for reviewing !
Note that this patch isn't in the series Greg queued up anyway. The
powerpc specific bits will be going in via Paulus an I already asked him
to drop that specific one until I send a fixed version.
Cheers,
Ben.
On Wed, Dec 26, 2007 at 08:26:27AM +1100, Benjamin Herrenschmidt wrote:
>
> > This is exactly what's supposed to be happening, but the code is buggy
> > and nobody noticed :-) (I'm mixing up IORESOURCE_* flags and
> > PCI_COMMAND_* flags). Thanks for reviewing !
>
> Note that this patch isn't in the series Greg queued up anyway. The
> powerpc specific bits will be going in via Paulus an I already asked him
> to drop that specific one until I send a fixed version.
Ah ok....but I was mostly worried about the use of PPC bits as an example.
Anyway, in general it looks like the right direction to me too.
cheers,
grant