Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751778AbdIKMJW (ORCPT ); Mon, 11 Sep 2017 08:09:22 -0400 Received: from szxga05-in.huawei.com ([45.249.212.191]:6024 "EHLO szxga05-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751118AbdIKMJU (ORCPT ); Mon, 11 Sep 2017 08:09:20 -0400 Subject: Re: [PATCH v6 3/7] acpi: apei: remove the unused code To: James Morse References: <1503916701-13516-1-git-send-email-gengdongjiu@huawei.com> <1503916701-13516-4-git-send-email-gengdongjiu@huawei.com> <59A84C51.3000506@arm.com> <45c69f99-61dd-e847-368b-20acb61b4d50@huawei.com> <59B2DEBA.6030608@arm.com> CC: , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , From: gengdongjiu Message-ID: Date: Mon, 11 Sep 2017 20:04:21 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <59B2DEBA.6030608@arm.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.142.68.147] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A090205.59B67BDF.00D2,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 6aea827d4abb4629158d40f81c60ee8e Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3385 Lines: 91 James, Thanks for the review. On 2017/9/9 2:17, James Morse wrote: > Hi gengdongjiu, > > On 04/09/17 12:43, gengdongjiu wrote: >> On 2017/9/1 1:50, James Morse wrote: >>> On 28/08/17 11:38, Dongjiu Geng wrote: >>>> In current code logic, the two functions ghes_sea_add() and >>>> ghes_sea_remove() are only called when CONFIG_ACPI_APEI_SEA >>>> is defined. If not, it will return errors in the ghes_probe() >>>> and not contiue. Hence, remove the unnecessary handling when >>>> CONFIG_ACPI_APEI_SEI is not defined. >>> >>> This doesn't match what the patch does. I get this feeling this is needed for >>> some future patch you haven't included. >> >> James, let check the code, when the ghes_probe, if the CONFIG_ACPI_APEI_SEA is not defined. >> it will return -ENOTSUPP and goto error, and the ghes_sea_add has no chance to execute. >> similar, if the probe is failed, it should not have chance to execute ghes_sea_remove. > > It's the 'unnecessary handling when CONFIG_ACPI_APEI_SEI' in the commit message > that confuses me: this patch doesn't reference that Kconfig symbol. I guess that > sentence needs removing for this v6? thanks for the pointing out, That needs to be removed for v6. > > Re-reading without that part of the commit-message: > > You're relying on the compiler's dead-code elimination to spot unused static > functions and silently drop them. Great! > (there is the small risk that gcc 3.2[0] can't do this, x86 still has to support > this gcc version) > > As this is just clean-up patch can you break it out of this series, it isn't > needed to add support for SEI. sure, I will. > > (This series adds support for what should be an APEI notification, but the only > code that touches APEI removes some code from a different notification method.) understand. > > >>> Setting NOTIFY_SEI as the GHES entry's notification type means the OS should >>> check the GHES->ErrorStatusAddress for CPER records when it receives an >>> SError-Interrupt, as it may be a notification of an error from this error source. > >> previously I added the NOTIFY_SEI support, > > (Yes, I saw that in v5 and expected this series to add some APEI support code ) > > >> but consider the error address in CPER is not accurate and calling memory_failure() may not make sense. >> so I remove it. > > 'not accurate'... this is going to be a problem, but lets keep that discussion > on the cover-letter. Ok. > > >>> If you aren't handling the notification, why is this is in the HEST at all? >>> (and if its not: its not firmware-first) > >> For the SEI notification, may be we can parse and handle the CPER record other than the Error physical address > > Sure, but I only see this cleanup patch in this series, where does APEI learn > about NOTIFY_SEI? As this is nothing will ever touch those CPER records, if > you're using GHESv2 firmware will be prevented from delivering subsequent > notifications. James, whether it is possible you can review the previous v5 patch which adds the support for NOTIFY_SEI? thanks in advancecIn that patch, I share the SEI notification handling with the SEA notification handling to avoid duplicated code. https://www.spinics.net/lists/arm-kernel/msg601767.html > > > Thanks, > > James > > [0] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/README.rst#n251 > > . >