diff mbox series

[v2,02/15] cxl: add function for type2 cxl regs setup

Message ID 20240715172835.24757-3-alejandro.lucero-palau@amd.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series cxl: add Type2 device support | expand

Commit Message

Lucero Palau, Alejandro July 15, 2024, 5:28 p.m. UTC 
From: Alejandro Lucero <alucerop@amd.com>

Create a new function for a type2 device initialising the opaque
cxl_dev_state struct regarding cxl regs setup and mapping.

Signed-off-by: Alejandro Lucero <alucerop@amd.com>
---
 drivers/cxl/pci.c                  | 28 ++++++++++++++++++++++++++++
 drivers/net/ethernet/sfc/efx_cxl.c |  3 +++
 include/linux/cxl_accel_mem.h      |  1 +
 3 files changed, 32 insertions(+)

Comments

Li, Ming4 July 16, 2024, 6:26 a.m. UTC | #1 
On 7/16/2024 1:28 AM, alejandro.lucero-palau@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
>
> Create a new function for a type2 device initialising the opaque
> cxl_dev_state struct regarding cxl regs setup and mapping.
>
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/pci.c                  | 28 ++++++++++++++++++++++++++++
>  drivers/net/ethernet/sfc/efx_cxl.c |  3 +++
>  include/linux/cxl_accel_mem.h      |  1 +
>  3 files changed, 32 insertions(+)
>
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index e53646e9f2fb..b34d6259faf4 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -11,6 +11,7 @@
>  #include <linux/pci.h>
>  #include <linux/aer.h>
>  #include <linux/io.h>
> +#include <linux/cxl_accel_mem.h>
>  #include "cxlmem.h"
>  #include "cxlpci.h"
>  #include "cxl.h"
> @@ -521,6 +522,33 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>  	return cxl_setup_regs(map);
>  }
>  
> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds)
> +{
> +	struct cxl_register_map map;
> +	int rc;
> +
> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
> +				&cxlds->reg_map);
> +	if (rc)
> +		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
> +
> +	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);
> +

My first feeling is that above function should be provided by cxl_core rather than cxl_pci.

Let's see if Dan has comments on that.


>  static int cxl_pci_ras_unmask(struct pci_dev *pdev)
>  {
>  	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 4554dd7cca76..10c4fb915278 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -47,6 +47,9 @@ void efx_cxl_init(struct efx_nic *efx)
>  
>  	res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram");
>  	cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_RAM);
> +
> +	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds))
> +		pci_info(pci_dev, "CXL accel setup regs failed");
>  }
>  
>  
> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
> index daf46d41f59c..ca7af4a9cefc 100644
> --- a/include/linux/cxl_accel_mem.h
> +++ b/include/linux/cxl_accel_mem.h
> @@ -19,4 +19,5 @@ void cxl_accel_set_dvsec(cxl_accel_state *cxlds, u16 dvsec);
>  void cxl_accel_set_serial(cxl_accel_state *cxlds, u64 serial);
>  void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res,
>  			    enum accel_resource);
> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
>  #endif
Dave Jiang July 18, 2024, 11:27 p.m. UTC | #2 
On 7/15/24 10:28 AM, alejandro.lucero-palau@amd.com wrote:
> From: Alejandro Lucero <alucerop@amd.com>
> 
> Create a new function for a type2 device initialising the opaque
> cxl_dev_state struct regarding cxl regs setup and mapping.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/pci.c                  | 28 ++++++++++++++++++++++++++++
>  drivers/net/ethernet/sfc/efx_cxl.c |  3 +++
>  include/linux/cxl_accel_mem.h      |  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index e53646e9f2fb..b34d6259faf4 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -11,6 +11,7 @@
>  #include <linux/pci.h>
>  #include <linux/aer.h>
>  #include <linux/io.h>
> +#include <linux/cxl_accel_mem.h>
>  #include "cxlmem.h"
>  #include "cxlpci.h"
>  #include "cxl.h"
> @@ -521,6 +522,33 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>  	return cxl_setup_regs(map);
>  }
>  
> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds)

Function should go into cxl/core/pci.c

> +{
> +	struct cxl_register_map map;
> +	int rc;
> +
> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
> +				&cxlds->reg_map);
> +	if (rc)
> +		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
> +
> +	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");

dev_warn()? also maybe add the errno in the error emissioni. 

> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, CXL);
> +
>  static int cxl_pci_ras_unmask(struct pci_dev *pdev)
>  {
>  	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 4554dd7cca76..10c4fb915278 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -47,6 +47,9 @@ void efx_cxl_init(struct efx_nic *efx)
>  
>  	res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram");
>  	cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_RAM);
> +
> +	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds))
> +		pci_info(pci_dev, "CXL accel setup regs failed");

pci_warn()? although seems unnecesary since error emitted in cxl_pci_accel_setup_regs(). 

>  }
>  
>  
> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
> index daf46d41f59c..ca7af4a9cefc 100644
> --- a/include/linux/cxl_accel_mem.h
> +++ b/include/linux/cxl_accel_mem.h
> @@ -19,4 +19,5 @@ void cxl_accel_set_dvsec(cxl_accel_state *cxlds, u16 dvsec);
>  void cxl_accel_set_serial(cxl_accel_state *cxlds, u64 serial);
>  void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res,
>  			    enum accel_resource);
> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
>  #endif
Jonathan Cameron Aug. 4, 2024, 5:15 p.m. UTC | #3 
On Mon, 15 Jul 2024 18:28:22 +0100
alejandro.lucero-palau@amd.com wrote:

> From: Alejandro Lucero <alucerop@amd.com>
> 
> Create a new function for a type2 device initialising the opaque
> cxl_dev_state struct regarding cxl regs setup and mapping.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/pci.c                  | 28 ++++++++++++++++++++++++++++
>  drivers/net/ethernet/sfc/efx_cxl.c |  3 +++
>  include/linux/cxl_accel_mem.h      |  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index e53646e9f2fb..b34d6259faf4 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -11,6 +11,7 @@
>  #include <linux/pci.h>
>  #include <linux/aer.h>
>  #include <linux/io.h>
> +#include <linux/cxl_accel_mem.h>
>  #include "cxlmem.h"
>  #include "cxlpci.h"
>  #include "cxl.h"
> @@ -521,6 +522,33 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>  	return cxl_setup_regs(map);
>  }
>  
> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds)
> +{
> +	struct cxl_register_map map;
> +	int rc;
> +
> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
> +	if (rc)
> +		return rc;
> +
> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
> +				&cxlds->reg_map);
> +	if (rc)
> +		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);

Not fatal?  If we think it will happen on real devices, then dev_warn
is too strong.

> +
> +	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");

pci_err() or similar would make sense here as we have asked for something
that isn't happening. Specification says this is mandatory so
definitely smells like a fatal error to me.


> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, CXL);
> +
>  static int cxl_pci_ras_unmask(struct pci_dev *pdev)
>  {
>  	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 4554dd7cca76..10c4fb915278 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -47,6 +47,9 @@ void efx_cxl_init(struct efx_nic *efx)
>  
>  	res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram");
>  	cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_RAM);
> +
> +	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds))
> +		pci_info(pci_dev, "CXL accel setup regs failed");
Handle errors fully. That is report them  up to the caller.

>  }
>  
>  
> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
> index daf46d41f59c..ca7af4a9cefc 100644
> --- a/include/linux/cxl_accel_mem.h
> +++ b/include/linux/cxl_accel_mem.h
> @@ -19,4 +19,5 @@ void cxl_accel_set_dvsec(cxl_accel_state *cxlds, u16 dvsec);
>  void cxl_accel_set_serial(cxl_accel_state *cxlds, u64 serial);
>  void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res,
>  			    enum accel_resource);
> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
>  #endif
Alejandro Lucero Palau Aug. 14, 2024, 7:46 a.m. UTC | #4 
On 7/16/24 07:26, Li, Ming4 wrote:
> On 7/16/2024 1:28 AM, alejandro.lucero-palau@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Create a new function for a type2 device initialising the opaque
>> cxl_dev_state struct regarding cxl regs setup and mapping.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/pci.c                  | 28 ++++++++++++++++++++++++++++
>>   drivers/net/ethernet/sfc/efx_cxl.c |  3 +++
>>   include/linux/cxl_accel_mem.h      |  1 +
>>   3 files changed, 32 insertions(+)
>>
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index e53646e9f2fb..b34d6259faf4 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/pci.h>
>>   #include <linux/aer.h>
>>   #include <linux/io.h>
>> +#include <linux/cxl_accel_mem.h>
>>   #include "cxlmem.h"
>>   #include "cxlpci.h"
>>   #include "cxl.h"
>> @@ -521,6 +522,33 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>>   	return cxl_setup_regs(map);
>>   }
>>   
>> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds)
>> +{
>> +	struct cxl_register_map map;
>> +	int rc;
>> +
>> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
>> +				&cxlds->reg_map);
>> +	if (rc)
>> +		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
>> +
>> +	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);
>> +
> My first feeling is that above function should be provided by cxl_core rather than cxl_pci.
>
> Let's see if Dan has comments on that.


This has also been suggested by another reviewer, so I take it as an 
action for v3.

Thanks


>
>>   static int cxl_pci_ras_unmask(struct pci_dev *pdev)
>>   {
>>   	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> index 4554dd7cca76..10c4fb915278 100644
>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -47,6 +47,9 @@ void efx_cxl_init(struct efx_nic *efx)
>>   
>>   	res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram");
>>   	cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_RAM);
>> +
>> +	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds))
>> +		pci_info(pci_dev, "CXL accel setup regs failed");
>>   }
>>   
>>   
>> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
>> index daf46d41f59c..ca7af4a9cefc 100644
>> --- a/include/linux/cxl_accel_mem.h
>> +++ b/include/linux/cxl_accel_mem.h
>> @@ -19,4 +19,5 @@ void cxl_accel_set_dvsec(cxl_accel_state *cxlds, u16 dvsec);
>>   void cxl_accel_set_serial(cxl_accel_state *cxlds, u64 serial);
>>   void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res,
>>   			    enum accel_resource);
>> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
>>   #endif
>
Alejandro Lucero Palau Aug. 14, 2024, 7:49 a.m. UTC | #5 
On 7/19/24 00:27, Dave Jiang wrote:
>
> On 7/15/24 10:28 AM, alejandro.lucero-palau@amd.com wrote:
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Create a new function for a type2 device initialising the opaque
>> cxl_dev_state struct regarding cxl regs setup and mapping.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/pci.c                  | 28 ++++++++++++++++++++++++++++
>>   drivers/net/ethernet/sfc/efx_cxl.c |  3 +++
>>   include/linux/cxl_accel_mem.h      |  1 +
>>   3 files changed, 32 insertions(+)
>>
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index e53646e9f2fb..b34d6259faf4 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/pci.h>
>>   #include <linux/aer.h>
>>   #include <linux/io.h>
>> +#include <linux/cxl_accel_mem.h>
>>   #include "cxlmem.h"
>>   #include "cxlpci.h"
>>   #include "cxl.h"
>> @@ -521,6 +522,33 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>>   	return cxl_setup_regs(map);
>>   }
>>   
>> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds)
> Function should go into cxl/core/pci.c


It will be in v3.


>> +{
>> +	struct cxl_register_map map;
>> +	int rc;
>> +
>> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
>> +				&cxlds->reg_map);
>> +	if (rc)
>> +		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
>> +
>> +	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");
> dev_warn()? also maybe add the errno in the error emissioni.


Yes. Thanks


>
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, CXL);
>> +
>>   static int cxl_pci_ras_unmask(struct pci_dev *pdev)
>>   {
>>   	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> index 4554dd7cca76..10c4fb915278 100644
>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -47,6 +47,9 @@ void efx_cxl_init(struct efx_nic *efx)
>>   
>>   	res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram");
>>   	cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_RAM);
>> +
>> +	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds))
>> +		pci_info(pci_dev, "CXL accel setup regs failed");
> pci_warn()? although seems unnecesary since error emitted in cxl_pci_accel_setup_regs().


Right. I think I'll remove it.

Thanks


>>   }
>>   
>>   
>> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
>> index daf46d41f59c..ca7af4a9cefc 100644
>> --- a/include/linux/cxl_accel_mem.h
>> +++ b/include/linux/cxl_accel_mem.h
>> @@ -19,4 +19,5 @@ void cxl_accel_set_dvsec(cxl_accel_state *cxlds, u16 dvsec);
>>   void cxl_accel_set_serial(cxl_accel_state *cxlds, u64 serial);
>>   void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res,
>>   			    enum accel_resource);
>> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
>>   #endif
Alejandro Lucero Palau Aug. 14, 2024, 7:56 a.m. UTC | #6 
On 8/4/24 18:15, Jonathan Cameron wrote:
> On Mon, 15 Jul 2024 18:28:22 +0100
> alejandro.lucero-palau@amd.com wrote:
>
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Create a new function for a type2 device initialising the opaque
>> cxl_dev_state struct regarding cxl regs setup and mapping.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/pci.c                  | 28 ++++++++++++++++++++++++++++
>>   drivers/net/ethernet/sfc/efx_cxl.c |  3 +++
>>   include/linux/cxl_accel_mem.h      |  1 +
>>   3 files changed, 32 insertions(+)
>>
>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>> index e53646e9f2fb..b34d6259faf4 100644
>> --- a/drivers/cxl/pci.c
>> +++ b/drivers/cxl/pci.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/pci.h>
>>   #include <linux/aer.h>
>>   #include <linux/io.h>
>> +#include <linux/cxl_accel_mem.h>
>>   #include "cxlmem.h"
>>   #include "cxlpci.h"
>>   #include "cxl.h"
>> @@ -521,6 +522,33 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
>>   	return cxl_setup_regs(map);
>>   }
>>   
>> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds)
>> +{
>> +	struct cxl_register_map map;
>> +	int rc;
>> +
>> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
>> +	if (rc)
>> +		return rc;
>> +
>> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
>> +				&cxlds->reg_map);
>> +	if (rc)
>> +		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
> Not fatal?  If we think it will happen on real devices, then dev_warn
> is too strong.


This is more complex than what it seems, and it is not properly handled 
with the current code.

I will cover it in another patch in more detail, but the fact is those 
calls to cxl_pci_setup_regs need to be handled better, because Type2 has 
some of these registers as optional.


>> +
>> +	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");
> pci_err() or similar would make sense here as we have asked for something
> that isn't happening. Specification says this is mandatory so
> definitely smells like a fatal error to me.
>
>
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, CXL);
>> +
>>   static int cxl_pci_ras_unmask(struct pci_dev *pdev)
>>   {
>>   	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> index 4554dd7cca76..10c4fb915278 100644
>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -47,6 +47,9 @@ void efx_cxl_init(struct efx_nic *efx)
>>   
>>   	res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram");
>>   	cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_RAM);
>> +
>> +	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds))
>> +		pci_info(pci_dev, "CXL accel setup regs failed");
> Handle errors fully. That is report them  up to the caller.
>
>>   }
>>   
>>   
>> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
>> index daf46d41f59c..ca7af4a9cefc 100644
>> --- a/include/linux/cxl_accel_mem.h
>> +++ b/include/linux/cxl_accel_mem.h
>> @@ -19,4 +19,5 @@ void cxl_accel_set_dvsec(cxl_accel_state *cxlds, u16 dvsec);
>>   void cxl_accel_set_serial(cxl_accel_state *cxlds, u64 serial);
>>   void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res,
>>   			    enum accel_resource);
>> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
>>   #endif
Jonathan Cameron Aug. 15, 2024, 4:40 p.m. UTC | #7 
On Wed, 14 Aug 2024 08:56:35 +0100
Alejandro Lucero Palau <alucerop@amd.com> wrote:

> On 8/4/24 18:15, Jonathan Cameron wrote:
> > On Mon, 15 Jul 2024 18:28:22 +0100
> > alejandro.lucero-palau@amd.com wrote:
> >  
> >> From: Alejandro Lucero <alucerop@amd.com>
> >>
> >> Create a new function for a type2 device initialising the opaque
> >> cxl_dev_state struct regarding cxl regs setup and mapping.
> >>
> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> >> ---
> >>   drivers/cxl/pci.c                  | 28 ++++++++++++++++++++++++++++
> >>   drivers/net/ethernet/sfc/efx_cxl.c |  3 +++
> >>   include/linux/cxl_accel_mem.h      |  1 +
> >>   3 files changed, 32 insertions(+)
> >>
> >> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> >> index e53646e9f2fb..b34d6259faf4 100644
> >> --- a/drivers/cxl/pci.c
> >> +++ b/drivers/cxl/pci.c
> >> @@ -11,6 +11,7 @@
> >>   #include <linux/pci.h>
> >>   #include <linux/aer.h>
> >>   #include <linux/io.h>
> >> +#include <linux/cxl_accel_mem.h>
> >>   #include "cxlmem.h"
> >>   #include "cxlpci.h"
> >>   #include "cxl.h"
> >> @@ -521,6 +522,33 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> >>   	return cxl_setup_regs(map);
> >>   }
> >>   
> >> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds)
> >> +{
> >> +	struct cxl_register_map map;
> >> +	int rc;
> >> +
> >> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
> >> +				&cxlds->reg_map);
> >> +	if (rc)
> >> +		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);  
> > Not fatal?  If we think it will happen on real devices, then dev_warn
> > is too strong.  
> 
> 
> This is more complex than what it seems, and it is not properly handled 
> with the current code.
> 
> I will cover it in another patch in more detail, but the fact is those 
> calls to cxl_pci_setup_regs need to be handled better, because Type2 has 
> some of these registers as optional.

I'd argue you don't have to support all type 2 devices with your
first code.  Things like optionality of registers can come in when
a device shows up where they aren't present.

Jonathan
Zhi Wang Aug. 18, 2024, 8:07 a.m. UTC | #8 
On Thu, 15 Aug 2024 17:40:35 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Wed, 14 Aug 2024 08:56:35 +0100
> Alejandro Lucero Palau <alucerop@amd.com> wrote:
> 
> > On 8/4/24 18:15, Jonathan Cameron wrote:
> > > On Mon, 15 Jul 2024 18:28:22 +0100
> > > alejandro.lucero-palau@amd.com wrote:
> > >  
> > >> From: Alejandro Lucero <alucerop@amd.com>
> > >>
> > >> Create a new function for a type2 device initialising the opaque
> > >> cxl_dev_state struct regarding cxl regs setup and mapping.
> > >>
> > >> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> > >> ---
> > >>   drivers/cxl/pci.c                  | 28
> > >> ++++++++++++++++++++++++++++ drivers/net/ethernet/sfc/efx_cxl.c
> > >> |  3 +++ include/linux/cxl_accel_mem.h      |  1 +
> > >>   3 files changed, 32 insertions(+)
> > >>
> > >> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > >> index e53646e9f2fb..b34d6259faf4 100644
> > >> --- a/drivers/cxl/pci.c
> > >> +++ b/drivers/cxl/pci.c
> > >> @@ -11,6 +11,7 @@
> > >>   #include <linux/pci.h>
> > >>   #include <linux/aer.h>
> > >>   #include <linux/io.h>
> > >> +#include <linux/cxl_accel_mem.h>
> > >>   #include "cxlmem.h"
> > >>   #include "cxlpci.h"
> > >>   #include "cxl.h"
> > >> @@ -521,6 +522,33 @@ static int cxl_pci_setup_regs(struct
> > >> pci_dev *pdev, enum cxl_regloc_type type, return
> > >> cxl_setup_regs(map); }
> > >>   
> > >> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct
> > >> cxl_dev_state *cxlds) +{
> > >> +	struct cxl_register_map map;
> > >> +	int rc;
> > >> +
> > >> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV,
> > >> &map);
> > >> +	if (rc)
> > >> +		return rc;
> > >> +
> > >> +	rc = cxl_map_device_regs(&map,
> > >> &cxlds->regs.device_regs);
> > >> +	if (rc)
> > >> +		return rc;
> > >> +
> > >> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
> > >> +				&cxlds->reg_map);
> > >> +	if (rc)
> > >> +		dev_warn(&pdev->dev, "No component registers
> > >> (%d)\n", rc);  
> > > Not fatal?  If we think it will happen on real devices, then
> > > dev_warn is too strong.  
> > 
> > 
> > This is more complex than what it seems, and it is not properly
> > handled with the current code.
> > 
> > I will cover it in another patch in more detail, but the fact is
> > those calls to cxl_pci_setup_regs need to be handled better,
> > because Type2 has some of these registers as optional.
> 
> I'd argue you don't have to support all type 2 devices with your
> first code.  Things like optionality of registers can come in when
> a device shows up where they aren't present.
> 
> Jonathan
> 

I think it is more like we need to change those register
probe routines to probe and return the result, but not decide
if the result is fatal or not. Let the caller decide it. E.g. type-3
assumes some registers group must be present, then the caller of type-3
can throw a fatal. While, type-2 just need to remember if the register
group is present or not. A register group is missing might not be fatal
to a type-2.

E.g.

1) moving the judges out of cxl_probe_regs() and wrap them into a
function. e.g. cxl_check_check_device_regs():
        case CXL_REGLOC_RBI_MEMDEV:
                dev_map = &map->device_map;
                cxl_probe_device_regs(host, base, dev_map);

		/* Moving the judeges out of here. */
                if (!dev_map->status.valid ||
                    ((caps & CXL_DRIVER_CAP_MBOX) &&
                !dev_map->mbox.valid) || !dev_map->memdev.valid) {
                        dev_err(host, "registers not found: %s%s%s\n",
                                !dev_map->status.valid ? "status " : "",
                                ((caps & CXL_DRIVER_CAP_MBOX) &&
                !dev_map->mbox.valid) ? "mbox " : "",
                !dev_map->memdev.valid ? "memdev " : ""); return -ENXIO;
                }

2) At the top caller for type-3 cxl_pci_probe():

        rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map,
                                cxlds->capabilities);
        if (rc)
                return rc;

	/* call cxl_check_device_regs() here, if fail, throw fatal! */

3) At the top caller for type-2 cxl_pci_accel_setup_regs():

	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map,
                                cxlds->capabilities);
        if (rc)
                return rc;

/* call cxl_check_device_regs() here,
 * if succeed, map the registers
 * if fail, move on, no need to throw fatal.
 */
	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
        if (rc)
                return rc;

With the changes, we can let the CXL core detects what the registers the
device has, maybe the driver even doesn't need to tell the CXL core,
what caps the driver/device has, then we don't need to introduce the
cxlds->capabilities? the CXL core just go to check if a register group's
vaddr mapping is present, then it knows if the device has a
register group or not, after the cxl_pci_accel_setup_regs().

Thanks,
Zhi.
Alejandro Lucero Palau Aug. 19, 2024, 11:28 a.m. UTC | #9 
On 8/18/24 09:07, Zhi Wang wrote:
> On Thu, 15 Aug 2024 17:40:35 +0100
> Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
>
>> On Wed, 14 Aug 2024 08:56:35 +0100
>> Alejandro Lucero Palau <alucerop@amd.com> wrote:
>>
>>> On 8/4/24 18:15, Jonathan Cameron wrote:
>>>> On Mon, 15 Jul 2024 18:28:22 +0100
>>>> alejandro.lucero-palau@amd.com wrote:
>>>>   
>>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>>
>>>>> Create a new function for a type2 device initialising the opaque
>>>>> cxl_dev_state struct regarding cxl regs setup and mapping.
>>>>>
>>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>>> ---
>>>>>    drivers/cxl/pci.c                  | 28
>>>>> ++++++++++++++++++++++++++++ drivers/net/ethernet/sfc/efx_cxl.c
>>>>> |  3 +++ include/linux/cxl_accel_mem.h      |  1 +
>>>>>    3 files changed, 32 insertions(+)
>>>>>
>>>>> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
>>>>> index e53646e9f2fb..b34d6259faf4 100644
>>>>> --- a/drivers/cxl/pci.c
>>>>> +++ b/drivers/cxl/pci.c
>>>>> @@ -11,6 +11,7 @@
>>>>>    #include <linux/pci.h>
>>>>>    #include <linux/aer.h>
>>>>>    #include <linux/io.h>
>>>>> +#include <linux/cxl_accel_mem.h>
>>>>>    #include "cxlmem.h"
>>>>>    #include "cxlpci.h"
>>>>>    #include "cxl.h"
>>>>> @@ -521,6 +522,33 @@ static int cxl_pci_setup_regs(struct
>>>>> pci_dev *pdev, enum cxl_regloc_type type, return
>>>>> cxl_setup_regs(map); }
>>>>>    
>>>>> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct
>>>>> cxl_dev_state *cxlds) +{
>>>>> +	struct cxl_register_map map;
>>>>> +	int rc;
>>>>> +
>>>>> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV,
>>>>> &map);
>>>>> +	if (rc)
>>>>> +		return rc;
>>>>> +
>>>>> +	rc = cxl_map_device_regs(&map,
>>>>> &cxlds->regs.device_regs);
>>>>> +	if (rc)
>>>>> +		return rc;
>>>>> +
>>>>> +	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
>>>>> +				&cxlds->reg_map);
>>>>> +	if (rc)
>>>>> +		dev_warn(&pdev->dev, "No component registers
>>>>> (%d)\n", rc);
>>>> Not fatal?  If we think it will happen on real devices, then
>>>> dev_warn is too strong.
>>>
>>> This is more complex than what it seems, and it is not properly
>>> handled with the current code.
>>>
>>> I will cover it in another patch in more detail, but the fact is
>>> those calls to cxl_pci_setup_regs need to be handled better,
>>> because Type2 has some of these registers as optional.
>> I'd argue you don't have to support all type 2 devices with your
>> first code.  Things like optionality of registers can come in when
>> a device shows up where they aren't present.
>>
>> Jonathan
>>
> I think it is more like we need to change those register
> probe routines to probe and return the result, but not decide
> if the result is fatal or not. Let the caller decide it. E.g. type-3
> assumes some registers group must be present, then the caller of type-3
> can throw a fatal. While, type-2 just need to remember if the register
> group is present or not. A register group is missing might not be fatal
> to a type-2.


I agree.


> E.g.
>
> 1) moving the judges out of cxl_probe_regs() and wrap them into a
> function. e.g. cxl_check_check_device_regs():
>          case CXL_REGLOC_RBI_MEMDEV:
>                  dev_map = &map->device_map;
>                  cxl_probe_device_regs(host, base, dev_map);
>
> 		/* Moving the judeges out of here. */
>                  if (!dev_map->status.valid ||
>                      ((caps & CXL_DRIVER_CAP_MBOX) &&
>                  !dev_map->mbox.valid) || !dev_map->memdev.valid) {
>                          dev_err(host, "registers not found: %s%s%s\n",
>                                  !dev_map->status.valid ? "status " : "",
>                                  ((caps & CXL_DRIVER_CAP_MBOX) &&
>                  !dev_map->mbox.valid) ? "mbox " : "",
>                  !dev_map->memdev.valid ? "memdev " : ""); return -ENXIO;
>                  }
>
> 2) At the top caller for type-3 cxl_pci_probe():
>
>          rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map,
>                                  cxlds->capabilities);
>          if (rc)
>                  return rc;
>
> 	/* call cxl_check_device_regs() here, if fail, throw fatal! */
>
> 3) At the top caller for type-2 cxl_pci_accel_setup_regs():
>
> 	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map,
>                                  cxlds->capabilities);
>          if (rc)
>                  return rc;
>
> /* call cxl_check_device_regs() here,
>   * if succeed, map the registers
>   * if fail, move on, no need to throw fatal.
>   */
> 	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
>          if (rc)
>                  return rc;
>
> With the changes, we can let the CXL core detects what the registers the
> device has, maybe the driver even doesn't need to tell the CXL core,
> what caps the driver/device has, then we don't need to introduce the
> cxlds->capabilities? the CXL core just go to check if a register group's
> vaddr mapping is present, then it knows if the device has a
> register group or not, after the cxl_pci_accel_setup_regs().


I thought about building up the device capabilities based on what the 
registers show instead of explicitly stated by the driver, what I think 
it is your point, but I think we need those capabilities in one way or 
another, not just for pure information purposes but also for finding out 
if other initialization should fail or not, what was the original goal 
behind this patch. The driver could also define those capabilities to 
expect and check out after identified by the registers initialization if 
they match.


So yes, I think it could go this way, but I would prefer to do such a 
refactoring after this initial type2 support.


> Thanks,
> Zhi.
>
diff mbox series

Patch

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index e53646e9f2fb..b34d6259faf4 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -11,6 +11,7 @@ 
 #include <linux/pci.h>
 #include <linux/aer.h>
 #include <linux/io.h>
+#include <linux/cxl_accel_mem.h>
 #include "cxlmem.h"
 #include "cxlpci.h"
 #include "cxl.h"
@@ -521,6 +522,33 @@  static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
 	return cxl_setup_regs(map);
 }
 
+int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds)
+{
+	struct cxl_register_map map;
+	int rc;
+
+	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map);
+	if (rc)
+		return rc;
+
+	rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs);
+	if (rc)
+		return rc;
+
+	rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT,
+				&cxlds->reg_map);
+	if (rc)
+		dev_warn(&pdev->dev, "No component registers (%d)\n", rc);
+
+	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);
+
 static int cxl_pci_ras_unmask(struct pci_dev *pdev)
 {
 	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
index 4554dd7cca76..10c4fb915278 100644
--- a/drivers/net/ethernet/sfc/efx_cxl.c
+++ b/drivers/net/ethernet/sfc/efx_cxl.c
@@ -47,6 +47,9 @@  void efx_cxl_init(struct efx_nic *efx)
 
 	res = DEFINE_RES_MEM_NAMED(0, EFX_CTPIO_BUFFER_SIZE, "ram");
 	cxl_accel_set_resource(cxl->cxlds, res, CXL_ACCEL_RES_RAM);
+
+	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds))
+		pci_info(pci_dev, "CXL accel setup regs failed");
 }
 
 
diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
index daf46d41f59c..ca7af4a9cefc 100644
--- a/include/linux/cxl_accel_mem.h
+++ b/include/linux/cxl_accel_mem.h
@@ -19,4 +19,5 @@  void cxl_accel_set_dvsec(cxl_accel_state *cxlds, u16 dvsec);
 void cxl_accel_set_serial(cxl_accel_state *cxlds, u64 serial);
 void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res,
 			    enum accel_resource);
+int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
 #endif