| Message ID | 20240325235914.1897647-3-dave.jiang@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: Add Secondary Bus Reset (SBR) support for CXL | expand |
Dave Jiang wrote: > CXL spec r3.1 8.1.5.2 > By default Secondary Bus Reset (SBR) is masked for CXL ports. Introduce a > new PCI reset method "cxl_bus_force" to force SBR on CXL ports by setting > the unmask SBR bit in the CXL DVSEC port control register before performing > the bus reset and restore the original value of the bit post reset. The > new reset method allows the user to intentionally perform SBR on a CXL > device without needing to set the "Unmask SBR" bit via a user tool. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > v2: > - Use pci_upstream_bridge() instead of dev->bus->self. > - Return -ENOTTY as error for reset function > --- > drivers/pci/pci.c | 52 +++++++++++++++++++++++++++++++++++++++++++-- > include/linux/pci.h | 2 +- > 2 files changed, 51 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 259e5d6538bb..cbcad8f0880d 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4933,6 +4933,12 @@ static bool pci_is_cxl(struct pci_dev *dev) > CXL_DVSEC_PCIE_DEVICE); > } > > +static int cxl_port_dvsec(struct pci_dev *dev) > +{ > + return pci_find_dvsec_capability(dev, PCI_DVSEC_VENDOR_ID_CXL, > + CXL_DVSEC_PCIE_PORT); > +} > + > static bool is_cxl_port_sbr_masked(struct pci_dev *dev) > { > int dvsec; > @@ -4942,8 +4948,7 @@ static bool is_cxl_port_sbr_masked(struct pci_dev *dev) > /* > * No DVSEC found, must not be CXL port. > */ > - dvsec = pci_find_dvsec_capability(dev, PCI_DVSEC_VENDOR_ID_CXL, > - CXL_DVSEC_PCIE_PORT); Once applied, those 2 lines had a very short life in mainline. Perhaps just define cxl_port_dvsec() in patch1? > + dvsec = cxl_port_dvsec(dev); > if (!dvsec) > return false; > > @@ -4982,6 +4987,48 @@ static int pci_reset_bus_function(struct pci_dev *dev, bool probe) > return pci_parent_bus_reset(dev, probe); > } > > +static int cxl_reset_bus_function(struct pci_dev *dev, bool probe) > +{ > + struct pci_dev *bridge; > + int dvsec; > + int rc; > + u16 reg, val; > + > + if (!pci_is_cxl(dev)) > + return -ENOTTY; > + > + bridge = pci_upstream_bridge(dev); > + if (!bridge) > + return -ENOTTY; > + > + dvsec = cxl_port_dvsec(bridge); > + if (!dvsec) > + return -ENOTTY; > + > + if (probe) > + return 0; > + > + rc = pci_read_config_word(bridge, dvsec + CXL_DVSEC_PORT_CONTROL, > + ®); > + if (rc) > + return -ENOTTY; > + > + if (!(reg & CXL_DVSEC_PORT_CONTROL_UNMASK_SBR)) { > + val = reg | CXL_DVSEC_PORT_CONTROL_UNMASK_SBR; > + pci_write_config_word(bridge, > + dvsec + CXL_DVSEC_PORT_CONTROL, val); > + } else { > + val = reg; > + } > + > + rc = pci_reset_bus_function(dev, probe); > + > + if (reg != val) > + pci_write_config_word(bridge, dvsec + CXL_DVSEC_PORT_CONTROL, reg); Doesn't this whole sequence need to be wrapped in pci_cfg_access_lock()? Otherwise userspace can get confused if it races to access CXL_DVSEC_PCIE_PORT while the link is down, or if it races to write Unmask SBR and messes up the saved value. I took a quick look and did not see this lock taken from reset_method_store(). > + > + return rc; > +} > + > void pci_dev_lock(struct pci_dev *dev) > { > /* block PM suspend, driver probe, etc. */ > @@ -5066,6 +5113,7 @@ static const struct pci_reset_fn_method pci_reset_fn_methods[] = { > { pci_af_flr, .name = "af_flr" }, > { pci_pm_reset, .name = "pm" }, > { pci_reset_bus_function, .name = "bus" }, > + { cxl_reset_bus_function, .name = "cxl_bus_force" }, Why include "_force" in the name? "cxl_bus" already implies "do what is needed to bus reset this CXL link".
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 259e5d6538bb..cbcad8f0880d 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4933,6 +4933,12 @@ static bool pci_is_cxl(struct pci_dev *dev) CXL_DVSEC_PCIE_DEVICE); } +static int cxl_port_dvsec(struct pci_dev *dev) +{ + return pci_find_dvsec_capability(dev, PCI_DVSEC_VENDOR_ID_CXL, + CXL_DVSEC_PCIE_PORT); +} + static bool is_cxl_port_sbr_masked(struct pci_dev *dev) { int dvsec; @@ -4942,8 +4948,7 @@ static bool is_cxl_port_sbr_masked(struct pci_dev *dev) /* * No DVSEC found, must not be CXL port. */ - dvsec = pci_find_dvsec_capability(dev, PCI_DVSEC_VENDOR_ID_CXL, - CXL_DVSEC_PCIE_PORT); + dvsec = cxl_port_dvsec(dev); if (!dvsec) return false; @@ -4982,6 +4987,48 @@ static int pci_reset_bus_function(struct pci_dev *dev, bool probe) return pci_parent_bus_reset(dev, probe); } +static int cxl_reset_bus_function(struct pci_dev *dev, bool probe) +{ + struct pci_dev *bridge; + int dvsec; + int rc; + u16 reg, val; + + if (!pci_is_cxl(dev)) + return -ENOTTY; + + bridge = pci_upstream_bridge(dev); + if (!bridge) + return -ENOTTY; + + dvsec = cxl_port_dvsec(bridge); + if (!dvsec) + return -ENOTTY; + + if (probe) + return 0; + + rc = pci_read_config_word(bridge, dvsec + CXL_DVSEC_PORT_CONTROL, + ®); + if (rc) + return -ENOTTY; + + if (!(reg & CXL_DVSEC_PORT_CONTROL_UNMASK_SBR)) { + val = reg | CXL_DVSEC_PORT_CONTROL_UNMASK_SBR; + pci_write_config_word(bridge, + dvsec + CXL_DVSEC_PORT_CONTROL, val); + } else { + val = reg; + } + + rc = pci_reset_bus_function(dev, probe); + + if (reg != val) + pci_write_config_word(bridge, dvsec + CXL_DVSEC_PORT_CONTROL, reg); + + return rc; +} + void pci_dev_lock(struct pci_dev *dev) { /* block PM suspend, driver probe, etc. */ @@ -5066,6 +5113,7 @@ static const struct pci_reset_fn_method pci_reset_fn_methods[] = { { pci_af_flr, .name = "af_flr" }, { pci_pm_reset, .name = "pm" }, { pci_reset_bus_function, .name = "bus" }, + { cxl_reset_bus_function, .name = "cxl_bus_force" }, }; static ssize_t reset_method_show(struct device *dev, diff --git a/include/linux/pci.h b/include/linux/pci.h index 16493426a04f..235f37715a43 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -51,7 +51,7 @@ PCI_STATUS_PARITY) /* Number of reset methods used in pci_reset_fn_methods array in pci.c */ -#define PCI_NUM_RESET_METHODS 7 +#define PCI_NUM_RESET_METHODS 8 #define PCI_RESET_PROBE true #define PCI_RESET_DO_RESET false
CXL spec r3.1 8.1.5.2 By default Secondary Bus Reset (SBR) is masked for CXL ports. Introduce a new PCI reset method "cxl_bus_force" to force SBR on CXL ports by setting the unmask SBR bit in the CXL DVSEC port control register before performing the bus reset and restore the original value of the bit post reset. The new reset method allows the user to intentionally perform SBR on a CXL device without needing to set the "Unmask SBR" bit via a user tool. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- v2: - Use pci_upstream_bridge() instead of dev->bus->self. - Return -ENOTTY as error for reset function --- drivers/pci/pci.c | 52 +++++++++++++++++++++++++++++++++++++++++++-- include/linux/pci.h | 2 +- 2 files changed, 51 insertions(+), 3 deletions(-)