netdev - Re: [PATCH v12 05/23] cxl: add function for type2 cxl regs setup

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: <320c4b16-2029-4792-9288-8ccf99bf07cd@amd.com>
Date: Mon, 7 Apr 2025 11:04:52 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 alejandro.lucero-palau@....com
Cc: 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, Ben Cheatham <benjamin.cheatham@....com>
Subject: Re: [PATCH v12 05/23] cxl: add function for type2 cxl regs setup


On 4/4/25 17:03, Jonathan Cameron wrote:
> On Mon, 31 Mar 2025 15:45:37 +0100
> alejandro.lucero-palau@....com wrote:
>
>> From: Alejandro Lucero <alucerop@....com>
>>
>> Create a new function for a type2 device initialising
>> cxl_dev_state struct regarding cxl regs setup and mapping.
>>
>> Export the capabilities found for checking them against the
>> expected ones by the driver.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@....com>
>> Reviewed-by: Ben Cheatham <benjamin.cheatham@....com>
>> ---
>>   drivers/cxl/core/pci.c | 52 ++++++++++++++++++++++++++++++++++++++++++
>>   include/cxl/cxl.h      |  5 ++++
>>   2 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
>> index 05399292209a..e48320e16a4f 100644
>> --- a/drivers/cxl/core/pci.c
>> +++ b/drivers/cxl/core/pci.c
>> @@ -1095,6 +1095,58 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>>   }
>>   EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, "CXL");
>>   
>> +static int cxl_pci_setup_memdev_regs(struct pci_dev *pdev,
>> +				     struct cxl_dev_state *cxlds,
>> +				     unsigned long *caps)
>> +{
>> +	struct cxl_register_map map;
>> +	int rc;
>> +
>> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map, caps);
>> +	/*
>> +	 * This call can return -ENODEV if regs not found. This is not an error
>> +	 * for Type2 since these regs are not mandatory. If they do exist then
>> +	 * mapping them should not fail. If they should exist, it is with driver
>> +	 * calling cxl_pci_check_caps where the problem should be found.
> Good to put () on end of functions when mentioned in comments.


I'll do.


>> +	 */
>> +	if (rc == -ENODEV)
>> +		return 0;
> Hmm. I don't mind hugely but I'd expect the -ENODEV handler in the
> clearly accelerator specific code that follows not here.
>
> That would require cxl_map_device_regs() to definitely not return
> -ENODEV though which is a bit ugly so I guess this is ok.
>
> I'm not entirely convinced this helper makes sense though given
> the 2 parts of the component regs are just done inline in
> cxl_pci_accel_setup_regs() and if you did that then this
> accelerator specific 'carry on anyway' would be in the function
> with accel in the name.
>
> 	You'd need a
> 	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map, caps);
> 	if (rc) {
> 		if (rc != -ENODEV)
> 			return rc;
> 	} else {
> 		rc = cxl_map_device_regs();
> 		if (rc)
> 			return rc;	
> 	}	
> though which is a little messy.


That messiness is the reason I added the other function keeping, I 
think, the code clearer.

Note that other function is only used by accel code, but I can change 
the name for making it more visible:


cxl_pci_setup_memdev_regs  ---> cxl_accel_setup_memdev_regs


>> +
>> +	if (rc)
>> +		return rc;
>> +
>> +	return cxl_map_device_regs(&map, &cxlds->regs.device_regs);
>> +}
>> +
>> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds,
>> +			      unsigned long *caps)
>> +{
>> +	int rc;
>> +
>> +	rc = cxl_pci_setup_memdev_regs(pdev, cxlds, caps);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
>> +				&cxlds->reg_map, caps);
>> +	if (rc) {
>> +		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
>> +		return rc;
>> +	}
>> +
>> +	if (!caps || !test_bit(CXL_CM_CAP_CAP_ID_RAS, caps))
> As before. Why not just mandate caps?  If someone really doesn't
> care they can provide a bitmap and ignore it.  Seems like a simpler
> interface to me.


Not sure what you meant here. This is not just about knowing by the 
caller the capabilities but also mapping the related structures if present.

The now returned caps is useful for dealing with mandatory vs optional 
caps which the current code targeting Type3-only can not. In other 
words, the core code can not know if a cap missing is an error or not.


>> +		return 0;
>> +
>> +	rc = cxl_map_component_regs(&cxlds->reg_map,
>> +				    &cxlds->regs.component,
>> +				    BIT(CXL_CM_CAP_CAP_ID_RAS));
>> +	if (rc)
>> +		dev_dbg(&pdev->dev, "Failed to map RAS capability.\n");
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, "CXL");