On 9/11/23 08:13, Herve Codina wrote:
Hi Lizhi,
On Tue, 15 Aug 2023 10:19:57 -0700
Lizhi Hou <lizhi.hou@xxxxxxx> 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é
