linux-kernel - Re: [PATCH v7 2/7] edac: synps: Add platform specific structures for ddrc controller

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]

Message-ID: <20180921090721.GB30439@nazgul.tnic>
Date:   Fri, 21 Sep 2018 11:07:21 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Manish Narani <MNARANI@...inx.com>
Cc:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        Michal Simek <michals@...inx.com>,
        "leoyang.li@....com" <leoyang.li@....com>,
        "sudeep.holla@....com" <sudeep.holla@....com>,
        "amit.kucheria@...aro.org" <amit.kucheria@...aro.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v7 2/7] edac: synps: Add platform specific structures for
 ddrc controller

On Wed, Sep 19, 2018 at 01:33:58PM +0000, Manish Narani wrote:
> Apart from this one, I have covered all the comments from the previous review.

Are you sure?

Let's see. I said:

| Kill all that "function pointer" fluff. Here's how I've changed it:
| 
| /**
|  * struct synps_platform_data -  synps platform data structure
|  * @edac_geterror_info: edac error info
|  * @edac_get_mtype:     get mtype
|  * @edac_get_dtype:     get dtype
|  * @edac_get_eccstate:  get ECC state
			  ^^^^^^^^^^^^^

This is supposed to denote that this function returns whether ECC
checking is enabled on the controller or not.

Your patch has:

+ * struct synps_platform_data -  synps platform data structure.
+ * @geterror_info:     Get error info.
+ * @get_mtype:         Get mtype.
+ * @get_dtype:         Get dtype.
+ * @get_eccstate:      Get eccstate.

So what is "eccstate"? Is it a struct or a variable or ...?

Do you see my point?

I know, it is a small thing but documenting our code properly is
something which people would be thanking us for. Even you will be
thanking yourself when you look at this months from now after having
forgotten it all.

Please check the rest of your additions for similar discrepancies.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--