Re: [PATCH v9 22/22] PCI/hotplug: PowerPC PowerNV PCI hotplug driver

On Fri, May 06, 2016 at 08:12:42AM -0500, Rob Herring wrote:

On Thu, May 5, 2016 at 7:28 PM, Gavin Shan [off-list ref] wrote:

quoted

On Thu, May 05, 2016 at 12:04:49PM -0500, Rob Herring wrote:

quoted

On Tue, May 3, 2016 at 8:22 AM, Gavin Shan [off-list ref] wrote:

quoted

This adds standalone driver to support PCI hotplug for PowerPC PowerNV
platform that runs on top of skiboot firmware. The firmware identifies
hotpluggable slots and marked their device tree node with proper
"ibm,slot-pluggable" and "ibm,reset-by-firmware". The driver scans
device tree nodes to create/register PCI hotplug slot accordingly.

The PCI slots are organized in fashion of tree, which means one
PCI slot might have parent PCI slot and parent PCI slot possibly
contains multiple child PCI slots. At the plugging time, the parent
PCI slot is populated before its children. The child PCI slots are
removed before their parent PCI slot can be removed from the system.

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>

[...]

quoted

+static void pnv_php_handle_poweron(struct pnv_php_slot *php_slot)
+{
+       void *fdt, *fdt1, *dt;
+       int confirm = PNV_PHP_POWER_CONFIRMED_SUCCESS;
+       int ret;
+
+       /* We don't know the FDT blob size. We try to get it through
+        * maximal memory chunk and then copy it to another chunk that
+        * fits the real size.
+        */
+       fdt1 = kzalloc(0x10000, GFP_KERNEL);
+       if (!fdt1)
+               goto error;
+
+       ret = pnv_pci_get_device_tree(php_slot->dn->phandle, fdt1, 0x10000);
+       if (ret)
+               goto free_fdt1;
+
+       fdt = kzalloc(fdt_totalsize(fdt1), GFP_KERNEL);
+       if (!fdt)
+               goto free_fdt1;
+
+       /* Unflatten device tree blob */
+       memcpy(fdt, fdt1, fdt_totalsize(fdt1));

This is wrong. If the size is greater than 64K, then you will be
overrunning the fdt1 buffer. You need to fetch the FDT again if it is
bigger than 64KB.

Thanks for review, Rob. Sorry that I don't see how it's a problem. An
errcode is returned from pnv_pci_get_device_tree() if the FDT blob
size is greater than 64K. In this case, memcpy() won't be triggered.
pnv_pci_get_device_tree() relies on firmware implementation which
avoids overrunning the buffer.

Okay, I missed that pnv_pci_get_device_tree would error out.

quoted

On the other hand, it would be reasonable to retry retriving the
FDT blob if 64K buffer isn't enough. Also, kzalloc() can be replaced
with alloc_pages() as 64K is the default page size on PPC64. I will
have something like below until some one has more concerns. As the
size of the allocated buffer will be greater than the real FDT blob
size, some memory (not too much) is wasted. I guess it should be ok.

        struct page *page;
        void *fdt;
        unsigned int order;
        int ret;

        for (order = 0; order < MAX_ORDER; order++) {
                page = alloc_pages(GFP_KERNEL, order);
                if (page) {
                        fdt = page_address(page);
                        ret = pnv_pci_get_device_tree(php_slot->dn->phandle,
                                                      fdt, (1 << order) * PAGE_SIZE);
                        if (ret) {
                                dev_dbg(&php_slot->pdev.dev, "Error %d getting device tree (%d)\n",
                                        ret, order);
                                free_pages(fdt, order);
                                continue;
                        }
                }
        }

I would allocate a minimal buffer to read the header, get the actual
size, then allocate a new buffer. There's no point in looping. If you
know 64KB is the biggest size you should ever see, then how you had it
is reasonable, too.

The interface pnv_pci_get_device_tree() returns header and meta-data
in one shoot as it was designed. Also, 64KB here is good enough as I
can see. I will keep the code as-is. Thanks a lot for looking into
details, Rob.

Thanks,
Gavin