Message-ID: <4ccd9525-455d-3608-dd39-caf2753620c1@amd.com>
Date:   Mon, 11 Sep 2023 10:53:17 -0700
From:   Lizhi Hou <lizhi.hou@....com>
To:     Herve Codina <herve.codina@...tlin.com>
CC:     <linux-pci@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <robh@...nel.org>,
        <max.zhen@....com>, <sonal.santan@....com>,
        <stefano.stabellini@...inx.com>
Subject: Re: [PATCH V13 2/5] PCI: Create device tree node for bridge


On 9/11/23 08:13, Herve Codina wrote:
> Hi Lizhi,
>
> On Tue, 15 Aug 2023 10:19:57 -0700
> Lizhi Hou <lizhi.hou@....com> wrote:
> ...
>> +void of_pci_make_dev_node(struct pci_dev *pdev)
>> +{
>> +	struct device_node *ppnode, *np = NULL;
>> +	const char *pci_type;
>> +	struct of_changeset *cset;
>> +	const char *name;
>> +	int ret;
>> +
>> +	/*
>> +	 * If there is already a device tree node linked to this device,
>> +	 * return immediately.
>> +	 */
>> +	if (pci_device_to_OF_node(pdev))
>> +		return;
>> +
>> +	/* Check if there is device tree node for parent device */
>> +	if (!pdev->bus->self)
>> +		ppnode = pdev->bus->dev.of_node;
>> +	else
>> +		ppnode = pdev->bus->self->dev.of_node;
>> +	if (!ppnode)
>> +		return;
>> +
>> +	if (pci_is_bridge(pdev))
>> +		pci_type = "pci";
>> +	else
>> +		pci_type = "dev";
>> +
>> +	name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
>> +			 PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>> +	if (!name)
>> +		return;
>> +
>> +	cset = kmalloc(sizeof(*cset), GFP_KERNEL);
>> +	if (!cset)
>> +		goto failed;
>> +	of_changeset_init(cset);
>> +
>> +	np = of_changeset_create_node(ppnode, name, cset);
>> +	if (!np)
>> +		goto failed;
> The "goto failed" will leak the cset previously allocated.
>
> np->data = cset; (next line) allows to free the cset when the node is destroyed
> (of_node_put() calls). When the node cannot be created, the allocated cset should
> be freed.
Thanks for pointing this out.
>
>> +	np->data = cset;
>> +
>> +	ret = of_pci_add_properties(pdev, cset, np);
>> +	if (ret)
>> +		goto failed;
>> +
>> +	ret = of_changeset_apply(cset);
>> +	if (ret)
>> +		goto failed;
>> +
>> +	pdev->dev.of_node = np;
>> +	kfree(name);
>> +
>> +	return;
>> +
>> +failed:
>> +	if (np)
>> +		of_node_put(np);
>> +	kfree(name);
>> +}
>> +#endif
>> +
>>   #endif /* CONFIG_PCI */
>>   
> ...
>> +static int of_pci_prop_intr_map(struct pci_dev *pdev, struct of_changeset *ocs,
>> +				struct device_node *np)
>> +{
>> +	struct of_phandle_args out_irq[OF_PCI_MAX_INT_PIN];
>> +	u32 i, addr_sz[OF_PCI_MAX_INT_PIN], map_sz = 0;
>> +	__be32 laddr[OF_PCI_ADDRESS_CELLS] = { 0 };
>> +	u32 int_map_mask[] = { 0xffff00, 0, 0, 7 };
>> +	struct device_node *pnode;
>> +	struct pci_dev *child;
>> +	u32 *int_map, *mapp;
>> +	int ret;
>> +	u8 pin;
>> +
>> +	pnode = pci_device_to_OF_node(pdev->bus->self);
>> +	if (!pnode)
>> +		pnode = pci_bus_to_OF_node(pdev->bus);
>> +
>> +	if (!pnode) {
>> +		pci_err(pdev, "failed to get parent device node");
>> +		return -EINVAL;
>> +	}
>> +
>> +	laddr[0] = cpu_to_be32((pdev->bus->number << 16) | (pdev->devfn << 8));
>> +	for (pin = 1; pin <= OF_PCI_MAX_INT_PIN;  pin++) {
>> +		i = pin - 1;
>> +		out_irq[i].np = pnode;
>> +		out_irq[i].args_count = 1;
>> +		out_irq[i].args[0] = pin;
>> +		ret = of_irq_parse_raw(laddr, &out_irq[i]);
>> +		if (ret) {
>> +			pci_err(pdev, "parse irq %d failed, ret %d", pin, ret);
>> +			continue;
>> +		}
>> +		ret = of_property_read_u32(out_irq[i].np, "#address-cells",
>> +					   &addr_sz[i]);
>> +		if (ret)
>> +			addr_sz[i] = 0;
>> +	}
> if of_irq_parse_raw() fails, addr_sz[i] is not initialized and map_sz bellow is
> computed with uninitialized values.
> On the test I did, this lead to a kernel crash due to the following kcalloc()
> called with incorrect values.
>
> Are interrupt-map and interrupt-map-mask properties needed in all cases ?
> I mean are they mandatory for the host pci bridge ?
interrupt-map is required for bridges when a legacy interrupt device is 
underneath. Otherwise, of_irq_parse_pci() needs to be changed for legacy 
interrupt device. Please see my previous patch and comments.
>
>> +
>> +	list_for_each_entry(child, &pdev->subordinate->devices, bus_list) {
>> +		for (pin = 1; pin <= OF_PCI_MAX_INT_PIN; pin++) {
>> +			i = pci_swizzle_interrupt_pin(child, pin) - 1;
>> +			map_sz += 5 + addr_sz[i] + out_irq[i].args_count;
> of_irq_parse_raw() can fail on some pins.
> Is it correct to set map_sz based on information related to all pins even if
> of_irq_parse_raw() previously failed on some pins ?

I think the interrupt-map pair should be skipped if of_irq_parse_raw() 
is failed. Thanks.


Lizhi

>
>> +		}
>> +	}
>> +
>> +	int_map = kcalloc(map_sz, sizeof(u32), GFP_KERNEL);
>> +	mapp = int_map;
>> +
>> +	list_for_each_entry(child, &pdev->subordinate->devices, bus_list) {
>> +		for (pin = 1; pin <= OF_PCI_MAX_INT_PIN; pin++) {
>> +			*mapp = (child->bus->number << 16) |
>> +				(child->devfn << 8);
>> +			mapp += OF_PCI_ADDRESS_CELLS;
>> +			*mapp = pin;
>> +			mapp++;
>> +			i = pci_swizzle_interrupt_pin(child, pin) - 1;
>> +			*mapp = out_irq[i].np->phandle;
>> +			mapp++;
>> +			if (addr_sz[i]) {
>> +				ret = of_property_read_u32_array(out_irq[i].np,
>> +								 "reg", mapp,
>> +								 addr_sz[i]);
>> +				if (ret)
>> +					goto failed;
>> +			}
>> +			mapp += addr_sz[i];
>> +			memcpy(mapp, out_irq[i].args,
>> +			       out_irq[i].args_count * sizeof(u32));
>> +			mapp += out_irq[i].args_count;
>> +		}
>> +	}
>> +
>> +	ret = of_changeset_add_prop_u32_array(ocs, np, "interrupt-map", int_map,
>> +					      map_sz);
>> +	if (ret)
>> +		goto failed;
>> +
>> +	ret = of_changeset_add_prop_u32(ocs, np, "#interrupt-cells", 1);
>> +	if (ret)
>> +		goto failed;
>> +
>> +	ret = of_changeset_add_prop_u32_array(ocs, np, "interrupt-map-mask",
>> +					      int_map_mask,
>> +					      ARRAY_SIZE(int_map_mask));
>> +	if (ret)
>> +		goto failed;
>> +
>> +	kfree(int_map);
>> +	return 0;
>> +
>> +failed:
>> +	kfree(int_map);
>> +	return ret;
>> +}
>> +
> ...
>
> Regards,
> Hervé
>

Powered by blists - more mailing lists