Re: [PATCH] IPMI: reserve memio regions separately

[Corey Minyard]

On 04/27/2016 08:24 AM, Mark Salter wrote:

On Tue, 2016-04-26 at 23:31 -0500, miny...@acm.org wrote:

From: Corey Minyard <cminy...@mvista.com>

Commit d61a3ead2680 ("[PATCH] IPMI: reserve I/O ports separately")
changed the way I/O ports were reserved and includes this comment in
log:

  Some BIOSes reserve disjoint I/O regions in their ACPI tables for the IPMI
  controller.  This causes problems when trying to register the entire I/O
  region.  Therefore we must register each I/O port separately.

There is a similar problem with memio regions on an arm64 platform
(AMD Seattle). Where I see:

  ipmi message handler version 39.2
  ipmi_si AMDI0300:00: probing via device tree
  ipmi_si AMDI0300:00: ipmi_si: probing via ACPI
  ipmi_si AMDI0300:00: [mem 0xe0010000] regsize 1 spacing 4 irq 23
  ipmi_si: Adding ACPI-specified kcs state machine
  IPMI System Interface driver.
  ipmi_si: Trying ACPI-specified kcs state machine at mem \
           address 0xe0010000, slave address 0x0, irq 23
  ipmi_si: Could not set up I/O space

The problem is that the ACPI core registers disjoint regions for the
platform device:

e0010000-e0010000 : AMDI0300:00
e0010004-e0010004 : AMDI0300:00

and the ipmi_si driver tries to register one region e0010000-e0010004.

Based on a patch from Mark Salter <msal...@redhat.com>

Signed-off-by: Corey Minyard <cminy...@mvista.com>
---

This works for me as well.

Tested-by: Mark Salter <msal...@redhat.com>

Thanks a bunch for testing this.  Queued for 4.7 and in linux-next.

-corey

  drivers/char/ipmi/ipmi_si_intf.c | 40 +++++++++++++++++++++++++++-------------
  1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 6ecf9af..a815044 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -1637,25 +1637,28 @@ static void mem_outq(const struct si_sm_io *io, 
unsigned int offset,
  }
  #endif

-static void mem_cleanup(struct smi_info *info)

+static void mem_region_cleanup(struct smi_info *info, int num)
  {
        unsigned long addr = info->io.addr_data;
-       int           mapsize;
+       int idx;
+
+       for (idx = 0; idx < num; idx++)
+               release_mem_region(addr + idx * info->io.regspacing,
+                                  info->io.regsize);
+}

+static void mem_cleanup(struct smi_info *info)

+{
        if (info->io.addr) {
                iounmap(info->io.addr);
-
-               mapsize = ((info->io_size * info->io.regspacing)
-                          - (info->io.regspacing - info->io.regsize));
-
-               release_mem_region(addr, mapsize);
+               mem_region_cleanup(info, info->io_size);
        }
  }

static int mem_setup(struct smi_info *info)

  {
        unsigned long addr = info->io.addr_data;
-       int           mapsize;
+       int           mapsize, idx;

if (!addr)

                return -ENODEV;
@@ -1692,6 +1695,21 @@ static int mem_setup(struct smi_info *info)
        }

/*

+        * Some BIOSes reserve disjoint memory regions in their ACPI
+        * tables.  This causes problems when trying to request the
+        * entire region.  Therefore we must request each register
+        * separately.
+        */
+       for (idx = 0; idx < info->io_size; idx++) {
+               if (request_mem_region(addr + idx * info->io.regspacing,
+                                      info->io.regsize, DEVICE_NAME) == NULL) {
+                       /* Undo allocations */
+                       mem_region_cleanup(info, idx);
+                       return -EIO;
+               }
+       }
+
+       /*
         * Calculate the total amount of memory to claim.  This is an
         * unusual looking calculation, but it avoids claiming any
         * more memory than it has to.  It will claim everything
@@ -1700,13 +1718,9 @@ static int mem_setup(struct smi_info *info)
         */
        mapsize = ((info->io_size * info->io.regspacing)
                   - (info->io.regspacing - info->io.regsize));
-
-       if (request_mem_region(addr, mapsize, DEVICE_NAME) == NULL)
-               return -EIO;
-
        info->io.addr = ioremap(addr, mapsize);
        if (info->io.addr == NULL) {
-               release_mem_region(addr, mapsize);
+               mem_region_cleanup(info, info->io_size);
                return -EIO;
        }
        return 0;