| 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
| ||
|
Message-ID: <700072760.81750944183483.JavaMail.epsvc@epcpadp2new>
Date: Thu, 26 Jun 2025 15:53:41 +0530
From: Neeraj Kumar <s.neeraj@...sung.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: dan.j.williams@...el.com, dave@...olabs.net, dave.jiang@...el.com,
alison.schofield@...el.com, vishal.l.verma@...el.com, ira.weiny@...el.com,
a.manzanares@...sung.com, nifan.cxl@...il.com, anisa.su@...sung.com,
vishak.g@...sung.com, krish.reddy@...sung.com, arun.george@...sung.com,
alok.rathore@...sung.com, neeraj.kernel@...il.com,
linux-kernel@...r.kernel.org, linux-cxl@...r.kernel.org,
nvdimm@...ts.linux.dev, gost.dev@...sung.com, cpgs@...sung.com
Subject: Re: [RFC PATCH 14/20] cxl/region: Add cxl pmem region creation
routine for region persistency
On 23/06/25 10:43AM, Jonathan Cameron wrote:
>On Tue, 17 Jun 2025 18:09:38 +0530
>Neeraj Kumar <s.neeraj@...sung.com> wrote:
>
>> Added exported cxl_create_pmem_region routine to create cxl pmem region
>
>For function names always add () after and drop 'function/routine' etc.
>Ends up shorter and easier to read.
>
Sure, Will fix it accordingly
>> from LSA parsed cxl region information.
>> Inspirition for the function is taken from ndctl device attribute
>> (_store) call. It allocates cxlr and fills information parsed from LSA
>> and calls device_add(&cxlr->dev) to initiates further region creation
>> porbes
>Spell check.
>
Thanks, Will fix it up.
>>
>> Signed-off-by: Neeraj Kumar <s.neeraj@...sung.com>
>> ---
>> drivers/cxl/core/port.c | 6 ++
>> drivers/cxl/core/region.c | 208 ++++++++++++++++++++++++++++++++++++++
>> drivers/cxl/cxl.h | 11 ++
>> 3 files changed, 225 insertions(+)
>>
>
>> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
>> index b98b1ccffd1c..8990e3c3474d 100644
>> --- a/drivers/cxl/core/region.c
>> +++ b/drivers/cxl/core/region.c
>> @@ -2522,6 +2522,214 @@ static ssize_t create_ram_region_show(struct device *dev,
>> return __create_region_show(to_cxl_root_decoder(dev), buf);
>> }
>>
>> +static ssize_t update_region_size(struct cxl_region *cxlr, u64 val)
>> +{
>> + int rc;
>> +
>> + rc = down_write_killable(&cxl_region_rwsem);
>ACQUIRE() as mentioned below.
>
Sure, Let me look at Dan's new ACQUIRE() series. I will update it in V1
>> + if (rc)
>> + return rc;
>> +
>> + if (val)
>> + rc = alloc_hpa(cxlr, val);
>> + else
>> + rc = free_hpa(cxlr);
>> + up_write(&cxl_region_rwsem);
>> +
>> + if (rc)
>> + return rc;
>
> return rc;
>
Thanks, I will update it.
>> +
>> + return 0;
>> +}
>> +
>> +static ssize_t update_region_dpa_size(struct cxl_region *cxlr,
>> + struct cxl_decoder *cxld,
>> + unsigned long long size)
>> +{
>> + int rc;
>> + struct cxl_endpoint_decoder *cxled =
>> + to_cxl_endpoint_decoder(&cxld->dev);
>> +
>> + if (!IS_ALIGNED(size, SZ_256M))
>> + return -EINVAL;
>> +
>> + rc = cxl_dpa_free(cxled);
>> + if (rc)
>> + return rc;
>> +
>> + if (size == 0)
>> + return 0;
>> +
>> + rc = cxl_dpa_alloc(cxled, size);
>return cxl_dpa_alloc()
>
>Unless something more is getting added here in later patches in which case
>you can ignore this comment.
>
Thanks, Will fix it accordingly
>> + if (rc)
>> + return rc;
>> +
>> + return 0;
>> +}
>> +
>> +static ssize_t update_region_dpa_mode(struct cxl_region *cxlr,
>> + struct cxl_decoder *cxld)
>> +{
>> + int rc;
>> + struct cxl_endpoint_decoder *cxled =
>> + to_cxl_endpoint_decoder(&cxld->dev);
>Maybe don't bother with local variable and just put this
>below as the parameter.
Sure, I will fix it.
>> +
>> + rc = cxl_dpa_set_mode(cxled, CXL_DECODER_PMEM);
>
>return cxl_dpa_set_mode()
>
>> + if (rc)
>> + return rc;
>> +
>> + return 0;
>> +}
>> +
>> +static size_t attach_region_target(struct cxl_region *cxlr,
>> + struct cxl_decoder *cxld, int pos)
>> +{
>> + int rc;
>> + struct cxl_endpoint_decoder *cxled =
>> + to_cxl_endpoint_decoder(&cxld->dev);
>> +
>> + rc = attach_target(cxlr, cxled, pos, TASK_INTERRUPTIBLE);
>> +
>No blank line here
Sure, I will fix it.
>> + if (rc < 0)
>Can it ever be > 0 ?
>If not, return attach_target() should be fine.
Yes we can directly return from here, will fix it in V1
>> + return rc;
>> +
>> + return 0;
>> +}
>> +
>> +static ssize_t commit_region(struct cxl_region *cxlr)
>> +{
>> + struct cxl_region_params *p = &cxlr->params;
>> + ssize_t rc;
>> +
>> + rc = down_write_killable(&cxl_region_rwsem);
>
>Maybe look at Dan's new ACQUIRE() series. The last patch in there
>is targeting code similar to this.
Sure, Let me look at Dan's new ACQUIRE() series. I will update it in V1
>
>> + if (rc)
>> + return rc;
>> +
>> + /* Already in the requested state? */
>> + if (p->state >= CXL_CONFIG_COMMIT)
>> + goto out;
>> +
>> + /* Not ready to commit? */
>> + if (p->state < CXL_CONFIG_ACTIVE) {
>> + rc = -ENXIO;
>> + goto out;
>> + }
>> +
>> + /*
>> + * Invalidate caches before region setup to drop any speculative
>> + * consumption of this address space
>> + */
>> + rc = cxl_region_invalidate_memregion(cxlr);
>> + if (rc)
>> + goto out;
>> +
>> + rc = cxl_region_decode_commit(cxlr);
>> + if (rc == 0)
>With AQUIRE() stuff you can just do
> if (rc)
> return rc;
>
>here
>
Thanks, Will update it accordingly.
>> + p->state = CXL_CONFIG_COMMIT;
>> +out:
>> + up_write(&cxl_region_rwsem);
>> + if (rc)
>> + return rc;
>> + return 0;
>> +}
>> +
>> +static struct cxl_region *
>> +devm_cxl_pmem_add_region(struct cxl_root_decoder *cxlrd,
>> + struct cxl_decoder *cxld,
>> + struct cxl_pmem_region_params *params, int id,
>> + enum cxl_decoder_mode mode, enum cxl_decoder_type type)
>> +{
>> + struct cxl_port *port;
>> + struct cxl_region *cxlr;
>> + struct cxl_region_params *p;
>> + struct device *dev;
>> + int rc;
>> +
>> + if (!cxlrd)
>> + return ERR_PTR(-EINVAL);
>
>For a check like this, add a comment on why it might be NULL.
>If it can't be, then drop the check.
>
Sure, I will fix it in V1
>> +
>> + port = to_cxl_port(cxlrd->cxlsd.cxld.dev.parent);
>
>Maybe add a comment on which port this actually is. These long indirections
>can make that hard to figure out!
>
Sure, I will fix it in V1
>> +
>> + cxlr = cxl_region_alloc(cxlrd, id);
>> + if (IS_ERR(cxlr))
>> + return cxlr;
>> + cxlr->mode = mode;
>> + cxlr->type = type;
>> +
>> + dev = &cxlr->dev;
>> + rc = dev_set_name(dev, "region%d", id);
>> + if (rc)
>> + goto err;
>> +
>> + p = &cxlr->params;
>> + p->uuid = params->uuid;
>> + p->interleave_ways = params->nlabel;
>> + p->interleave_granularity = params->ig;
>> +
>> + /* Update region size */
>> + if (update_region_size(cxlr, params->rawsize))
>> + goto err;
>> +
>> + /* Flush cxl wq */
>
>Much more useful to say 'why' you are flushing it. It's obvious
>from the code that you are.
>
Sure Will update the comment properly
>> + cxl_wq_flush();
>> +
>> + /* Clear DPA Size */
>
>I'd look at all these comments and where they are obvious, drop the comment
>and let the code speak for itself.
>
Sure, Will update accordingly
>> + if (update_region_dpa_size(cxlr, cxld, 0))
>> + goto err;
>> +
>> + /* Update DPA mode */
>> + if (update_region_dpa_mode(cxlr, cxld))
>> + goto err;
>> +
>> + /* Update DPA Size */
>> + if (update_region_dpa_size(cxlr, cxld, params->rawsize))
>> + goto err;
>> +
>> + /* Attach region targets */
>It's attaching just one by the look of it. I'd just drop the comment.
>
Sure, Will update accordingly
>> + if (attach_region_target(cxlr, cxld, params->position))
>> + goto err;
>> +
>> + /* Commit Region */
>> + if (commit_region(cxlr))
>> + goto err;
>> +
>> + rc = device_add(dev);
>> + if (rc)
>> + goto err;
>> +
>> + rc = devm_add_action_or_reset(port->uport_dev, unregister_region, cxlr);
>> + if (rc)
>> + return ERR_PTR(rc);
>> +
>> + dev_dbg(port->uport_dev, "%s: created %s\n",
>> + dev_name(&cxlrd->cxlsd.cxld.dev), dev_name(dev));
>> + return cxlr;
>> +
>> +err:
>> + put_device(dev);
>> + return ERR_PTR(rc);
>> +}
>> +
>> +struct cxl_region *cxl_create_pmem_region(struct cxl_root_decoder *cxlrd,
>> + struct cxl_decoder *cxld,
>> + struct cxl_pmem_region_params *params, int id)
>> +{
>> + int rc;
>> +
>> + rc = memregion_alloc(GFP_KERNEL);
>> + if (rc < 0)
>> + return ERR_PTR(rc);
>> +
>> + if (atomic_cmpxchg(&cxlrd->region_id, id, rc) != id) {
>> + memregion_free(rc);
>> + return ERR_PTR(-EBUSY);
>> + }
>I'm a little surprised not to see cleanup of the memregion via
>devm somewhere here.
Actually its cleanup is happening at cxl_region_release().
During cxl_region_alloc, cxlr is allocated whose dev->type is
cxl_region_type
const struct device_type cxl_region_type = {
.name = "cxl_region",
.release = cxl_region_release,
.groups = region_groups
};
>> +
>> + return devm_cxl_pmem_add_region(cxlrd, cxld, params, id,
>> + CXL_DECODER_PMEM, CXL_DECODER_HOSTONLYMEM);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(cxl_create_pmem_region, "CXL");
>> +
>> static struct cxl_region *__create_region(struct cxl_root_decoder *cxlrd,
>> enum cxl_decoder_mode mode, int id)
>> {
>
>
Powered by blists - more mailing lists