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

From: Gavin Shan <gwshan@linux.vnet.ibm.com>
To: Bjorn Helgaas <bhelgaas@google.com>
Cc: Gavin Shan <gwshan@linux.vnet.ibm.com>,
	linuxppc-dev@lists.ozlabs.org, linux-pci@vger.kernel.org,
	devicetree@vger.kernel.org, benh@kernel.crashing.org,
	aik@ozlabs.ru, panto@antoniou-consulting.com,
	robherring2@gmail.com, grant.likely@linaro.org
Subject: Re: [PATCH v5 42/42] pci/hotplug: PowerPC PowerNV PCI hotplug driver
Date: Tue, 9 Jun 2015 16:08:12 +1000	[thread overview]
Message-ID: <20150609060812.GA30787@gwshan> (raw)
In-Reply-To: <20150605201110.GP3631@google.com>

On Fri, Jun 05, 2015 at 03:11:10PM -0500, Bjorn Helgaas wrote:
>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 <gwshan@linux.vnet.ibm.com>
>
>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):
>

Thanks for your review, Bjorn.

>> +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.
>

There is PCI bus referred (struct powernv_php_slot::bus), but it's
not always valid. So I'll add one more field "struct pci_dev *pdev"
which is initialized to the parent PCI device of the slot, then print
those messages with dev_warn().

>> +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.
>

Ben already suggested some better names in another reply. I'll pick
it if you agree: pci_add_pci_devices().

>> +	/* 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.
>

According to Ben's suggestion in another reply, it would be pci_remove_pci_devices()
if you agree :-)

>> +	/* Slot indentifier */
>
>s/indentifier/identifier/
>

Thanks for pointing it out. I'll fix it up in next revision.

>> +	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).
>

I'll pick Ben's suggested name if you agree: of_pci_node_to_bus().

Thanks,
Gavin

>Bjorn
>