diff mbox series

[V12,7/9] cxl/port: Introduce cxl_cdat_valid()

Message ID 20220628041527.742333-8-ira.weiny@intel.com
State New
Headers show
Series CXL: Read CDAT and DSMAS data | expand

Commit Message

Ira Weiny June 28, 2022, 4:15 a.m. UTC 
From: Ira Weiny <ira.weiny@intel.com>

The CDAT data is protected by a checksum and should be the proper
length.

Introduce cxl_cdat_valid() to validate the data.  While at it check and
store the sequence number.

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

---
Changes from V8
	Move code to cxl/core/pci.c

Changes from V6
	Change name to cxl_cdat_valid() as this validates all the CDAT
		data not just the header
	Add error and debug prints

Changes from V5
	New patch, split out
	Update cdat_hdr_valid()
		Remove revision and cs field parsing
			There is no point in these
		Add seq check and debug print.
---
 drivers/cxl/cdat.h     |  2 ++
 drivers/cxl/core/pci.c | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

Comments

Jonathan Cameron June 28, 2022, 2:49 p.m. UTC | #1 
On Mon, 27 Jun 2022 21:15:25 -0700
ira.weiny@intel.com wrote:

> From: Ira Weiny <ira.weiny@intel.com>
> 
> The CDAT data is protected by a checksum and should be the proper
> length.
> 
> Introduce cxl_cdat_valid() to validate the data.  While at it check and
> store the sequence number.
> 
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
Minor ordering comment.  With that tidied up
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 4bd479ec0253..6d775cc3dca1 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -532,6 +532,40 @@ static int cxl_cdat_get_length(struct device *dev,
>  	return rc;
>  }
>  
> +static bool cxl_cdat_valid(struct device *dev, struct cxl_cdat *cdat)
> +{
> +	u32 *table = cdat->table;
> +	u8 *data8 = cdat->table;
> +	u32 length, seq;
> +	u8 check;
> +	int i;
> +
> +	length = FIELD_GET(CDAT_HEADER_DW0_LENGTH, table[0]);
> +	if ((length < CDAT_HEADER_LENGTH_BYTES) || (length > cdat->length)) {
> +		dev_err(dev, "CDAT Invalid length %u (%zu-%zu)\n", length,
> +			CDAT_HEADER_LENGTH_BYTES, cdat->length);
> +		return false;
> +	}
> +
> +	for (check = 0, i = 0; i < length; i++)
> +		check += data8[i];
> +
> +	dev_dbg(dev, "CDAT length %u CS %u\n", length, check);
> +	if (check != 0) {
> +		dev_err(dev, "CDAT Invalid checksum %u\n", check);
> +		return false;
> +	}
> +
> +	seq = FIELD_GET(CDAT_HEADER_DW3_SEQUENCE, table[3]);
> +	/* Store the sequence for now. */
> +	if (cdat->seq != seq) {
> +		dev_info(dev, "CDAT seq change %x -> %x\n", cdat->seq, seq);
> +		cdat->seq = seq;
> +	}
> +
> +	return true;
> +}
> +
>  static int cxl_cdat_read_table(struct device *dev,
>  			       struct pci_doe_mb *cdat_mb,
>  			       struct cxl_cdat *cdat)
> @@ -579,6 +613,8 @@ static int cxl_cdat_read_table(struct device *dev,
>  
>  	} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
>  
> +	if (!rc && !cxl_cdat_valid(dev, cdat))
> +		return -EIO;

I'd prefer those handled separately as flow is more readable if error
handling always out of line.

	if (rc)
		return rc;

	if (!cxl_cdata_valid)
		return -EIO;

	return 0;

>  	return rc;
>  }
>
Ira Weiny June 30, 2022, 4:42 a.m. UTC | #2 
On Tue, Jun 28, 2022 at 03:49:42PM +0100, Jonathan Cameron wrote:
> On Mon, 27 Jun 2022 21:15:25 -0700
> ira.weiny@intel.com wrote:
> 
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > The CDAT data is protected by a checksum and should be the proper
> > length.
> > 
> > Introduce cxl_cdat_valid() to validate the data.  While at it check and
> > store the sequence number.
> > 
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > 
> Minor ordering comment.  With that tidied up
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 
> > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> > index 4bd479ec0253..6d775cc3dca1 100644
> > --- a/drivers/cxl/core/pci.c
> > +++ b/drivers/cxl/core/pci.c
> > @@ -532,6 +532,40 @@ static int cxl_cdat_get_length(struct device *dev,
> >  	return rc;
> >  }
> >  
> > +static bool cxl_cdat_valid(struct device *dev, struct cxl_cdat *cdat)
> > +{
> > +	u32 *table = cdat->table;
> > +	u8 *data8 = cdat->table;
> > +	u32 length, seq;
> > +	u8 check;
> > +	int i;
> > +
> > +	length = FIELD_GET(CDAT_HEADER_DW0_LENGTH, table[0]);
> > +	if ((length < CDAT_HEADER_LENGTH_BYTES) || (length > cdat->length)) {
> > +		dev_err(dev, "CDAT Invalid length %u (%zu-%zu)\n", length,
> > +			CDAT_HEADER_LENGTH_BYTES, cdat->length);
> > +		return false;
> > +	}
> > +
> > +	for (check = 0, i = 0; i < length; i++)
> > +		check += data8[i];
> > +
> > +	dev_dbg(dev, "CDAT length %u CS %u\n", length, check);
> > +	if (check != 0) {
> > +		dev_err(dev, "CDAT Invalid checksum %u\n", check);
> > +		return false;
> > +	}
> > +
> > +	seq = FIELD_GET(CDAT_HEADER_DW3_SEQUENCE, table[3]);
> > +	/* Store the sequence for now. */
> > +	if (cdat->seq != seq) {
> > +		dev_info(dev, "CDAT seq change %x -> %x\n", cdat->seq, seq);
> > +		cdat->seq = seq;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> >  static int cxl_cdat_read_table(struct device *dev,
> >  			       struct pci_doe_mb *cdat_mb,
> >  			       struct cxl_cdat *cdat)
> > @@ -579,6 +613,8 @@ static int cxl_cdat_read_table(struct device *dev,
> >  
> >  	} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
> >  
> > +	if (!rc && !cxl_cdat_valid(dev, cdat))
> > +		return -EIO;
> 
> I'd prefer those handled separately as flow is more readable if error
> handling always out of line.
> 
> 	if (rc)
> 		return rc;

Actually rc is never not 0 here.  :-/

> 
> 	if (!cxl_cdata_valid)
> 		return -EIO;
> 
> 	return 0;

So this is all that is needed.  But the previous patch needs some clean up
to make sense in the series.

Ira
diff mbox series

Patch

diff --git a/drivers/cxl/cdat.h b/drivers/cxl/cdat.h
index c6a48ab326bf..39eb561081f2 100644
--- a/drivers/cxl/cdat.h
+++ b/drivers/cxl/cdat.h
@@ -91,10 +91,12 @@ 
  *
  * @table: cache of CDAT table
  * @length: length of cached CDAT table
+ * @seq: Last read Sequence number of the CDAT table
  */
 struct cxl_cdat {
 	void *table;
 	size_t length;
+	u32 seq;
 };
 
 #endif /* !__CXL_CDAT_H__ */
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
index 4bd479ec0253..6d775cc3dca1 100644
--- a/drivers/cxl/core/pci.c
+++ b/drivers/cxl/core/pci.c
@@ -532,6 +532,40 @@  static int cxl_cdat_get_length(struct device *dev,
 	return rc;
 }
 
+static bool cxl_cdat_valid(struct device *dev, struct cxl_cdat *cdat)
+{
+	u32 *table = cdat->table;
+	u8 *data8 = cdat->table;
+	u32 length, seq;
+	u8 check;
+	int i;
+
+	length = FIELD_GET(CDAT_HEADER_DW0_LENGTH, table[0]);
+	if ((length < CDAT_HEADER_LENGTH_BYTES) || (length > cdat->length)) {
+		dev_err(dev, "CDAT Invalid length %u (%zu-%zu)\n", length,
+			CDAT_HEADER_LENGTH_BYTES, cdat->length);
+		return false;
+	}
+
+	for (check = 0, i = 0; i < length; i++)
+		check += data8[i];
+
+	dev_dbg(dev, "CDAT length %u CS %u\n", length, check);
+	if (check != 0) {
+		dev_err(dev, "CDAT Invalid checksum %u\n", check);
+		return false;
+	}
+
+	seq = FIELD_GET(CDAT_HEADER_DW3_SEQUENCE, table[3]);
+	/* Store the sequence for now. */
+	if (cdat->seq != seq) {
+		dev_info(dev, "CDAT seq change %x -> %x\n", cdat->seq, seq);
+		cdat->seq = seq;
+	}
+
+	return true;
+}
+
 static int cxl_cdat_read_table(struct device *dev,
 			       struct pci_doe_mb *cdat_mb,
 			       struct cxl_cdat *cdat)
@@ -579,6 +613,8 @@  static int cxl_cdat_read_table(struct device *dev,
 
 	} while (entry_handle != CXL_DOE_TABLE_ACCESS_LAST_ENTRY);
 
+	if (!rc && !cxl_cdat_valid(dev, cdat))
+		return -EIO;
 	return rc;
 }