Re: [PATCH V6 08/10] cxl/cdat: Introduce cdat_hdr_valid()

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

On Tue, 1 Feb 2022 14:29:03 -0800
Ira Weiny <ira.weiny@intel.com> wrote:

> On Tue, Feb 01, 2022 at 10:56:40AM -0800, Widawsky, Ben wrote:
> > On 22-01-31 23:19:50, ira.weiny@intel.com wrote:  
> > > From: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > The CDAT data is protected by a checksum which should be checked when
> > > the CDAT is read to ensure it is valid.  In addition the lengths
> > > specified should be checked.
> > > 
> > > Introduce cdat_hdr_valid() to check the checksum.  While at it check and
> > > store the sequence number.
> > > 
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > 
> > > ---
> > > Changes from V5
> > > 	New patch, split out
> > > 	Update cdat_hdr_valid()
> > > 		Remove revision and cs field parsing
> > > 			There is no point in these
> > > 		Add seq check and debug print.
> > > ---
> > >  drivers/cxl/cdat.h |  2 ++
> > >  drivers/cxl/pci.c  | 32 ++++++++++++++++++++++++++++++++
> > >  2 files changed, 34 insertions(+)
> > > 
> > > diff --git a/drivers/cxl/cdat.h b/drivers/cxl/cdat.h
> > > index 4722b6bbbaf0..a7725d26f2d2 100644
> > > --- a/drivers/cxl/cdat.h
> > > +++ b/drivers/cxl/cdat.h
> > > @@ -88,10 +88,12 @@
> > >   *
> > >   * @table: cache of CDAT table
> > >   * @length: length of cached CDAT table
> > > + * @seq: Last read Sequence number of the CDAT table
> > >   */
> > >  struct cxl_cdat {
> > >  	void *table;
> > >  	size_t length;
> > > +	u32 seq;
> > >  };
> > >  
> > >  #endif /* !__CXL_CDAT_H__ */
> > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > > index 28b973a9e29e..c362c75feed2 100644
> > > --- a/drivers/cxl/pci.c
> > > +++ b/drivers/cxl/pci.c
> > > @@ -586,6 +586,35 @@ static int cxl_setup_doe_devices(struct cxl_dev_state *cxlds)
> > >  	return 0;
> > >  }
> > >  
> > > +static bool cxl_cdat_hdr_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)
> > > +		return false;
> > > +
> > > +	if (length > cdat->length)
> > > +		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;
> > > +	}  
> > 
> > If sequence hasn't changed you could short-circuit the checksum.  
> 
> I'm not sure.  Jonathan mentioned that reading may race with updates and that
> the correct thing to do is re-read.[1]

As things stand I 'think' a failure of the checksum on a previous run wouldn't
mean we didn't store the sequence number.

Now we only call this once at the moment so that doesn't matter yet..

If on each call we rerun to hopefully get an update after the race with
a good checksum / sequence number and don't store it on failure to validate
then we could indeed just use the sequence check to skip the checksum validation.
Mind you this isn't a hot path... Do we really care? 


> 
> But I should probably check the CS first...
> 
> Ira
> 
> [1] https://lore.kernel.org/linux-cxl/20211108145239.000010a5@Huawei.com/
> 
> >   
> > > +
> > > +	for (check = 0, i = 0; i < length; i++)
> > > +		check += data8[i];
> > > +
> > > +	return check == 0;
> > > +}
> > > +
> > >  #define CDAT_DOE_REQ(entry_handle)					\
> > >  	(FIELD_PREP(CXL_DOE_TABLE_ACCESS_REQ_CODE,			\
> > >  		    CXL_DOE_TABLE_ACCESS_REQ_CODE_READ) |		\
> > > @@ -658,6 +687,9 @@ static int cxl_cdat_read_table(struct cxl_dev_state *cxlds,
> > >  
> > >  	} while (entry_handle != 0xFFFF);
> > >  
> > > +	if (!cxl_cdat_hdr_valid(cxlds->dev, cdat))
> > > +		return -EIO;
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > -- 
> > > 2.31.1
> > >