Hello Bjorn, List, Sometimes we come across PCI devices that have an errata that require special handling when accessing that device's configuration space. (E.g. https://github.com/rajatxjain/public_shared/blob/main/OZ711LV2_appnote.pdf): - Some registers may only support access of a particular size. - Some registers may have a bad value, so need to return some other good value. - etc. So I have been thinking about implementing a mechanism that allows a PCI quirk for a device (VID/DID/ etc), such that all the PCI configuration accesses to that device goes through that quirk (thus giving it a chance to do any special handling). This includes access from kernel, userspace and drivers. AFAIU, a reasonable central point where all the PCI configuration accesses go through are: pci_bus->ops->read() pci_bus->ops->write() thus making the macros pci_bus_read_config_##size() pci_user_read_config_##size() good candidates for tapping configuration accesses. Here is a proposal, I'd appreciate it if you could please let me know what you think about this. I've tried this to be as less intrusive and as efficient as possible. BASIC IDEA: (1) Provide macros that follow existing fixup macros: #define DECLARE_PCI_FIXUP_CONFIG_READ(...) .... #define DECLARE_PCI_FIXUP_CONFIG_WRITE(...) .... These macros shall allow to place hooks for particular VIDs/DIDs in special sections (.pci_fixup_config_read, .pci_fixup_config_write) (2) Add the following data structures to the struct pci_bus, all of them initialized with 0/NULL: /* * Bitmap with 1 bit for each devfn on this pci_bus. * Each bit, if set, means that config accesses to that * devfn needs to go through a quirk */ u32 bitmap_quirk; /* * Array of 32 function pointers (config space quirk functions), * 1 for each devfn possible on this pci_bus. * ("quirk_function_ptr" is a typedef here) * / quirk_function_ptr config_read_quirk[32]; quirk_function_ptr config_write_quirk[32]; (3) When the config access is made, in the following macros: pci_bus_read_config_##size pci_bus_write_config_##size pci_user_read_config_##size pci_user_write_config_##size Check and quirk if we need to: if (bus->bitmap_quirk & (1 << devfn)) { /* Does this devfn needs config quirk? */ bus->config_read_quirk[devfn](...) } else { bus->ops->read(...) } (4) Somewhere during the pci_dev addition time (perhaps in pci_setup_device()): Do something like: pci_attach_config_fixup(pci_fixup_config_read, dev); pci_attach_config_fixup(pci_fixup_config_write, dev); - These functions shall be somewhat similar to pci_do_fixups() in matching DID/VID etc and if there is a match: - Set the corresponding bit and function pointers in the parent pci_bus. (5) Somewhere during the pci_dev removal time (perhaps in pci_device_add() or pci_setup_device()): Do something like: pci_attach_config_fixup(pci_fixup_config_read, dev); pci_attach_config_fixup(pci_fixup_config_write, dev); - These functions shall be somewhat similar to pci_do_fixups() in matching DID/VID etc and if there is a match: - Set the corresponding bit in the bitmap and function pointer in the parent pci_bus. NOTES: a. Since the device quirk, shall be called while holding the pci_lock, it shall need to be fast (perhaps not allowed to do anything else rather than dev->bus->ops->read() and perhaps massage values before returning it). b. Note that in step (4) and (5), we're changing the pci_bus fields when we add or remove pci_dev. I think that should be OK to do without a lock because we can choose to do it at a place where we are holding the pci_bus_sem. c. I think I might have to use a lock to protect the pci_bus new fields anyway but I think it should be fast. Anyway, I think we can think about the locking as a next step once I get some feedback. This is something I came up with this morning, so of course there might be gaps or things I didn't think of. Please let me know your comments and / or thoughts. Thanks & Best Regards, Rajat
