On Tue, Jun 28, 2022 at 03:49:42PM +0100, Jonathan Cameron wrote:
> 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;
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
