linux-kernel - Re: [PATCH v2 1/3] ghes_edac: unify memory error report format with cper

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 31 Dec 2021 11:04:08 +0800
From:   Shuai Xue <xueshuai@...ux.alibaba.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     mchehab@...nel.org, tony.luck@...el.com, james.morse@....com,
        rric@...nel.org, ardb@...nel.org, linux-edac@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-efi@...r.kernel.org,
        zhangliguang@...ux.alibaba.com, zhuo.song@...ux.alibaba.com
Subject: Re: [PATCH v2 1/3] ghes_edac: unify memory error report format with
 cper

Hi, Borislav,

Thank you for your comments.

在 2021/12/31 AM1:57, Borislav Petkov 写道:
> On Wed, Dec 29, 2021 at 11:22:11AM +0800, Shuai Xue wrote:
>> Yep, these fields are unpopulated by BIOS, I manually enable all Validation
>> Bits for debug so that we see the difference more clearly. I will declare it
>> in next version.
> 
> Declare what? I can't parse your statement.

The ghes_edac log message is printed only when a validation bit is set, e.g.:

	if (mem_err->validation_bits & CPER_MEM_VALID_NODE)
		p += sprintf(p, "node:%d ", mem_err->node);

Not all bits are populated by BIOS in my platform, I manually enable all
validation bits during test so that we can see log message and differences of all
fields more clearly.

	+ 	mem_err->validation_bits = 0xfffffffffffffff;

>> Well, the purpose is not to improve but unify.
> 
> The most importang goal with kernel code is improvement and less bugs.
> Unification is second. We should not jump through hoops and unify at
> every price just because there's a duplicated function somewhere.
> Remember that when doing your changes.

I see. Thank you.

>> Well, Robert suggested me add a unification patch[1] so that we could review
>> the changes more clearly. I think it makes sense.
> 
> Not really. I can imagine why Robert suggested that but this strategy is
> causing unnecessary churn. What one usually does in such cases is:
> 
> 1. Add changes to the target functionality - the one in cper.c - by
> explaining *why* those changes are needed.
> 
> 2. Switch ghes_edac.c to that functionality and remove the redundant one
> there.
> 
> Simple and clean diffstat and easy review.
> 
> Thx.

Got it. I will send next version latter.

Merry Christmas and happy New Year.

Best Regards,
Shuai