From:   Fan Ni <fan.ni@samsung.com>
To:     "alison.schofield@intel.com" <alison.schofield@intel.com>,
        "vishal.l.verma@intel.com" <vishal.l.verma@intel.com>,
        "ira.weiny@intel.com" <ira.weiny@intel.com>,
        "bwidawsk@kernel.org" <bwidawsk@kernel.org>,
        "dan.j.williams@intel.com" <dan.j.williams@intel.com>,
        "Jonathan.Cameron@huawei.com" <Jonathan.Cameron@huawei.com>
CC:     "linux-cxl@vger.kernel.org" <linux-cxl@vger.kernel.org>,
        Adam Manzanares <a.manzanares@samsung.com>,
        "dave@stgolabs.net" <dave@stgolabs.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Fan Ni <fan.ni@samsung.com>
Subject: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check
Thread-Topic: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field
        check
Thread-Index: AQHZS8WnqoF9q+YY/UCqLavADTGrLQ==
Date:   Tue, 28 Feb 2023 22:40:28 +0000
Message-ID: <20230228224014.1402545-1-fan.ni@samsung.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
CMS-TYPE: 301P
References: <CGME20230228224029uscas1p1e2fb92a8a595f80fa2985b452899d785@uscas1p1.samsung.com>
Precedence: bulk

Add COMMIT field check aside with existing COMMITTED field check during
hdm decoder initialization to avoid a system crash during module removal
after destroying a region which leaves the COMMIT field being reset while
the COMMITTED field still being set.

In current kernel implementation, when destroying a region (cxl
destroy-region),the decoders associated to the region will be reset
as that in cxl_decoder_reset, where the COMMIT field will be reset.
However, resetting COMMIT field will not automatically reset the
COMMITTED field, causing a situation where COMMIT is reset (0) while
COMMITTED is set (1) after the region is destroyed. Later, when
init_hdm_decoder is called (during modprobe), current code only check
the COMMITTED to decide whether the decoder is enabled or not. Since
the COMMITTED will be 1 and the code treats the decoder as enabled,
which will cause unexpected behaviour.

Before the fix, a system crash was observed when performing following
steps:
1. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem
2. cxl create-region -m -d decoder0.0 -w 1 mem0 -s 256M
3. cxl destroy-region region0 -f
4. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core
5. modprobe -a cxl_acpi cxl_core cxl_pci cxl_port cxl_mem (showing
"no CXL window for range 0x0:0xffffffffffffffff" error message)
6. rmmod cxl_acpi cxl_pci cxl_port cxl_mem cxl_pmem cxl_core (kernel
crash at cxl_dpa_release due to dpa_res has been freed when destroying
the region).

The patch fixed the above issue, and is tested based on follow patch series=
:

[PATCH 00/18] CXL RAM and the 'Soft Reserved' =3D> 'System RAM' default
Message-ID: 167601992097.1924368.18291887895351917895.stgit@dwillia2-xfh.jf=
.intel.com

Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
 drivers/cxl/core/hdm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index 80eccae6ba9e..6cf854c949f0 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -695,6 +695,7 @@ static int init_hdm_decoder(struct cxl_port *port, stru=
ct cxl_decoder *cxld,
 	struct cxl_endpoint_decoder *cxled =3D NULL;
 	u64 size, base, skip, dpa_size;
 	bool committed;
+	bool should_commit;
 	u32 remainder;
 	int i, rc;
 	u32 ctrl;
@@ -710,10 +711,11 @@ static int init_hdm_decoder(struct cxl_port *port, st=
ruct cxl_decoder *cxld,
 	base =3D ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which));
 	size =3D ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which));
 	committed =3D !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED);
+	should_commit =3D !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
 	cxld->commit =3D cxl_decoder_commit;
 	cxld->reset =3D cxl_decoder_reset;
=20
-	if (!committed)
+	if (!should_commit || !committed)
 		size =3D 0;
 	if (base =3D=3D U64_MAX || size =3D=3D U64_MAX) {
 		dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n",
@@ -727,7 +729,7 @@ static int init_hdm_decoder(struct cxl_port *port, stru=
ct cxl_decoder *cxld,
 	};
=20
 	/* decoders are enabled if committed */
-	if (committed) {
+	if (should_commit && committed) {
 		cxld->flags |=3D CXL_DECODER_F_ENABLE;
 		if (ctrl & CXL_HDM_DECODER0_CTRL_LOCK)
 			cxld->flags |=3D CXL_DECODER_F_LOCK;
@@ -772,7 +774,7 @@ static int init_hdm_decoder(struct cxl_port *port, stru=
ct cxl_decoder *cxld,
 		return 0;
 	}
=20
-	if (!committed)
+	if (!should_commit || !committed)
 		return 0;
=20
 	dpa_size =3D div_u64_rem(size, cxld->interleave_ways, &remainder);
--=20
2.25.1