diff mbox series

[2/2] cxl/core/port: Handle invalid decoders

Message ID 164317464918.3438644.12371149695618136198.stgit@dwillia2-desk3.amr.corp.intel.com (mailing list archive)
State Not Applicable
Headers show
Series cxl/port: Robustness fixes for decoder enumeration | expand

Commit Message

Dan Williams Jan. 26, 2022, 5:24 a.m. UTC 
In case init_hdm_decoder() finds invalid settings, skip to the next
valid decoder. Only fail port enumeration if zero valid decoders are
found. This protects the driver init against broken hardware and / or
future interleave capabilities.

Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/cxl/core/hdm.c |   36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

Comments

Jonathan Cameron Feb. 1, 2022, 12:59 p.m. UTC | #1 
On Tue, 25 Jan 2022 21:24:09 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> In case init_hdm_decoder() finds invalid settings, skip to the next
> valid decoder. Only fail port enumeration if zero valid decoders are
> found. This protects the driver init against broken hardware and / or
> future interleave capabilities.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
One comment inline, but I'm fine with this as it is.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/cxl/core/hdm.c |   36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index c966ab0d51fe..4955ba16c9c8 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -150,8 +150,8 @@ static int to_interleave_ways(u32 ctrl)
>  	}
>  }
>  
> -static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map,
> -			     void __iomem *hdm, int which)
> +static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
> +			    int *target_map, void __iomem *hdm, int which)
>  {
>  	u64 size, base;
>  	u32 ctrl;
> @@ -167,6 +167,11 @@ static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map,
>  
>  	if (!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED))
>  		size = 0;
> +	if (base == U64_MAX || size == U64_MAX) {
> +		dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n",
> +			 port->id, cxld->id);
> +		return -ENXIO;
> +	}
>  
>  	cxld->decoder_range = (struct range) {
>  		.start = base,
> @@ -180,6 +185,12 @@ static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map,
>  			cxld->flags |= CXL_DECODER_F_LOCK;
>  	}
>  	cxld->interleave_ways = to_interleave_ways(ctrl);
> +	if (!cxld->interleave_ways) {
> +		dev_warn(&port->dev,
> +			 "decoder%d.%d: Invalid interleave ways (ctrl: %#x)\n",
> +			 port->id, cxld->id, ctrl);
> +		return -ENXIO;
> +	}
>  	cxld->interleave_granularity = to_interleave_granularity(ctrl);
>  
>  	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl))
> @@ -188,12 +199,14 @@ static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map,
>  		cxld->target_type = CXL_DECODER_ACCELERATOR;
>  
>  	if (is_cxl_endpoint(to_cxl_port(cxld->dev.parent)))
> -		return;
> +		return 0;
>  
>  	target_list.value =
>  		ioread64_hi_lo(hdm + CXL_HDM_DECODER0_TL_LOW(which));
>  	for (i = 0; i < cxld->interleave_ways; i++)
>  		target_map[i] = target_list.target_id[i];
> +
> +	return 0;
>  }
>  
>  /**
> @@ -204,7 +217,7 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
>  {
>  	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
>  	struct cxl_port *port = cxlhdm->port;
> -	int i, committed;
> +	int i, committed, failed;
>  	u32 ctrl;
>  
>  	/*
> @@ -224,7 +237,7 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
>  	if (committed != cxlhdm->decoder_count)
>  		msleep(20);
>  
> -	for (i = 0; i < cxlhdm->decoder_count; i++) {
> +	for (i = 0, failed = 0; i < cxlhdm->decoder_count; i++) {
>  		int target_map[CXL_DECODER_MAX_INTERLEAVE] = { 0 };
>  		int rc, target_count = cxlhdm->target_count;
>  		struct cxl_decoder *cxld;
> @@ -239,7 +252,13 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
>  			return PTR_ERR(cxld);
>  		}
>  
> -		init_hdm_decoder(cxld, target_map, cxlhdm->regs.hdm_decoder, i);
> +		rc = init_hdm_decoder(port, cxld, target_map,
> +				      cxlhdm->regs.hdm_decoder, i);
> +		if (rc) {
> +			put_device(&cxld->dev);
> +			failed++;
Not sure I'd have done it this way around, as opposed to
inited++ or similar, but up to you.
> +			continue;
> +		}
>  		rc = add_hdm_decoder(port, cxld, target_map);
>  		if (rc) {
>  			dev_warn(&port->dev,
> @@ -248,6 +267,11 @@ int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
>  		}
>  	}
>  
> +	if (failed == cxlhdm->decoder_count) {
> +		dev_err(&port->dev, "No valid decoders found\n");
> +		return -ENXIO;
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_decoders, CXL);
>
diff mbox series

Patch

diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
index c966ab0d51fe..4955ba16c9c8 100644
--- a/drivers/cxl/core/hdm.c
+++ b/drivers/cxl/core/hdm.c
@@ -150,8 +150,8 @@  static int to_interleave_ways(u32 ctrl)
 	}
 }
 
-static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map,
-			     void __iomem *hdm, int which)
+static int init_hdm_decoder(struct cxl_port *port, struct cxl_decoder *cxld,
+			    int *target_map, void __iomem *hdm, int which)
 {
 	u64 size, base;
 	u32 ctrl;
@@ -167,6 +167,11 @@  static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map,
 
 	if (!(ctrl & CXL_HDM_DECODER0_CTRL_COMMITTED))
 		size = 0;
+	if (base == U64_MAX || size == U64_MAX) {
+		dev_warn(&port->dev, "decoder%d.%d: Invalid resource range\n",
+			 port->id, cxld->id);
+		return -ENXIO;
+	}
 
 	cxld->decoder_range = (struct range) {
 		.start = base,
@@ -180,6 +185,12 @@  static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map,
 			cxld->flags |= CXL_DECODER_F_LOCK;
 	}
 	cxld->interleave_ways = to_interleave_ways(ctrl);
+	if (!cxld->interleave_ways) {
+		dev_warn(&port->dev,
+			 "decoder%d.%d: Invalid interleave ways (ctrl: %#x)\n",
+			 port->id, cxld->id, ctrl);
+		return -ENXIO;
+	}
 	cxld->interleave_granularity = to_interleave_granularity(ctrl);
 
 	if (FIELD_GET(CXL_HDM_DECODER0_CTRL_TYPE, ctrl))
@@ -188,12 +199,14 @@  static void init_hdm_decoder(struct cxl_decoder *cxld, int *target_map,
 		cxld->target_type = CXL_DECODER_ACCELERATOR;
 
 	if (is_cxl_endpoint(to_cxl_port(cxld->dev.parent)))
-		return;
+		return 0;
 
 	target_list.value =
 		ioread64_hi_lo(hdm + CXL_HDM_DECODER0_TL_LOW(which));
 	for (i = 0; i < cxld->interleave_ways; i++)
 		target_map[i] = target_list.target_id[i];
+
+	return 0;
 }
 
 /**
@@ -204,7 +217,7 @@  int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
 {
 	void __iomem *hdm = cxlhdm->regs.hdm_decoder;
 	struct cxl_port *port = cxlhdm->port;
-	int i, committed;
+	int i, committed, failed;
 	u32 ctrl;
 
 	/*
@@ -224,7 +237,7 @@  int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
 	if (committed != cxlhdm->decoder_count)
 		msleep(20);
 
-	for (i = 0; i < cxlhdm->decoder_count; i++) {
+	for (i = 0, failed = 0; i < cxlhdm->decoder_count; i++) {
 		int target_map[CXL_DECODER_MAX_INTERLEAVE] = { 0 };
 		int rc, target_count = cxlhdm->target_count;
 		struct cxl_decoder *cxld;
@@ -239,7 +252,13 @@  int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
 			return PTR_ERR(cxld);
 		}
 
-		init_hdm_decoder(cxld, target_map, cxlhdm->regs.hdm_decoder, i);
+		rc = init_hdm_decoder(port, cxld, target_map,
+				      cxlhdm->regs.hdm_decoder, i);
+		if (rc) {
+			put_device(&cxld->dev);
+			failed++;
+			continue;
+		}
 		rc = add_hdm_decoder(port, cxld, target_map);
 		if (rc) {
 			dev_warn(&port->dev,
@@ -248,6 +267,11 @@  int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm)
 		}
 	}
 
+	if (failed == cxlhdm->decoder_count) {
+		dev_err(&port->dev, "No valid decoders found\n");
+		return -ENXIO;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_NS_GPL(devm_cxl_enumerate_decoders, CXL);