From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A1D12C433E1 for ; Fri, 21 Aug 2020 13:00:41 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 6C382207BB for ; Fri, 21 Aug 2020 13:00:41 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6C382207BB Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=Huawei.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id D0A4E8D0003; Fri, 21 Aug 2020 09:00:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id CBAE98D0002; Fri, 21 Aug 2020 09:00:40 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B5C3F8D0003; Fri, 21 Aug 2020 09:00:40 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0218.hostedemail.com [216.40.44.218]) by kanga.kvack.org (Postfix) with ESMTP id 9575D8D0002 for ; Fri, 21 Aug 2020 09:00:40 -0400 (EDT) Received: from smtpin02.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 37ED1180AD837 for ; Fri, 21 Aug 2020 13:00:39 +0000 (UTC) X-FDA: 77174584998.02.star98_111354227039 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin02.hostedemail.com (Postfix) with ESMTP id 76C8410097AA3 for ; Fri, 21 Aug 2020 13:00:36 +0000 (UTC) X-HE-Tag: star98_111354227039 X-Filterd-Recvd-Size: 5897 Received: from huawei.com (lhrrgout.huawei.com [185.176.76.210]) by imf35.hostedemail.com (Postfix) with ESMTP for ; Fri, 21 Aug 2020 13:00:35 +0000 (UTC) Received: from lhreml710-chm.china.huawei.com (unknown [172.18.7.108]) by Forcepoint Email with ESMTP id EE9029E83CF50866E9B6; Fri, 21 Aug 2020 14:00:33 +0100 (IST) Received: from localhost (10.52.123.86) by lhreml710-chm.china.huawei.com (10.201.108.61) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1913.5; Fri, 21 Aug 2020 14:00:33 +0100 Date: Fri, 21 Aug 2020 13:59:01 +0100 From: Jonathan Cameron To: Bjorn Helgaas CC: , , , , Lorenzo Pieralisi , Bjorn Helgaas , , , Ingo Molnar , Thomas Gleixner , , Dan Williams , "Brice Goglin" , Sean V Kelley , , Keith Busch Subject: Re: [PATCH v9 4/6] ACPI: HMAT: Fix handling of changes from ACPI 6.2 to ACPI 6.3 Message-ID: <20200821135901.0000260b@Huawei.com> In-Reply-To: <20200821121356.GA1616281@bjorn-Precision-5520> References: <20200821094258.00007925@Huawei.com> <20200821121356.GA1616281@bjorn-Precision-5520> Organization: Huawei Technologies Research and Development (UK) Ltd. X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; i686-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.52.123.86] X-ClientProxiedBy: lhreml701-chm.china.huawei.com (10.201.108.50) To lhreml710-chm.china.huawei.com (10.201.108.61) X-CFilter-Loop: Reflected X-Rspamd-Queue-Id: 76C8410097AA3 X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam04 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, 21 Aug 2020 07:13:56 -0500 Bjorn Helgaas wrote: > [+cc Keith, author of 3accf7ae37a9 ("acpi/hmat: Parse and report > heterogeneous memory")] > > On Fri, Aug 21, 2020 at 09:42:58AM +0100, Jonathan Cameron wrote: > > On Thu, 20 Aug 2020 17:21:29 -0500 > > Bjorn Helgaas wrote: > > > > > On Wed, Aug 19, 2020 at 10:51:09PM +0800, Jonathan Cameron wrote: > > > > In ACPI 6.3, the Memory Proximity Domain Attributes Structure > > > > changed substantially. One of those changes was that the flag > > > > for "Memory Proximity Domain field is valid" was deprecated. > > > > > > > > This was because the field "Proximity Domain for the Memory" > > > > became a required field and hence having a validity flag makes > > > > no sense. > > > > > > > > So the correct logic is to always assume the field is there. > > > > Current code assumes it never is. > > > > > > > > Signed-off-by: Jonathan Cameron > > > > --- > > > > drivers/acpi/numa/hmat.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/acpi/numa/hmat.c b/drivers/acpi/numa/hmat.c > > > > index 2c32cfb72370..07cfe50136e0 100644 > > > > --- a/drivers/acpi/numa/hmat.c > > > > +++ b/drivers/acpi/numa/hmat.c > > > > @@ -424,7 +424,7 @@ static int __init hmat_parse_proximity_domain(union acpi_subtable_headers *heade > > > > pr_info("HMAT: Memory Flags:%04x Processor Domain:%u Memory Domain:%u\n", > > > > p->flags, p->processor_PD, p->memory_PD); > > > > > > > > - if (p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) { > > > > + if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || hmat_revision == 2) { > > > > > > I hope/assume the spec is written in such a way that p->memory_PD is > > > required for any revision > 1? So maybe this should be: > > > > > > if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID && hmat_revision == 1) || > > > hmat_revision > 1) { > > I should have said simply: > > if (hmat_revision == 1 && p->flags & ACPI_HMAT_MEMORY_PD_VALID) > > We shouldn't even test p->flags for ACPI_HMAT_MEMORY_PD_VALID unless > we already know it's revision 1. > > And unless there was a revision 0 of HMAT, there's no need to look for > hmat_revison > 1. It needs to stay as an or statement as you had the first time. The field is always valid for hmat_revision > 1, and valid for hmat_revision == 1 with the flag set. You could express it as if ((p->flags & ACPI_HMAT_MEMORY_PD_VALID) || (hmat_revision != 1)) but that seems more confusing to me. > > > Good point. We have existing protections elsewhere against > > hmat_revision being anything other than 1 or 2, so we should aim to > > keep that in only one place. > > I think the "Ignoring HMAT: Unknown revision" test in hmat_init(), > added by 3accf7ae37a9 ("acpi/hmat: Parse and report heterogeneous > memory"), is a mistake. > > And I think hmat_normalize() has a similar mistake in that it tests > explicitly for hmat_revision == 2 when it should accept 2 AND anything > later. > > We should assume that future spec revisions will be backwards > compatible. Otherwise we're forced to make kernel changes when we > otherwise would not have to. I disagree with this. There is no rule in ACPI about maintaining backwards compatibility. The assumption is that the version number will always be checked. The meaning of fields changed between version 1 and version 2 so it would be bold to assume that won't happen in the future! HMAT is an optional table, so if someone boots up an old kernel they are probably better off failing to use it at all than misinterpreting it. Having the sanity check in one place makes sense, but removing it entirely is a bad idea. Jonathan > > Bjorn