Re: [PATCH 1/5 v3] Extend the request_region() infrastructure

Re: [PATCH 1/5 v3] Extend the request_region() infrastructure

[Guenter Roeck]

On Mon, Dec 18, 2017 at 09:48:37AM +0100, Zoltan Boszormenyi wrote:
> From: Böszörményi Zoltán <zboszor@pr.hu>
> 
> Add a new IORESOURCE_ALLOCATED flag that is automatically used
> when alloc_resource() is used internally in kernel/resource.c
> and free_resource() now takes this flag into account.
> 
> The core of __request_region() was factored out into a new function
> called __request_declared_region() that needs struct resource *
> instead of the (start, n, name) triplet.
> 
> These changes allow using statically declared struct resource
> data coupled with the pre-existing DEFINE_RES_IO_NAMED() static
> initializer macro. The new macro exploiting
> __request_declared_region() is request_declared_muxed_region()
> 
> v2:
> 
> Fixed checkpatch.pl warnings and errors and extended the macro
> API with request_declared_region() and release_declared_region()
> 
> Reversed the order of __request_declared_region and __request_region
> 
> Added high level description of the muxed and declared variants
> of the macros.
> 
> v3:
> 
> Rebased for 4.15-rc4
> 
> Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
> ---
>  include/linux/ioport.h | 14 ++++++++++++++
>  kernel/resource.c      | 40 +++++++++++++++++++++++++++++++++++++---
>  2 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 93b4183cf53d..d198d6a58574 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -53,6 +53,7 @@ struct resource {
>  #define IORESOURCE_MEM_64	0x00100000
>  #define IORESOURCE_WINDOW	0x00200000	/* forwarded by bridge */
>  #define IORESOURCE_MUXED	0x00400000	/* Resource is software muxed */
> +#define IORESOURCE_ALLOCATED	0x00800000	/* Resource was allocated */
>  
>  #define IORESOURCE_EXT_TYPE_BITS 0x01000000	/* Resource extended types */
>  #define IORESOURCE_SYSRAM	0x01000000	/* System RAM (modifier) */
> @@ -218,7 +219,14 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2)
>  
>  /* Convenience shorthand with allocation */
>  #define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
> +#define request_declared_region(res)		__request_region( \
> +							&ioport_resource, \
> +							(res), 0)
>  #define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
> +#define request_declared_muxed_region(res)	__request_declared_region( \
> +							&ioport_resource, \
> +							(res), \
> +							IORESOURCE_MUXED)
>  #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
>  #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
>  #define request_mem_region_exclusive(start,n,name) \
> @@ -230,8 +238,14 @@ extern struct resource * __request_region(struct resource *,
>  					resource_size_t n,
>  					const char *name, int flags);
>  
> +extern struct resource *__request_declared_region(struct resource *parent,
> +					struct resource *res, int flags);
> +
>  /* Compatibility cruft */
>  #define release_region(start,n)	__release_region(&ioport_resource, (start), (n))
> +#define release_declared_region(res)	__release_region(&ioport_resource, \
> +						(res)->start, \
> +						(res)->end - (res)->start + 1)
>  #define release_mem_region(start,n)	__release_region(&iomem_resource, (start), (n))
>  
>  extern void __release_region(struct resource *, resource_size_t,
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 54ba6de3757c..05db9c4e3992 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -184,6 +184,9 @@ static void free_resource(struct resource *res)
>  	if (!res)
>  		return;
>  
> +	if (!(res->flags & IORESOURCE_ALLOCATED))
> +		return;
> +
>  	if (!PageSlab(virt_to_head_page(res))) {
>  		spin_lock(&bootmem_resource_lock);
>  		res->sibling = bootmem_resource_free;
> @@ -210,6 +213,8 @@ static struct resource *alloc_resource(gfp_t flags)
>  	else
>  		res = kzalloc(sizeof(struct resource), flags);
>  
> +	res->flags = IORESOURCE_ALLOCATED;
> +

kzalloc() can fail, thus res can be NULL.

>  	return res;
>  }
>  
> @@ -1128,8 +1133,19 @@ resource_size_t resource_alignment(struct resource *res)
>   * the IO flag meanings (busy etc).
>   *
>   * request_region creates a new busy region.
> + * The resource descriptor is allocated by this function.
> + *
> + * request_declared_region creates a new busy region
> + * described in an existing resource descriptor.
> + *
> + * request_muxed_region creates a new shared busy region.
> + * The resource descriptor is allocated by this function.
> + *
> + * request_declared_muxed_region creates a new shared busy region
> + * described in an existing resource descriptor.
>   *
>   * release_region releases a matching busy region.
> + * The region is only freed if it was allocated.
>   */
>  
>  static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait);
> @@ -1146,7 +1162,6 @@ struct resource * __request_region(struct resource *parent,
>  				   resource_size_t start, resource_size_t n,
>  				   const char *name, int flags)
>  {
> -	DECLARE_WAITQUEUE(wait, current);
>  	struct resource *res = alloc_resource(GFP_KERNEL);
>  
>  	if (!res)
> @@ -1156,6 +1171,26 @@ struct resource * __request_region(struct resource *parent,
>  	res->start = start;
>  	res->end = start + n - 1;
>  
> +	if (!__request_declared_region(parent, res, flags)) {
> +		free_resource(res);
> +		res = NULL;
> +	}
> +
> +	return res;
> +}
> +EXPORT_SYMBOL(__request_region);
> +
> +/**
> + * __request_declared_region - create a new busy resource region
> + * @parent: parent resource descriptor
> + * @res: child resource descriptor
> + * @flags: IO resource flags
> + */
> +struct resource *__request_declared_region(struct resource *parent,
> +				   struct resource *res, int flags)
> +{
> +	DECLARE_WAITQUEUE(wait, current);
> +
>  	write_lock(&resource_lock);
>  
>  	for (;;) {
> @@ -1184,14 +1219,13 @@ struct resource * __request_region(struct resource *parent,
>  			continue;
>  		}
>  		/* Uhhuh, that didn't work out.. */
> -		free_resource(res);
>  		res = NULL;
>  		break;
>  	}
>  	write_unlock(&resource_lock);
>  	return res;
>  }
> -EXPORT_SYMBOL(__request_region);
> +EXPORT_SYMBOL(__request_declared_region);
>  
>  /**
>   * __release_region - release a previously reserved resource region
> -- 
> 2.13.6
> 

Re: [PATCH 2/5 v3] Modify behaviour of request_*muxed_region()

[Guenter Roeck]

On Mon, Dec 18, 2017 at 09:48:38AM +0100, Zoltan Boszormenyi wrote:
> From: Böszörményi Zoltán <zboszor@pr.hu>
> 
> In order to make request_*muxed_region() behave more like
> mutex_lock(), a possible failure case needs to be eliminated.
> When drivers do not properly share the same I/O region, e.g.
> one is using request_region() and the other is using
> request_muxed_region(), the kernel didn't warn the user about it.
> This change modifies IORESOURCE_MUXED behaviour so it always
> goes to sleep waiting for the resuorce to be freed and the

resource

> inconsistent resource flag usage is logged with KERN_ERR.
> 
> v2: Fixed checkpatch.pl warnings and extended the comment
>     about request_declared_muxed_region.
> 
> v3: Rebased for 4.15-rc4
> 
> Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
> ---
>  kernel/resource.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 05db9c4e3992..0f26f887ac33 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -1143,6 +1143,7 @@ resource_size_t resource_alignment(struct resource *res)
>   *
>   * request_declared_muxed_region creates a new shared busy region
>   * described in an existing resource descriptor.
> + * It only returns if it succeeded.
>   *
>   * release_region releases a matching busy region.
>   * The region is only freed if it was allocated.
> @@ -1209,7 +1210,10 @@ struct resource *__request_declared_region(struct resource *parent,
>  				continue;
>  			}
>  		}
> -		if (conflict->flags & flags & IORESOURCE_MUXED) {
> +		if (flags & IORESOURCE_MUXED) {
> +			if (!(conflict->flags & IORESOURCE_MUXED))
> +				pr_err("Resource conflict between muxed \"%s\" and non-muxed \"%s\" I/O regions!\n",
> +					res->name, conflict->name);

With this, the muxed resource requestor will hang since the non-muxed
requestor will not release the resource. I understand that you are trying
to get rid of the error return, but is replacing it with a hang really
better ?

>  			add_wait_queue(&muxed_resource_wait, &wait);
>  			write_unlock(&resource_lock);
>  			set_current_state(TASK_UNINTERRUPTIBLE);
> -- 
> 2.13.6
> 

Re: [PATCH 2/5 v3] Modify behaviour of request_*muxed_region()

[Guenter Roeck]

On Tue, Dec 19, 2017 at 07:11:11AM +0100, Boszormenyi Zoltan wrote:
> 2017-12-18 20:07 keltezéssel, Guenter Roeck írta:
> >On Mon, Dec 18, 2017 at 09:48:38AM +0100, Zoltan Boszormenyi wrote:
> >>From: Böszörményi Zoltán <zboszor@pr.hu>
> >>
> >>In order to make request_*muxed_region() behave more like
> >>mutex_lock(), a possible failure case needs to be eliminated.
> >>When drivers do not properly share the same I/O region, e.g.
> >>one is using request_region() and the other is using
> >>request_muxed_region(), the kernel didn't warn the user about it.
> >>This change modifies IORESOURCE_MUXED behaviour so it always
> >>goes to sleep waiting for the resuorce to be freed and the
> >
> >resource
> >
> >>inconsistent resource flag usage is logged with KERN_ERR.
> >>
> >>v2: Fixed checkpatch.pl warnings and extended the comment
> >>     about request_declared_muxed_region.
> >>
> >>v3: Rebased for 4.15-rc4
> >>
> >>Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
> >>---
> >>  kernel/resource.c | 6 +++++-
> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/kernel/resource.c b/kernel/resource.c
> >>index 05db9c4e3992..0f26f887ac33 100644
> >>--- a/kernel/resource.c
> >>+++ b/kernel/resource.c
> >>@@ -1143,6 +1143,7 @@ resource_size_t resource_alignment(struct resource *res)
> >>   *
> >>   * request_declared_muxed_region creates a new shared busy region
> >>   * described in an existing resource descriptor.
> >>+ * It only returns if it succeeded.
> >>   *
> >>   * release_region releases a matching busy region.
> >>   * The region is only freed if it was allocated.
> >>@@ -1209,7 +1210,10 @@ struct resource *__request_declared_region(struct resource *parent,
> >>  				continue;
> >>  			}
> >>  		}
> >>-		if (conflict->flags & flags & IORESOURCE_MUXED) {
> >>+		if (flags & IORESOURCE_MUXED) {
> >>+			if (!(conflict->flags & IORESOURCE_MUXED))
> >>+				pr_err("Resource conflict between muxed \"%s\" and non-muxed \"%s\" I/O regions!\n",
> >>+					res->name, conflict->name);
> >
> >With this, the muxed resource requestor will hang since the non-muxed
> >requestor will not release the resource. I understand that you are trying
> >to get rid of the error return, but is replacing it with a hang really
> >better ?
> 
> I think it's a definite yes.
> 

I disagree. Some systems are configured to crash on stalls, which is
always worse than failure to load a driver.

I think the real problem is that usb_amd_quirk_pll() does not return
an error. It should, usb_amd_quirk_pll_disable() as well as
usb_amd_quirk_pll_enable() should pass the error to the calling code,
which should handle it. However, there is an even simpler solution -
see below.

> Consider the case of the regression this series is trying to fix.
> 
> Two drivers were trying to access a shared resource, both thinking
> that it's the only one, which was almost true[1] initially.
> An improvement in the i2c-piix4 driver in 4.4-rc3 broke sp5100_tco.
> sp5100_tco was loaded second and failed outright.
> 
> This didn't make anyone to fix the situation, despite the fact
> that there are at least three bugtrackers (kernel, Debian and Red Hat)
> that point out the same problem.
> 
> So, a failing driver initialization is not horrifying enough to get
> something fixed. Maybe a hang is.
> 
> Maybe this behaviour is still too weak, instead of doing it for an
> inconsistent muxed/non-muxed case, it (at least the logging) should
> be done for every case, to detect drivers that try to lock the same
> resource.
> 
> In the case of an inconsistent resource flags usage, the bug is
> not the hang itself, it's the inconsistent resource flags.
> 
Separate problem from the one you are trying to solve. One could argue
that the inconsistent resource use should dump a warning with traceback.
Sure, that would also cause the systems I referred to above to crash,
but it would do so with an actionable traceback, not with an obscure
hang which requires detailed analysis.

> [1] The USB PCI quirks were accessing the same I/O resource without
> locking and I found that on a Kabini machine, we got rare
> "disabled by hub (EMI?), re-enabling..." reports that doesn't occur
> with the locking applied. It is possible that two kernel threads were
> doing I/O at the same time without synchronization.
> 

The usb quirks code only accesses the registers in question to obtain
another set of index registers. It _could_ do that in its initialization
code instead and get the address through amd_chipset_info when needed.
If done corectly, this could actually simplify the related code in
usb_amd_quirk_pll(), since the operation on the address obtained
is the same for all supported chipsets.

Overall, I don't see a need for this API change. There are at least
two other solutions available to handle the usb quirks issue, one of
which is substantially less invasive. The problems in the other drivers
can be solved by using request_muxed_region().

Guenter

Re: [PATCH 4/5 v5] i2c: i2c-piix4: Use request_declared_muxed_region()

[Guenter Roeck]

On 12/18/2017 12:48 AM, Zoltan Boszormenyi wrote:
> From: Böszörményi Zoltán <zboszor@pr.hu>
> 
> Use the new request_declared_muxed_region() macro to
> synchronize access to the I/O port pair 0xcd6 / 0xcd7.
> 
> At the same time, remove the long lifetime request_region()
> call to reserve these I/O ports, so the sp5100_tco watchdog
> driver can also load.
> 
> This fixes an old regression in Linux 4.4-rc4, caused by
> commit 2fee61d22e60 ("i2c: piix4: Add support for multiplexed
> main adapter in SB800")
> 
> v1: Started with a common mutex in a C source file.
> 
> v2: Referenced to common mutex from drivers/usb/host/pci-quirks.c
> 
> v3: Switched to using the new request_declared_muxed_region
>      macro.
> 
> v4: Fixed checkpatch.pl warnings and use the new
>      release_declared_region() macro.
> 
> v5: Rebased for 4.15-rc4
>      Use struct i2c_algorithm pointer directly instead of
>      a bool to select the proper algo.

This change makes sense, but it should be a separate patch.

Guenter

> 
> Signed-off-by: Zoltán Böszörményi <zboszor@pr.hu>
> ---
>   drivers/i2c/busses/i2c-piix4.c | 51 +++++++++++++++---------------------------
>   1 file changed, 18 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 462948e2c535..1846d9ea7d27 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -153,10 +153,11 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
>   
>   /*
>    * SB800 globals
> - * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
> + * request_declared_muxed_region() protects piix4_port_sel_sb800 and the pair
>    * of I/O ports at SB800_PIIX4_SMB_IDX.
>    */
> -static DEFINE_MUTEX(piix4_mutex_sb800);
> +static struct resource sb800_res = DEFINE_RES_IO_NAMED(SB800_PIIX4_SMB_IDX, 2,
> +						 "i2c-piix4");
>   static u8 piix4_port_sel_sb800;
>   static u8 piix4_port_mask_sb800;
>   static u8 piix4_port_shift_sb800;
> @@ -169,7 +170,6 @@ struct i2c_piix4_adapdata {
>   	unsigned short smba;
>   
>   	/* SB800 */
> -	bool sb800_main;
>   	bool notify_imc;
>   	u8 port;		/* Port number, shifted */
>   };
> @@ -298,12 +298,12 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>   	else
>   		smb_en = (aux) ? 0x28 : 0x2c;
>   
> -	mutex_lock(&piix4_mutex_sb800);
> +	request_declared_muxed_region(&sb800_res);
>   	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
>   	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
>   	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
>   	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
> -	mutex_unlock(&piix4_mutex_sb800);
> +	release_declared_region(&sb800_res);
>   
>   	if (!smb_en) {
>   		smb_en_status = smba_en_lo & 0x10;
> @@ -373,7 +373,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>   			break;
>   		}
>   	} else {
> -		mutex_lock(&piix4_mutex_sb800);
> +		request_declared_muxed_region(&sb800_res);
>   		outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
>   		port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
>   		piix4_port_sel_sb800 = (port_sel & 0x01) ?
> @@ -381,7 +381,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>   				       SB800_PIIX4_PORT_IDX;
>   		piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK;
>   		piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT;
> -		mutex_unlock(&piix4_mutex_sb800);
> +		release_declared_region(&sb800_res);
>   	}
>   
>   	dev_info(&PIIX4_dev->dev,
> @@ -679,7 +679,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>   	u8 port;
>   	int retval;
>   
> -	mutex_lock(&piix4_mutex_sb800);
> +	request_declared_muxed_region(&sb800_res);
>   
>   	/* Request the SMBUS semaphore, avoid conflicts with the IMC */
>   	smbslvcnt  = inb_p(SMBSLVCNT);
> @@ -695,7 +695,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>   	} while (--retries);
>   	/* SMBus is still owned by the IMC, we give up */
>   	if (!retries) {
> -		mutex_unlock(&piix4_mutex_sb800);
> +		release_declared_region(&sb800_res);
>   		return -EBUSY;
>   	}
>   
> @@ -753,7 +753,7 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>   	if ((size == I2C_SMBUS_BLOCK_DATA) && adapdata->notify_imc)
>   		piix4_imc_wakeup();
>   
> -	mutex_unlock(&piix4_mutex_sb800);
> +	release_declared_region(&sb800_res);
>   
>   	return retval;
>   }
> @@ -804,7 +804,8 @@ static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
>   static struct i2c_adapter *piix4_aux_adapter;
>   
>   static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
> -			     bool sb800_main, u8 port, bool notify_imc,
> +			     struct i2c_algorithm *algo,
> +			     u8 port, bool notify_imc,
>   			     const char *name, struct i2c_adapter **padap)
>   {
>   	struct i2c_adapter *adap;
> @@ -819,8 +820,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>   
>   	adap->owner = THIS_MODULE;
>   	adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> -	adap->algo = sb800_main ? &piix4_smbus_algorithm_sb800
> -				: &smbus_algorithm;
> +	adap->algo = algo;
>   
>   	adapdata = kzalloc(sizeof(*adapdata), GFP_KERNEL);
>   	if (adapdata == NULL) {
> @@ -830,7 +830,6 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>   	}
>   
>   	adapdata->smba = smba;
> -	adapdata->sb800_main = sb800_main;
>   	adapdata->port = port << piix4_port_shift_sb800;
>   	adapdata->notify_imc = notify_imc;
>   
> @@ -862,7 +861,9 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
>   	int retval;
>   
>   	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
> -		retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
> +		retval = piix4_add_adapter(dev, smba,
> +					   &piix4_smbus_algorithm_sb800,
> +					   port, notify_imc,
>   					   piix4_main_port_names_sb800[port],
>   					   &piix4_main_adapters[port]);
>   		if (retval < 0)
> @@ -899,13 +900,6 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>   		bool notify_imc = false;
>   		is_sb800 = true;
>   
> -		if (!request_region(SB800_PIIX4_SMB_IDX, 2, "smba_idx")) {
> -			dev_err(&dev->dev,
> -			"SMBus base address index region 0x%x already in use!\n",
> -			SB800_PIIX4_SMB_IDX);
> -			return -EBUSY;
> -		}
> -
>   		if (dev->vendor == PCI_VENDOR_ID_AMD &&
>   		    dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
>   			u8 imc;
> @@ -922,27 +916,24 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>   
>   		/* base address location etc changed in SB800 */
>   		retval = piix4_setup_sb800(dev, id, 0);
> -		if (retval < 0) {
> -			release_region(SB800_PIIX4_SMB_IDX, 2);
> +		if (retval < 0)
>   			return retval;
> -		}
>   
>   		/*
>   		 * Try to register multiplexed main SMBus adapter,
>   		 * give up if we can't
>   		 */
>   		retval = piix4_add_adapters_sb800(dev, retval, notify_imc);
> -		if (retval < 0) {
> -			release_region(SB800_PIIX4_SMB_IDX, 2);
> +		if (retval < 0)
>   			return retval;
> -		}
>   	} else {
>   		retval = piix4_setup(dev, id);
>   		if (retval < 0)
>   			return retval;
>   
>   		/* Try to register main SMBus adapter, give up if we can't */
> -		retval = piix4_add_adapter(dev, retval, false, 0, false, "",
> +		retval = piix4_add_adapter(dev, retval,
> +					   &smbus_algorithm, 0, false, "",
>   					   &piix4_main_adapters[0]);
>   		if (retval < 0)
>   			return retval;
> @@ -983,11 +974,8 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
>   
>   	if (adapdata->smba) {
>   		i2c_del_adapter(adap);
> -		if (adapdata->port == (0 << piix4_port_shift_sb800)) {
> +		if (adapdata->port == (0 << piix4_port_shift_sb800))
>   			release_region(adapdata->smba, SMBIOSIZE);
> -			if (adapdata->sb800_main)
> -				release_region(SB800_PIIX4_SMB_IDX, 2);
> -		}
>   		kfree(adapdata);
>   		kfree(adap);
>   	}
>