On Fri, May 06, 2016 at 08:12:42AM -0500, Rob Herring wrote:
>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.
>
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
--
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
