netdev - Re: [PATCH v16 04/22] cxl: Move register/capability check to driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <682e1a27e285e_1626e100c9@dwillia2-xfh.jf.intel.com.notmuch>
Date: Wed, 21 May 2025 11:23:35 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: <alejandro.lucero-palau@....com>, <linux-cxl@...r.kernel.org>,
	<netdev@...r.kernel.org>, <dan.j.williams@...el.com>, <edward.cree@....com>,
	<davem@...emloft.net>, <kuba@...nel.org>, <pabeni@...hat.com>,
	<edumazet@...gle.com>, <dave.jiang@...el.com>
CC: Alejandro Lucero <alucerop@....com>, Jonathan Cameron
	<Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v16 04/22] cxl: Move register/capability check to driver

alejandro.lucero-palau@ wrote:
> From: Alejandro Lucero <alucerop@....com>
> 
> Type3 has some mandatory capabilities which are optional for Type2.
> 
> In order to support same register/capability discovery code for both
> types, avoid any assumption about what capabilities should be there, and
> export the capabilities found for the caller doing the capabilities
> check based on the expected ones.
> 
> Add a function for facilitating the report of capabilities missing the
> expected ones.
> 
> Signed-off-by: Alejandro Lucero <alucerop@....com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
> ---
>  drivers/cxl/core/pci.c  | 41 +++++++++++++++++++++++++++++++++++++++--
>  drivers/cxl/core/port.c |  8 ++++----
>  drivers/cxl/core/regs.c | 38 ++++++++++++++++++++++----------------
>  drivers/cxl/cxl.h       |  6 +++---
>  drivers/cxl/cxlpci.h    |  2 +-
>  drivers/cxl/pci.c       | 24 +++++++++++++++++++++---
>  include/cxl/cxl.h       | 24 ++++++++++++++++++++++++
>  7 files changed, 114 insertions(+), 29 deletions(-)
[..]
> 
> @@ -434,7 +449,7 @@ static void cxl_unmap_regblock(struct cxl_register_map *map)
>  	map->base = NULL;
>  }
>  
> -static int cxl_probe_regs(struct cxl_register_map *map)
> +static int cxl_probe_regs(struct cxl_register_map *map, unsigned long *caps)
>  {
>  	struct cxl_component_reg_map *comp_map;
>  	struct cxl_device_reg_map *dev_map;
> @@ -444,21 +459,12 @@ static int cxl_probe_regs(struct cxl_register_map *map)
>  	switch (map->reg_type) {
>  	case CXL_REGLOC_RBI_COMPONENT:
>  		comp_map = &map->component_map;
> -		cxl_probe_component_regs(host, base, comp_map);
> +		cxl_probe_component_regs(host, base, comp_map, caps);
>  		dev_dbg(host, "Set up component registers\n");
>  		break;
>  	case CXL_REGLOC_RBI_MEMDEV:
>  		dev_map = &map->device_map;
> -		cxl_probe_device_regs(host, base, dev_map);
> -		if (!dev_map->status.valid || !dev_map->mbox.valid ||
> -		    !dev_map->memdev.valid) {
> -			dev_err(host, "registers not found: %s%s%s\n",
> -				!dev_map->status.valid ? "status " : "",
> -				!dev_map->mbox.valid ? "mbox " : "",
> -				!dev_map->memdev.valid ? "memdev " : "");
> -			return -ENXIO;
> -		}
> -
> +		cxl_probe_device_regs(host, base, dev_map, caps);

I thought we talked about this before [1] , i.e. that there is no need
to pass @caps through the stack.

[1]: http://lore.kernel.org/678b06a26cddc_20fa29492@dwillia2-xfh.jf.intel.com.notmuch

Here is the proposal that moves this simple check to the leaf consumer
where it belongs vs plumbing @caps everywhere, note how this removes
burden from the core, not add burden to support more use cases:

diff --git a/drivers/cxl/core/regs.c b/drivers/cxl/core/regs.c
index ecdb22ae6952..5f511cf4bab0 100644
--- a/drivers/cxl/core/regs.c
+++ b/drivers/cxl/core/regs.c
@@ -434,7 +434,7 @@ static void cxl_unmap_regblock(struct cxl_register_map *map)
 	map->base = NULL;
 }
 
-static int cxl_probe_regs(struct cxl_register_map *map)
+static void cxl_probe_regs(struct cxl_register_map *map)
 {
 	struct cxl_component_reg_map *comp_map;
 	struct cxl_device_reg_map *dev_map;
@@ -450,22 +450,11 @@ static int cxl_probe_regs(struct cxl_register_map *map)
 	case CXL_REGLOC_RBI_MEMDEV:
 		dev_map = &map->device_map;
 		cxl_probe_device_regs(host, base, dev_map);
-		if (!dev_map->status.valid || !dev_map->mbox.valid ||
-		    !dev_map->memdev.valid) {
-			dev_err(host, "registers not found: %s%s%s\n",
-				!dev_map->status.valid ? "status " : "",
-				!dev_map->mbox.valid ? "mbox " : "",
-				!dev_map->memdev.valid ? "memdev " : "");
-			return -ENXIO;
-		}
-
 		dev_dbg(host, "Probing device registers...\n");
 		break;
 	default:
 		break;
 	}
-
-	return 0;
 }
 
 int cxl_setup_regs(struct cxl_register_map *map)
@@ -476,10 +465,10 @@ int cxl_setup_regs(struct cxl_register_map *map)
 	if (rc)
 		return rc;
 
-	rc = cxl_probe_regs(map);
+	cxl_probe_regs(map);
 	cxl_unmap_regblock(map);
 
-	return rc;
+	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(cxl_setup_regs, "CXL");
 
diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index d5447c7d540f..cfe4b5fa948a 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -945,6 +945,16 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (rc)
 		return rc;
 
+	/* Check for mandatory CXL Memory Class Device capabilities */
+	if (!map.device_map.status.valid || !map.device_map.mbox.valid ||
+	    !map.device_map.memdev.valid) {
+		dev_err(&pdev->dev, "registers not found: %s%s%s\n",
+			!map.device_map.status.valid ? "status " : "",
+			!map.device_map.mbox.valid ? "mbox " : "",
+			!map.device_map.memdev.valid ? "memdev " : "");
+		return -ENXIO;
+	}
+
 	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
 	if (rc)
 		return rc;