Bjorn Helgaas wrote: > 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. Oh, true, I think this should be PCI_DVSEC_VENDOR_ID_CXL, because afaics it is still possible that 0x1e98 be used as a non-DVSEC vendor-id in some future device. In other words I think the CXL specification usage of 0x1e98 is scoped as "DVSEC Vendor ID", not "Vendor ID". However that would mean that a future 0x1e98 device could not publish DVSECs without colliding with CXL DVSECs. I note this footnote in section 3.1.2 about the 0x1e98 value (all caps emphasis is from the spec, not me): --- NOTICE TO USERS: THE UNIQUE VALUE THAT IS PROVIDED IN THIS CXL SPECIFICATION IS FOR USE IN VENDOR DEFINED MESSAGE FIELDS, DESIGNATED VENDOR SPECIFIC EXTENDED CAPABILITIES, AND ALTERNATE PROTOCOL NEGOTIATION ONLY...
