Message-ID: <7a94adae-6fb2-4505-8988-4b82c1e2dfe6@amd.com>
Date: Fri, 18 Oct 2024 11:40:32 -0500
From: Ben Cheatham <benjamin.cheatham@....com>
To: Alejandro Lucero Palau <alucerop@....com>,
	<alejandro.lucero-palau@....com>
CC: <linux-cxl@...r.kernel.org>, <netdev@...r.kernel.org>,
	<dan.j.williams@...el.com>, <martin.habets@...inx.com>,
	<edward.cree@....com>, <davem@...emloft.net>, <kuba@...nel.org>,
	<pabeni@...hat.com>, <edumazet@...gle.com>
Subject: Re: [PATCH v4 08/26] cxl: add functions for resource request/release
 by a driver



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, alejandro.lucero-palau@....com wrote:
>>> From: Alejandro Lucero <alucerop@....com>
>>>
>>> Create accessors for an accel driver requesting and releasing a resource.
>>>
>>> Signed-off-by: Alejandro Lucero <alucerop@....com>
>>> ---
>>>   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

Powered by blists - more mailing lists