Messages in this thread | | | Date | Fri, 3 Mar 2023 12:58:10 -0800 | From | Dan Williams <> | Subject | RE: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field check |
| |
Fan Ni wrote: > 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).
I think a separate fix for that crash is needed, can you send the backtrace? I.e. I worry that crash can be triggered by other means.
> > The patch fixed the above issue, and is tested based on follow patch series: > > [PATCH 00/18] CXL RAM and the 'Soft Reserved' => '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, struct cxl_decoder *cxld, > struct cxl_endpoint_decoder *cxled = 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, struct cxl_decoder *cxld, > base = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_BASE_LOW_OFFSET(which)); > size = ioread64_hi_lo(hdm + CXL_HDM_DECODER0_SIZE_LOW_OFFSET(which)); > committed = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED); > + should_commit = !!(ctrl & CXL_HDM_DECODER0_CTRL_COMMIT);
This change looks like a good idea in general given the ambiguity of 'committed'. However just combine the two checks into the @committed variable with something like this:
commit_mask = CXL_HDM_DECODER0_CTRL_COMMITTED|CXL_HDM_DECODER0_CTRL_COMMIT; committed = (ctrl & commit_mask) == commit_mask;
| |