[v4,08/26] cxl: add functions for resource request/release by a driver

Commit Message

Lucero Palau, Alejandro Oct. 17, 2024, 4:52 p.m. UTC

From: Alejandro Lucero <[e-mail]>

Create accessors for an accel driver requesting and releasing a resource.

Signed-off-by: Alejandro Lucero <[e-mail]>
---
 drivers/cxl/core/memdev.c | 51 +++++++++++++++++++++++++++++++++++++++
 include/linux/cxl/cxl.h   |  2 ++
 2 files changed, 53 insertions(+)

Comments

Ben Cheatham Oct. 17, 2024, 9:49 p.m. UTC | #1

On 10/17/24 11:52 AM, [e-mail] wrote:
> From: Alejandro Lucero <[e-mail]>
> 
> Create accessors for an accel driver requesting and releasing a resource.
> 
> Signed-off-by: Alejandro Lucero <[e-mail]>
> ---
>  drivers/cxl/core/memdev.c | 51 +++++++++++++++++++++++++++++++++++++++
>  include/linux/cxl/cxl.h   |  2 ++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index 94b8a7b53c92..4b2641f20128 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -744,6 +744,57 @@ int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res,
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_set_resource, CXL);
>  
> +int cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource type)
> +{
> +	int rc;
> +
> +	switch (type) {
> +	case CXL_RES_RAM:
> +		if (!resource_size(&cxlds->ram_res)) {
> +			dev_err(cxlds->dev,
> +				"resource request for ram with size 0\n");
> +			return -EINVAL;
> +		}
> +
> +		rc = request_resource(&cxlds->dpa_res, &cxlds->ram_res);
> +		break;
> +	case CXL_RES_PMEM:
> +		if (!resource_size(&cxlds->pmem_res)) {
> +			dev_err(cxlds->dev,
> +				"resource request for pmem with size 0\n");
> +			return -EINVAL;
> +		}
> +		rc = request_resource(&cxlds->dpa_res, &cxlds->pmem_res);
> +		break;
> +	default:
> +		dev_err(cxlds->dev, "unsupported resource type (%u)\n", type);
> +		return -EINVAL;
> +	}
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_request_resource, CXL);

It looks like add_dpa_res() in cxl/core/mbox.c already does what you are doing here, minus the enum.
Is there a way that could be reused, or a good reason not too? Even if you don't export the function
outside of the cxl tree, you could reuse that function for the internals of this one.

> +
> +int cxl_release_resource(struct cxl_dev_state *cxlds, enum cxl_resource type)
> +{
> +	int rc;
> +
> +	switch (type) {
> +	case CXL_RES_RAM:
> +		rc = release_resource(&cxlds->ram_res);
> +		break;
> +	case CXL_RES_PMEM:
> +		rc = release_resource(&cxlds->pmem_res);
> +		break;
> +	default:
> +		dev_err(cxlds->dev, "unknown resource type (%u)\n", type);
> +		return -EINVAL;
> +	}
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_NS_GPL(cxl_release_resource, CXL);
> +

Same thing here, but with cxl_dpa_release() instead of add_dpa_res().

Looking at it some more, it looks like there is also some stuff to do with locking for CXL DPA resources in
that function that you would be skipping with your functions above. Will that be a problem later? I have no
clue, but thought I should ask just in case.

>  static int cxl_memdev_release_file(struct inode *inode, struct file *file)
>  {
>  	struct cxl_memdev *cxlmd =
> diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h
> index 2f48ee591259..6c6d27721067 100644
> --- a/include/linux/cxl/cxl.h
> +++ b/include/linux/cxl/cxl.h
> @@ -54,4 +54,6 @@ bool cxl_pci_check_caps(struct cxl_dev_state *cxlds,
>  			unsigned long *expected_caps,
>  			unsigned long *current_caps);
>  int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
> +int cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
> +int cxl_release_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
>  #endif

Alejandro Lucero Palau Oct. 18, 2024, 2:58 p.m. UTC | #2

On 10/17/24 22:49, Ben Cheatham wrote:
> On 10/17/24 11:52 AM, [e-mail] wrote:
>> From: Alejandro Lucero <[e-mail]>
>>
>> Create accessors for an accel driver requesting and releasing a resource.
>>
>> Signed-off-by: Alejandro Lucero <[e-mail]>
>> ---
>>   drivers/cxl/core/memdev.c | 51 +++++++++++++++++++++++++++++++++++++++
>>   include/linux/cxl/cxl.h   |  2 ++
>>   2 files changed, 53 insertions(+)
>>
>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>> index 94b8a7b53c92..4b2641f20128 100644
>> --- a/drivers/cxl/core/memdev.c
>> +++ b/drivers/cxl/core/memdev.c
>> @@ -744,6 +744,57 @@ int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res,
>>   }
>>   EXPORT_SYMBOL_NS_GPL(cxl_set_resource, CXL);
>>   
>> +int cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource type)
>> +{
>> +	int rc;
>> +
>> +	switch (type) {
>> +	case CXL_RES_RAM:
>> +		if (!resource_size(&cxlds->ram_res)) {
>> +			dev_err(cxlds->dev,
>> +				"resource request for ram with size 0\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		rc = request_resource(&cxlds->dpa_res, &cxlds->ram_res);
>> +		break;
>> +	case CXL_RES_PMEM:
>> +		if (!resource_size(&cxlds->pmem_res)) {
>> +			dev_err(cxlds->dev,
>> +				"resource request for pmem with size 0\n");
>> +			return -EINVAL;
>> +		}
>> +		rc = request_resource(&cxlds->dpa_res, &cxlds->pmem_res);
>> +		break;
>> +	default:
>> +		dev_err(cxlds->dev, "unsupported resource type (%u)\n", type);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_request_resource, CXL);
> It looks like add_dpa_res() in cxl/core/mbox.c already does what you are doing here, minus the enum.
> Is there a way that could be reused, or a good reason not too? Even if you don't export the function
> outside of the cxl tree, you could reuse that function for the internals of this one.


Although they are obviously similar, I think it makes sense to keep 
both. The CXL accel API is being implemented for avoiding accel drivers 
to manipulate cxl structs but through the API calls. With add_dpa_res we 
would break that, and calling it from the new cxl_request_resource would 
need changes as inside add_dpa_res the resource is initialized what has 
already been done in this implementation. IMO, those changes would make 
the code uglier.


Moreover, your comment below about cxl_dpa_release is, I think, wrong, 
since inside that function other things are being done related to 
regions. BTW, I can not see other release_resource calls from the 
current code than those added by this patch.


So, , I'm not keen to change this now, but maybe a good follow-up work.


>> +
>> +int cxl_release_resource(struct cxl_dev_state *cxlds, enum cxl_resource type)
>> +{
>> +	int rc;
>> +
>> +	switch (type) {
>> +	case CXL_RES_RAM:
>> +		rc = release_resource(&cxlds->ram_res);
>> +		break;
>> +	case CXL_RES_PMEM:
>> +		rc = release_resource(&cxlds->pmem_res);
>> +		break;
>> +	default:
>> +		dev_err(cxlds->dev, "unknown resource type (%u)\n", type);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return rc;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_release_resource, CXL);
>> +
> Same thing here, but with cxl_dpa_release() instead of add_dpa_res().
>
> Looking at it some more, it looks like there is also some stuff to do with locking for CXL DPA resources in
> that function that you would be skipping with your functions above. Will that be a problem later? I have no
> clue, but thought I should ask just in case.
>
>>   static int cxl_memdev_release_file(struct inode *inode, struct file *file)
>>   {
>>   	struct cxl_memdev *cxlmd =
>> diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h
>> index 2f48ee591259..6c6d27721067 100644
>> --- a/include/linux/cxl/cxl.h
>> +++ b/include/linux/cxl/cxl.h
>> @@ -54,4 +54,6 @@ bool cxl_pci_check_caps(struct cxl_dev_state *cxlds,
>>   			unsigned long *expected_caps,
>>   			unsigned long *current_caps);
>>   int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
>> +int cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
>> +int cxl_release_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
>>   #endif

Ben Cheatham Oct. 18, 2024, 4:40 p.m. UTC | #3

On 10/18/24 9:58 AM, Alejandro Lucero Palau wrote:
> 
> On 10/17/24 22:49, Ben Cheatham wrote:
>> On 10/17/24 11:52 AM, [e-mail] wrote:
>>> From: Alejandro Lucero <[e-mail]>
>>>
>>> Create accessors for an accel driver requesting and releasing a resource.
>>>
>>> Signed-off-by: Alejandro Lucero <[e-mail]>
>>> ---
>>>   drivers/cxl/core/memdev.c | 51 +++++++++++++++++++++++++++++++++++++++
>>>   include/linux/cxl/cxl.h   |  2 ++
>>>   2 files changed, 53 insertions(+)
>>>
>>> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
>>> index 94b8a7b53c92..4b2641f20128 100644
>>> --- a/drivers/cxl/core/memdev.c
>>> +++ b/drivers/cxl/core/memdev.c
>>> @@ -744,6 +744,57 @@ int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res,
>>>   }
>>>   EXPORT_SYMBOL_NS_GPL(cxl_set_resource, CXL);
>>>   +int cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource type)
>>> +{
>>> +    int rc;
>>> +
>>> +    switch (type) {
>>> +    case CXL_RES_RAM:
>>> +        if (!resource_size(&cxlds->ram_res)) {
>>> +            dev_err(cxlds->dev,
>>> +                "resource request for ram with size 0\n");
>>> +            return -EINVAL;
>>> +        }
>>> +
>>> +        rc = request_resource(&cxlds->dpa_res, &cxlds->ram_res);
>>> +        break;
>>> +    case CXL_RES_PMEM:
>>> +        if (!resource_size(&cxlds->pmem_res)) {
>>> +            dev_err(cxlds->dev,
>>> +                "resource request for pmem with size 0\n");
>>> +            return -EINVAL;
>>> +        }
>>> +        rc = request_resource(&cxlds->dpa_res, &cxlds->pmem_res);
>>> +        break;
>>> +    default:
>>> +        dev_err(cxlds->dev, "unsupported resource type (%u)\n", type);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    return rc;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(cxl_request_resource, CXL);
>> It looks like add_dpa_res() in cxl/core/mbox.c already does what you are doing here, minus the enum.
>> Is there a way that could be reused, or a good reason not too? Even if you don't export the function
>> outside of the cxl tree, you could reuse that function for the internals of this one.
> 
> 
> Although they are obviously similar, I think it makes sense to keep both. The CXL accel API is being implemented for avoiding accel drivers to manipulate cxl structs but through the API calls. With add_dpa_res we would break that, and calling it from the new cxl_request_resource would need changes as inside add_dpa_res the resource is initialized what has already been done in this implementation. IMO, those changes would make the code uglier.
> 

That sounds good to me. I just wanted to make sure there was a good reason to have this set of functions as well!

> 
> Moreover, your comment below about cxl_dpa_release is, I think, wrong, since inside that function other things are being done related to regions. BTW, I can not see other release_resource calls from the current code than those added by this patch.
> 
> 
> So, , I'm not keen to change this now, but maybe a good follow-up work.
> 

From what I've seen, cxl_dpa_release() is only used as part of device cleanup so that's probably why you aren't seeing much usage.

I agree with you with regards to the extra stuff in cxl_dpa_release() with how the patch is right now. I think if DAX region
support ends up being adding the extra region management done in cxl_dpa_release() may be required. My reasoning here is that
at that point we can expect more users than just the driver accessing the CXL resources, so a more managed remove will probably be
necessary.

If I'm wrong about this, then this patch is fine as-is.

> 
>>> +
>>> +int cxl_release_resource(struct cxl_dev_state *cxlds, enum cxl_resource type)
>>> +{
>>> +    int rc;
>>> +
>>> +    switch (type) {
>>> +    case CXL_RES_RAM:
>>> +        rc = release_resource(&cxlds->ram_res);
>>> +        break;
>>> +    case CXL_RES_PMEM:
>>> +        rc = release_resource(&cxlds->pmem_res);
>>> +        break;
>>> +    default:
>>> +        dev_err(cxlds->dev, "unknown resource type (%u)\n", type);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    return rc;
>>> +}
>>> +EXPORT_SYMBOL_NS_GPL(cxl_release_resource, CXL);
>>> +
>> Same thing here, but with cxl_dpa_release() instead of add_dpa_res().
>>
>> Looking at it some more, it looks like there is also some stuff to do with locking for CXL DPA resources in
>> that function that you would be skipping with your functions above. Will that be a problem later? I have no
>> clue, but thought I should ask just in case.
>>
>>>   static int cxl_memdev_release_file(struct inode *inode, struct file *file)
>>>   {
>>>       struct cxl_memdev *cxlmd =
>>> diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h
>>> index 2f48ee591259..6c6d27721067 100644
>>> --- a/include/linux/cxl/cxl.h
>>> +++ b/include/linux/cxl/cxl.h
>>> @@ -54,4 +54,6 @@ bool cxl_pci_check_caps(struct cxl_dev_state *cxlds,
>>>               unsigned long *expected_caps,
>>>               unsigned long *current_caps);
>>>   int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
>>> +int cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
>>> +int cxl_release_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
>>>   #endif

Patch

diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
index 94b8a7b53c92..4b2641f20128 100644
--- a/drivers/cxl/core/memdev.c
+++ b/drivers/cxl/core/memdev.c
@@ -744,6 +744,57 @@  int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res,
 }
 EXPORT_SYMBOL_NS_GPL(cxl_set_resource, CXL);
 
+int cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource type)
+{
+	int rc;
+
+	switch (type) {
+	case CXL_RES_RAM:
+		if (!resource_size(&cxlds->ram_res)) {
+			dev_err(cxlds->dev,
+				"resource request for ram with size 0\n");
+			return -EINVAL;
+		}
+
+		rc = request_resource(&cxlds->dpa_res, &cxlds->ram_res);
+		break;
+	case CXL_RES_PMEM:
+		if (!resource_size(&cxlds->pmem_res)) {
+			dev_err(cxlds->dev,
+				"resource request for pmem with size 0\n");
+			return -EINVAL;
+		}
+		rc = request_resource(&cxlds->dpa_res, &cxlds->pmem_res);
+		break;
+	default:
+		dev_err(cxlds->dev, "unsupported resource type (%u)\n", type);
+		return -EINVAL;
+	}
+
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_request_resource, CXL);
+
+int cxl_release_resource(struct cxl_dev_state *cxlds, enum cxl_resource type)
+{
+	int rc;
+
+	switch (type) {
+	case CXL_RES_RAM:
+		rc = release_resource(&cxlds->ram_res);
+		break;
+	case CXL_RES_PMEM:
+		rc = release_resource(&cxlds->pmem_res);
+		break;
+	default:
+		dev_err(cxlds->dev, "unknown resource type (%u)\n", type);
+		return -EINVAL;
+	}
+
+	return rc;
+}
+EXPORT_SYMBOL_NS_GPL(cxl_release_resource, CXL);
+
 static int cxl_memdev_release_file(struct inode *inode, struct file *file)
 {
 	struct cxl_memdev *cxlmd =
diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h
index 2f48ee591259..6c6d27721067 100644
--- a/include/linux/cxl/cxl.h
+++ b/include/linux/cxl/cxl.h
@@ -54,4 +54,6 @@  bool cxl_pci_check_caps(struct cxl_dev_state *cxlds,
 			unsigned long *expected_caps,
 			unsigned long *current_caps);
 int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds);
+int cxl_request_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
+int cxl_release_resource(struct cxl_dev_state *cxlds, enum cxl_resource type);
 #endif