Received-SPF: pass (google.com: domain of linux-kernel+bounces-135915-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Mon, 8 Apr 2024 13:45:14 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: PJ Waskiewicz <ppwaskie@kernel.org>, Jonathan Cameron
	<Jonathan.Cameron@huawei.com>
CC: Lukas Wunner <lukas@wunner.de>, Dan Williams <dan.j.williams@intel.com>,
	<linux-cxl@vger.kernel.org>, <linux-pci@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/1] cxl/acpi.c: Add buggy BIOS hint for CXL ACPI lookup
 failure
Message-ID: <6614575a1c15c_2583ad29476@dwillia2-xfh.jf.intel.com.notmuch>
References: <20240407210526.8500-1-ppwaskie@kernel.org>
 <ZhMP-NBMb387KD4Y@wunner.de>
 <ZhNQa8wAflycciNA@snoopy>
 <20240408093422.000062d9@Huawei.com>
 <ZhRFsUiDieY+HhfV@snoopy>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZhRFsUiDieY+HhfV@snoopy>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?qbzErlEW8kj/za9P3Cev7NHvceuXfpFEW17uX83hDDdxYaaBefx8YgGqLNYE?=
 =?us-ascii?Q?VQ2WoboFgqF4+Q9rpWLmJyu3ms84ljmelpUXqCIb4FZERb155iHvzh7907CS?=
 =?us-ascii?Q?1x9UU/SYLKfxC/NjTV+UvnmnQ92lfU+x7jRlkTVstk3fMgl6Hyo8s0VEqeew?=
 =?us-ascii?Q?6LM0cemPYzKdtkffTkWRxk3WtfXCFA5dQi18OmA66sBryCDDcJLA3GcUgf6G?=
 =?us-ascii?Q?+wn9iGVo3Jl2ZY9gDntjWkuPnesVgpCkzyaOftASeImXsBhUG83f6lwINCcg?=
 =?us-ascii?Q?Fqkq1otuMbQp4PN8EMrVBLzwK0ZJRsTknDUA7Ad5jlp/QMsf4fgxSY7nH+YY?=
 =?us-ascii?Q?cUylYl3zX/oRyKW45DvBSKUHoUbqDaa7iBhH/9DekcSt5vq37Rjc4pjnfcBD?=
 =?us-ascii?Q?Lxl6hvbqQCaDHZdgoGSsoxc8eU/Soxrvud8cQ2wLHf/pFAxKZYBiOp/O1a6E?=
 =?us-ascii?Q?p7hgZ+SNAb2by7wB6dz4xCX18HwX65vVnQC4QHEqvGhN4MErbjuFTsDZwUD4?=
 =?us-ascii?Q?2HTqQ82UNGrMgl0MP8HWC7jDJ5rc94cyRSx3GWzRUYQ7umLJ6rfs4X9AU6jd?=
 =?us-ascii?Q?Yw+j7XBOpHD21kGZ5sLyesR1GyrzW9VkwLDjf5kbaQLdXmB+5xKmrpjW2y/g?=
 =?us-ascii?Q?YWM6/LEXx1dzXiARdoXkichBM8ywhqacJimTAvbGs3jtvMNbbeNwOhU3GeLd?=
 =?us-ascii?Q?ODns55t5wno4Sp0gRIUOSUti6Kvq+zhKQe3zWc0ZFdWkpS5GUAfsMkeyU+qp?=
 =?us-ascii?Q?wI7bIGGc08uF7YE/oI6YoYvztUji0iZ9d8cjceWe+K8uXHbifgj29yYS6Yl2?=
 =?us-ascii?Q?tLe57zA/uCYc4AL9Fq0fTLPivt2yLwNwpO+w79ZN/YkBC6irbFWzZP87KMFM?=
 =?us-ascii?Q?/7Sa2EYfN1HT7WRpsccJeHFf3q8Ff52C8rCyeNuGgELO97PISGgTMuuyR6Oi?=
 =?us-ascii?Q?I89zHzOomQ3biE73AJGcRmAMGibK0RBboo1lLc5pCysqGLRcfQgOEY20jzhH?=
 =?us-ascii?Q?qAUrV6YpNxgALOzCA9TeOh0RHu1VKSQebk9kqPoecJdTrzDyuyFIeoJPdnX2?=
 =?us-ascii?Q?qrN0deuOV2G4nhV6A0MmrUm+hUXUWhNlXiD2z6QS+wVhGZsNToPiCMnPQQ17?=
 =?us-ascii?Q?eqrgnTD+xyv5eSdD4R9hcCQDF4q7mPI419Hbt9IK/PpCh0nIGDa9Pa2D4psz?=
 =?us-ascii?Q?pveicfDDfgqMKK0r74JPypbY3D3bx+id8UUVE8gFe+6hrpJeer1Aj3FvObrX?=
 =?us-ascii?Q?Y+s56n3k7E59j0s3CP2rw0DvQtZka1NPGTAN3vpea7HLzKjXas78un8jJvNa?=
 =?us-ascii?Q?ZFFFID/jzj+2fpd4ceRZBetkkkcHD+6TuQiqJ/958hyzAuZL1hIktOwwURU2?=
 =?us-ascii?Q?mNcmDaktZJyc10t24+nDxw9ejN7gvhWK1AzhpF/pNR/3b1RBucNgkiAx+mCc?=
 =?us-ascii?Q?Lp8MckIboZPfv0/iWPqwALK1FvhzNP5DOyxZII4I0WEBs9CsbQ9SKCp0F1Xd?=
 =?us-ascii?Q?c/1HTc0NHDd/HEUyKcRK5Boy7vzYzW7TOxDHDTpi7WuEi8PH7veHHBaPyWiL?=
 =?us-ascii?Q?IKuvNxOvbSe/nPl7rfY4map2UpHBu418b1vAAYunhGH74tRFAZlcFcOJt2ZN?=
 =?us-ascii?Q?DQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 60bcb3d1-9b6c-4c78-ae7f-08dc580ccbd6
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Apr 2024 20:45:16.3792
 (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: +9tXYTLhXXXyDgCdGoZ29CIyv1RKXVFOCAzledDqe3nkm6FUFzUL4ZYF3WKeFEM5jcFNFnud+vCduA1OYRLJw1Hntc+Dyhj+t9A9og+nrN8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB8456
X-OriginatorOrg: intel.com

PJ Waskiewicz wrote:
[..]
> > Other than that seems reasonable to hint it is probably a bios
> > bug - however I wonder how many other cases we should do this for and
> > whether it is worth the effort of marking them all?
> 
> I can confirm this was definitely a BIOS bug in this particular case.
> The vendor spun a quick test BIOS for us to test on an EMR and SPR host,
> and the _UID's were finally correct.  I could successfully walk the CEDT
> and get to the CAPS structs I was after (link speed, bus width, etc.).

Oh, in that case I think there's no need to worry about a Linux quirk.

I do think cxl_acpi has multiple overlapping failure cases when what you
really want to know is whether:

   "CXL host bridge can be found in CEDT.CHBS"

Turns out that straightforward message is aleady a driver message, but
it gets skipped in this case. So, I am thinking of cleanup /
clarification along the following lines:

1/ Lean on the existing cxl_get_chbs() validation paths to report on
errors

2/ Include the device-name rather than the UID since if UID is
unreliable it does not help you communicate with your BIOS vendor. I.e.
give a breadcrumb for the BIOS engineer to match the AML device name
with the CEDT content.

3/ Do not fail driver load on a single host-bridge parsing failure

4/ These are all cxl_acpi driver init events, so consistently use the
ACPI0017 device, and the cxl_acpi driver, as the originator of the error
message.

Would this clarification have saved you time with the debug?

diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
index 32091379a97b..5a70d7312c64 100644
--- a/drivers/cxl/acpi.c
+++ b/drivers/cxl/acpi.c
@@ -511,29 +511,26 @@ static int cxl_get_chbs_iter(union acpi_subtable_headers *header, void *arg,
 	return 0;
 }
 
-static int cxl_get_chbs(struct device *dev, struct acpi_device *hb,
-			struct cxl_chbs_context *ctx)
+static void 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) {
-		dev_err(dev, "unable to retrieve _UID\n");
-		return -ENOENT;
-	}
-
-	dev_dbg(dev, "UID found: %lld\n", uid);
-	*ctx = (struct cxl_chbs_context) {
+	*ctx = (struct cxl_chbs_context){
 		.dev = dev,
-		.uid = uid,
 		.base = CXL_RESOURCE_NONE,
 		.cxl_version = UINT_MAX,
 	};
 
-	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs_iter, ctx);
+	rc = acpi_evaluate_integer(hb->handle, METHOD_NAME__UID, NULL,
+				   &ctx->uid);
+	if (rc != AE_OK) {
+		dev_dbg(dev, "unable to retrieve _UID\n");
+		return;
+	}
 
-	return 0;
+	dev_dbg(dev, "UID found: %lld\n", ctx->uid);
+	acpi_table_parse_cedt(ACPI_CEDT_TYPE_CHBS, cxl_get_chbs_iter, ctx);
 }
 
 static int get_genport_coordinates(struct device *dev, struct cxl_dport *dport)
@@ -561,7 +558,6 @@ static int get_genport_coordinates(struct device *dev, struct cxl_dport *dport)
 static int add_host_bridge_dport(struct device *match, void *arg)
 {
 	int ret;
-	acpi_status rc;
 	struct device *bridge;
 	struct cxl_dport *dport;
 	struct cxl_chbs_context ctx;
@@ -573,19 +569,16 @@ static int add_host_bridge_dport(struct device *match, void *arg)
 	if (!hb)
 		return 0;
 
-	rc = cxl_get_chbs(match, hb, &ctx);
-	if (rc)
-		return rc;
-
+	cxl_get_chbs(match, hb, &ctx);
 	if (ctx.cxl_version == UINT_MAX) {
-		dev_warn(match, "No CHBS found for Host Bridge (UID %lld)\n",
-			 ctx.uid);
+		dev_err(host, FW_BUG "No CHBS found for Host Bridge (%s)\n",
+			dev_name(match));
 		return 0;
 	}
 
 	if (ctx.base == CXL_RESOURCE_NONE) {
-		dev_warn(match, "CHBS invalid for Host Bridge (UID %lld)\n",
-			 ctx.uid);
+		dev_err(host, FW_BUG "CHBS invalid for Host Bridge (%s)\n",
+			dev_name(match));
 		return 0;
 	}
 
@@ -650,13 +643,11 @@ static int add_host_bridge_uport(struct device *match, void *arg)
 		return 0;
 	}
 
-	rc = cxl_get_chbs(match, hb, &ctx);
-	if (rc)
-		return rc;
-
+	cxl_get_chbs(match, hb, &ctx);
 	if (ctx.cxl_version == ACPI_CEDT_CHBS_VERSION_CXL11) {
-		dev_warn(bridge,
-			 "CXL CHBS version mismatch, skip port registration\n");
+		dev_err(host,
+			FW_BUG "CXL CHBS version mismatch, skip port registration for %s\n",
+			dev_name(match));
 		return 0;
 	}