Re: [PATCH] cxl/region: Fix region reference target accounting

[Ira Weiny <ira.weiny@intel.com>]

On Mon, Aug 01, 2022 at 04:00:21PM -0700, Dan Williams wrote:
> Dan reports:
> 
>     The error handling in cxl_port_attach_region() looks like it might
>     have a similar bug.  The cxl_rr->nr_targets++; might want a --.
> 
>     That function is more complicated.
> 
> Indeed cxl_rr->nr_targets leaks when cxl_rr_ep_add() fails, but that
> flow is not clear. Fix the bug and the clarity by separating the 'new'
                                         ^^^^^^^
                                         clarify?

> region-reference case from the 'extend' region-reference case. This also
> moves the host-physical-address (HPA) validation, that the HPA of a new
> region being accounted to the port is greater than the HPA of all other
> regions associated with the port, to alloc_region_ref().

Technically this means the kdoc 'steps list' is out of order a bit.  :-(

But I like that this is only done on creation.  And I'm not sure it matter much
because 'steps' is more of a list of things this function does.

> 
> Introduce @nr_targets_inc to track when the error exit path needs to
> clean up cxl_rr->nr_targets.
> 
> Fixes: 384e624bb211 ("cxl/region: Attach endpoint decoders")
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/cxl/core/region.c |   71 +++++++++++++++++++++++++++------------------
>  1 file changed, 43 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index aff4f736b63c..cf5d5811fe4c 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -709,12 +709,26 @@ static struct cxl_decoder *cxl_region_find_decoder(struct cxl_port *port,
>  static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
>  					       struct cxl_region *cxlr)
>  {
> -	struct cxl_region_ref *cxl_rr;
> +	struct cxl_region_params *p = &cxlr->params;
> +	struct cxl_region_ref *cxl_rr, *iter;
> +	unsigned long index;
>  	int rc;
>  
> +	xa_for_each(&port->regions, index, iter) {
> +		struct cxl_region_params *ip = &iter->region->params;
> +
> +		if (ip->res->start > p->res->start) {
> +			dev_dbg(&cxlr->dev,
> +				"%s: HPA order violation %s:%pr vs %pr\n",
> +				dev_name(&port->dev),
> +				dev_name(&iter->region->dev), ip->res, p->res);
> +			return ERR_PTR(-EBUSY);
> +		}
> +	}
> +
>  	cxl_rr = kzalloc(sizeof(*cxl_rr), GFP_KERNEL);
>  	if (!cxl_rr)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  	cxl_rr->port = port;
>  	cxl_rr->region = cxlr;
>  	cxl_rr->nr_targets = 1;
> @@ -726,7 +740,7 @@ static struct cxl_region_ref *alloc_region_ref(struct cxl_port *port,
>  			"%s: failed to track region reference: %d\n",
>  			dev_name(&port->dev), rc);
>  		kfree(cxl_rr);
> -		return NULL;
> +		return ERR_PTR(rc);
>  	}
>  
>  	return cxl_rr;
> @@ -801,33 +815,25 @@ static int cxl_port_attach_region(struct cxl_port *port,
>  {
>  	struct cxl_memdev *cxlmd = cxled_to_memdev(cxled);
>  	struct cxl_ep *ep = cxl_ep_load(port, cxlmd);
> -	struct cxl_region_ref *cxl_rr = NULL, *iter;
> -	struct cxl_region_params *p = &cxlr->params;
> -	struct cxl_decoder *cxld = NULL;
> +	struct cxl_region_ref *cxl_rr;
> +	bool nr_targets_inc = false;
> +	struct cxl_decoder *cxld;
>  	unsigned long index;
>  	int rc = -EBUSY;
>  
>  	lockdep_assert_held_write(&cxl_region_rwsem);
>  
> -	xa_for_each(&port->regions, index, iter) {
> -		struct cxl_region_params *ip = &iter->region->params;
> -
> -		if (iter->region == cxlr)
> -			cxl_rr = iter;
> -		if (ip->res->start > p->res->start) {
> -			dev_dbg(&cxlr->dev,
> -				"%s: HPA order violation %s:%pr vs %pr\n",
> -				dev_name(&port->dev),
> -				dev_name(&iter->region->dev), ip->res, p->res);
> -			return -EBUSY;
> -		}
> -	}
> -
> +	cxl_rr = cxl_rr_load(port, cxlr);
>  	if (cxl_rr) {
>  		struct cxl_ep *ep_iter;
>  		int found = 0;
>  
> -		cxld = cxl_rr->decoder;
> +		/*
> +		 * Walk the existing endpoints that have been attached to
> +		 * @cxlr at @port and see if they share the same 'next' port
> +		 * in the downstream direction. I.e. endpoints that share common
> +		 * upstream switch.
> +		 */
>  		xa_for_each(&cxl_rr->endpoints, index, ep_iter) {
>  			if (ep_iter == ep)
>  				continue;
> @@ -838,22 +844,29 @@ static int cxl_port_attach_region(struct cxl_port *port,
>  		}
>  
>  		/*
> -		 * If this is a new target or if this port is direct connected
> -		 * to this endpoint then add to the target count.
> +		 * New target port, or @port is an endpoint port that always
> +		 * accounts its own local decode as a target.
>  		 */
> -		if (!found || !ep->next)
> +		if (!found || !ep->next) {
>  			cxl_rr->nr_targets++;
> +			nr_targets_inc = true;
> +		}
> +
> +		/*
> +		 * The decoder for @cxlr was allocated when the region was first
> +		 * attached to @port.
> +		 */
> +		cxld = cxl_rr->decoder;
>  	} else {
>  		cxl_rr = alloc_region_ref(port, cxlr);
> -		if (!cxl_rr) {
> +		if (IS_ERR(cxl_rr)) {
>  			dev_dbg(&cxlr->dev,
>  				"%s: failed to allocate region reference\n",
>  				dev_name(&port->dev));
> -			return -ENOMEM;
> +			return PTR_ERR(cxl_rr);
>  		}
> -	}
> +		nr_targets_inc = true;

I don't think this is technically needed but I would leave it if I were you.

I think the leak only occurs on the extend case but I think it is safe to do
this and does make the code cleaner.

Regardless of the kdoc subtlety and with the spelling fix.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

>  
> -	if (!cxld) {
>  		if (port == cxled_to_port(cxled))
>  			cxld = &cxled->cxld;
>  		else
> @@ -896,6 +909,8 @@ static int cxl_port_attach_region(struct cxl_port *port,
>  
>  	return 0;
>  out_erase:
> +	if (nr_targets_inc)
> +		cxl_rr->nr_targets--;
>  	if (cxl_rr->nr_eps == 0)
>  		free_region_ref(cxl_rr);
>  	return rc;
>