| 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
| ||
|
Message-ID: <0636c174-4633-4018-bf52-f7f53a82f71a@amd.com>
Date: Thu, 22 May 2025 10:45:26 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Dan Williams <dan.j.williams@...el.com>, alejandro.lucero-palau@....com,
linux-cxl@...r.kernel.org, netdev@...r.kernel.org, edward.cree@....com,
davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
edumazet@...gle.com, dave.jiang@...el.com
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Subject: Re: [PATCH v16 04/22] cxl: Move register/capability check to driver
On 5/21/25 19:23, Dan Williams wrote:
> 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.
You did not like to add a new capability field to current structs
because the information needed was already there in the map. I think it
was a fair comment, so the caps, a variable the caller gives, is set
during the discovery without any internal struct added.
Regarding what you suggest below, I have to disagree. This change was
introduced for dealing with a driver using CXL, that is a Type2 or
future Type1 driver. IMO, most of the innerworkings should be hidden to
those clients and therefore working with the map struct is far from
ideal, and it is not currently accessible from those drivers. With these
new drivers the core does not know what should be there, so the check is
delayed and left to the driver. IMO, from a Type2/Type1 driver
perspective, it is better to deal with caps expected/found than being
aware of those internal CXL register discovery and maps. I will go back
to this in a later comment in your review what states "the driver should
know" . That is true, as the patchset introduces this expectation for
facilitating to know/check if the right registers are there. But note
this is not only about that final end, but the API handling something
going wrong on that discovery/mapping.
> [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;
Powered by blists - more mailing lists