From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============4849006276304628654==" MIME-Version: 1.0 From: James Morse Subject: Re: [Devel] [PATCH v6 3/7] acpi: apei: remove the unused code Date: Fri, 08 Sep 2017 19:17:30 +0100 Message-ID: <59B2DEBA.6030608@arm.com> In-Reply-To: 45c69f99-61dd-e847-368b-20acb61b4d50@huawei.com List-ID: To: devel@acpica.org --===============4849006276304628654== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable 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 neede= d for >> some future patch you haven't included. > = > James, let check the code, when the ghes_probe, if the CONFIG_ACPI_APEI_S= EA is not defined. > it will return -ENOTSUPP and goto error, and the ghes_sea_add has no chan= ce to execute. > similar, if the probe is failed, it should not have chance to execute ghe= s_sea_remove. It's the 'unnecessary handling when CONFIG_ACPI_APEI_SEI' in the commit mes= sage that confuses me: this patch doesn't reference that Kconfig symbol. I guess= that sentence needs removing for this 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 su= pport 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. (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 meth= od.) >> Setting NOTIFY_SEI as the GHES entry's notification type means the OS sh= ould >> check the GHES->ErrorStatusAddress for CPER records when it receives an >> SError-Interrupt, as it may be a notification of an error from this erro= r source. > previously I added the NOTIFY_SEI support, (Yes, I saw that in v5 and expected this series to add some APEI support co= de ) > 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 discuss= ion on the cover-letter. >> If you aren't handling the notification, why is this is in the HEST at a= ll? >> (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 lea= rn 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. Thanks, James [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Doc= umentation/admin-guide/README.rst#n251 --===============4849006276304628654==-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: James Morse Subject: Re: [PATCH v6 3/7] acpi: apei: remove the unused code Date: Fri, 08 Sep 2017 19:17:30 +0100 Message-ID: <59B2DEBA.6030608@arm.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from localhost (localhost [127.0.0.1]) by mm01.cs.columbia.edu (Postfix) with ESMTP id C9EEB49C65 for ; Fri, 8 Sep 2017 14:16:35 -0400 (EDT) Received: from mm01.cs.columbia.edu ([127.0.0.1]) by localhost (mm01.cs.columbia.edu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 3C31lJCgppR5 for ; Fri, 8 Sep 2017 14:16:34 -0400 (EDT) Received: from foss.arm.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by mm01.cs.columbia.edu (Postfix) with ESMTP id 7524A49C5D for ; Fri, 8 Sep 2017 14:16:34 -0400 (EDT) In-Reply-To: <45c69f99-61dd-e847-368b-20acb61b4d50@huawei.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kvmarm-bounces@lists.cs.columbia.edu Sender: kvmarm-bounces@lists.cs.columbia.edu To: gengdongjiu Cc: wangkefeng.wang@huawei.com, kvm@vger.kernel.org, david.daney@cavium.com, catalin.marinas@arm.com, tbaicar@codeaurora.org, will.deacon@arm.com, linuxarm@huawei.com, robert.moore@intel.com, lv.zheng@intel.com, zjzhang@codeaurora.org, mingo@kernel.org, stefan@hello-penguin.com, linux@armlinux.org.uk, kvmarm@lists.cs.columbia.edu, linux-acpi@vger.kernel.org, huangshaoyu@huawei.com, huangdaode@hisilicon.com, bp@suse.de, Dave.Martin@arm.com, lenb@kernel.org, wuquanming@huawei.com, marc.zyngier@arm.com, john.garry@huawei.com, kristina.martsenko@arm.com, cov@codeaurora.org, jonathan.cameron@huawei.com, zhengqiang10@huawei.com, linux-arm-kernel@lists.infradead.org, devel@acpica.org, ard.biesheuvel@linaro.org, linux-kernel@vger.kernel.org, wangzhou1@hisilicon.com, mst@redhat.com, shiju.jose@huawei.com List-Id: kvmarm@lists.cs.columbia.edu 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? 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. (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.) >> 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. >> 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. Thanks, James [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/README.rst#n251 From mboxrd@z Thu Jan 1 00:00:00 1970 From: james.morse@arm.com (James Morse) Date: Fri, 08 Sep 2017 19:17:30 +0100 Subject: [PATCH v6 3/7] acpi: apei: remove the unused code In-Reply-To: <45c69f99-61dd-e847-368b-20acb61b4d50@huawei.com> 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> Message-ID: <59B2DEBA.6030608@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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? 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. (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.) >> 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. >> 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. Thanks, James [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/README.rst#n251 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756699AbdIHSTG (ORCPT ); Fri, 8 Sep 2017 14:19:06 -0400 Received: from foss.arm.com ([217.140.101.70]:43912 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756210AbdIHSTE (ORCPT ); Fri, 8 Sep 2017 14:19:04 -0400 Message-ID: <59B2DEBA.6030608@arm.com> Date: Fri, 08 Sep 2017 19:17:30 +0100 From: James Morse User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.6.0 MIME-Version: 1.0 To: gengdongjiu CC: christoffer.dall@linaro.org, marc.zyngier@arm.com, rkrcmar@redhat.com, linux@armlinux.org.uk, catalin.marinas@arm.com, will.deacon@arm.com, lenb@kernel.org, robert.moore@intel.com, lv.zheng@intel.com, mark.rutland@arm.com, xiexiuqi@huawei.com, cov@codeaurora.org, david.daney@cavium.com, suzuki.poulose@arm.com, stefan@hello-penguin.com, Dave.Martin@arm.com, kristina.martsenko@arm.com, wangkefeng.wang@huawei.com, tbaicar@codeaurora.org, ard.biesheuvel@linaro.org, mingo@kernel.org, bp@suse.de, shiju.jose@huawei.com, zjzhang@codeaurora.org, linux-arm-kernel@lists.infradead.org, kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, devel@acpica.org, mst@redhat.com, john.garry@huawei.com, jonathan.cameron@huawei.com, shameerali.kolothum.thodi@huawei.com, huangdaode@hisilicon.com, wangzhou1@hisilicon.com, huangshaoyu@huawei.com, wuquanming@huawei.com, linuxarm@huawei.com, zhengqiang10@huawei.com Subject: Re: [PATCH v6 3/7] acpi: apei: remove the unused code 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> In-Reply-To: <45c69f99-61dd-e847-368b-20acb61b4d50@huawei.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? 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. (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.) >> 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. >> 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. Thanks, James [0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/admin-guide/README.rst#n251