Messages in this thread | | | Date | Tue, 9 Jan 2024 14:44:54 +0530 | Subject | Re: [PATCH v4 6/8] PCI/link: Re-add BW notification portdrv as PCIe BW controller | From | Krishna Chaitanya Chundru <> |
| |
On 1/5/2024 4:55 PM, Ilpo Järvinen wrote: > This mostly reverts b4c7d2076b4e ("PCI/LINK: Remove bandwidth > notification") and builds PCIe bandwidth controller on top of it. > > The PCIe bandwidth notification were first added in the commit > e8303bb7a75c ("PCI/LINK: Report degraded links via link bandwidth > notification") but later had to be removed. The significant changes > compared with the old bandwidth notification driver include: > > 1) Don't print the notifications into kernel log, just keep the Link > Speed cached into the struct pci_bus updated. While somewhat > unfortunate, the log spam was the source of complaints that > eventually lead to the removal of the bandwidth notifications driver > (see the links below for further information). > > 2) Besides the Link Bandwidth Management Interrupt, enable also Link > Autonomous Bandwidth Interrupt to cover the other source of > bandwidth changes. > > 3) Use threaded IRQ with IRQF_ONESHOT to handle Bandwidth Notification > Interrupts to address the problem fixed in the commit 3e82a7f9031f > ("PCI/LINK: Supply IRQ handler so level-triggered IRQs are acked")). > > 4) Handle Link Speed updates robustly. Refresh the cached Link Speed > when enabling Bandwidth Notification Interrupts, and solve the race > between Link Speed read and LBMS/LABS update in > pcie_bandwidth_notification_irq_thread(). > > 5) Use concurrency safe LNKCTL RMW operations. > > 6) The driver is now called PCIe bwctrl (bandwidth controller) instead > of just bandwidth notifications because of increased scope and > functionality within the driver. > > PCIe bandwidth controller introduces an in-kernel API to set PCIe Link > Speed. This new API is intended to be used in an upcoming commit that > adds a thermal cooling device to throttle PCIe bandwidth when thermal > thresholds are reached. No users are introduced in this commit yet. > > The PCIe bandwidth control procedure is as follows. The highest speed > supported by the Port and the PCIe device which is not higher than the > requested speed is selected and written into the Target Link Speed in > the Link Control 2 Register. Then bandwidth controller retrains the > PCIe Link. > > Bandwidth Notifications enable the cur_bus_speed in the struct pci_bus > to keep track PCIe Link Speed changes. While Bandwidth Notifications > should also be generated when bandwidth controller alters the PCIe Link > Speed, a few platforms do not deliver LMBS interrupt after Link > Training as expected. Thus, after changing the Link Speed, bandwidth > controller makes additional read for the Link Status Register to ensure > cur_bus_speed is consistent with the new PCIe Link Speed. > > Link: https://lore.kernel.org/all/20190429185611.121751-1-helgaas@kernel.org/ > Link: https://lore.kernel.org/linux-pci/20190501142942.26972-1-keith.busch@intel.com/ > Link: https://lore.kernel.org/linux-pci/20200115221008.GA191037@google.com/ > Suggested-by: Lukas Wunner <lukas@wunner.de> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > MAINTAINERS | 7 + > drivers/pci/pcie/Kconfig | 12 ++ > drivers/pci/pcie/Makefile | 1 + > drivers/pci/pcie/bwctrl.c | 263 +++++++++++++++++++++++++++++++++++++ > drivers/pci/pcie/portdrv.c | 9 +- > drivers/pci/pcie/portdrv.h | 10 +- > include/linux/pci-bwctrl.h | 17 +++ > 7 files changed, 313 insertions(+), 6 deletions(-) > create mode 100644 drivers/pci/pcie/bwctrl.c > create mode 100644 include/linux/pci-bwctrl.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 97f51d5ec1cf..ccc5d50bf340 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16766,6 +16766,13 @@ F: include/linux/pci* > F: include/uapi/linux/pci* > F: lib/pci* > > +PCIE BANDWIDTH CONTROLLER > +M: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > +L: linux-pci@vger.kernel.org > +S: Supported > +F: drivers/pci/pcie/bwctrl.c > +F: include/linux/pci-bwctrl.h > + > PCIE DRIVER FOR AMAZON ANNAPURNA LABS > M: Jonathan Chocron <jonnyc@amazon.com> > L: linux-pci@vger.kernel.org > diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig > index 8999fcebde6a..f1ef6030d2c2 100644 > --- a/drivers/pci/pcie/Kconfig > +++ b/drivers/pci/pcie/Kconfig > @@ -146,6 +146,18 @@ config PCIE_PTM > This is only useful if you have devices that support PTM, but it > is safe to enable even if you don't. > > +config PCIE_BW > + bool "PCI Express Bandwidth Controller" > + depends on PCIEPORTBUS > + help > + This enables PCI Express Bandwidth Controller. The Bandwidth > + Controller allows controlling PCIe Link Speed and listens for Link > + Bandwidth Change Notifications. The current Link Speed is available > + through /sys/bus/pci/devices/.../current_link_speed. > + > + If you know Link Width or Speed changes occur (e.g., to correct > + unreliable links), you may answer Y. > + > config PCIE_EDR > bool "PCI Express Error Disconnect Recover support" > depends on PCIE_DPC && ACPI > diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile > index 8de4ed5f98f1..175065a495cf 100644 > --- a/drivers/pci/pcie/Makefile > +++ b/drivers/pci/pcie/Makefile > @@ -12,4 +12,5 @@ obj-$(CONFIG_PCIEAER_INJECT) += aer_inject.o > obj-$(CONFIG_PCIE_PME) += pme.o > obj-$(CONFIG_PCIE_DPC) += dpc.o > obj-$(CONFIG_PCIE_PTM) += ptm.o > +obj-$(CONFIG_PCIE_BW) += bwctrl.o > obj-$(CONFIG_PCIE_EDR) += edr.o > diff --git a/drivers/pci/pcie/bwctrl.c b/drivers/pci/pcie/bwctrl.c > new file mode 100644 > index 000000000000..e17c98c25f06 > --- /dev/null > +++ b/drivers/pci/pcie/bwctrl.c > @@ -0,0 +1,263 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * PCIe bandwidth controller > + * > + * Author: Alexandru Gagniuc <mr.nuke.me@gmail.com> > + * > + * Copyright (C) 2019 Dell Inc > + * Copyright (C) 2023 Intel Corporation > + * > + * The PCIe bandwidth controller provides a way to alter PCIe Link Speeds > + * and notify the operating system when the Link Width or Speed changes. The > + * notification capability is required for all Root Ports and Downstream > + * Ports supporting Link Width wider than x1 and/or multiple Link Speeds. > + * > + * This service port driver hooks into the Bandwidth Notification interrupt > + * watching for changes or links becoming degraded in operation. It updates > + * the cached Current Link Speed that is exposed to user space through sysfs. > + */ > + > +#define dev_fmt(fmt) "bwctrl: " fmt > + > +#include <linux/bitops.h> > +#include <linux/bits.h> > +#include <linux/device.h> > +#include <linux/errno.h> > +#include <linux/interrupt.h> > +#include <linux/mutex.h> > +#include <linux/pci.h> > +#include <linux/pci-bwctrl.h> > +#include <linux/types.h> > + > +#include "../pci.h" > +#include "portdrv.h" > + > +/** > + * struct pcie_bwctrl_service_data - PCIe bandwidth controller > + * @set_speed_mutex: serializes link speed changes > + */ > +struct pcie_bwctrl_service_data { > + struct mutex set_speed_mutex; > +}; > + > +static bool pcie_valid_speed(enum pci_bus_speed speed) > +{ > + return (speed >= PCIE_SPEED_2_5GT) && (speed <= PCIE_SPEED_64_0GT); > +} > + > +static u16 pci_bus_speed2lnkctl2(enum pci_bus_speed speed) > +{ > + static const u8 speed_conv[] = { > + [PCIE_SPEED_2_5GT] = PCI_EXP_LNKCTL2_TLS_2_5GT, > + [PCIE_SPEED_5_0GT] = PCI_EXP_LNKCTL2_TLS_5_0GT, > + [PCIE_SPEED_8_0GT] = PCI_EXP_LNKCTL2_TLS_8_0GT, > + [PCIE_SPEED_16_0GT] = PCI_EXP_LNKCTL2_TLS_16_0GT, > + [PCIE_SPEED_32_0GT] = PCI_EXP_LNKCTL2_TLS_32_0GT, > + [PCIE_SPEED_64_0GT] = PCI_EXP_LNKCTL2_TLS_64_0GT, > + }; > + > + if (WARN_ON_ONCE(!pcie_valid_speed(speed))) > + return 0; > + > + return speed_conv[speed]; > +} > + > +static inline u16 pcie_supported_speeds2target_speed(u8 supported_speeds) > +{ > + return __fls(supported_speeds); > +} > + > +static void pcie_bwnotif_enable(struct pci_dev *dev) > +{ > + u16 link_status; > + int ret; > + > + pcie_capability_set_word(dev, PCI_EXP_LNKCTL, > + PCI_EXP_LNKCTL_LBMIE | PCI_EXP_LNKCTL_LABIE); > + pcie_capability_write_word(dev, PCI_EXP_LNKSTA, > + PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_LABS); > + > + /* Read after enabling notifications to ensure link speed is up to date */ > + ret = pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &link_status); > + if (ret == PCIBIOS_SUCCESSFUL) > + pcie_update_link_speed(dev->subordinate, link_status); > +} > + > +static void pcie_bwnotif_disable(struct pci_dev *dev) > +{ > + pcie_capability_clear_word(dev, PCI_EXP_LNKCTL, > + PCI_EXP_LNKCTL_LBMIE | PCI_EXP_LNKCTL_LABIE); > +} > + > +static irqreturn_t pcie_bwnotif_irq_thread(int irq, void *context) > +{ > + struct pcie_device *srv = context; > + struct pci_dev *port = srv->port; > + u16 link_status, events; > + int ret; > + > + ret = pcie_capability_read_word(port, PCI_EXP_LNKSTA, &link_status); > + events = link_status & (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_LABS); > + > + if (ret != PCIBIOS_SUCCESSFUL || !events) > + return IRQ_NONE; > + > + pcie_capability_write_word(port, PCI_EXP_LNKSTA, events); > + > + /* > + * The write to clear LBMS prevents getting interrupt from the > + * latest link speed when the link speed changes between the above > + * LNKSTA read and write. Therefore, re-read the speed before > + * updating it. > + */ > + ret = pcie_capability_read_word(port, PCI_EXP_LNKSTA, &link_status); > + if (ret != PCIBIOS_SUCCESSFUL) > + return IRQ_HANDLED; > + pcie_update_link_speed(port->subordinate, link_status); > + > + return IRQ_HANDLED; > +} > + > +/** > + * pcie_bwctrl_select_speed - Select Target Link Speed > + * @srv: PCIe Port > + * @speed_req: requested PCIe Link Speed > + * > + * Select Target Link Speed by take into account Supported Link Speeds of > + * both the Root Port and the Endpoint. > + * > + * Return: Target Link Speed (1=2.5GT/s, 2=5GT/s, 3=8GT/s, etc.) > + */ > +static u16 pcie_bwctrl_select_speed(struct pcie_device *srv, enum pci_bus_speed speed_req) > +{ > + struct pci_bus *bus = srv->port->subordinate; > + u8 desired_speeds; > + > + if (WARN_ON_ONCE(!bus->supported_speeds)) > + return PCI_EXP_LNKCAP2_SLS_2_5GB; > + > + desired_speeds = GENMASK(pci_bus_speed2lnkctl2(speed_req), > + __fls(PCI_EXP_LNKCAP2_SLS_2_5GB)); > + > + return pcie_supported_speeds2target_speed(bus->supported_speeds & desired_speeds); > +} > + > +/** > + * pcie_bwctrl_set_current_speed - Set downstream Link Speed for PCIe Port > + * @srv: PCIe Port > + * @speed_req: requested PCIe Link Speed > + * > + * Attempts to set PCIe Port Link Speed to @speed_req. @speed_req may be > + * adjusted downwards to the best speed supported by both the Port and PCIe > + * Device underneath it. > + * > + * Return: > + * * 0 - on success > + * * -EINVAL - @speed_req is not a PCIe Link Speed > + * * -ETIMEDOUT - changing Link Speed took too long > + * * -EAGAIN - Link Speed was changed but @speed_req was not achieved > + */ > +int pcie_bwctrl_set_current_speed(struct pcie_device *srv, enum pci_bus_speed speed_req)
Hi Ilpo Järvinen,
we want to use this API from PCIe client driver, but can't use API as client driver is not aware of pcie_device structure,
can you please make changes in this API so that PCIe client drivers can also use it. And also can you please export this API.
Thanks & Regards,
Krishna Chaitanya.
> +{ > + struct pcie_bwctrl_service_data *data = get_service_data(srv); > + struct pci_dev *port = srv->port; > + u16 target_speed, link_status; > + int ret; > + > + if (WARN_ON_ONCE(!pcie_valid_speed(speed_req))) > + return -EINVAL; > + > + if (port->subordinate->cur_bus_speed == speed_req) > + return 0; > + > + target_speed = pcie_bwctrl_select_speed(srv, speed_req); > + > + mutex_lock(&data->set_speed_mutex); > + ret = pcie_capability_clear_and_set_word(port, PCI_EXP_LNKCTL2, > + PCI_EXP_LNKCTL2_TLS, target_speed); > + if (ret != PCIBIOS_SUCCESSFUL) { > + ret = pcibios_err_to_errno(ret); > + goto unlock; > + } > + > + ret = pcie_retrain_link(port, true); > + if (ret < 0) > + goto unlock; > + > + /* > + * Ensure link speed updates also with platforms that have problems > + * with notifications > + */ > + ret = pcie_capability_read_word(port, PCI_EXP_LNKSTA, &link_status); > + if (ret == PCIBIOS_SUCCESSFUL) > + pcie_update_link_speed(port->subordinate, link_status); > + > + if (port->subordinate->cur_bus_speed != speed_req) > + ret = -EAGAIN; > + > +unlock: > + mutex_unlock(&data->set_speed_mutex); > + > + return ret; > +} > + > +static int pcie_bwnotif_probe(struct pcie_device *srv) > +{ > + struct pcie_bwctrl_service_data *data; > + struct pci_dev *port = srv->port; > + int ret; > + > + data = devm_kzalloc(&srv->device, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + mutex_init(&data->set_speed_mutex); > + set_service_data(srv, data); > + > + ret = devm_request_threaded_irq(&srv->device, srv->irq, NULL, > + pcie_bwnotif_irq_thread, > + IRQF_SHARED | IRQF_ONESHOT, > + "PCIe BW ctrl", srv); > + if (ret) > + return ret; > + > + pcie_bwnotif_enable(port); > + pci_info(port, "enabled with IRQ %d\n", srv->irq); > + > + return 0; > +} > + > +static void pcie_bwnotif_remove(struct pcie_device *srv) > +{ > + struct pcie_bwctrl_service_data *data = get_service_data(srv); > + > + pcie_bwnotif_disable(srv->port); > + mutex_destroy(&data->set_speed_mutex); > +} > + > +static int pcie_bwnotif_suspend(struct pcie_device *srv) > +{ > + pcie_bwnotif_disable(srv->port); > + return 0; > +} > + > +static int pcie_bwnotif_resume(struct pcie_device *srv) > +{ > + pcie_bwnotif_enable(srv->port); > + return 0; > +} > + > +static struct pcie_port_service_driver pcie_bwctrl_driver = { > + .name = "pcie_bwctrl", > + .port_type = PCIE_ANY_PORT, > + .service = PCIE_PORT_SERVICE_BWCTRL, > + .probe = pcie_bwnotif_probe, > + .suspend = pcie_bwnotif_suspend, > + .resume = pcie_bwnotif_resume, > + .remove = pcie_bwnotif_remove, > +}; > + > +int __init pcie_bwctrl_init(void) > +{ > + return pcie_port_service_register(&pcie_bwctrl_driver); > +} > diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c > index 14a4b89a3b83..e8a348949d70 100644 > --- a/drivers/pci/pcie/portdrv.c > +++ b/drivers/pci/pcie/portdrv.c > @@ -68,7 +68,7 @@ static int pcie_message_numbers(struct pci_dev *dev, int mask, > */ > > if (mask & (PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP | > - PCIE_PORT_SERVICE_BWNOTIF)) { > + PCIE_PORT_SERVICE_BWCTRL)) { > pcie_capability_read_word(dev, PCI_EXP_FLAGS, ®16); > *pme = FIELD_GET(PCI_EXP_FLAGS_IRQ, reg16); > nvec = *pme + 1; > @@ -150,11 +150,11 @@ static int pcie_port_enable_irq_vec(struct pci_dev *dev, int *irqs, int mask) > > /* PME, hotplug and bandwidth notification share an MSI/MSI-X vector */ > if (mask & (PCIE_PORT_SERVICE_PME | PCIE_PORT_SERVICE_HP | > - PCIE_PORT_SERVICE_BWNOTIF)) { > + PCIE_PORT_SERVICE_BWCTRL)) { > pcie_irq = pci_irq_vector(dev, pme); > irqs[PCIE_PORT_SERVICE_PME_SHIFT] = pcie_irq; > irqs[PCIE_PORT_SERVICE_HP_SHIFT] = pcie_irq; > - irqs[PCIE_PORT_SERVICE_BWNOTIF_SHIFT] = pcie_irq; > + irqs[PCIE_PORT_SERVICE_BWCTRL_SHIFT] = pcie_irq; > } > > if (mask & PCIE_PORT_SERVICE_AER) > @@ -271,7 +271,7 @@ static int get_port_device_capability(struct pci_dev *dev) > > pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &linkcap); > if (linkcap & PCI_EXP_LNKCAP_LBNC) > - services |= PCIE_PORT_SERVICE_BWNOTIF; > + services |= PCIE_PORT_SERVICE_BWCTRL; > } > > return services; > @@ -829,6 +829,7 @@ static void __init pcie_init_services(void) > pcie_pme_init(); > pcie_dpc_init(); > pcie_hp_init(); > + pcie_bwctrl_init(); > } > > static int __init pcie_portdrv_init(void) > diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h > index 1f3803bde7ee..9628bb04a8ff 100644 > --- a/drivers/pci/pcie/portdrv.h > +++ b/drivers/pci/pcie/portdrv.h > @@ -20,8 +20,8 @@ > #define PCIE_PORT_SERVICE_HP (1 << PCIE_PORT_SERVICE_HP_SHIFT) > #define PCIE_PORT_SERVICE_DPC_SHIFT 3 /* Downstream Port Containment */ > #define PCIE_PORT_SERVICE_DPC (1 << PCIE_PORT_SERVICE_DPC_SHIFT) > -#define PCIE_PORT_SERVICE_BWNOTIF_SHIFT 4 /* Bandwidth notification */ > -#define PCIE_PORT_SERVICE_BWNOTIF (1 << PCIE_PORT_SERVICE_BWNOTIF_SHIFT) > +#define PCIE_PORT_SERVICE_BWCTRL_SHIFT 4 /* Bandwidth Controller (notifications) */ > +#define PCIE_PORT_SERVICE_BWCTRL (1 << PCIE_PORT_SERVICE_BWCTRL_SHIFT) > > #define PCIE_PORT_DEVICE_MAXSERVICES 5 > > @@ -51,6 +51,12 @@ int pcie_dpc_init(void); > static inline int pcie_dpc_init(void) { return 0; } > #endif > > +#ifdef CONFIG_PCIE_BW > +int pcie_bwctrl_init(void); > +#else > +static inline int pcie_bwctrl_init(void) { return 0; } > +#endif > + > /* Port Type */ > #define PCIE_ANY_PORT (~0) > > diff --git a/include/linux/pci-bwctrl.h b/include/linux/pci-bwctrl.h > new file mode 100644 > index 000000000000..be51f6f2b340 > --- /dev/null > +++ b/include/linux/pci-bwctrl.h > @@ -0,0 +1,17 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * PCIe bandwidth controller > + * > + * Copyright (C) 2023 Intel Corporation > + */ > + > +#ifndef LINUX_PCI_BWCTRL_H > +#define LINUX_PCI_BWCTRL_H > + > +#include <linux/pci.h> > + > +struct pcie_device; > + > +int pcie_bwctrl_set_current_speed(struct pcie_device *srv, enum pci_bus_speed speed_req); > + > +#endif
| |