Re: [PATCH v4 1/2] cxl/region: Find free cxl decoder by device_for_each_child()

[Zijun Hu <zijun_hu@icloud.com>]

On 2024/9/11 02:27, Dan Williams wrote:
> Dan Williams wrote:
> [..]
>> So, while regionB would be the next decoder to allocate after regionC is
>> torn down, it is not a free decoder while decoderC and regionC have not been
>> reclaimed.
> 
> The "simple" conversion is bug compatible with the current
> implementation, but here's a path to both constify the
> device_find_child() argument, *and* prevent unwanted allocations by
> allocating decoders precisely by id.  Something like this, it passes a
> quick unit test run:
> 

sounds good.

> diff --git a/drivers/cxl/core/port.c b/drivers/cxl/core/port.c
> index 1d5007e3795a..749a281819b4 100644
> --- a/drivers/cxl/core/port.c
> +++ b/drivers/cxl/core/port.c
> @@ -1750,7 +1750,8 @@ static int cxl_decoder_init(struct cxl_port *port, struct cxl_decoder *cxld)
>  	struct device *dev;
>  	int rc;
>  
> -	rc = ida_alloc(&port->decoder_ida, GFP_KERNEL);
> +	rc = ida_alloc_max(&port->decoder_ida, CXL_DECODER_NR_MAX - 1,
> +			   GFP_KERNEL);
>  	if (rc < 0)
>  		return rc;
>  
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 21ad5f242875..1f7b3a9ebfa3 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -794,26 +794,16 @@ static size_t show_targetN(struct cxl_region *cxlr, char *buf, int pos)
>  	return rc;
>  }
>  
> -static int match_free_decoder(struct device *dev, void *data)
> +static int match_decoder_id(struct device *dev, void *data)
>  {
>  	struct cxl_decoder *cxld;
> -	int *id = data;
> +	int id = *(int *) data;
>  
>  	if (!is_switch_decoder(dev))
>  		return 0;
>  
>  	cxld = to_cxl_decoder(dev);
> -
> -	/* enforce ordered allocation */
> -	if (cxld->id != *id)
> -		return 0;
> -
> -	if (!cxld->region)
> -		return 1;
> -
> -	(*id)++;
> -
> -	return 0;
> +	return cxld->id == id;
>  }
>  
>  static int match_auto_decoder(struct device *dev, void *data)
> @@ -840,16 +830,29 @@ cxl_region_find_decoder(struct cxl_port *port,
>  			struct cxl_region *cxlr)
>  {
>  	struct device *dev;
> -	int id = 0;
> -
>  	if (port == cxled_to_port(cxled))
>  		return &cxled->cxld;
>  
>  	if (test_bit(CXL_REGION_F_AUTO, &cxlr->flags))
>  		dev = device_find_child(&port->dev, &cxlr->params,
>  					match_auto_decoder);
> -	else
> -		dev = device_find_child(&port->dev, &id, match_free_decoder);
> +	else {
> +		int id, last;
> +
> +		/*
> +		 * Find next available decoder, but fail new decoder
> +		 * allocations if out-of-order region destruction has
> +		 * occurred
> +		 */
> +		id = find_first_zero_bit(port->decoder_alloc,
> +					 CXL_DECODER_NR_MAX);
> +		last = find_last_bit(port->decoder_alloc, CXL_DECODER_NR_MAX);
> +
> +		if (id >= CXL_DECODER_NR_MAX ||
> +		    (last < CXL_DECODER_NR_MAX && id != last + 1))
> +			return NULL;

Above finding logic seems wrong.
what about below one ?

 last = find_last_bit(port->decoder_alloc, CXL_DECODER_NR_MAX);

 if (last >= CXL_DECODER_NR_MAX)
    id = 0;
 else if (last + 1 < CXL_DECODER_NR_MAX)
    id = last + 1;
 else
    return NULL;

> +		dev = device_find_child(&port->dev, &id, match_decoder_id);
> +	}
>  	if (!dev)
>  		return NULL;
>  	/*
> @@ -943,6 +946,9 @@ static void cxl_rr_free_decoder(struct cxl_region_ref *cxl_rr)
>  
>  	dev_WARN_ONCE(&cxlr->dev, cxld->region != cxlr, "region mismatch\n");
>  	if (cxld->region == cxlr) {
> +		struct cxl_port *port = to_cxl_port(cxld->dev.parent);
> +
> +		clear_bit(cxld->id, port->decoder_alloc);
>  		cxld->region = NULL;
>  		put_device(&cxlr->dev);
>  	}
> @@ -977,6 +983,7 @@ static int cxl_rr_ep_add(struct cxl_region_ref *cxl_rr,
>  	cxl_rr->nr_eps++;
>  
>  	if (!cxld->region) {
> +		set_bit(cxld->id, port->decoder_alloc);
>  		cxld->region = cxlr;
>  		get_device(&cxlr->dev);
>  	}
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 9afb407d438f..750cd027d0b0 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -578,6 +578,9 @@ struct cxl_dax_region {
>  	struct range hpa_range;
>  };
>  
> +/* Max as of CXL 3.1 (8.2.4.20.1 CXL HDM Decoder Capability Register) */
> +#define CXL_DECODER_NR_MAX 32
> +
>  /**
>   * struct cxl_port - logical collection of upstream port devices and
>   *		     downstream port devices to construct a CXL memory
> @@ -591,6 +594,7 @@ struct cxl_dax_region {
>   * @regions: cxl_region_ref instances, regions mapped by this port
>   * @parent_dport: dport that points to this port in the parent
>   * @decoder_ida: allocator for decoder ids
> + * @decoder_alloc: decoder busy/free (@cxld->region set) bitmap
>   * @reg_map: component and ras register mapping parameters
>   * @nr_dports: number of entries in @dports
>   * @hdm_end: track last allocated HDM decoder instance for allocation ordering
> @@ -611,6 +615,7 @@ struct cxl_port {
>  	struct xarray regions;
>  	struct cxl_dport *parent_dport;
>  	struct ida decoder_ida;
> +	DECLARE_BITMAP(decoder_alloc, CXL_DECODER_NR_MAX);
>  	struct cxl_register_map reg_map;
>  	int nr_dports;
>  	int hdm_end;