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

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;