Re: [PATCH 2/3] cxl/region: Flag partially torn down regions as unusable

[Dave Jiang <dave.jiang@intel.com>]

On 6/16/23 18:24, Dan Williams wrote:
> cxl_region_decode_reset() walks all the decoders associated with a given
> region and disables them. Due to decoder ordering rules it is possible
> that a switch in the topology notices that a given decoder can not be
> shutdown before another region with a higher HPA is shutdown first. That
> can leave the region in a partially committed state.
> 
> Capture that state in a new CXL_REGION_F_NEEDS_RESET flag and require
> that a successful cxl_region_decode_reset() attempt must be completed
> before cxl_region_probe() accepts the region.
> 
> This is a corollary for the bug that Jonathan identified in "CXL/region
> :  commit reset of out of order region appears to succeed." [1].
> 
> Cc: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Link: http://lore.kernel.org/r/20230316171441.0000205b@Huawei.com [1]
> Fixes: 176baefb2eb5 ("cxl/hdm: Commit decoder state to hardware")
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>   drivers/cxl/core/region.c |   12 ++++++++++++
>   drivers/cxl/cxl.h         |    8 ++++++++
>   2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index d48c5916f2e9..31f498f0fb3a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -182,14 +182,19 @@ static int cxl_region_decode_reset(struct cxl_region *cxlr, int count)
>   				rc = cxld->reset(cxld);
>   			if (rc)
>   				return rc;
> +			set_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags);
>   		}
>   
>   endpoint_reset:
>   		rc = cxled->cxld.reset(&cxled->cxld);
>   		if (rc)
>   			return rc;
> +		set_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags);
>   	}
>   
> +	/* all decoders associated with this region have been torn down */
> +	clear_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags);
> +
>   	return 0;
>   }
>   
> @@ -2876,6 +2881,13 @@ static int cxl_region_probe(struct device *dev)
>   		goto out;
>   	}
>   
> +	if (test_bit(CXL_REGION_F_NEEDS_RESET, &cxlr->flags)) {
> +		dev_err(&cxlr->dev,
> +			"failed to activate, re-commit region and retry\n");
> +		rc = -ENXIO;
> +		goto out;
> +	}
> +
>   	/*
>   	 * From this point on any path that changes the region's state away from
>   	 * CXL_CONFIG_COMMIT is also responsible for releasing the driver.
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index 187abd8ba673..cd7bf6697a51 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -469,6 +469,14 @@ struct cxl_region_params {
>    */
>   #define CXL_REGION_F_AUTO 0
>   
> +/*
> + * Require that a committed region successfully complete a teardown once
> + * any of its associated decoders have been torn down. This maintains
> + * the commit state for the region since there are committed decoders,
> + * but blocks cxl_region_probe().
> + */
> +#define CXL_REGION_F_NEEDS_RESET 1
> +
>   /**
>    * struct cxl_region - CXL region
>    * @dev: This region's device
>