Re: [PATCH] IPMI: reserve memio regions separately

[Corey Minyard <minyard@acm.org>]

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;