[v3] PCI: hotplug: Add OCTEON PCI hotplug controller driver

Commit Message

Shijith Thotton Aug. 26, 2024, 10:45 a.m. UTC

This patch introduces a PCI hotplug controller driver for the OCTEON
PCIe device, a multi-function PCIe device where the first function acts
as a hotplug controller. It is equipped with MSI-x interrupts to notify
the host of hotplug events from the OCTEON firmware.

The driver facilitates the hotplugging of non-controller functions
within the same device. During probe, non-controller functions are
removed and registered as PCI hotplug slots. The slots are added back
only upon request from the device firmware. The driver also allows the
enabling and disabling of the slots via sysfs slot entries, provided by
the PCI hotplug framework.

Signed-off-by: Shijith Thotton <[e-mail]>
Co-developed-by: Vamsi Attunuru <[e-mail]>
Signed-off-by: Vamsi Attunuru <[e-mail]>
---

This patch introduces a PCI hotplug controller driver for OCTEON PCIe hotplug
controller. The OCTEON PCIe device is a multi-function device where the first
function acts as a PCI hotplug controller.

              +--------------------------------+
              |           Root Port            |
              +--------------------------------+
                              |
                             PCIe
                              |
+---------------------------------------------------------------+
|              OCTEON PCIe Multifunction Device                 |
+---------------------------------------------------------------+
            |                    |              |            |
            |                    |              |            |
+---------------------+  +----------------+  +-----+  +----------------+
|      Function 0     |  |   Function 1   |  | ... |  |   Function 7   |
| (Hotplug controller)|  | (Hotplug slot) |  |     |  | (Hotplug slot) |
+---------------------+  +----------------+  +-----+  +----------------+
            |
            |
+-------------------------+
|   Controller Firmware   |
+-------------------------+

The hotplug controller driver facilitates the hotplugging of non-controller
functions in the same device. During the probe of the driver, the non-controller
function are removed and registered as PCI hotplug slots. They are added back
only upon request from the device firmware. The driver also allows the user to
enable/disable the functions using sysfs slot entries provided by PCI hotplug
framework.

This solution adopts a hardware + software approach for several reasons:

1. To reduce hardware implementation cost. Supporting complete hotplug
   capability within the card would require a PCI switch implemented within.

2. In the multi-function device, non-controller functions can act as emulated
   devices. The firmware can dynamically enable or disable them at runtime.

3. Not all root ports support PCI hotplug. This approach provides greater
   flexibility and compatibility across different hardware configurations.

The hotplug controller function is lightweight and is equipped with MSI-x
interrupts to notify the host about hotplug events. Upon receiving an
interrupt, the hotplug register is read, and the required function is enabled
or disabled.

This driver will be beneficial for managing PCI hotplug events on the OCTEON
PCIe device without requiring complex hardware solutions.

Changes in v2:
- Added missing include files.
- Used dev_err_probe() for error handling.
- Used guard() for mutex locking.
- Splited cleanup actions and added per-slot cleanup action.
- Fixed coding style issues.
- Added co-developed-by tag.

Changes in v3:
- Explicit assignment of enum values.
- Use pcim_iomap_region() instead of pcim_iomap_regions().

 MAINTAINERS                    |   6 +
 drivers/pci/hotplug/Kconfig    |  10 +
 drivers/pci/hotplug/Makefile   |   1 +
 drivers/pci/hotplug/octep_hp.c | 409 +++++++++++++++++++++++++++++++++
 4 files changed, 426 insertions(+)
 create mode 100644 drivers/pci/hotplug/octep_hp.c

Comments

Shijith Thotton Sept. 16, 2024, 4:43 a.m. UTC | #1

Hi Helgaas,

Please check this patch.

>This patch introduces a PCI hotplug controller driver for the OCTEON
>PCIe device, a multi-function PCIe device where the first function acts
>as a hotplug controller. It is equipped with MSI-x interrupts to notify
>the host of hotplug events from the OCTEON firmware.
>
>The driver facilitates the hotplugging of non-controller functions
>within the same device. During probe, non-controller functions are
>removed and registered as PCI hotplug slots. The slots are added back
>only upon request from the device firmware. The driver also allows the
>enabling and disabling of the slots via sysfs slot entries, provided by
>the PCI hotplug framework.
>
>Signed-off-by: Shijith Thotton <[e-mail]>
>Co-developed-by: Vamsi Attunuru <[e-mail]>
>Signed-off-by: Vamsi Attunuru <[e-mail]>
>---
>
>This patch introduces a PCI hotplug controller driver for OCTEON PCIe hotplug
>controller. The OCTEON PCIe device is a multi-function device where the first
>function acts as a PCI hotplug controller.
>
>              +--------------------------------+
>              |           Root Port            |
>              +--------------------------------+
>                              |
>                             PCIe
>                              |
>+---------------------------------------------------------------+
>|              OCTEON PCIe Multifunction Device                 |
>+---------------------------------------------------------------+
>            |                    |              |            |
>            |                    |              |            |
>+---------------------+  +----------------+  +-----+  +----------------+
>|      Function 0     |  |   Function 1   |  | ... |  |   Function 7   |
>| (Hotplug controller)|  | (Hotplug slot) |  |     |  | (Hotplug slot) |
>+---------------------+  +----------------+  +-----+  +----------------+
>            |
>            |
>+-------------------------+
>|   Controller Firmware   |
>+-------------------------+
>
>The hotplug controller driver facilitates the hotplugging of non-controller
>functions in the same device. During the probe of the driver, the non-controller
>function are removed and registered as PCI hotplug slots. They are added back
>only upon request from the device firmware. The driver also allows the user to
>enable/disable the functions using sysfs slot entries provided by PCI hotplug
>framework.
>
>This solution adopts a hardware + software approach for several reasons:
>
>1. To reduce hardware implementation cost. Supporting complete hotplug
>   capability within the card would require a PCI switch implemented within.
>
>2. In the multi-function device, non-controller functions can act as emulated
>   devices. The firmware can dynamically enable or disable them at runtime.
>
>3. Not all root ports support PCI hotplug. This approach provides greater
>   flexibility and compatibility across different hardware configurations.
>
>The hotplug controller function is lightweight and is equipped with MSI-x
>interrupts to notify the host about hotplug events. Upon receiving an
>interrupt, the hotplug register is read, and the required function is enabled
>or disabled.
>
>This driver will be beneficial for managing PCI hotplug events on the OCTEON
>PCIe device without requiring complex hardware solutions.
>
>Changes in v2:
>- Added missing include files.
>- Used dev_err_probe() for error handling.
>- Used guard() for mutex locking.
>- Splited cleanup actions and added per-slot cleanup action.
>- Fixed coding style issues.
>- Added co-developed-by tag.
>
>Changes in v3:
>- Explicit assignment of enum values.
>- Use pcim_iomap_region() instead of pcim_iomap_regions().
>
> MAINTAINERS                    |   6 +
> drivers/pci/hotplug/Kconfig    |  10 +
> drivers/pci/hotplug/Makefile   |   1 +
> drivers/pci/hotplug/octep_hp.c | 409
>+++++++++++++++++++++++++++++++++
> 4 files changed, 426 insertions(+)
> create mode 100644 drivers/pci/hotplug/octep_hp.c
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index 42decde38320..7b5a618eed1c 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -13677,6 +13677,12 @@ R:	[e-mail]
> R:	[e-mail]
> F:	drivers/vdpa/octeon_ep/
>
>+MARVELL OCTEON HOTPLUG CONTROLLER DRIVER
>+R:	Shijith Thotton <[e-mail]>
>+R:	Vamsi Attunuru <[e-mail]>
>+S:	Supported
>+F:	drivers/pci/hotplug/octep_hp.c
>+
> MATROX FRAMEBUFFER DRIVER
> L:	[e-mail]
> S:	Orphan
>diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
>index 1472aef0fb81..2e38fd25f7ef 100644
>--- a/drivers/pci/hotplug/Kconfig
>+++ b/drivers/pci/hotplug/Kconfig
>@@ -173,4 +173,14 @@ config HOTPLUG_PCI_S390
>
> 	  When in doubt, say Y.
>
>+config HOTPLUG_PCI_OCTEONEP
>+	bool "OCTEON PCI device Hotplug controller driver"
>+	depends on HOTPLUG_PCI
>+	help
>+	  Say Y here if you have an OCTEON PCIe device with a hotplug
>+	  controller. This driver enables the non-controller functions of the
>+	  device to be registered as hotplug slots.
>+
>+	  When in doubt, say N.
>+
> endif # HOTPLUG_PCI
>diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile
>index 240c99517d5e..40aaf31fe338 100644
>--- a/drivers/pci/hotplug/Makefile
>+++ b/drivers/pci/hotplug/Makefile
>@@ -20,6 +20,7 @@ obj-$(CONFIG_HOTPLUG_PCI_RPA)		+=
>rpaphp.o
> obj-$(CONFIG_HOTPLUG_PCI_RPA_DLPAR)	+= rpadlpar_io.o
> obj-$(CONFIG_HOTPLUG_PCI_ACPI)		+= acpiphp.o
> obj-$(CONFIG_HOTPLUG_PCI_S390)		+= s390_pci_hpc.o
>+obj-$(CONFIG_HOTPLUG_PCI_OCTEONEP)	+= octep_hp.o
>
> # acpiphp_ibm extends acpiphp, so should be linked afterwards.
>
>diff --git a/drivers/pci/hotplug/octep_hp.c b/drivers/pci/hotplug/octep_hp.c
>new file mode 100644
>index 000000000000..77dc2740f43e
>--- /dev/null
>+++ b/drivers/pci/hotplug/octep_hp.c
>@@ -0,0 +1,409 @@
>+// SPDX-License-Identifier: GPL-2.0-only
>+/* Copyright (C) 2024 Marvell. */
>+
>+#include <linux/cleanup.h>
>+#include <linux/container_of.h>
>+#include <linux/delay.h>
>+#include <linux/dev_printk.h>
>+#include <linux/init.h>
>+#include <linux/interrupt.h>
>+#include <linux/io-64-nonatomic-lo-hi.h>
>+#include <linux/kernel.h>
>+#include <linux/list.h>
>+#include <linux/module.h>
>+#include <linux/mutex.h>
>+#include <linux/pci.h>
>+#include <linux/pci_hotplug.h>
>+#include <linux/slab.h>
>+#include <linux/spinlock.h>
>+#include <linux/workqueue.h>
>+
>+#define OCTEP_HP_INTR_OFFSET(x) (0x20400 + ((x) << 4))
>+#define OCTEP_HP_INTR_VECTOR(x) (16 + (x))
>+#define OCTEP_HP_DRV_NAME "octep_hp"
>+
>+/*
>+ * Type of MSI-X interrupts.
>+ * The macros OCTEP_HP_INTR_VECTOR and OCTEP_HP_INTR_OFFSET are
>used to
>+ * generate the vector and offset for an interrupt type.
>+ */
>+enum octep_hp_intr_type {
>+	OCTEP_HP_INTR_INVALID = -1,
>+	OCTEP_HP_INTR_ENA = 0,
>+	OCTEP_HP_INTR_DIS = 1,
>+	OCTEP_HP_INTR_MAX = 2,
>+};
>+
>+struct octep_hp_cmd {
>+	struct list_head list;
>+	enum octep_hp_intr_type intr_type;
>+	u64 intr_val;
>+};
>+
>+struct octep_hp_slot {
>+	struct list_head list;
>+	struct hotplug_slot slot;
>+	u16 slot_number;
>+	struct pci_dev *hp_pdev;
>+	unsigned int hp_devfn;
>+	struct octep_hp_controller *ctrl;
>+};
>+
>+struct octep_hp_intr_info {
>+	enum octep_hp_intr_type type;
>+	int number;
>+	char name[16];
>+};
>+
>+struct octep_hp_controller {
>+	void __iomem *base;
>+	struct pci_dev *pdev;
>+	struct octep_hp_intr_info intr[OCTEP_HP_INTR_MAX];
>+	struct work_struct work;
>+	struct list_head slot_list;
>+	struct mutex slot_lock; /* Protects slot_list */
>+	struct list_head hp_cmd_list;
>+	spinlock_t hp_cmd_lock; /* Protects hp_cmd_list */
>+};
>+
>+static void octep_hp_enable_pdev(struct octep_hp_controller *hp_ctrl,
>+				 struct octep_hp_slot *hp_slot)
>+{
>+	guard(mutex)(&hp_ctrl->slot_lock);
>+	if (hp_slot->hp_pdev) {
>+		dev_dbg(&hp_slot->hp_pdev->dev, "Slot %u already
>enabled\n",
>+			hp_slot->slot_number);
>+		return;
>+	}
>+
>+	/* Scan the device and add it to the bus */
>+	hp_slot->hp_pdev = pci_scan_single_device(hp_ctrl->pdev->bus,
>+						  hp_slot->hp_devfn);
>+	pci_bus_assign_resources(hp_ctrl->pdev->bus);
>+	pci_bus_add_device(hp_slot->hp_pdev);
>+
>+	dev_dbg(&hp_slot->hp_pdev->dev, "Enabled slot %u\n",
>+		hp_slot->slot_number);
>+}
>+
>+static void octep_hp_disable_pdev(struct octep_hp_controller *hp_ctrl,
>+				  struct octep_hp_slot *hp_slot)
>+{
>+	guard(mutex)(&hp_ctrl->slot_lock);
>+	if (!hp_slot->hp_pdev) {
>+		dev_dbg(&hp_ctrl->pdev->dev, "Slot %u already disabled\n",
>+			hp_slot->slot_number);
>+		return;
>+	}
>+
>+	dev_dbg(&hp_slot->hp_pdev->dev, "Disabling slot %u\n",
>+		hp_slot->slot_number);
>+
>+	/* Remove the device from the bus */
>+	pci_stop_and_remove_bus_device_locked(hp_slot->hp_pdev);
>+	hp_slot->hp_pdev = NULL;
>+}
>+
>+static int octep_hp_enable_slot(struct hotplug_slot *slot)
>+{
>+	struct octep_hp_slot *hp_slot =
>+		container_of(slot, struct octep_hp_slot, slot);
>+
>+	octep_hp_enable_pdev(hp_slot->ctrl, hp_slot);
>+	return 0;
>+}
>+
>+static int octep_hp_disable_slot(struct hotplug_slot *slot)
>+{
>+	struct octep_hp_slot *hp_slot =
>+		container_of(slot, struct octep_hp_slot, slot);
>+
>+	octep_hp_disable_pdev(hp_slot->ctrl, hp_slot);
>+	return 0;
>+}
>+
>+static struct hotplug_slot_ops octep_hp_slot_ops = {
>+	.enable_slot = octep_hp_enable_slot,
>+	.disable_slot = octep_hp_disable_slot,
>+};
>+
>+#define SLOT_NAME_SIZE 16
>+static struct octep_hp_slot *
>+octep_hp_register_slot(struct octep_hp_controller *hp_ctrl,
>+		       struct pci_dev *pdev, u16 slot_number)
>+{
>+	char slot_name[SLOT_NAME_SIZE];
>+	struct octep_hp_slot *hp_slot;
>+	int ret;
>+
>+	hp_slot = kzalloc(sizeof(*hp_slot), GFP_KERNEL);
>+	if (!hp_slot)
>+		return ERR_PTR(-ENOMEM);
>+
>+	hp_slot->ctrl = hp_ctrl;
>+	hp_slot->hp_pdev = pdev;
>+	hp_slot->hp_devfn = pdev->devfn;
>+	hp_slot->slot_number = slot_number;
>+	hp_slot->slot.ops = &octep_hp_slot_ops;
>+
>+	snprintf(slot_name, sizeof(slot_name), "octep_hp_%u", slot_number);
>+	ret = pci_hp_register(&hp_slot->slot, hp_ctrl->pdev->bus,
>+			      PCI_SLOT(pdev->devfn), slot_name);
>+	if (ret) {
>+		kfree(hp_slot);
>+		return ERR_PTR(ret);
>+	}
>+
>+	list_add_tail(&hp_slot->list, &hp_ctrl->slot_list);
>+	octep_hp_disable_pdev(hp_ctrl, hp_slot);
>+
>+	return hp_slot;
>+}
>+
>+static void octep_hp_deregister_slot(void *data)
>+{
>+	struct octep_hp_slot *hp_slot = data;
>+	struct octep_hp_controller *hp_ctrl = hp_slot->ctrl;
>+
>+	pci_hp_deregister(&hp_slot->slot);
>+	octep_hp_enable_pdev(hp_ctrl, hp_slot);
>+	list_del(&hp_slot->list);
>+	kfree(hp_slot);
>+}
>+
>+static bool octep_hp_is_slot(struct octep_hp_controller *hp_ctrl,
>+			     struct pci_dev *pdev)
>+{
>+	/* Check if the PCI device can be hotplugged */
>+	return pdev != hp_ctrl->pdev && pdev->bus == hp_ctrl->pdev->bus &&
>+	       PCI_SLOT(pdev->devfn) == PCI_SLOT(hp_ctrl->pdev->devfn);
>+}
>+
>+static void octep_hp_cmd_handler(struct octep_hp_controller *hp_ctrl,
>+				 struct octep_hp_cmd *hp_cmd)
>+{
>+	struct octep_hp_slot *hp_slot;
>+
>+	/*
>+	 * Enable or disable the slots based on the slot mask.
>+	 * intr_val is a bit mask where each bit represents a slot.
>+	 */
>+	list_for_each_entry(hp_slot, &hp_ctrl->slot_list, list) {
>+		if (!(hp_cmd->intr_val & BIT(hp_slot->slot_number)))
>+			continue;
>+
>+		if (hp_cmd->intr_type == OCTEP_HP_INTR_ENA)
>+			octep_hp_enable_pdev(hp_ctrl, hp_slot);
>+		else
>+			octep_hp_disable_pdev(hp_ctrl, hp_slot);
>+	}
>+}
>+
>+static void octep_hp_work_handler(struct work_struct *work)
>+{
>+	struct octep_hp_controller *hp_ctrl;
>+	struct octep_hp_cmd *hp_cmd;
>+	unsigned long flags;
>+
>+	hp_ctrl = container_of(work, struct octep_hp_controller, work);
>+
>+	/* Process all the hotplug commands */
>+	spin_lock_irqsave(&hp_ctrl->hp_cmd_lock, flags);
>+	while (!list_empty(&hp_ctrl->hp_cmd_list)) {
>+		hp_cmd = list_first_entry(&hp_ctrl->hp_cmd_list,
>+					  struct octep_hp_cmd, list);
>+		list_del(&hp_cmd->list);
>+		spin_unlock_irqrestore(&hp_ctrl->hp_cmd_lock, flags);
>+
>+		octep_hp_cmd_handler(hp_ctrl, hp_cmd);
>+		kfree(hp_cmd);
>+
>+		spin_lock_irqsave(&hp_ctrl->hp_cmd_lock, flags);
>+	}
>+	spin_unlock_irqrestore(&hp_ctrl->hp_cmd_lock, flags);
>+}
>+
>+static enum octep_hp_intr_type octep_hp_intr_type(struct
>octep_hp_intr_info *intr,
>+						  int irq)
>+{
>+	enum octep_hp_intr_type type;
>+
>+	for (type = OCTEP_HP_INTR_ENA; type < OCTEP_HP_INTR_MAX;
>type++) {
>+		if (intr[type].number == irq)
>+			return type;
>+	}
>+
>+	return OCTEP_HP_INTR_INVALID;
>+}
>+
>+static irqreturn_t octep_hp_intr_handler(int irq, void *data)
>+{
>+	struct octep_hp_controller *hp_ctrl = data;
>+	struct pci_dev *pdev = hp_ctrl->pdev;
>+	enum octep_hp_intr_type type;
>+	struct octep_hp_cmd *hp_cmd;
>+	u64 intr_val;
>+
>+	type = octep_hp_intr_type(hp_ctrl->intr, irq);
>+	if (type == OCTEP_HP_INTR_INVALID) {
>+		dev_err(&pdev->dev, "Invalid interrupt %d\n", irq);
>+		return IRQ_HANDLED;
>+	}
>+
>+	/* Read and clear the interrupt */
>+	intr_val = readq(hp_ctrl->base + OCTEP_HP_INTR_OFFSET(type));
>+	writeq(intr_val, hp_ctrl->base + OCTEP_HP_INTR_OFFSET(type));
>+
>+	hp_cmd = kzalloc(sizeof(*hp_cmd), GFP_ATOMIC);
>+	if (!hp_cmd)
>+		return IRQ_HANDLED;
>+
>+	hp_cmd->intr_val = intr_val;
>+	hp_cmd->intr_type = type;
>+
>+	/* Add the command to the list and schedule the work */
>+	spin_lock(&hp_ctrl->hp_cmd_lock);
>+	list_add_tail(&hp_cmd->list, &hp_ctrl->hp_cmd_list);
>+	spin_unlock(&hp_ctrl->hp_cmd_lock);
>+	schedule_work(&hp_ctrl->work);
>+
>+	return IRQ_HANDLED;
>+}
>+
>+static void octep_hp_irq_cleanup(void *data)
>+{
>+	struct octep_hp_controller *hp_ctrl = data;
>+
>+	pci_free_irq_vectors(hp_ctrl->pdev);
>+	flush_work(&hp_ctrl->work);
>+}
>+
>+static int octep_hp_request_irq(struct octep_hp_controller *hp_ctrl,
>+				enum octep_hp_intr_type type)
>+{
>+	struct pci_dev *pdev = hp_ctrl->pdev;
>+	struct octep_hp_intr_info *intr;
>+	int irq;
>+
>+	irq = pci_irq_vector(pdev, OCTEP_HP_INTR_VECTOR(type));
>+	if (irq < 0)
>+		return irq;
>+
>+	intr = &hp_ctrl->intr[type];
>+	intr->number = irq;
>+	intr->type = type;
>+	snprintf(intr->name, sizeof(intr->name), "octep_hp_%d", type);
>+
>+	return devm_request_irq(&pdev->dev, irq, octep_hp_intr_handler,
>+				IRQF_SHARED, intr->name, hp_ctrl);
>+}
>+
>+static int octep_hp_controller_setup(struct pci_dev *pdev,
>+				     struct octep_hp_controller *hp_ctrl)
>+{
>+	struct device *dev = &pdev->dev;
>+	enum octep_hp_intr_type type;
>+	int ret;
>+
>+	ret = pcim_enable_device(pdev);
>+	if (ret)
>+		return dev_err_probe(dev, ret, "Failed to enable PCI device\n");
>+
>+	hp_ctrl->base = pcim_iomap_region(pdev, 0, OCTEP_HP_DRV_NAME);
>+	if (IS_ERR(hp_ctrl->base))
>+		return dev_err_probe(dev, PTR_ERR(hp_ctrl->base),
>+				     "Failed to map PCI device region\n");
>+
>+	pci_set_master(pdev);
>+	pci_set_drvdata(pdev, hp_ctrl);
>+
>+	INIT_LIST_HEAD(&hp_ctrl->slot_list);
>+	INIT_LIST_HEAD(&hp_ctrl->hp_cmd_list);
>+	mutex_init(&hp_ctrl->slot_lock);
>+	spin_lock_init(&hp_ctrl->hp_cmd_lock);
>+	INIT_WORK(&hp_ctrl->work, octep_hp_work_handler);
>+	hp_ctrl->pdev = pdev;
>+
>+	ret = pci_alloc_irq_vectors(pdev, 1,
>+
>OCTEP_HP_INTR_VECTOR(OCTEP_HP_INTR_MAX),
>+				    PCI_IRQ_MSIX);
>+	if (ret < 0)
>+		return dev_err_probe(dev, ret, "Failed to alloc MSI-X
>vectors\n");
>+
>+	ret = devm_add_action(&pdev->dev, octep_hp_irq_cleanup, hp_ctrl);
>+	if (ret)
>+		return dev_err_probe(&pdev->dev, ret, "Failed to add IRQ
>cleanup action\n");
>+
>+	for (type = OCTEP_HP_INTR_ENA; type < OCTEP_HP_INTR_MAX;
>type++) {
>+		ret = octep_hp_request_irq(hp_ctrl, type);
>+		if (ret)
>+			return dev_err_probe(dev, ret,
>+					     "Failed to request IRQ for vector
>%d\n",
>+					     OCTEP_HP_INTR_VECTOR(type));
>+	}
>+
>+	return 0;
>+}
>+
>+static int octep_hp_pci_probe(struct pci_dev *pdev,
>+			      const struct pci_device_id *id)
>+{
>+	struct octep_hp_controller *hp_ctrl;
>+	struct pci_dev *tmp_pdev = NULL;
>+	struct octep_hp_slot *hp_slot;
>+	u16 slot_number = 0;
>+	int ret;
>+
>+	hp_ctrl = devm_kzalloc(&pdev->dev, sizeof(*hp_ctrl), GFP_KERNEL);
>+	if (!hp_ctrl)
>+		return -ENOMEM;
>+
>+	ret = octep_hp_controller_setup(pdev, hp_ctrl);
>+	if (ret)
>+		return ret;
>+
>+	/*
>+	 * Register all hotplug slots. Hotplug controller is the first function
>+	 * of the PCI device. The hotplug slots are the remaining functions of
>+	 * the PCI device. They are removed from the bus and are added back
>when
>+	 * the hotplug event is triggered.
>+	 */
>+	for_each_pci_dev(tmp_pdev) {
>+		if (!octep_hp_is_slot(hp_ctrl, tmp_pdev))
>+			continue;
>+
>+		hp_slot = octep_hp_register_slot(hp_ctrl, tmp_pdev,
>slot_number);
>+		if (IS_ERR(hp_slot))
>+			return dev_err_probe(&pdev->dev, PTR_ERR(hp_slot),
>+					     "Failed to register hotplug slot
>%u\n",
>+					     slot_number);
>+
>+		ret = devm_add_action(&pdev->dev, octep_hp_deregister_slot,
>+				      hp_slot);
>+		if (ret)
>+			return dev_err_probe(&pdev->dev, ret,
>+					     "Failed to add action for
>deregistering slot %u\n",
>+					     slot_number);
>+		slot_number++;
>+	}
>+
>+	return 0;
>+}
>+
>+#define OCTEP_DEVID_HP_CONTROLLER 0xa0e3
>+static struct pci_device_id octep_hp_pci_map[] = {
>+	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM,
>OCTEP_DEVID_HP_CONTROLLER) },
>+	{ },
>+};
>+
>+static struct pci_driver octep_hp = {
>+	.name = OCTEP_HP_DRV_NAME,
>+	.id_table = octep_hp_pci_map,
>+	.probe = octep_hp_pci_probe,
>+};
>+
>+module_pci_driver(octep_hp);
>+
>+MODULE_LICENSE("GPL");
>+MODULE_AUTHOR("Marvell");
>+MODULE_DESCRIPTION("OCTEON PCIe device hotplug controller driver");
>--
>2.25.1

Thanks,
Shijith

Shijith Thotton Sept. 26, 2024, 1:01 p.m. UTC | #2

Just a gentle reminder.
Could you please comments if there are improvements needed?

>Hi Helgaas,
>
>Please check this patch.
>
>>This patch introduces a PCI hotplug controller driver for the OCTEON
>>PCIe device, a multi-function PCIe device where the first function acts
>>as a hotplug controller. It is equipped with MSI-x interrupts to notify
>>the host of hotplug events from the OCTEON firmware.
>>
>>The driver facilitates the hotplugging of non-controller functions
>>within the same device. During probe, non-controller functions are
>>removed and registered as PCI hotplug slots. The slots are added back
>>only upon request from the device firmware. The driver also allows the
>>enabling and disabling of the slots via sysfs slot entries, provided by
>>the PCI hotplug framework.
>>
>>Signed-off-by: Shijith Thotton <[e-mail]>
>>Co-developed-by: Vamsi Attunuru <[e-mail]>
>>Signed-off-by: Vamsi Attunuru <[e-mail]>
>>---
>>
>>This patch introduces a PCI hotplug controller driver for OCTEON PCIe hotplug
>>controller. The OCTEON PCIe device is a multi-function device where the first
>>function acts as a PCI hotplug controller.
>>
>>              +--------------------------------+
>>              |           Root Port            |
>>              +--------------------------------+
>>                              |
>>                             PCIe
>>                              |
>>+---------------------------------------------------------------+
>>|              OCTEON PCIe Multifunction Device                 |
>>+---------------------------------------------------------------+
>>            |                    |              |            |
>>            |                    |              |            |
>>+---------------------+  +----------------+  +-----+  +----------------+
>>|      Function 0     |  |   Function 1   |  | ... |  |   Function 7   |
>>| (Hotplug controller)|  | (Hotplug slot) |  |     |  | (Hotplug slot) |
>>+---------------------+  +----------------+  +-----+  +----------------+
>>            |
>>            |
>>+-------------------------+
>>|   Controller Firmware   |
>>+-------------------------+
>>
>>The hotplug controller driver facilitates the hotplugging of non-controller
>>functions in the same device. During the probe of the driver, the non-
>controller
>>function are removed and registered as PCI hotplug slots. They are added back
>>only upon request from the device firmware. The driver also allows the user to
>>enable/disable the functions using sysfs slot entries provided by PCI hotplug
>>framework.
>>
>>This solution adopts a hardware + software approach for several reasons:
>>
>>1. To reduce hardware implementation cost. Supporting complete hotplug
>>   capability within the card would require a PCI switch implemented within.
>>
>>2. In the multi-function device, non-controller functions can act as emulated
>>   devices. The firmware can dynamically enable or disable them at runtime.
>>
>>3. Not all root ports support PCI hotplug. This approach provides greater
>>   flexibility and compatibility across different hardware configurations.
>>
>>The hotplug controller function is lightweight and is equipped with MSI-x
>>interrupts to notify the host about hotplug events. Upon receiving an
>>interrupt, the hotplug register is read, and the required function is enabled
>>or disabled.
>>
>>This driver will be beneficial for managing PCI hotplug events on the OCTEON
>>PCIe device without requiring complex hardware solutions.
>>
>>Changes in v2:
>>- Added missing include files.
>>- Used dev_err_probe() for error handling.
>>- Used guard() for mutex locking.
>>- Splited cleanup actions and added per-slot cleanup action.
>>- Fixed coding style issues.
>>- Added co-developed-by tag.
>>
>>Changes in v3:
>>- Explicit assignment of enum values.
>>- Use pcim_iomap_region() instead of pcim_iomap_regions().
>>
>> MAINTAINERS                    |   6 +
>> drivers/pci/hotplug/Kconfig    |  10 +
>> drivers/pci/hotplug/Makefile   |   1 +
>> drivers/pci/hotplug/octep_hp.c | 409
>>+++++++++++++++++++++++++++++++++
>> 4 files changed, 426 insertions(+)
>> create mode 100644 drivers/pci/hotplug/octep_hp.c
>>
>>diff --git a/MAINTAINERS b/MAINTAINERS
>>index 42decde38320..7b5a618eed1c 100644
>>--- a/MAINTAINERS
>>+++ b/MAINTAINERS
>>@@ -13677,6 +13677,12 @@ R:	[e-mail]
>> R:	[e-mail]
>> F:	drivers/vdpa/octeon_ep/
>>
>>+MARVELL OCTEON HOTPLUG CONTROLLER DRIVER
>>+R:	Shijith Thotton <[e-mail]>
>>+R:	Vamsi Attunuru <[e-mail]>
>>+S:	Supported
>>+F:	drivers/pci/hotplug/octep_hp.c
>>+
>> MATROX FRAMEBUFFER DRIVER
>> L:	[e-mail]
>> S:	Orphan
>>diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
>>index 1472aef0fb81..2e38fd25f7ef 100644
>>--- a/drivers/pci/hotplug/Kconfig
>>+++ b/drivers/pci/hotplug/Kconfig
>>@@ -173,4 +173,14 @@ config HOTPLUG_PCI_S390
>>
>> 	  When in doubt, say Y.
>>
>>+config HOTPLUG_PCI_OCTEONEP
>>+	bool "OCTEON PCI device Hotplug controller driver"
>>+	depends on HOTPLUG_PCI
>>+	help
>>+	  Say Y here if you have an OCTEON PCIe device with a hotplug
>>+	  controller. This driver enables the non-controller functions of the
>>+	  device to be registered as hotplug slots.
>>+
>>+	  When in doubt, say N.
>>+
>> endif # HOTPLUG_PCI
>>diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile
>>index 240c99517d5e..40aaf31fe338 100644
>>--- a/drivers/pci/hotplug/Makefile
>>+++ b/drivers/pci/hotplug/Makefile
>>@@ -20,6 +20,7 @@ obj-$(CONFIG_HOTPLUG_PCI_RPA)		+=
>>rpaphp.o
>> obj-$(CONFIG_HOTPLUG_PCI_RPA_DLPAR)	+= rpadlpar_io.o
>> obj-$(CONFIG_HOTPLUG_PCI_ACPI)		+= acpiphp.o
>> obj-$(CONFIG_HOTPLUG_PCI_S390)		+= s390_pci_hpc.o
>>+obj-$(CONFIG_HOTPLUG_PCI_OCTEONEP)	+= octep_hp.o
>>
>> # acpiphp_ibm extends acpiphp, so should be linked afterwards.
>>
>>diff --git a/drivers/pci/hotplug/octep_hp.c b/drivers/pci/hotplug/octep_hp.c
>>new file mode 100644
>>index 000000000000..77dc2740f43e
>>--- /dev/null
>>+++ b/drivers/pci/hotplug/octep_hp.c
>>@@ -0,0 +1,409 @@
>>+// SPDX-License-Identifier: GPL-2.0-only
>>+/* Copyright (C) 2024 Marvell. */
>>+
>>+#include <linux/cleanup.h>
>>+#include <linux/container_of.h>
>>+#include <linux/delay.h>
>>+#include <linux/dev_printk.h>
>>+#include <linux/init.h>
>>+#include <linux/interrupt.h>
>>+#include <linux/io-64-nonatomic-lo-hi.h>
>>+#include <linux/kernel.h>
>>+#include <linux/list.h>
>>+#include <linux/module.h>
>>+#include <linux/mutex.h>
>>+#include <linux/pci.h>
>>+#include <linux/pci_hotplug.h>
>>+#include <linux/slab.h>
>>+#include <linux/spinlock.h>
>>+#include <linux/workqueue.h>
>>+
>>+#define OCTEP_HP_INTR_OFFSET(x) (0x20400 + ((x) << 4))
>>+#define OCTEP_HP_INTR_VECTOR(x) (16 + (x))
>>+#define OCTEP_HP_DRV_NAME "octep_hp"
>>+
>>+/*
>>+ * Type of MSI-X interrupts.
>>+ * The macros OCTEP_HP_INTR_VECTOR and OCTEP_HP_INTR_OFFSET are
>>used to
>>+ * generate the vector and offset for an interrupt type.
>>+ */
>>+enum octep_hp_intr_type {
>>+	OCTEP_HP_INTR_INVALID = -1,
>>+	OCTEP_HP_INTR_ENA = 0,
>>+	OCTEP_HP_INTR_DIS = 1,
>>+	OCTEP_HP_INTR_MAX = 2,
>>+};
>>+
>>+struct octep_hp_cmd {
>>+	struct list_head list;
>>+	enum octep_hp_intr_type intr_type;
>>+	u64 intr_val;
>>+};
>>+
>>+struct octep_hp_slot {
>>+	struct list_head list;
>>+	struct hotplug_slot slot;
>>+	u16 slot_number;
>>+	struct pci_dev *hp_pdev;
>>+	unsigned int hp_devfn;
>>+	struct octep_hp_controller *ctrl;
>>+};
>>+
>>+struct octep_hp_intr_info {
>>+	enum octep_hp_intr_type type;
>>+	int number;
>>+	char name[16];
>>+};
>>+
>>+struct octep_hp_controller {
>>+	void __iomem *base;
>>+	struct pci_dev *pdev;
>>+	struct octep_hp_intr_info intr[OCTEP_HP_INTR_MAX];
>>+	struct work_struct work;
>>+	struct list_head slot_list;
>>+	struct mutex slot_lock; /* Protects slot_list */
>>+	struct list_head hp_cmd_list;
>>+	spinlock_t hp_cmd_lock; /* Protects hp_cmd_list */
>>+};
>>+
>>+static void octep_hp_enable_pdev(struct octep_hp_controller *hp_ctrl,
>>+				 struct octep_hp_slot *hp_slot)
>>+{
>>+	guard(mutex)(&hp_ctrl->slot_lock);
>>+	if (hp_slot->hp_pdev) {
>>+		dev_dbg(&hp_slot->hp_pdev->dev, "Slot %u already
>>enabled\n",
>>+			hp_slot->slot_number);
>>+		return;
>>+	}
>>+
>>+	/* Scan the device and add it to the bus */
>>+	hp_slot->hp_pdev = pci_scan_single_device(hp_ctrl->pdev->bus,
>>+						  hp_slot->hp_devfn);
>>+	pci_bus_assign_resources(hp_ctrl->pdev->bus);
>>+	pci_bus_add_device(hp_slot->hp_pdev);
>>+
>>+	dev_dbg(&hp_slot->hp_pdev->dev, "Enabled slot %u\n",
>>+		hp_slot->slot_number);
>>+}
>>+
>>+static void octep_hp_disable_pdev(struct octep_hp_controller *hp_ctrl,
>>+				  struct octep_hp_slot *hp_slot)
>>+{
>>+	guard(mutex)(&hp_ctrl->slot_lock);
>>+	if (!hp_slot->hp_pdev) {
>>+		dev_dbg(&hp_ctrl->pdev->dev, "Slot %u already disabled\n",
>>+			hp_slot->slot_number);
>>+		return;
>>+	}
>>+
>>+	dev_dbg(&hp_slot->hp_pdev->dev, "Disabling slot %u\n",
>>+		hp_slot->slot_number);
>>+
>>+	/* Remove the device from the bus */
>>+	pci_stop_and_remove_bus_device_locked(hp_slot->hp_pdev);
>>+	hp_slot->hp_pdev = NULL;
>>+}
>>+
>>+static int octep_hp_enable_slot(struct hotplug_slot *slot)
>>+{
>>+	struct octep_hp_slot *hp_slot =
>>+		container_of(slot, struct octep_hp_slot, slot);
>>+
>>+	octep_hp_enable_pdev(hp_slot->ctrl, hp_slot);
>>+	return 0;
>>+}
>>+
>>+static int octep_hp_disable_slot(struct hotplug_slot *slot)
>>+{
>>+	struct octep_hp_slot *hp_slot =
>>+		container_of(slot, struct octep_hp_slot, slot);
>>+
>>+	octep_hp_disable_pdev(hp_slot->ctrl, hp_slot);
>>+	return 0;
>>+}
>>+
>>+static struct hotplug_slot_ops octep_hp_slot_ops = {
>>+	.enable_slot = octep_hp_enable_slot,
>>+	.disable_slot = octep_hp_disable_slot,
>>+};
>>+
>>+#define SLOT_NAME_SIZE 16
>>+static struct octep_hp_slot *
>>+octep_hp_register_slot(struct octep_hp_controller *hp_ctrl,
>>+		       struct pci_dev *pdev, u16 slot_number)
>>+{
>>+	char slot_name[SLOT_NAME_SIZE];
>>+	struct octep_hp_slot *hp_slot;
>>+	int ret;
>>+
>>+	hp_slot = kzalloc(sizeof(*hp_slot), GFP_KERNEL);
>>+	if (!hp_slot)
>>+		return ERR_PTR(-ENOMEM);
>>+
>>+	hp_slot->ctrl = hp_ctrl;
>>+	hp_slot->hp_pdev = pdev;
>>+	hp_slot->hp_devfn = pdev->devfn;
>>+	hp_slot->slot_number = slot_number;
>>+	hp_slot->slot.ops = &octep_hp_slot_ops;
>>+
>>+	snprintf(slot_name, sizeof(slot_name), "octep_hp_%u", slot_number);
>>+	ret = pci_hp_register(&hp_slot->slot, hp_ctrl->pdev->bus,
>>+			      PCI_SLOT(pdev->devfn), slot_name);
>>+	if (ret) {
>>+		kfree(hp_slot);
>>+		return ERR_PTR(ret);
>>+	}
>>+
>>+	list_add_tail(&hp_slot->list, &hp_ctrl->slot_list);
>>+	octep_hp_disable_pdev(hp_ctrl, hp_slot);
>>+
>>+	return hp_slot;
>>+}
>>+
>>+static void octep_hp_deregister_slot(void *data)
>>+{
>>+	struct octep_hp_slot *hp_slot = data;
>>+	struct octep_hp_controller *hp_ctrl = hp_slot->ctrl;
>>+
>>+	pci_hp_deregister(&hp_slot->slot);
>>+	octep_hp_enable_pdev(hp_ctrl, hp_slot);
>>+	list_del(&hp_slot->list);
>>+	kfree(hp_slot);
>>+}
>>+
>>+static bool octep_hp_is_slot(struct octep_hp_controller *hp_ctrl,
>>+			     struct pci_dev *pdev)
>>+{
>>+	/* Check if the PCI device can be hotplugged */
>>+	return pdev != hp_ctrl->pdev && pdev->bus == hp_ctrl->pdev->bus &&
>>+	       PCI_SLOT(pdev->devfn) == PCI_SLOT(hp_ctrl->pdev->devfn);
>>+}
>>+
>>+static void octep_hp_cmd_handler(struct octep_hp_controller *hp_ctrl,
>>+				 struct octep_hp_cmd *hp_cmd)
>>+{
>>+	struct octep_hp_slot *hp_slot;
>>+
>>+	/*
>>+	 * Enable or disable the slots based on the slot mask.
>>+	 * intr_val is a bit mask where each bit represents a slot.
>>+	 */
>>+	list_for_each_entry(hp_slot, &hp_ctrl->slot_list, list) {
>>+		if (!(hp_cmd->intr_val & BIT(hp_slot->slot_number)))
>>+			continue;
>>+
>>+		if (hp_cmd->intr_type == OCTEP_HP_INTR_ENA)
>>+			octep_hp_enable_pdev(hp_ctrl, hp_slot);
>>+		else
>>+			octep_hp_disable_pdev(hp_ctrl, hp_slot);
>>+	}
>>+}
>>+
>>+static void octep_hp_work_handler(struct work_struct *work)
>>+{
>>+	struct octep_hp_controller *hp_ctrl;
>>+	struct octep_hp_cmd *hp_cmd;
>>+	unsigned long flags;
>>+
>>+	hp_ctrl = container_of(work, struct octep_hp_controller, work);
>>+
>>+	/* Process all the hotplug commands */
>>+	spin_lock_irqsave(&hp_ctrl->hp_cmd_lock, flags);
>>+	while (!list_empty(&hp_ctrl->hp_cmd_list)) {
>>+		hp_cmd = list_first_entry(&hp_ctrl->hp_cmd_list,
>>+					  struct octep_hp_cmd, list);
>>+		list_del(&hp_cmd->list);
>>+		spin_unlock_irqrestore(&hp_ctrl->hp_cmd_lock, flags);
>>+
>>+		octep_hp_cmd_handler(hp_ctrl, hp_cmd);
>>+		kfree(hp_cmd);
>>+
>>+		spin_lock_irqsave(&hp_ctrl->hp_cmd_lock, flags);
>>+	}
>>+	spin_unlock_irqrestore(&hp_ctrl->hp_cmd_lock, flags);
>>+}
>>+
>>+static enum octep_hp_intr_type octep_hp_intr_type(struct
>>octep_hp_intr_info *intr,
>>+						  int irq)
>>+{
>>+	enum octep_hp_intr_type type;
>>+
>>+	for (type = OCTEP_HP_INTR_ENA; type < OCTEP_HP_INTR_MAX;
>>type++) {
>>+		if (intr[type].number == irq)
>>+			return type;
>>+	}
>>+
>>+	return OCTEP_HP_INTR_INVALID;
>>+}
>>+
>>+static irqreturn_t octep_hp_intr_handler(int irq, void *data)
>>+{
>>+	struct octep_hp_controller *hp_ctrl = data;
>>+	struct pci_dev *pdev = hp_ctrl->pdev;
>>+	enum octep_hp_intr_type type;
>>+	struct octep_hp_cmd *hp_cmd;
>>+	u64 intr_val;
>>+
>>+	type = octep_hp_intr_type(hp_ctrl->intr, irq);
>>+	if (type == OCTEP_HP_INTR_INVALID) {
>>+		dev_err(&pdev->dev, "Invalid interrupt %d\n", irq);
>>+		return IRQ_HANDLED;
>>+	}
>>+
>>+	/* Read and clear the interrupt */
>>+	intr_val = readq(hp_ctrl->base + OCTEP_HP_INTR_OFFSET(type));
>>+	writeq(intr_val, hp_ctrl->base + OCTEP_HP_INTR_OFFSET(type));
>>+
>>+	hp_cmd = kzalloc(sizeof(*hp_cmd), GFP_ATOMIC);
>>+	if (!hp_cmd)
>>+		return IRQ_HANDLED;
>>+
>>+	hp_cmd->intr_val = intr_val;
>>+	hp_cmd->intr_type = type;
>>+
>>+	/* Add the command to the list and schedule the work */
>>+	spin_lock(&hp_ctrl->hp_cmd_lock);
>>+	list_add_tail(&hp_cmd->list, &hp_ctrl->hp_cmd_list);
>>+	spin_unlock(&hp_ctrl->hp_cmd_lock);
>>+	schedule_work(&hp_ctrl->work);
>>+
>>+	return IRQ_HANDLED;
>>+}
>>+
>>+static void octep_hp_irq_cleanup(void *data)
>>+{
>>+	struct octep_hp_controller *hp_ctrl = data;
>>+
>>+	pci_free_irq_vectors(hp_ctrl->pdev);
>>+	flush_work(&hp_ctrl->work);
>>+}
>>+
>>+static int octep_hp_request_irq(struct octep_hp_controller *hp_ctrl,
>>+				enum octep_hp_intr_type type)
>>+{
>>+	struct pci_dev *pdev = hp_ctrl->pdev;
>>+	struct octep_hp_intr_info *intr;
>>+	int irq;
>>+
>>+	irq = pci_irq_vector(pdev, OCTEP_HP_INTR_VECTOR(type));
>>+	if (irq < 0)
>>+		return irq;
>>+
>>+	intr = &hp_ctrl->intr[type];
>>+	intr->number = irq;
>>+	intr->type = type;
>>+	snprintf(intr->name, sizeof(intr->name), "octep_hp_%d", type);
>>+
>>+	return devm_request_irq(&pdev->dev, irq, octep_hp_intr_handler,
>>+				IRQF_SHARED, intr->name, hp_ctrl);
>>+}
>>+
>>+static int octep_hp_controller_setup(struct pci_dev *pdev,
>>+				     struct octep_hp_controller *hp_ctrl)
>>+{
>>+	struct device *dev = &pdev->dev;
>>+	enum octep_hp_intr_type type;
>>+	int ret;
>>+
>>+	ret = pcim_enable_device(pdev);
>>+	if (ret)
>>+		return dev_err_probe(dev, ret, "Failed to enable PCI device\n");
>>+
>>+	hp_ctrl->base = pcim_iomap_region(pdev, 0, OCTEP_HP_DRV_NAME);
>>+	if (IS_ERR(hp_ctrl->base))
>>+		return dev_err_probe(dev, PTR_ERR(hp_ctrl->base),
>>+				     "Failed to map PCI device region\n");
>>+
>>+	pci_set_master(pdev);
>>+	pci_set_drvdata(pdev, hp_ctrl);
>>+
>>+	INIT_LIST_HEAD(&hp_ctrl->slot_list);
>>+	INIT_LIST_HEAD(&hp_ctrl->hp_cmd_list);
>>+	mutex_init(&hp_ctrl->slot_lock);
>>+	spin_lock_init(&hp_ctrl->hp_cmd_lock);
>>+	INIT_WORK(&hp_ctrl->work, octep_hp_work_handler);
>>+	hp_ctrl->pdev = pdev;
>>+
>>+	ret = pci_alloc_irq_vectors(pdev, 1,
>>+
>>OCTEP_HP_INTR_VECTOR(OCTEP_HP_INTR_MAX),
>>+				    PCI_IRQ_MSIX);
>>+	if (ret < 0)
>>+		return dev_err_probe(dev, ret, "Failed to alloc MSI-X
>>vectors\n");
>>+
>>+	ret = devm_add_action(&pdev->dev, octep_hp_irq_cleanup, hp_ctrl);
>>+	if (ret)
>>+		return dev_err_probe(&pdev->dev, ret, "Failed to add IRQ
>>cleanup action\n");
>>+
>>+	for (type = OCTEP_HP_INTR_ENA; type < OCTEP_HP_INTR_MAX;
>>type++) {
>>+		ret = octep_hp_request_irq(hp_ctrl, type);
>>+		if (ret)
>>+			return dev_err_probe(dev, ret,
>>+					     "Failed to request IRQ for vector
>>%d\n",
>>+					     OCTEP_HP_INTR_VECTOR(type));
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>>+static int octep_hp_pci_probe(struct pci_dev *pdev,
>>+			      const struct pci_device_id *id)
>>+{
>>+	struct octep_hp_controller *hp_ctrl;
>>+	struct pci_dev *tmp_pdev = NULL;
>>+	struct octep_hp_slot *hp_slot;
>>+	u16 slot_number = 0;
>>+	int ret;
>>+
>>+	hp_ctrl = devm_kzalloc(&pdev->dev, sizeof(*hp_ctrl), GFP_KERNEL);
>>+	if (!hp_ctrl)
>>+		return -ENOMEM;
>>+
>>+	ret = octep_hp_controller_setup(pdev, hp_ctrl);
>>+	if (ret)
>>+		return ret;
>>+
>>+	/*
>>+	 * Register all hotplug slots. Hotplug controller is the first function
>>+	 * of the PCI device. The hotplug slots are the remaining functions of
>>+	 * the PCI device. They are removed from the bus and are added back
>>when
>>+	 * the hotplug event is triggered.
>>+	 */
>>+	for_each_pci_dev(tmp_pdev) {
>>+		if (!octep_hp_is_slot(hp_ctrl, tmp_pdev))
>>+			continue;
>>+
>>+		hp_slot = octep_hp_register_slot(hp_ctrl, tmp_pdev,
>>slot_number);
>>+		if (IS_ERR(hp_slot))
>>+			return dev_err_probe(&pdev->dev, PTR_ERR(hp_slot),
>>+					     "Failed to register hotplug slot
>>%u\n",
>>+					     slot_number);
>>+
>>+		ret = devm_add_action(&pdev->dev, octep_hp_deregister_slot,
>>+				      hp_slot);
>>+		if (ret)
>>+			return dev_err_probe(&pdev->dev, ret,
>>+					     "Failed to add action for
>>deregistering slot %u\n",
>>+					     slot_number);
>>+		slot_number++;
>>+	}
>>+
>>+	return 0;
>>+}
>>+
>>+#define OCTEP_DEVID_HP_CONTROLLER 0xa0e3
>>+static struct pci_device_id octep_hp_pci_map[] = {
>>+	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM,
>>OCTEP_DEVID_HP_CONTROLLER) },
>>+	{ },
>>+};
>>+
>>+static struct pci_driver octep_hp = {
>>+	.name = OCTEP_HP_DRV_NAME,
>>+	.id_table = octep_hp_pci_map,
>>+	.probe = octep_hp_pci_probe,
>>+};
>>+
>>+module_pci_driver(octep_hp);
>>+
>>+MODULE_LICENSE("GPL");
>>+MODULE_AUTHOR("Marvell");
>>+MODULE_DESCRIPTION("OCTEON PCIe device hotplug controller driver");
>>--
>>2.25.1
>

Thanks,
Shijith

Shijith Thotton Oct. 14, 2024, 12:01 p.m. UTC | #3

Just a gentle reminder to review the patch when you get a chance.
let me know if any adjustments are needed. Thanks!

>Just a gentle reminder.
>Could you please comments if there are improvements needed?
>
>>Hi Helgaas,
>>
>>Please check this patch.
>>
>>>This patch introduces a PCI hotplug controller driver for the OCTEON
>>>PCIe device, a multi-function PCIe device where the first function acts
>>>as a hotplug controller. It is equipped with MSI-x interrupts to notify
>>>the host of hotplug events from the OCTEON firmware.
>>>
>>>The driver facilitates the hotplugging of non-controller functions
>>>within the same device. During probe, non-controller functions are
>>>removed and registered as PCI hotplug slots. The slots are added back
>>>only upon request from the device firmware. The driver also allows the
>>>enabling and disabling of the slots via sysfs slot entries, provided by
>>>the PCI hotplug framework.
>>>
>>>Signed-off-by: Shijith Thotton <[e-mail]>
>>>Co-developed-by: Vamsi Attunuru <[e-mail]>
>>>Signed-off-by: Vamsi Attunuru <[e-mail]>
>>>---
>>>
>>>This patch introduces a PCI hotplug controller driver for OCTEON PCIe
>hotplug
>>>controller. The OCTEON PCIe device is a multi-function device where the first
>>>function acts as a PCI hotplug controller.
>>>
>>>              +--------------------------------+
>>>              |           Root Port            |
>>>              +--------------------------------+
>>>                              |
>>>                             PCIe
>>>                              |
>>>+---------------------------------------------------------------+
>>>|              OCTEON PCIe Multifunction Device                 |
>>>+---------------------------------------------------------------+
>>>            |                    |              |            |
>>>            |                    |              |            |
>>>+---------------------+  +----------------+  +-----+  +----------------+
>>>|      Function 0     |  |   Function 1   |  | ... |  |   Function 7   |
>>>| (Hotplug controller)|  | (Hotplug slot) |  |     |  | (Hotplug slot) |
>>>+---------------------+  +----------------+  +-----+  +----------------+
>>>            |
>>>            |
>>>+-------------------------+
>>>|   Controller Firmware   |
>>>+-------------------------+
>>>
>>>The hotplug controller driver facilitates the hotplugging of non-controller
>>>functions in the same device. During the probe of the driver, the non-
>>controller
>>>function are removed and registered as PCI hotplug slots. They are added
>back
>>>only upon request from the device firmware. The driver also allows the user
>to
>>>enable/disable the functions using sysfs slot entries provided by PCI hotplug
>>>framework.
>>>
>>>This solution adopts a hardware + software approach for several reasons:
>>>
>>>1. To reduce hardware implementation cost. Supporting complete hotplug
>>>   capability within the card would require a PCI switch implemented within.
>>>
>>>2. In the multi-function device, non-controller functions can act as emulated
>>>   devices. The firmware can dynamically enable or disable them at runtime.
>>>
>>>3. Not all root ports support PCI hotplug. This approach provides greater
>>>   flexibility and compatibility across different hardware configurations.
>>>
>>>The hotplug controller function is lightweight and is equipped with MSI-x
>>>interrupts to notify the host about hotplug events. Upon receiving an
>>>interrupt, the hotplug register is read, and the required function is enabled
>>>or disabled.
>>>
>>>This driver will be beneficial for managing PCI hotplug events on the OCTEON
>>>PCIe device without requiring complex hardware solutions.
>>>
>>>Changes in v2:
>>>- Added missing include files.
>>>- Used dev_err_probe() for error handling.
>>>- Used guard() for mutex locking.
>>>- Splited cleanup actions and added per-slot cleanup action.
>>>- Fixed coding style issues.
>>>- Added co-developed-by tag.
>>>
>>>Changes in v3:
>>>- Explicit assignment of enum values.
>>>- Use pcim_iomap_region() instead of pcim_iomap_regions().
>>>
>>> MAINTAINERS                    |   6 +
>>> drivers/pci/hotplug/Kconfig    |  10 +
>>> drivers/pci/hotplug/Makefile   |   1 +
>>> drivers/pci/hotplug/octep_hp.c | 409
>>>+++++++++++++++++++++++++++++++++
>>> 4 files changed, 426 insertions(+)
>>> create mode 100644 drivers/pci/hotplug/octep_hp.c
>>>
>>>diff --git a/MAINTAINERS b/MAINTAINERS
>>>index 42decde38320..7b5a618eed1c 100644
>>>--- a/MAINTAINERS
>>>+++ b/MAINTAINERS
>>>@@ -13677,6 +13677,12 @@ R:	[e-mail]
>>> R:	[e-mail]
>>> F:	drivers/vdpa/octeon_ep/
>>>
>>>+MARVELL OCTEON HOTPLUG CONTROLLER DRIVER
>>>+R:	Shijith Thotton <[e-mail]>
>>>+R:	Vamsi Attunuru <[e-mail]>
>>>+S:	Supported
>>>+F:	drivers/pci/hotplug/octep_hp.c
>>>+
>>> MATROX FRAMEBUFFER DRIVER
>>> L:	[e-mail]
>>> S:	Orphan
>>>diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
>>>index 1472aef0fb81..2e38fd25f7ef 100644
>>>--- a/drivers/pci/hotplug/Kconfig
>>>+++ b/drivers/pci/hotplug/Kconfig
>>>@@ -173,4 +173,14 @@ config HOTPLUG_PCI_S390
>>>
>>> 	  When in doubt, say Y.
>>>
>>>+config HOTPLUG_PCI_OCTEONEP
>>>+	bool "OCTEON PCI device Hotplug controller driver"
>>>+	depends on HOTPLUG_PCI
>>>+	help
>>>+	  Say Y here if you have an OCTEON PCIe device with a hotplug
>>>+	  controller. This driver enables the non-controller functions of the
>>>+	  device to be registered as hotplug slots.
>>>+
>>>+	  When in doubt, say N.
>>>+
>>> endif # HOTPLUG_PCI
>>>diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile
>>>index 240c99517d5e..40aaf31fe338 100644
>>>--- a/drivers/pci/hotplug/Makefile
>>>+++ b/drivers/pci/hotplug/Makefile
>>>@@ -20,6 +20,7 @@ obj-$(CONFIG_HOTPLUG_PCI_RPA)		+=
>>>rpaphp.o
>>> obj-$(CONFIG_HOTPLUG_PCI_RPA_DLPAR)	+= rpadlpar_io.o
>>> obj-$(CONFIG_HOTPLUG_PCI_ACPI)		+= acpiphp.o
>>> obj-$(CONFIG_HOTPLUG_PCI_S390)		+= s390_pci_hpc.o
>>>+obj-$(CONFIG_HOTPLUG_PCI_OCTEONEP)	+= octep_hp.o
>>>
>>> # acpiphp_ibm extends acpiphp, so should be linked afterwards.
>>>
>>>diff --git a/drivers/pci/hotplug/octep_hp.c
>b/drivers/pci/hotplug/octep_hp.c
>>>new file mode 100644
>>>index 000000000000..77dc2740f43e
>>>--- /dev/null
>>>+++ b/drivers/pci/hotplug/octep_hp.c
>>>@@ -0,0 +1,409 @@
>>>+// SPDX-License-Identifier: GPL-2.0-only
>>>+/* Copyright (C) 2024 Marvell. */
>>>+
>>>+#include <linux/cleanup.h>
>>>+#include <linux/container_of.h>
>>>+#include <linux/delay.h>
>>>+#include <linux/dev_printk.h>
>>>+#include <linux/init.h>
>>>+#include <linux/interrupt.h>
>>>+#include <linux/io-64-nonatomic-lo-hi.h>
>>>+#include <linux/kernel.h>
>>>+#include <linux/list.h>
>>>+#include <linux/module.h>
>>>+#include <linux/mutex.h>
>>>+#include <linux/pci.h>
>>>+#include <linux/pci_hotplug.h>
>>>+#include <linux/slab.h>
>>>+#include <linux/spinlock.h>
>>>+#include <linux/workqueue.h>
>>>+
>>>+#define OCTEP_HP_INTR_OFFSET(x) (0x20400 + ((x) << 4))
>>>+#define OCTEP_HP_INTR_VECTOR(x) (16 + (x))
>>>+#define OCTEP_HP_DRV_NAME "octep_hp"
>>>+
>>>+/*
>>>+ * Type of MSI-X interrupts.
>>>+ * The macros OCTEP_HP_INTR_VECTOR and OCTEP_HP_INTR_OFFSET are
>>>used to
>>>+ * generate the vector and offset for an interrupt type.
>>>+ */
>>>+enum octep_hp_intr_type {
>>>+	OCTEP_HP_INTR_INVALID = -1,
>>>+	OCTEP_HP_INTR_ENA = 0,
>>>+	OCTEP_HP_INTR_DIS = 1,
>>>+	OCTEP_HP_INTR_MAX = 2,
>>>+};
>>>+
>>>+struct octep_hp_cmd {
>>>+	struct list_head list;
>>>+	enum octep_hp_intr_type intr_type;
>>>+	u64 intr_val;
>>>+};
>>>+
>>>+struct octep_hp_slot {
>>>+	struct list_head list;
>>>+	struct hotplug_slot slot;
>>>+	u16 slot_number;
>>>+	struct pci_dev *hp_pdev;
>>>+	unsigned int hp_devfn;
>>>+	struct octep_hp_controller *ctrl;
>>>+};
>>>+
>>>+struct octep_hp_intr_info {
>>>+	enum octep_hp_intr_type type;
>>>+	int number;
>>>+	char name[16];
>>>+};
>>>+
>>>+struct octep_hp_controller {
>>>+	void __iomem *base;
>>>+	struct pci_dev *pdev;
>>>+	struct octep_hp_intr_info intr[OCTEP_HP_INTR_MAX];
>>>+	struct work_struct work;
>>>+	struct list_head slot_list;
>>>+	struct mutex slot_lock; /* Protects slot_list */
>>>+	struct list_head hp_cmd_list;
>>>+	spinlock_t hp_cmd_lock; /* Protects hp_cmd_list */
>>>+};
>>>+
>>>+static void octep_hp_enable_pdev(struct octep_hp_controller *hp_ctrl,
>>>+				 struct octep_hp_slot *hp_slot)
>>>+{
>>>+	guard(mutex)(&hp_ctrl->slot_lock);
>>>+	if (hp_slot->hp_pdev) {
>>>+		dev_dbg(&hp_slot->hp_pdev->dev, "Slot %u already
>>>enabled\n",
>>>+			hp_slot->slot_number);
>>>+		return;
>>>+	}
>>>+
>>>+	/* Scan the device and add it to the bus */
>>>+	hp_slot->hp_pdev = pci_scan_single_device(hp_ctrl->pdev->bus,
>>>+						  hp_slot->hp_devfn);
>>>+	pci_bus_assign_resources(hp_ctrl->pdev->bus);
>>>+	pci_bus_add_device(hp_slot->hp_pdev);
>>>+
>>>+	dev_dbg(&hp_slot->hp_pdev->dev, "Enabled slot %u\n",
>>>+		hp_slot->slot_number);
>>>+}
>>>+
>>>+static void octep_hp_disable_pdev(struct octep_hp_controller *hp_ctrl,
>>>+				  struct octep_hp_slot *hp_slot)
>>>+{
>>>+	guard(mutex)(&hp_ctrl->slot_lock);
>>>+	if (!hp_slot->hp_pdev) {
>>>+		dev_dbg(&hp_ctrl->pdev->dev, "Slot %u already disabled\n",
>>>+			hp_slot->slot_number);
>>>+		return;
>>>+	}
>>>+
>>>+	dev_dbg(&hp_slot->hp_pdev->dev, "Disabling slot %u\n",
>>>+		hp_slot->slot_number);
>>>+
>>>+	/* Remove the device from the bus */
>>>+	pci_stop_and_remove_bus_device_locked(hp_slot->hp_pdev);
>>>+	hp_slot->hp_pdev = NULL;
>>>+}
>>>+
>>>+static int octep_hp_enable_slot(struct hotplug_slot *slot)
>>>+{
>>>+	struct octep_hp_slot *hp_slot =
>>>+		container_of(slot, struct octep_hp_slot, slot);
>>>+
>>>+	octep_hp_enable_pdev(hp_slot->ctrl, hp_slot);
>>>+	return 0;
>>>+}
>>>+
>>>+static int octep_hp_disable_slot(struct hotplug_slot *slot)
>>>+{
>>>+	struct octep_hp_slot *hp_slot =
>>>+		container_of(slot, struct octep_hp_slot, slot);
>>>+
>>>+	octep_hp_disable_pdev(hp_slot->ctrl, hp_slot);
>>>+	return 0;
>>>+}
>>>+
>>>+static struct hotplug_slot_ops octep_hp_slot_ops = {
>>>+	.enable_slot = octep_hp_enable_slot,
>>>+	.disable_slot = octep_hp_disable_slot,
>>>+};
>>>+
>>>+#define SLOT_NAME_SIZE 16
>>>+static struct octep_hp_slot *
>>>+octep_hp_register_slot(struct octep_hp_controller *hp_ctrl,
>>>+		       struct pci_dev *pdev, u16 slot_number)
>>>+{
>>>+	char slot_name[SLOT_NAME_SIZE];
>>>+	struct octep_hp_slot *hp_slot;
>>>+	int ret;
>>>+
>>>+	hp_slot = kzalloc(sizeof(*hp_slot), GFP_KERNEL);
>>>+	if (!hp_slot)
>>>+		return ERR_PTR(-ENOMEM);
>>>+
>>>+	hp_slot->ctrl = hp_ctrl;
>>>+	hp_slot->hp_pdev = pdev;
>>>+	hp_slot->hp_devfn = pdev->devfn;
>>>+	hp_slot->slot_number = slot_number;
>>>+	hp_slot->slot.ops = &octep_hp_slot_ops;
>>>+
>>>+	snprintf(slot_name, sizeof(slot_name), "octep_hp_%u", slot_number);
>>>+	ret = pci_hp_register(&hp_slot->slot, hp_ctrl->pdev->bus,
>>>+			      PCI_SLOT(pdev->devfn), slot_name);
>>>+	if (ret) {
>>>+		kfree(hp_slot);
>>>+		return ERR_PTR(ret);
>>>+	}
>>>+
>>>+	list_add_tail(&hp_slot->list, &hp_ctrl->slot_list);
>>>+	octep_hp_disable_pdev(hp_ctrl, hp_slot);
>>>+
>>>+	return hp_slot;
>>>+}
>>>+
>>>+static void octep_hp_deregister_slot(void *data)
>>>+{
>>>+	struct octep_hp_slot *hp_slot = data;
>>>+	struct octep_hp_controller *hp_ctrl = hp_slot->ctrl;
>>>+
>>>+	pci_hp_deregister(&hp_slot->slot);
>>>+	octep_hp_enable_pdev(hp_ctrl, hp_slot);
>>>+	list_del(&hp_slot->list);
>>>+	kfree(hp_slot);
>>>+}
>>>+
>>>+static bool octep_hp_is_slot(struct octep_hp_controller *hp_ctrl,
>>>+			     struct pci_dev *pdev)
>>>+{
>>>+	/* Check if the PCI device can be hotplugged */
>>>+	return pdev != hp_ctrl->pdev && pdev->bus == hp_ctrl->pdev->bus &&
>>>+	       PCI_SLOT(pdev->devfn) == PCI_SLOT(hp_ctrl->pdev->devfn);
>>>+}
>>>+
>>>+static void octep_hp_cmd_handler(struct octep_hp_controller *hp_ctrl,
>>>+				 struct octep_hp_cmd *hp_cmd)
>>>+{
>>>+	struct octep_hp_slot *hp_slot;
>>>+
>>>+	/*
>>>+	 * Enable or disable the slots based on the slot mask.
>>>+	 * intr_val is a bit mask where each bit represents a slot.
>>>+	 */
>>>+	list_for_each_entry(hp_slot, &hp_ctrl->slot_list, list) {
>>>+		if (!(hp_cmd->intr_val & BIT(hp_slot->slot_number)))
>>>+			continue;
>>>+
>>>+		if (hp_cmd->intr_type == OCTEP_HP_INTR_ENA)
>>>+			octep_hp_enable_pdev(hp_ctrl, hp_slot);
>>>+		else
>>>+			octep_hp_disable_pdev(hp_ctrl, hp_slot);
>>>+	}
>>>+}
>>>+
>>>+static void octep_hp_work_handler(struct work_struct *work)
>>>+{
>>>+	struct octep_hp_controller *hp_ctrl;
>>>+	struct octep_hp_cmd *hp_cmd;
>>>+	unsigned long flags;
>>>+
>>>+	hp_ctrl = container_of(work, struct octep_hp_controller, work);
>>>+
>>>+	/* Process all the hotplug commands */
>>>+	spin_lock_irqsave(&hp_ctrl->hp_cmd_lock, flags);
>>>+	while (!list_empty(&hp_ctrl->hp_cmd_list)) {
>>>+		hp_cmd = list_first_entry(&hp_ctrl->hp_cmd_list,
>>>+					  struct octep_hp_cmd, list);
>>>+		list_del(&hp_cmd->list);
>>>+		spin_unlock_irqrestore(&hp_ctrl->hp_cmd_lock, flags);
>>>+
>>>+		octep_hp_cmd_handler(hp_ctrl, hp_cmd);
>>>+		kfree(hp_cmd);
>>>+
>>>+		spin_lock_irqsave(&hp_ctrl->hp_cmd_lock, flags);
>>>+	}
>>>+	spin_unlock_irqrestore(&hp_ctrl->hp_cmd_lock, flags);
>>>+}
>>>+
>>>+static enum octep_hp_intr_type octep_hp_intr_type(struct
>>>octep_hp_intr_info *intr,
>>>+						  int irq)
>>>+{
>>>+	enum octep_hp_intr_type type;
>>>+
>>>+	for (type = OCTEP_HP_INTR_ENA; type < OCTEP_HP_INTR_MAX;
>>>type++) {
>>>+		if (intr[type].number == irq)
>>>+			return type;
>>>+	}
>>>+
>>>+	return OCTEP_HP_INTR_INVALID;
>>>+}
>>>+
>>>+static irqreturn_t octep_hp_intr_handler(int irq, void *data)
>>>+{
>>>+	struct octep_hp_controller *hp_ctrl = data;
>>>+	struct pci_dev *pdev = hp_ctrl->pdev;
>>>+	enum octep_hp_intr_type type;
>>>+	struct octep_hp_cmd *hp_cmd;
>>>+	u64 intr_val;
>>>+
>>>+	type = octep_hp_intr_type(hp_ctrl->intr, irq);
>>>+	if (type == OCTEP_HP_INTR_INVALID) {
>>>+		dev_err(&pdev->dev, "Invalid interrupt %d\n", irq);
>>>+		return IRQ_HANDLED;
>>>+	}
>>>+
>>>+	/* Read and clear the interrupt */
>>>+	intr_val = readq(hp_ctrl->base + OCTEP_HP_INTR_OFFSET(type));
>>>+	writeq(intr_val, hp_ctrl->base + OCTEP_HP_INTR_OFFSET(type));
>>>+
>>>+	hp_cmd = kzalloc(sizeof(*hp_cmd), GFP_ATOMIC);
>>>+	if (!hp_cmd)
>>>+		return IRQ_HANDLED;
>>>+
>>>+	hp_cmd->intr_val = intr_val;
>>>+	hp_cmd->intr_type = type;
>>>+
>>>+	/* Add the command to the list and schedule the work */
>>>+	spin_lock(&hp_ctrl->hp_cmd_lock);
>>>+	list_add_tail(&hp_cmd->list, &hp_ctrl->hp_cmd_list);
>>>+	spin_unlock(&hp_ctrl->hp_cmd_lock);
>>>+	schedule_work(&hp_ctrl->work);
>>>+
>>>+	return IRQ_HANDLED;
>>>+}
>>>+
>>>+static void octep_hp_irq_cleanup(void *data)
>>>+{
>>>+	struct octep_hp_controller *hp_ctrl = data;
>>>+
>>>+	pci_free_irq_vectors(hp_ctrl->pdev);
>>>+	flush_work(&hp_ctrl->work);
>>>+}
>>>+
>>>+static int octep_hp_request_irq(struct octep_hp_controller *hp_ctrl,
>>>+				enum octep_hp_intr_type type)
>>>+{
>>>+	struct pci_dev *pdev = hp_ctrl->pdev;
>>>+	struct octep_hp_intr_info *intr;
>>>+	int irq;
>>>+
>>>+	irq = pci_irq_vector(pdev, OCTEP_HP_INTR_VECTOR(type));
>>>+	if (irq < 0)
>>>+		return irq;
>>>+
>>>+	intr = &hp_ctrl->intr[type];
>>>+	intr->number = irq;
>>>+	intr->type = type;
>>>+	snprintf(intr->name, sizeof(intr->name), "octep_hp_%d", type);
>>>+
>>>+	return devm_request_irq(&pdev->dev, irq, octep_hp_intr_handler,
>>>+				IRQF_SHARED, intr->name, hp_ctrl);
>>>+}
>>>+
>>>+static int octep_hp_controller_setup(struct pci_dev *pdev,
>>>+				     struct octep_hp_controller *hp_ctrl)
>>>+{
>>>+	struct device *dev = &pdev->dev;
>>>+	enum octep_hp_intr_type type;
>>>+	int ret;
>>>+
>>>+	ret = pcim_enable_device(pdev);
>>>+	if (ret)
>>>+		return dev_err_probe(dev, ret, "Failed to enable PCI device\n");
>>>+
>>>+	hp_ctrl->base = pcim_iomap_region(pdev, 0, OCTEP_HP_DRV_NAME);
>>>+	if (IS_ERR(hp_ctrl->base))
>>>+		return dev_err_probe(dev, PTR_ERR(hp_ctrl->base),
>>>+				     "Failed to map PCI device region\n");
>>>+
>>>+	pci_set_master(pdev);
>>>+	pci_set_drvdata(pdev, hp_ctrl);
>>>+
>>>+	INIT_LIST_HEAD(&hp_ctrl->slot_list);
>>>+	INIT_LIST_HEAD(&hp_ctrl->hp_cmd_list);
>>>+	mutex_init(&hp_ctrl->slot_lock);
>>>+	spin_lock_init(&hp_ctrl->hp_cmd_lock);
>>>+	INIT_WORK(&hp_ctrl->work, octep_hp_work_handler);
>>>+	hp_ctrl->pdev = pdev;
>>>+
>>>+	ret = pci_alloc_irq_vectors(pdev, 1,
>>>+
>>>OCTEP_HP_INTR_VECTOR(OCTEP_HP_INTR_MAX),
>>>+				    PCI_IRQ_MSIX);
>>>+	if (ret < 0)
>>>+		return dev_err_probe(dev, ret, "Failed to alloc MSI-X
>>>vectors\n");
>>>+
>>>+	ret = devm_add_action(&pdev->dev, octep_hp_irq_cleanup, hp_ctrl);
>>>+	if (ret)
>>>+		return dev_err_probe(&pdev->dev, ret, "Failed to add IRQ
>>>cleanup action\n");
>>>+
>>>+	for (type = OCTEP_HP_INTR_ENA; type < OCTEP_HP_INTR_MAX;
>>>type++) {
>>>+		ret = octep_hp_request_irq(hp_ctrl, type);
>>>+		if (ret)
>>>+			return dev_err_probe(dev, ret,
>>>+					     "Failed to request IRQ for vector
>>>%d\n",
>>>+					     OCTEP_HP_INTR_VECTOR(type));
>>>+	}
>>>+
>>>+	return 0;
>>>+}
>>>+
>>>+static int octep_hp_pci_probe(struct pci_dev *pdev,
>>>+			      const struct pci_device_id *id)
>>>+{
>>>+	struct octep_hp_controller *hp_ctrl;
>>>+	struct pci_dev *tmp_pdev = NULL;
>>>+	struct octep_hp_slot *hp_slot;
>>>+	u16 slot_number = 0;
>>>+	int ret;
>>>+
>>>+	hp_ctrl = devm_kzalloc(&pdev->dev, sizeof(*hp_ctrl), GFP_KERNEL);
>>>+	if (!hp_ctrl)
>>>+		return -ENOMEM;
>>>+
>>>+	ret = octep_hp_controller_setup(pdev, hp_ctrl);
>>>+	if (ret)
>>>+		return ret;
>>>+
>>>+	/*
>>>+	 * Register all hotplug slots. Hotplug controller is the first function
>>>+	 * of the PCI device. The hotplug slots are the remaining functions of
>>>+	 * the PCI device. They are removed from the bus and are added back
>>>when
>>>+	 * the hotplug event is triggered.
>>>+	 */
>>>+	for_each_pci_dev(tmp_pdev) {
>>>+		if (!octep_hp_is_slot(hp_ctrl, tmp_pdev))
>>>+			continue;
>>>+
>>>+		hp_slot = octep_hp_register_slot(hp_ctrl, tmp_pdev,
>>>slot_number);
>>>+		if (IS_ERR(hp_slot))
>>>+			return dev_err_probe(&pdev->dev, PTR_ERR(hp_slot),
>>>+					     "Failed to register hotplug slot
>>>%u\n",
>>>+					     slot_number);
>>>+
>>>+		ret = devm_add_action(&pdev->dev, octep_hp_deregister_slot,
>>>+				      hp_slot);
>>>+		if (ret)
>>>+			return dev_err_probe(&pdev->dev, ret,
>>>+					     "Failed to add action for
>>>deregistering slot %u\n",
>>>+					     slot_number);
>>>+		slot_number++;
>>>+	}
>>>+
>>>+	return 0;
>>>+}
>>>+
>>>+#define OCTEP_DEVID_HP_CONTROLLER 0xa0e3
>>>+static struct pci_device_id octep_hp_pci_map[] = {
>>>+	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM,
>>>OCTEP_DEVID_HP_CONTROLLER) },
>>>+	{ },
>>>+};
>>>+
>>>+static struct pci_driver octep_hp = {
>>>+	.name = OCTEP_HP_DRV_NAME,
>>>+	.id_table = octep_hp_pci_map,
>>>+	.probe = octep_hp_pci_probe,
>>>+};
>>>+
>>>+module_pci_driver(octep_hp);
>>>+
>>>+MODULE_LICENSE("GPL");
>>>+MODULE_AUTHOR("Marvell");
>>>+MODULE_DESCRIPTION("OCTEON PCIe device hotplug controller driver");
>>>--
>>>2.25.1
>>

Shijith Thotton Oct. 23, 2024, 9:06 a.m. UTC | #4

Ping. Please check this patch.

>>>>This patch introduces a PCI hotplug controller driver for the OCTEON
>>>>PCIe device, a multi-function PCIe device where the first function acts
>>>>as a hotplug controller. It is equipped with MSI-x interrupts to notify
>>>>the host of hotplug events from the OCTEON firmware.
>>>>
>>>>The driver facilitates the hotplugging of non-controller functions
>>>>within the same device. During probe, non-controller functions are
>>>>removed and registered as PCI hotplug slots. The slots are added back
>>>>only upon request from the device firmware. The driver also allows the
>>>>enabling and disabling of the slots via sysfs slot entries, provided by
>>>>the PCI hotplug framework.
>>>>
>>>>Signed-off-by: Shijith Thotton <[e-mail]>
>>>>Co-developed-by: Vamsi Attunuru <[e-mail]>
>>>>Signed-off-by: Vamsi Attunuru <[e-mail]>
>>>>---
>>>>
>>>>This patch introduces a PCI hotplug controller driver for OCTEON PCIe
>>hotplug
>>>>controller. The OCTEON PCIe device is a multi-function device where the
>first
>>>>function acts as a PCI hotplug controller.
>>>>
>>>>              +--------------------------------+
>>>>              |           Root Port            |
>>>>              +--------------------------------+
>>>>                              |
>>>>                             PCIe
>>>>                              |
>>>>+---------------------------------------------------------------+
>>>>|              OCTEON PCIe Multifunction Device                 |
>>>>+---------------------------------------------------------------+
>>>>            |                    |              |            |
>>>>            |                    |              |            |
>>>>+---------------------+  +----------------+  +-----+  +----------------+
>>>>|      Function 0     |  |   Function 1   |  | ... |  |   Function 7   |
>>>>| (Hotplug controller)|  | (Hotplug slot) |  |     |  | (Hotplug slot) |
>>>>+---------------------+  +----------------+  +-----+  +----------------+
>>>>            |
>>>>            |
>>>>+-------------------------+
>>>>|   Controller Firmware   |
>>>>+-------------------------+
>>>>
>>>>The hotplug controller driver facilitates the hotplugging of non-controller
>>>>functions in the same device. During the probe of the driver, the non-
>>>controller
>>>>function are removed and registered as PCI hotplug slots. They are added
>>back
>>>>only upon request from the device firmware. The driver also allows the user
>>to
>>>>enable/disable the functions using sysfs slot entries provided by PCI hotplug
>>>>framework.
>>>>
>>>>This solution adopts a hardware + software approach for several reasons:
>>>>
>>>>1. To reduce hardware implementation cost. Supporting complete hotplug
>>>>   capability within the card would require a PCI switch implemented within.
>>>>
>>>>2. In the multi-function device, non-controller functions can act as
>emulated
>>>>   devices. The firmware can dynamically enable or disable them at runtime.
>>>>
>>>>3. Not all root ports support PCI hotplug. This approach provides greater
>>>>   flexibility and compatibility across different hardware configurations.
>>>>
>>>>The hotplug controller function is lightweight and is equipped with MSI-x
>>>>interrupts to notify the host about hotplug events. Upon receiving an
>>>>interrupt, the hotplug register is read, and the required function is enabled
>>>>or disabled.
>>>>
>>>>This driver will be beneficial for managing PCI hotplug events on the
>OCTEON
>>>>PCIe device without requiring complex hardware solutions.
>>>>
>>>>Changes in v2:
>>>>- Added missing include files.
>>>>- Used dev_err_probe() for error handling.
>>>>- Used guard() for mutex locking.
>>>>- Splited cleanup actions and added per-slot cleanup action.
>>>>- Fixed coding style issues.
>>>>- Added co-developed-by tag.
>>>>
>>>>Changes in v3:
>>>>- Explicit assignment of enum values.
>>>>- Use pcim_iomap_region() instead of pcim_iomap_regions().
>>>>
>>>> MAINTAINERS                    |   6 +
>>>> drivers/pci/hotplug/Kconfig    |  10 +
>>>> drivers/pci/hotplug/Makefile   |   1 +
>>>> drivers/pci/hotplug/octep_hp.c | 409
>>>>+++++++++++++++++++++++++++++++++
>>>> 4 files changed, 426 insertions(+)
>>>> create mode 100644 drivers/pci/hotplug/octep_hp.c
>>>>
>>>>diff --git a/MAINTAINERS b/MAINTAINERS
>>>>index 42decde38320..7b5a618eed1c 100644
>>>>--- a/MAINTAINERS
>>>>+++ b/MAINTAINERS
>>>>@@ -13677,6 +13677,12 @@ R:	[e-mail]
>>>> R:	[e-mail]
>>>> F:	drivers/vdpa/octeon_ep/
>>>>
>>>>+MARVELL OCTEON HOTPLUG CONTROLLER DRIVER
>>>>+R:	Shijith Thotton <[e-mail]>
>>>>+R:	Vamsi Attunuru <[e-mail]>
>>>>+S:	Supported
>>>>+F:	drivers/pci/hotplug/octep_hp.c
>>>>+
>>>> MATROX FRAMEBUFFER DRIVER
>>>> L:	[e-mail]
>>>> S:	Orphan
>>>>diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
>>>>index 1472aef0fb81..2e38fd25f7ef 100644
>>>>--- a/drivers/pci/hotplug/Kconfig
>>>>+++ b/drivers/pci/hotplug/Kconfig
>>>>@@ -173,4 +173,14 @@ config HOTPLUG_PCI_S390
>>>>
>>>> 	  When in doubt, say Y.
>>>>
>>>>+config HOTPLUG_PCI_OCTEONEP
>>>>+	bool "OCTEON PCI device Hotplug controller driver"
>>>>+	depends on HOTPLUG_PCI
>>>>+	help
>>>>+	  Say Y here if you have an OCTEON PCIe device with a hotplug
>>>>+	  controller. This driver enables the non-controller functions of the
>>>>+	  device to be registered as hotplug slots.
>>>>+
>>>>+	  When in doubt, say N.
>>>>+
>>>> endif # HOTPLUG_PCI
>>>>diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile
>>>>index 240c99517d5e..40aaf31fe338 100644
>>>>--- a/drivers/pci/hotplug/Makefile
>>>>+++ b/drivers/pci/hotplug/Makefile
>>>>@@ -20,6 +20,7 @@ obj-$(CONFIG_HOTPLUG_PCI_RPA)		+=
>>>>rpaphp.o
>>>> obj-$(CONFIG_HOTPLUG_PCI_RPA_DLPAR)	+= rpadlpar_io.o
>>>> obj-$(CONFIG_HOTPLUG_PCI_ACPI)		+= acpiphp.o
>>>> obj-$(CONFIG_HOTPLUG_PCI_S390)		+= s390_pci_hpc.o
>>>>+obj-$(CONFIG_HOTPLUG_PCI_OCTEONEP)	+= octep_hp.o
>>>>
>>>> # acpiphp_ibm extends acpiphp, so should be linked afterwards.
>>>>
>>>>diff --git a/drivers/pci/hotplug/octep_hp.c
>>b/drivers/pci/hotplug/octep_hp.c
>>>>new file mode 100644
>>>>index 000000000000..77dc2740f43e
>>>>--- /dev/null
>>>>+++ b/drivers/pci/hotplug/octep_hp.c
>>>>@@ -0,0 +1,409 @@
>>>>+// SPDX-License-Identifier: GPL-2.0-only
>>>>+/* Copyright (C) 2024 Marvell. */
>>>>+
>>>>+#include <linux/cleanup.h>
>>>>+#include <linux/container_of.h>
>>>>+#include <linux/delay.h>
>>>>+#include <linux/dev_printk.h>
>>>>+#include <linux/init.h>
>>>>+#include <linux/interrupt.h>
>>>>+#include <linux/io-64-nonatomic-lo-hi.h>
>>>>+#include <linux/kernel.h>
>>>>+#include <linux/list.h>
>>>>+#include <linux/module.h>
>>>>+#include <linux/mutex.h>
>>>>+#include <linux/pci.h>
>>>>+#include <linux/pci_hotplug.h>
>>>>+#include <linux/slab.h>
>>>>+#include <linux/spinlock.h>
>>>>+#include <linux/workqueue.h>
>>>>+
>>>>+#define OCTEP_HP_INTR_OFFSET(x) (0x20400 + ((x) << 4))
>>>>+#define OCTEP_HP_INTR_VECTOR(x) (16 + (x))
>>>>+#define OCTEP_HP_DRV_NAME "octep_hp"
>>>>+
>>>>+/*
>>>>+ * Type of MSI-X interrupts.
>>>>+ * The macros OCTEP_HP_INTR_VECTOR and OCTEP_HP_INTR_OFFSET are
>>>>used to
>>>>+ * generate the vector and offset for an interrupt type.
>>>>+ */
>>>>+enum octep_hp_intr_type {
>>>>+	OCTEP_HP_INTR_INVALID = -1,
>>>>+	OCTEP_HP_INTR_ENA = 0,
>>>>+	OCTEP_HP_INTR_DIS = 1,
>>>>+	OCTEP_HP_INTR_MAX = 2,
>>>>+};
>>>>+
>>>>+struct octep_hp_cmd {
>>>>+	struct list_head list;
>>>>+	enum octep_hp_intr_type intr_type;
>>>>+	u64 intr_val;
>>>>+};
>>>>+
>>>>+struct octep_hp_slot {
>>>>+	struct list_head list;
>>>>+	struct hotplug_slot slot;
>>>>+	u16 slot_number;
>>>>+	struct pci_dev *hp_pdev;
>>>>+	unsigned int hp_devfn;
>>>>+	struct octep_hp_controller *ctrl;
>>>>+};
>>>>+
>>>>+struct octep_hp_intr_info {
>>>>+	enum octep_hp_intr_type type;
>>>>+	int number;
>>>>+	char name[16];
>>>>+};
>>>>+
>>>>+struct octep_hp_controller {
>>>>+	void __iomem *base;
>>>>+	struct pci_dev *pdev;
>>>>+	struct octep_hp_intr_info intr[OCTEP_HP_INTR_MAX];
>>>>+	struct work_struct work;
>>>>+	struct list_head slot_list;
>>>>+	struct mutex slot_lock; /* Protects slot_list */
>>>>+	struct list_head hp_cmd_list;
>>>>+	spinlock_t hp_cmd_lock; /* Protects hp_cmd_list */
>>>>+};
>>>>+
>>>>+static void octep_hp_enable_pdev(struct octep_hp_controller *hp_ctrl,
>>>>+				 struct octep_hp_slot *hp_slot)
>>>>+{
>>>>+	guard(mutex)(&hp_ctrl->slot_lock);
>>>>+	if (hp_slot->hp_pdev) {
>>>>+		dev_dbg(&hp_slot->hp_pdev->dev, "Slot %u already
>>>>enabled\n",
>>>>+			hp_slot->slot_number);
>>>>+		return;
>>>>+	}
>>>>+
>>>>+	/* Scan the device and add it to the bus */
>>>>+	hp_slot->hp_pdev = pci_scan_single_device(hp_ctrl->pdev->bus,
>>>>+						  hp_slot->hp_devfn);
>>>>+	pci_bus_assign_resources(hp_ctrl->pdev->bus);
>>>>+	pci_bus_add_device(hp_slot->hp_pdev);
>>>>+
>>>>+	dev_dbg(&hp_slot->hp_pdev->dev, "Enabled slot %u\n",
>>>>+		hp_slot->slot_number);
>>>>+}
>>>>+
>>>>+static void octep_hp_disable_pdev(struct octep_hp_controller *hp_ctrl,
>>>>+				  struct octep_hp_slot *hp_slot)
>>>>+{
>>>>+	guard(mutex)(&hp_ctrl->slot_lock);
>>>>+	if (!hp_slot->hp_pdev) {
>>>>+		dev_dbg(&hp_ctrl->pdev->dev, "Slot %u already disabled\n",
>>>>+			hp_slot->slot_number);
>>>>+		return;
>>>>+	}
>>>>+
>>>>+	dev_dbg(&hp_slot->hp_pdev->dev, "Disabling slot %u\n",
>>>>+		hp_slot->slot_number);
>>>>+
>>>>+	/* Remove the device from the bus */
>>>>+	pci_stop_and_remove_bus_device_locked(hp_slot->hp_pdev);
>>>>+	hp_slot->hp_pdev = NULL;
>>>>+}
>>>>+
>>>>+static int octep_hp_enable_slot(struct hotplug_slot *slot)
>>>>+{
>>>>+	struct octep_hp_slot *hp_slot =
>>>>+		container_of(slot, struct octep_hp_slot, slot);
>>>>+
>>>>+	octep_hp_enable_pdev(hp_slot->ctrl, hp_slot);
>>>>+	return 0;
>>>>+}
>>>>+
>>>>+static int octep_hp_disable_slot(struct hotplug_slot *slot)
>>>>+{
>>>>+	struct octep_hp_slot *hp_slot =
>>>>+		container_of(slot, struct octep_hp_slot, slot);
>>>>+
>>>>+	octep_hp_disable_pdev(hp_slot->ctrl, hp_slot);
>>>>+	return 0;
>>>>+}
>>>>+
>>>>+static struct hotplug_slot_ops octep_hp_slot_ops = {
>>>>+	.enable_slot = octep_hp_enable_slot,
>>>>+	.disable_slot = octep_hp_disable_slot,
>>>>+};
>>>>+
>>>>+#define SLOT_NAME_SIZE 16
>>>>+static struct octep_hp_slot *
>>>>+octep_hp_register_slot(struct octep_hp_controller *hp_ctrl,
>>>>+		       struct pci_dev *pdev, u16 slot_number)
>>>>+{
>>>>+	char slot_name[SLOT_NAME_SIZE];
>>>>+	struct octep_hp_slot *hp_slot;
>>>>+	int ret;
>>>>+
>>>>+	hp_slot = kzalloc(sizeof(*hp_slot), GFP_KERNEL);
>>>>+	if (!hp_slot)
>>>>+		return ERR_PTR(-ENOMEM);
>>>>+
>>>>+	hp_slot->ctrl = hp_ctrl;
>>>>+	hp_slot->hp_pdev = pdev;
>>>>+	hp_slot->hp_devfn = pdev->devfn;
>>>>+	hp_slot->slot_number = slot_number;
>>>>+	hp_slot->slot.ops = &octep_hp_slot_ops;
>>>>+
>>>>+	snprintf(slot_name, sizeof(slot_name), "octep_hp_%u", slot_number);
>>>>+	ret = pci_hp_register(&hp_slot->slot, hp_ctrl->pdev->bus,
>>>>+			      PCI_SLOT(pdev->devfn), slot_name);
>>>>+	if (ret) {
>>>>+		kfree(hp_slot);
>>>>+		return ERR_PTR(ret);
>>>>+	}
>>>>+
>>>>+	list_add_tail(&hp_slot->list, &hp_ctrl->slot_list);
>>>>+	octep_hp_disable_pdev(hp_ctrl, hp_slot);
>>>>+
>>>>+	return hp_slot;
>>>>+}
>>>>+
>>>>+static void octep_hp_deregister_slot(void *data)
>>>>+{
>>>>+	struct octep_hp_slot *hp_slot = data;
>>>>+	struct octep_hp_controller *hp_ctrl = hp_slot->ctrl;
>>>>+
>>>>+	pci_hp_deregister(&hp_slot->slot);
>>>>+	octep_hp_enable_pdev(hp_ctrl, hp_slot);
>>>>+	list_del(&hp_slot->list);
>>>>+	kfree(hp_slot);
>>>>+}
>>>>+
>>>>+static bool octep_hp_is_slot(struct octep_hp_controller *hp_ctrl,
>>>>+			     struct pci_dev *pdev)
>>>>+{
>>>>+	/* Check if the PCI device can be hotplugged */
>>>>+	return pdev != hp_ctrl->pdev && pdev->bus == hp_ctrl->pdev->bus &&
>>>>+	       PCI_SLOT(pdev->devfn) == PCI_SLOT(hp_ctrl->pdev->devfn);
>>>>+}
>>>>+
>>>>+static void octep_hp_cmd_handler(struct octep_hp_controller *hp_ctrl,
>>>>+				 struct octep_hp_cmd *hp_cmd)
>>>>+{
>>>>+	struct octep_hp_slot *hp_slot;
>>>>+
>>>>+	/*
>>>>+	 * Enable or disable the slots based on the slot mask.
>>>>+	 * intr_val is a bit mask where each bit represents a slot.
>>>>+	 */
>>>>+	list_for_each_entry(hp_slot, &hp_ctrl->slot_list, list) {
>>>>+		if (!(hp_cmd->intr_val & BIT(hp_slot->slot_number)))
>>>>+			continue;
>>>>+
>>>>+		if (hp_cmd->intr_type == OCTEP_HP_INTR_ENA)
>>>>+			octep_hp_enable_pdev(hp_ctrl, hp_slot);
>>>>+		else
>>>>+			octep_hp_disable_pdev(hp_ctrl, hp_slot);
>>>>+	}
>>>>+}
>>>>+
>>>>+static void octep_hp_work_handler(struct work_struct *work)
>>>>+{
>>>>+	struct octep_hp_controller *hp_ctrl;
>>>>+	struct octep_hp_cmd *hp_cmd;
>>>>+	unsigned long flags;
>>>>+
>>>>+	hp_ctrl = container_of(work, struct octep_hp_controller, work);
>>>>+
>>>>+	/* Process all the hotplug commands */
>>>>+	spin_lock_irqsave(&hp_ctrl->hp_cmd_lock, flags);
>>>>+	while (!list_empty(&hp_ctrl->hp_cmd_list)) {
>>>>+		hp_cmd = list_first_entry(&hp_ctrl->hp_cmd_list,
>>>>+					  struct octep_hp_cmd, list);
>>>>+		list_del(&hp_cmd->list);
>>>>+		spin_unlock_irqrestore(&hp_ctrl->hp_cmd_lock, flags);
>>>>+
>>>>+		octep_hp_cmd_handler(hp_ctrl, hp_cmd);
>>>>+		kfree(hp_cmd);
>>>>+
>>>>+		spin_lock_irqsave(&hp_ctrl->hp_cmd_lock, flags);
>>>>+	}
>>>>+	spin_unlock_irqrestore(&hp_ctrl->hp_cmd_lock, flags);
>>>>+}
>>>>+
>>>>+static enum octep_hp_intr_type octep_hp_intr_type(struct
>>>>octep_hp_intr_info *intr,
>>>>+						  int irq)
>>>>+{
>>>>+	enum octep_hp_intr_type type;
>>>>+
>>>>+	for (type = OCTEP_HP_INTR_ENA; type < OCTEP_HP_INTR_MAX;
>>>>type++) {
>>>>+		if (intr[type].number == irq)
>>>>+			return type;
>>>>+	}
>>>>+
>>>>+	return OCTEP_HP_INTR_INVALID;
>>>>+}
>>>>+
>>>>+static irqreturn_t octep_hp_intr_handler(int irq, void *data)
>>>>+{
>>>>+	struct octep_hp_controller *hp_ctrl = data;
>>>>+	struct pci_dev *pdev = hp_ctrl->pdev;
>>>>+	enum octep_hp_intr_type type;
>>>>+	struct octep_hp_cmd *hp_cmd;
>>>>+	u64 intr_val;
>>>>+
>>>>+	type = octep_hp_intr_type(hp_ctrl->intr, irq);
>>>>+	if (type == OCTEP_HP_INTR_INVALID) {
>>>>+		dev_err(&pdev->dev, "Invalid interrupt %d\n", irq);
>>>>+		return IRQ_HANDLED;
>>>>+	}
>>>>+
>>>>+	/* Read and clear the interrupt */
>>>>+	intr_val = readq(hp_ctrl->base + OCTEP_HP_INTR_OFFSET(type));
>>>>+	writeq(intr_val, hp_ctrl->base + OCTEP_HP_INTR_OFFSET(type));
>>>>+
>>>>+	hp_cmd = kzalloc(sizeof(*hp_cmd), GFP_ATOMIC);
>>>>+	if (!hp_cmd)
>>>>+		return IRQ_HANDLED;
>>>>+
>>>>+	hp_cmd->intr_val = intr_val;
>>>>+	hp_cmd->intr_type = type;
>>>>+
>>>>+	/* Add the command to the list and schedule the work */
>>>>+	spin_lock(&hp_ctrl->hp_cmd_lock);
>>>>+	list_add_tail(&hp_cmd->list, &hp_ctrl->hp_cmd_list);
>>>>+	spin_unlock(&hp_ctrl->hp_cmd_lock);
>>>>+	schedule_work(&hp_ctrl->work);
>>>>+
>>>>+	return IRQ_HANDLED;
>>>>+}
>>>>+
>>>>+static void octep_hp_irq_cleanup(void *data)
>>>>+{
>>>>+	struct octep_hp_controller *hp_ctrl = data;
>>>>+
>>>>+	pci_free_irq_vectors(hp_ctrl->pdev);
>>>>+	flush_work(&hp_ctrl->work);
>>>>+}
>>>>+
>>>>+static int octep_hp_request_irq(struct octep_hp_controller *hp_ctrl,
>>>>+				enum octep_hp_intr_type type)
>>>>+{
>>>>+	struct pci_dev *pdev = hp_ctrl->pdev;
>>>>+	struct octep_hp_intr_info *intr;
>>>>+	int irq;
>>>>+
>>>>+	irq = pci_irq_vector(pdev, OCTEP_HP_INTR_VECTOR(type));
>>>>+	if (irq < 0)
>>>>+		return irq;
>>>>+
>>>>+	intr = &hp_ctrl->intr[type];
>>>>+	intr->number = irq;
>>>>+	intr->type = type;
>>>>+	snprintf(intr->name, sizeof(intr->name), "octep_hp_%d", type);
>>>>+
>>>>+	return devm_request_irq(&pdev->dev, irq, octep_hp_intr_handler,
>>>>+				IRQF_SHARED, intr->name, hp_ctrl);
>>>>+}
>>>>+
>>>>+static int octep_hp_controller_setup(struct pci_dev *pdev,
>>>>+				     struct octep_hp_controller *hp_ctrl)
>>>>+{
>>>>+	struct device *dev = &pdev->dev;
>>>>+	enum octep_hp_intr_type type;
>>>>+	int ret;
>>>>+
>>>>+	ret = pcim_enable_device(pdev);
>>>>+	if (ret)
>>>>+		return dev_err_probe(dev, ret, "Failed to enable PCI device\n");
>>>>+
>>>>+	hp_ctrl->base = pcim_iomap_region(pdev, 0, OCTEP_HP_DRV_NAME);
>>>>+	if (IS_ERR(hp_ctrl->base))
>>>>+		return dev_err_probe(dev, PTR_ERR(hp_ctrl->base),
>>>>+				     "Failed to map PCI device region\n");
>>>>+
>>>>+	pci_set_master(pdev);
>>>>+	pci_set_drvdata(pdev, hp_ctrl);
>>>>+
>>>>+	INIT_LIST_HEAD(&hp_ctrl->slot_list);
>>>>+	INIT_LIST_HEAD(&hp_ctrl->hp_cmd_list);
>>>>+	mutex_init(&hp_ctrl->slot_lock);
>>>>+	spin_lock_init(&hp_ctrl->hp_cmd_lock);
>>>>+	INIT_WORK(&hp_ctrl->work, octep_hp_work_handler);
>>>>+	hp_ctrl->pdev = pdev;
>>>>+
>>>>+	ret = pci_alloc_irq_vectors(pdev, 1,
>>>>+
>>>>OCTEP_HP_INTR_VECTOR(OCTEP_HP_INTR_MAX),
>>>>+				    PCI_IRQ_MSIX);
>>>>+	if (ret < 0)
>>>>+		return dev_err_probe(dev, ret, "Failed to alloc MSI-X
>>>>vectors\n");
>>>>+
>>>>+	ret = devm_add_action(&pdev->dev, octep_hp_irq_cleanup, hp_ctrl);
>>>>+	if (ret)
>>>>+		return dev_err_probe(&pdev->dev, ret, "Failed to add IRQ
>>>>cleanup action\n");
>>>>+
>>>>+	for (type = OCTEP_HP_INTR_ENA; type < OCTEP_HP_INTR_MAX;
>>>>type++) {
>>>>+		ret = octep_hp_request_irq(hp_ctrl, type);
>>>>+		if (ret)
>>>>+			return dev_err_probe(dev, ret,
>>>>+					     "Failed to request IRQ for vector
>>>>%d\n",
>>>>+					     OCTEP_HP_INTR_VECTOR(type));
>>>>+	}
>>>>+
>>>>+	return 0;
>>>>+}
>>>>+
>>>>+static int octep_hp_pci_probe(struct pci_dev *pdev,
>>>>+			      const struct pci_device_id *id)
>>>>+{
>>>>+	struct octep_hp_controller *hp_ctrl;
>>>>+	struct pci_dev *tmp_pdev = NULL;
>>>>+	struct octep_hp_slot *hp_slot;
>>>>+	u16 slot_number = 0;
>>>>+	int ret;
>>>>+
>>>>+	hp_ctrl = devm_kzalloc(&pdev->dev, sizeof(*hp_ctrl), GFP_KERNEL);
>>>>+	if (!hp_ctrl)
>>>>+		return -ENOMEM;
>>>>+
>>>>+	ret = octep_hp_controller_setup(pdev, hp_ctrl);
>>>>+	if (ret)
>>>>+		return ret;
>>>>+
>>>>+	/*
>>>>+	 * Register all hotplug slots. Hotplug controller is the first function
>>>>+	 * of the PCI device. The hotplug slots are the remaining functions of
>>>>+	 * the PCI device. They are removed from the bus and are added back
>>>>when
>>>>+	 * the hotplug event is triggered.
>>>>+	 */
>>>>+	for_each_pci_dev(tmp_pdev) {
>>>>+		if (!octep_hp_is_slot(hp_ctrl, tmp_pdev))
>>>>+			continue;
>>>>+
>>>>+		hp_slot = octep_hp_register_slot(hp_ctrl, tmp_pdev,
>>>>slot_number);
>>>>+		if (IS_ERR(hp_slot))
>>>>+			return dev_err_probe(&pdev->dev, PTR_ERR(hp_slot),
>>>>+					     "Failed to register hotplug slot
>>>>%u\n",
>>>>+					     slot_number);
>>>>+
>>>>+		ret = devm_add_action(&pdev->dev, octep_hp_deregister_slot,
>>>>+				      hp_slot);
>>>>+		if (ret)
>>>>+			return dev_err_probe(&pdev->dev, ret,
>>>>+					     "Failed to add action for
>>>>deregistering slot %u\n",
>>>>+					     slot_number);
>>>>+		slot_number++;
>>>>+	}
>>>>+
>>>>+	return 0;
>>>>+}
>>>>+
>>>>+#define OCTEP_DEVID_HP_CONTROLLER 0xa0e3
>>>>+static struct pci_device_id octep_hp_pci_map[] = {
>>>>+	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM,
>>>>OCTEP_DEVID_HP_CONTROLLER) },
>>>>+	{ },
>>>>+};
>>>>+
>>>>+static struct pci_driver octep_hp = {
>>>>+	.name = OCTEP_HP_DRV_NAME,
>>>>+	.id_table = octep_hp_pci_map,
>>>>+	.probe = octep_hp_pci_probe,
>>>>+};
>>>>+
>>>>+module_pci_driver(octep_hp);
>>>>+
>>>>+MODULE_LICENSE("GPL");
>>>>+MODULE_AUTHOR("Marvell");
>>>>+MODULE_DESCRIPTION("OCTEON PCIe device hotplug controller driver");
>>>>--
>>>>2.25.1
>>>

Shijith Thotton Nov. 6, 2024, 7:45 a.m. UTC | #5

Just a gentle reminder to review the patch. Let me know if any adjustments are
needed. Thanks!

>
>This patch introduces a PCI hotplug controller driver for the OCTEON
>PCIe device, a multi-function PCIe device where the first function acts
>as a hotplug controller. It is equipped with MSI-x interrupts to notify
>the host of hotplug events from the OCTEON firmware.
>
>The driver facilitates the hotplugging of non-controller functions
>within the same device. During probe, non-controller functions are
>removed and registered as PCI hotplug slots. The slots are added back
>only upon request from the device firmware. The driver also allows the
>enabling and disabling of the slots via sysfs slot entries, provided by
>the PCI hotplug framework.
>
>Signed-off-by: Shijith Thotton <[e-mail]>
>Co-developed-by: Vamsi Attunuru <[e-mail]>
>Signed-off-by: Vamsi Attunuru <[e-mail]>
>---
>
>This patch introduces a PCI hotplug controller driver for OCTEON PCIe hotplug
>controller. The OCTEON PCIe device is a multi-function device where the first
>function acts as a PCI hotplug controller.
>
>              +--------------------------------+
>              |           Root Port            |
>              +--------------------------------+
>                              |
>                             PCIe
>                              |
>+---------------------------------------------------------------+
>|              OCTEON PCIe Multifunction Device                 |
>+---------------------------------------------------------------+
>            |                    |              |            |
>            |                    |              |            |
>+---------------------+  +----------------+  +-----+  +----------------+
>|      Function 0     |  |   Function 1   |  | ... |  |   Function 7   |
>| (Hotplug controller)|  | (Hotplug slot) |  |     |  | (Hotplug slot) |
>+---------------------+  +----------------+  +-----+  +----------------+
>            |
>            |
>+-------------------------+
>|   Controller Firmware   |
>+-------------------------+
>
>The hotplug controller driver facilitates the hotplugging of non-controller
>functions in the same device. During the probe of the driver, the non-controller
>function are removed and registered as PCI hotplug slots. They are added back
>only upon request from the device firmware. The driver also allows the user to
>enable/disable the functions using sysfs slot entries provided by PCI hotplug
>framework.
>
>This solution adopts a hardware + software approach for several reasons:
>
>1. To reduce hardware implementation cost. Supporting complete hotplug
>   capability within the card would require a PCI switch implemented within.
>
>2. In the multi-function device, non-controller functions can act as emulated
>   devices. The firmware can dynamically enable or disable them at runtime.
>
>3. Not all root ports support PCI hotplug. This approach provides greater
>   flexibility and compatibility across different hardware configurations.
>
>The hotplug controller function is lightweight and is equipped with MSI-x
>interrupts to notify the host about hotplug events. Upon receiving an
>interrupt, the hotplug register is read, and the required function is enabled
>or disabled.
>
>This driver will be beneficial for managing PCI hotplug events on the OCTEON
>PCIe device without requiring complex hardware solutions.
>
>Changes in v2:
>- Added missing include files.
>- Used dev_err_probe() for error handling.
>- Used guard() for mutex locking.
>- Splited cleanup actions and added per-slot cleanup action.
>- Fixed coding style issues.
>- Added co-developed-by tag.
>
>Changes in v3:
>- Explicit assignment of enum values.
>- Use pcim_iomap_region() instead of pcim_iomap_regions().
>
> MAINTAINERS                    |   6 +
> drivers/pci/hotplug/Kconfig    |  10 +
> drivers/pci/hotplug/Makefile   |   1 +
> drivers/pci/hotplug/octep_hp.c | 409
>+++++++++++++++++++++++++++++++++
> 4 files changed, 426 insertions(+)
> create mode 100644 drivers/pci/hotplug/octep_hp.c
>
>diff --git a/MAINTAINERS b/MAINTAINERS
>index 42decde38320..7b5a618eed1c 100644
>--- a/MAINTAINERS
>+++ b/MAINTAINERS
>@@ -13677,6 +13677,12 @@ R:	[e-mail]
> R:	[e-mail]
> F:	drivers/vdpa/octeon_ep/
>
>+MARVELL OCTEON HOTPLUG CONTROLLER DRIVER
>+R:	Shijith Thotton <[e-mail]>
>+R:	Vamsi Attunuru <[e-mail]>
>+S:	Supported
>+F:	drivers/pci/hotplug/octep_hp.c
>+
> MATROX FRAMEBUFFER DRIVER
> L:	[e-mail]
> S:	Orphan
>diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
>index 1472aef0fb81..2e38fd25f7ef 100644
>--- a/drivers/pci/hotplug/Kconfig
>+++ b/drivers/pci/hotplug/Kconfig
>@@ -173,4 +173,14 @@ config HOTPLUG_PCI_S390
>
> 	  When in doubt, say Y.
>
>+config HOTPLUG_PCI_OCTEONEP
>+	bool "OCTEON PCI device Hotplug controller driver"
>+	depends on HOTPLUG_PCI
>+	help
>+	  Say Y here if you have an OCTEON PCIe device with a hotplug
>+	  controller. This driver enables the non-controller functions of the
>+	  device to be registered as hotplug slots.
>+
>+	  When in doubt, say N.
>+
> endif # HOTPLUG_PCI
>diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile
>index 240c99517d5e..40aaf31fe338 100644
>--- a/drivers/pci/hotplug/Makefile
>+++ b/drivers/pci/hotplug/Makefile
>@@ -20,6 +20,7 @@ obj-$(CONFIG_HOTPLUG_PCI_RPA)		+=
>rpaphp.o
> obj-$(CONFIG_HOTPLUG_PCI_RPA_DLPAR)	+= rpadlpar_io.o
> obj-$(CONFIG_HOTPLUG_PCI_ACPI)		+= acpiphp.o
> obj-$(CONFIG_HOTPLUG_PCI_S390)		+= s390_pci_hpc.o
>+obj-$(CONFIG_HOTPLUG_PCI_OCTEONEP)	+= octep_hp.o
>
> # acpiphp_ibm extends acpiphp, so should be linked afterwards.
>
>diff --git a/drivers/pci/hotplug/octep_hp.c b/drivers/pci/hotplug/octep_hp.c
>new file mode 100644
>index 000000000000..77dc2740f43e
>--- /dev/null
>+++ b/drivers/pci/hotplug/octep_hp.c
>@@ -0,0 +1,409 @@
>+// SPDX-License-Identifier: GPL-2.0-only
>+/* Copyright (C) 2024 Marvell. */
>+
>+#include <linux/cleanup.h>
>+#include <linux/container_of.h>
>+#include <linux/delay.h>
>+#include <linux/dev_printk.h>
>+#include <linux/init.h>
>+#include <linux/interrupt.h>
>+#include <linux/io-64-nonatomic-lo-hi.h>
>+#include <linux/kernel.h>
>+#include <linux/list.h>
>+#include <linux/module.h>
>+#include <linux/mutex.h>
>+#include <linux/pci.h>
>+#include <linux/pci_hotplug.h>
>+#include <linux/slab.h>
>+#include <linux/spinlock.h>
>+#include <linux/workqueue.h>
>+
>+#define OCTEP_HP_INTR_OFFSET(x) (0x20400 + ((x) << 4))
>+#define OCTEP_HP_INTR_VECTOR(x) (16 + (x))
>+#define OCTEP_HP_DRV_NAME "octep_hp"
>+
>+/*
>+ * Type of MSI-X interrupts.
>+ * The macros OCTEP_HP_INTR_VECTOR and OCTEP_HP_INTR_OFFSET are
>used to
>+ * generate the vector and offset for an interrupt type.
>+ */
>+enum octep_hp_intr_type {
>+	OCTEP_HP_INTR_INVALID = -1,
>+	OCTEP_HP_INTR_ENA = 0,
>+	OCTEP_HP_INTR_DIS = 1,
>+	OCTEP_HP_INTR_MAX = 2,
>+};
>+
>+struct octep_hp_cmd {
>+	struct list_head list;
>+	enum octep_hp_intr_type intr_type;
>+	u64 intr_val;
>+};
>+
>+struct octep_hp_slot {
>+	struct list_head list;
>+	struct hotplug_slot slot;
>+	u16 slot_number;
>+	struct pci_dev *hp_pdev;
>+	unsigned int hp_devfn;
>+	struct octep_hp_controller *ctrl;
>+};
>+
>+struct octep_hp_intr_info {
>+	enum octep_hp_intr_type type;
>+	int number;
>+	char name[16];
>+};
>+
>+struct octep_hp_controller {
>+	void __iomem *base;
>+	struct pci_dev *pdev;
>+	struct octep_hp_intr_info intr[OCTEP_HP_INTR_MAX];
>+	struct work_struct work;
>+	struct list_head slot_list;
>+	struct mutex slot_lock; /* Protects slot_list */
>+	struct list_head hp_cmd_list;
>+	spinlock_t hp_cmd_lock; /* Protects hp_cmd_list */
>+};
>+
>+static void octep_hp_enable_pdev(struct octep_hp_controller *hp_ctrl,
>+				 struct octep_hp_slot *hp_slot)
>+{
>+	guard(mutex)(&hp_ctrl->slot_lock);
>+	if (hp_slot->hp_pdev) {
>+		dev_dbg(&hp_slot->hp_pdev->dev, "Slot %u already
>enabled\n",
>+			hp_slot->slot_number);
>+		return;
>+	}
>+
>+	/* Scan the device and add it to the bus */
>+	hp_slot->hp_pdev = pci_scan_single_device(hp_ctrl->pdev->bus,
>+						  hp_slot->hp_devfn);
>+	pci_bus_assign_resources(hp_ctrl->pdev->bus);
>+	pci_bus_add_device(hp_slot->hp_pdev);
>+
>+	dev_dbg(&hp_slot->hp_pdev->dev, "Enabled slot %u\n",
>+		hp_slot->slot_number);
>+}
>+
>+static void octep_hp_disable_pdev(struct octep_hp_controller *hp_ctrl,
>+				  struct octep_hp_slot *hp_slot)
>+{
>+	guard(mutex)(&hp_ctrl->slot_lock);
>+	if (!hp_slot->hp_pdev) {
>+		dev_dbg(&hp_ctrl->pdev->dev, "Slot %u already disabled\n",
>+			hp_slot->slot_number);
>+		return;
>+	}
>+
>+	dev_dbg(&hp_slot->hp_pdev->dev, "Disabling slot %u\n",
>+		hp_slot->slot_number);
>+
>+	/* Remove the device from the bus */
>+	pci_stop_and_remove_bus_device_locked(hp_slot->hp_pdev);
>+	hp_slot->hp_pdev = NULL;
>+}
>+
>+static int octep_hp_enable_slot(struct hotplug_slot *slot)
>+{
>+	struct octep_hp_slot *hp_slot =
>+		container_of(slot, struct octep_hp_slot, slot);
>+
>+	octep_hp_enable_pdev(hp_slot->ctrl, hp_slot);
>+	return 0;
>+}
>+
>+static int octep_hp_disable_slot(struct hotplug_slot *slot)
>+{
>+	struct octep_hp_slot *hp_slot =
>+		container_of(slot, struct octep_hp_slot, slot);
>+
>+	octep_hp_disable_pdev(hp_slot->ctrl, hp_slot);
>+	return 0;
>+}
>+
>+static struct hotplug_slot_ops octep_hp_slot_ops = {
>+	.enable_slot = octep_hp_enable_slot,
>+	.disable_slot = octep_hp_disable_slot,
>+};
>+
>+#define SLOT_NAME_SIZE 16
>+static struct octep_hp_slot *
>+octep_hp_register_slot(struct octep_hp_controller *hp_ctrl,
>+		       struct pci_dev *pdev, u16 slot_number)
>+{
>+	char slot_name[SLOT_NAME_SIZE];
>+	struct octep_hp_slot *hp_slot;
>+	int ret;
>+
>+	hp_slot = kzalloc(sizeof(*hp_slot), GFP_KERNEL);
>+	if (!hp_slot)
>+		return ERR_PTR(-ENOMEM);
>+
>+	hp_slot->ctrl = hp_ctrl;
>+	hp_slot->hp_pdev = pdev;
>+	hp_slot->hp_devfn = pdev->devfn;
>+	hp_slot->slot_number = slot_number;
>+	hp_slot->slot.ops = &octep_hp_slot_ops;
>+
>+	snprintf(slot_name, sizeof(slot_name), "octep_hp_%u", slot_number);
>+	ret = pci_hp_register(&hp_slot->slot, hp_ctrl->pdev->bus,
>+			      PCI_SLOT(pdev->devfn), slot_name);
>+	if (ret) {
>+		kfree(hp_slot);
>+		return ERR_PTR(ret);
>+	}
>+
>+	list_add_tail(&hp_slot->list, &hp_ctrl->slot_list);
>+	octep_hp_disable_pdev(hp_ctrl, hp_slot);
>+
>+	return hp_slot;
>+}
>+
>+static void octep_hp_deregister_slot(void *data)
>+{
>+	struct octep_hp_slot *hp_slot = data;
>+	struct octep_hp_controller *hp_ctrl = hp_slot->ctrl;
>+
>+	pci_hp_deregister(&hp_slot->slot);
>+	octep_hp_enable_pdev(hp_ctrl, hp_slot);
>+	list_del(&hp_slot->list);
>+	kfree(hp_slot);
>+}
>+
>+static bool octep_hp_is_slot(struct octep_hp_controller *hp_ctrl,
>+			     struct pci_dev *pdev)
>+{
>+	/* Check if the PCI device can be hotplugged */
>+	return pdev != hp_ctrl->pdev && pdev->bus == hp_ctrl->pdev->bus &&
>+	       PCI_SLOT(pdev->devfn) == PCI_SLOT(hp_ctrl->pdev->devfn);
>+}
>+
>+static void octep_hp_cmd_handler(struct octep_hp_controller *hp_ctrl,
>+				 struct octep_hp_cmd *hp_cmd)
>+{
>+	struct octep_hp_slot *hp_slot;
>+
>+	/*
>+	 * Enable or disable the slots based on the slot mask.
>+	 * intr_val is a bit mask where each bit represents a slot.
>+	 */
>+	list_for_each_entry(hp_slot, &hp_ctrl->slot_list, list) {
>+		if (!(hp_cmd->intr_val & BIT(hp_slot->slot_number)))
>+			continue;
>+
>+		if (hp_cmd->intr_type == OCTEP_HP_INTR_ENA)
>+			octep_hp_enable_pdev(hp_ctrl, hp_slot);
>+		else
>+			octep_hp_disable_pdev(hp_ctrl, hp_slot);
>+	}
>+}
>+
>+static void octep_hp_work_handler(struct work_struct *work)
>+{
>+	struct octep_hp_controller *hp_ctrl;
>+	struct octep_hp_cmd *hp_cmd;
>+	unsigned long flags;
>+
>+	hp_ctrl = container_of(work, struct octep_hp_controller, work);
>+
>+	/* Process all the hotplug commands */
>+	spin_lock_irqsave(&hp_ctrl->hp_cmd_lock, flags);
>+	while (!list_empty(&hp_ctrl->hp_cmd_list)) {
>+		hp_cmd = list_first_entry(&hp_ctrl->hp_cmd_list,
>+					  struct octep_hp_cmd, list);
>+		list_del(&hp_cmd->list);
>+		spin_unlock_irqrestore(&hp_ctrl->hp_cmd_lock, flags);
>+
>+		octep_hp_cmd_handler(hp_ctrl, hp_cmd);
>+		kfree(hp_cmd);
>+
>+		spin_lock_irqsave(&hp_ctrl->hp_cmd_lock, flags);
>+	}
>+	spin_unlock_irqrestore(&hp_ctrl->hp_cmd_lock, flags);
>+}
>+
>+static enum octep_hp_intr_type octep_hp_intr_type(struct
>octep_hp_intr_info *intr,
>+						  int irq)
>+{
>+	enum octep_hp_intr_type type;
>+
>+	for (type = OCTEP_HP_INTR_ENA; type < OCTEP_HP_INTR_MAX;
>type++) {
>+		if (intr[type].number == irq)
>+			return type;
>+	}
>+
>+	return OCTEP_HP_INTR_INVALID;
>+}
>+
>+static irqreturn_t octep_hp_intr_handler(int irq, void *data)
>+{
>+	struct octep_hp_controller *hp_ctrl = data;
>+	struct pci_dev *pdev = hp_ctrl->pdev;
>+	enum octep_hp_intr_type type;
>+	struct octep_hp_cmd *hp_cmd;
>+	u64 intr_val;
>+
>+	type = octep_hp_intr_type(hp_ctrl->intr, irq);
>+	if (type == OCTEP_HP_INTR_INVALID) {
>+		dev_err(&pdev->dev, "Invalid interrupt %d\n", irq);
>+		return IRQ_HANDLED;
>+	}
>+
>+	/* Read and clear the interrupt */
>+	intr_val = readq(hp_ctrl->base + OCTEP_HP_INTR_OFFSET(type));
>+	writeq(intr_val, hp_ctrl->base + OCTEP_HP_INTR_OFFSET(type));
>+
>+	hp_cmd = kzalloc(sizeof(*hp_cmd), GFP_ATOMIC);
>+	if (!hp_cmd)
>+		return IRQ_HANDLED;
>+
>+	hp_cmd->intr_val = intr_val;
>+	hp_cmd->intr_type = type;
>+
>+	/* Add the command to the list and schedule the work */
>+	spin_lock(&hp_ctrl->hp_cmd_lock);
>+	list_add_tail(&hp_cmd->list, &hp_ctrl->hp_cmd_list);
>+	spin_unlock(&hp_ctrl->hp_cmd_lock);
>+	schedule_work(&hp_ctrl->work);
>+
>+	return IRQ_HANDLED;
>+}
>+
>+static void octep_hp_irq_cleanup(void *data)
>+{
>+	struct octep_hp_controller *hp_ctrl = data;
>+
>+	pci_free_irq_vectors(hp_ctrl->pdev);
>+	flush_work(&hp_ctrl->work);
>+}
>+
>+static int octep_hp_request_irq(struct octep_hp_controller *hp_ctrl,
>+				enum octep_hp_intr_type type)
>+{
>+	struct pci_dev *pdev = hp_ctrl->pdev;
>+	struct octep_hp_intr_info *intr;
>+	int irq;
>+
>+	irq = pci_irq_vector(pdev, OCTEP_HP_INTR_VECTOR(type));
>+	if (irq < 0)
>+		return irq;
>+
>+	intr = &hp_ctrl->intr[type];
>+	intr->number = irq;
>+	intr->type = type;
>+	snprintf(intr->name, sizeof(intr->name), "octep_hp_%d", type);
>+
>+	return devm_request_irq(&pdev->dev, irq, octep_hp_intr_handler,
>+				IRQF_SHARED, intr->name, hp_ctrl);
>+}
>+
>+static int octep_hp_controller_setup(struct pci_dev *pdev,
>+				     struct octep_hp_controller *hp_ctrl)
>+{
>+	struct device *dev = &pdev->dev;
>+	enum octep_hp_intr_type type;
>+	int ret;
>+
>+	ret = pcim_enable_device(pdev);
>+	if (ret)
>+		return dev_err_probe(dev, ret, "Failed to enable PCI device\n");
>+
>+	hp_ctrl->base = pcim_iomap_region(pdev, 0, OCTEP_HP_DRV_NAME);
>+	if (IS_ERR(hp_ctrl->base))
>+		return dev_err_probe(dev, PTR_ERR(hp_ctrl->base),
>+				     "Failed to map PCI device region\n");
>+
>+	pci_set_master(pdev);
>+	pci_set_drvdata(pdev, hp_ctrl);
>+
>+	INIT_LIST_HEAD(&hp_ctrl->slot_list);
>+	INIT_LIST_HEAD(&hp_ctrl->hp_cmd_list);
>+	mutex_init(&hp_ctrl->slot_lock);
>+	spin_lock_init(&hp_ctrl->hp_cmd_lock);
>+	INIT_WORK(&hp_ctrl->work, octep_hp_work_handler);
>+	hp_ctrl->pdev = pdev;
>+
>+	ret = pci_alloc_irq_vectors(pdev, 1,
>+
>OCTEP_HP_INTR_VECTOR(OCTEP_HP_INTR_MAX),
>+				    PCI_IRQ_MSIX);
>+	if (ret < 0)
>+		return dev_err_probe(dev, ret, "Failed to alloc MSI-X
>vectors\n");
>+
>+	ret = devm_add_action(&pdev->dev, octep_hp_irq_cleanup, hp_ctrl);
>+	if (ret)
>+		return dev_err_probe(&pdev->dev, ret, "Failed to add IRQ
>cleanup action\n");
>+
>+	for (type = OCTEP_HP_INTR_ENA; type < OCTEP_HP_INTR_MAX;
>type++) {
>+		ret = octep_hp_request_irq(hp_ctrl, type);
>+		if (ret)
>+			return dev_err_probe(dev, ret,
>+					     "Failed to request IRQ for vector
>%d\n",
>+					     OCTEP_HP_INTR_VECTOR(type));
>+	}
>+
>+	return 0;
>+}
>+
>+static int octep_hp_pci_probe(struct pci_dev *pdev,
>+			      const struct pci_device_id *id)
>+{
>+	struct octep_hp_controller *hp_ctrl;
>+	struct pci_dev *tmp_pdev = NULL;
>+	struct octep_hp_slot *hp_slot;
>+	u16 slot_number = 0;
>+	int ret;
>+
>+	hp_ctrl = devm_kzalloc(&pdev->dev, sizeof(*hp_ctrl), GFP_KERNEL);
>+	if (!hp_ctrl)
>+		return -ENOMEM;
>+
>+	ret = octep_hp_controller_setup(pdev, hp_ctrl);
>+	if (ret)
>+		return ret;
>+
>+	/*
>+	 * Register all hotplug slots. Hotplug controller is the first function
>+	 * of the PCI device. The hotplug slots are the remaining functions of
>+	 * the PCI device. They are removed from the bus and are added back
>when
>+	 * the hotplug event is triggered.
>+	 */
>+	for_each_pci_dev(tmp_pdev) {
>+		if (!octep_hp_is_slot(hp_ctrl, tmp_pdev))
>+			continue;
>+
>+		hp_slot = octep_hp_register_slot(hp_ctrl, tmp_pdev,
>slot_number);
>+		if (IS_ERR(hp_slot))
>+			return dev_err_probe(&pdev->dev, PTR_ERR(hp_slot),
>+					     "Failed to register hotplug slot
>%u\n",
>+					     slot_number);
>+
>+		ret = devm_add_action(&pdev->dev, octep_hp_deregister_slot,
>+				      hp_slot);
>+		if (ret)
>+			return dev_err_probe(&pdev->dev, ret,
>+					     "Failed to add action for
>deregistering slot %u\n",
>+					     slot_number);
>+		slot_number++;
>+	}
>+
>+	return 0;
>+}
>+
>+#define OCTEP_DEVID_HP_CONTROLLER 0xa0e3
>+static struct pci_device_id octep_hp_pci_map[] = {
>+	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM,
>OCTEP_DEVID_HP_CONTROLLER) },
>+	{ },
>+};
>+
>+static struct pci_driver octep_hp = {
>+	.name = OCTEP_HP_DRV_NAME,
>+	.id_table = octep_hp_pci_map,
>+	.probe = octep_hp_pci_probe,
>+};
>+
>+module_pci_driver(octep_hp);
>+
>+MODULE_LICENSE("GPL");
>+MODULE_AUTHOR("Marvell");
>+MODULE_DESCRIPTION("OCTEON PCIe device hotplug controller driver");
>--
>2.25.1

Bjorn Helgaas Nov. 7, 2024, 8:32 p.m. UTC | #6

On Mon, Aug 26, 2024 at 04:15:03PM +0530, Shijith Thotton wrote:
> This patch introduces a PCI hotplug controller driver for the OCTEON
> PCIe device, a multi-function PCIe device where the first function acts
> as a hotplug controller. It is equipped with MSI-x interrupts to notify
> the host of hotplug events from the OCTEON firmware.

s/MSI-x/MSI-X/ to match spec and other uses

> The driver facilitates the hotplugging of non-controller functions
> within the same device. During probe, non-controller functions are
> removed and registered as PCI hotplug slots. The slots are added back
> only upon request from the device firmware. The driver also allows the
> enabling and disabling of the slots via sysfs slot entries, provided by
> the PCI hotplug framework.
> 
> Signed-off-by: Shijith Thotton <[e-mail]>
> Co-developed-by: Vamsi Attunuru <[e-mail]>
> Signed-off-by: Vamsi Attunuru <[e-mail]>

This order is incorrect because the handler (Shijith in this case)
should be last in the chain; see
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.11#n396

> This patch introduces a PCI hotplug controller driver for OCTEON PCIe hotplug
> controller. The OCTEON PCIe device is a multi-function device where the first
> function acts as a PCI hotplug controller.
> 
>               +--------------------------------+
>               |           Root Port            |
>               +--------------------------------+
>                               |
>                              PCIe
>                               |
> +---------------------------------------------------------------+
> |              OCTEON PCIe Multifunction Device                 |
> +---------------------------------------------------------------+
>             |                    |              |            |
>             |                    |              |            |
> +---------------------+  +----------------+  +-----+  +----------------+
> |      Function 0     |  |   Function 1   |  | ... |  |   Function 7   |
> | (Hotplug controller)|  | (Hotplug slot) |  |     |  | (Hotplug slot) |
> +---------------------+  +----------------+  +-----+  +----------------+
>             |
>             |
> +-------------------------+
> |   Controller Firmware   |
> +-------------------------+
> 
> The hotplug controller driver facilitates the hotplugging of non-controller
> functions in the same device. During the probe of the driver, the non-controller
> function are removed and registered as PCI hotplug slots. They are added back
> only upon request from the device firmware. The driver also allows the user to
> enable/disable the functions using sysfs slot entries provided by PCI hotplug
> framework.

I think the diagram and this text would be useful in the commit log.

I would rephrase "functions are removed ..." as "the driver removes
functions" to make it clear that this is purely a software thing and
there's no PCIe-level change.  Similar for adding them back.

> This solution adopts a hardware + software approach for several reasons:
> 
> 1. To reduce hardware implementation cost. Supporting complete hotplug
>    capability within the card would require a PCI switch implemented within.
> 
> 2. In the multi-function device, non-controller functions can act as emulated
>    devices. The firmware can dynamically enable or disable them at runtime.
> 
> 3. Not all root ports support PCI hotplug. This approach provides greater
>    flexibility and compatibility across different hardware configurations.

The downside of all this is the need for special-purpose software,
which slows things down (you need to develop it, others need to review
it) and burdens everybody with ongoing maintenance, e.g., changes to
PCI device removal, resource assignment, slot registration, etc. now
have to consider another case.

> The hotplug controller function is lightweight and is equipped with MSI-x
> interrupts to notify the host about hotplug events. Upon receiving an
> interrupt, the hotplug register is read, and the required function is enabled
> or disabled.
> 
> This driver will be beneficial for managing PCI hotplug events on the OCTEON
> PCIe device without requiring complex hardware solutions.

> +config HOTPLUG_PCI_OCTEONEP
> +	bool "OCTEON PCI device Hotplug controller driver"

s/Marvell PCI device/Cavium OCTEON PCI/ to match other entries.

This Kconfig file isn't completely sorted, but if you move this above
SHPC, it will at least be closer.

> +static int octep_hp_pci_probe(struct pci_dev *pdev,
> +			      const struct pci_device_id *id)
> +{
> +	struct octep_hp_controller *hp_ctrl;
> +	struct pci_dev *tmp_pdev = NULL;
> +	struct octep_hp_slot *hp_slot;
> +	u16 slot_number = 0;
> +	int ret;
> +
> +	hp_ctrl = devm_kzalloc(&pdev->dev, sizeof(*hp_ctrl), GFP_KERNEL);
> +	if (!hp_ctrl)
> +		return -ENOMEM;
> +
> +	ret = octep_hp_controller_setup(pdev, hp_ctrl);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Register all hotplug slots. Hotplug controller is the first function
> +	 * of the PCI device. The hotplug slots are the remaining functions of
> +	 * the PCI device. They are removed from the bus and are added back when
> +	 * the hotplug event is triggered.

"Logically removed," I guess?  Obviously the PCIe Link remains up
since function 0 is still alive, and I assume these other functions
would still respond to config accesses.

> +	 */
> +	for_each_pci_dev(tmp_pdev) {

Ugh.  We should avoid for_each_pci_dev() when possible.

IIUC you only care about other functions of *this* device, and those
functions should all be on the bus->devices list.  There are many
instances of this, which I think would be sufficient:

  list_for_each_entry(dev, &bus->devices, bus_list)

> +		if (!octep_hp_is_slot(hp_ctrl, tmp_pdev))
> +			continue;

Which would make this check mostly unnecessary.

> +		hp_slot = octep_hp_register_slot(hp_ctrl, tmp_pdev, slot_number);
> +		if (IS_ERR(hp_slot))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(hp_slot),
> +					     "Failed to register hotplug slot %u\n",
> +					     slot_number);
> +
> +		ret = devm_add_action(&pdev->dev, octep_hp_deregister_slot,
> +				      hp_slot);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +					     "Failed to add action for deregistering slot %u\n",
> +					     slot_number);
> +		slot_number++;
> +	}

AFAICS this driver logs nothing at all in dmesg (except for errors and
a few dev_dbg() uses).  Seems like it might be useful to have some
hints there about the hotplug controller existence, possibly the
connection between the slot name used in sysfs and the PCI function,
and maybe even hot-add and hot-remove events.

> +	return 0;
> +}
> +
> +#define OCTEP_DEVID_HP_CONTROLLER 0xa0e3

Even though this is a private #define, I think something like
PCI_DEVICE_ID_CAVIUM_OCTEP_HP_CTLR would be nice because that's the
typical pattern of include/linux/pci_ids.h.

> +static struct pci_device_id octep_hp_pci_map[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, OCTEP_DEVID_HP_CONTROLLER) },
> +	{ },
> +};

Shijith Thotton Nov. 8, 2024, 12:17 p.m. UTC | #7

>> This patch introduces a PCI hotplug controller driver for the OCTEON
>> PCIe device, a multi-function PCIe device where the first function acts
>> as a hotplug controller. It is equipped with MSI-x interrupts to notify
>> the host of hotplug events from the OCTEON firmware.
>
>s/MSI-x/MSI-X/ to match spec and other uses

I will change in v4

>
>> The driver facilitates the hotplugging of non-controller functions
>> within the same device. During probe, non-controller functions are
>> removed and registered as PCI hotplug slots. The slots are added back
>> only upon request from the device firmware. The driver also allows the
>> enabling and disabling of the slots via sysfs slot entries, provided by
>> the PCI hotplug framework.
>>
>> Signed-off-by: Shijith Thotton <[e-mail]>
>> Co-developed-by: Vamsi Attunuru <[e-mail]>
>> Signed-off-by: Vamsi Attunuru <[e-mail]>
>
>This order is incorrect because the handler (Shijith in this case)
>should be last in the chain; see
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v6.11#n396
>

I will change in v4.

>> This patch introduces a PCI hotplug controller driver for OCTEON PCIe hotplug
>> controller. The OCTEON PCIe device is a multi-function device where the first
>> function acts as a PCI hotplug controller.
>>
>>               +--------------------------------+
>>               |           Root Port            |
>>               +--------------------------------+
>>                               |
>>                              PCIe
>>                               |
>> +---------------------------------------------------------------+
>> |              OCTEON PCIe Multifunction Device                 |
>> +---------------------------------------------------------------+
>>             |                    |              |            |
>>             |                    |              |            |
>> +---------------------+  +----------------+  +-----+  +----------------+
>> |      Function 0     |  |   Function 1   |  | ... |  |   Function 7   |
>> | (Hotplug controller)|  | (Hotplug slot) |  |     |  | (Hotplug slot) |
>> +---------------------+  +----------------+  +-----+  +----------------+
>>             |
>>             |
>> +-------------------------+
>> |   Controller Firmware   |
>> +-------------------------+
>>
>> The hotplug controller driver facilitates the hotplugging of non-controller
>> functions in the same device. During the probe of the driver, the non-
>controller
>> function are removed and registered as PCI hotplug slots. They are added
>back
>> only upon request from the device firmware. The driver also allows the user
>to
>> enable/disable the functions using sysfs slot entries provided by PCI hotplug
>> framework.
>
>I think the diagram and this text would be useful in the commit log.
>
>I would rephrase "functions are removed ..." as "the driver removes
>functions" to make it clear that this is purely a software thing and
>there's no PCIe-level change.  Similar for adding them back.
>

I will update the commit log with the diagram and more wordings from above.

>> This solution adopts a hardware + software approach for several reasons:
>>
>> 1. To reduce hardware implementation cost. Supporting complete hotplug
>>    capability within the card would require a PCI switch implemented within.
>>
>> 2. In the multi-function device, non-controller functions can act as emulated
>>    devices. The firmware can dynamically enable or disable them at runtime.
>>
>> 3. Not all root ports support PCI hotplug. This approach provides greater
>>    flexibility and compatibility across different hardware configurations.
>
>The downside of all this is the need for special-purpose software,
>which slows things down (you need to develop it, others need to review
>it) and burdens everybody with ongoing maintenance, e.g., changes to
>PCI device removal, resource assignment, slot registration, etc. now
>have to consider another case.
>
>> The hotplug controller function is lightweight and is equipped with MSI-x
>> interrupts to notify the host about hotplug events. Upon receiving an
>> interrupt, the hotplug register is read, and the required function is enabled
>> or disabled.
>>
>> This driver will be beneficial for managing PCI hotplug events on the OCTEON
>> PCIe device without requiring complex hardware solutions.
>
>> +config HOTPLUG_PCI_OCTEONEP
>> +	bool "OCTEON PCI device Hotplug controller driver"
>
>s/Marvell PCI device/Cavium OCTEON PCI/ to match other entries.
>

Cavium was acquired by Marvell, but we are still using the PCI vendor ID  
`PCI_VENDOR_ID_CAVIUM`. I hope using Marvell is acceptable; please let me  
know if this poses any issues.

>This Kconfig file isn't completely sorted, but if you move this above
>SHPC, it will at least be closer.
>

I will move above SHPC.

>> +static int octep_hp_pci_probe(struct pci_dev *pdev,
>> +			      const struct pci_device_id *id)
>> +{
>> +	struct octep_hp_controller *hp_ctrl;
>> +	struct pci_dev *tmp_pdev = NULL;
>> +	struct octep_hp_slot *hp_slot;
>> +	u16 slot_number = 0;
>> +	int ret;
>> +
>> +	hp_ctrl = devm_kzalloc(&pdev->dev, sizeof(*hp_ctrl), GFP_KERNEL);
>> +	if (!hp_ctrl)
>> +		return -ENOMEM;
>> +
>> +	ret = octep_hp_controller_setup(pdev, hp_ctrl);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/*
>> +	 * Register all hotplug slots. Hotplug controller is the first function
>> +	 * of the PCI device. The hotplug slots are the remaining functions of
>> +	 * the PCI device. They are removed from the bus and are added back
>when
>> +	 * the hotplug event is triggered.
>
>"Logically removed," I guess?  Obviously the PCIe Link remains up
>since function 0 is still alive, and I assume these other functions
>would still respond to config accesses.
>

Yes, it's just a software removal. I will update the text.

>> +	 */
>> +	for_each_pci_dev(tmp_pdev) {
>
>Ugh.  We should avoid for_each_pci_dev() when possible.
>
>IIUC you only care about other functions of *this* device, and those
>functions should all be on the bus->devices list.  There are many
>instances of this, which I think would be sufficient:
>
>  list_for_each_entry(dev, &bus->devices, bus_list)
>

Okay. I will replace for_each_pci_dev() with list_for_each_entry_safe().

>> +		if (!octep_hp_is_slot(hp_ctrl, tmp_pdev))
>> +			continue;
>
>Which would make this check mostly unnecessary.

Yes, I will replace it with the check below to skip the controller function.
  if (tmp_pdev == pdev)
      continue;

>
>> +		hp_slot = octep_hp_register_slot(hp_ctrl, tmp_pdev,
>slot_number);
>> +		if (IS_ERR(hp_slot))
>> +			return dev_err_probe(&pdev->dev, PTR_ERR(hp_slot),
>> +					     "Failed to register hotplug slot
>%u\n",
>> +					     slot_number);
>> +
>> +		ret = devm_add_action(&pdev->dev, octep_hp_deregister_slot,
>> +				      hp_slot);
>> +		if (ret)
>> +			return dev_err_probe(&pdev->dev, ret,
>> +					     "Failed to add action for
>deregistering slot %u\n",
>> +					     slot_number);
>> +		slot_number++;
>> +	}
>
>AFAICS this driver logs nothing at all in dmesg (except for errors and
>a few dev_dbg() uses).  Seems like it might be useful to have some
>hints there about the hotplug controller existence, possibly the
>connection between the slot name used in sysfs and the PCI function,
>and maybe even hot-add and hot-remove events.
>

Okay. I’ll add more informative prints as mentioned above.

>> +	return 0;
>> +}
>> +
>> +#define OCTEP_DEVID_HP_CONTROLLER 0xa0e3
>
>Even though this is a private #define, I think something like
>PCI_DEVICE_ID_CAVIUM_OCTEP_HP_CTLR would be nice because that's the
>typical pattern of include/linux/pci_ids.h.
>

The same reason mentioned above for not using the name CAVIUM.
Is it  okay to use `PCI_DEVICE_ID_MARVELL_OCTEP_HP_CTLR`?

>> +static struct pci_device_id octep_hp_pci_map[] = {
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM,
>OCTEP_DEVID_HP_CONTROLLER) },
>> +	{ },
>> +};

Thanks,
Shijith

Bjorn Helgaas Nov. 8, 2024, 12:39 p.m. UTC | #8

On Fri, Nov 08, 2024 at 12:17:22PM +0000, Shijith Thotton wrote:
> >> This patch introduces a PCI hotplug controller driver for the OCTEON
> >> PCIe device, a multi-function PCIe device where the first function acts
> >> as a hotplug controller. It is equipped with MSI-x interrupts to notify
> >> the host of hotplug events from the OCTEON firmware.

> >> +config HOTPLUG_PCI_OCTEONEP
> >> +	bool "OCTEON PCI device Hotplug controller driver"
> >
> >s/Marvell PCI device/Cavium OCTEON PCI/ to match other entries.
> 
> Cavium was acquired by Marvell, but we are still using the PCI
> vendor ID  `PCI_VENDOR_ID_CAVIUM`. I hope using Marvell is
> acceptable; please let me  know if this poses any issues.

Oops, sorry, this was my mistake.  I meant to suggest "Marvell OCTEON
PCI Hotplug driver", but I messed up the editing.

> >> +#define OCTEP_DEVID_HP_CONTROLLER 0xa0e3
> >
> >Even though this is a private #define, I think something like
> >PCI_DEVICE_ID_CAVIUM_OCTEP_HP_CTLR would be nice because that's the
> >typical pattern of include/linux/pci_ids.h.
> 
> The same reason mentioned above for not using the name CAVIUM.
> Is it okay to use `PCI_DEVICE_ID_MARVELL_OCTEP_HP_CTLR`?

In this case, I think you should use PCI_DEVICE_ID_CAVIUM... because
the PCI_VENDOR_ID_CAVIUM identifies the Device ID namespace, so the
#defines for the Vendor ID and the Device IDs in that namespace should
match.

> >> +static struct pci_device_id octep_hp_pci_map[] = {
> >> +	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM,
> >OCTEP_DEVID_HP_CONTROLLER) },
> >> +	{ },
> >> +};

Shijith Thotton Nov. 10, 2024, 5:20 a.m. UTC | #9

>> >> This patch introduces a PCI hotplug controller driver for the OCTEON
>> >> PCIe device, a multi-function PCIe device where the first function acts
>> >> as a hotplug controller. It is equipped with MSI-x interrupts to notify
>> >> the host of hotplug events from the OCTEON firmware.
>
>> >> +config HOTPLUG_PCI_OCTEONEP
>> >> +	bool "OCTEON PCI device Hotplug controller driver"
>> >
>> >s/Marvell PCI device/Cavium OCTEON PCI/ to match other entries.
>>
>> Cavium was acquired by Marvell, but we are still using the PCI
>> vendor ID  `PCI_VENDOR_ID_CAVIUM`. I hope using Marvell is
>> acceptable; please let me  know if this poses any issues.
>
>Oops, sorry, this was my mistake.  I meant to suggest "Marvell OCTEON
>PCI Hotplug driver", but I messed up the editing.
>
>> >> +#define OCTEP_DEVID_HP_CONTROLLER 0xa0e3
>> >
>> >Even though this is a private #define, I think something like
>> >PCI_DEVICE_ID_CAVIUM_OCTEP_HP_CTLR would be nice because that's the
>> >typical pattern of include/linux/pci_ids.h.
>>
>> The same reason mentioned above for not using the name CAVIUM.
>> Is it okay to use `PCI_DEVICE_ID_MARVELL_OCTEP_HP_CTLR`?
>
>In this case, I think you should use PCI_DEVICE_ID_CAVIUM... because
>the PCI_VENDOR_ID_CAVIUM identifies the Device ID namespace, so the
>#defines for the Vendor ID and the Device IDs in that namespace should
>match.
>

Okay. I will use CAVIUM for the define.

>> >> +static struct pci_device_id octep_hp_pci_map[] = {
>> >> +	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM,
>> >OCTEP_DEVID_HP_CONTROLLER) },
>> >> +	{ },
>> >> +};

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 42decde38320..7b5a618eed1c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13677,6 +13677,12 @@  R:	[e-mail]
 R:	[e-mail]
 F:	drivers/vdpa/octeon_ep/
 
+MARVELL OCTEON HOTPLUG CONTROLLER DRIVER
+R:	Shijith Thotton <[e-mail]>
+R:	Vamsi Attunuru <[e-mail]>
+S:	Supported
+F:	drivers/pci/hotplug/octep_hp.c
+
 MATROX FRAMEBUFFER DRIVER
 L:	[e-mail]
 S:	Orphan
diff --git a/drivers/pci/hotplug/Kconfig b/drivers/pci/hotplug/Kconfig
index 1472aef0fb81..2e38fd25f7ef 100644
--- a/drivers/pci/hotplug/Kconfig
+++ b/drivers/pci/hotplug/Kconfig
@@ -173,4 +173,14 @@  config HOTPLUG_PCI_S390
 
 	  When in doubt, say Y.
 
+config HOTPLUG_PCI_OCTEONEP
+	bool "OCTEON PCI device Hotplug controller driver"
+	depends on HOTPLUG_PCI
+	help
+	  Say Y here if you have an OCTEON PCIe device with a hotplug
+	  controller. This driver enables the non-controller functions of the
+	  device to be registered as hotplug slots.
+
+	  When in doubt, say N.
+
 endif # HOTPLUG_PCI
diff --git a/drivers/pci/hotplug/Makefile b/drivers/pci/hotplug/Makefile
index 240c99517d5e..40aaf31fe338 100644
--- a/drivers/pci/hotplug/Makefile
+++ b/drivers/pci/hotplug/Makefile
@@ -20,6 +20,7 @@  obj-$(CONFIG_HOTPLUG_PCI_RPA)		+= rpaphp.o
 obj-$(CONFIG_HOTPLUG_PCI_RPA_DLPAR)	+= rpadlpar_io.o
 obj-$(CONFIG_HOTPLUG_PCI_ACPI)		+= acpiphp.o
 obj-$(CONFIG_HOTPLUG_PCI_S390)		+= s390_pci_hpc.o
+obj-$(CONFIG_HOTPLUG_PCI_OCTEONEP)	+= octep_hp.o
 
 # acpiphp_ibm extends acpiphp, so should be linked afterwards.
 
diff --git a/drivers/pci/hotplug/octep_hp.c b/drivers/pci/hotplug/octep_hp.c
new file mode 100644
index 000000000000..77dc2740f43e
--- /dev/null
+++ b/drivers/pci/hotplug/octep_hp.c
@@ -0,0 +1,409 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2024 Marvell. */
+
+#include <linux/cleanup.h>
+#include <linux/container_of.h>
+#include <linux/delay.h>
+#include <linux/dev_printk.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pci.h>
+#include <linux/pci_hotplug.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
+
+#define OCTEP_HP_INTR_OFFSET(x) (0x20400 + ((x) << 4))
+#define OCTEP_HP_INTR_VECTOR(x) (16 + (x))
+#define OCTEP_HP_DRV_NAME "octep_hp"
+
+/*
+ * Type of MSI-X interrupts.
+ * The macros OCTEP_HP_INTR_VECTOR and OCTEP_HP_INTR_OFFSET are used to
+ * generate the vector and offset for an interrupt type.
+ */
+enum octep_hp_intr_type {
+	OCTEP_HP_INTR_INVALID = -1,
+	OCTEP_HP_INTR_ENA = 0,
+	OCTEP_HP_INTR_DIS = 1,
+	OCTEP_HP_INTR_MAX = 2,
+};
+
+struct octep_hp_cmd {
+	struct list_head list;
+	enum octep_hp_intr_type intr_type;
+	u64 intr_val;
+};
+
+struct octep_hp_slot {
+	struct list_head list;
+	struct hotplug_slot slot;
+	u16 slot_number;
+	struct pci_dev *hp_pdev;
+	unsigned int hp_devfn;
+	struct octep_hp_controller *ctrl;
+};
+
+struct octep_hp_intr_info {
+	enum octep_hp_intr_type type;
+	int number;
+	char name[16];
+};
+
+struct octep_hp_controller {
+	void __iomem *base;
+	struct pci_dev *pdev;
+	struct octep_hp_intr_info intr[OCTEP_HP_INTR_MAX];
+	struct work_struct work;
+	struct list_head slot_list;
+	struct mutex slot_lock; /* Protects slot_list */
+	struct list_head hp_cmd_list;
+	spinlock_t hp_cmd_lock; /* Protects hp_cmd_list */
+};
+
+static void octep_hp_enable_pdev(struct octep_hp_controller *hp_ctrl,
+				 struct octep_hp_slot *hp_slot)
+{
+	guard(mutex)(&hp_ctrl->slot_lock);
+	if (hp_slot->hp_pdev) {
+		dev_dbg(&hp_slot->hp_pdev->dev, "Slot %u already enabled\n",
+			hp_slot->slot_number);
+		return;
+	}
+
+	/* Scan the device and add it to the bus */
+	hp_slot->hp_pdev = pci_scan_single_device(hp_ctrl->pdev->bus,
+						  hp_slot->hp_devfn);
+	pci_bus_assign_resources(hp_ctrl->pdev->bus);
+	pci_bus_add_device(hp_slot->hp_pdev);
+
+	dev_dbg(&hp_slot->hp_pdev->dev, "Enabled slot %u\n",
+		hp_slot->slot_number);
+}
+
+static void octep_hp_disable_pdev(struct octep_hp_controller *hp_ctrl,
+				  struct octep_hp_slot *hp_slot)
+{
+	guard(mutex)(&hp_ctrl->slot_lock);
+	if (!hp_slot->hp_pdev) {
+		dev_dbg(&hp_ctrl->pdev->dev, "Slot %u already disabled\n",
+			hp_slot->slot_number);
+		return;
+	}
+
+	dev_dbg(&hp_slot->hp_pdev->dev, "Disabling slot %u\n",
+		hp_slot->slot_number);
+
+	/* Remove the device from the bus */
+	pci_stop_and_remove_bus_device_locked(hp_slot->hp_pdev);
+	hp_slot->hp_pdev = NULL;
+}
+
+static int octep_hp_enable_slot(struct hotplug_slot *slot)
+{
+	struct octep_hp_slot *hp_slot =
+		container_of(slot, struct octep_hp_slot, slot);
+
+	octep_hp_enable_pdev(hp_slot->ctrl, hp_slot);
+	return 0;
+}
+
+static int octep_hp_disable_slot(struct hotplug_slot *slot)
+{
+	struct octep_hp_slot *hp_slot =
+		container_of(slot, struct octep_hp_slot, slot);
+
+	octep_hp_disable_pdev(hp_slot->ctrl, hp_slot);
+	return 0;
+}
+
+static struct hotplug_slot_ops octep_hp_slot_ops = {
+	.enable_slot = octep_hp_enable_slot,
+	.disable_slot = octep_hp_disable_slot,
+};
+
+#define SLOT_NAME_SIZE 16
+static struct octep_hp_slot *
+octep_hp_register_slot(struct octep_hp_controller *hp_ctrl,
+		       struct pci_dev *pdev, u16 slot_number)
+{
+	char slot_name[SLOT_NAME_SIZE];
+	struct octep_hp_slot *hp_slot;
+	int ret;
+
+	hp_slot = kzalloc(sizeof(*hp_slot), GFP_KERNEL);
+	if (!hp_slot)
+		return ERR_PTR(-ENOMEM);
+
+	hp_slot->ctrl = hp_ctrl;
+	hp_slot->hp_pdev = pdev;
+	hp_slot->hp_devfn = pdev->devfn;
+	hp_slot->slot_number = slot_number;
+	hp_slot->slot.ops = &octep_hp_slot_ops;
+
+	snprintf(slot_name, sizeof(slot_name), "octep_hp_%u", slot_number);
+	ret = pci_hp_register(&hp_slot->slot, hp_ctrl->pdev->bus,
+			      PCI_SLOT(pdev->devfn), slot_name);
+	if (ret) {
+		kfree(hp_slot);
+		return ERR_PTR(ret);
+	}
+
+	list_add_tail(&hp_slot->list, &hp_ctrl->slot_list);
+	octep_hp_disable_pdev(hp_ctrl, hp_slot);
+
+	return hp_slot;
+}
+
+static void octep_hp_deregister_slot(void *data)
+{
+	struct octep_hp_slot *hp_slot = data;
+	struct octep_hp_controller *hp_ctrl = hp_slot->ctrl;
+
+	pci_hp_deregister(&hp_slot->slot);
+	octep_hp_enable_pdev(hp_ctrl, hp_slot);
+	list_del(&hp_slot->list);
+	kfree(hp_slot);
+}
+
+static bool octep_hp_is_slot(struct octep_hp_controller *hp_ctrl,
+			     struct pci_dev *pdev)
+{
+	/* Check if the PCI device can be hotplugged */
+	return pdev != hp_ctrl->pdev && pdev->bus == hp_ctrl->pdev->bus &&
+	       PCI_SLOT(pdev->devfn) == PCI_SLOT(hp_ctrl->pdev->devfn);
+}
+
+static void octep_hp_cmd_handler(struct octep_hp_controller *hp_ctrl,
+				 struct octep_hp_cmd *hp_cmd)
+{
+	struct octep_hp_slot *hp_slot;
+
+	/*
+	 * Enable or disable the slots based on the slot mask.
+	 * intr_val is a bit mask where each bit represents a slot.
+	 */
+	list_for_each_entry(hp_slot, &hp_ctrl->slot_list, list) {
+		if (!(hp_cmd->intr_val & BIT(hp_slot->slot_number)))
+			continue;
+
+		if (hp_cmd->intr_type == OCTEP_HP_INTR_ENA)
+			octep_hp_enable_pdev(hp_ctrl, hp_slot);
+		else
+			octep_hp_disable_pdev(hp_ctrl, hp_slot);
+	}
+}
+
+static void octep_hp_work_handler(struct work_struct *work)
+{
+	struct octep_hp_controller *hp_ctrl;
+	struct octep_hp_cmd *hp_cmd;
+	unsigned long flags;
+
+	hp_ctrl = container_of(work, struct octep_hp_controller, work);
+
+	/* Process all the hotplug commands */
+	spin_lock_irqsave(&hp_ctrl->hp_cmd_lock, flags);
+	while (!list_empty(&hp_ctrl->hp_cmd_list)) {
+		hp_cmd = list_first_entry(&hp_ctrl->hp_cmd_list,
+					  struct octep_hp_cmd, list);
+		list_del(&hp_cmd->list);
+		spin_unlock_irqrestore(&hp_ctrl->hp_cmd_lock, flags);
+
+		octep_hp_cmd_handler(hp_ctrl, hp_cmd);
+		kfree(hp_cmd);
+
+		spin_lock_irqsave(&hp_ctrl->hp_cmd_lock, flags);
+	}
+	spin_unlock_irqrestore(&hp_ctrl->hp_cmd_lock, flags);
+}
+
+static enum octep_hp_intr_type octep_hp_intr_type(struct octep_hp_intr_info *intr,
+						  int irq)
+{
+	enum octep_hp_intr_type type;
+
+	for (type = OCTEP_HP_INTR_ENA; type < OCTEP_HP_INTR_MAX; type++) {
+		if (intr[type].number == irq)
+			return type;
+	}
+
+	return OCTEP_HP_INTR_INVALID;
+}
+
+static irqreturn_t octep_hp_intr_handler(int irq, void *data)
+{
+	struct octep_hp_controller *hp_ctrl = data;
+	struct pci_dev *pdev = hp_ctrl->pdev;
+	enum octep_hp_intr_type type;
+	struct octep_hp_cmd *hp_cmd;
+	u64 intr_val;
+
+	type = octep_hp_intr_type(hp_ctrl->intr, irq);
+	if (type == OCTEP_HP_INTR_INVALID) {
+		dev_err(&pdev->dev, "Invalid interrupt %d\n", irq);
+		return IRQ_HANDLED;
+	}
+
+	/* Read and clear the interrupt */
+	intr_val = readq(hp_ctrl->base + OCTEP_HP_INTR_OFFSET(type));
+	writeq(intr_val, hp_ctrl->base + OCTEP_HP_INTR_OFFSET(type));
+
+	hp_cmd = kzalloc(sizeof(*hp_cmd), GFP_ATOMIC);
+	if (!hp_cmd)
+		return IRQ_HANDLED;
+
+	hp_cmd->intr_val = intr_val;
+	hp_cmd->intr_type = type;
+
+	/* Add the command to the list and schedule the work */
+	spin_lock(&hp_ctrl->hp_cmd_lock);
+	list_add_tail(&hp_cmd->list, &hp_ctrl->hp_cmd_list);
+	spin_unlock(&hp_ctrl->hp_cmd_lock);
+	schedule_work(&hp_ctrl->work);
+
+	return IRQ_HANDLED;
+}
+
+static void octep_hp_irq_cleanup(void *data)
+{
+	struct octep_hp_controller *hp_ctrl = data;
+
+	pci_free_irq_vectors(hp_ctrl->pdev);
+	flush_work(&hp_ctrl->work);
+}
+
+static int octep_hp_request_irq(struct octep_hp_controller *hp_ctrl,
+				enum octep_hp_intr_type type)
+{
+	struct pci_dev *pdev = hp_ctrl->pdev;
+	struct octep_hp_intr_info *intr;
+	int irq;
+
+	irq = pci_irq_vector(pdev, OCTEP_HP_INTR_VECTOR(type));
+	if (irq < 0)
+		return irq;
+
+	intr = &hp_ctrl->intr[type];
+	intr->number = irq;
+	intr->type = type;
+	snprintf(intr->name, sizeof(intr->name), "octep_hp_%d", type);
+
+	return devm_request_irq(&pdev->dev, irq, octep_hp_intr_handler,
+				IRQF_SHARED, intr->name, hp_ctrl);
+}
+
+static int octep_hp_controller_setup(struct pci_dev *pdev,
+				     struct octep_hp_controller *hp_ctrl)
+{
+	struct device *dev = &pdev->dev;
+	enum octep_hp_intr_type type;
+	int ret;
+
+	ret = pcim_enable_device(pdev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable PCI device\n");
+
+	hp_ctrl->base = pcim_iomap_region(pdev, 0, OCTEP_HP_DRV_NAME);
+	if (IS_ERR(hp_ctrl->base))
+		return dev_err_probe(dev, PTR_ERR(hp_ctrl->base),
+				     "Failed to map PCI device region\n");
+
+	pci_set_master(pdev);
+	pci_set_drvdata(pdev, hp_ctrl);
+
+	INIT_LIST_HEAD(&hp_ctrl->slot_list);
+	INIT_LIST_HEAD(&hp_ctrl->hp_cmd_list);
+	mutex_init(&hp_ctrl->slot_lock);
+	spin_lock_init(&hp_ctrl->hp_cmd_lock);
+	INIT_WORK(&hp_ctrl->work, octep_hp_work_handler);
+	hp_ctrl->pdev = pdev;
+
+	ret = pci_alloc_irq_vectors(pdev, 1,
+				    OCTEP_HP_INTR_VECTOR(OCTEP_HP_INTR_MAX),
+				    PCI_IRQ_MSIX);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to alloc MSI-X vectors\n");
+
+	ret = devm_add_action(&pdev->dev, octep_hp_irq_cleanup, hp_ctrl);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Failed to add IRQ cleanup action\n");
+
+	for (type = OCTEP_HP_INTR_ENA; type < OCTEP_HP_INTR_MAX; type++) {
+		ret = octep_hp_request_irq(hp_ctrl, type);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to request IRQ for vector %d\n",
+					     OCTEP_HP_INTR_VECTOR(type));
+	}
+
+	return 0;
+}
+
+static int octep_hp_pci_probe(struct pci_dev *pdev,
+			      const struct pci_device_id *id)
+{
+	struct octep_hp_controller *hp_ctrl;
+	struct pci_dev *tmp_pdev = NULL;
+	struct octep_hp_slot *hp_slot;
+	u16 slot_number = 0;
+	int ret;
+
+	hp_ctrl = devm_kzalloc(&pdev->dev, sizeof(*hp_ctrl), GFP_KERNEL);
+	if (!hp_ctrl)
+		return -ENOMEM;
+
+	ret = octep_hp_controller_setup(pdev, hp_ctrl);
+	if (ret)
+		return ret;
+
+	/*
+	 * Register all hotplug slots. Hotplug controller is the first function
+	 * of the PCI device. The hotplug slots are the remaining functions of
+	 * the PCI device. They are removed from the bus and are added back when
+	 * the hotplug event is triggered.
+	 */
+	for_each_pci_dev(tmp_pdev) {
+		if (!octep_hp_is_slot(hp_ctrl, tmp_pdev))
+			continue;
+
+		hp_slot = octep_hp_register_slot(hp_ctrl, tmp_pdev, slot_number);
+		if (IS_ERR(hp_slot))
+			return dev_err_probe(&pdev->dev, PTR_ERR(hp_slot),
+					     "Failed to register hotplug slot %u\n",
+					     slot_number);
+
+		ret = devm_add_action(&pdev->dev, octep_hp_deregister_slot,
+				      hp_slot);
+		if (ret)
+			return dev_err_probe(&pdev->dev, ret,
+					     "Failed to add action for deregistering slot %u\n",
+					     slot_number);
+		slot_number++;
+	}
+
+	return 0;
+}
+
+#define OCTEP_DEVID_HP_CONTROLLER 0xa0e3
+static struct pci_device_id octep_hp_pci_map[] = {
+	{ PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, OCTEP_DEVID_HP_CONTROLLER) },
+	{ },
+};
+
+static struct pci_driver octep_hp = {
+	.name = OCTEP_HP_DRV_NAME,
+	.id_table = octep_hp_pci_map,
+	.probe = octep_hp_pci_probe,
+};
+
+module_pci_driver(octep_hp);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Marvell");
+MODULE_DESCRIPTION("OCTEON PCIe device hotplug controller driver");