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

[Ira Weiny]

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