[PATCH 1/3] x86/mce/AMD: Use msr_stat when clearing MCA_STATUS

[PATCH 1/3] x86/mce/AMD: Use msr_stat when clearing MCA_STATUS

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

The value of MCA_STATUS is used as the MSR when clearing MCA_STATUS.

This may cause the following warning:
 unchecked MSR access error: WRMSR to 0x11b (tried to write 0x0000000000000000)
 Call Trace:
  <IRQ>
  ? amd_threshold_interrupt+0x209/0x220
  smp_threshold_interrupt+0x1b/0x40
  threshold_interrupt+0x89/0x90

Use msr_stat instead which has the MSR address.

Fixes: 37d43acfd79f ("x86/mce/AMD: Redo error logging from APIC LVT interrupt handlers")
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index d00f299..d11f94e 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -815,7 +815,7 @@ _log_error_bank(unsigned int bank, u32 msr_stat, u32 msr_addr, u64 misc)
 
 	__log_error(bank, status, addr, misc);
 
-	wrmsrl(status, 0);
+	wrmsrl(msr_stat, 0);
 
 	return status & MCI_STATUS_DEFERRED;
 }
-- 
2.7.4

[1/3] x86/mce/AMD: Use msr_stat when clearing MCA_STATUS

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

The value of MCA_STATUS is used as the MSR when clearing MCA_STATUS.

This may cause the following warning:
 unchecked MSR access error: WRMSR to 0x11b (tried to write 0x0000000000000000)
 Call Trace:
  <IRQ>
  ? amd_threshold_interrupt+0x209/0x220
  smp_threshold_interrupt+0x1b/0x40
  threshold_interrupt+0x89/0x90

Use msr_stat instead which has the MSR address.

Fixes: 37d43acfd79f ("x86/mce/AMD: Redo error logging from APIC LVT interrupt handlers")
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index d00f299..d11f94e 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -815,7 +815,7 @@ _log_error_bank(unsigned int bank, u32 msr_stat, u32 msr_addr, u64 misc)
 
 	__log_error(bank, status, addr, misc);
 
-	wrmsrl(status, 0);
+	wrmsrl(msr_stat, 0);
 
 	return status & MCI_STATUS_DEFERRED;
 }

[PATCH 2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

There needs to be a list_head outside of a linked list in order to iterate
over it and have access to all its elements. This is because the
list_for_each* macros iterate starting from head->next rather than head.

Define a list_head for the threshold blocks list in struct threshold_bank
since this is the container of the list. Use this list_head as the head
instead of the first element in the blocks list.

This is needed in a future patch where we read the blocks list in the
threshold interrupt handler. Currently, we'll always skip block 0 since it
is the head of the list.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/include/asm/amd_nb.h        |  2 ++
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 19 ++++++++++---------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
index da181ad..81334b4 100644
--- a/arch/x86/include/asm/amd_nb.h
+++ b/arch/x86/include/asm/amd_nb.h
@@ -55,6 +55,8 @@ struct threshold_bank {
 	struct kobject		*kobj;
 	struct threshold_block	*blocks;
 
+	struct list_head	blocks_head;
+
 	/* initialized to the number of CPUs on the node sharing this bank */
 	refcount_t		cpus;
 };
diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index d11f94e..2074b870 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -1119,15 +1119,15 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 		threshold_ktype.default_attrs[2] = NULL;
 	}
 
-	INIT_LIST_HEAD(&b->miscj);
-
-	if (per_cpu(threshold_banks, cpu)[bank]->blocks) {
-		list_add(&b->miscj,
-			 &per_cpu(threshold_banks, cpu)[bank]->blocks->miscj);
-	} else {
+	if (!per_cpu(threshold_banks, cpu)[bank]->blocks) {
+		INIT_LIST_HEAD(&per_cpu(threshold_banks, cpu)[bank]->blocks_head);
 		per_cpu(threshold_banks, cpu)[bank]->blocks = b;
 	}
 
+	INIT_LIST_HEAD(&b->miscj);
+
+	list_add(&b->miscj, &per_cpu(threshold_banks, cpu)[bank]->blocks_head);
+
 	err = kobject_init_and_add(&b->kobj, &threshold_ktype,
 				   per_cpu(threshold_banks, cpu)[bank]->kobj,
 				   get_name(bank, b));
@@ -1158,7 +1158,7 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 
 static int __threshold_add_blocks(struct threshold_bank *b)
 {
-	struct list_head *head = &b->blocks->miscj;
+	struct list_head *head = &b->blocks_head;
 	struct threshold_block *pos = NULL;
 	struct threshold_block *tmp = NULL;
 	int err = 0;
@@ -1256,7 +1256,7 @@ static void deallocate_threshold_block(unsigned int cpu,
 	if (!head)
 		return;
 
-	list_for_each_entry_safe(pos, tmp, &head->blocks->miscj, miscj) {
+	list_for_each_entry_safe(pos, tmp, &head->blocks_head, miscj) {
 		kobject_put(&pos->kobj);
 		list_del(&pos->miscj);
 		kfree(pos);
@@ -1273,7 +1273,7 @@ static void __threshold_remove_blocks(struct threshold_bank *b)
 
 	kobject_del(b->kobj);
 
-	list_for_each_entry_safe(pos, tmp, &b->blocks->miscj, miscj)
+	list_for_each_entry_safe(pos, tmp, &b->blocks_head, miscj)
 		kobject_del(&pos->kobj);
 }
 
@@ -1307,6 +1307,7 @@ static void threshold_remove_bank(unsigned int cpu, int bank)
 	deallocate_threshold_block(cpu, bank);
 
 free_out:
+	list_del(&b->blocks_head);
 	kobject_del(b->kobj);
 	kobject_put(b->kobj);
 	kfree(b);
-- 
2.7.4

[2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

There needs to be a list_head outside of a linked list in order to iterate
over it and have access to all its elements. This is because the
list_for_each* macros iterate starting from head->next rather than head.

Define a list_head for the threshold blocks list in struct threshold_bank
since this is the container of the list. Use this list_head as the head
instead of the first element in the blocks list.

This is needed in a future patch where we read the blocks list in the
threshold interrupt handler. Currently, we'll always skip block 0 since it
is the head of the list.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/include/asm/amd_nb.h        |  2 ++
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 19 ++++++++++---------
 2 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
index da181ad..81334b4 100644
--- a/arch/x86/include/asm/amd_nb.h
+++ b/arch/x86/include/asm/amd_nb.h
@@ -55,6 +55,8 @@ struct threshold_bank {
 	struct kobject		*kobj;
 	struct threshold_block	*blocks;
 
+	struct list_head	blocks_head;
+
 	/* initialized to the number of CPUs on the node sharing this bank */
 	refcount_t		cpus;
 };
diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index d11f94e..2074b870 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -1119,15 +1119,15 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 		threshold_ktype.default_attrs[2] = NULL;
 	}
 
-	INIT_LIST_HEAD(&b->miscj);
-
-	if (per_cpu(threshold_banks, cpu)[bank]->blocks) {
-		list_add(&b->miscj,
-			 &per_cpu(threshold_banks, cpu)[bank]->blocks->miscj);
-	} else {
+	if (!per_cpu(threshold_banks, cpu)[bank]->blocks) {
+		INIT_LIST_HEAD(&per_cpu(threshold_banks, cpu)[bank]->blocks_head);
 		per_cpu(threshold_banks, cpu)[bank]->blocks = b;
 	}
 
+	INIT_LIST_HEAD(&b->miscj);
+
+	list_add(&b->miscj, &per_cpu(threshold_banks, cpu)[bank]->blocks_head);
+
 	err = kobject_init_and_add(&b->kobj, &threshold_ktype,
 				   per_cpu(threshold_banks, cpu)[bank]->kobj,
 				   get_name(bank, b));
@@ -1158,7 +1158,7 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 
 static int __threshold_add_blocks(struct threshold_bank *b)
 {
-	struct list_head *head = &b->blocks->miscj;
+	struct list_head *head = &b->blocks_head;
 	struct threshold_block *pos = NULL;
 	struct threshold_block *tmp = NULL;
 	int err = 0;
@@ -1256,7 +1256,7 @@ static void deallocate_threshold_block(unsigned int cpu,
 	if (!head)
 		return;
 
-	list_for_each_entry_safe(pos, tmp, &head->blocks->miscj, miscj) {
+	list_for_each_entry_safe(pos, tmp, &head->blocks_head, miscj) {
 		kobject_put(&pos->kobj);
 		list_del(&pos->miscj);
 		kfree(pos);
@@ -1273,7 +1273,7 @@ static void __threshold_remove_blocks(struct threshold_bank *b)
 
 	kobject_del(b->kobj);
 
-	list_for_each_entry_safe(pos, tmp, &b->blocks->miscj, miscj)
+	list_for_each_entry_safe(pos, tmp, &b->blocks_head, miscj)
 		kobject_del(&pos->kobj);
 }
 
@@ -1307,6 +1307,7 @@ static void threshold_remove_bank(unsigned int cpu, int bank)
 	deallocate_threshold_block(cpu, bank);
 
 free_out:
+	list_del(&b->blocks_head);
 	kobject_del(b->kobj);
 	kobject_put(b->kobj);
 	kfree(b);

[PATCH 3/3] x86/mce/AMD: Use saved threshold block info in interrupt handler

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

In the amd_threshold_interrupt() handler, we loop through every possible
block in each bank and rediscover the block's address and if it's valid,
e.g. valid, counter present and not locked. However, we already have the
address saved in the threshold blocks list for each CPU and bank. The list
only contains blocks that have passed all the valid checks.

Besides the redundancy, there's also an smp_call_function* in
get_block_address() and this causes a warning when servicing the interrupt.

 WARNING: CPU: 0 PID: 0 at kernel/smp.c:281 smp_call_function_single+0xdd/0xf0
 ...
 Call Trace:
  <IRQ>
  rdmsr_safe_on_cpu+0x5d/0x90
  get_block_address.isra.2+0x97/0x100
  amd_threshold_interrupt+0xae/0x220
  smp_threshold_interrupt+0x1b/0x40
  threshold_interrupt+0x89/0x90

Iterate over the threshold blocks list and use the saved address. Also,
drop the redundant valid checks.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 2074b870..14e4dab 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -873,30 +873,23 @@ static void log_error_thresholding(unsigned int bank, u64 misc)
  */
 static void amd_threshold_interrupt(void)
 {
-	u32 low = 0, high = 0, address = 0;
-	unsigned int bank, block, cpu = smp_processor_id();
+	u32 low = 0, high = 0;
+	unsigned int bank, cpu = smp_processor_id();
 	struct thresh_restart tr;
+	struct threshold_block *block = NULL, *tmp = NULL;
+	struct list_head *head = NULL;
 
 	for (bank = 0; bank < mca_cfg.banks; ++bank) {
 		if (!(per_cpu(bank_map, cpu) & (1 << bank)))
 			continue;
-		for (block = 0; block < NR_BLOCKS; ++block) {
-			address = get_block_address(cpu, address, low, high, bank, block);
-			if (!address)
-				break;
 
-			if (rdmsr_safe(address, &low, &high))
-				break;
+		if (!per_cpu(threshold_banks, cpu)[bank]->blocks)
+			continue;
 
-			if (!(high & MASK_VALID_HI)) {
-				if (block)
-					continue;
-				else
-					break;
-			}
+		head = &per_cpu(threshold_banks, cpu)[bank]->blocks_head;
 
-			if (!(high & MASK_CNTP_HI)  ||
-			     (high & MASK_LOCKED_HI))
+		list_for_each_entry_safe(block, tmp, head, miscj) {
+			if (rdmsr_safe(block->address, &low, &high))
 				continue;
 
 			if (!(high & MASK_OVERFLOW_HI))
@@ -907,7 +900,7 @@ static void amd_threshold_interrupt(void)
 
 			/* Reset threshold block after logging error. */
 			memset(&tr, 0, sizeof(tr));
-			tr.b = &per_cpu(threshold_banks, cpu)[bank]->blocks[block];
+			tr.b = block;
 			threshold_restart_bank(&tr);
 		}
 	}
-- 
2.7.4

[3/3] x86/mce/AMD: Use saved threshold block info in interrupt handler

[Yazen Ghannam]

From: Yazen Ghannam <yazen.ghannam@amd.com>

In the amd_threshold_interrupt() handler, we loop through every possible
block in each bank and rediscover the block's address and if it's valid,
e.g. valid, counter present and not locked. However, we already have the
address saved in the threshold blocks list for each CPU and bank. The list
only contains blocks that have passed all the valid checks.

Besides the redundancy, there's also an smp_call_function* in
get_block_address() and this causes a warning when servicing the interrupt.

 WARNING: CPU: 0 PID: 0 at kernel/smp.c:281 smp_call_function_single+0xdd/0xf0
 ...
 Call Trace:
  <IRQ>
  rdmsr_safe_on_cpu+0x5d/0x90
  get_block_address.isra.2+0x97/0x100
  amd_threshold_interrupt+0xae/0x220
  smp_threshold_interrupt+0x1b/0x40
  threshold_interrupt+0x89/0x90

Iterate over the threshold blocks list and use the saved address. Also,
drop the redundant valid checks.

Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 2074b870..14e4dab 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -873,30 +873,23 @@ static void log_error_thresholding(unsigned int bank, u64 misc)
  */
 static void amd_threshold_interrupt(void)
 {
-	u32 low = 0, high = 0, address = 0;
-	unsigned int bank, block, cpu = smp_processor_id();
+	u32 low = 0, high = 0;
+	unsigned int bank, cpu = smp_processor_id();
 	struct thresh_restart tr;
+	struct threshold_block *block = NULL, *tmp = NULL;
+	struct list_head *head = NULL;
 
 	for (bank = 0; bank < mca_cfg.banks; ++bank) {
 		if (!(per_cpu(bank_map, cpu) & (1 << bank)))
 			continue;
-		for (block = 0; block < NR_BLOCKS; ++block) {
-			address = get_block_address(cpu, address, low, high, bank, block);
-			if (!address)
-				break;
 
-			if (rdmsr_safe(address, &low, &high))
-				break;
+		if (!per_cpu(threshold_banks, cpu)[bank]->blocks)
+			continue;
 
-			if (!(high & MASK_VALID_HI)) {
-				if (block)
-					continue;
-				else
-					break;
-			}
+		head = &per_cpu(threshold_banks, cpu)[bank]->blocks_head;
 
-			if (!(high & MASK_CNTP_HI)  ||
-			     (high & MASK_LOCKED_HI))
+		list_for_each_entry_safe(block, tmp, head, miscj) {
+			if (rdmsr_safe(block->address, &low, &high))
 				continue;
 
 			if (!(high & MASK_OVERFLOW_HI))
@@ -907,7 +900,7 @@ static void amd_threshold_interrupt(void)
 
 			/* Reset threshold block after logging error. */
 			memset(&tr, 0, sizeof(tr));
-			tr.b = &per_cpu(threshold_banks, cpu)[bank]->blocks[block];
+			tr.b = block;
 			threshold_restart_bank(&tr);
 		}
 	}

Re: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

[Borislav Petkov]

On Wed, May 24, 2017 at 03:41:46PM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> There needs to be a list_head outside of a linked list in order to iterate
> over it and have access to all its elements. This is because the
> list_for_each* macros iterate starting from head->next rather than head.
> 
> Define a list_head for the threshold blocks list in struct threshold_bank

struct threshold_block {

	...

        struct list_head miscj;                 /*
                                                 * List of threshold blocks
                                                 * within a bank.
                                                 */

There's your list_head right there.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

[Borislav Petkov]

On Wed, May 24, 2017 at 03:41:46PM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> There needs to be a list_head outside of a linked list in order to iterate
> over it and have access to all its elements. This is because the
> list_for_each* macros iterate starting from head->next rather than head.
> 
> Define a list_head for the threshold blocks list in struct threshold_bank

struct threshold_block {

	...

        struct list_head miscj;                 /*
                                                 * List of threshold blocks
                                                 * within a bank.
                                                 */

There's your list_head right there.

RE: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Sunday, May 28, 2017 1:22 PM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; Borislav Petkov <bp@suse.de>; Tony Luck
> <tony.luck@intel.com>; x86@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold
> blocks outside the list
> 
> On Wed, May 24, 2017 at 03:41:46PM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > There needs to be a list_head outside of a linked list in order to
> > iterate over it and have access to all its elements. This is because
> > the
> > list_for_each* macros iterate starting from head->next rather than head.
> >
> > Define a list_head for the threshold blocks list in struct
> > threshold_bank
> 
> struct threshold_block {
> 
> 	...
> 
>         struct list_head miscj;                 /*
>                                                  * List of threshold blocks
>                                                  * within a bank.
>                                                  */
> 
> There's your list_head right there.
> 

Like I said in the commit message, the list_head needs to be outside the
list to access all the elements using list_for_each*. Otherwise, we won't
get a reference to the "head" element since we iterate starting from
head->next and break when !head.

For example here, we use block 0 as the head and I find that I don't get a
reference to it when using list_for_each* as the code is currently. Am I
doing something wrong?

Thanks,
Yazen

[2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

[Yazen Ghannam]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Sunday, May 28, 2017 1:22 PM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; Borislav Petkov <bp@suse.de>; Tony Luck
> <tony.luck@intel.com>; x86@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold
> blocks outside the list
> 
> On Wed, May 24, 2017 at 03:41:46PM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > There needs to be a list_head outside of a linked list in order to
> > iterate over it and have access to all its elements. This is because
> > the
> > list_for_each* macros iterate starting from head->next rather than head.
> >
> > Define a list_head for the threshold blocks list in struct
> > threshold_bank
> 
> struct threshold_block {
> 
> 	...
> 
>         struct list_head miscj;                 /*
>                                                  * List of threshold blocks
>                                                  * within a bank.
>                                                  */
> 
> There's your list_head right there.
> 

Like I said in the commit message, the list_head needs to be outside the
list to access all the elements using list_for_each*. Otherwise, we won't
get a reference to the "head" element since we iterate starting from
head->next and break when !head.

For example here, we use block 0 as the head and I find that I don't get a
reference to it when using list_for_each* as the code is currently. Am I
doing something wrong?

Thanks,
Yazen

Re: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

[Borislav Petkov]

On Tue, May 30, 2017 at 12:39:03PM +0000, Ghannam, Yazen wrote:
> Like I said in the commit message, the list_head needs to be outside the
> list to access all the elements using list_for_each*. Otherwise, we won't
> get a reference to the "head" element since we iterate starting from
> head->next and break when !head.

I believe the whole hierarchy here is done a bit differently: the list
starts at threshold_bank->blocks which points to the first threshold
block. So when iterating, you need to look at threshold_bank->blocks
first, which is the first element and then traverse the list.

This is basically how the list gets built:

allocate_threshold_blocks:

	...

        if (per_cpu(threshold_banks, cpu)[bank]->blocks)
                list_add(&b->miscj, &per_cpu(threshold_banks, cpu)[bank]->blocks->miscj);
        else
                per_cpu(threshold_banks, cpu)[bank]->blocks = b;

per_cpu(threshold_banks, cpu)[bank]->blocks is the first element as
it points to a struct threshold_block and then the ..->blocks->miscj
contains any further threshold blocks present on this bank and we queue
them there if ->blocks is not NULL.

HTH.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

[Borislav Petkov]

On Tue, May 30, 2017 at 12:39:03PM +0000, Ghannam, Yazen wrote:
> Like I said in the commit message, the list_head needs to be outside the
> list to access all the elements using list_for_each*. Otherwise, we won't
> get a reference to the "head" element since we iterate starting from
> head->next and break when !head.

I believe the whole hierarchy here is done a bit differently: the list
starts at threshold_bank->blocks which points to the first threshold
block. So when iterating, you need to look at threshold_bank->blocks
first, which is the first element and then traverse the list.

This is basically how the list gets built:

allocate_threshold_blocks:

	...

        if (per_cpu(threshold_banks, cpu)[bank]->blocks)
                list_add(&b->miscj, &per_cpu(threshold_banks, cpu)[bank]->blocks->miscj);
        else
                per_cpu(threshold_banks, cpu)[bank]->blocks = b;

per_cpu(threshold_banks, cpu)[bank]->blocks is the first element as
it points to a struct threshold_block and then the ..->blocks->miscj
contains any further threshold blocks present on this bank and we queue
them there if ->blocks is not NULL.

HTH.

RE: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

[Ghannam, Yazen]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, May 30, 2017 9:57 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; Tony Luck <tony.luck@intel.com>;
> x86@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold
> blocks outside the list
> 
> On Tue, May 30, 2017 at 12:39:03PM +0000, Ghannam, Yazen wrote:
> > Like I said in the commit message, the list_head needs to be outside
> > the list to access all the elements using list_for_each*. Otherwise,
> > we won't get a reference to the "head" element since we iterate
> > starting from
> > head->next and break when !head.
> 
> I believe the whole hierarchy here is done a bit differently: the list starts at
> threshold_bank->blocks which points to the first threshold block. So when
> iterating, you need to look at threshold_bank->blocks first, which is the first
> element and then traverse the list.
> 

Yeah, I thought about doing this in the THR interrupt handler. But then I thought
it might be better to re-define the head so that we could iterate through all of
the elements without having to check the first and iterate the rest.

> This is basically how the list gets built:
> 
> allocate_threshold_blocks:
> 
> 	...
> 
>         if (per_cpu(threshold_banks, cpu)[bank]->blocks)
>                 list_add(&b->miscj, &per_cpu(threshold_banks, cpu)[bank]->blocks-
> >miscj);
>         else
>                 per_cpu(threshold_banks, cpu)[bank]->blocks = b;
> 
> per_cpu(threshold_banks, cpu)[bank]->blocks is the first element as it points
> to a struct threshold_block and then the ..->blocks->miscj contains any further
> threshold blocks present on this bank and we queue them there if ->blocks is
> not NULL.
> 

Okay, so I'll redo Patch 3 in this set and drop this one. Any comments on Patch 1?

Thanks,
Yazen

[2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

[Yazen Ghannam]

> -----Original Message-----
> From: Borislav Petkov [mailto:bp@alien8.de]
> Sent: Tuesday, May 30, 2017 9:57 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; Tony Luck <tony.luck@intel.com>;
> x86@kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold
> blocks outside the list
> 
> On Tue, May 30, 2017 at 12:39:03PM +0000, Ghannam, Yazen wrote:
> > Like I said in the commit message, the list_head needs to be outside
> > the list to access all the elements using list_for_each*. Otherwise,
> > we won't get a reference to the "head" element since we iterate
> > starting from
> > head->next and break when !head.
> 
> I believe the whole hierarchy here is done a bit differently: the list starts at
> threshold_bank->blocks which points to the first threshold block. So when
> iterating, you need to look at threshold_bank->blocks first, which is the first
> element and then traverse the list.
> 

Yeah, I thought about doing this in the THR interrupt handler. But then I thought
it might be better to re-define the head so that we could iterate through all of
the elements without having to check the first and iterate the rest.

> This is basically how the list gets built:
> 
> allocate_threshold_blocks:
> 
> 	...
> 
>         if (per_cpu(threshold_banks, cpu)[bank]->blocks)
>                 list_add(&b->miscj, &per_cpu(threshold_banks, cpu)[bank]->blocks-
> >miscj);
>         else
>                 per_cpu(threshold_banks, cpu)[bank]->blocks = b;
> 
> per_cpu(threshold_banks, cpu)[bank]->blocks is the first element as it points
> to a struct threshold_block and then the ..->blocks->miscj contains any further
> threshold blocks present on this bank and we queue them there if ->blocks is
> not NULL.
> 

Okay, so I'll redo Patch 3 in this set and drop this one. Any comments on Patch 1?

Thanks,
Yazen

Re: [PATCH 2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

[Borislav Petkov]

On Tue, May 30, 2017 at 02:06:11PM +0000, Ghannam, Yazen wrote:
> Okay, so I'll redo Patch 3 in this set and drop this one. Any comments on Patch 1?

Nope, I took it already.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[2/3] x86/mce/AMD: Define a list_head for threshold blocks outside the list

[Borislav Petkov]

On Tue, May 30, 2017 at 02:06:11PM +0000, Ghannam, Yazen wrote:
> Okay, so I'll redo Patch 3 in this set and drop this one. Any comments on Patch 1?

Nope, I took it already.