Re: [PATCH v3 17/20] cxl: allow region creation by type2 drivers

Alejandro Lucero Palau
2024-09-16T16:33:00Z

On 9/13/24 19:08, Jonathan Cameron wrote:
> On Sat, 7 Sep 2024 09:18:33 +0100
> alejandro.lucero-palau@amd.com wrote:
>
>> From: Alejandro Lucero <alucerop@amd.com>
>>
>> Creating a CXL region requires userspace intervention through the cxl
>> sysfs files. Type2 support should allow accelerator drivers to create
>> such cxl region from kernel code.
>>
>> Adding that functionality and integrating it with current support for
>> memory expanders.
>>
>> Based on https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
>>
>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> Sign off doesn't make sense given Dan didn't sent the mail. Co-developed missing?
>
>
> Minor stuff inline,
>
> Jonathan
>
>> +static struct cxl_region *construct_region_begin(struct cxl_root_decoder *cxlrd,
>> + struct cxl_endpoint_decoder *cxled)
>> {
>> struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>> - struct cxl_port *port = cxlrd_to_port(cxlrd);
>> - struct range *hpa = &cxled->cxld.hpa_range;
>> struct cxl_region_params *p;
>> struct cxl_region *cxlr;
>> - struct resource *res;
>> - int rc;
>> + int err = 0;
>>
>> do {
>> cxlr = __create_region(cxlrd, cxled->mode,
>> @@ -3404,8 +3414,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>> } while (IS_ERR(cxlr) && PTR_ERR(cxlr) == -EBUSY);
>>
>> if (IS_ERR(cxlr)) {
>> - dev_err(cxlmd->dev.parent,
>> - "%s:%s: %s failed assign region: %ld\n",
>> + dev_err(cxlmd->dev.parent, "%s:%s: %s failed assign region: %ld\n",
>> dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
>> __func__, PTR_ERR(cxlr));
>> return cxlr;
>> @@ -3415,19 +3424,41 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>> p = &cxlr->params;
>> if (p->state >= CXL_CONFIG_INTERLEAVE_ACTIVE) {
>> dev_err(cxlmd->dev.parent,
>> - "%s:%s: %s autodiscovery interrupted\n",
>> + "%s:%s: %s region setup interrupted\n",
>> dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev),
>> __func__);
>> - rc = -EBUSY;
>> - goto err;
>> + err = -EBUSY;
>> }
>>
>> + if (err) {
>> + construct_region_end();
> Here I'd just release the semaphore not call this warpper as it's taken within this
> function I think?

No, it is taken inside construct_region_begin. It is hard to know just
looking at the patch.

>> + drop_region(cxlr);
>> + return ERR_PTR(err);
>> + }
>> + return cxlr;
>> +}
>> +
>> *res = DEFINE_RES_MEM_NAMED(hpa->start, range_len(hpa),
>> @@ -3444,6 +3475,7 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>> __func__, dev_name(&cxlr->dev));
>> }
>>
>> + p = &cxlr->params;
>> p->res = res;
>> p->interleave_ways = cxled->cxld.interleave_ways;
>> p->interleave_granularity = cxled->cxld.interleave_granularity;
>> @@ -3451,24 +3483,106 @@ static struct cxl_region *construct_region(struct cxl_root_decoder *cxlrd,
>>
>> rc = sysfs_update_group(&cxlr->dev.kobj, get_cxl_region_target_group());
>> if (rc)
>> - goto err;
>> + goto out;
>>
>> dev_dbg(cxlmd->dev.parent, "%s:%s: %s %s res: %pr iw: %d ig: %d\n",
>> - dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev), __func__,
>> - dev_name(&cxlr->dev), p->res, p->interleave_ways,
>> - p->interleave_granularity);
>> + dev_name(&cxlmd->dev),
>> + dev_name(&cxled->cxld.dev), __func__,
> Avoid reformatting unless necessary. Hard to tell what changed
> if anything.

I'll fix it.

>> + dev_name(&cxlr->dev), p->res,
>> + p->interleave_ways,
>> + p->interleave_granularity);
>>
>> /* ...to match put_device() in cxl_add_to_region() */
>> get_device(&cxlr->dev);
>> up_write(&cxl_region_rwsem);
>>
>> +out:
>> + construct_region_end();
> As below. I'd have separate error handling path.
> Same in the other paths.
>

OK

>> + if (rc) {
>> + drop_region(cxlr);
>> + return ERR_PTR(rc);
>> + }
>> return cxlr;
>> +}
>>
>> -err:
>> - up_write(&cxl_region_rwsem);
>> - devm_release_action(port->uport_dev, unregister_region, cxlr);
>> - return ERR_PTR(rc);
>> +static struct cxl_region *
>> +__construct_new_region(struct cxl_root_decoder *cxlrd,
>> + struct cxl_endpoint_decoder *cxled)
>> +{
>> + struct cxl_decoder *cxld = &cxlrd->cxlsd.cxld;
>> + struct cxl_region_params *p;
>> + resource_size_t size = 0;
> set in all paths that use it.
>
>> + struct cxl_region *cxlr;
>> + int rc;
>> +
>> + cxlr = construct_region_begin(cxlrd, cxled);
>> + if (IS_ERR(cxlr))
>> + return cxlr;
>> +
>> + rc = set_interleave_ways(cxlr, 1);
>> + if (rc)
>> + goto out;
>> +
>> + rc = set_interleave_granularity(cxlr, cxld->interleave_granularity);
>> + if (rc)
>> + goto out;
>> +
>> + size = resource_size(cxled->dpa_res);
>> +
> only used once
> rc = alloc_hpa(cxlr, resource_size(cxled->dpa_res));
>

Clever.

I'll do it.

>> + rc = alloc_hpa(cxlr, size);
>> + if (rc)
>> + goto out;
>> +
>> + down_read(&cxl_dpa_rwsem);
>> + rc = cxl_region_attach(cxlr, cxled, 0);
>> + up_read(&cxl_dpa_rwsem);
>> +
>> + if (rc)
>> + goto out;
>> +
>> + rc = cxl_region_decode_commit(cxlr);
>> + if (rc)
>> + goto out;
>> +
>> + p = &cxlr->params;
>> + p->state = CXL_CONFIG_COMMIT;
>> +out:
> I'd break out a separate error handling path and
> just duplicate the next line.

I'll do it.

Thanks!

>
>> + construct_region_end();
>> + if (rc) {
>> + drop_region(cxlr);
>> + return ERR_PTR(rc);
>> + }
>> + return cxlr;
>> +}