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

[Jonathan Cameron]

On Mon, 27 Jun 2022 21:15:25 -0700
ira.weiny@xxxxxxxxx wrote:

> From: Ira Weiny <ira.weiny@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> 
Minor ordering comment.  With that tidied up
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>


> 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;
>  }
>