Re: [PATCH v5 42/42] pci/hotplug: PowerPC PowerNV PCI hotplug driver

On Thu, Jun 04, 2015 at 04:42:11PM +1000, Gavin Shan wrote:

The patch intends to add standalone driver to support PCI hotplug
for PowerPC PowerNV platform, which runs on top of skiboot firmware.
The firmware identified hotpluggable slots and marked their device
tree node with proper "ibm,slot-pluggable" and "ibm,reset-by-firmware".
The driver simply scans device-tree to create/register PCI hotplug slot
accordingly.

If the skiboot firmware doesn't support slot status retrieval, the PCI
slot device node shouldn't have property "ibm,reset-by-firmware". In
that case, none of valid PCI slots will be detected from device tree.
The skiboot firmware doesn't export the capability to access attention
LEDs yet and it's something for TBD.

Signed-off-by: Gavin Shan <redacted>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

But I do have a few comments (my ack is valid whether you do anything with
them or not):

+static void slot_power_off_handler(struct powernv_php_slot *slot)
+{
+	int ret;
+
+	/* Release the firmware data for the child device nodes */
+	remove_child_pdn(slot->dn);
+
+	/*
+	 * Release the child device nodes. If the sub-tree was
+	 * built with the help of overlay, we just need revert
+	 * the changes introduced by the overlay
+	 */
+	if (slot->overlay_id >= 0) {
+		ret = of_overlay_destroy(slot->overlay_id);
+		if (ret)
+			pr_warn("%s: Error %d destroying overlay %d\n",
+				__func__, ret, slot->overlay_id);

For this and similar messages: isn't there a device you can use with
dev_warn() here?  I think a device name would be much better than a
function name.

+scan:
+	switch (presence) {
+	case POWERNV_PHP_SLOT_PRESENT:
+		if (rescan) {
+			pci_lock_rescan_remove();
+			pcibios_add_pci_devices(slot->bus);

You didn't add this, but "pcibios_add_pci_devices" doesn't seem like the
right name.  "pcibios" generally refers to an arch-specific hook that's
called by the generic PCI core.  In this case, pcibios_add_pci_devices()
contains powerpc-specific code, and it's only called from powerpc code, so
I think using "pcibios_" in the name is a bit misleading.

+	/* Remove all devices behind the slot */
+	pci_lock_rescan_remove();
+	pcibios_remove_pci_devices(slot->bus);

Same comment for pcibios_remove_pci_devices().  It would be better if the
name didn't suggest that this was part of the pcibios_ interface between
the PCI core and the arch code, because it's not.

+	/* Slot indentifier */

s/indentifier/identifier/

+	if (!php_slot_get_id(dn, &id))
+		return NULL;
+

+	/* PCI bus */
+	bus = pcibios_find_pci_bus(dn);

And pcibios_find_pci_bus() (it's also powerpc-specific).

Bjorn