On 23/09/25 04:50PM, Dave Jiang wrote:
+static ssize_t alloc_region_hpa(struct cxl_region *cxlr, u64 size)
+{
+ int rc;
+
+ ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
+ rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem);
+ if (rc)
+ return rc;
Just a nit. Please conform to existing style in the subsystem for this new
usage.
+ ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region);
+ if ((rc = ACQUIRE_ERR(rwsem_write_kill, &rwsem))))
+ return rc;
Actually because of checkpatch.pl error, it is different from existing
style. But recent fix by Alison at [1] will allow to fix it as per others.
Sure, I will fix it in next patch-set
[1]:
https://lore.kernel.org/linux-cxl/[email protected]/
+
+ if (!size)
+ return -EINVAL;
+
+ return alloc_hpa(cxlr, size);
+}
I think you can create another helper free_region_hpa() and call them in
size_store() function to remove the duplicate code.
Sure Dave, I will fix it in next patch-set
+
+static ssize_t alloc_region_dpa(struct cxl_endpoint_decoder *cxled, u64 size)
+{
+ int rc;
+
+ if (!size)
+ return -EINVAL;
+
+ if (!IS_ALIGNED(size, SZ_256M))
+ return -EINVAL;
+
+ rc = cxl_dpa_free(cxled);
+ if (rc)
+ return rc;
+
+ return cxl_dpa_alloc(cxled, size);
+}
+
+static struct cxl_region *
+devm_cxl_pmem_add_region(struct cxl_root_decoder *cxlrd, int id,
+ enum cxl_partition_mode mode,
Wouldn't this not needed since it would be CXL_PARTMODE_PMEM always? I also
wonder if we need to rename devm_cxl_add_region() to devm_cxl_add_ram_region()
to be explicit.
Yes devm_cxl_pmem_add_region() always need CXL_PARTMODE_PMEM, So I will
modify it accordingly. Also I will rename devm_cxl_add_region() to
devm_cxl_add_ram_region().
+ enum cxl_decoder_type type,
+ struct cxl_pmem_region_params *params,
+ struct cxl_decoder *cxld)
+{
+ struct cxl_endpoint_decoder *cxled;
+ struct cxl_region_params *p;
+ struct cxl_port *root_port;
+ struct device *dev;
+ int rc;
+
<snip>
+ rc = alloc_region_dpa(cxled, params->rawsize);
+ if (rc)
+ return ERR_PTR(rc);
+
+ /*
+ * TODO: Currently we have support of interleave_way == 1, where
+ * we can only have one region per mem device. It means mem device
+ * position (params->position) will always be 0. It is therefore
+ * attaching only one target at params->position
+ */
+ if (params->position)
+ return ERR_PTR(-EINVAL);
EOPNOTSUPP?
Yes, EOPNOTSUPP would be more appropriate than EINVAL. I will fix it in
next patch-set
Speaking of which, are there plans to support interleave in the near future?
DJ
My current focus is to get this upstreamed and after that will focus on
multi-interleave support. Multi-interleave support will require some more
efforts on top of this change. So will take it in another series.
Regards,
Neeraj
