[PATCH] IPMI: reserve memio regions separately

[PATCH] IPMI: reserve memio regions separately

[Mark Salter]

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.

This patch creates helper functions for region reservation so that
mem/port setup/cleanup ops can share the code which requests and
releases the regions separately.

Signed-off-by: Mark Salter <msalter@redhat.com>
---
 drivers/char/ipmi/ipmi_si_intf.c | 74 +++++++++++++++++++++++++++++-----------
 1 file changed, 54 insertions(+), 20 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 7fddd86..0149397 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -1476,6 +1476,51 @@ static int std_irq_setup(struct smi_info *info)
 	return rv;
 }
 
+static void region_cleanup(struct smi_info *info, int num, bool is_memio)
+{
+	unsigned long addr = info->io.addr_data;
+	int idx;
+
+	for (idx = 0; idx < num; idx++) {
+		if (is_memio)
+			release_mem_region(addr + idx * info->io.regspacing,
+					   info->io.regsize);
+		else
+			release_region(addr + idx * info->io.regspacing,
+				       info->io.regsize);
+	}
+}
+
+static int region_setup(struct smi_info *info, bool is_memio)
+{
+	unsigned long addr = info->io.addr_data;
+	struct resource *res;
+	int idx, offset;
+
+	/*
+	 * Some BIOSes reserve disjoint I/O regions in their ACPI
+	 * tables.  This causes problems when trying to register the
+	 * entire I/O region.  Therefore we must register each I/O
+	 * region separately.
+	 */
+	for (idx = 0; idx < info->io_size; idx++) {
+		offset = idx * info->io.regspacing;
+		if (is_memio)
+			res = request_mem_region(addr + offset,
+						 info->io.regsize, DEVICE_NAME);
+		else
+			res = request_region(addr + offset,
+					     info->io.regsize, DEVICE_NAME);
+
+		if (res == NULL) {
+			/* Undo allocations */
+			region_cleanup(info, idx, is_memio);
+			return -EIO;
+		}
+	}
+	return 0;
+}
+
 static unsigned char port_inb(const struct si_sm_io *io, unsigned int offset)
 {
 	unsigned int addr = io->addr_data;
@@ -1523,14 +1568,8 @@ static void port_outl(const struct si_sm_io *io, unsigned int offset,
 
 static void port_cleanup(struct smi_info *info)
 {
-	unsigned int addr = info->io.addr_data;
-	int          idx;
-
-	if (addr) {
-		for (idx = 0; idx < info->io_size; idx++)
-			release_region(addr + idx * info->io.regspacing,
-				       info->io.regsize);
-	}
+	if (info->io.addr_data)
+		region_cleanup(info, info->io_size, false);
 }
 
 static int port_setup(struct smi_info *info)
@@ -1640,23 +1679,17 @@ static void mem_outq(const struct si_sm_io *io, unsigned int offset,
 
 static void mem_cleanup(struct smi_info *info)
 {
-	unsigned long addr = info->io.addr_data;
-	int           mapsize;
-
 	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);
+		region_cleanup(info, info->io_size, true);
 	}
 }
 
 static int mem_setup(struct smi_info *info)
 {
 	unsigned long addr = info->io.addr_data;
-	int           mapsize;
+	int           mapsize, err;
 
 	if (!addr)
 		return -ENODEV;
@@ -1692,6 +1725,10 @@ static int mem_setup(struct smi_info *info)
 		return -EINVAL;
 	}
 
+	err = region_setup(info, true);
+	if (err)
+		return err;
+
 	/*
 	 * Calculate the total amount of memory to claim.  This is an
 	 * unusual looking calculation, but it avoids claiming any
@@ -1702,12 +1739,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);
+		region_cleanup(info, info->io_size, true);
 		return -EIO;
 	}
 	return 0;
-- 
1.8.3.1

[PATCH] IPMI: reserve memio regions separately

[minyard]

From: Corey Minyard <cminyard@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 <msalter@redhat.com>

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 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;
-- 
2.7.4

Re: [PATCH] IPMI: reserve memio regions separately

[Mark Salter]

On Tue, 2016-04-26 at 23:31 -0500, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@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 <msalter@redhat.com>
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> ---

This works for me as well.

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

>  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;

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, minyard@acm.org wrote:
>> From: Corey Minyard <cminyard@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 <msalter@redhat.com>
>>
>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> ---
> This works for me as well.
>
> Tested-by: Mark Salter <msalter@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;

Re: [PATCH] IPMI: reserve memio regions separately

[Corey Minyard]

On 04/25/2016 11:08 AM, Mark Salter wrote:
> 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:

Sigh.  I guess it was bound to happen.

>   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.
>
> This patch creates helper functions for region reservation so that
> mem/port setup/cleanup ops can share the code which requests and
> releases the regions separately.
>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> ---
>   drivers/char/ipmi/ipmi_si_intf.c | 74 +++++++++++++++++++++++++++++-----------
>   1 file changed, 54 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 7fddd86..0149397 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -1476,6 +1476,51 @@ static int std_irq_setup(struct smi_info *info)
>   	return rv;
>   }

I've been debating far too long about this.  You end up with less code 
if you just modify the memory function to add the loop like was in the 
port function.  All the reuse you really get is the loop on the index.  
I'm not sure there's a lot of value in splitting this out.  It kind of 
defeats the purpose of the function variables, sort of.

I'll send you a patch I did in a separate email.  It ends up adding 14 
lines instead of 34 lines.

>   
> +static void region_cleanup(struct smi_info *info, int num, bool is_memio)
> +{

I really don't like passing in the is_memio.  It's hard to tell what 
true and false mean when you pass them in like that.  If we go this way, 
just use info->io.addr_type so you don't have to pass anything in.


-corey
> +	unsigned long addr = info->io.addr_data;
> +	int idx;
> +
> +	for (idx = 0; idx < num; idx++) {
> +		if (is_memio)
> +			release_mem_region(addr + idx * info->io.regspacing,
> +					   info->io.regsize);
> +		else
> +			release_region(addr + idx * info->io.regspacing,
> +				       info->io.regsize);
> +	}
> +}
> +
> +static int region_setup(struct smi_info *info, bool is_memio)
> +{
> +	unsigned long addr = info->io.addr_data;
> +	struct resource *res;
> +	int idx, offset;
> +
> +	/*
> +	 * Some BIOSes reserve disjoint I/O regions in their ACPI
> +	 * tables.  This causes problems when trying to register the
> +	 * entire I/O region.  Therefore we must register each I/O
> +	 * region separately.
> +	 */
> +	for (idx = 0; idx < info->io_size; idx++) {
> +		offset = idx * info->io.regspacing;
> +		if (is_memio)
> +			res = request_mem_region(addr + offset,
> +						 info->io.regsize, DEVICE_NAME);
> +		else
> +			res = request_region(addr + offset,
> +					     info->io.regsize, DEVICE_NAME);
> +
> +		if (res == NULL) {
> +			/* Undo allocations */
> +			region_cleanup(info, idx, is_memio);
> +			return -EIO;
> +		}
> +	}
> +	return 0;
> +}
> +
>   static unsigned char port_inb(const struct si_sm_io *io, unsigned int offset)
>   {
>   	unsigned int addr = io->addr_data;
> @@ -1523,14 +1568,8 @@ static void port_outl(const struct si_sm_io *io, unsigned int offset,
>   
>   static void port_cleanup(struct smi_info *info)
>   {
> -	unsigned int addr = info->io.addr_data;
> -	int          idx;
> -
> -	if (addr) {
> -		for (idx = 0; idx < info->io_size; idx++)
> -			release_region(addr + idx * info->io.regspacing,
> -				       info->io.regsize);
> -	}
> +	if (info->io.addr_data)
> +		region_cleanup(info, info->io_size, false);
>   }
>   
>   static int port_setup(struct smi_info *info)
> @@ -1640,23 +1679,17 @@ static void mem_outq(const struct si_sm_io *io, unsigned int offset,
>   
>   static void mem_cleanup(struct smi_info *info)
>   {
> -	unsigned long addr = info->io.addr_data;
> -	int           mapsize;
> -
>   	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);
> +		region_cleanup(info, info->io_size, true);
>   	}
>   }
>   
>   static int mem_setup(struct smi_info *info)
>   {
>   	unsigned long addr = info->io.addr_data;
> -	int           mapsize;
> +	int           mapsize, err;
>   
>   	if (!addr)
>   		return -ENODEV;
> @@ -1692,6 +1725,10 @@ static int mem_setup(struct smi_info *info)
>   		return -EINVAL;
>   	}
>   
> +	err = region_setup(info, true);
> +	if (err)
> +		return err;
> +
>   	/*
>   	 * Calculate the total amount of memory to claim.  This is an
>   	 * unusual looking calculation, but it avoids claiming any
> @@ -1702,12 +1739,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);
> +		region_cleanup(info, info->io_size, true);
>   		return -EIO;
>   	}
>   	return 0;