Re: [PATCH 06/18] cxl/region: Refactor attach_target() for autodiscovery

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sun, 05 Feb 2023 17:03:02 -0800
Dan Williams <dan.j.williams@intel.com> 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@huawei.com>

> 
> No behavior change is intended.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  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;
> 
>