Date:   Fri, 3 Mar 2023 12:58:10 -0800
From:   Dan Williams <dan.j.williams@intel.com>
To:     Fan Ni <fan.ni@samsung.com>,
        "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: RE: [PATCH] cxl/hdm: Fix hdm decoder init by adding COMMIT field
 check
Message-ID: <64025f6219d2d_71138294e5@dwillia2-xfh.jf.intel.com.notmuch>
References: <CGME20230228224029uscas1p1e2fb92a8a595f80fa2985b452899d785@uscas1p1.samsung.com>
 <20230228224014.1402545-1-fan.ni@samsung.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230228224014.1402545-1-fan.ni@samsung.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?R6zhciBasSZJEckrh8bsnPnQjsNqDyJN80mW3wyt9MIcUUtIIEjHbmtUGDLh?=
 =?us-ascii?Q?MQPvQ/RqknCRUcTLU2Ee5bifFE3BkszldO+KxVHcnc5yc6QLRaYtgyjOAnpR?=
 =?us-ascii?Q?FuaNGM3idRf+4zI983Gc4hYfMoHfHUHb3Xd0KtZmIBerheHGMiGI0f9FDEmc?=
 =?us-ascii?Q?FZ0B1E2MM2fPCtzM9pBXPgM2f8SmTGZdfX+LJd/YUgeaLarKZBkSKCIO0oUI?=
 =?us-ascii?Q?cuyzX2hEx6vMhtk8ADPM3AgbSXvnQNGhHH09+G6lSHpzR2b3rjvMFfzXASAt?=
 =?us-ascii?Q?+xqvnJgylm45i1Ch9xA/v+RUz+ZAZn2rp3mi3goWL2Bzh+co3lcP+zbl0u88?=
 =?us-ascii?Q?miHgyfkmJ4qkoC0/Q3xSsv4iIj0jWdn2xLjy5Tixg8X/3Q6lCvoExjLqM0xy?=
 =?us-ascii?Q?wgp5F6CUKiGTwV75LhVw8qweAjHZI2vA7nfsynUHgYnpJqEPsWt+xoV4p/eo?=
 =?us-ascii?Q?dlSubIFCNW3FyVy/OLh7W/G+xApCaDw0bS3WJbQtEWeMGcHR8znzjRZCRf2O?=
 =?us-ascii?Q?k1f/o8xOrBgjQ/GFetKM1G8Ptzzp0f9d+9Owy2RGlIwoySo4eriMotuGBash?=
 =?us-ascii?Q?Ish7/b5R39oPBAnNG4OZK8apG4sj+fsmXPvPi56E5RHanR7tN1V+8AEbR9Jx?=
 =?us-ascii?Q?jjHgVE3sEnMxJkMrxtG/kLtBi5zugfAJBfBQwVkswtbbAshNl6+X0ZswS7qa?=
 =?us-ascii?Q?0dX1ZL/BBps8LvAx7BTfYEUFRKEOpXVbLF/OMQHIJl4D058CMeZV/4MkMTr6?=
 =?us-ascii?Q?5aBfO9+eGBUVbBQajQFNNl/qoheEK1AdItsqeU7PSV3jblIuYg1Zv0mOV4ft?=
 =?us-ascii?Q?re7pSRZLqnElZP+Ywiydkce+VWVVgzmpCmS5ky5atZLg8yKX8zR4pQJwuJoc?=
 =?us-ascii?Q?gWc76T1iB4VTZRZWuReQe6SEDut6hK7VcFRpbptxD59yml/xfMupha0LddWB?=
 =?us-ascii?Q?KoJ3yFQTRP17oTKJUq53SklXEpsj8Spqavij2vJIrDeDBlwxiAgh5M+IGs28?=
 =?us-ascii?Q?pisp5j3ajzCC2FeZVEc/CHcO5RuO8rNHMxKFMRjGwmUQu/qDy1IAZMWcADxX?=
 =?us-ascii?Q?lfwFXP+QKNWjIesHSLwbP33Cbxm76uyuWGeI5Hkwe3t8OpOslQ+BEf48TSpg?=
 =?us-ascii?Q?tc0iWnQe19KcN+8HfIP8Xc00vZCPqg2HiiecXDUyfcNg/JC5m1OONZ0pguYO?=
 =?us-ascii?Q?CtHbLc7Szb9/FAI/DazBuSajH/odf1GPkQHxB+GdV2B6g+HA6nKjbEoLXYER?=
 =?us-ascii?Q?1w3xH8hI8Xs/yy/8stJ03iySv+gwQN+rx7xsgl3K0lWNW386XFCfpMAl4uDv?=
 =?us-ascii?Q?hl+SRL3zfFP9/aoUnLJRIVnMfuOsS4Q+IjvO195l0IcaLWDuzGPlh+NtAedu?=
 =?us-ascii?Q?o2DwjHxYE4MTt1XsLUiRzw/ATvqznjV/UuAPbpI0g7NvI17+BiBdc9tKzBmP?=
 =?us-ascii?Q?fRIAMwAwI9cght1cYfVRdCY3AN5I0rFnakFawcUR6ufbHuLpG9RAFKc3gWXH?=
 =?us-ascii?Q?F1wyaFcYfRjYsPL473D+M9S8iHX1FVYFOjo5FgwSjAuAETQVRdc6Ii/nyFPJ?=
 =?us-ascii?Q?NpU2QAYL+atd3C43mgLXrQCHJrlQzz6p2/j9zxS9/cDy1WPsmrII5L8pu5t1?=
 =?us-ascii?Q?1A=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 88853d88-1f76-4d30-40e7-08db1c2a0060
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Mar 2023 20:58:12.5444
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WESLp7bhSwyDAxKkSzHtCIdctYP5H0dgctTZsYYcYLcC2/lrzRarKpI2gJbAHYDg//CeaGrxslIULPhibVZq2DEKW8cE5oOJXKvUPD60b9M=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6368
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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;