Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S968967AbdDSSbe (ORCPT ); Wed, 19 Apr 2017 14:31:34 -0400 Received: from mail.skyhub.de ([5.9.137.197]:37068 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S967519AbdDSSbb (ORCPT ); Wed, 19 Apr 2017 14:31:31 -0400 Date: Wed, 19 Apr 2017 20:31:12 +0200 From: Borislav Petkov To: Tyler Baicar Cc: christoffer.dall@linaro.org, marc.zyngier@arm.com, pbonzini@redhat.com, rkrcmar@redhat.com, linux@armlinux.org.uk, catalin.marinas@arm.com, will.deacon@arm.com, rjw@rjwysocki.net, lenb@kernel.org, matt@codeblueprint.co.uk, robert.moore@intel.com, lv.zheng@intel.com, nkaje@codeaurora.org, zjzhang@codeaurora.org, mark.rutland@arm.com, james.morse@arm.com, akpm@linux-foundation.org, eun.taik.lee@samsung.com, sandeepa.s.prabhu@gmail.com, labbott@redhat.com, shijie.huang@arm.com, rruigrok@codeaurora.org, paul.gortmaker@windriver.com, tn@semihalf.com, fu.wei@linaro.org, rostedt@goodmis.org, bristot@redhat.com, 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, linux-efi@vger.kernel.org, devel@acpica.org, Suzuki.Poulose@arm.com, punit.agrawal@arm.com, astone@redhat.com, harba@codeaurora.org, hanjun.guo@linaro.org, john.garry@huawei.com, shiju.jose@huawei.com, joe@perches.com, rafael@kernel.org, tony.luck@intel.com, gengdongjiu@huawei.com, xiexiuqi@huawei.com Subject: Re: [PATCH V15 01/11] acpi: apei: read ack upon ghes record consumption Message-ID: <20170419183112.x7tmjzpoq7ds64s2@pd.tnic> References: <1492556723-9189-1-git-send-email-tbaicar@codeaurora.org> <1492556723-9189-2-git-send-email-tbaicar@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1492556723-9189-2-git-send-email-tbaicar@codeaurora.org> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5923 Lines: 192 On Tue, Apr 18, 2017 at 05:05:13PM -0600, Tyler Baicar wrote: > A RAS (Reliability, Availability, Serviceability) controller > may be a separate processor running in parallel with OS > execution, and may generate error records for consumption by > the OS. If the RAS controller produces multiple error records, > then they may be overwritten before the OS has consumed them. > > The Generic Hardware Error Source (GHES) v2 structure > introduces the capability for the OS to acknowledge the > consumption of the error record generated by the RAS > controller. A RAS controller supporting GHESv2 shall wait for > the acknowledgment before writing a new error record, thus > eliminating the race condition. > > Add support for parsing of GHESv2 sub-tables as well. > > Signed-off-by: Tyler Baicar > CC: Jonathan (Zhixiong) Zhang > Reviewed-by: James Morse > --- > drivers/acpi/apei/ghes.c | 55 +++++++++++++++++++++++++++++++++++++++++++++--- > drivers/acpi/apei/hest.c | 7 ++++-- > include/acpi/ghes.h | 5 ++++- > 3 files changed, 61 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 79b3c9c..6d87ab7 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -46,6 +46,7 @@ > #include > #include > > +#include > #include > #include > #include > @@ -80,6 +81,10 @@ > ((struct acpi_hest_generic_status *) \ > ((struct ghes_estatus_node *)(estatus_node) + 1)) > > +#define IS_HEST_TYPE_GENERIC_V2(ghes) \ > + ((struct acpi_hest_header *)ghes->generic)->type == \ This is a nasty hack: casting the ghes->generic pointer to a pointer of its first member which is a acpi_hest_header. Why isn't this a nice inline function with proper dereferencing: static inline bool is_hest_type_generic_v2(struct ghes *ghes) { return ghes->generic->header.type == ACPI_HEST_TYPE_GENERIC_ERROR_V2; } ? Also, please integrate scripts/checkpatch.pl in your patch creation workflow. Some of the warnings/errors *actually* make sense. > /* > * This driver isn't really modular, however for the time being, > * continuing to use module_param is the easiest way to remain > @@ -240,6 +245,17 @@ static int ghes_estatus_pool_expand(unsigned long len) > return 0; > } > > +static int map_gen_v2(struct ghes *ghes) > +{ > + return apei_map_generic_address(&ghes->generic_v2->read_ack_register); > +} > + > +static void unmap_gen_v2(struct ghes *ghes) > +{ > + apei_unmap_generic_address(&ghes->generic_v2->read_ack_register); > + return; > +} Like this one, for example: WARNING: void function return statements are not generally useful #89: FILE: drivers/acpi/apei/ghes.c:257: + return; +} > + > static struct ghes *ghes_new(struct acpi_hest_generic *generic) > { > struct ghes *ghes; > @@ -249,10 +265,17 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic) > ghes = kzalloc(sizeof(*ghes), GFP_KERNEL); > if (!ghes) > return ERR_PTR(-ENOMEM); > + > ghes->generic = generic; > + if (IS_HEST_TYPE_GENERIC_V2(ghes)) { > + rc = map_gen_v2(ghes); > + if (rc) > + goto err_free; > + } > + > rc = apei_map_generic_address(&generic->error_status_address); > if (rc) > - goto err_free; > + goto err_unmap_read_ack_addr; > error_block_length = generic->error_block_length; > if (error_block_length > GHES_ESTATUS_MAX_SIZE) { > pr_warning(FW_WARN GHES_PFX > @@ -264,13 +287,16 @@ static struct ghes *ghes_new(struct acpi_hest_generic *generic) > ghes->estatus = kmalloc(error_block_length, GFP_KERNEL); > if (!ghes->estatus) { > rc = -ENOMEM; > - goto err_unmap; > + goto err_unmap_status_addr; > } > > return ghes; > > -err_unmap: > +err_unmap_status_addr: > apei_unmap_generic_address(&generic->error_status_address); > +err_unmap_read_ack_addr: > + if (IS_HEST_TYPE_GENERIC_V2(ghes)) > + unmap_gen_v2(ghes); > err_free: > kfree(ghes); > return ERR_PTR(rc); > @@ -280,6 +306,8 @@ static void ghes_fini(struct ghes *ghes) > { > kfree(ghes->estatus); > apei_unmap_generic_address(&ghes->generic->error_status_address); > + if (IS_HEST_TYPE_GENERIC_V2(ghes)) > + unmap_gen_v2(ghes); > } > > static inline int ghes_severity(int severity) > @@ -649,6 +677,21 @@ static void ghes_estatus_cache_add( > rcu_read_unlock(); > } > > +static int ghes_ack_error(struct acpi_hest_generic_v2 *generic_v2) If you name this function parameter to something shorter, say gv2, for example... > +{ > + int rc; > + u64 val = 0; > + > + rc = apei_read(&val, &generic_v2->read_ack_register); > + if (rc) > + return rc; > + > + val &= generic_v2->read_ack_preserve << generic_v2->read_ack_register.bit_offset; > + val |= generic_v2->read_ack_write << generic_v2->read_ack_register.bit_offset; ... you can align those two nicely while remaining within the 80 cols width: val &= gv2->read_ack_preserve << gv2->read_ack_register.bit_offset; val |= gv2->read_ack_write << gv2->read_ack_register.bit_offset; and make them readable at a quick glance. > + > + return apei_write(val, &generic_v2->read_ack_register); > +} > + > static int ghes_proc(struct ghes *ghes) > { > int rc; > @@ -661,6 +704,12 @@ static int ghes_proc(struct ghes *ghes) > ghes_estatus_cache_add(ghes->generic, ghes->estatus); > } > ghes_do_proc(ghes, ghes->estatus); This needs a comment why v2 needs to ACK the error. The commit message is not necessarily something we'll find quickly in the future. > + > + if (IS_HEST_TYPE_GENERIC_V2(ghes)) { > + rc = ghes_ack_error(ghes->generic_v2); > + if (rc) > + return rc; > + } > out: > ghes_clear_estatus(ghes); > return rc; -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.