Date:   Tue, 14 Feb 2023 14:28:51 -0800
From:   Dan Williams <dan.j.williams@intel.com>
To:     Robert Richter <rric@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>
CC:     Robert Richter <rrichter@amd.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        "Ira Weiny" <ira.weiny@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        "Jonathan Cameron" <jonathan.cameron@huawei.com>,
        Dave Jiang <dave.jiang@intel.com>, <linux-cxl@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] cxl/port: Disable decoder setup for endpoints in RCD mode
Message-ID: <63ec0b2370e46_32d6129434@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230208071758.658652-1-rrichter@amd.com>
 <63e52846175ae_36c729428@dwillia2-xfh.jf.intel.com.notmuch>
 <Y+pOD5+DAsm3IKup@rric.localdomain>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Y+pOD5+DAsm3IKup@rric.localdomain>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?UDzi05eG7wliQuzp/jSx8HsRhCAy65jwZ9VYUQJzZ7JDFGogIz4+dzJ3whEU?=
 =?us-ascii?Q?O5XRi25ZLcV7cL9WcmZa6EdDmYoD0qPn1VUf+M/OnyCMfjqowxJkf7EL4Cht?=
 =?us-ascii?Q?QbIW/NftOb00MwGUs8oKL70ihGWgI7WtClVmTKtq3YrNntVKpw82NLcfNJBM?=
 =?us-ascii?Q?d1tUekC2VLEBkJSUwmMSJ5zfutf1IzHzmdhxJ5OBI0OvO/RWCYoFVeGLPPzF?=
 =?us-ascii?Q?bzF1ltV1GQf4CE0PMld6dha+Q9ErODKVx6qtAP8hw2Cqfj28qtTXDhS1Pfeo?=
 =?us-ascii?Q?jMP1sWq0or186r5kSJfNR1yzQaSGIj+nm4fLs3xa/RDXjSpKd/mdxXqEAUWh?=
 =?us-ascii?Q?RbmEH9spNCTsbndNcTiG/kDsRhyTS4tcC7XssfhpQq01EPor/47l8b1NJKoF?=
 =?us-ascii?Q?QG4JZRxNqhKQgwMb0P5BhUpkarGE7kBpTznCLUlk/hzGvjF5SL+UCaDpY+rD?=
 =?us-ascii?Q?qe8chS7uCUPO+6ctE5auxyEfWcpJXk+QQ7FrjmqpGqAeTSsuDT9msaefiDp3?=
 =?us-ascii?Q?W/tASFMTUCFleVGFSrOtD4oOX3mo+MHbzRwf/iYo2vQEqTt8PjuG13FaQi07?=
 =?us-ascii?Q?zUGvL4As+KVQFVuiKMttPVfCUcTqVTtP80CeuotbAnfPZuFWW0jFBABNjoxk?=
 =?us-ascii?Q?kowqAASSmJOSDWlmUBsUkSwcOVwqHeiTs9PLepkZsgD3r2oTup8lmrk4GXJ8?=
 =?us-ascii?Q?sK/YKyEY48gwFNKjF69/qxUy/sSIOq5G5ARMlxXsO4FI17TQXHbZZ9vtA2xm?=
 =?us-ascii?Q?0IjhtUMNbtb/X80k1B2lFSnOqK57n6T0fdHb2u6lHsxiIMmnMPTOgylMM8AK?=
 =?us-ascii?Q?oTweJ9LOVV9ecYjXLyDEqGpdTS7yiCyRImDUjZMyHQ/KKtm/F2Vu834fdSfX?=
 =?us-ascii?Q?AF4wgUMgpn9OWifhfSPWhySC/X+pQUaR17E9rdNlFfqLHrj+pOkZQN18Luo+?=
 =?us-ascii?Q?uD3FpjPwi2cSKujHeu3uI7TjhLU3v/7Or4EDYnK3j8dHF9eMw3FT775fIOhM?=
 =?us-ascii?Q?+ci4Q+k6g08qZT+664Utz6s9qYIrwixoklzvXuUcpi/BMP8mlvhfIEfNwwhO?=
 =?us-ascii?Q?X4IyYwD3GNu3LeW6aMOXiKTt4VQ99CC8yRdKbYPZwdV1bVdcEROJPthSJ/t/?=
 =?us-ascii?Q?S9FlzAYQR6g3IEhh9+hYD0HaQkpCpB9YWl3mdT9Zien212Bx9NBxSHkUnNu9?=
 =?us-ascii?Q?nzMrA1Umy1CCWp9Q32RcFBAHkLcxLNQFDqWKnhBlqTsjXzzOzig58YxjOYMv?=
 =?us-ascii?Q?Da+edTIT/KV/jg4bxCuCoUqbWOZ0OO7DE1MXHhQhX5U26+Yzz3jMuQWj8HGU?=
 =?us-ascii?Q?A3y0kz5w7E2layJnVmTojtRBaPuGmaIbR4hPTzuIEPXCzBuFLVi8pgBjArBH?=
 =?us-ascii?Q?zI6/JnHgtPbAE3U24b9J3Y1vWRj0aE24489kFlQjHaOemJY9cyx1+DJiZCrx?=
 =?us-ascii?Q?Ix0kF9Nl4XijHjdA28co73u8RX3TN3yuNpe1uDfQ0kCk6b+n9HL/KvPT5Upg?=
 =?us-ascii?Q?8abRhH1Mh+0LylapsFkIcwOEtWYV0fusWUa2ShVLN6Jfx8nK6j49u+89rM08?=
 =?us-ascii?Q?vL2UWveJsIZHbFktjGwhrVqaaJNay8vgVRZAj8mIsL0/7Ls/X7XtNA1onsLn?=
 =?us-ascii?Q?ag=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 713a62cf-a7a7-4fbc-54ee-08db0edadd9f
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Feb 2023 22:28:58.8954
 (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: RePCr9s1tPwbNVA0Y8wFJEugmU0QsFikJZPQcf9SqieAW/uuAS2dWQHTlL8ZyyIC9PXm4Nc72WfW3tm+a2wPt3u+LQOk+kwwzxpu3gjehDE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7450
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Robert Richter wrote:
> Dan,
> 
> On 09.02.23 09:07:18, Dan Williams wrote:
> > Robert Richter wrote:
> > > In RCD mode the HDM decoder capability is optional for endpoints and
> > > may not exist. The HDM range registers are used instead. Since the
> > > driver relies on the existence of an HDM decoder capability, its
> > > absence will cause the initialization of a memory card to fail.
> > > 
> > > Moreover, the driver also tries to enable or reuse enabled memory
> > > ranges. In the worst case this may lead to a system hang due to
> > > disabling system memory that was previously provided and setup by
> > > system firmware.
> > > 
> > > To solve the issues described, disable decoder setup for RCD endpoints
> > > and instead rely exclusively on system firmware to enable those memory
> > > ranges. Decoders are used by the kernel to setup and configure CXL
> > > memory regions, esp. to enable and disable them. Since Hot-plug is not
> > > supported for devices in RCD mode, the ability to disable that memory
> > > by the kernel using a decoder is not a necessarily requirement,
> > > decoders are not needed then.
> > > 
> > > Fixes: 34e37b4c432c ("cxl/port: Enable HDM Capability after validating DVSEC Ranges")
> > > Signed-off-by: Robert Richter <rrichter@amd.com>
> > 
> > Does Dave's series address this problem?
> > 
> > https://lore.kernel.org/linux-cxl/167588394236.1155956.8466475582138210344.stgit@djiang5-mobl3.local/
> > 
> > ...that is arranging for the driver to carry-on in the absence of the
> > HDM Decoder Capability.
> 
> it might only solve the missing hdm decoder capability. I need to take
> a closer look if that also solves a system hang I was debugging which
> is caused by clearing the memory disable bit in the hdm dvsec range
> register. So the best would be to use this patch now to fix decoder
> initialization in RCD mode and then have Dave's patches on top. I am
> going to test the series too.

My concern with this patch is that it skips HDM decoder enumeration
entirely in RCD mode. The CXL cards I have seen are CXL 1.1+ and do
export the HDM decoder capability.

The driver turns off mem_enable in a few scenarios, one of them indeed
looks buggy, but does not seem to be the one you addressed. The driver
should only disable mem if it was also the agent that enabled mem, but
looks like it does not always do that.

Can you confirm if this fixes this issue?

diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index c18ed1bbb54d..2db3b5cf41e9 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -385,7 +385,8 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm,
         * If the HDM Decoder Capability is already enabled then assume
         * that some other agent like platform firmware set it up.
         */
-       if (global_ctrl & CXL_HDM_DECODER_ENABLE || (!hdm && info->mem_enabled))
+       if (!info->mem_enabled &&
+           (global_ctrl & CXL_HDM_DECODER_ENABLE || !hdm))
                return devm_cxl_enable_mem(&port->dev, cxlds);
        else if (!hdm)
                return -ENODEV;

Otherwise can you confirm if the platform provides a CFMWS window that
matches the range-register programming? If this is the problem then I
think this needs a platform quirk to workaround a BIOS that violates
kernel expectations.