i20 currently uses pci_find_parent_resource() followed by
allocate_resource() to allocate PCI space. The problem is that this won't
work reliably because before we allocate the space, we don't know its
address, and therefore we can't find the parent resource. Even if we know
the *type* of space we want, in some cases there are multiple possibilities
(root buses with multiple apertures to them, prefetchable or
non-prefetchable apertures, etc.)
This changes it to use pci_bus_alloc_resource(), which takes care of those
details.
This also fixes some things that look like copy/paste errors, e.g., trying
to allocate an I/O space with 1MB alignment.
I don't have hardware to test these changes, but I don't think these
allocation paths actually worked before, this shouldn't be any worse than
what we have today.
---
Bjorn Helgaas (4):
i2o: Fix I/O space allocation copy/paste error
i2o: Fix I/O space alignment requirement
i2o: Refactor i2o_iop_systab_set() PCI space allocation
i2o: Use pci_bus_alloc_resource(), not allocate_resource() directly
drivers/message/i2o/iop.c | 85 ++++++++++++++++++++++-----------------------
1 file changed, 42 insertions(+), 43 deletions(-)
When i2o_iop_systab_set() allocates I/O port space, it assigns the base of
the new I/O port region to sb->current_mem_base, not sb->current_io_base.
This looks like a copy/paste error, because we do use current_io_base, but
there's no other place that sets it.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/message/i2o/iop.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/message/i2o/iop.c b/drivers/message/i2o/iop.c
index a8c08f332da0..a8373d7aaef7 100644
--- a/drivers/message/i2o/iop.c
+++ b/drivers/message/i2o/iop.c
@@ -704,7 +704,7 @@ static int i2o_iop_systab_set(struct i2o_controller *c)
NULL, NULL) >= 0) {
c->io_alloc = 1;
sb->current_io_size = resource_size(res);
- sb->current_mem_base = res->start;
+ sb->current_io_base = res->start;
osm_info("%s: allocated %llu bytes of PCI I/O at "
"0x%016llX.\n", c->name,
(unsigned long long)resource_size(res),
When i2o_iop_systab_set() allocates I/O port space, it specifies 1Mb
alignment required. This seems unlikely, since most platforms have only
64Kb of I/O space total. I think 4Kb is a more reasonable choice, since
that's the minimum alignment of a PCI-PCI bridge I/O window.
My guess is that this is a copy/paste error from the memory allocation
code, which specifies 1Mb alignment (which is the minimum alignment of a
PCI-PCI bridge memory window).
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/message/i2o/iop.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/message/i2o/iop.c b/drivers/message/i2o/iop.c
index a8373d7aaef7..68aef58bf89c 100644
--- a/drivers/message/i2o/iop.c
+++ b/drivers/message/i2o/iop.c
@@ -700,7 +700,7 @@ static int i2o_iop_systab_set(struct i2o_controller *c)
root = pci_find_parent_resource(c->pdev, res);
if (root == NULL)
osm_warn("%s: Can't find parent resource!\n", c->name);
- if (root && allocate_resource(root, res, sb->desired_io_size, sb->desired_io_size, sb->desired_io_size, 1 << 20, /* Unspecified, so use 1Mb and play safe */
+ if (root && allocate_resource(root, res, sb->desired_io_size, sb->desired_io_size, sb->desired_io_size, 1 << 12, /* Unspecified, so use 4Kb and play safe */
NULL, NULL) >= 0) {
c->io_alloc = 1;
sb->current_io_size = resource_size(res);
Convert i2o_res_alloc() to use pci_bus_alloc_resource() rather than
pci_find_parent_resource() and allocate_resource(). We don't have a
resource to start with, so pci_find_parent_resource() can't do anything
useful: a bus may have several memory resources available, so there might
be several possible parents. This is more likely on root buses because
host bridges may have any number of apertures.
I'm pretty sure this didn't work in the first place because it passed
size == min == max to allocate_resource(). The min and max parameters are
constraints on the *addresses* of the resource, not on its size, so I think
it was impossible for allocate_resource() to succeed.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/message/i2o/iop.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/message/i2o/iop.c b/drivers/message/i2o/iop.c
index c334ba7224a1..5e7b1b75f999 100644
--- a/drivers/message/i2o/iop.c
+++ b/drivers/message/i2o/iop.c
@@ -655,8 +655,8 @@ static int i2o_iop_activate(struct i2o_controller *c)
static void i2o_res_alloc(struct i2o_controller *c, unsigned long flags)
{
i2o_status_block *sb = c->status_block.virt;
- struct resource *root, *res = &c->mem_resource;
- resource_size_t size, min, max, align;
+ struct resource *res = &c->mem_resource;
+ resource_size_t size, align;
int err;
res->name = c->pdev->bus->name;
@@ -664,21 +664,17 @@ static void i2o_res_alloc(struct i2o_controller *c, unsigned long flags)
res->start = 0;
res->end = 0;
osm_info("%s: requires private memory resources.\n", c->name);
- root = pci_find_parent_resource(c->pdev, res);
- if (root == NULL) {
- osm_warn("%s: Can't find parent resource!\n", c->name);
- return;
- }
if (flags & IORESOURCE_MEM) {
- size = min = max = sb->desired_mem_size;
+ size = sb->desired_mem_size;
align = 1 << 20; /* unspecified, use 1Mb and play safe */
} else if (flags & IORESOURCE_IO) {
- size = min = max = sb->desired_io_size;
+ size = sb->desired_io_size;
align = 1 << 12; /* unspecified, use 4Kb and play safe */
}
- err = allocate_resource(root, res, size, min, max, align, NULL, NULL);
+ err = pci_bus_alloc_resource(c->pdev->bus, res, size, align, 0, 0,
+ NULL, NULL);
if (err < 0)
return;
Refactor the PCI space allocation in i2o_iop_systab_set(). This might
improve readability slightly, but mainly it is to make the next patch
simpler.
No functional change.
Signed-off-by: Bjorn Helgaas <[email protected]>
---
drivers/message/i2o/iop.c | 89 +++++++++++++++++++++++----------------------
1 file changed, 46 insertions(+), 43 deletions(-)
diff --git a/drivers/message/i2o/iop.c b/drivers/message/i2o/iop.c
index 68aef58bf89c..c334ba7224a1 100644
--- a/drivers/message/i2o/iop.c
+++ b/drivers/message/i2o/iop.c
@@ -652,6 +652,48 @@ static int i2o_iop_activate(struct i2o_controller *c)
return i2o_hrt_get(c);
};
+static void i2o_res_alloc(struct i2o_controller *c, unsigned long flags)
+{
+ i2o_status_block *sb = c->status_block.virt;
+ struct resource *root, *res = &c->mem_resource;
+ resource_size_t size, min, max, align;
+ int err;
+
+ res->name = c->pdev->bus->name;
+ res->flags = flags;
+ res->start = 0;
+ res->end = 0;
+ osm_info("%s: requires private memory resources.\n", c->name);
+ root = pci_find_parent_resource(c->pdev, res);
+ if (root == NULL) {
+ osm_warn("%s: Can't find parent resource!\n", c->name);
+ return;
+ }
+
+ if (flags & IORESOURCE_MEM) {
+ size = min = max = sb->desired_mem_size;
+ align = 1 << 20; /* unspecified, use 1Mb and play safe */
+ } else if (flags & IORESOURCE_IO) {
+ size = min = max = sb->desired_io_size;
+ align = 1 << 12; /* unspecified, use 4Kb and play safe */
+ }
+
+ err = allocate_resource(root, res, size, min, max, align, NULL, NULL);
+ if (err < 0)
+ return;
+
+ if (flags & IORESOURCE_MEM) {
+ c->mem_alloc = 1;
+ sb->current_mem_size = resource_size(res);
+ sb->current_mem_base = res->start;
+ } else if (flags & IORESOURCE_IO) {
+ c->io_alloc = 1;
+ sb->current_io_size = resource_size(res);
+ sb->current_io_base = res->start;
+ }
+ osm_info("%s: allocated PCI space %pR\n", c->name, res);
+}
+
/**
* i2o_iop_systab_set - Set the I2O System Table of the specified IOP
* @c: I2O controller to which the system table should be send
@@ -665,52 +707,13 @@ static int i2o_iop_systab_set(struct i2o_controller *c)
struct i2o_message *msg;
i2o_status_block *sb = c->status_block.virt;
struct device *dev = &c->pdev->dev;
- struct resource *root;
int rc;
- if (sb->current_mem_size < sb->desired_mem_size) {
- struct resource *res = &c->mem_resource;
- res->name = c->pdev->bus->name;
- res->flags = IORESOURCE_MEM;
- res->start = 0;
- res->end = 0;
- osm_info("%s: requires private memory resources.\n", c->name);
- root = pci_find_parent_resource(c->pdev, res);
- if (root == NULL)
- osm_warn("%s: Can't find parent resource!\n", c->name);
- if (root && allocate_resource(root, res, sb->desired_mem_size, sb->desired_mem_size, sb->desired_mem_size, 1 << 20, /* Unspecified, so use 1Mb and play safe */
- NULL, NULL) >= 0) {
- c->mem_alloc = 1;
- sb->current_mem_size = resource_size(res);
- sb->current_mem_base = res->start;
- osm_info("%s: allocated %llu bytes of PCI memory at "
- "0x%016llX.\n", c->name,
- (unsigned long long)resource_size(res),
- (unsigned long long)res->start);
- }
- }
+ if (sb->current_mem_size < sb->desired_mem_size)
+ i2o_res_alloc(c, IORESOURCE_MEM);
- if (sb->current_io_size < sb->desired_io_size) {
- struct resource *res = &c->io_resource;
- res->name = c->pdev->bus->name;
- res->flags = IORESOURCE_IO;
- res->start = 0;
- res->end = 0;
- osm_info("%s: requires private memory resources.\n", c->name);
- root = pci_find_parent_resource(c->pdev, res);
- if (root == NULL)
- osm_warn("%s: Can't find parent resource!\n", c->name);
- if (root && allocate_resource(root, res, sb->desired_io_size, sb->desired_io_size, sb->desired_io_size, 1 << 12, /* Unspecified, so use 4Kb and play safe */
- NULL, NULL) >= 0) {
- c->io_alloc = 1;
- sb->current_io_size = resource_size(res);
- sb->current_io_base = res->start;
- osm_info("%s: allocated %llu bytes of PCI I/O at "
- "0x%016llX.\n", c->name,
- (unsigned long long)resource_size(res),
- (unsigned long long)res->start);
- }
- }
+ if (sb->current_io_size < sb->desired_io_size)
+ i2o_res_alloc(c, IORESOURCE_IO);
msg = i2o_msg_get_wait(c, I2O_TIMEOUT_MESSAGE_GET);
if (IS_ERR(msg))
On Wed, 26 Feb 2014 12:09:27 -0700
Bjorn Helgaas <[email protected]> wrote:
> Convert i2o_res_alloc() to use pci_bus_alloc_resource() rather than
> pci_find_parent_resource() and allocate_resource(). We don't have a
> resource to start with, so pci_find_parent_resource() can't do anything
> useful: a bus may have several memory resources available, so there might
> be several possible parents. This is more likely on root buses because
> host bridges may have any number of apertures.
>
> I'm pretty sure this didn't work in the first place because it passed
> size == min == max to allocate_resource(). The min and max parameters are
> constraints on the *addresses* of the resource, not on its size, so I think
> it was impossible for allocate_resource() to succeed.
I don't think many i2o controllers ever used that path, and I doubt any
in normal use did as the vision of offloading for devices on the host bus
basically never happened (it happened even less than i2o)
A rather more sensible question might be "If i2o went away is there
anyone who would even notice". About the only devices that ever used i2o
in the real world (AMI MegaRAID and some FC stuff) had native firmware or
modes that worked better anyway.
Alan
On Wed, Feb 26, 2014 at 3:48 PM, One Thousand Gnomes
<[email protected]> wrote:
> On Wed, 26 Feb 2014 12:09:27 -0700
> Bjorn Helgaas <[email protected]> wrote:
>
>> Convert i2o_res_alloc() to use pci_bus_alloc_resource() rather than
>> pci_find_parent_resource() and allocate_resource(). We don't have a
>> resource to start with, so pci_find_parent_resource() can't do anything
>> useful: a bus may have several memory resources available, so there might
>> be several possible parents. This is more likely on root buses because
>> host bridges may have any number of apertures.
>>
>> I'm pretty sure this didn't work in the first place because it passed
>> size == min == max to allocate_resource(). The min and max parameters are
>> constraints on the *addresses* of the resource, not on its size, so I think
>> it was impossible for allocate_resource() to succeed.
>
> I don't think many i2o controllers ever used that path, and I doubt any
> in normal use did as the vision of offloading for devices on the host bus
> basically never happened (it happened even less than i2o)
>
> A rather more sensible question might be "If i2o went away is there
> anyone who would even notice". About the only devices that ever used i2o
> in the real world (AMI MegaRAID and some FC stuff) had native firmware or
> modes that worked better anyway.
I don't know anything about i2o, so I have no idea whether it could be
completely removed. I just want to remove its usage of
pci_find_parent_resource() so I can change the semantics of that a bit
(see [1]).
Bjorn
[1] https://lkml.kernel.org/r/20140226193723.10125.15799.stgit@bhelgaas-glaptop.roam.corp.google.com