[PATCH] [RESEND] perf/x86/intel/uncore: Fix the index of PCU.3 Broadwell CPUs

[PATCH] [RESEND] perf/x86/intel/uncore: Fix the index of PCU.3 Broadwell CPUs

[Masayoshi Mizuma]

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

commit 15a3e845b01c ("perf/x86/intel/uncore: Fix SBOX support for
Broadwell CPUs") introduced PCU.3 for Broadwell CPU. Unfortunately,
the driver_data of PCU.3 conflicts to QPI Port 2 filter.

    { /* QPI Port 2 filter  */
            PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f46),
            .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 2),

    { /* PCU.3 (for Capability registers) */
            PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6fc0),
            .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
                                               HSWEP_PCI_PCU_3),
                             // HSWEP_PCI_PCU_3 == 2

As the result, the index for each device gets same, so the pci device
is overwrited in uncore_extra_pci_dev[pkg].dev[idx] when it is probed.

static int uncore_pci_probe(struct pci_dev *pdev,...
{
...
        if (UNCORE_PCI_DEV_TYPE(id->driver_data) == UNCORE_EXTRA_PCI_DEV) {
                int idx = UNCORE_PCI_DEV_IDX(id->driver_data); // HERE!!

                uncore_extra_pci_dev[pkg].dev[idx] = pdev;     // HERE!!
                pci_set_drvdata(pdev, NULL);
                return 0;
        }

Due to the overwriting, the following warning message are shown
while CPU hot-removing.

  WARNING: CPU: 126 PID: 6 at arch/x86/events/intel/uncore.c:988
  uncore_pci_remove+0x10b/0x150
  Call Trace:
   pci_device_remove+0x42/0xd0
   device_release_driver_internal+0x148/0x220
   pci_stop_bus_device+0x76/0xa0
   pci_stop_root_bus+0x44/0x60
   acpi_pci_root_remove+0x1f/0x80
   acpi_bus_trim+0x57/0x90
   acpi_bus_trim+0x2e/0x90
   acpi_device_hotplug+0x2bc/0x4b0
   acpi_hotplug_work_fn+0x1a/0x30
   process_one_work+0x174/0x3a0
   worker_thread+0x4c/0x3d0
   kthread+0xf8/0x130

To avoid the conflict, this patch changes the PCU.3 driver_data.
And also increase UNCORE_EXTRA_PCI_DEV_MAX to handle the new index.

Fixes: 15a3e845b01c ("perf/x86/intel/uncore: Fix SBOX support for Broadwell CPUs")

Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
---
 arch/x86/events/intel/uncore.h       | 2 +-
 arch/x86/events/intel/uncore_snbep.c | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
index c9e1e0b..e17ab88 100644
--- a/arch/x86/events/intel/uncore.h
+++ b/arch/x86/events/intel/uncore.h
@@ -28,7 +28,7 @@
 #define UNCORE_PCI_DEV_TYPE(data)	((data >> 8) & 0xff)
 #define UNCORE_PCI_DEV_IDX(data)	(data & 0xff)
 #define UNCORE_EXTRA_PCI_DEV		0xff
-#define UNCORE_EXTRA_PCI_DEV_MAX	3
+#define UNCORE_EXTRA_PCI_DEV_MAX	4
 
 #define UNCORE_EVENT_CONSTRAINT(c, n) EVENT_CONSTRAINT(c, n, 0xff)
 
diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index 87dc026..62f007c 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -1030,6 +1030,7 @@ enum {
 	SNBEP_PCI_QPI_PORT0_FILTER,
 	SNBEP_PCI_QPI_PORT1_FILTER,
 	HSWEP_PCI_PCU_3,
+	BDX_PCI_PCU_3,
 };
 
 static int snbep_qpi_hw_config(struct intel_uncore_box *box, struct perf_event *event)
@@ -3070,11 +3071,11 @@ void bdx_uncore_cpu_init(void)
 	if (boot_cpu_data.x86_model == 86) {
 		uncore_msr_uncores[BDX_MSR_UNCORE_SBOX] = NULL;
 	/* Detect systems with no SBOXes */
-	} else if (uncore_extra_pci_dev[pkg].dev[HSWEP_PCI_PCU_3]) {
+	} else if (uncore_extra_pci_dev[pkg].dev[BDX_PCI_PCU_3]) {
 		struct pci_dev *pdev;
 		u32 capid4;
 
-		pdev = uncore_extra_pci_dev[pkg].dev[HSWEP_PCI_PCU_3];
+		pdev = uncore_extra_pci_dev[pkg].dev[BDX_PCI_PCU_3];
 		pci_read_config_dword(pdev, 0x94, &capid4);
 		if (((capid4 >> 6) & 0x3) == 0)
 			bdx_msr_uncores[BDX_MSR_UNCORE_SBOX] = NULL;
@@ -3299,7 +3300,7 @@ static const struct pci_device_id bdx_uncore_pci_ids[] = {
 	{ /* PCU.3 (for Capability registers) */
 		PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6fc0),
 		.driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
-						   HSWEP_PCI_PCU_3),
+						   BDX_PCI_PCU_3),
 	},
 	{ /* end: all zeroes */ }
 };
-- 
2.16.1

Re: [PATCH] [RESEND] perf/x86/intel/uncore: Fix the index of PCU.3 Broadwell CPUs

[Ingo Molnar]

* Masayoshi Mizuma <msys.mizuma@gmail.com> wrote:

> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> 
> commit 15a3e845b01c ("perf/x86/intel/uncore: Fix SBOX support for
> Broadwell CPUs") introduced PCU.3 for Broadwell CPU. Unfortunately,
> the driver_data of PCU.3 conflicts to QPI Port 2 filter.
> 
>     { /* QPI Port 2 filter  */
>             PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f46),
>             .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 2),
> 
>     { /* PCU.3 (for Capability registers) */
>             PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6fc0),
>             .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
>                                                HSWEP_PCI_PCU_3),
>                              // HSWEP_PCI_PCU_3 == 2

> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -1030,6 +1030,7 @@ enum {
>  	SNBEP_PCI_QPI_PORT0_FILTER,
>  	SNBEP_PCI_QPI_PORT1_FILTER,
>  	HSWEP_PCI_PCU_3,
> +	BDX_PCI_PCU_3,
>  };

So we use a magic '2' enumerator in the 'QPI Port 2 filter', and that overlaps 
with HSWEP_PCI_PCU_3, right?

Shouldn't we clean up all the enumerators and not use magic numbers, and this fix 
the conflict?

Thanks,

	Ingo

Re: [PATCH] [RESEND] perf/x86/intel/uncore: Fix the index of PCU.3 Broadwell CPUs

[Liang, Kan]

On 7/15/2018 6:34 PM, Ingo Molnar wrote:
> 
> * Masayoshi Mizuma <msys.mizuma@gmail.com> wrote:
> 
>> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
>>
>> commit 15a3e845b01c ("perf/x86/intel/uncore: Fix SBOX support for
>> Broadwell CPUs") introduced PCU.3 for Broadwell CPU. Unfortunately,
>> the driver_data of PCU.3 conflicts to QPI Port 2 filter.
>>
>>      { /* QPI Port 2 filter  */
>>              PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f46),
>>              .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 2),
>>
>>      { /* PCU.3 (for Capability registers) */
>>              PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6fc0),
>>              .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
>>                                                 HSWEP_PCI_PCU_3),
>>                               // HSWEP_PCI_PCU_3 == 2
> 
>> --- a/arch/x86/events/intel/uncore_snbep.c
>> +++ b/arch/x86/events/intel/uncore_snbep.c
>> @@ -1030,6 +1030,7 @@ enum {
>>   	SNBEP_PCI_QPI_PORT0_FILTER,
>>   	SNBEP_PCI_QPI_PORT1_FILTER,
>>   	HSWEP_PCI_PCU_3,
>> +	BDX_PCI_PCU_3,
>>   };
> 
> So we use a magic '2' enumerator in the 'QPI Port 2 filter', and that overlaps
> with HSWEP_PCI_PCU_3, right?
> 
> Shouldn't we clean up all the enumerators and not use magic numbers, and this fix
> the conflict?
>

Yes, it should fix the conflict. I will clean up the code.

Thanks,
Kan


> Thanks,
> 
> 	Ingo
> 

Re: [PATCH] [RESEND] perf/x86/intel/uncore: Fix the index of PCU.3 Broadwell CPUs

[Masayoshi Mizuma]

On 07/16/2018 10:29 AM, Liang, Kan wrote:
> 
> 
> On 7/15/2018 6:34 PM, Ingo Molnar wrote:
>>
>> * Masayoshi Mizuma <msys.mizuma@gmail.com> wrote:
>>
>>> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
>>>
>>> commit 15a3e845b01c ("perf/x86/intel/uncore: Fix SBOX support for
>>> Broadwell CPUs") introduced PCU.3 for Broadwell CPU. Unfortunately,
>>> the driver_data of PCU.3 conflicts to QPI Port 2 filter.
>>>
>>>      { /* QPI Port 2 filter  */
>>>              PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f46),
>>>              .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 2),
>>>
>>>      { /* PCU.3 (for Capability registers) */
>>>              PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6fc0),
>>>              .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
>>>                                                 HSWEP_PCI_PCU_3),
>>>                               // HSWEP_PCI_PCU_3 == 2
>>
>>> --- a/arch/x86/events/intel/uncore_snbep.c
>>> +++ b/arch/x86/events/intel/uncore_snbep.c
>>> @@ -1030,6 +1030,7 @@ enum {
>>>       SNBEP_PCI_QPI_PORT0_FILTER,
>>>       SNBEP_PCI_QPI_PORT1_FILTER,
>>>       HSWEP_PCI_PCU_3,
>>> +    BDX_PCI_PCU_3,
>>>   };
>>
>> So we use a magic '2' enumerator in the 'QPI Port 2 filter', and that overlaps
>> with HSWEP_PCI_PCU_3, right?
>>
>> Shouldn't we clean up all the enumerators and not use magic numbers, and this fix
>> the conflict?
>>
> 
> Yes, it should fix the conflict. I will clean up the code.

Thanks a lot!
I would appreciate if you could add CC to me when you post the patch.

Thanks,
Masa 

> 
> Thanks,
> Kan
> 
> 
>> Thanks,
>>
>>     Ingo
>>

Re: [PATCH] [RESEND] perf/x86/intel/uncore: Fix the index of PCU.3 Broadwell CPUs

[Liang, Kan]

On 7/16/2018 11:07 AM, Masayoshi Mizuma wrote:
> 
> 
> On 07/16/2018 10:29 AM, Liang, Kan wrote:
>>
>>
>> On 7/15/2018 6:34 PM, Ingo Molnar wrote:
>>>
>>> * Masayoshi Mizuma <msys.mizuma@gmail.com> wrote:
>>>
>>>> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
>>>>
>>>> commit 15a3e845b01c ("perf/x86/intel/uncore: Fix SBOX support for
>>>> Broadwell CPUs") introduced PCU.3 for Broadwell CPU. Unfortunately,
>>>> the driver_data of PCU.3 conflicts to QPI Port 2 filter.
>>>>
>>>>       { /* QPI Port 2 filter  */
>>>>               PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f46),
>>>>               .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 2),
>>>>
>>>>       { /* PCU.3 (for Capability registers) */
>>>>               PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6fc0),
>>>>               .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
>>>>                                                  HSWEP_PCI_PCU_3),
>>>>                                // HSWEP_PCI_PCU_3 == 2
>>>
>>>> --- a/arch/x86/events/intel/uncore_snbep.c
>>>> +++ b/arch/x86/events/intel/uncore_snbep.c
>>>> @@ -1030,6 +1030,7 @@ enum {
>>>>        SNBEP_PCI_QPI_PORT0_FILTER,
>>>>        SNBEP_PCI_QPI_PORT1_FILTER,
>>>>        HSWEP_PCI_PCU_3,
>>>> +    BDX_PCI_PCU_3,
>>>>    };
>>>
>>> So we use a magic '2' enumerator in the 'QPI Port 2 filter', and that overlaps
>>> with HSWEP_PCI_PCU_3, right?
>>>
>>> Shouldn't we clean up all the enumerators and not use magic numbers, and this fix
>>> the conflict?
>>>
>>
>> Yes, it should fix the conflict. I will clean up the code.
> 
> Thanks a lot!
> I would appreciate if you could add CC to me when you post the patch.
> 

Here is the patch.

Masa, could you please give it a try?

Thanks,
Kan

 From 688378a4003ec33156958a52dc822105c18075af Mon Sep 17 00:00:00 2001
From: Kan Liang <kan.liang@linux.intel.com>
Date: Mon, 16 Jul 2018 04:57:51 -0400
Subject: [PATCH] perf/x86/intel/uncore: Fix hardcode index of Broadwell 
extra PCI DEV

Masa reports that a warning message is shown while CPU hot-removing on
Broadwell server.

   WARNING: CPU: 126 PID: 6 at arch/x86/events/intel/uncore.c:988
   uncore_pci_remove+0x10b/0x150
   Call Trace:
    pci_device_remove+0x42/0xd0
    device_release_driver_internal+0x148/0x220
    pci_stop_bus_device+0x76/0xa0
    pci_stop_root_bus+0x44/0x60
    acpi_pci_root_remove+0x1f/0x80
    acpi_bus_trim+0x57/0x90
    acpi_bus_trim+0x2e/0x90
    acpi_device_hotplug+0x2bc/0x4b0
    acpi_hotplug_work_fn+0x1a/0x30
    process_one_work+0x174/0x3a0
    worker_thread+0x4c/0x3d0
    kthread+0xf8/0x130

This bug was introduced in:

   commit 15a3e845b01c ("perf/x86/intel/uncore: Fix SBOX support for
Broadwell CPUs")

The index of "QPI Port 2 filter" was hardcode to 2. The index of
"PCU.3" used enumerator "HSWEP_PCI_PCU_3", which equals to 2 as well.

To fix the conflict, the hardcode index needs to be cleaned up.
Introduce a new enumerator "BDX_PCI_QPI_PORT2_FILTER" for "QPI Port 2
filter" on Broadwell, and increase the UNCORE_EXTRA_PCI_DEV_MAX.
Clean up hardcode index.

Reported-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
Fixes: 15a3e845b01c ("perf/x86/intel/uncore: Fix SBOX support for
Broadwell CPUs")
---
  arch/x86/events/intel/uncore.h       |  2 +-
  arch/x86/events/intel/uncore_snbep.c | 10 +++++++---
  2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
index c9e1e0b..e17ab88 100644
--- a/arch/x86/events/intel/uncore.h
+++ b/arch/x86/events/intel/uncore.h
@@ -28,7 +28,7 @@
  #define UNCORE_PCI_DEV_TYPE(data)	((data >> 8) & 0xff)
  #define UNCORE_PCI_DEV_IDX(data)	(data & 0xff)
  #define UNCORE_EXTRA_PCI_DEV		0xff
-#define UNCORE_EXTRA_PCI_DEV_MAX	3
+#define UNCORE_EXTRA_PCI_DEV_MAX	4

  #define UNCORE_EVENT_CONSTRAINT(c, n) EVENT_CONSTRAINT(c, n, 0xff)

diff --git a/arch/x86/events/intel/uncore_snbep.c 
b/arch/x86/events/intel/uncore_snbep.c
index 87dc026..51d7c11 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -1029,6 +1029,7 @@ void snbep_uncore_cpu_init(void)
  enum {
  	SNBEP_PCI_QPI_PORT0_FILTER,
  	SNBEP_PCI_QPI_PORT1_FILTER,
+	BDX_PCI_QPI_PORT2_FILTER,
  	HSWEP_PCI_PCU_3,
  };

@@ -3286,15 +3287,18 @@ static const struct pci_device_id 
bdx_uncore_pci_ids[] = {
  	},
  	{ /* QPI Port 0 filter  */
  		PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f86),
-		.driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 0),
+		.driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
+						   SNBEP_PCI_QPI_PORT0_FILTER),
  	},
  	{ /* QPI Port 1 filter  */
  		PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f96),
-		.driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 1),
+		.driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
+						   SNBEP_PCI_QPI_PORT1_FILTER),
  	},
  	{ /* QPI Port 2 filter  */
  		PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f46),
-		.driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 2),
+		.driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
+						   BDX_PCI_QPI_PORT2_FILTER),
  	},
  	{ /* PCU.3 (for Capability registers) */
  		PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6fc0),
-- 
2.4.11

Re: [PATCH] [RESEND] perf/x86/intel/uncore: Fix the index of PCU.3 Broadwell CPUs

[Masayoshi Mizuma]

On 07/16/2018 12:31 PM, Liang, Kan wrote:
> 
> 
> On 7/16/2018 11:07 AM, Masayoshi Mizuma wrote:
>>
>>
>> On 07/16/2018 10:29 AM, Liang, Kan wrote:
>>>
>>>
>>> On 7/15/2018 6:34 PM, Ingo Molnar wrote:
>>>>
>>>> * Masayoshi Mizuma <msys.mizuma@gmail.com> wrote:
>>>>
>>>>> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
>>>>>
>>>>> commit 15a3e845b01c ("perf/x86/intel/uncore: Fix SBOX support for
>>>>> Broadwell CPUs") introduced PCU.3 for Broadwell CPU. Unfortunately,
>>>>> the driver_data of PCU.3 conflicts to QPI Port 2 filter.
>>>>>
>>>>>       { /* QPI Port 2 filter  */
>>>>>               PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f46),
>>>>>               .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 2),
>>>>>
>>>>>       { /* PCU.3 (for Capability registers) */
>>>>>               PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6fc0),
>>>>>               .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
>>>>>                                                  HSWEP_PCI_PCU_3),
>>>>>                                // HSWEP_PCI_PCU_3 == 2
>>>>
>>>>> --- a/arch/x86/events/intel/uncore_snbep.c
>>>>> +++ b/arch/x86/events/intel/uncore_snbep.c
>>>>> @@ -1030,6 +1030,7 @@ enum {
>>>>>        SNBEP_PCI_QPI_PORT0_FILTER,
>>>>>        SNBEP_PCI_QPI_PORT1_FILTER,
>>>>>        HSWEP_PCI_PCU_3,
>>>>> +    BDX_PCI_PCU_3,
>>>>>    };
>>>>
>>>> So we use a magic '2' enumerator in the 'QPI Port 2 filter', and that overlaps
>>>> with HSWEP_PCI_PCU_3, right?
>>>>
>>>> Shouldn't we clean up all the enumerators and not use magic numbers, and this fix
>>>> the conflict?
>>>>
>>>
>>> Yes, it should fix the conflict. I will clean up the code.
>>
>> Thanks a lot!
>> I would appreciate if you could add CC to me when you post the patch.
>>
> 
> Here is the patch.
> 
> Masa, could you please give it a try?

Thank you for the patch, it works well!

Please feel free to add:

Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

Thanks,
Masa

> 
> Thanks,
> Kan
> 
> From 688378a4003ec33156958a52dc822105c18075af Mon Sep 17 00:00:00 2001
> From: Kan Liang <kan.liang@linux.intel.com>
> Date: Mon, 16 Jul 2018 04:57:51 -0400
> Subject: [PATCH] perf/x86/intel/uncore: Fix hardcode index of Broadwell extra PCI DEV
> 
> Masa reports that a warning message is shown while CPU hot-removing on
> Broadwell server.
> 
>   WARNING: CPU: 126 PID: 6 at arch/x86/events/intel/uncore.c:988
>   uncore_pci_remove+0x10b/0x150
>   Call Trace:
>    pci_device_remove+0x42/0xd0
>    device_release_driver_internal+0x148/0x220
>    pci_stop_bus_device+0x76/0xa0
>    pci_stop_root_bus+0x44/0x60
>    acpi_pci_root_remove+0x1f/0x80
>    acpi_bus_trim+0x57/0x90
>    acpi_bus_trim+0x2e/0x90
>    acpi_device_hotplug+0x2bc/0x4b0
>    acpi_hotplug_work_fn+0x1a/0x30
>    process_one_work+0x174/0x3a0
>    worker_thread+0x4c/0x3d0
>    kthread+0xf8/0x130
> 
> This bug was introduced in:
> 
>   commit 15a3e845b01c ("perf/x86/intel/uncore: Fix SBOX support for
> Broadwell CPUs")
> 
> The index of "QPI Port 2 filter" was hardcode to 2. The index of
> "PCU.3" used enumerator "HSWEP_PCI_PCU_3", which equals to 2 as well.
> 
> To fix the conflict, the hardcode index needs to be cleaned up.
> Introduce a new enumerator "BDX_PCI_QPI_PORT2_FILTER" for "QPI Port 2
> filter" on Broadwell, and increase the UNCORE_EXTRA_PCI_DEV_MAX.
> Clean up hardcode index.
> 
> Reported-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> Suggested-by: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
> Fixes: 15a3e845b01c ("perf/x86/intel/uncore: Fix SBOX support for
> Broadwell CPUs")
> ---
>  arch/x86/events/intel/uncore.h       |  2 +-
>  arch/x86/events/intel/uncore_snbep.c | 10 +++++++---
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
> index c9e1e0b..e17ab88 100644
> --- a/arch/x86/events/intel/uncore.h
> +++ b/arch/x86/events/intel/uncore.h
> @@ -28,7 +28,7 @@
>  #define UNCORE_PCI_DEV_TYPE(data)    ((data >> 8) & 0xff)
>  #define UNCORE_PCI_DEV_IDX(data)    (data & 0xff)
>  #define UNCORE_EXTRA_PCI_DEV        0xff
> -#define UNCORE_EXTRA_PCI_DEV_MAX    3
> +#define UNCORE_EXTRA_PCI_DEV_MAX    4
> 
>  #define UNCORE_EVENT_CONSTRAINT(c, n) EVENT_CONSTRAINT(c, n, 0xff)
> 
> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
> index 87dc026..51d7c11 100644
> --- a/arch/x86/events/intel/uncore_snbep.c
> +++ b/arch/x86/events/intel/uncore_snbep.c
> @@ -1029,6 +1029,7 @@ void snbep_uncore_cpu_init(void)
>  enum {
>      SNBEP_PCI_QPI_PORT0_FILTER,
>      SNBEP_PCI_QPI_PORT1_FILTER,
> +    BDX_PCI_QPI_PORT2_FILTER,
>      HSWEP_PCI_PCU_3,
>  };
> 
> @@ -3286,15 +3287,18 @@ static const struct pci_device_id bdx_uncore_pci_ids[] = {
>      },
>      { /* QPI Port 0 filter  */
>          PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f86),
> -        .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 0),
> +        .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
> +                           SNBEP_PCI_QPI_PORT0_FILTER),
>      },
>      { /* QPI Port 1 filter  */
>          PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f96),
> -        .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 1),
> +        .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
> +                           SNBEP_PCI_QPI_PORT1_FILTER),
>      },
>      { /* QPI Port 2 filter  */
>          PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f46),
> -        .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 2),
> +        .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
> +                           BDX_PCI_QPI_PORT2_FILTER),
>      },
>      { /* PCU.3 (for Capability registers) */
>          PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6fc0),

Re: [PATCH] [RESEND] perf/x86/intel/uncore: Fix the index of PCU.3 Broadwell CPUs

[Masayoshi Mizuma]

Hi Ingo,

Is the following Kan's patch ready to merge...?

Thanks,
Masa

On 07/17/2018 12:37 PM, Masayoshi Mizuma wrote:
...
>> Here is the patch.
>>
>> Masa, could you please give it a try?
> 
> Thank you for the patch, it works well!
> 
> Please feel free to add:
> 
> Tested-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> 
> Thanks,
> Masa
> 
>>
>> Thanks,
>> Kan
>>
>> From 688378a4003ec33156958a52dc822105c18075af Mon Sep 17 00:00:00 2001
>> From: Kan Liang <kan.liang@linux.intel.com>
>> Date: Mon, 16 Jul 2018 04:57:51 -0400
>> Subject: [PATCH] perf/x86/intel/uncore: Fix hardcode index of Broadwell extra PCI DEV
>>
>> Masa reports that a warning message is shown while CPU hot-removing on
>> Broadwell server.
>>
>>   WARNING: CPU: 126 PID: 6 at arch/x86/events/intel/uncore.c:988
>>   uncore_pci_remove+0x10b/0x150
>>   Call Trace:
>>    pci_device_remove+0x42/0xd0
>>    device_release_driver_internal+0x148/0x220
>>    pci_stop_bus_device+0x76/0xa0
>>    pci_stop_root_bus+0x44/0x60
>>    acpi_pci_root_remove+0x1f/0x80
>>    acpi_bus_trim+0x57/0x90
>>    acpi_bus_trim+0x2e/0x90
>>    acpi_device_hotplug+0x2bc/0x4b0
>>    acpi_hotplug_work_fn+0x1a/0x30
>>    process_one_work+0x174/0x3a0
>>    worker_thread+0x4c/0x3d0
>>    kthread+0xf8/0x130
>>
>> This bug was introduced in:
>>
>>   commit 15a3e845b01c ("perf/x86/intel/uncore: Fix SBOX support for
>> Broadwell CPUs")
>>
>> The index of "QPI Port 2 filter" was hardcode to 2. The index of
>> "PCU.3" used enumerator "HSWEP_PCI_PCU_3", which equals to 2 as well.
>>
>> To fix the conflict, the hardcode index needs to be cleaned up.
>> Introduce a new enumerator "BDX_PCI_QPI_PORT2_FILTER" for "QPI Port 2
>> filter" on Broadwell, and increase the UNCORE_EXTRA_PCI_DEV_MAX.
>> Clean up hardcode index.
>>
>> Reported-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
>> Suggested-by: Ingo Molnar <mingo@kernel.org>
>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>> Fixes: 15a3e845b01c ("perf/x86/intel/uncore: Fix SBOX support for
>> Broadwell CPUs")
>> ---
>>  arch/x86/events/intel/uncore.h       |  2 +-
>>  arch/x86/events/intel/uncore_snbep.c | 10 +++++++---
>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
>> index c9e1e0b..e17ab88 100644
>> --- a/arch/x86/events/intel/uncore.h
>> +++ b/arch/x86/events/intel/uncore.h
>> @@ -28,7 +28,7 @@
>>  #define UNCORE_PCI_DEV_TYPE(data)    ((data >> 8) & 0xff)
>>  #define UNCORE_PCI_DEV_IDX(data)    (data & 0xff)
>>  #define UNCORE_EXTRA_PCI_DEV        0xff
>> -#define UNCORE_EXTRA_PCI_DEV_MAX    3
>> +#define UNCORE_EXTRA_PCI_DEV_MAX    4
>>
>>  #define UNCORE_EVENT_CONSTRAINT(c, n) EVENT_CONSTRAINT(c, n, 0xff)
>>
>> diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
>> index 87dc026..51d7c11 100644
>> --- a/arch/x86/events/intel/uncore_snbep.c
>> +++ b/arch/x86/events/intel/uncore_snbep.c
>> @@ -1029,6 +1029,7 @@ void snbep_uncore_cpu_init(void)
>>  enum {
>>      SNBEP_PCI_QPI_PORT0_FILTER,
>>      SNBEP_PCI_QPI_PORT1_FILTER,
>> +    BDX_PCI_QPI_PORT2_FILTER,
>>      HSWEP_PCI_PCU_3,
>>  };
>>
>> @@ -3286,15 +3287,18 @@ static const struct pci_device_id bdx_uncore_pci_ids[] = {
>>      },
>>      { /* QPI Port 0 filter  */
>>          PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f86),
>> -        .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 0),
>> +        .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
>> +                           SNBEP_PCI_QPI_PORT0_FILTER),
>>      },
>>      { /* QPI Port 1 filter  */
>>          PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f96),
>> -        .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 1),
>> +        .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
>> +                           SNBEP_PCI_QPI_PORT1_FILTER),
>>      },
>>      { /* QPI Port 2 filter  */
>>          PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6f46),
>> -        .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV, 2),
>> +        .driver_data = UNCORE_PCI_DEV_DATA(UNCORE_EXTRA_PCI_DEV,
>> +                           BDX_PCI_QPI_PORT2_FILTER),
>>      },
>>      { /* PCU.3 (for Capability registers) */
>>          PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x6fc0),

Re: [PATCH] [RESEND] perf/x86/intel/uncore: Fix the index of PCU.3 Broadwell CPUs

[Ingo Molnar]

* Masayoshi Mizuma <msys.mizuma@gmail.com> wrote:

> Hi Ingo,
> 
> Is the following Kan's patch ready to merge...?

Looks good at first sight - but it was whitespace damaged here so I couldn't apply it.

Kan, mind re-sending it properly as a standalone patch, with Masayoshi's Reported-by, Debugged-by and Tested-by 
tags added?

Thanks,

	Ingo

Re: [PATCH] [RESEND] perf/x86/intel/uncore: Fix the index of PCU.3 Broadwell CPUs

[Liang, Kan]

On 7/30/2018 6:06 AM, Ingo Molnar wrote:
> 
> * Masayoshi Mizuma <msys.mizuma@gmail.com> wrote:
> 
>> Hi Ingo,
>>
>> Is the following Kan's patch ready to merge...?
> 
> Looks good at first sight - but it was whitespace damaged here so I couldn't apply it.
> 
> Kan, mind re-sending it properly as a standalone patch, with Masayoshi's Reported-by, Debugged-by and Tested-by
> tags added?
> 

Sure, I will re-send the patch.

Thanks,
Kan