[PATCH 0/4] i2o: Use pci_bus_alloc_resource()

[PATCH 0/4] i2o: Use pci_bus_alloc_resource()

[Bjorn Helgaas]

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(-)

[PATCH 1/4] i2o: Fix I/O space allocation copy/paste error

[Bjorn Helgaas]

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 <bhelgaas@google.com>
---
 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),

[PATCH 2/4] i2o: Fix I/O space alignment requirement

[Bjorn Helgaas]

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 <bhelgaas@google.com>
---
 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);

[PATCH 3/4] i2o: Refactor i2o_iop_systab_set() PCI space allocation

[Bjorn Helgaas]

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 <bhelgaas@google.com>
---
 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))

[PATCH 4/4] i2o: Use pci_bus_alloc_resource(), not allocate_resource() directly

[Bjorn Helgaas]

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 <bhelgaas@google.com>
---
 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;
 

Re: [PATCH 4/4] i2o: Use pci_bus_alloc_resource(), not allocate_resource() directly

[One Thousand Gnomes]

On Wed, 26 Feb 2014 12:09:27 -0700
Bjorn Helgaas <bhelgaas@google.com> 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

Re: [PATCH 4/4] i2o: Use pci_bus_alloc_resource(), not allocate_resource() directly

[Bjorn Helgaas]

On Wed, Feb 26, 2014 at 3:48 PM, One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:
> On Wed, 26 Feb 2014 12:09:27 -0700
> Bjorn Helgaas <bhelgaas@google.com> 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

Re: [PATCH 0/4] i2o: Use pci_bus_alloc_resource()

[Bjorn Helgaas]

On Wed, Feb 26, 2014 at 12:08 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> 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(-)

I applied these to pci/resource for v3.15.