Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Jun 2023 21:24:44 -0700
From:   Dan Williams <dan.j.williams@intel.com>
To:     Terry Bowman <terry.bowman@amd.com>, <alison.schofield@intel.com>,
        <vishal.l.verma@intel.com>, <ira.weiny@intel.com>,
        <bwidawsk@kernel.org>, <dan.j.williams@intel.com>,
        <dave.jiang@intel.com>, <Jonathan.Cameron@huawei.com>,
        <linux-cxl@vger.kernel.org>
CC:     <terry.bowman@amd.com>, <rrichter@amd.com>,
        <linux-kernel@vger.kernel.org>, <bhelgaas@google.com>
Subject: RE: [PATCH v5 08/26] cxl/acpi: Directly bind the CEDT detected CHBCR
 to the Host Bridge's port
Message-ID: <6482a98cde722_1433ac29444@dwillia2-xfh.jf.intel.com.notmuch>
References: <20230607221651.2454764-1-terry.bowman@amd.com>
 <20230607221651.2454764-9-terry.bowman@amd.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230607221651.2454764-9-terry.bowman@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?oLeUVfEtLLxglpDUauQutrqRlgCZ0v0uMEiJ+KYK/Bnf2wvzHEd13u/67mmV?=
 =?us-ascii?Q?/cRnJCEzt9JsmIBIsv7YQkfKBUs6n2cNs+P2OAcudZ6bjvBDzP3+ggaCSkH6?=
 =?us-ascii?Q?mhxImmkiOOAHLOPHidafYIxNF1luCmcoFhY4xGHYZ6LI3hfK6jpUmLR/uQn2?=
 =?us-ascii?Q?uKUYcpKDU4Soy2qizMRS1Dxe18BE3PAUP2pOYT5XPzY4GR2nkGozR/4s9u3c?=
 =?us-ascii?Q?Z2FaoPZ5PzGHj9TsiA2UiC3WcBAWGw5j4ASp+ReXF3MKdXGZFOdn9zWUgat8?=
 =?us-ascii?Q?aOoWCQtOjLuH3Sfmrnce1UiEE3kO9q20YNQdrkeWGlCydK5abbTHZTOQlhpu?=
 =?us-ascii?Q?xP3elW4tDPAzTsjoDSG452o06ru0xxKMY4tha8M33oIl0VEfATDnHTOr4PBM?=
 =?us-ascii?Q?WkLeNKmK5O8/8WuYYQk/rVgt5Ex0VaWZFdGnHsxLYuLbnETjc8DsF1BkhHrJ?=
 =?us-ascii?Q?+TYi+L/IhTQ9E3rbpaOL23K1Hn+nnsnqRWjdef+Li60ns4l97XFfLOVQ95kS?=
 =?us-ascii?Q?kor4xB+QjQ7P7xt1A8Wx04xfozShvw4DVAxI7WCaEysiyoUWOYQvQ/goz3V9?=
 =?us-ascii?Q?V2eKxLTq0WmAzyjkCZs6gydob18W+2c4L68KhX29MYyl+drFSnk//TmVlczr?=
 =?us-ascii?Q?h32MXJKW5H2FDE0WEglPY07zxk7bjYXplUkZE5lAjz8bF2sw9kDxZc7v3Zm8?=
 =?us-ascii?Q?Dw78+juMg0A6kqp2tA84GR4PKM5agwrnVa5Dt3UnQpxAMFXhoGu6iVoiuF5D?=
 =?us-ascii?Q?9TZYDJyoKo++dgBCY92d/IYP4iFodjSFfHBSN9G4BPUNqxay84e4rtZQInRQ?=
 =?us-ascii?Q?unQBDcvzRs9mAMN+Wz988jOmPRgqRKAvsQ9PkbNkKkuj0HtxwY+6TgrkOkrl?=
 =?us-ascii?Q?PODthTJv6b13ww1jR1dvR7tQJ61ciL/fTlqy2z84Iavnx+jU6Vew6TtKN7qL?=
 =?us-ascii?Q?yTS3Ec5wx4fAsoAshfJ9loS1FvtGhktT038iNtrT37y6e01do4tfaa96hx+A?=
 =?us-ascii?Q?PJnv4aEQSBCAgAWkLGybI+HK+lY9h30TI/PJIOiM+VTT0znod0gcUv8UaAyj?=
 =?us-ascii?Q?X9g4QlqNl1L+X/Az3GSIU2bI1+G3jQCqhOyVpMFwpYFv4PLhpZ5+BBYta4ES?=
 =?us-ascii?Q?TPaumrCYGDpJHxeBGvHDrW9Se0Ds7592uYgXEbcfyJhMBssmxmRMC2DUxxFB?=
 =?us-ascii?Q?OHG5K8oLuzo6mm5/FJaony4kMhrWSbJu1y3Cli7hvfDGlTZBaVOC6Jo+UKah?=
 =?us-ascii?Q?dT56EjoYJ8EEb8eI6zfnZaoLbPHI8/dpETc8UthVnIf8mq5XRGLAMxvY8fG6?=
 =?us-ascii?Q?jcr94UbZx4OzSbmgPtloOzuYSFfz+Ej98Gt0T4rkBfHgwk3Sy1/G8tHnGIdA?=
 =?us-ascii?Q?jKbgk+gT1xuY9tUxiobSWp6w/kJrvapa+jIxO//Z8d1CxChDkkvL/R+42sn1?=
 =?us-ascii?Q?aS09M0mGWkjJof4kjPFpa+t/wsMDGVVomI8q6COMwEBahivhbGCRs7iy7BCI?=
 =?us-ascii?Q?S9SKv7fap6w57nkBBfsp1pH+dqg79RUhzw+TYMzL9mN6+POyki64bXYKY5nr?=
 =?us-ascii?Q?IoFgmsuPtODDiSC5H0tVtgG33JUC69qNUI9KLU7bHjwDvVoBAAb4ZBERHdDM?=
 =?us-ascii?Q?bg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d6378320-1c23-44fa-4026-08db68a176b1
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jun 2023 04:24:49.6681
 (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: tpXZFEKIB+rLJBsr+wgLN58LHCzBZRqBPi3um75PCwMFnigw9Kx/dzTYCzOGQBMUk+0Jxl6grqcbMyPBCJQZvb86SS5N8OGJNUFrCSnALGA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR11MB8038
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Terry Bowman wrote:
> From: Robert Richter <rrichter@amd.com>
> 
> During a Host Bridge's downstream port enumeration the CHBS entries in
> the CEDT table are parsed, its Component Register base address
> extracted and then stored in struct cxl_dport. The CHBS may contain
> either the RCRB (RCH mode) or the Host Bridge's Component Registers
> (CHBCR, VH mode). The RCRB further contains the CXL downstream port
> register base address, while in VH mode the CXL Downstream Switch
> Ports are visible in the PCI hierarchy and the DP's component regs are
> disovered using the CXL DVSEC register locator capability. The
> Component Registers derived from the CHBS for both modes are different
> and thus also must be treated differently. That is, in RCH mode, the
> component regs base should be bound to the dport, but in VH mode to
> the CXL host bridge's port object.
> 
> The current implementation stores the CHBCR in addition in struct
> cxl_dport and copies it later from there to struct cxl_port. As a
> result, the dport contains the wrong Component Registers base address
> and, e.g. the RAS capability of a CXL Root Port cannot be detected.
> 
> To fix the CHBCR binding, attach it directly to the Host Bridge's
> @cxl_port structure. Do this during port creation of the Host Bridge
> in add_host_bridge_uport(). Factor out CHBS parsing code in
> add_host_bridge_dport() and use it in both functions.
> 
> Signed-off-by: Robert Richter <rrichter@amd.com>
> Signed-off-by: Terry Bowman <terry.bowman@amd.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/cxl/acpi.c | 65 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 50 insertions(+), 15 deletions(-)

I like where this patch is going. I needed to rebase and fix it up to
get cxl_test working again, and spotted some additional cleanups along
the way. I notice that after this patch dport->component_reg_phys is
totally unused. So I would like to see that removal patch comes next in
the series before going into the port->component_reg_phys reworks.

-- >8 --
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index eb64dc7a3d8d..7df6c6f5e902 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -341,7 +341,7 @@ static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg,
 	struct cxl_chbs_context *ctx = arg;
 	struct acpi_cedt_chbs *chbs;
 
-	if (ctx->base)
+	if (ctx->base != CXL_RESOURCE_NONE)
 		return 0;
 
 	chbs = (struct acpi_cedt_chbs *) header;
@@ -350,8 +350,6 @@ static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg,
 		return 0;
 
 	ctx->cxl_version = chbs->cxl_version;
-	ctx->base = CXL_RESOURCE_NONE;
-
 	if (!chbs->base)
 		return 0;
 
@@ -364,17 +362,26 @@ static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg,
 	return 0;
 }
 
-static int cxl_get_chbs(struct acpi_device *hb, struct cxl_chbs_context *ctx)
+static int cxl_get_chbs(struct device *dev, struct acpi_device *hb,
+			struct cxl_chbs_context *ctx)
 {
 	unsigned long long uid;
 	int rc;
 
 	rc = acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL, &uid);
-	if (rc != AE_OK)
+	if (rc != AE_OK) {
+		dev_err(dev, "unable to retrieve _UID\n");
 		return -ENOENT;
+	}
+
+	dev_dbg(dev, "UID found: %lld\n", uid);
+	*ctx = (struct cxl_chbs_context) {
+		.dev = dev,
+		.uid = uid,
+		.base = CXL_RESOURCE_NONE,
+		.cxl_version = UINT_MAX,
+	};
 
-	memset(ctx, 0, sizeof(*ctx));
-	ctx->uid = uid;
 	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs_iter, ctx);
 
 	return 0;
@@ -384,7 +391,6 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 {
 	acpi_status rc;
 	struct device *bridge;
-	unsigned long long uid;
 	struct cxl_dport *dport;
 	struct cxl_chbs_context ctx;
 	struct acpi_pci_root *pci_root;
@@ -395,24 +401,19 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 	if (!hb)
 		return 0;
 
-	rc = cxl_get_chbs(hb, &ctx);
-	if (rc == -ENOENT)
-		dev_err(match, "unable to retrieve _UID\n");
+	rc = cxl_get_chbs(match, hb, &ctx);
 	if (rc)
 		return rc;
 
-	uid = ctx.uid;
-	dev_dbg(match, "UID found: %lld\n", uid);
-
-	if (!ctx.base) {
+	if (ctx.cxl_version == UINT_MAX) {
 		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n",
-			 uid);
+			 ctx.uid);
 		return 0;
 	}
 
 	if (ctx.base == CXL_RESOURCE_NONE) {
 		dev_warn(match, "CHBS invalid for Host Bridge (UID %lld)\n",
-			 uid);
+			 ctx.uid);
 		return 0;
 	}
 
@@ -425,10 +426,12 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 	 * object later in add_host_bridge_uport().
 	 */
 	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) {
-		dev_dbg(match, "RCRB found for UID %lld: %pa\n", uid, &ctx.base);
-		dport = devm_cxl_add_rch_dport(root_port, bridge, uid, ctx.base);
+		dev_dbg(match, "RCRB found for UID %lld: %pa\n", ctx.uid,
+			&ctx.base);
+		dport = devm_cxl_add_rch_dport(root_port, bridge, ctx.uid,
+					       ctx.base);
 	} else {
-		dport = devm_cxl_add_dport(root_port, bridge, uid,
+		dport = devm_cxl_add_dport(root_port, bridge, ctx.uid,
 					   CXL_RESOURCE_NONE);
 	}
 
@@ -471,19 +474,17 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 		return 0;
 	}
 
-	rc = cxl_get_chbs(hb, &ctx);
+	rc = cxl_get_chbs(match, hb, &ctx);
 	if (rc)
 		return rc;
 
-	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11)
-		/* RCH mode, should never happen */
+	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) {
+		dev_warn(bridge,
+			 "CXL CHBS version mismatch, skip port registration\n");
 		return 0;
+	}
 
-	if (ctx.base)
-		component_reg_phys = ctx.base;
-	else
-		component_reg_phys = CXL_RESOURCE_NONE;
-
+	component_reg_phys = ctx.base;
 	if (component_reg_phys != CXL_RESOURCE_NONE)
 		dev_dbg(match, "CHBCR found for UID %lld: %pa\n",
 			ctx.uid, &component_reg_phys);