| Message ID | 20240402234848.3287160-3-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, Apr 02, 2024 at 04:45:30PM -0700, Dave Jiang wrote: > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4927,10 +4927,55 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe) > return pci_reset_hotplug_slot(dev->slot->hotplug, probe); > } > > +static int cxl_port_dvsec(struct pci_dev *dev) > +{ > + return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL, > + PCI_DVSEC_CXL_PORT); > +} Hm, seems a bit odd that this returns an int even though pci_find_dvsec_capability() returns a u16 and all the callers of cxl_port_dvsec() seem to assign the return value to a u16 as well. Is the "int" on purpose? Thanks, Lukas
On Tue, 2 Apr 2024 16:45:30 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > Per CXL spec r3.1 8.1.5.2, Secondary Bus Reset (SBR) is masked unless the > "Unmask SBR" bit is set. Add a check to the PCI secondary bus reset > path to fail the CXL SBR request if the "Unmask SBR" bit is clear in > the CXL Port Control Extensions register by returning -ENOTTY. > > When the "Unmask SBR" bit is set to 0 (default), the bus_reset would > appear to have executed successfully. However the operation is actually > masked. The intention is to inform the user that SBR for the CXL device > is masked and will not go through. > > If the "Unmask SBR" bit is set to 1, then the bus reset will execute > successfully. > > Link: https://lore.kernel.org/linux-cxl/20240220203956.GA1502351@bhelgaas/ > Signed-off-by: Dave Jiang <dave.jiang@intel.com> LGTM though Lukas' comment make sense so I'll assume you'll tidy that up. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > v3: > - Move and rename PCI_DVSEC_VENDOR_ID_CXL to PCI_VENDOR_ID_CXL. > Move to pci_ids.h in a different patch. (Bjorn) > - Remove of CXL device checking. (Bjorn) > - Rename defines to PCI_DVSEC_CXL_PORT_*. (Bjorn) > - Fixup SBR define in commit log. (Bjorn) > - Update comment on dvsec not found. (Dan) > - Check return of dvsec value read for error. (Dan) > --- > drivers/pci/pci.c | 45 +++++++++++++++++++++++++++++++++++ > include/uapi/linux/pci_regs.h | 5 ++++ > 2 files changed, 50 insertions(+) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index e5f243dd4288..00eddb451102 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -4927,10 +4927,55 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe) > return pci_reset_hotplug_slot(dev->slot->hotplug, probe); > } > > +static int cxl_port_dvsec(struct pci_dev *dev) > +{ > + return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL, > + PCI_DVSEC_CXL_PORT); > +} > + > +static bool cxl_sbr_masked(struct pci_dev *dev) > +{ > + u16 dvsec, reg; > + int rc; > + > + /* > + * No DVSEC found, either is not a CXL port, or not connected in which > + * case mask state is a nop (CXL r3.1 sec 9.12.3 "Enumerating CXL RPs > + * and DSPs" > + */ > + dvsec = cxl_port_dvsec(dev); > + if (!dvsec) > + return false; > + > + rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_PORT_CTL, ®); > + if (rc || PCI_POSSIBLE_ERROR(reg)) > + return false; > + > + /* > + * CXL spec r3.1 8.1.5.2 > + * When 0, SBR bit in Bridge Control register of this Port has no effect. > + * When 1, the Port shall generate hot reset when SBR bit in Bridge > + * Control gets set to 1. > + */ > + if (reg & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR) > + return false; > + > + return true; > +} > + > static int pci_reset_bus_function(struct pci_dev *dev, bool probe) > { > + struct pci_dev *bridge = pci_upstream_bridge(dev); > int rc; > > + /* If it's a CXL port and the SBR control is masked, fail the SBR */ > + if (bridge && cxl_sbr_masked(bridge)) { > + if (probe) > + return 0; > + > + return -ENOTTY; > + } > + > rc = pci_dev_reset_slot_function(dev, probe); > if (rc != -ENOTTY) > return rc; > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h > index a39193213ff2..d61fa43662e3 100644 > --- a/include/uapi/linux/pci_regs.h > +++ b/include/uapi/linux/pci_regs.h > @@ -1148,4 +1148,9 @@ > #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL 0x00ff0000 > #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX 0xff000000 > > +/* Compute Express Link (CXL) */ > +#define PCI_DVSEC_CXL_PORT 3 > +#define PCI_DVSEC_CXL_PORT_CTL 0x0c > +#define PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR 0x00000001 > + > #endif /* LINUX_PCI_REGS_H */
On 4/3/24 1:26 AM, Lukas Wunner wrote: > On Tue, Apr 02, 2024 at 04:45:30PM -0700, Dave Jiang wrote: >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -4927,10 +4927,55 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe) >> return pci_reset_hotplug_slot(dev->slot->hotplug, probe); >> } >> >> +static int cxl_port_dvsec(struct pci_dev *dev) >> +{ >> + return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL, >> + PCI_DVSEC_CXL_PORT); >> +} > > Hm, seems a bit odd that this returns an int even though > pci_find_dvsec_capability() returns a u16 and all the callers > of cxl_port_dvsec() seem to assign the return value to a u16 > as well. Is the "int" on purpose? Should be u16. Oversight. Thanks. > > Thanks, > > Lukas
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index e5f243dd4288..00eddb451102 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -4927,10 +4927,55 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, bool probe) return pci_reset_hotplug_slot(dev->slot->hotplug, probe); } +static int cxl_port_dvsec(struct pci_dev *dev) +{ + return pci_find_dvsec_capability(dev, PCI_VENDOR_ID_CXL, + PCI_DVSEC_CXL_PORT); +} + +static bool cxl_sbr_masked(struct pci_dev *dev) +{ + u16 dvsec, reg; + int rc; + + /* + * No DVSEC found, either is not a CXL port, or not connected in which + * case mask state is a nop (CXL r3.1 sec 9.12.3 "Enumerating CXL RPs + * and DSPs" + */ + dvsec = cxl_port_dvsec(dev); + if (!dvsec) + return false; + + rc = pci_read_config_word(dev, dvsec + PCI_DVSEC_CXL_PORT_CTL, ®); + if (rc || PCI_POSSIBLE_ERROR(reg)) + return false; + + /* + * CXL spec r3.1 8.1.5.2 + * When 0, SBR bit in Bridge Control register of this Port has no effect. + * When 1, the Port shall generate hot reset when SBR bit in Bridge + * Control gets set to 1. + */ + if (reg & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR) + return false; + + return true; +} + static int pci_reset_bus_function(struct pci_dev *dev, bool probe) { + struct pci_dev *bridge = pci_upstream_bridge(dev); int rc; + /* If it's a CXL port and the SBR control is masked, fail the SBR */ + if (bridge && cxl_sbr_masked(bridge)) { + if (probe) + return 0; + + return -ENOTTY; + } + rc = pci_dev_reset_slot_function(dev, probe); if (rc != -ENOTTY) return rc; diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h index a39193213ff2..d61fa43662e3 100644 --- a/include/uapi/linux/pci_regs.h +++ b/include/uapi/linux/pci_regs.h @@ -1148,4 +1148,9 @@ #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_PROTOCOL 0x00ff0000 #define PCI_DOE_DATA_OBJECT_DISC_RSP_3_NEXT_INDEX 0xff000000 +/* Compute Express Link (CXL) */ +#define PCI_DVSEC_CXL_PORT 3 +#define PCI_DVSEC_CXL_PORT_CTL 0x0c +#define PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR 0x00000001 + #endif /* LINUX_PCI_REGS_H */
Per CXL spec r3.1 8.1.5.2, Secondary Bus Reset (SBR) is masked unless the "Unmask SBR" bit is set. Add a check to the PCI secondary bus reset path to fail the CXL SBR request if the "Unmask SBR" bit is clear in the CXL Port Control Extensions register by returning -ENOTTY. When the "Unmask SBR" bit is set to 0 (default), the bus_reset would appear to have executed successfully. However the operation is actually masked. The intention is to inform the user that SBR for the CXL device is masked and will not go through. If the "Unmask SBR" bit is set to 1, then the bus reset will execute successfully. Link: https://lore.kernel.org/linux-cxl/20240220203956.GA1502351@bhelgaas/ Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- v3: - Move and rename PCI_DVSEC_VENDOR_ID_CXL to PCI_VENDOR_ID_CXL. Move to pci_ids.h in a different patch. (Bjorn) - Remove of CXL device checking. (Bjorn) - Rename defines to PCI_DVSEC_CXL_PORT_*. (Bjorn) - Fixup SBR define in commit log. (Bjorn) - Update comment on dvsec not found. (Dan) - Check return of dvsec value read for error. (Dan) --- drivers/pci/pci.c | 45 +++++++++++++++++++++++++++++++++++ include/uapi/linux/pci_regs.h | 5 ++++ 2 files changed, 50 insertions(+)