| Message ID | 20240402234848.3287160-4-dave.jiang@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: Add Secondary Bus Reset (SBR) support for CXL | expand |
On Tue, 2 Apr 2024 16:45:31 -0700 Dave Jiang <dave.jiang@intel.com> 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" 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> A few trivial things inline. Otherwise looks fine. FWIW Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > v3: > - move cxl_port_dvsec() to previous patch. (Dan) > - add pci_cfg_access_lock() for the bridge. (Dan) > - Change cxl_bus_force method to cxl_bus. (Dan) > --- > drivers/pci/pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/pci.h | 2 +- > 2 files changed, 45 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 00eddb451102..3989c8888813 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4982,6 +4982,49 @@ 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; Lukas' comment on previous applies to this as well. > + int rc; > + u16 reg, val; Maybe combine lines as appropriate. > + > + bridge = pci_upstream_bridge(dev); > + if (!bridge) > + return -ENOTTY; > + > + dvsec = cxl_port_dvsec(bridge); > + if (!dvsec) > + return -ENOTTY; > + > + if (probe) > + return 0; > + > + pci_cfg_access_lock(bridge); > + rc = pci_read_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, ®); > + if (rc) { > + rc = -ENOTTY; > + goto out; > + } > + > + if (!(reg & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR)) { > + val = reg | PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR; > + pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, > + val); > + } else { > + val = reg; > + } > + > + rc = pci_reset_bus_function(dev, probe); > + > + if (reg != val) > + pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, reg); > + > +out: > + pci_cfg_access_unlock(bridge); Maybe a guard() use case to allow early returns in error paths? > + return rc; > +} > + > void pci_dev_lock(struct pci_dev *dev) > { > /* block PM suspend, driver probe, etc. */ > @@ -5066,6 +5109,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" }, > }; > > 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
On 4/3/24 8:09 AM, Jonathan Cameron wrote: > On Tue, 2 Apr 2024 16:45:31 -0700 > Dave Jiang <dave.jiang@intel.com> 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" 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> > A few trivial things inline. Otherwise looks fine. > > FWIW > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >> --- >> v3: >> - move cxl_port_dvsec() to previous patch. (Dan) >> - add pci_cfg_access_lock() for the bridge. (Dan) >> - Change cxl_bus_force method to cxl_bus. (Dan) >> --- >> drivers/pci/pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> include/linux/pci.h | 2 +- >> 2 files changed, 45 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 00eddb451102..3989c8888813 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -4982,6 +4982,49 @@ 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; > > Lukas' comment on previous applies to this as well. ok > >> + int rc; >> + u16 reg, val; > > Maybe combine lines as appropriate. ok > >> + >> + bridge = pci_upstream_bridge(dev); >> + if (!bridge) >> + return -ENOTTY; >> + >> + dvsec = cxl_port_dvsec(bridge); >> + if (!dvsec) >> + return -ENOTTY; >> + >> + if (probe) >> + return 0; >> + >> + pci_cfg_access_lock(bridge); >> + rc = pci_read_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, ®); >> + if (rc) { >> + rc = -ENOTTY; >> + goto out; >> + } >> + >> + if (!(reg & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR)) { >> + val = reg | PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR; >> + pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, >> + val); >> + } else { >> + val = reg; >> + } >> + >> + rc = pci_reset_bus_function(dev, probe); >> + >> + if (reg != val) >> + pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, reg); >> + >> +out: >> + pci_cfg_access_unlock(bridge); > > Maybe a guard() use case to allow early returns in error paths? I'm not seeing a good way to do it. pci_cfg_access_lock/unlock() isn't like your typical lock/unlock. It locks, changes some pci_dev internal stuff, and then unlocks in both functions. The pci_lock isn't being held after lock() call. > >> + return rc; >> +} >> + >> void pci_dev_lock(struct pci_dev *dev) >> { >> /* block PM suspend, driver probe, etc. */ >> @@ -5066,6 +5109,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" }, >> }; >> >> 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 >
> > > >> + > >> + bridge = pci_upstream_bridge(dev); > >> + if (!bridge) > >> + return -ENOTTY; > >> + > >> + dvsec = cxl_port_dvsec(bridge); > >> + if (!dvsec) > >> + return -ENOTTY; > >> + > >> + if (probe) > >> + return 0; > >> + > >> + pci_cfg_access_lock(bridge); > >> + rc = pci_read_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, ®); > >> + if (rc) { > >> + rc = -ENOTTY; > >> + goto out; > >> + } > >> + > >> + if (!(reg & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR)) { > >> + val = reg | PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR; > >> + pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, > >> + val); > >> + } else { > >> + val = reg; > >> + } > >> + > >> + rc = pci_reset_bus_function(dev, probe); > >> + > >> + if (reg != val) > >> + pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, reg); > >> + > >> +out: > >> + pci_cfg_access_unlock(bridge); > > > > Maybe a guard() use case to allow early returns in error paths? > > I'm not seeing a good way to do it. pci_cfg_access_lock/unlock() isn't like your typical lock/unlock. It locks, changes some pci_dev internal stuff, and then unlocks in both functions. The pci_lock isn't being held after lock() call. > You've lost me. Why does guard() care about the internals? All it does is stash a copy of the '_lock' - here the bridge struct pci_dev then call the _unlock on it when the stashed copy of that pointer when it goes out of scope. Those functions don't need to hold a conventional lock. Though in this case I believe the lock is effectively pci_dev->block_cfg_access. FWIW we do the similar in IIO (with a conditional lock for extra fun :) https://elixir.bootlin.com/linux/v6.9-rc2/source/include/linux/iio/iio.h#L650 That is setting a flag much like this one. Don't look too closely at that though as it evolved into a slightly odd form and needs a revisit. This was a possible nice to have, not something I care that much about in this patch set so feel free to not do it :) Jonathan > > > >> + return rc; > >> +} > >> + > >> void pci_dev_lock(struct pci_dev *dev) > >> { > >> /* block PM suspend, driver probe, etc. */ > >> @@ -5066,6 +5109,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" }, > >> }; > >> > >> 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 > > >
Jonathan Cameron wrote: [..] > > > Maybe a guard() use case to allow early returns in error paths? > > > > I'm not seeing a good way to do it. pci_cfg_access_lock/unlock() isn't like your typical lock/unlock. It locks, changes some pci_dev internal stuff, and then unlocks in both functions. The pci_lock isn't being held after lock() call. > > > > You've lost me. > > Why does guard() care about the internals? > > All it does is stash a copy of the '_lock' - here the bridge struct > pci_dev then call the _unlock on it when the stashed copy of that > pointer when it goes out of scope. Agree, and I suggested offlist to just use pci_dev_lock() similar to pci_reset_function(). There is already a guard() for that, and pci_dev_lock() is amenable to lockdep.
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 00eddb451102..3989c8888813 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4982,6 +4982,49 @@ 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; + + bridge = pci_upstream_bridge(dev); + if (!bridge) + return -ENOTTY; + + dvsec = cxl_port_dvsec(bridge); + if (!dvsec) + return -ENOTTY; + + if (probe) + return 0; + + pci_cfg_access_lock(bridge); + rc = pci_read_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, ®); + if (rc) { + rc = -ENOTTY; + goto out; + } + + if (!(reg & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR)) { + val = reg | PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR; + pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, + val); + } else { + val = reg; + } + + rc = pci_reset_bus_function(dev, probe); + + if (reg != val) + pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL, reg); + +out: + pci_cfg_access_unlock(bridge); + return rc; +} + void pci_dev_lock(struct pci_dev *dev) { /* block PM suspend, driver probe, etc. */ @@ -5066,6 +5109,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" }, }; 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" 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> --- v3: - move cxl_port_dvsec() to previous patch. (Dan) - add pci_cfg_access_lock() for the bridge. (Dan) - Change cxl_bus_force method to cxl_bus. (Dan) --- drivers/pci/pci.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/pci.h | 2 +- 2 files changed, 45 insertions(+), 1 deletion(-)