| Message ID | 20220611020246.140637-1-dave@stgolabs.net |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cxl/port: Check for sane CXL.io values for device DVSEC | expand |
On 22-06-10 19:02:46, Davidlohr Bueso wrote: > These must always be set in the capability and control words. > Explicitly check for these and identify any bogus setups > while doing cxl_hdm_decode_init(). > > Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> > --- > drivers/cxl/core/pci.c | 14 ++++++++++++-- > drivers/cxl/cxlpci.h | 2 ++ > 2 files changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index c4c99ff7b55e..c69be8f120fc 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -367,12 +367,22 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm) > if (rc) > return rc; > > + if (!(cap & CXL_DVSEC_IO_CAPABLE)) { > + dev_dbg(dev, "Not IO Capable\n"); > + return -ENXIO; > + } I don't know if it's worth doing this. We wouldn't have gotten to the registers if it wasn't CXL.io capable. > + > + if (!(cap & CXL_DVSEC_MEM_CAPABLE)) { > + dev_dbg(dev, "Not MEM Capable\n"); > + return -ENXIO; > + } > + > rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl); > if (rc) > return rc; > > - if (!(cap & CXL_DVSEC_MEM_CAPABLE)) { > - dev_dbg(dev, "Not MEM Capable\n"); > + if (!(ctrl & CXL_DVSEC_IO_ENABLE)) { > + dev_dbg(dev, "Not IO Enable\n"); > return -ENXIO; > } > > diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h > index fce1c11729c2..845f59288dc3 100644 > --- a/drivers/cxl/cxlpci.h > +++ b/drivers/cxl/cxlpci.h > @@ -18,9 +18,11 @@ > /* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */ > #define CXL_DVSEC_PCIE_DEVICE 0 > #define CXL_DVSEC_CAP_OFFSET 0xA > +#define CXL_DVSEC_IO_CAPABLE BIT(1) > #define CXL_DVSEC_MEM_CAPABLE BIT(2) > #define CXL_DVSEC_HDM_COUNT_MASK GENMASK(5, 4) > #define CXL_DVSEC_CTRL_OFFSET 0xC > +#define CXL_DVSEC_IO_ENABLE BIT(1) > #define CXL_DVSEC_MEM_ENABLE BIT(2) > #define CXL_DVSEC_RANGE_SIZE_HIGH(i) (0x18 + (i * 0x10)) > #define CXL_DVSEC_RANGE_SIZE_LOW(i) (0x1C + (i * 0x10)) > -- > 2.36.1 >
On Fri, 10 Jun 2022, Ben Widawsky wrote: >On 22-06-10 19:02:46, Davidlohr Bueso wrote: >> + if (!(cap & CXL_DVSEC_IO_CAPABLE)) { >> + dev_dbg(dev, "Not IO Capable\n"); >> + return -ENXIO; >> + } > >I don't know if it's worth doing this. We wouldn't have gotten to the registers >if it wasn't CXL.io capable. Yeah I thought there might be reason as to why bit 2 was checked but not 1. Does this also apply for CXL_DVSEC_IO_ENABLE? fwiw I thought this was handy in case of bogus dvsec setups via qemu. >> + >> + if (!(cap & CXL_DVSEC_MEM_CAPABLE)) { >> + dev_dbg(dev, "Not MEM Capable\n"); >> + return -ENXIO; >> + } >> + Either way this check should still be moved before reading the ctrl word. Thanks, Davidlohr >> rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl); >> if (rc) >> return rc; >> >> - if (!(cap & CXL_DVSEC_MEM_CAPABLE)) { >> - dev_dbg(dev, "Not MEM Capable\n"); >> + if (!(ctrl & CXL_DVSEC_IO_ENABLE)) { >> + dev_dbg(dev, "Not IO Enable\n"); >> return -ENXIO; >> } >> >> diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h >> index fce1c11729c2..845f59288dc3 100644 >> --- a/drivers/cxl/cxlpci.h >> +++ b/drivers/cxl/cxlpci.h >> @@ -18,9 +18,11 @@ >> /* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */ >> #define CXL_DVSEC_PCIE_DEVICE 0 >> #define CXL_DVSEC_CAP_OFFSET 0xA >> +#define CXL_DVSEC_IO_CAPABLE BIT(1) >> #define CXL_DVSEC_MEM_CAPABLE BIT(2) >> #define CXL_DVSEC_HDM_COUNT_MASK GENMASK(5, 4) >> #define CXL_DVSEC_CTRL_OFFSET 0xC >> +#define CXL_DVSEC_IO_ENABLE BIT(1) >> #define CXL_DVSEC_MEM_ENABLE BIT(2) >> #define CXL_DVSEC_RANGE_SIZE_HIGH(i) (0x18 + (i * 0x10)) >> #define CXL_DVSEC_RANGE_SIZE_LOW(i) (0x1C + (i * 0x10)) >> -- >> 2.36.1 >>
On 22-06-10 21:26:01, Davidlohr Bueso wrote: > On Fri, 10 Jun 2022, Ben Widawsky wrote: > > > On 22-06-10 19:02:46, Davidlohr Bueso wrote: > > > > + if (!(cap & CXL_DVSEC_IO_CAPABLE)) { > > > + dev_dbg(dev, "Not IO Capable\n"); > > > + return -ENXIO; > > > + } > > > > I don't know if it's worth doing this. We wouldn't have gotten to the registers > > if it wasn't CXL.io capable. > > Yeah I thought there might be reason as to why bit 2 was checked but not 1. > Does this also apply for CXL_DVSEC_IO_ENABLE? fwiw I thought this was handy > in case of bogus dvsec setups via qemu. > > > > + > > > + if (!(cap & CXL_DVSEC_MEM_CAPABLE)) { > > > + dev_dbg(dev, "Not MEM Capable\n"); > > > + return -ENXIO; > > > + } > > > + > > Either way this check should still be moved before reading the ctrl > word. > > Thanks, > Davidlohr I don't seem to have this code in any branch readily available, so it's hard to say. In general I don't see a marked improvement in doing this, but if you can address it in the commit message on why/how it might make better debugging, then I'm all for it. Ben > > > > rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl); > > > if (rc) > > > return rc; > > > > > > - if (!(cap & CXL_DVSEC_MEM_CAPABLE)) { > > > - dev_dbg(dev, "Not MEM Capable\n"); > > > + if (!(ctrl & CXL_DVSEC_IO_ENABLE)) { > > > + dev_dbg(dev, "Not IO Enable\n"); > > > return -ENXIO; > > > } > > > > > > diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h > > > index fce1c11729c2..845f59288dc3 100644 > > > --- a/drivers/cxl/cxlpci.h > > > +++ b/drivers/cxl/cxlpci.h > > > @@ -18,9 +18,11 @@ > > > /* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */ > > > #define CXL_DVSEC_PCIE_DEVICE 0 > > > #define CXL_DVSEC_CAP_OFFSET 0xA > > > +#define CXL_DVSEC_IO_CAPABLE BIT(1) > > > #define CXL_DVSEC_MEM_CAPABLE BIT(2) > > > #define CXL_DVSEC_HDM_COUNT_MASK GENMASK(5, 4) > > > #define CXL_DVSEC_CTRL_OFFSET 0xC > > > +#define CXL_DVSEC_IO_ENABLE BIT(1) > > > #define CXL_DVSEC_MEM_ENABLE BIT(2) > > > #define CXL_DVSEC_RANGE_SIZE_HIGH(i) (0x18 + (i * 0x10)) > > > #define CXL_DVSEC_RANGE_SIZE_LOW(i) (0x1C + (i * 0x10)) > > > -- > > > 2.36.1 > > >
Ben Widawsky wrote: > On 22-06-10 21:26:01, Davidlohr Bueso wrote: > > On Fri, 10 Jun 2022, Ben Widawsky wrote: > > > > > On 22-06-10 19:02:46, Davidlohr Bueso wrote: > > > > > > + if (!(cap & CXL_DVSEC_IO_CAPABLE)) { > > > > + dev_dbg(dev, "Not IO Capable\n"); > > > > + return -ENXIO; > > > > + } > > > > > > I don't know if it's worth doing this. We wouldn't have gotten to the registers > > > if it wasn't CXL.io capable. > > > > Yeah I thought there might be reason as to why bit 2 was checked but not 1. > > Does this also apply for CXL_DVSEC_IO_ENABLE? fwiw I thought this was handy > > in case of bogus dvsec setups via qemu. > > > > > > + > > > > + if (!(cap & CXL_DVSEC_MEM_CAPABLE)) { > > > > + dev_dbg(dev, "Not MEM Capable\n"); > > > > + return -ENXIO; > > > > + } > > > > + > > > > Either way this check should still be moved before reading the ctrl > > word. > > > > Thanks, > > Davidlohr > > I don't seem to have this code in any branch readily available, so it's hard to > say. In general I don't see a marked improvement in doing this, but if you can > address it in the commit message on why/how it might make better debugging, then > I'm all for it. > Agree, the capabilities are additive. So CXL.mem does not get negotiated unless CXL.io is also present. However, if a device declares to support CXL.mem and not CXL.io I would just claim "Robustness Principle" and try to keep going with init because as long as CXL.mem cycles work and the device responds to mmio and config cycles then the value of this bit is moot.
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index c4c99ff7b55e..c69be8f120fc 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -367,12 +367,22 @@ int cxl_hdm_decode_init(struct cxl_dev_state *cxlds, struct cxl_hdm *cxlhdm) if (rc) return rc; + if (!(cap & CXL_DVSEC_IO_CAPABLE)) { + dev_dbg(dev, "Not IO Capable\n"); + return -ENXIO; + } + + if (!(cap & CXL_DVSEC_MEM_CAPABLE)) { + dev_dbg(dev, "Not MEM Capable\n"); + return -ENXIO; + } + rc = pci_read_config_word(pdev, d + CXL_DVSEC_CTRL_OFFSET, &ctrl); if (rc) return rc; - if (!(cap & CXL_DVSEC_MEM_CAPABLE)) { - dev_dbg(dev, "Not MEM Capable\n"); + if (!(ctrl & CXL_DVSEC_IO_ENABLE)) { + dev_dbg(dev, "Not IO Enable\n"); return -ENXIO; } diff --git a/drivers/cxl/cxlpci.h b/drivers/cxl/cxlpci.h index fce1c11729c2..845f59288dc3 100644 --- a/drivers/cxl/cxlpci.h +++ b/drivers/cxl/cxlpci.h @@ -18,9 +18,11 @@ /* CXL 2.0 8.1.3: PCIe DVSEC for CXL Device */ #define CXL_DVSEC_PCIE_DEVICE 0 #define CXL_DVSEC_CAP_OFFSET 0xA +#define CXL_DVSEC_IO_CAPABLE BIT(1) #define CXL_DVSEC_MEM_CAPABLE BIT(2) #define CXL_DVSEC_HDM_COUNT_MASK GENMASK(5, 4) #define CXL_DVSEC_CTRL_OFFSET 0xC +#define CXL_DVSEC_IO_ENABLE BIT(1) #define CXL_DVSEC_MEM_ENABLE BIT(2) #define CXL_DVSEC_RANGE_SIZE_HIGH(i) (0x18 + (i * 0x10)) #define CXL_DVSEC_RANGE_SIZE_LOW(i) (0x1C + (i * 0x10))
These must always be set in the capability and control words. Explicitly check for these and identify any bogus setups while doing cxl_hdm_decode_init(). Signed-off-by: Davidlohr Bueso <dave@stgolabs.net> --- drivers/cxl/core/pci.c | 14 ++++++++++++-- drivers/cxl/cxlpci.h | 2 ++ 2 files changed, 14 insertions(+), 2 deletions(-)