On Wed, Jun 26, 2024 at 12:24:20PM -0500, Naik, Avadhut wrote:

On 6/26/2024 06:10, Borislav Petkov wrote:

quoted

On Tue, Jun 25, 2024 at 02:56:22PM -0500, Avadhut Naik wrote:

quoted

AMD's Scalable MCA systems viz. Genoa will include two new registers:

"viz."?

Right. Will mention Zen4 instead of Genoa.

I still don't know what "viz." means...

Yes, I catch your drift. Will reword the commit message to explain that the
new syndrome registers are going to be exported through the tracepoint
in a dynamic array, as they are vendor-specific, so that usersapce error
decoding tools can retrieve the supplemental error information within them.

Again, why?

Why is it important to have them in the tracepoint?

quoted

quoted

Note: Checkpatch warnings/errors are ignored to maintain coding style.

This goes...

quoted

[Yazen: Drop Yazen's Co-developed-by tag and moved SoB tag.]

Yes, you did but now your SOB chain is wrong:

quoted

Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>

This tells me Avadhut is the author, Yazen handled it and he's sending it to
me. But nope, he isn't. So it needs another Avadhut SOB underneath.

Audit all patches pls.

Wasn't aware of this chronology. Thanks for this information!

Well, there's documentation for that which you should've read already, before
sending patches:

https://kernel.org/doc/html/latest/process/development-process.html

and

https://kernel.org/doc/html/latest/process/submitting-patches.html

especially.

So, IIUC, the sequence for this patch should be as follows?

Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>
Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
Signed-off-by: Avadhut Naik <avadhut.naik@amd.com>

Yes, now I leave it to you to explain why. Hint: it is in those docs above.

quoted

quoted

---

... right under those three "---" as such notes do not belong in the commit
message. Remember that for the future.

Okay. Will move the note here.

Or remove it completely. checkpatch is crap - I know. No need to have it in
every patch.

Had considered this. But struct mce_hw_err *err wouldn't really be used in
mce_read_aux() in patch 1. Only struct mce m, which is already available, will
be used.

So?

Hence, deferred the change to this patch where usage of struct mce_hw_err *err
is actually introduced in mce_read_aux().

Do you prefer having this change in patch 1 instead?

I prefer a patch to contain one logical and complete change only. Because this
makes review easier. You should try reviewing patches sometimes too and you'll
know.

quoted

So that vendor data layout - is that ABI too? Or are we free to shuffle the
fields around in the future or even remove some?

This all needs to be specified somewhere explicitly so that nothing relies on
that layout.

And I'm not sure that that's enough because when userspace tools start using
them, then they're practically an ABI so you can't change them even if you
wanted to.

So is libtraceevent or all the other libraries going to parse this as a blob
and it is always going to remain such?

But then the tools which interpret it need to know its layout and if it
changes, perhaps check kernel version which then becomes RealUgly(tm).

So you might just as well dump the separate fields one by one, without
a dynamic array.

Or do a dynamic array but specify that their layout in struct
mce_hw_er.vendor.amd are cast in stone so that we're all clear on what goes
where.

Questions over questions...

Should we document this where struct mce_hw_err is defined, in
arch/x86/include/asm/mce.h? Or do you have any other recommendations?

I don't know. If I knew I wouldn't have questions which you can read again and
try to answer.

-- 
Regards/Gruss,
    Boris.

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