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