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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 5, 2016 at 7:28 PM, Gavin Shan <gwshan@xxxxxxxxxxxxxxxxxx> wrote:
> On Thu, May 05, 2016 at 12:04:49PM -0500, Rob Herring wrote:
>>On Tue, May 3, 2016 at 8:22 AM, Gavin Shan <gwshan@xxxxxxxxxxxxxxxxxx> wrote:
>>> 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 <gwshan@xxxxxxxxxxxxxxxxxx>
>>> Acked-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>>
>>[...]
>>
>>> +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.

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

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux