Hi Jan,
On 05/15/2018 08:58 AM, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
>
> of_pci_get_host_bridge_resources() allocates the resource structures it
> fills dynamically, but none of its callers care to release them so far.
> Rather than requiring everyone to do this explicitly, convert the
> existing function to a managed version.
>
> CC: Jingoo Han <jingoohan1@xxxxxxxxx>
> CC: Joao Pinto <Joao.Pinto@xxxxxxxxxxxx>
> CC: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> Signed-off-by: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
[snip]
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 4f21514cb4e4..00f42389aa56 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -244,7 +244,8 @@ EXPORT_SYMBOL_GPL(of_pci_check_probe_only);
>
> #if defined(CONFIG_OF_ADDRESS)
> /**
> - * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT
> + * devm_of_pci_get_host_bridge_resources() - Resource-managed parsing of PCI
> + * host bridge resources from DT
> * @dev: host bridge device
> * @busno: bus number associated with the bridge root bus
> * @bus_max: maximum number of buses for this bridge
> @@ -253,8 +254,6 @@ EXPORT_SYMBOL_GPL(of_pci_check_probe_only);
> * address for the start of the I/O range. Can be NULL if the caller doesn't
> * expect I/O ranges to be present in the device tree.
> *
> - * It is the caller's job to free the @resources list.
> - *
> * This function will parse the "ranges" property of a PCI host bridge device
> * node and setup the resource mapping based on its content. It is expected
> * that the property conforms with the Power ePAPR document.
> @@ -262,12 +261,11 @@ EXPORT_SYMBOL_GPL(of_pci_check_probe_only);
> * It returns zero if the range parsing has been successful or a standard error
> * value if it failed.
> */
> -int of_pci_get_host_bridge_resources(struct device *dev,
> +int devm_of_pci_get_host_bridge_resources(struct device *dev,
> unsigned char busno, unsigned char bus_max,
> struct list_head *resources, resource_size_t *io_base)
> {
> struct device_node *dev_node = dev->of_node;
> - struct resource_entry *window;
> struct resource *res;
> struct resource *bus_range;
> struct of_pci_range range;
> @@ -278,7 +276,7 @@ int of_pci_get_host_bridge_resources(struct device *dev,
> if (io_base)
> *io_base = (resource_size_t)OF_BAD_ADDR;
>
> - bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
> + bus_range = devm_kzalloc(dev, sizeof(*bus_range), GFP_KERNEL);
> if (!bus_range)
> return -ENOMEM;
>
> @@ -300,7 +298,7 @@ int of_pci_get_host_bridge_resources(struct device *dev,
> /* Check for ranges property */
> err = of_pci_range_parser_init(&parser, dev_node);
> if (err)
> - goto parse_failed;
> + return err;
In my opinion allocated by pci_add_resource() and pci_add_resource_offset()
resource entries are leaked on error paths, and pci_free_resource_list() should
be called.
>
> dev_dbg(dev, "Parsing ranges property...\n");
> for_each_of_pci_range(&parser, &range) {
> @@ -322,15 +320,13 @@ int of_pci_get_host_bridge_resources(struct device *dev,
> if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> continue;
>
> - res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> - if (!res) {
> - err = -ENOMEM;
> - goto parse_failed;
> - }
> + res = devm_kzalloc(dev, sizeof(struct resource), GFP_KERNEL);
> + if (!res)
> + return -ENOMEM;
Same as above.
>
> err = of_pci_range_to_resource(&range, dev_node, res);
> if (err) {
> - kfree(res);
> + devm_kfree(dev, res);
> continue;
> }
>
> @@ -339,8 +335,7 @@ int of_pci_get_host_bridge_resources(struct device *dev,
> dev_err(dev,
> "I/O range found for %pOF. Please provide an io_base pointer to save CPU base address\n",
> dev_node);
> - err = -EINVAL;
> - goto conversion_failed;
> + return -EINVAL;
Same as above.
> }
> if (*io_base != (resource_size_t)OF_BAD_ADDR)
> dev_warn(dev,
> @@ -353,16 +348,8 @@ int of_pci_get_host_bridge_resources(struct device *dev,
> }
>
> return 0;
> -
> -conversion_failed:
> - kfree(res);
> -parse_failed:
> - resource_list_for_each_entry(window, resources)
> - kfree(window->res);
> - pci_free_resource_list(resources);
> - return err;
> }
> -EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources);
> +EXPORT_SYMBOL_GPL(devm_of_pci_get_host_bridge_resources);
> #endif /* CONFIG_OF_ADDRESS */
>
> /**
> @@ -606,7 +593,7 @@ int pci_parse_request_of_pci_ranges(struct device *dev,
> struct resource_entry *win, *tmp;
>
> INIT_LIST_HEAD(resources);
> - err = of_pci_get_host_bridge_resources(dev, 0, 0xff, resources,
> + err = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff, resources,
> &iobase);
> if (err)
> return err;
--
With best wishes,
Vladimir
