On Sun, 05 Feb 2023 17:03:02 -0800
Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> Region autodiscovery is the process of kernel creating 'struct
> cxl_region' object to represent active CXL memory ranges it finds
> already active in hardware when the driver loads. Typically this happens
> when platform firmware establishes CXL memory regions and then publishes
> them in the memory map. However, this can also happen in the case of
> kexec-reboot after the kernel has created regions.
>
> In the autodiscovery case the region creation process starts with a
> known endpoint decoder. Refactor attach_target() into a helper that is
> suitable to be called from either sysfs, for runtime region creation, or
> from cxl_port_probe() after it has enumerated all endpoint decoders.
>
> The cxl_port_probe() context is an async device-core probing context, so
> it is not appropriate to allow SIGTERM to interrupt the assembly
> process. Refactor attach_target() to take @cxled and @state as arguments
> where @state indicates whether waiting from the region rwsem is
> interruptible or not.
As below, I'd have broken this change out as a follow up patch - that
way reviewer could check for strict noop refactor, then look in ioslation
at that case.
I don't care that much though as second patch would only have about 4 lines
of diff.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
> No behavior change is intended.
>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
> drivers/cxl/core/region.c | 47 +++++++++++++++++++++++++++------------------
> 1 file changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 8dea49c021b8..97eafdd75675 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -1418,31 +1418,25 @@ void cxl_decoder_kill_region(struct cxl_endpoint_decoder *cxled)
> up_write(&cxl_region_rwsem);
> }
>
> -static int attach_target(struct cxl_region *cxlr, const char *decoder, int pos)
> +static int attach_target(struct cxl_region *cxlr,
> + struct cxl_endpoint_decoder *cxled, int pos,
> + unsigned int state)
> {
> - struct device *dev;
> - int rc;
> -
> - dev = bus_find_device_by_name(&cxl_bus_type, NULL, decoder);
> - if (!dev)
> - return -ENODEV;
> -
> - if (!is_endpoint_decoder(dev)) {
> - put_device(dev);
> - return -EINVAL;
> - }
> + int rc = 0;
>
> - rc = down_write_killable(&cxl_region_rwsem);
> + if (state == TASK_INTERRUPTIBLE)
> + rc = down_write_killable(&cxl_region_rwsem);
> + else
> + down_write(&cxl_region_rwsem);
I'd be tempted to do this in two hops for patch readability. First
make the code reorg then follow up with this bit before the use
of it in the next patch.
> if (rc)
> - goto out;
> + return rc;
> +
> down_read(&cxl_dpa_rwsem);
> - rc = cxl_region_attach(cxlr, to_cxl_endpoint_decoder(dev), pos);
> + rc = cxl_region_attach(cxlr, cxled, pos);
> if (rc == 0)
> set_bit(CXL_REGION_F_INCOHERENT, &cxlr->flags);
> up_read(&cxl_dpa_rwsem);
> up_write(&cxl_region_rwsem);
> -out:
> - put_device(dev);
> return rc;
> }
>
> @@ -1480,8 +1474,23 @@ static size_t store_targetN(struct cxl_region *cxlr, const char *buf, int pos,
>
> if (sysfs_streq(buf, "\n"))
> rc = detach_target(cxlr, pos);
> - else
> - rc = attach_target(cxlr, buf, pos);
> + else {
> + struct device *dev;
> +
> + dev = bus_find_device_by_name(&cxl_bus_type, NULL, buf);
> + if (!dev)
> + return -ENODEV;
> +
> + if (!is_endpoint_decoder(dev)) {
> + rc = -EINVAL;
> + goto out;
> + }
> +
> + rc = attach_target(cxlr, to_cxl_endpoint_decoder(dev), pos,
> + TASK_INTERRUPTIBLE);
> +out:
> + put_device(dev);
> + }
>
> if (rc < 0)
> return rc;
>
>
