Date:   Fri, 3 Dec 2021 23:17:45 +0100
From:   Borislav Petkov <bp@alien8.de>
To:     Yazen Ghannam <yazen.ghannam@amd.com>
Cc:     linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org,
        tony.luck@intel.com, x86@kernel.org,
        Smita.KoralahalliChannabasappa@amd.com, mukul.joshi@amd.com,
        alexander.deucher@amd.com, william.roche@oracle.com
Subject: Re: [PATCH 1/3] x86/MCE/AMD: Provide an "Unknown" MCA bank type
Message-ID: <YaqXiVjNLINxwz8G@zn.tnic>
References: <20211203020017.728440-1-yazen.ghannam@amd.com>
 <20211203020017.728440-2-yazen.ghannam@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20211203020017.728440-2-yazen.ghannam@amd.com>
Precedence: bulk

On Fri, Dec 03, 2021 at 02:00:15AM +0000, Yazen Ghannam wrote:
> The AMD MCA Thresholding sysfs interface populates directories for each
> bank and thresholding block. The name used for each directory is looked
> up in a table of known bank types. However, new bank types won't match
> in this list and will return NULL for the name. This will cause the
> machinecheck sysfs interface to fail to be populated.
> 
> Set new and unknown MCA bank types to the "unknown" type. Also,
> ensure that the bank's thresholding block directories have unique names.
> This will ensure that the machinecheck sysfs interface can be
> initialized.

What is the advantage of having a sysfs directory structure headed with
an "unknown" entry vs not having that structure at all when the kernel
runs on a machine for which it has not been enabled yet?

IOW, if those new banks would need additional enablement, what's the
point of having "unknown" on older kernels which do not have any
functionality?

IOW, how does this:

/sys/devices/system/machinecheck/machinecheck0/unknown/unknown/
├── error_count
├── interrupt_enable
└── threshold_limit

help a user?

Btw, looking at the current layout:

...
├── insn_fetch
│   └── insn_fetch
│       ├── error_count
│       ├── interrupt_enable
│       └── threshold_limit
├── l2_cache
│   └── l2_cache
│       ├── error_count
│       ├── interrupt_enable
│       └── threshold_limit
...

we have those names repeated which looks wonky and useless too. I'd
expect them to be:

...
├── insn_fetch
│   ├── error_count
│   ├── interrupt_enable
│   └── threshold_limit
├── l2_cache
│   ├── error_count
│   ├── interrupt_enable
│   └── threshold_limit
...

Can we fix that too pls?

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette