diff mbox series

[2/3] PCI: of: create DT nodes for PCI devices if they do not exists

Message ID 20220427094502.456111-3-clement.leger@bootlin.com (mailing list archive)
State Changes Requested
Headers show
Series add dynamic PCI device of_node creation for overlay | expand

Commit Message

Clément Léger April 27, 2022, 9:45 a.m. UTC 
In order to apply overlays to PCI device nodes, the nodes must first
exist. This commit add support to populate a skeleton tree for PCI bus
and devices. These nodes can then be used by drivers to apply overlays.

Co-developed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Rob Herring <robh@kernel.org>
Signed-off-by: Clément Léger <clement.leger@bootlin.com>
---
 drivers/pci/of.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 184 insertions(+)

Comments

kernel test robot April 27, 2022, 5:37 p.m. UTC | #1 
Hi "Clément,

I love your patch! Yet something to improve:

[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on v5.18-rc4 next-20220427]
[cannot apply to robh/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Cl-ment-L-ger/add-dynamic-PCI-device-of_node-creation-for-overlay/20220427-190828
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: microblaze-randconfig-r005-20220427 (https://download.01.org/0day-ci/archive/20220428/202204280121.ObD8tBkn-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/787f8567f04db060522e198fbdc55a805e99b922
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Cl-ment-L-ger/add-dynamic-PCI-device-of_node-creation-for-overlay/20220427-190828
        git checkout 787f8567f04db060522e198fbdc55a805e99b922
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=microblaze SHELL=/bin/bash drivers/pci/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/pci/of.c: In function 'of_pci_add_property':
>> drivers/pci/of.c:50:9: error: implicit declaration of function 'of_property_set_flag'; did you mean 'of_node_set_flag'? [-Werror=implicit-function-declaration]
      50 |         of_property_set_flag(prop, OF_DYNAMIC);
         |         ^~~~~~~~~~~~~~~~~~~~
         |         of_node_set_flag
>> drivers/pci/of.c:54:15: error: implicit declaration of function 'of_changeset_add_property'; did you mean 'of_pci_add_property'? [-Werror=implicit-function-declaration]
      54 |         ret = of_changeset_add_property(ocs, np, prop);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~~
         |               of_pci_add_property
   drivers/pci/of.c: In function 'of_pci_make_dev_node':
>> drivers/pci/of.c:129:9: error: implicit declaration of function 'of_changeset_init' [-Werror=implicit-function-declaration]
     129 |         of_changeset_init(&cs);
         |         ^~~~~~~~~~~~~~~~~
>> drivers/pci/of.c:147:15: error: implicit declaration of function 'of_changeset_attach_node' [-Werror=implicit-function-declaration]
     147 |         ret = of_changeset_attach_node(&cs, node);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/pci/of.c:151:15: error: implicit declaration of function 'of_changeset_apply' [-Werror=implicit-function-declaration]
     151 |         ret = of_changeset_apply(&cs);
         |               ^~~~~~~~~~~~~~~~~~
>> drivers/pci/of.c:160:9: error: implicit declaration of function 'of_changeset_destroy' [-Werror=implicit-function-declaration]
     160 |         of_changeset_destroy(&cs);
         |         ^~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +50 drivers/pci/of.c

    17	
    18	#ifdef CONFIG_PCI
    19	static int of_pci_add_property(struct of_changeset *ocs, struct device_node *np,
    20				       const char *name, const void *value, int length)
    21	{
    22		struct property *prop;
    23		int ret = -ENOMEM;
    24	
    25		prop = kzalloc(sizeof(*prop), GFP_KERNEL);
    26		if (!prop)
    27			return -ENOMEM;
    28	
    29		prop->name = kstrdup(name, GFP_KERNEL);
    30		if (!prop->name)
    31			goto out_err;
    32	
    33		if (value) {
    34			prop->value = kmemdup(value, length, GFP_KERNEL);
    35			if (!prop->value)
    36				goto out_err;
    37		} else {
    38			/*
    39			 * Even if the property has no value, it must be set to a
    40			 * non-null value since of_get_property() is used to check
    41			 * some values that might or not have a values (ranges for
    42			 * instance). Moreover, when the node is released, prop->value
    43			 * is kfreed so the memory must come from kmalloc.
    44			 */
    45			prop->value = kmalloc(1, GFP_KERNEL);
    46			if (!prop->value)
    47				goto out_err;
    48		}
    49	
  > 50		of_property_set_flag(prop, OF_DYNAMIC);
    51	
    52		prop->length = length;
    53	
  > 54		ret = of_changeset_add_property(ocs, np, prop);
    55		if (!ret)
    56			return 0;
    57	
    58	out_err:
    59		kfree(prop->value);
    60		kfree(prop->name);
    61		kfree(prop);
    62	
    63		return ret;
    64	}
    65	
    66	static struct device_node *of_pci_make_node(void)
    67	{
    68		struct device_node *node;
    69	
    70		node = kzalloc(sizeof(*node), GFP_KERNEL);
    71		if (!node)
    72			return NULL;
    73	
    74		of_node_set_flag(node, OF_DYNAMIC);
    75		of_node_set_flag(node, OF_DETACHED);
    76		of_node_init(node);
    77	
    78		return node;
    79	}
    80	
    81	static int of_pci_add_cells_props(struct device_node *node,
    82					  struct of_changeset *cs, int n_addr_cells,
    83					  int n_size_cells)
    84	{
    85		__be32 val;
    86		int ret;
    87	
    88		ret = of_pci_add_property(cs, node, "ranges", NULL, 0);
    89		if (ret)
    90			return ret;
    91	
    92		val = __cpu_to_be32(n_addr_cells);
    93		ret = of_pci_add_property(cs, node, "#address-cells", &val,
    94					  sizeof(__be32));
    95		if (ret)
    96			return ret;
    97	
    98		val = __cpu_to_be32(n_size_cells);
    99		return of_pci_add_property(cs, node, "#size-cells", &val,
   100					   sizeof(__be32));
   101	}
   102	
   103	static int of_pci_add_pci_bus_props(struct device_node *node,
   104					    struct of_changeset *cs)
   105	{
   106		int ret;
   107	
   108		ret = of_pci_add_property(cs, node, "device_type", "pci",
   109					  strlen("pci") + 1);
   110		if (ret)
   111			return ret;
   112	
   113		return of_pci_add_cells_props(node, cs, 3, 2);
   114	}
   115	
   116	static void of_pci_make_dev_node(struct pci_dev *dev)
   117	{
   118		static struct of_changeset cs;
   119		const char *pci_type = "dev";
   120		struct device_node *node;
   121		__be32 val[5] = {0};
   122		int ret;
   123	
   124		node = of_pci_make_node();
   125		if (!node)
   126			return;
   127	
   128		node->parent = dev->bus->dev.of_node;
 > 129		of_changeset_init(&cs);
   130	
   131		if (pci_is_bridge(dev)) {
   132			ret = of_pci_add_pci_bus_props(node, &cs);
   133			if (ret)
   134				goto changeset_destroy;
   135			pci_type = "pci";
   136		}
   137	
   138		node->full_name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
   139					    PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
   140	
   141		val[0] = __cpu_to_be32(dev->devfn << 8);
   142		val[4] = __cpu_to_be32(SZ_4K);
   143		ret = of_pci_add_property(&cs, node, "reg", val, 5 * sizeof(__be32));
   144		if (ret)
   145			goto changeset_destroy;
   146	
 > 147		ret = of_changeset_attach_node(&cs, node);
   148		if (ret)
   149			goto changeset_destroy;
   150	
 > 151		ret = of_changeset_apply(&cs);
   152		if (ret)
   153			goto changeset_destroy;
   154	
   155		dev->dev.of_node = node;
   156	
   157		return;
   158	
   159	changeset_destroy:
 > 160		of_changeset_destroy(&cs);
   161		kfree(node);
   162	}
   163
kernel test robot April 27, 2022, 5:47 p.m. UTC | #2 
Hi "Clément,

I love your patch! Yet something to improve:

[auto build test ERROR on helgaas-pci/next]
[also build test ERROR on v5.18-rc4 next-20220427]
[cannot apply to robh/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Cl-ment-L-ger/add-dynamic-PCI-device-of_node-creation-for-overlay/20220427-190828
base:   https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
config: sparc64-buildonly-randconfig-r003-20220427 (https://download.01.org/0day-ci/archive/20220428/202204280126.T5VOPZdN-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/787f8567f04db060522e198fbdc55a805e99b922
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Cl-ment-L-ger/add-dynamic-PCI-device-of_node-creation-for-overlay/20220427-190828
        git checkout 787f8567f04db060522e198fbdc55a805e99b922
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=sparc64 SHELL=/bin/bash drivers/pci/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/pci/of.c: In function 'of_pci_add_property':
>> drivers/pci/of.c:54:15: error: implicit declaration of function 'of_changeset_add_property'; did you mean 'of_pci_add_property'? [-Werror=implicit-function-declaration]
      54 |         ret = of_changeset_add_property(ocs, np, prop);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~~
         |               of_pci_add_property
   drivers/pci/of.c: In function 'of_pci_make_dev_node':
>> drivers/pci/of.c:129:9: error: implicit declaration of function 'of_changeset_init' [-Werror=implicit-function-declaration]
     129 |         of_changeset_init(&cs);
         |         ^~~~~~~~~~~~~~~~~
>> drivers/pci/of.c:147:15: error: implicit declaration of function 'of_changeset_attach_node' [-Werror=implicit-function-declaration]
     147 |         ret = of_changeset_attach_node(&cs, node);
         |               ^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/pci/of.c:151:15: error: implicit declaration of function 'of_changeset_apply' [-Werror=implicit-function-declaration]
     151 |         ret = of_changeset_apply(&cs);
         |               ^~~~~~~~~~~~~~~~~~
>> drivers/pci/of.c:160:9: error: implicit declaration of function 'of_changeset_destroy' [-Werror=implicit-function-declaration]
     160 |         of_changeset_destroy(&cs);
         |         ^~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +54 drivers/pci/of.c

    17	
    18	#ifdef CONFIG_PCI
    19	static int of_pci_add_property(struct of_changeset *ocs, struct device_node *np,
    20				       const char *name, const void *value, int length)
    21	{
    22		struct property *prop;
    23		int ret = -ENOMEM;
    24	
    25		prop = kzalloc(sizeof(*prop), GFP_KERNEL);
    26		if (!prop)
    27			return -ENOMEM;
    28	
    29		prop->name = kstrdup(name, GFP_KERNEL);
    30		if (!prop->name)
    31			goto out_err;
    32	
    33		if (value) {
    34			prop->value = kmemdup(value, length, GFP_KERNEL);
    35			if (!prop->value)
    36				goto out_err;
    37		} else {
    38			/*
    39			 * Even if the property has no value, it must be set to a
    40			 * non-null value since of_get_property() is used to check
    41			 * some values that might or not have a values (ranges for
    42			 * instance). Moreover, when the node is released, prop->value
    43			 * is kfreed so the memory must come from kmalloc.
    44			 */
    45			prop->value = kmalloc(1, GFP_KERNEL);
    46			if (!prop->value)
    47				goto out_err;
    48		}
    49	
    50		of_property_set_flag(prop, OF_DYNAMIC);
    51	
    52		prop->length = length;
    53	
  > 54		ret = of_changeset_add_property(ocs, np, prop);
    55		if (!ret)
    56			return 0;
    57	
    58	out_err:
    59		kfree(prop->value);
    60		kfree(prop->name);
    61		kfree(prop);
    62	
    63		return ret;
    64	}
    65	
    66	static struct device_node *of_pci_make_node(void)
    67	{
    68		struct device_node *node;
    69	
    70		node = kzalloc(sizeof(*node), GFP_KERNEL);
    71		if (!node)
    72			return NULL;
    73	
    74		of_node_set_flag(node, OF_DYNAMIC);
    75		of_node_set_flag(node, OF_DETACHED);
    76		of_node_init(node);
    77	
    78		return node;
    79	}
    80	
    81	static int of_pci_add_cells_props(struct device_node *node,
    82					  struct of_changeset *cs, int n_addr_cells,
    83					  int n_size_cells)
    84	{
    85		__be32 val;
    86		int ret;
    87	
    88		ret = of_pci_add_property(cs, node, "ranges", NULL, 0);
    89		if (ret)
    90			return ret;
    91	
    92		val = __cpu_to_be32(n_addr_cells);
    93		ret = of_pci_add_property(cs, node, "#address-cells", &val,
    94					  sizeof(__be32));
    95		if (ret)
    96			return ret;
    97	
    98		val = __cpu_to_be32(n_size_cells);
    99		return of_pci_add_property(cs, node, "#size-cells", &val,
   100					   sizeof(__be32));
   101	}
   102	
   103	static int of_pci_add_pci_bus_props(struct device_node *node,
   104					    struct of_changeset *cs)
   105	{
   106		int ret;
   107	
   108		ret = of_pci_add_property(cs, node, "device_type", "pci",
   109					  strlen("pci") + 1);
   110		if (ret)
   111			return ret;
   112	
   113		return of_pci_add_cells_props(node, cs, 3, 2);
   114	}
   115	
   116	static void of_pci_make_dev_node(struct pci_dev *dev)
   117	{
   118		static struct of_changeset cs;
   119		const char *pci_type = "dev";
   120		struct device_node *node;
   121		__be32 val[5] = {0};
   122		int ret;
   123	
   124		node = of_pci_make_node();
   125		if (!node)
   126			return;
   127	
   128		node->parent = dev->bus->dev.of_node;
 > 129		of_changeset_init(&cs);
   130	
   131		if (pci_is_bridge(dev)) {
   132			ret = of_pci_add_pci_bus_props(node, &cs);
   133			if (ret)
   134				goto changeset_destroy;
   135			pci_type = "pci";
   136		}
   137	
   138		node->full_name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
   139					    PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
   140	
   141		val[0] = __cpu_to_be32(dev->devfn << 8);
   142		val[4] = __cpu_to_be32(SZ_4K);
   143		ret = of_pci_add_property(&cs, node, "reg", val, 5 * sizeof(__be32));
   144		if (ret)
   145			goto changeset_destroy;
   146	
 > 147		ret = of_changeset_attach_node(&cs, node);
   148		if (ret)
   149			goto changeset_destroy;
   150	
 > 151		ret = of_changeset_apply(&cs);
   152		if (ret)
   153			goto changeset_destroy;
   154	
   155		dev->dev.of_node = node;
   156	
   157		return;
   158	
   159	changeset_destroy:
 > 160		of_changeset_destroy(&cs);
   161		kfree(node);
   162	}
   163
Rob Herring May 3, 2022, 2:12 p.m. UTC | #3 
On Wed, Apr 27, 2022 at 11:45:01AM +0200, Clément Léger wrote:
> In order to apply overlays to PCI device nodes, the nodes must first
> exist. This commit add support to populate a skeleton tree for PCI bus
> and devices. These nodes can then be used by drivers to apply overlays.
> 

While I implemented this creating the nodes as the PCI devices are 
created, I think probably we're going to want to create the device node 
and any needed parent nodes on demand. Otherwise, just turning on 
CONFIG_OF could break platforms.

One potential issue is that fwnode assumes there is either a DT node or 
ACPI node. With this, we have the potential for both. I'm not sure how 
much that's going to be an issue.

> Co-developed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---
>  drivers/pci/of.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 184 insertions(+)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index cb2e8351c2cc..f2325708726e 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -16,12 +16,194 @@
>  #include "pci.h"
>  
>  #ifdef CONFIG_PCI
> +static int of_pci_add_property(struct of_changeset *ocs, struct device_node *np,
> +			       const char *name, const void *value, int length)

Nothing really PCI specific about this function.

The kernel support for creating nodes and properties is pretty poor. We 
should improve it with functions like this (in drivers/of/). Maybe the 
changeset part should be separate though. We have some cases of creating 
properties or nodes already, and whatever new APIs we make those 
cases should be able to use them. And if they are converted, then it can 
be merged sooner rather than when all the PCI parts are ready.

> +{
> +	struct property *prop;
> +	int ret = -ENOMEM;
> +
> +	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	prop->name = kstrdup(name, GFP_KERNEL);
> +	if (!prop->name)
> +		goto out_err;
> +
> +	if (value) {
> +		prop->value = kmemdup(value, length, GFP_KERNEL);
> +		if (!prop->value)
> +			goto out_err;
> +	} else {
> +		/*
> +		 * Even if the property has no value, it must be set to a
> +		 * non-null value since of_get_property() is used to check
> +		 * some values that might or not have a values (ranges for
> +		 * instance). Moreover, when the node is released, prop->value
> +		 * is kfreed so the memory must come from kmalloc.
> +		 */
> +		prop->value = kmalloc(1, GFP_KERNEL);
> +		if (!prop->value)
> +			goto out_err;
> +	}
> +
> +	of_property_set_flag(prop, OF_DYNAMIC);
> +
> +	prop->length = length;
> +
> +	ret = of_changeset_add_property(ocs, np, prop);
> +	if (!ret)
> +		return 0;
> +
> +out_err:
> +	kfree(prop->value);
> +	kfree(prop->name);
> +	kfree(prop);
> +
> +	return ret;
> +}
> +
> +static struct device_node *of_pci_make_node(void)
> +{

This isn't PCI specific either.

> +	struct device_node *node;
> +
> +	node = kzalloc(sizeof(*node), GFP_KERNEL);
> +	if (!node)
> +		return NULL;
> +
> +	of_node_set_flag(node, OF_DYNAMIC);
> +	of_node_set_flag(node, OF_DETACHED);
> +	of_node_init(node);
> +
> +	return node;
> +}
> +
> +static int of_pci_add_cells_props(struct device_node *node,
> +				  struct of_changeset *cs, int n_addr_cells,
> +				  int n_size_cells)
> +{
> +	__be32 val;
> +	int ret;
> +
> +	ret = of_pci_add_property(cs, node, "ranges", NULL, 0);

The host bridge node is going to need to fill in 'ranges'. Empty ranges 
is not valid when there's a change in number of cells.

The root node also will need "#address-cells" and "#size-cells".
 
> +	if (ret)
> +		return ret;
> +
> +	val = __cpu_to_be32(n_addr_cells);
> +	ret = of_pci_add_property(cs, node, "#address-cells", &val,
> +				  sizeof(__be32));
> +	if (ret)
> +		return ret;
> +
> +	val = __cpu_to_be32(n_size_cells);
> +	return of_pci_add_property(cs, node, "#size-cells", &val,
> +				   sizeof(__be32));
> +}
> +
> +static int of_pci_add_pci_bus_props(struct device_node *node,
> +				    struct of_changeset *cs)
> +{
> +	int ret;
> +
> +	ret = of_pci_add_property(cs, node, "device_type", "pci",
> +				  strlen("pci") + 1);
> +	if (ret)
> +		return ret;
> +
> +	return of_pci_add_cells_props(node, cs, 3, 2);
> +}
> +
> +static void of_pci_make_dev_node(struct pci_dev *dev)
> +{
> +	static struct of_changeset cs;
> +	const char *pci_type = "dev";
> +	struct device_node *node;
> +	__be32 val[5] = {0};
> +	int ret;
> +
> +	node = of_pci_make_node();
> +	if (!node)
> +		return;
> +
> +	node->parent = dev->bus->dev.of_node;
> +	of_changeset_init(&cs);
> +
> +	if (pci_is_bridge(dev)) {
> +		ret = of_pci_add_pci_bus_props(node, &cs);
> +		if (ret)
> +			goto changeset_destroy;
> +		pci_type = "pci";
> +	}
> +
> +	node->full_name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
> +				    PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
> +
> +	val[0] = __cpu_to_be32(dev->devfn << 8);
> +	val[4] = __cpu_to_be32(SZ_4K);
> +	ret = of_pci_add_property(&cs, node, "reg", val, 5 * sizeof(__be32));
> +	if (ret)
> +		goto changeset_destroy;
> +
> +	ret = of_changeset_attach_node(&cs, node);
> +	if (ret)
> +		goto changeset_destroy;
> +
> +	ret = of_changeset_apply(&cs);
> +	if (ret)
> +		goto changeset_destroy;
> +
> +	dev->dev.of_node = node;
> +
> +	return;
> +
> +changeset_destroy:
> +	of_changeset_destroy(&cs);
> +	kfree(node);
> +}
> +
> +static struct device_node *of_pci_create_root_bus_node(struct pci_bus *bus)
> +{
> +	static struct of_changeset cs;
> +	struct device_node *node;
> +	int ret;
> +
> +	node = of_pci_make_node();
> +	if (!node)
> +		return NULL;
> +
> +	node->full_name = "pci";
> +	node->parent = of_root;
> +
> +	of_changeset_init(&cs);
> +	ret = of_pci_add_pci_bus_props(node, &cs);
> +	if (ret)
> +		goto changeset_destroy;
> +
> +	ret = of_changeset_attach_node(&cs, node);
> +	if (ret)
> +		goto changeset_destroy;
> +
> +	ret = of_changeset_apply(&cs);
> +	if (ret)
> +		goto changeset_destroy;
> +
> +	return node;
> +
> +changeset_destroy:
> +	of_changeset_destroy(&cs);
> +	kfree(node);
> +
> +	return NULL;
> +}
> +
>  void pci_set_of_node(struct pci_dev *dev)
>  {
>  	if (!dev->bus->dev.of_node)
>  		return;
>  	dev->dev.of_node = of_pci_find_child_device(dev->bus->dev.of_node,
>  						    dev->devfn);
> +	if (!dev->dev.of_node)
> +		of_pci_make_dev_node(dev);
>  	if (dev->dev.of_node)
>  		dev->dev.fwnode = &dev->dev.of_node->fwnode;
>  }
> @@ -39,6 +221,8 @@ void pci_set_bus_of_node(struct pci_bus *bus)
>  
>  	if (bus->self == NULL) {
>  		node = pcibios_get_phb_of_node(bus);
> +		if (!node)
> +			node = of_pci_create_root_bus_node(bus);
>  	} else {
>  		node = of_node_get(bus->self->dev.of_node);
>  		if (node && of_property_read_bool(node, "external-facing"))
> -- 
> 2.34.1
> 
>
Clément Léger May 3, 2022, 4:05 p.m. UTC | #4 
Le Tue, 3 May 2022 09:12:06 -0500,
Rob Herring <robh@kernel.org> a écrit :

> On Wed, Apr 27, 2022 at 11:45:01AM +0200, Clément Léger wrote:
> > In order to apply overlays to PCI device nodes, the nodes must first
> > exist. This commit add support to populate a skeleton tree for PCI bus
> > and devices. These nodes can then be used by drivers to apply overlays.
> >   
> 
> While I implemented this creating the nodes as the PCI devices are 
> created, I think probably we're going to want to create the device node 
> and any needed parent nodes on demand. Otherwise, just turning on 
> CONFIG_OF could break platforms.

Ok, so this creation would potentially be done on request from some PCI
driver that want to apply it's overlay on the tree. Should I actually
add some function such as of_pci_apply_overlay() which would create the
PCI node tree if not present and apply the overlay to the of_node that
is associated to the PCIe device ?

> 
> One potential issue is that fwnode assumes there is either a DT node or 
> ACPI node. With this, we have the potential for both. I'm not sure how 
> much that's going to be an issue.

Not sure either but that's better not to play with that.

> 
> > Co-developed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> > ---
> >  drivers/pci/of.c | 184 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 184 insertions(+)
> > 
> > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > index cb2e8351c2cc..f2325708726e 100644
> > --- a/drivers/pci/of.c
> > +++ b/drivers/pci/of.c
> > @@ -16,12 +16,194 @@
> >  #include "pci.h"
> >  
> >  #ifdef CONFIG_PCI
> > +static int of_pci_add_property(struct of_changeset *ocs, struct device_node *np,
> > +			       const char *name, const void *value, int length)  
> 
> Nothing really PCI specific about this function.
> 
> The kernel support for creating nodes and properties is pretty poor. We 
> should improve it with functions like this (in drivers/of/). Maybe the 
> changeset part should be separate though. We have some cases of creating 
> properties or nodes already, and whatever new APIs we make those 
> cases should be able to use them. And if they are converted, then it can 
> be merged sooner rather than when all the PCI parts are ready.

Ok, so this will be done as a first separate series to add property
creation then.

> > +
> > +static int of_pci_add_cells_props(struct device_node *node,
> > +				  struct of_changeset *cs, int n_addr_cells,
> > +				  int n_size_cells)
> > +{
> > +	__be32 val;
> > +	int ret;
> > +
> > +	ret = of_pci_add_property(cs, node, "ranges", NULL, 0);  
> 
> The host bridge node is going to need to fill in 'ranges'. Empty ranges 
> is not valid when there's a change in number of cells.

Ok, wasn't aware of that. If I understand, I'll need to obtain the
range of PCI addresses that are behind the bridge to fill in this
ranges property right ?

> 
> The root node also will need "#address-cells" and "#size-cells".
>  

Ok.
Bjorn Helgaas May 3, 2022, 10:53 p.m. UTC | #5 
In subject:

  PCI: of: Create DT nodes ... if they do not exist

The subject could be read as saying that you're going to create DT
nodes before the PCI devices exist, but I think you mean that when we
enumerate a PCI devices, we're *always* going to create a DT node for
it, even if the DT didn't mention it.

Maybe something like:

  PCI: of: Create DT node for every PCI device

although I see Rob thinks this should be done on demand instead of
doing it for every device, which sounds sensible to me.

On Wed, Apr 27, 2022 at 11:45:01AM +0200, Clément Léger wrote:
> In order to apply overlays to PCI device nodes, the nodes must first
> exist. This commit add support to populate a skeleton tree for PCI bus
> and devices. These nodes can then be used by drivers to apply overlays.

s/This commit add support/Add support/

Bjorn
Clément Léger May 4, 2022, 1:43 p.m. UTC | #6 
Le Tue, 3 May 2022 17:53:53 -0500,
Bjorn Helgaas <helgaas@kernel.org> a écrit :

> In subject:
> 
>   PCI: of: Create DT nodes ... if they do not exist
> 
> The subject could be read as saying that you're going to create DT
> nodes before the PCI devices exist, but I think you mean that when we
> enumerate a PCI devices, we're *always* going to create a DT node for
> it, even if the DT didn't mention it.

Hi Bjorn,

Indeed ! I'll modify that.

> 
> Maybe something like:
> 
>   PCI: of: Create DT node for every PCI device
> 
> although I see Rob thinks this should be done on demand instead of
> doing it for every device, which sounds sensible to me.

Agreed, I'll rework this series.

Thanks,

> 
> On Wed, Apr 27, 2022 at 11:45:01AM +0200, Clément Léger wrote:
> > In order to apply overlays to PCI device nodes, the nodes must first
> > exist. This commit add support to populate a skeleton tree for PCI bus
> > and devices. These nodes can then be used by drivers to apply overlays.  
> 
> s/This commit add support/Add support/
> 
> Bjorn
Lizhi Hou May 18, 2022, 7:22 p.m. UTC | #7 
On 5/4/22 06:43, Clément Léger wrote:
> Le Tue, 3 May 2022 17:53:53 -0500,
> Bjorn Helgaas <helgaas@kernel.org> a écrit :
>
>> In subject:
>>
>>    PCI: of: Create DT nodes ... if they do not exist
>>
>> The subject could be read as saying that you're going to create DT
>> nodes before the PCI devices exist, but I think you mean that when we
>> enumerate a PCI devices, we're *always* going to create a DT node for
>> it, even if the DT didn't mention it.
> Hi Bjorn,
>
> Indeed ! I'll modify that.

Linking the dynamic generated dt node to PCIe device through 
pci_dev->dev.of_node may cause issues. Kernel and driver code may check 
of_node pointer and run complete different code path if of_node is not 
NULL.

For example:  in of_irq_parse_pci(): 
https://elixir.bootlin.com/linux/v5.18-rc2/source/drivers/pci/of.c#L492

I encountered different issues when I tried to create a prototype. And I 
have sent all may questions/thoughts through 
https://lore.kernel.org/lkml/79e4c876-e5a4-861f-cfbc-c75ed1a07ddd@xilinx.com/#t

I am wondering what would be the right way to resolve it?


Thanks,

Lizhi

>
>> Maybe something like:
>>
>>    PCI: of: Create DT node for every PCI device
>>
>> although I see Rob thinks this should be done on demand instead of
>> doing it for every device, which sounds sensible to me.
> Agreed, I'll rework this series.
>
> Thanks,
>
>> On Wed, Apr 27, 2022 at 11:45:01AM +0200, Clément Léger wrote:
>>> In order to apply overlays to PCI device nodes, the nodes must first
>>> exist. This commit add support to populate a skeleton tree for PCI bus
>>> and devices. These nodes can then be used by drivers to apply overlays.
>> s/This commit add support/Add support/
>>
>> Bjorn
>
>
diff mbox series

Patch

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index cb2e8351c2cc..f2325708726e 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -16,12 +16,194 @@ 
 #include "pci.h"
 
 #ifdef CONFIG_PCI
+static int of_pci_add_property(struct of_changeset *ocs, struct device_node *np,
+			       const char *name, const void *value, int length)
+{
+	struct property *prop;
+	int ret = -ENOMEM;
+
+	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
+	if (!prop)
+		return -ENOMEM;
+
+	prop->name = kstrdup(name, GFP_KERNEL);
+	if (!prop->name)
+		goto out_err;
+
+	if (value) {
+		prop->value = kmemdup(value, length, GFP_KERNEL);
+		if (!prop->value)
+			goto out_err;
+	} else {
+		/*
+		 * Even if the property has no value, it must be set to a
+		 * non-null value since of_get_property() is used to check
+		 * some values that might or not have a values (ranges for
+		 * instance). Moreover, when the node is released, prop->value
+		 * is kfreed so the memory must come from kmalloc.
+		 */
+		prop->value = kmalloc(1, GFP_KERNEL);
+		if (!prop->value)
+			goto out_err;
+	}
+
+	of_property_set_flag(prop, OF_DYNAMIC);
+
+	prop->length = length;
+
+	ret = of_changeset_add_property(ocs, np, prop);
+	if (!ret)
+		return 0;
+
+out_err:
+	kfree(prop->value);
+	kfree(prop->name);
+	kfree(prop);
+
+	return ret;
+}
+
+static struct device_node *of_pci_make_node(void)
+{
+	struct device_node *node;
+
+	node = kzalloc(sizeof(*node), GFP_KERNEL);
+	if (!node)
+		return NULL;
+
+	of_node_set_flag(node, OF_DYNAMIC);
+	of_node_set_flag(node, OF_DETACHED);
+	of_node_init(node);
+
+	return node;
+}
+
+static int of_pci_add_cells_props(struct device_node *node,
+				  struct of_changeset *cs, int n_addr_cells,
+				  int n_size_cells)
+{
+	__be32 val;
+	int ret;
+
+	ret = of_pci_add_property(cs, node, "ranges", NULL, 0);
+	if (ret)
+		return ret;
+
+	val = __cpu_to_be32(n_addr_cells);
+	ret = of_pci_add_property(cs, node, "#address-cells", &val,
+				  sizeof(__be32));
+	if (ret)
+		return ret;
+
+	val = __cpu_to_be32(n_size_cells);
+	return of_pci_add_property(cs, node, "#size-cells", &val,
+				   sizeof(__be32));
+}
+
+static int of_pci_add_pci_bus_props(struct device_node *node,
+				    struct of_changeset *cs)
+{
+	int ret;
+
+	ret = of_pci_add_property(cs, node, "device_type", "pci",
+				  strlen("pci") + 1);
+	if (ret)
+		return ret;
+
+	return of_pci_add_cells_props(node, cs, 3, 2);
+}
+
+static void of_pci_make_dev_node(struct pci_dev *dev)
+{
+	static struct of_changeset cs;
+	const char *pci_type = "dev";
+	struct device_node *node;
+	__be32 val[5] = {0};
+	int ret;
+
+	node = of_pci_make_node();
+	if (!node)
+		return;
+
+	node->parent = dev->bus->dev.of_node;
+	of_changeset_init(&cs);
+
+	if (pci_is_bridge(dev)) {
+		ret = of_pci_add_pci_bus_props(node, &cs);
+		if (ret)
+			goto changeset_destroy;
+		pci_type = "pci";
+	}
+
+	node->full_name = kasprintf(GFP_KERNEL, "%s@%x,%x", pci_type,
+				    PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
+
+	val[0] = __cpu_to_be32(dev->devfn << 8);
+	val[4] = __cpu_to_be32(SZ_4K);
+	ret = of_pci_add_property(&cs, node, "reg", val, 5 * sizeof(__be32));
+	if (ret)
+		goto changeset_destroy;
+
+	ret = of_changeset_attach_node(&cs, node);
+	if (ret)
+		goto changeset_destroy;
+
+	ret = of_changeset_apply(&cs);
+	if (ret)
+		goto changeset_destroy;
+
+	dev->dev.of_node = node;
+
+	return;
+
+changeset_destroy:
+	of_changeset_destroy(&cs);
+	kfree(node);
+}
+
+static struct device_node *of_pci_create_root_bus_node(struct pci_bus *bus)
+{
+	static struct of_changeset cs;
+	struct device_node *node;
+	int ret;
+
+	node = of_pci_make_node();
+	if (!node)
+		return NULL;
+
+	node->full_name = "pci";
+	node->parent = of_root;
+
+	of_changeset_init(&cs);
+	ret = of_pci_add_pci_bus_props(node, &cs);
+	if (ret)
+		goto changeset_destroy;
+
+	ret = of_changeset_attach_node(&cs, node);
+	if (ret)
+		goto changeset_destroy;
+
+	ret = of_changeset_apply(&cs);
+	if (ret)
+		goto changeset_destroy;
+
+	return node;
+
+changeset_destroy:
+	of_changeset_destroy(&cs);
+	kfree(node);
+
+	return NULL;
+}
+
 void pci_set_of_node(struct pci_dev *dev)
 {
 	if (!dev->bus->dev.of_node)
 		return;
 	dev->dev.of_node = of_pci_find_child_device(dev->bus->dev.of_node,
 						    dev->devfn);
+	if (!dev->dev.of_node)
+		of_pci_make_dev_node(dev);
 	if (dev->dev.of_node)
 		dev->dev.fwnode = &dev->dev.of_node->fwnode;
 }
@@ -39,6 +221,8 @@  void pci_set_bus_of_node(struct pci_bus *bus)
 
 	if (bus->self == NULL) {
 		node = pcibios_get_phb_of_node(bus);
+		if (!node)
+			node = of_pci_create_root_bus_node(bus);
 	} else {
 		node = of_node_get(bus->self->dev.of_node);
 		if (node && of_property_read_bool(node, "external-facing"))