diff mbox series

[v2,03/15] cxl: add function for type2 resource request

Message ID 20240715172835.24757-4-alejandro.lucero-palau@amd.com
State Superseded
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 requesting a resource
passing the opaque struct to work with.

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

Comments

Dave Jiang July 18, 2024, 11:36 p.m. UTC | #1 
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 requesting a resource
> passing the opaque struct to work with.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/memdev.c          | 13 +++++++++++++
>  drivers/net/ethernet/sfc/efx_cxl.c |  7 ++++++-
>  include/linux/cxl_accel_mem.h      |  1 +
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 61b5d35b49e7..04c3a0f8bc2e 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -744,6 +744,19 @@ void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res,
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL);
>  
> +int cxl_accel_request_resource(struct cxl_dev_state *cxlds, bool is_ram)
Maybe declare a common enum like cxl_resource_type instead of 'enum accel_resource' and use here instead of bool?

> +{
> +	int rc;
> +
> +	if (is_ram)
> +		rc = request_resource(&cxlds->dpa_res, &cxlds->ram_res);
> +	else
> +		rc = request_resource(&cxlds->dpa_res, &cxlds->pmem_res);
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_accel_request_resource, CXL);
> +
>  static int cxl_memdev_release_file(struct inode *inode, struct file *file)
>  {
>  	struct cxl_memdev *cxlmd =
> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 10c4fb915278..9cefcaf3caca 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -48,8 +48,13 @@ 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))
> +	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds)) {
>  		pci_info(pci_dev, "CXL accel setup regs failed");
> +		return;
> +	}
> +
> +	if (cxl_accel_request_resource(cxl->cxlds, true))
> +		pci_info(pci_dev, "CXL accel resource request failed");

pci_warn()? also emitting the errno would be nice. 

>  }
>  
>  
> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
> index ca7af4a9cefc..c7b254edc096 100644
> --- a/include/linux/cxl_accel_mem.h
> +++ b/include/linux/cxl_accel_mem.h
> @@ -20,4 +20,5 @@ 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);
> +int cxl_accel_request_resource(struct cxl_dev_state *cxlds, bool is_ram);
>  #endif
Jonathan Cameron Aug. 4, 2024, 5:16 p.m. UTC | #2 
On Thu, 18 Jul 2024 16:36:00 -0700
Dave Jiang <dave.jiang@intel.com> 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 requesting a resource
> > passing the opaque struct to work with.
> > 
> > Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> > ---
> >  drivers/cxl/core/memdev.c          | 13 +++++++++++++
> >  drivers/net/ethernet/sfc/efx_cxl.c |  7 ++++++-
> >  include/linux/cxl_accel_mem.h      |  1 +
> >  3 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> > index 61b5d35b49e7..04c3a0f8bc2e 100644
> > --- a/drivers/cxl/core/memdev.c
> > +++ b/drivers/cxl/core/memdev.c
> > @@ -744,6 +744,19 @@ void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res,
> >  }
> >  EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL);
> >  
> > +int cxl_accel_request_resource(struct cxl_dev_state *cxlds, bool is_ram)  
> Maybe declare a common enum like cxl_resource_type instead of 'enum accel_resource' and use here instead of bool?
> 
> > +{
> > +	int rc;
> > +
> > +	if (is_ram)
> > +		rc = request_resource(&cxlds->dpa_res, &cxlds->ram_res);
> > +	else
> > +		rc = request_resource(&cxlds->dpa_res, &cxlds->pmem_res);
> > +
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(cxl_accel_request_resource, CXL);
> > +
> >  static int cxl_memdev_release_file(struct inode *inode, struct file *file)
> >  {
> >  	struct cxl_memdev *cxlmd =
> > diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> > index 10c4fb915278..9cefcaf3caca 100644
> > --- a/drivers/net/ethernet/sfc/efx_cxl.c
> > +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> > @@ -48,8 +48,13 @@ 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))
> > +	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds)) {
> >  		pci_info(pci_dev, "CXL accel setup regs failed");
> > +		return;
> > +	}
> > +
> > +	if (cxl_accel_request_resource(cxl->cxlds, true))
> > +		pci_info(pci_dev, "CXL accel resource request failed");  
> 
> pci_warn()? also emitting the errno would be nice. 
Don't hide it at all.  Fail if this doesn't succeed and let the caller
know. Not to mention, tear down any other state already set up.
 
> >  }
> >  
> >  
> > diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
> > index ca7af4a9cefc..c7b254edc096 100644
> > --- a/include/linux/cxl_accel_mem.h
> > +++ b/include/linux/cxl_accel_mem.h
> > @@ -20,4 +20,5 @@ 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);
> > +int cxl_accel_request_resource(struct cxl_dev_state *cxlds, bool is_ram);
> >  #endif  
>
Zhi Wang Aug. 9, 2024, 9:01 a.m. UTC | #3 
On Mon, 15 Jul 2024 18:28:23 +0100
<alejandro.lucero-palau@amd.com> wrote:

> From: Alejandro Lucero <alucerop@amd.com>
> 
> Create a new function for a type2 device requesting a resource
> passing the opaque struct to work with.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/memdev.c          | 13 +++++++++++++
>  drivers/net/ethernet/sfc/efx_cxl.c |  7 ++++++-
>  include/linux/cxl_accel_mem.h      |  1 +
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 61b5d35b49e7..04c3a0f8bc2e 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -744,6 +744,19 @@ void cxl_accel_set_resource(struct cxl_dev_state
> *cxlds, struct resource res, }
>  EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL);
>  
> +int cxl_accel_request_resource(struct cxl_dev_state *cxlds, bool
> is_ram) +{
> +	int rc;
> +

In PATCH 1, you got the resource type enumeration. Let's use them here
instead of a bool. 

> +	if (is_ram)
> +		rc = request_resource(&cxlds->dpa_res,
> &cxlds->ram_res);
> +	else
> +		rc = request_resource(&cxlds->dpa_res,
> &cxlds->pmem_res); +
> +	return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_accel_request_resource, CXL);
> +
>  static int cxl_memdev_release_file(struct inode *inode, struct file
> *file) {
>  	struct cxl_memdev *cxlmd =
> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c
> b/drivers/net/ethernet/sfc/efx_cxl.c index 10c4fb915278..9cefcaf3caca
> 100644 --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -48,8 +48,13 @@ 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))
> +	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds)) {
>  		pci_info(pci_dev, "CXL accel setup regs failed");
> +		return;
> +	}
> +
> +	if (cxl_accel_request_resource(cxl->cxlds, true))
> +		pci_info(pci_dev, "CXL accel resource request
> failed"); }
>  
> 

The guidelines of error reporting from a driver is mostly considered
from the user perspective. If it is an error, shout, let the user know
what happened. Otherwise, we usually don't disturb the user other than
telling them we are loaded and everything works fine.

Please use pci_err() instead. So the user can spot it from a
message folder filtered by error level in a kernel dmesg logger.

> diff --git a/include/linux/cxl_accel_mem.h
> b/include/linux/cxl_accel_mem.h index ca7af4a9cefc..c7b254edc096
> 100644 --- a/include/linux/cxl_accel_mem.h
> +++ b/include/linux/cxl_accel_mem.h
> @@ -20,4 +20,5 @@ 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); +int cxl_accel_request_resource(struct
> cxl_dev_state *cxlds, bool is_ram); #endif
Alejandro Lucero Palau Aug. 14, 2024, 8 a.m. UTC | #4 
On 7/19/24 00:36, 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 requesting a resource
>> passing the opaque struct to work with.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/core/memdev.c          | 13 +++++++++++++
>>   drivers/net/ethernet/sfc/efx_cxl.c |  7 ++++++-
>>   include/linux/cxl_accel_mem.h      |  1 +
>>   3 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index 61b5d35b49e7..04c3a0f8bc2e 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -744,6 +744,19 @@ void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res,
>>   }
>>   EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL);
>>   
>> +int cxl_accel_request_resource(struct cxl_dev_state *cxlds, bool is_ram)
> Maybe declare a common enum like cxl_resource_type instead of 'enum accel_resource' and use here instead of bool?


Yes. Thanks

>> +{
>> +	int rc;
>> +
>> +	if (is_ram)
>> +		rc = request_resource(&cxlds->dpa_res, &cxlds->ram_res);
>> +	else
>> +		rc = request_resource(&cxlds->dpa_res, &cxlds->pmem_res);
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_accel_request_resource, CXL);
>> +
>>   static int cxl_memdev_release_file(struct inode *inode, struct file *file)
>>   {
>>   	struct cxl_memdev *cxlmd =
>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>> index 10c4fb915278..9cefcaf3caca 100644
>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>> @@ -48,8 +48,13 @@ 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))
>> +	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds)) {
>>   		pci_info(pci_dev, "CXL accel setup regs failed");
>> +		return;
>> +	}
>> +
>> +	if (cxl_accel_request_resource(cxl->cxlds, true))
>> +		pci_info(pci_dev, "CXL accel resource request failed");
> pci_warn()? also emitting the errno would be nice.
>
>>   }
>>   
>>   
>> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
>> index ca7af4a9cefc..c7b254edc096 100644
>> --- a/include/linux/cxl_accel_mem.h
>> +++ b/include/linux/cxl_accel_mem.h
>> @@ -20,4 +20,5 @@ 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);
>> +int cxl_accel_request_resource(struct cxl_dev_state *cxlds, bool is_ram);
>>   #endif
Alejandro Lucero Palau Aug. 14, 2024, 8:08 a.m. UTC | #5 
On 8/4/24 18:16, Jonathan Cameron wrote:
> On Thu, 18 Jul 2024 16:36:00 -0700
> Dave Jiang <dave.jiang@intel.com> 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 requesting a resource
>>> passing the opaque struct to work with.
>>>
>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>> ---
>>>   drivers/cxl/core/memdev.c          | 13 +++++++++++++
>>>   drivers/net/ethernet/sfc/efx_cxl.c |  7 ++++++-
>>>   include/linux/cxl_accel_mem.h      |  1 +
>>>   3 files changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>>> index 61b5d35b49e7..04c3a0f8bc2e 100644
>>> --- a/drivers/cxl/core/memdev.c
>>> +++ b/drivers/cxl/core/memdev.c
>>> @@ -744,6 +744,19 @@ void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res,
>>>   }
>>>   EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL);
>>>   
>>> +int cxl_accel_request_resource(struct cxl_dev_state *cxlds, bool is_ram)
>> Maybe declare a common enum like cxl_resource_type instead of 'enum accel_resource' and use here instead of bool?
>>
>>> +{
>>> +	int rc;
>>> +
>>> +	if (is_ram)
>>> +		rc = request_resource(&cxlds->dpa_res, &cxlds->ram_res);
>>> +	else
>>> +		rc = request_resource(&cxlds->dpa_res, &cxlds->pmem_res);
>>> +
>>> +	return rc;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(cxl_accel_request_resource, CXL);
>>> +
>>>   static int cxl_memdev_release_file(struct inode *inode, struct file *file)
>>>   {
>>>   	struct cxl_memdev *cxlmd =
>>> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
>>> index 10c4fb915278..9cefcaf3caca 100644
>>> --- a/drivers/net/ethernet/sfc/efx_cxl.c
>>> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
>>> @@ -48,8 +48,13 @@ 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))
>>> +	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds)) {
>>>   		pci_info(pci_dev, "CXL accel setup regs failed");
>>> +		return;
>>> +	}
>>> +
>>> +	if (cxl_accel_request_resource(cxl->cxlds, true))
>>> +		pci_info(pci_dev, "CXL accel resource request failed");
>> pci_warn()? also emitting the errno would be nice.
> Don't hide it at all.  Fail if this doesn't succeed and let the caller
> know. Not to mention, tear down any other state already set up.
>   


It is obvious I have problems with the way errors are reported, 
specifically about what should be considered a serious problem not 
expected at all.

I think we can expect some unexpected situations with the novelty behind 
CXL and, indeed, Type2 support. I guess a good approach could be to be 
chatty at this point and refine the way these errors are reported later 
when the maturity of the support and our experience give us the knowledge.


>>>   }
>>>   
>>>   
>>> diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
>>> index ca7af4a9cefc..c7b254edc096 100644
>>> --- a/include/linux/cxl_accel_mem.h
>>> +++ b/include/linux/cxl_accel_mem.h
>>> @@ -20,4 +20,5 @@ 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);
>>> +int cxl_accel_request_resource(struct cxl_dev_state *cxlds, bool is_ram);
>>>   #endif
Zhi Wang Aug. 22, 2024, 1:07 p.m. UTC | #6 
On Mon, 15 Jul 2024 18:28:23 +0100
<alejandro.lucero-palau@amd.com> wrote:

> From: Alejandro Lucero <alucerop@amd.com>
> 
> Create a new function for a type2 device requesting a resource
> passing the opaque struct to work with.
> 
> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
> ---
>  drivers/cxl/core/memdev.c          | 13 +++++++++++++
>  drivers/net/ethernet/sfc/efx_cxl.c |  7 ++++++-
>  include/linux/cxl_accel_mem.h      |  1 +
>  3 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 61b5d35b49e7..04c3a0f8bc2e 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -744,6 +744,19 @@ void cxl_accel_set_resource(struct cxl_dev_state
> *cxlds, struct resource res, }
>  EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL);
>  
> +int cxl_accel_request_resource(struct cxl_dev_state *cxlds, bool
> is_ram) +{
> +	int rc;
> +
> +	if (is_ram)
> +		rc = request_resource(&cxlds->dpa_res,
> &cxlds->ram_res);
> +	else
> +		rc = request_resource(&cxlds->dpa_res,
> &cxlds->pmem_res); +
> +	return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_accel_request_resource, CXL);
> +

Hi Alejandro:

Since we only have cxl_accel_request_resource() here, how is
the resource going to be released? e.g. in an error handling path. 

Thanks,
Zhi.

>  static int cxl_memdev_release_file(struct inode *inode, struct file
> *file) {
>  	struct cxl_memdev *cxlmd =
> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c
> b/drivers/net/ethernet/sfc/efx_cxl.c index 10c4fb915278..9cefcaf3caca
> 100644 --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -48,8 +48,13 @@ 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))
> +	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds)) {
>  		pci_info(pci_dev, "CXL accel setup regs failed");
> +		return;
> +	}
> +
> +	if (cxl_accel_request_resource(cxl->cxlds, true))
> +		pci_info(pci_dev, "CXL accel resource request
> failed"); }
>  
>  
> diff --git a/include/linux/cxl_accel_mem.h
> b/include/linux/cxl_accel_mem.h index ca7af4a9cefc..c7b254edc096
> 100644 --- a/include/linux/cxl_accel_mem.h
> +++ b/include/linux/cxl_accel_mem.h
> @@ -20,4 +20,5 @@ 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); +int cxl_accel_request_resource(struct
> cxl_dev_state *cxlds, bool is_ram); #endif
Alejandro Lucero Palau Aug. 23, 2024, 9:30 a.m. UTC | #7 
On 8/22/24 14:07, Zhi Wang wrote:
> On Mon, 15 Jul 2024 18:28:23 +0100
> <alejandro.lucero-palau@amd.com> wrote:
>
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Create a new function for a type2 device requesting a resource
>> passing the opaque struct to work with.
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> ---
>>   drivers/cxl/core/memdev.c          | 13 +++++++++++++
>>   drivers/net/ethernet/sfc/efx_cxl.c |  7 ++++++-
>>   include/linux/cxl_accel_mem.h      |  1 +
>>   3 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index 61b5d35b49e7..04c3a0f8bc2e 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -744,6 +744,19 @@ void cxl_accel_set_resource(struct cxl_dev_state
>> *cxlds, struct resource res, }
>>   EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL);
>>   
>> +int cxl_accel_request_resource(struct cxl_dev_state *cxlds, bool
>> is_ram) +{
>> +	int rc;
>> +
>> +	if (is_ram)
>> +		rc = request_resource(&cxlds->dpa_res,
>> &cxlds->ram_res);
>> +	else
>> +		rc = request_resource(&cxlds->dpa_res,
>> &cxlds->pmem_res); +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_accel_request_resource, CXL);
>> +
> Hi Alejandro:
>
> Since we only have cxl_accel_request_resource() here, how is
> the resource going to be released? e.g. in an error handling path.
>
> Thanks,
> Zhi.
>

Right. I will use devm_request_resource in v3 using cxlds->dev and the 
owner.


Thanks
diff mbox series

Patch

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 61b5d35b49e7..04c3a0f8bc2e 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -744,6 +744,19 @@  void cxl_accel_set_resource(struct cxl_dev_state *cxlds, struct resource res,
 }
 EXPORT_SYMBOL_NS_GPL(cxl_accel_set_resource, CXL);
 
+int cxl_accel_request_resource(struct cxl_dev_state *cxlds, bool is_ram)
+{
+	int rc;
+
+	if (is_ram)
+		rc = request_resource(&cxlds->dpa_res, &cxlds->ram_res);
+	else
+		rc = request_resource(&cxlds->dpa_res, &cxlds->pmem_res);
+
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_accel_request_resource, CXL);
+
 static int cxl_memdev_release_file(struct inode *inode, struct file *file)
 {
 	struct cxl_memdev *cxlmd =
diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
index 10c4fb915278..9cefcaf3caca 100644
--- a/drivers/net/ethernet/sfc/efx_cxl.c
+++ b/drivers/net/ethernet/sfc/efx_cxl.c
@@ -48,8 +48,13 @@  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))
+	if (cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds)) {
 		pci_info(pci_dev, "CXL accel setup regs failed");
+		return;
+	}
+
+	if (cxl_accel_request_resource(cxl->cxlds, true))
+		pci_info(pci_dev, "CXL accel resource request failed");
 }
 
 
diff --git a/include/linux/cxl_accel_mem.h b/include/linux/cxl_accel_mem.h
index ca7af4a9cefc..c7b254edc096 100644
--- a/include/linux/cxl_accel_mem.h
+++ b/include/linux/cxl_accel_mem.h
@@ -20,4 +20,5 @@  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);
+int cxl_accel_request_resource(struct cxl_dev_state *cxlds, bool is_ram);
 #endif