lkml.org 
[lkml]   [2022]   [Sep]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH RFC 2/2] pci: create device tree node for selected devices
    On Mon, Aug 29, 2022 at 02:43:37PM -0700, Lizhi Hou wrote:
    > The PCI endpoint device such as Xilinx Alveo PCI card maps the register
    > spaces from multiple hardware peripherals to its PCI BAR. Normally,
    > the PCI core discovers devices and BARs using the PCI enumeration process.
    > And the process does not provide a way to discover the hardware peripherals
    > been mapped to PCI BARs.

    This sentence doesn't make sense.

    > For Alveo PCI card, the card firmware provides a
    > flattened device tree to describe the hardware peripherals on its BARs.
    > And the Alveo card driver can load this flattened device tree and leverage
    > device tree framework to generate platform devices for the hardware
    > peripherals eventually.
    >
    > Apparently, the device tree framework requires a device tree node for the
    > PCI device. Thus, it can generate the device tree nodes for hardware
    > peripherals underneath. Because PCI is self discoverable bus, there might
    > not be a device tree node created for PCI devices. This patch is to add
    > support to generate device tree node for PCI devices. It introduces a
    > kernel option. When the option is turned on, the kernel will generate
    > device tree nodes for PCI bridges unconditionally. It will also generate
    > a device tree node for Xilinx Alveo U50 by using PCI quirks. The generated
    > device tree nodes do not have any property. The future patches will add
    > necessary properties.
    >
    > Signed-off-by: Lizhi Hou <lizhi.hou@amd.com>
    > Signed-off-by: Sonal Santan <sonal.santan@amd.com>
    > Signed-off-by: Max Zhen <max.zhen@amd.com>
    > Signed-off-by: Brian Xu <brian.xu@amd.com>
    > ---
    > drivers/pci/Kconfig | 11 ++++
    > drivers/pci/bus.c | 2 +
    > drivers/pci/msi/irqdomain.c | 6 +-
    > drivers/pci/of.c | 106 ++++++++++++++++++++++++++++++++++++
    > drivers/pci/pci-driver.c | 3 +-
    > drivers/pci/pci.h | 16 ++++++
    > drivers/pci/quirks.c | 11 ++++
    > drivers/pci/remove.c | 1 +
    > 8 files changed, 153 insertions(+), 3 deletions(-)
    >
    > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
    > index 55c028af4bd9..9eca5420330b 100644
    > --- a/drivers/pci/Kconfig
    > +++ b/drivers/pci/Kconfig
    > @@ -198,6 +198,17 @@ config PCI_HYPERV
    > The PCI device frontend driver allows the kernel to import arbitrary
    > PCI devices from a PCI backend to support PCI driver domains.
    >
    > +config PCI_OF

    We already have OF_PCI so this is confusing. Maybe
    'PCI_DYNAMIC_OF_NODES'.


    > + bool "Device tree node for PCI devices"
    > + select OF_DYNAMIC
    > + help
    > + This option enables support for generating device tree nodes for some
    > + PCI devices. Thus, the driver of this kind can load and overlay
    > + flattened device tree for its downstream devices.
    > +
    > + Once this option is selected, the device tree nodes will be generated
    > + for all PCI/PCIE bridges.
    > +
    > choice
    > prompt "PCI Express hierarchy optimization setting"
    > default PCIE_BUS_DEFAULT
    > diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
    > index 3cef835b375f..f752b804ad1f 100644
    > --- a/drivers/pci/bus.c
    > +++ b/drivers/pci/bus.c
    > @@ -316,6 +316,8 @@ void pci_bus_add_device(struct pci_dev *dev)
    > */
    > pcibios_bus_add_device(dev);
    > pci_fixup_device(pci_fixup_final, dev);
    > + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)

    Would pci_is_bridge() work here? That would include cardbus, but I think
    that won't matter in practice.

    > + of_pci_make_dev_node(dev);


    > pci_create_sysfs_dev_files(dev);
    > pci_proc_attach_device(dev);
    > pci_bridge_d3_update(dev);
    > diff --git a/drivers/pci/msi/irqdomain.c b/drivers/pci/msi/irqdomain.c
    > index e9cf318e6670..eeaf44169bfd 100644
    > --- a/drivers/pci/msi/irqdomain.c
    > +++ b/drivers/pci/msi/irqdomain.c
    > @@ -230,8 +230,10 @@ u32 pci_msi_domain_get_msi_rid(struct irq_domain *domain, struct pci_dev *pdev)
    > pci_for_each_dma_alias(pdev, get_msi_id_cb, &rid);
    >
    > of_node = irq_domain_get_of_node(domain);
    > - rid = of_node ? of_msi_map_id(&pdev->dev, of_node, rid) :
    > - iort_msi_map_id(&pdev->dev, rid);
    > + if (of_node && !of_node_check_flag(of_node, OF_DYNAMIC))
    > + rid = of_msi_map_id(&pdev->dev, of_node, rid);
    > + else
    > + rid = iort_msi_map_id(&pdev->dev, rid);
    >
    > return rid;
    > }
    > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
    > index 196834ed44fe..19856d42e147 100644
    > --- a/drivers/pci/of.c
    > +++ b/drivers/pci/of.c
    > @@ -469,6 +469,8 @@ static int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *
    > } else {
    > /* We found a P2P bridge, check if it has a node */
    > ppnode = pci_device_to_OF_node(ppdev);
    > + if (of_node_check_flag(ppnode, OF_DYNAMIC))
    > + ppnode = NULL;
    > }
    >
    > /*
    > @@ -599,6 +601,110 @@ int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge)
    > return pci_parse_request_of_pci_ranges(dev, bridge);
    > }
    >
    > +#if IS_ENABLED(CONFIG_PCI_OF)
    > +struct of_pci_node {
    > + struct list_head node;
    > + struct device_node *dt_node;
    > + struct of_changeset cset;
    > +};
    > +
    > +static LIST_HEAD(of_pci_node_list);
    > +static DEFINE_MUTEX(of_pci_node_lock);

    There is a 'data' pointer in device_node which you could use to store
    the changeset. Then you wouldn't need a list. (though 'data' is rarely
    used and I hoped to get rid of it)

    > +
    > +static void of_pci_free_node(struct of_pci_node *node)
    > +{
    > + of_changeset_destroy(&node->cset);
    > + kfree(node->dt_node->full_name);
    > + if (node->dt_node)
    > + of_node_put(node->dt_node);

    You free full_name before freeing the node, so you could have a UAF.
    That needs to be taken care of when releasing the node.

    > + kfree(node);
    > +}
    > +
    > +void of_pci_remove_node(struct pci_dev *pdev)
    > +{
    > + struct list_head *ele, *next;
    > + struct of_pci_node *node;
    > +
    > + mutex_lock(&of_pci_node_lock);
    > +
    > + list_for_each_safe(ele, next, &of_pci_node_list) {
    > + node = list_entry(ele, struct of_pci_node, node);
    > + if (node->dt_node == pdev->dev.of_node) {
    > + list_del(&node->node);
    > + mutex_unlock(&of_pci_node_lock);
    > + of_pci_free_node(node);
    > + break;
    > + }
    > + }
    > + mutex_unlock(&of_pci_node_lock);
    > +}

    The above bits aren't really particular to PCI, so they probably
    belong in the DT core code. Frank will probably have thoughts on what
    this should look like.

    > +
    > +void of_pci_make_dev_node(struct pci_dev *pdev)
    > +{
    > + const char *pci_type = "dev";
    > + struct device_node *parent;
    > + struct of_pci_node *node;
    > + 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)
    > + parent = pdev->bus->dev.of_node;
    > + else
    > + parent = pdev->bus->self->dev.of_node;
    > + if (!parent)
    > + return;
    > +
    > + node = kzalloc(sizeof(*node), GFP_KERNEL);
    > + if (!node)
    > + return;
    > + of_changeset_init(&node->cset);
    > +
    > + node->dt_node = of_node_alloc(NULL);
    > + if (!node->dt_node) {
    > + ret = -ENOMEM;
    > + goto failed;
    > + }
    > + node->dt_node->parent = parent;
    > +
    > + if (pci_is_bridge(pdev))
    > + pci_type = "pci";
    > +
    > + node->dt_node->full_name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
    > + PCI_SLOT(pdev->devfn),
    > + PCI_FUNC(pdev->devfn));
    > + if (!node->dt_node->full_name) {
    > + ret = -ENOMEM;
    > + goto failed;
    > + }

    Wait, aren't you missing adding properties? You need 'reg',
    'compatbile', and 'device_type = "pci"' for bridges.

    > +
    > + ret = of_changeset_attach_node(&node->cset, node->dt_node);
    > + if (ret)
    > + goto failed;
    > +
    > + ret = of_changeset_apply(&node->cset);
    > + if (ret)
    > + goto failed;
    > +
    > + pdev->dev.of_node = node->dt_node;
    > +
    > + mutex_lock(&of_pci_node_lock);
    > + list_add(&node->node, &of_pci_node_list);
    > + mutex_unlock(&of_pci_node_lock);
    > +
    > + return;
    > +
    > +failed:
    > + of_pci_free_node(node);
    > +}

    Pass in the parent node and node name, and this function is not PCI
    specific either.

    > +#endif
    > +
    > #endif /* CONFIG_PCI */
    >
    > /**
    > diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
    > index 49238ddd39ee..1540c4c9a770 100644
    > --- a/drivers/pci/pci-driver.c
    > +++ b/drivers/pci/pci-driver.c
    > @@ -1628,7 +1628,8 @@ static int pci_dma_configure(struct device *dev)
    > bridge = pci_get_host_bridge_device(to_pci_dev(dev));
    >
    > if (IS_ENABLED(CONFIG_OF) && bridge->parent &&
    > - bridge->parent->of_node) {
    > + bridge->parent->of_node &&
    > + !of_node_check_flag(bridge->parent->of_node, OF_DYNAMIC)) {
    > ret = of_dma_configure(dev, bridge->parent->of_node, true);
    > } else if (has_acpi_companion(bridge)) {
    > struct acpi_device *adev = to_acpi_device_node(bridge->fwnode);
    > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
    > index 785f31086313..319b79920d40 100644
    > --- a/drivers/pci/pci.h
    > +++ b/drivers/pci/pci.h
    > @@ -678,6 +678,22 @@ static inline int devm_of_pci_bridge_init(struct device *dev, struct pci_host_br
    >
    > #endif /* CONFIG_OF */
    >
    > +#ifdef CONFIG_PCI_OF
    > +void of_pci_make_dev_node(struct pci_dev *pdev);
    > +void of_pci_remove_node(struct pci_dev *pdev);
    > +
    > +#else
    > +static inline void
    > +of_pci_make_dev_node(struct pci_dev *pdev)
    > +{
    > +}
    > +
    > +static inline void
    > +of_pci_remove_node(struct pci_dev *pdev);
    > +{
    > +}
    > +#endif /* CONFIG_OF_DYNAMIC */
    > +
    > #ifdef CONFIG_PCIEAER
    > void pci_no_aer(void);
    > void pci_aer_init(struct pci_dev *dev);
    > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
    > index 4944798e75b5..58a81e9ff0ed 100644
    > --- a/drivers/pci/quirks.c
    > +++ b/drivers/pci/quirks.c
    > @@ -5956,3 +5956,14 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56b1, aspm_l1_acceptable_latency
    > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c0, aspm_l1_acceptable_latency);
    > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x56c1, aspm_l1_acceptable_latency);
    > #endif
    > +
    > +/*
    > + * For PCIe device which have multiple downstream devices, its driver may use
    > + * a flattened device tree to describe the downstream devices.
    > + * To overlay the flattened device tree, the PCI device and all its ancestor
    > + * devices need to have device tree nodes on system base device tree. Thus,
    > + * before driver probing, it might need to add a device tree node as the final
    > + * fixup.
    > + */
    > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
    > +DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
    > diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
    > index 4c54c75050dc..0eaa9d9a3609 100644
    > --- a/drivers/pci/remove.c
    > +++ b/drivers/pci/remove.c
    > @@ -23,6 +23,7 @@ static void pci_stop_dev(struct pci_dev *dev)
    > device_release_driver(&dev->dev);
    > pci_proc_detach_device(dev);
    > pci_remove_sysfs_dev_files(dev);
    > + of_pci_remove_node(dev);
    >
    > pci_dev_assign_added(dev, false);
    > }
    > --
    > 2.27.0
    >
    >

    \
     
     \ /
      Last update: 2022-09-02 20:55    [W:4.070 / U:0.592 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site