Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1450673 > unrolled thread

[PATCH 0/4] Fixing a set of bugs for ioapic hotplug

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] Fixing a set of bugs for ioapic hotplug Rui Wang <rui.y.wang@intel.com> - 2016-07-26 18:40 +0200
    [PATCH 2/4] x86/ioapic: Fix setup_res() failing to get resource Rui Wang <rui.y.wang@intel.com> - 2016-07-26 18:40 +0200
      Re: [PATCH 2/4] x86/ioapic: Fix setup_res() failing to get resource Bjorn Helgaas <helgaas@kernel.org> - 2016-07-26 22:30 +0200
    [PATCH 3/4] x86/ioapic: Fix lost ioapic resource after hot-removal and hotadd Rui Wang <rui.y.wang@intel.com> - 2016-07-26 18:40 +0200
      Re: [PATCH 3/4] x86/ioapic: Fix lost ioapic resource after  hot-removal and hotadd kbuild test robot <lkp@intel.com> - 2016-07-26 19:00 +0200
      Re: [PATCH 3/4] x86/ioapic: Fix lost ioapic resource after  hot-removal and hotadd Bjorn Helgaas <helgaas@kernel.org> - 2016-07-26 22:30 +0200
        Re: [PATCH 3/4] x86/ioapic: Fix lost ioapic resource after hot-removal and hotadd Rui Wang <rui.y.wang@intel.com> - 2016-07-27 05:00 +0200

#1450673 — [PATCH 0/4] Fixing a set of bugs for ioapic hotplug

Hi all,

The 1st patch has been discussed before Bjorn went on vacation. I've fixed
all the issues. Bjorn, please advise how we'll move forward.

The remaining patches fix newly found bugs while testing ioapic hotplug.

Regards,
Rui

Rui Wang (4):
  x86/ioapic: Support hot-removal of IOAPICs present during boot
  x86/ioapic: Fix setup_res() failing to get resource
  x86/ioapic: Fix lost ioapic resource after hot-removal and hotadd
  x86/ioapic: Fix ioapic failing to request resource

 drivers/acpi/internal.h |  2 --
 drivers/acpi/ioapic.c   | 47 +++++++++++++++++++++++++++--------------------
 drivers/acpi/pci_root.c | 13 ++++++++++++-
 drivers/pci/setup-bus.c |  5 ++++-
 include/linux/acpi.h    |  6 ++++++
 5 files changed, 49 insertions(+), 24 deletions(-)

-- 
1.8.3.1

[toc] | [next] | [standalone]

#1450675 — [PATCH 2/4] x86/ioapic: Fix setup_res() failing to get resource

setup_res() doesn't actually get any resoure because it mistakenly
checks the return value of acpi_dev_filter_resource_type(), which
returns 0 on success, and 1 on failure. Fix it by taking the return
value of non-zero as failing to match the specified resource type.

Signed-off-by: Rui Wang <rui.y.wang@intel.com>
---
 drivers/acpi/ioapic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/ioapic.c b/drivers/acpi/ioapic.c
index 0f272e2..daf4a40 100644
--- a/drivers/acpi/ioapic.c
+++ b/drivers/acpi/ioapic.c
@@ -46,7 +46,7 @@ static acpi_status setup_res(struct acpi_resource *acpi_res, void *data)
 	struct resource_win win;
 
 	res->flags = 0;
-	if (acpi_dev_filter_resource_type(acpi_res, IORESOURCE_MEM) == 0)
+	if (acpi_dev_filter_resource_type(acpi_res, IORESOURCE_MEM))
 		return AE_OK;
 
 	if (!acpi_dev_resource_memory(acpi_res, res)) {
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1450854 — Re: [PATCH 2/4] x86/ioapic: Fix setup_res() failing to get resource

On Wed, Jul 27, 2016 at 12:13:15AM +0800, Rui Wang wrote:
> setup_res() doesn't actually get any resoure because it mistakenly

s/resoure/resource/

> checks the return value of acpi_dev_filter_resource_type(), which
> returns 0 on success, and 1 on failure. Fix it by taking the return
> value of non-zero as failing to match the specified resource type.
> 
> Signed-off-by: Rui Wang <rui.y.wang@intel.com>
> ---
>  drivers/acpi/ioapic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/ioapic.c b/drivers/acpi/ioapic.c
> index 0f272e2..daf4a40 100644
> --- a/drivers/acpi/ioapic.c
> +++ b/drivers/acpi/ioapic.c
> @@ -46,7 +46,7 @@ static acpi_status setup_res(struct acpi_resource *acpi_res, void *data)
>  	struct resource_win win;
>  
>  	res->flags = 0;
> -	if (acpi_dev_filter_resource_type(acpi_res, IORESOURCE_MEM) == 0)
> +	if (acpi_dev_filter_resource_type(acpi_res, IORESOURCE_MEM))
>  		return AE_OK;
>  
>  	if (!acpi_dev_resource_memory(acpi_res, res)) {
> -- 
> 1.8.3.1
> 

[toc] | [prev] | [next] | [standalone]

#1450676 — [PATCH 3/4] x86/ioapic: Fix lost ioapic resource after hot-removal and hotadd

ioapic resource at 0xfecxxxxx gets lost from /proc/iomem after
hot-removing and then hot-adding the ioapic devices.

After system boot, in /proc/iomem:
fec00000-fecfffff : PNP0003:00
  fec00000-fec003ff : IOAPIC 0
  fec01000-fec013ff : IOAPIC 1
  fec40000-fec403ff : IOAPIC 2
  fec80000-fec803ff : IOAPIC 3
  fecc0000-fecc03ff : IOAPIC 4

Then hot-remove IOAPIC 2 and hot-add it again:
fec00000-fecfffff : PNP0003:00
  fec00000-fec003ff : IOAPIC 0
  fec01000-fec013ff : IOAPIC 1
  fec80000-fec803ff : IOAPIC 3
  fecc0000-fecc03ff : IOAPIC 4

The range at 0xfec40000 is lost from /proc/iomem. It is because
handle_ioapic_add() requests resource from either PCI config BAR or
acpi _CRS, not both. But Intel platforms map the IOxAPIC registers
at both the PCI config BAR (called MBAR) and the 0xfecX_YZ00 to
0xfecX_Y2FF range (called ABAR). Both of the ranges should be claimed
from /proc/iomem for exclusive use.

Signed-off-by: Rui Wang <rui.y.wang@intel.com>
---
 drivers/acpi/ioapic.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/acpi/ioapic.c b/drivers/acpi/ioapic.c
index daf4a40..80b0b1a 100644
--- a/drivers/acpi/ioapic.c
+++ b/drivers/acpi/ioapic.c
@@ -97,7 +97,7 @@ static acpi_status handle_ioapic_add(acpi_handle handle, u32 lvl,
 	unsigned long long gsi_base;
 	struct acpi_pci_ioapic *ioapic;
 	struct pci_dev *dev = NULL;
-	struct resource *res = NULL;
+	struct resource *res = NULL, *pci_res, *crs_res;
 	char *type = NULL;
 
 	if (!acpi_is_ioapic(handle, &type))
@@ -137,23 +137,28 @@ static acpi_status handle_ioapic_add(acpi_handle handle, u32 lvl,
 		pci_set_master(dev);
 		if (pci_request_region(dev, 0, type))
 			goto exit_disable;
-		res = &dev->resource[0];
+		pci_res = &dev->resource[0];
 		ioapic->pdev = dev;
 	} else {
 		pci_dev_put(dev);
 		dev = NULL;
+	}
 
-		res = &ioapic->res;
-		acpi_walk_resources(handle, METHOD_NAME__CRS, setup_res, res);
-		if (res->flags == 0) {
-			acpi_handle_warn(handle, "failed to get resource\n");
-			goto exit_free;
-		} else if (request_resource(&iomem_resource, res)) {
-			acpi_handle_warn(handle, "failed to insert resource\n");
-			goto exit_free;
-		}
+	crs_res = &ioapic->res;
+	acpi_walk_resources(handle, METHOD_NAME__CRS, setup_res, crs_res);
+	if (crs_res->flags == 0) {
+		acpi_handle_warn(handle, "failed to get resource\n");
+		goto exit_release;
+	} else if (request_resource(&iomem_resource, crs_res)) {
+		acpi_handle_warn(handle, "failed to insert resource\n");
+		goto exit_release;
 	}
 
+	/* try pci resource first, then "_CRS" resource */
+	res = pci_res;
+	if (!res || !res->flags)
+		res = crs_res;
+
 	if (acpi_register_ioapic(handle, res->start, (u32)gsi_base)) {
 		acpi_handle_warn(handle, "failed to register IOAPIC\n");
 		goto exit_release;
@@ -174,14 +179,13 @@ done:
 exit_release:
 	if (dev)
 		pci_release_region(dev, 0);
-	else
-		release_resource(res);
+	if (ioapic->res.flags && ioapic->res.parent)
+		release_resource(&ioapic->res);
 exit_disable:
 	if (dev)
 		pci_disable_device(dev);
 exit_put:
 	pci_dev_put(dev);
-exit_free:
 	kfree(ioapic);
 exit:
 	mutex_unlock(&ioapic_list_lock);
@@ -218,9 +222,9 @@ int acpi_ioapic_remove(struct acpi_pci_root *root)
 			pci_release_region(ioapic->pdev, 0);
 			pci_disable_device(ioapic->pdev);
 			pci_dev_put(ioapic->pdev);
-		} else if (ioapic->res.flags && ioapic->res.parent) {
-			release_resource(&ioapic->res);
 		}
+		if (ioapic->res.flags && ioapic->res.parent)
+			release_resource(&ioapic->res);
 		list_del(&ioapic->list);
 		kfree(ioapic);
 	}
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1450689 — Re: [PATCH 3/4] x86/ioapic: Fix lost ioapic resource after hot-removal and hotadd

[Multipart message — attachments visible in raw view] — view raw

Hi,

[auto build test WARNING on pm/linux-next]
[also build test WARNING on v4.7 next-20160726]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Rui-Wang/Fixing-a-set-of-bugs-for-ioapic-hotplug/20160727-003628
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
config: x86_64-randconfig-x015-201630 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/acpi/ioapic.c: In function 'handle_ioapic_add':
>> drivers/acpi/ioapic.c:159:18: warning: 'pci_res' may be used uninitialized in this function [-Wmaybe-uninitialized]
     if (!res || !res->flags)
                  ~~~^~~~~~~

vim +/pci_res +159 drivers/acpi/ioapic.c

   143			pci_dev_put(dev);
   144			dev = NULL;
   145		}
   146	
   147		crs_res = &ioapic->res;
   148		acpi_walk_resources(handle, METHOD_NAME__CRS, setup_res, crs_res);
   149		if (crs_res->flags == 0) {
   150			acpi_handle_warn(handle, "failed to get resource\n");
   151			goto exit_release;
   152		} else if (request_resource(&iomem_resource, crs_res)) {
   153			acpi_handle_warn(handle, "failed to insert resource\n");
   154			goto exit_release;
   155		}
   156	
   157		/* try pci resource first, then "_CRS" resource */
   158		res = pci_res;
 > 159		if (!res || !res->flags)
   160			res = crs_res;
   161	
   162		if (acpi_register_ioapic(handle, res->start, (u32)gsi_base)) {
   163			acpi_handle_warn(handle, "failed to register IOAPIC\n");
   164			goto exit_release;
   165		}
   166	done:
   167		list_add(&ioapic->list, &ioapic_list);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1450856 — Re: [PATCH 3/4] x86/ioapic: Fix lost ioapic resource after hot-removal and hotadd

On Wed, Jul 27, 2016 at 12:13:16AM +0800, Rui Wang wrote:
> ioapic resource at 0xfecxxxxx gets lost from /proc/iomem after
> hot-removing and then hot-adding the ioapic devices.
> 
> After system boot, in /proc/iomem:
> fec00000-fecfffff : PNP0003:00
>   fec00000-fec003ff : IOAPIC 0
>   fec01000-fec013ff : IOAPIC 1
>   fec40000-fec403ff : IOAPIC 2
>   fec80000-fec803ff : IOAPIC 3
>   fecc0000-fecc03ff : IOAPIC 4
> 
> Then hot-remove IOAPIC 2 and hot-add it again:
> fec00000-fecfffff : PNP0003:00
>   fec00000-fec003ff : IOAPIC 0
>   fec01000-fec013ff : IOAPIC 1
>   fec80000-fec803ff : IOAPIC 3
>   fecc0000-fecc03ff : IOAPIC 4
> 
> The range at 0xfec40000 is lost from /proc/iomem. It is because
> handle_ioapic_add() requests resource from either PCI config BAR or
> acpi _CRS, not both. But Intel platforms map the IOxAPIC registers

s/acpi/ACPI/
s/ioapic/IOAPIC/ throughout

> at both the PCI config BAR (called MBAR) and the 0xfecX_YZ00 to
> 0xfecX_Y2FF range (called ABAR). Both of the ranges should be claimed

I guess you mean the 0xfecX_YZ00-0xfecX_Y2FF range appears in _CRS?

> from /proc/iomem for exclusive use.
> 
> Signed-off-by: Rui Wang <rui.y.wang@intel.com>
> ---
>  drivers/acpi/ioapic.c | 36 ++++++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/acpi/ioapic.c b/drivers/acpi/ioapic.c
> index daf4a40..80b0b1a 100644
> --- a/drivers/acpi/ioapic.c
> +++ b/drivers/acpi/ioapic.c
> @@ -97,7 +97,7 @@ static acpi_status handle_ioapic_add(acpi_handle handle, u32 lvl,
>  	unsigned long long gsi_base;
>  	struct acpi_pci_ioapic *ioapic;
>  	struct pci_dev *dev = NULL;
> -	struct resource *res = NULL;
> +	struct resource *res = NULL, *pci_res, *crs_res;
>  	char *type = NULL;
>  
>  	if (!acpi_is_ioapic(handle, &type))
> @@ -137,23 +137,28 @@ static acpi_status handle_ioapic_add(acpi_handle handle, u32 lvl,
>  		pci_set_master(dev);
>  		if (pci_request_region(dev, 0, type))
>  			goto exit_disable;
> -		res = &dev->resource[0];
> +		pci_res = &dev->resource[0];
>  		ioapic->pdev = dev;
>  	} else {
>  		pci_dev_put(dev);
>  		dev = NULL;
> +	}
>  
> -		res = &ioapic->res;
> -		acpi_walk_resources(handle, METHOD_NAME__CRS, setup_res, res);
> -		if (res->flags == 0) {
> -			acpi_handle_warn(handle, "failed to get resource\n");
> -			goto exit_free;
> -		} else if (request_resource(&iomem_resource, res)) {
> -			acpi_handle_warn(handle, "failed to insert resource\n");
> -			goto exit_free;
> -		}
> +	crs_res = &ioapic->res;
> +	acpi_walk_resources(handle, METHOD_NAME__CRS, setup_res, crs_res);
> +	if (crs_res->flags == 0) {
> +		acpi_handle_warn(handle, "failed to get resource\n");
> +		goto exit_release;
> +	} else if (request_resource(&iomem_resource, crs_res)) {
> +		acpi_handle_warn(handle, "failed to insert resource\n");
> +		goto exit_release;
>  	}
>  
> +	/* try pci resource first, then "_CRS" resource */
> +	res = pci_res;
> +	if (!res || !res->flags)
> +		res = crs_res;
> +
>  	if (acpi_register_ioapic(handle, res->start, (u32)gsi_base)) {
>  		acpi_handle_warn(handle, "failed to register IOAPIC\n");
>  		goto exit_release;
> @@ -174,14 +179,13 @@ done:
>  exit_release:
>  	if (dev)
>  		pci_release_region(dev, 0);
> -	else
> -		release_resource(res);
> +	if (ioapic->res.flags && ioapic->res.parent)
> +		release_resource(&ioapic->res);
>  exit_disable:
>  	if (dev)
>  		pci_disable_device(dev);
>  exit_put:
>  	pci_dev_put(dev);
> -exit_free:
>  	kfree(ioapic);
>  exit:
>  	mutex_unlock(&ioapic_list_lock);
> @@ -218,9 +222,9 @@ int acpi_ioapic_remove(struct acpi_pci_root *root)
>  			pci_release_region(ioapic->pdev, 0);
>  			pci_disable_device(ioapic->pdev);
>  			pci_dev_put(ioapic->pdev);
> -		} else if (ioapic->res.flags && ioapic->res.parent) {
> -			release_resource(&ioapic->res);
>  		}
> +		if (ioapic->res.flags && ioapic->res.parent)
> +			release_resource(&ioapic->res);
>  		list_del(&ioapic->list);
>  		kfree(ioapic);
>  	}
> -- 
> 1.8.3.1
> 

[toc] | [prev] | [next] | [standalone]

#1451012 — Re: [PATCH 3/4] x86/ioapic: Fix lost ioapic resource after hot-removal and hotadd

On Wed, July 27, 2016 4:24 AM, Bjorn Helgaas wrote:
> On Wed, Jul 27, 2016 at 12:13:16AM +0800, Rui Wang wrote:
> > ioapic resource at 0xfecxxxxx gets lost from /proc/iomem after
> > hot-removing and then hot-adding the ioapic devices.
> >
> > After system boot, in /proc/iomem:
> > fec00000-fecfffff : PNP0003:00
> >   fec00000-fec003ff : IOAPIC 0
> >   fec01000-fec013ff : IOAPIC 1
> >   fec40000-fec403ff : IOAPIC 2
> >   fec80000-fec803ff : IOAPIC 3
> >   fecc0000-fecc03ff : IOAPIC 4
> >
> > Then hot-remove IOAPIC 2 and hot-add it again:
> > fec00000-fecfffff : PNP0003:00
> >   fec00000-fec003ff : IOAPIC 0
> >   fec01000-fec013ff : IOAPIC 1
> >   fec80000-fec803ff : IOAPIC 3
> >   fecc0000-fecc03ff : IOAPIC 4
> >
> > The range at 0xfec40000 is lost from /proc/iomem. It is because
> > handle_ioapic_add() requests resource from either PCI config BAR or
> > acpi _CRS, not both. But Intel platforms map the IOxAPIC registers
> 
> s/acpi/ACPI/
> s/ioapic/IOAPIC/ throughout
> 
> > at both the PCI config BAR (called MBAR) and the 0xfecX_YZ00 to
> > 0xfecX_Y2FF range (called ABAR). Both of the ranges should be claimed
> 
> I guess you mean the 0xfecX_YZ00-0xfecX_Y2FF range appears in _CRS?

Yes. That range appears in _CRS for each IOAPIC. I'll make it cleaner in
the commit message.

Thanks
Rui

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web