On Wed, Mar 27, 2024 at 04:57:40PM -0700, Dave Jiang wrote: > On 3/27/24 2:26 PM, Bjorn Helgaas wrote: > > On Mon, Mar 25, 2024 at 04:58:01PM -0700, Dave Jiang wrote: > >> With the current behavior, the bus_reset would appear to have executed > >> successfully, however the operation is actually masked if the "Unmask > >> SBR" bit is set with the default value. The intention is to inform the > >> user that SBR for the CXL device is masked and will not go through. > > > > The default value of Unmask SBR isn't really relevant here. > > Changing to: > 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. > > > > >> The expectation is that if a user overrides the "Unmask SBR" via a > >> user tool such as setpci then they can trigger a bus reset successfully. > > > > ... if a user *sets* Unmask SBR ... > > to be more specific about what value is required. > > > > If a user overrides the "Unmask SBR" via a user tool such as setpci and > sets the value to 1, then the bus reset will execute successfully. I'm not super enamored with the "if a user overrides" language because a driver may change that bit in the future, and then the suggestion of a "user" will be misleading. It doesn't matter *how* it gets set to 1; it only matters that SBR doesn't work when "Unmask SBR" is 0 and it does work when "Unmask SBR" is 1. > >> +/* Compute Express Link (CXL) */ > >> +#define PCI_DVSEC_VENDOR_ID_CXL 0x1e98 > > > > I see that you're just moving this #define from drivers/cxl/cxlpci.h, > > but I'm scratching my head a bit. As used here, this is to match the > > DVSEC Vendor ID (PCIe r6.0, sec 7.9.6.2). > > > > IIUC, this should be just a PCI SIG-defined "Vendor ID", e.g., > > "PCI_VENDOR_ID_CXL", that doesn't need the "DVSEC" qualifier in the > > name, and would normally be defined in include/linux/pci_ids.h. > > > > But I don't see CXL in pci_ids.h, and I don't see either "CXL" or the > > value "1e98" in the PCI SIG list at > > https://pcisig.com/membership/member-companies. > > > I'll create a new patch and move to include/linux/pci_ids.h first > for this define and change to PCI_VENDOR_ID_CXL. The value is > defined in CXL spec r3.1 sec 8.1.1. I saw the CXL mentions of 0x1e98, but IMO that's not an authoritative source; no vendor is allowed to just squat on a Vendor ID value simply by mentioning it in their own internal specs. That would obviously lead to madness. The footnote in CXL r3.1, sec 3.1.2, about how the 1E98h value is only for use in DVSEC etc., is really weird. IIUC, the PCI SIG controls the Vendor ID namespace, so I'm still really confused about why it is not reserved there. I emailed the PCI SIG, but the footnote suggests to me that there some kind of history here that I don't know. Anyway, I think all we can do here is to put the definition in include/linux/pci_ids.h as you did and hope 0x1e98 is never allocated to another vendor. > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h > index a0c75e467df3..7dfbf6d96b3d 100644 > --- a/include/linux/pci_ids.h > +++ b/include/linux/pci_ids.h > @@ -2607,6 +2607,8 @@ > > #define PCI_VENDOR_ID_ALIBABA 0x1ded > > +#define PCI_VENDOR_ID_CXL 0x1e98 > + > #define PCI_VENDOR_ID_TEHUTI 0x1fc9 > #define PCI_DEVICE_ID_TEHUTI_3009 0x3009 > #define PCI_DEVICE_ID_TEHUTI_3010 0x3010
