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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
