Re: [PATCH] IPMI: reserve memio regions separately

[Mark Salter]

On Tue, 2016-04-26 at 23:31 -0500, minyard@xxxxxxx wrote:
> From: Corey Minyard <cminyard@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> 
> Signed-off-by: Corey Minyard <cminyard@xxxxxxxxxx>
> ---

This works for me as well.

Tested-by: Mark Salter <msalter@xxxxxxxxxx>

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