[2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Yazen Ghannam]

> -----Original Message-----
> From: linux-edac-owner@vger.kernel.org <linux-edac-owner@vger.kernel.org>
> On Behalf Of Borislav Petkov
> Sent: Thursday, August 9, 2018 11:18 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org;
> tony.luck@intel.com; x86@kernel.org
> Subject: Re: [PATCH 2/2] x86/MCE/AMD: Skip creating kobjects with NULL
> names
> 
> On Thu, Aug 09, 2018 at 09:08:34AM -0500, Yazen Ghannam wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > During mce_threshold_create_device() data structures are allocated for
> > each CPUs MCA banks and thresholding blocks. These data structures are
> > used to save information related to AMD's MCA Error Thresholding
> > feature. The structures are used in the thresholding interrupt handler,
> > and they are exposed to the user through sysfs. The sysfs interface has
> > user-friendly names for each bank.
> >
> > However, errors in mce_threshold_create_device() will cause all the data
> > structures to be deallocated. This will break the thresholding interrupt
> > handler since it depends on these structures.
> 
> Same argument as before: if our init fails in some fashion, we should
> not be running the interrupt handler.
> 

This patch makes it so that we don't fail init just because some banks don't
have names. The data caching we do is useful even if we fail to create sysfs
entries for some banks. The interrupt handler can work fine without a sysfs
entry for every bank. It seems like overkill to deallocate all the structures
and sysfs entries for all the banks just because we fail to create entries for
some banks that don't have names. 

In other words, I think we should decouple the interrupt handler from the
sysfs interface. The interface is nice to have but not necessary for the HW
and OS to handle threshold interrupts. If we do so, then new HW with new,
unnamed types will work with older versions of Linux. We can then add the
new type names without having to backport to fix the interrupt handler.

Thanks,
Yazen

[2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Yazen Ghannam]

> -----Original Message-----
> From: Borislav Petkov <bp@alien8.de>
> Sent: Tuesday, August 21, 2018 8:16 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org;
> tony.luck@intel.com; x86@kernel.org
> Subject: Re: [PATCH 2/2] x86/MCE/AMD: Skip creating kobjects with NULL
> names
> 
> On Thu, Aug 16, 2018 at 06:46:33PM +0000, Ghannam, Yazen wrote:
> > Future SMCA systems may have banks whose MCA_IPID is not known so we
> 
> You lost me here. How would the IPID not be known? Are you talking about
> an old kernel running on new HW and thus the HardwareID in its MCA_IPID
> to be unknown to the old kernel?
> 

Yes, that's right: old kernel on new HW.

> > In the past, the sysfs entries were named something like "th_bank#" before
> > each bank was given a more descriptive name. We can go back to this format
> > as a fallback for unrecognized bank types. This will fix the issue with sysfs
> > failing and also the interface will still work on new hardware.
> 
> You could do: th_bank#<HardwareID> so that we can find what it is by
> grepping the PPR.
> 

Good idea. Will do.

Thanks,
Yazen

[2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Borislav Petkov]

On Thu, Aug 16, 2018 at 06:46:33PM +0000, Ghannam, Yazen wrote:
> Future SMCA systems may have banks whose MCA_IPID is not known so we

You lost me here. How would the IPID not be known? Are you talking about
an old kernel running on new HW and thus the HardwareID in its MCA_IPID
to be unknown to the old kernel?

> In the past, the sysfs entries were named something like "th_bank#" before
> each bank was given a more descriptive name. We can go back to this format
> as a fallback for unrecognized bank types. This will fix the issue with sysfs
> failing and also the interface will still work on new hardware.

You could do: th_bank#<HardwareID> so that we can find what it is by
grepping the PPR.

> I agree. I just wanted to make a small change and fix this issue. I
> think there's a lot we can do to clean up the thresholding code going
> forward.

ACK.

[2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Yazen Ghannam]

> -----Original Message-----
> From: linux-edac-owner@vger.kernel.org <linux-edac-owner@vger.kernel.org>
> On Behalf Of Borislav Petkov
> Sent: Wednesday, August 15, 2018 10:55 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org;
> tony.luck@intel.com; x86@kernel.org
> Subject: Re: [PATCH 2/2] x86/MCE/AMD: Skip creating kobjects with NULL
> names
> 
> On Thu, Aug 09, 2018 at 06:46:26PM +0000, Ghannam, Yazen wrote:
> > This patch makes it so that we don't fail init just because some banks don't
> > have names. The data caching we do is useful even if we fail to create sysfs
> > entries for some banks. The interrupt handler can work fine without a sysfs
> > entry for every bank. It seems like overkill to deallocate all the structures
> > and sysfs entries for all the banks just because we fail to create entries for
> > some banks that don't have names.
> 
> Maybe I'm missing the big picture here but why, all of a sudden, are
> some banks without names? This clearly is new because it wasn't like
> that before, so what is it? Maybe you should explain the bigger picture
> first.
> 

Yes, you're right. I'll update the commit message.

Before Fam17h, we went generations with only a few MCA banks and their
bank numbers and types were consistent. With Fam17h and SMCA we got a
whole new set of bank types and now these banks may not have the same
ordering between models. So we created names for the new banks and we
match the banks to a name based on values from the MCA_IPID register.

Future SMCA systems may have banks whose MCA_IPID is not known so we
don't match a name to them. This could happen if we have a minor revision
to a known bank, or if we have a completely new bank type.

> And if banks don't have names, then we should generate some for them
> instead. Because this code is already ugly and recursive - we certainly
> don't want to add more conditionals to it because that thing is a mess
> as it is now.
> 

This is a good idea.

In the past, the sysfs entries were named something like "th_bank#" before
each bank was given a more descriptive name. We can go back to this format
as a fallback for unrecognized bank types. This will fix the issue with sysfs
failing and also the interface will still work on new hardware.

I can do this and update the commit message in a v2 patch.

> > In other words, I think we should decouple the interrupt handler from the
> > sysfs interface. The interface is nice to have but not necessary for the HW
> > and OS to handle threshold interrupts. If we do so, then new HW with new,
> > unnamed types will work with older versions of Linux.
> 
> To tell you the truth, I'm not at all psyched about telling the future.
> We can try to be future-proof to a certain degree but this should not be
> the determining factor how we design things.
> 
> But the aspect of decoupling sysfs representation from handler makes
> sense. It just needs to be done cleanly.
> 

I agree. I just wanted to make a small change and fix this issue. I think there's
a lot we can do to clean up the thresholding code going forward.

Thanks,
Yazen

[2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Borislav Petkov]

On Thu, Aug 09, 2018 at 06:46:26PM +0000, Ghannam, Yazen wrote:
> This patch makes it so that we don't fail init just because some banks don't
> have names. The data caching we do is useful even if we fail to create sysfs
> entries for some banks. The interrupt handler can work fine without a sysfs
> entry for every bank. It seems like overkill to deallocate all the structures
> and sysfs entries for all the banks just because we fail to create entries for
> some banks that don't have names.

Maybe I'm missing the big picture here but why, all of a sudden, are
some banks without names? This clearly is new because it wasn't like
that before, so what is it? Maybe you should explain the bigger picture
first.

And if banks don't have names, then we should generate some for them
instead. Because this code is already ugly and recursive - we certainly
don't want to add more conditionals to it because that thing is a mess
as it is now.

> In other words, I think we should decouple the interrupt handler from the
> sysfs interface. The interface is nice to have but not necessary for the HW
> and OS to handle threshold interrupts. If we do so, then new HW with new,
> unnamed types will work with older versions of Linux.

To tell you the truth, I'm not at all psyched about telling the future.
We can try to be future-proof to a certain degree but this should not be
the determining factor how we design things.

But the aspect of decoupling sysfs representation from handler makes
sense. It just needs to be done cleanly.

[2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Borislav Petkov]

On Thu, Aug 09, 2018 at 09:08:34AM -0500, Yazen Ghannam wrote:
> From: Yazen Ghannam <yazen.ghannam@amd.com>
> 
> During mce_threshold_create_device() data structures are allocated for
> each CPUs MCA banks and thresholding blocks. These data structures are
> used to save information related to AMD's MCA Error Thresholding
> feature. The structures are used in the thresholding interrupt handler,
> and they are exposed to the user through sysfs. The sysfs interface has
> user-friendly names for each bank.
> 
> However, errors in mce_threshold_create_device() will cause all the data
> structures to be deallocated. This will break the thresholding interrupt
> handler since it depends on these structures.

Same argument as before: if our init fails in some fashion, we should
not be running the interrupt handler.

[2/2] x86/MCE/AMD: Skip creating kobjects with NULL names

[Yazen Ghannam]

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

During mce_threshold_create_device() data structures are allocated for
each CPUs MCA banks and thresholding blocks. These data structures are
used to save information related to AMD's MCA Error Thresholding
feature. The structures are used in the thresholding interrupt handler,
and they are exposed to the user through sysfs. The sysfs interface has
user-friendly names for each bank.

However, errors in mce_threshold_create_device() will cause all the data
structures to be deallocated. This will break the thresholding interrupt
handler since it depends on these structures.

One possible error is creating a kobject with a NULL name. This will
happen if a bank exists on a system that doesn't have a name, e.g. new
bank types on future systems.

Skip creating kobjects for banks without a name. This means that the
sysfs interface for this bank will not exist. But this will keep all the
data structures allocated, so the thresholding interrupt handler will
work, even for the unnamed bank. Also, the sysfs interface will still be
populated for all existing, known bank types.

Cc: <stable@vger.kernel.org> # 4.13.x
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
---
 arch/x86/kernel/cpu/mcheck/mce_amd.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce_amd.c b/arch/x86/kernel/cpu/mcheck/mce_amd.c
index 2dbf34250bbf..521fd8f406df 100644
--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -1130,6 +1130,7 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 	struct threshold_block *b = NULL;
 	u32 low, high;
 	int err;
+	const char *name = NULL;
 
 	if ((bank >= mca_cfg.banks) || (block >= NR_BLOCKS))
 		return 0;
@@ -1176,9 +1177,13 @@ static int allocate_threshold_blocks(unsigned int cpu, unsigned int bank,
 		per_cpu(threshold_banks, cpu)[bank]->blocks = b;
 	}
 
+	name = get_name(bank, b);
+	if (!name)
+		goto recurse;
+
 	err = kobject_init_and_add(&b->kobj, &threshold_ktype,
 				   per_cpu(threshold_banks, cpu)[bank]->kobj,
-				   get_name(bank, b));
+				   name);
 	if (err)
 		goto out_free;
 recurse:
@@ -1265,12 +1270,16 @@ static int threshold_create_bank(unsigned int cpu, unsigned int bank)
 		goto out;
 	}
 
+	if (!name)
+		goto allocate;
+
 	b->kobj = kobject_create_and_add(name, &dev->kobj);
 	if (!b->kobj) {
 		err = -EINVAL;
 		goto out_free;
 	}
 
+allocate:
 	per_cpu(threshold_banks, cpu)[bank] = b;
 
 	if (is_shared_bank(bank)) {