Re: [PATCH 03/18] cxl: Add checksum verification to CDAT from CXL

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 06 Feb 2023 13:49:48 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> A CDAT table is available from a CXL device. The table is read by the
> driver and cached in software. With the CXL subsystem needing to parse the
> CDAT table, the checksum should be verified. Add checksum verification
> after the CDAT table is read from device.
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
Hi Dave,

Some comments on this follow on from previous patch so may not
be relevant once you've updated how that is done.

Jonathan

> ---
>  drivers/cxl/core/pci.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c
> index 57764e9cd19d..a24dac36bedd 100644
> --- a/drivers/cxl/core/pci.c
> +++ b/drivers/cxl/core/pci.c
> @@ -3,6 +3,7 @@
>  #include <linux/io-64-nonatomic-lo-hi.h>
>  #include <linux/device.h>
>  #include <linux/delay.h>
> +#include <linux/acpi.h>
>  #include <linux/pci.h>
>  #include <linux/pci-doe.h>
>  #include <cxlpci.h>
> @@ -592,6 +593,7 @@ void read_cdat_data(struct cxl_port *port)
>  	struct device *dev = &port->dev;
>  	struct device *uport = port->uport;
>  	size_t cdat_length;
> +	acpi_status status;
>  	int rc;
>  
>  	cdat_doe = find_cdat_doe(uport);
> @@ -620,5 +622,14 @@ void read_cdat_data(struct cxl_port *port)
>  		port->cdat.length = 0;
>  		dev_err(dev, "CDAT data read error\n");
>  	}
> +
> +	status = acpi_ut_verify_cdat_checksum(port->cdat.table, port->cdat.length);
> +	if (status != AE_OK) {

if (ACPI_FAILURE(acpi_ut...))  or better still put that in the wrapper I suggeste
in previous patch so that we have normal kernel return code handling out here.


> +		/* Don't leave table data allocated on error */
> +		devm_kfree(dev, port->cdat.table);
> +		port->cdat.table = NULL;
> +		port->cdat.length = 0;

I'd rather see us manipulate a local copy of cdat_length, and cdat_table
then only assign them to the port->cdat fields the success path rather
than setting them then unsetting the on error.

Diff will be bigger, but nicer resulting code (and hopefully diff
won't be too big!)


> +		dev_err(dev, "CDAT data checksum error\n");
> +	}
>  }
>  EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
> 
>