Re: [PATCH 12/18] cxl: Add helpers to calculate pci latency for the CXL device

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Feb 07, 2023 at 01:51:17PM -0700, Dave Jiang wrote:
> 
> 
> On 2/6/23 3:39 PM, Bjorn Helgaas wrote:
> > On Mon, Feb 06, 2023 at 01:51:10PM -0700, Dave Jiang wrote:
> > > The latency is calculated by dividing the FLIT size over the
> > > bandwidth. Add support to retrieve the FLIT size for the CXL
> > > device and calculate the latency of the downstream link.

> > I guess you only care about the latency of a single link, not the
> > entire path?
> 
> I am adding each of the link individually together in the next
> patch. Are you suggesting a similar function like
> pcie_bandwidth_available() but for latency for the entire path?

Only a clarifying question.

> > > +static int cxl_get_flit_size(struct pci_dev *pdev)
> > > +{
> > > +	if (cxl_pci_flit_256(pdev))
> > > +		return 256;
> > > +
> > > +	return 66;
> > 
> > I don't know about the 66-byte flit format, maybe this part is
> > CXL-specific?
> 
> 68-byte flit format. Looks like this is a typo from me.

This part must be CXL-specific, since I don't think PCIe mentions
68-byte flits.

> > > + * The table indicates that if PCIe Flit Mode is set, then CXL is in 256B flits
> > > + * mode, otherwise it's 68B flits mode.
> > > + */
> > > +static inline bool cxl_pci_flit_256(struct pci_dev *pdev)
> > > +{
> > > +	u32 lnksta2;
> > > +
> > > +	pcie_capability_read_dword(pdev, PCI_EXP_LNKSTA2, &lnksta2);
> > > +	return lnksta2 & BIT(10);
> > 
> > Add a #define for the bit.
> 
> ok will add.
> 
> > 
> > AFAICT, the PCIe spec defines this bit, and it only indicates the link
> > is or will be operating in Flit Mode; it doesn't actually say anything
> > about how large the flits are.  I suppose that's because PCIe only
> > talks about 256B flits, not 66B ones?
> 
> Looking at CXL v1.0 rev3.0 6.2.3 "256B Flit Mode", table 6-4, it shows that
> when PCIe Flit Mode is set, then CXL is in 256B flits mode, otherwise, it is
> 68B flits. So an assumption is made here regarding the flit side based on
> the table.

So reading PCI_EXP_LNKSTA2 and extracting the Flit Mode bit is
PCIe-generic, but the interpretation of "PCIe Flit Mode not enabled
means 68-byte flits" is CXL-specific?

This sounds wrong, but I don't know quite how.  How would the PCI core
manage links where Flit Mode being cleared really means Flit Mode is
*enabled* but with a different size?  Seems like something could go
wrong there.

Bjorn