| Message ID | 167571658747.587790.17252249256706733075.stgit@djiang5-mobl3.local |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Add support for QTG ID retrieval for CXL subsystem | expand |
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); > >
On 2/9/23 4:34 AM, Jonathan Cameron wrote: > 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. Dan advised to just drop the ACPICA changes and just do it locally since the verification code is tiny and simple. > > 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!) Ok, I'll create a prep patch to change this as you suggested. > > >> + dev_err(dev, "CDAT data checksum error\n"); >> + } >> } >> EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL); >> >> >
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) { + /* Don't leave table data allocated on error */ + devm_kfree(dev, port->cdat.table); + port->cdat.table = NULL; + port->cdat.length = 0; + dev_err(dev, "CDAT data checksum error\n"); + } } EXPORT_SYMBOL_NS_GPL(read_cdat_data, CXL);
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> --- drivers/cxl/core/pci.c | 11 +++++++++++ 1 file changed, 11 insertions(+)