Re: [RFC PATCH v2 3/4] cxl/mem: Add CDAT table reading from DOE

[Ben Widawsky <ben.widawsky@intel.com>]

On 21-04-14 14:14:34, Konrad Rzeszutek Wilk wrote:
> > +static int cdat_to_buffer(struct pcie_doe *doe, u32 *buffer, size_t length)
> > +{
> > +	int entry_handle = 0;
> > +	int rc;
> > +
> > +	do {
> > +		u32 cdat_request_pl = CDAT_DOE_REQ(entry_handle);
> > +		u32 cdat_response_pl[32];
> > +		struct pcie_doe_exchange ex = {
> > +			.vid = PCI_DVSEC_VENDOR_ID_CXL,
> > +			.protocol = CXL_DOE_PROTOCOL_TABLE_ACCESS,
> > +			.request_pl = &cdat_request_pl,
> > +			.request_pl_sz = sizeof(cdat_request_pl),
> > +			.response_pl = cdat_response_pl,
> > +			.response_pl_sz = sizeof(cdat_response_pl),
> > +		};
> > +		size_t entry_dw;
> > +		u32 *entry;
> > +
> > +		rc = pcie_doe_sync(doe, &ex);
> > +		if (rc < 0)
> > +			return rc;
> > +
> > +		entry = cdat_response_pl + 1;
> > +		entry_dw = rc / sizeof(u32);
> 
> Could you >> 2?

What's the reasoning for >> 2? As someone who regularly does this because of
habit, using divide is generally a lot more implicitly descriptive.

> > +		/* Skip Header */
> > +		entry_dw -= 1;
> > +		entry_dw = min(length / 4, entry_dw);
> 
> Ditto here on length?
> 
> Also should you double check that length is not greater than
> sizeof(cdat_respone_pl)?
> 
> > +		memcpy(buffer, entry, entry_dw * sizeof(u32));
> > +		length -= entry_dw * sizeof(u32);
> 
> Would it be just easier to convert length at the start to be >> 2
> instead of this * and / ?
> 

Same as above. Assuming the compiler isn't braindead, I think it reads better as
it is.

> 
> > +		buffer += entry_dw;
> > +		entry_handle = FIELD_GET(CXL_DOE_TABLE_ACCESS_ENTRY_HANDLE, cdat_response_pl[0]);
> > +
> > +	} while (entry_handle != 0xFFFF);
> > +
> > +	return 0;
> > +}
> > +