On Wed, May 17, 2023 at 12:53 PM Ilpo Järvinen
<ilpo.jarvinen@xxxxxxxxxxxxxxx> wrote:
>
> Many places in the kernel write the Link Control and Root Control PCI
> Express Capability Registers without proper concurrency control and
> this could result in losing the changes one of the writers intended to
> make.
>
> Add pcie_cap_lock spinlock into the struct pci_dev and use it to
> protect bit changes made in the RMW capability accessors. Protect only
> a selected set of registers by differentiating the RMW accessor
> internally to locked/unlocked variants using a wrapper which has the
> same signature as pcie_capability_clear_and_set_word(). As the
> Capability Register (pos) given to the wrapper is always a constant,
> the compiler should be able to simplify all the dead-code away.
>
> The RMW locking is only added to pcie_capability_clear_and_set_word()
> because so far only the Link Control Register (ASPM, hotplug, various
> drivers) and the Root Control Register (AER & PME) require RMW locking.
>
> Fixes: c7f486567c1d ("PCI PM: PCIe PME root port service driver")
> Fixes: f12eb72a268b ("PCI/ASPM: Use PCI Express Capability accessors")
> Fixes: 7d715a6c1ae5 ("PCI: add PCI Express ASPM support")
> Fixes: affa48de8417 ("staging/rdma/hfi1: Add support for enabling/disabling PCIe ASPM")
> Fixes: 849a9366cba9 ("misc: rtsx: Add support new chip rts5228 mmc: rtsx: Add support MMC_CAP2_NO_MMC")
> Fixes: 3d1e7aa80d1c ("misc: rtsx: Use pcie_capability_clear_and_set_word() for PCI_EXP_LNKCTL")
> Fixes: c0e5f4e73a71 ("misc: rtsx: Add support for RTS5261")
> Fixes: 3df4fce739e2 ("misc: rtsx: separate aspm mode into MODE_REG and MODE_CFG")
> Fixes: 121e9c6b5c4c ("misc: rtsx: modify and fix init_hw function")
> Fixes: 19f3bd548f27 ("mfd: rtsx: Remove LCTLR defination")
> Fixes: 773ccdfd9cc6 ("mfd: rtsx: Read vendor setting from config space")
> Fixes: 8275b77a1513 ("mfd: rts5249: Add support for RTS5250S power saving")
> Fixes: 5da4e04ae480 ("misc: rtsx: Add support for RTS5260")
> Fixes: 0f49bfbd0f2e ("tg3: Use PCI Express Capability accessors")
> Fixes: 5e7dfd0fb94a ("tg3: Prevent corruption at 10 / 100Mbps w CLKREQ")
> Fixes: b726e493e8dc ("r8169: sync existing 8168 device hardware start sequences with vendor driver")
> Fixes: e6de30d63eb1 ("r8169: more 8168dp support.")
> Fixes: 8a06127602de ("Bluetooth: hci_bcm4377: Add new driver for BCM4377 PCIe boards")
> Fixes: 6f461f6c7c96 ("e1000e: enable/disable ASPM L0s and L1 and ERT according to hardware errata")
> Fixes: 1eae4eb2a1c7 ("e1000e: Disable L1 ASPM power savings for 82573 mobile variants")
> Fixes: 8060e169e02f ("ath9k: Enable extended synch for AR9485 to fix L0s recovery issue")
> Fixes: 69ce674bfa69 ("ath9k: do btcoex ASPM disabling at initialization time")
> Fixes: f37f05503575 ("mt76: mt76x2e: disable pcie_aspm by default")
> Suggested-by: Lukas Wunner <lukas@xxxxxxxxx>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
No objections, so
Reviewed-by: Rafael J. Wysocki <rafael@xxxxxxxxxx>
> ---
> drivers/pci/access.c | 20 +++++++++++++++++---
> drivers/pci/probe.c | 1 +
> include/linux/pci.h | 34 ++++++++++++++++++++++++++++++++--
> 3 files changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index 3c230ca3de58..0b2e90d2f04f 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
> @@ -497,8 +497,8 @@ int pcie_capability_write_dword(struct pci_dev *dev, int pos, u32 val)
> }
> EXPORT_SYMBOL(pcie_capability_write_dword);
>
> -int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
> - u16 clear, u16 set)
> +int pcie_capability_clear_and_set_word_unlocked(struct pci_dev *dev, int pos,
> + u16 clear, u16 set)
> {
> int ret;
> u16 val;
> @@ -512,7 +512,21 @@ int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
>
> return ret;
> }
> -EXPORT_SYMBOL(pcie_capability_clear_and_set_word);
> +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_unlocked);
> +
> +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> + u16 clear, u16 set)
> +{
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&dev->pcie_cap_lock, flags);
> + ret = pcie_capability_clear_and_set_word_unlocked(dev, pos, clear, set);
> + spin_unlock_irqrestore(&dev->pcie_cap_lock, flags);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(pcie_capability_clear_and_set_word_locked);
>
> int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
> u32 clear, u32 set)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 0b2826c4a832..53ac0d3287a8 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2318,6 +2318,7 @@ struct pci_dev *pci_alloc_dev(struct pci_bus *bus)
> .end = -1,
> };
>
> + spin_lock_init(&dev->pcie_cap_lock);
> #ifdef CONFIG_PCI_MSI
> raw_spin_lock_init(&dev->msi_lock);
> #endif
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 60b8772b5bd4..ab7682ed172f 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -467,6 +467,7 @@ struct pci_dev {
> pci_dev_flags_t dev_flags;
> atomic_t enable_cnt; /* pci_enable_device has been called */
>
> + spinlock_t pcie_cap_lock; /* Protects RMW ops in capability accessors */
> u32 saved_config_space[16]; /* Config space saved at suspend time */
> struct hlist_head saved_cap_space;
> int rom_attr_enabled; /* Display of ROM attribute enabled? */
> @@ -1217,11 +1218,40 @@ int pcie_capability_read_word(struct pci_dev *dev, int pos, u16 *val);
> int pcie_capability_read_dword(struct pci_dev *dev, int pos, u32 *val);
> int pcie_capability_write_word(struct pci_dev *dev, int pos, u16 val);
> int pcie_capability_write_dword(struct pci_dev *dev, int pos, u32 val);
> -int pcie_capability_clear_and_set_word(struct pci_dev *dev, int pos,
> - u16 clear, u16 set);
> +int pcie_capability_clear_and_set_word_unlocked(struct pci_dev *dev, int pos,
> + u16 clear, u16 set);
> +int pcie_capability_clear_and_set_word_locked(struct pci_dev *dev, int pos,
> + u16 clear, u16 set);
> int pcie_capability_clear_and_set_dword(struct pci_dev *dev, int pos,
> u32 clear, u32 set);
>
> +/**
> + * pcie_capability_clear_and_set_word - RMW accessor for PCI Express Capability Registers
> + * @dev: PCI device structure of the PCI Express device
> + * @pos: PCI Express Capability Register
> + * @clear: Clear bitmask
> + * @set: Set bitmask
> + *
> + * Perform a Read-Modify-Write (RMW) operation using @clear and @set
> + * bitmasks on PCI Express Capability Register at @pos. Certain PCI Express
> + * Capability Registers are accessed concurrently in RMW fashion, hence
> + * require locking which is handled transparently to the caller.
> + */
> +static inline int pcie_capability_clear_and_set_word(struct pci_dev *dev,
> + int pos,
> + u16 clear, u16 set)
> +{
> + switch (pos) {
> + case PCI_EXP_LNKCTL:
> + case PCI_EXP_RTCTL:
> + return pcie_capability_clear_and_set_word_locked(dev, pos,
> + clear, set);
> + default:
> + return pcie_capability_clear_and_set_word_unlocked(dev, pos,
> + clear, set);
> + }
> +}
> +
> static inline int pcie_capability_set_word(struct pci_dev *dev, int pos,
> u16 set)
> {
> --
> 2.30.2
>
