Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: Pass (protection.outlook.com: domain of amd.com designates
 165.204.84.17 as permitted sender) receiver=protection.outlook.com;
 client-ip=165.204.84.17; helo=SATLEXMB04.amd.com; pr=C
Message-ID: <89f6af46-e995-9456-a7b3-0bbbcb1f70eb@amd.com>
Date:   Mon, 12 Sep 2022 22:49:27 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH RFC 2/2] pci: create device tree node for selected devices
Content-Language: en-US
To:     Rob Herring <robh@kernel.org>
CC:     <linux-pci@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <frowand.list@gmail.com>,
        <helgaas@kernel.org>, <clement.leger@bootlin.com>,
        <max.zhen@amd.com>, <sonal.santan@amd.com>, <larry.liu@amd.com>,
        <brian.xu@amd.com>, <stefano.stabellini@xilinx.com>,
        <trix@redhat.com>
References: <1661809417-11370-1-git-send-email-lizhi.hou@amd.com>
 <1661809417-11370-3-git-send-email-lizhi.hou@amd.com>
 <20220902185403.GA173255-robh@kernel.org>
From:   Lizhi Hou <lizhi.hou@amd.com>
In-Reply-To: <20220902185403.GA173255-robh@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Sep 2022 05:49:52.4449
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: bf53a335-3c3a-46a9-3df2-08da954bc75a
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=3dd8961f-e488-4e60-8e11-a82d994e183d;Ip=[165.204.84.17];Helo=[SATLEXMB04.amd.com]
X-MS-Exchange-CrossTenant-AuthSource: CY4PEPF0000B8EB.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR12MB4431
X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 9/2/22 11:54, Rob Herring wrote:
> 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.

How about changing it to:

And it does not discover the hardware peripherals that are present in a 
PCI device, and which can be accessed through the PCI BARs.

>
>> 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'.
Sure. I will change it to 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.
ok. I will use pci_is_bridge() here.
>
>> +		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)

Ok. So if I understand correctly, in of_pci_removed_node(), it may check 
the flag and assume 'data' is pointing to cset if OF_DYNAMIC is set.

>> +
>> +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.
Got it. I will fix this.
>
>> +	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.
Sure. Looking forward Frank's comment.
>
>> +
>> +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.
In this patch series nobody consumes the dynamic generated node yet, 
Thus I did not add any property. I will add one or two patches to this 
series for the properties you listed.
>
>> +
>> +	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.

Ok. How about introducing new functions of_changeset_create_node(), 
of_changeset_create_property_*()  to of/dynamic.c?

So the function could be like:

of_pci_make_dev_node ()

{

     node = of_changeset_create_node (cset, full_name, parent); //alloc  
of_node and add to cset

     of_changeset_create_property_string(cset, node, name, string);  
//alloc of_property and add to cset

     of_changeset_create_property_u32_array(cset, node, name, array, len);

     ....  add more properties;

     of_changeset_apply(cset)

}


Thanks,

Lizhi

>
>> +#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
>>
>>