linux-kernel - Re: [PATCH v9 2/2] EDAC: add EDAC driver for DMC520

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 for Android: free password hash cracker in your pocket

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

Message-ID: <d5989a4c-8173-2f03-7d20-6fdd32d19591@gmail.com>
Date:   Fri, 17 Jan 2020 10:31:18 -0800
From:   Shiping Ji <shiping.linux@...il.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     James Morse <james.morse@....com>, robh+dt@...nel.org,
        mark.rutland@....com, devicetree@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>, linux-edac@...r.kernel.org,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        sashal@...nel.org, hangl@...rosoft.com,
        Lei Wang <lewan@...rosoft.com>, shji@...rosoft.com,
        ruizhao@...rosoft.com, Scott Branden <scott.branden@...adcom.com>,
        Yuqing Shen <yuqing.shen@...adcom.com>, ray.jui@...adcom.com,
        wangglei@...il.com
Subject: Re: [PATCH v9 2/2] EDAC: add EDAC driver for DMC520

On 1/16/2020 4:18 PM, Borislav Petkov wrote:

>> +/* The EDAC driver private data */
>> +struct dmc520_edac {
>> +	void __iomem *reg_base;
>> +	spinlock_t ecc_lock;
> 
> What does that spinlock protect? Also, its name is not very optimal.

This is to protect concurrent writes to the mci->error_desc as suggested by James when reviewing the patch v3.
 
>> +	reg_offset_low = is_ce ? REG_OFFSET_DRAM_ECC_ERRC_INT_INFO_31_00 :
>> +				 REG_OFFSET_DRAM_ECC_ERRD_INT_INFO_31_00;
>> +	reg_offset_high = is_ce ? REG_OFFSET_DRAM_ECC_ERRC_INT_INFO_63_32 :
>> +				  REG_OFFSET_DRAM_ECC_ERRD_INT_INFO_63_32;
> 
> Those define names could be shorter.

I'm trying to find a good scheme to make them shorter, at the moment they are named according to the TRM.
 
>> +		if (irq >= 0) {
>> +			ret = devm_request_irq(&pdev->dev, irq,
>> +							dmc520_isr, IRQF_SHARED,
>> +							dev_name(&pdev->dev), mci);
> 
> Align arguments on the opening brace.

I'm not sure how this can be done perfectly with tabs only :)

All other comments have been addressed in the next patch, many thanks!

-- 
Best regards,
Shiping Ji