[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <158453976.61759726503893.JavaMail.epsvc@epcpadp1new>
Date: Mon, 29 Sep 2025 19:07:38 +0530
From: Neeraj Kumar <s.neeraj@...sung.com>
To: Dave Jiang <dave.jiang@...el.com>
Cc: linux-cxl@...r.kernel.org, nvdimm@...ts.linux.dev,
linux-kernel@...r.kernel.org, gost.dev@...sung.com,
a.manzanares@...sung.com, vishak.g@...sung.com, neeraj.kernel@...il.com,
cpgs@...sung.com
Subject: Re: [PATCH V3 14/20] cxl/region: Add devm_cxl_pmem_add_region() for
pmem region creation
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/20250815010645.2980846-1-alison.schofield@intel.com/
>> +
>> + 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
Powered by blists - more mailing lists