Message-ID: <20241107203252.GA1623581@bhelgaas>
Date: Thu, 7 Nov 2024 14:32:52 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Shijith Thotton <sthotton@...vell.com>
Cc: bhelgaas@...gle.com, ilpo.jarvinen@...ux.intel.com,
	Jonathan.Cameron@...wei.com, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org, rafael@...nel.org,
	scott@...amperecomputing.com, jerinj@...vell.com,
	schalla@...vell.com, vattunuru@...vell.com
Subject: Re: [PATCH v3] PCI: hotplug: Add OCTEON PCI hotplug controller driver

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 <sthotton@...vell.com>
> Co-developed-by: Vamsi Attunuru <vattunuru@...vell.com>
> Signed-off-by: Vamsi Attunuru <vattunuru@...vell.com>

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) },
> +	{ },
> +};

Powered by blists - more mailing lists