Re: [PATCH v11 08/10] OF: PCI: Add support for parsing PCI host bridge resources from DT

[Liviu Dudau <Liviu.Dudau@arm.com>]

On Sat, Sep 20, 2014 at 01:28:38AM +0100, Rob Herring wrote:
> On 09/19/2014 04:06 PM, Bjorn Helgaas wrote:
> > On Thu, Sep 18, 2014 at 02:30:23AM +0100, Liviu Dudau wrote:
> >> Provide a function to parse the PCI DT ranges that can be used to
> >> create a pci_host_bridge structure together with its associated
> >> bus.
> >>
> >> Cc: Bjorn Helgaas <bhelgaas@google.com>
> >> Cc: Arnd Bergmann <arnd@arndb.de>
> >> Cc: Grant Likely <grant.likely@linaro.org>
> >> Cc: Rob Herring <robh+dt@kernel.org>
> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> >> ---
> 
> [...]
> 
> >> +int of_pci_get_host_bridge_resources(struct device_node *dev,
> >> +			unsigned char busno, unsigned char bus_max,
> >> +			struct list_head *resources, resource_size_t *io_base)
> >> +{
> >> +	struct resource *res;
> >> +	struct resource *bus_range;
> >> +	struct of_pci_range range;
> >> +	struct of_pci_range_parser parser;
> >> +	char range_type[4];
> >> +	int err;
> >> +
> >> +	if (!io_base)
> >> +		return -EINVAL;
> >> +	*io_base = OF_BAD_ADDR;
> >> +
> >> +	bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
> 
> This function does a lot of kalloc's but there is not an easy way to
> undo those allocations. Hot unplug of a host bridge or probe error
> handling would leak memory.
> 
> You could pass in struct device and use the devm_ variant (also
> addressing Bjorn's comment), but not having an uninit/remove function
> make what clean-up drivers have to do error prone. For example, on
> uninit a driver needs to call pci_free_resource_list.

If the function fails to parse the ranges for whatever reason it will
call pci_free_resource_list on the resources that have been added already
and it will clean up. If it is successful, then it is the job of the caller
to free the list, as mentioned in the comment associated with the function.

The reason why I am reluctant to use devm_ here is that the ranges are only
parsed here, no filtering is applied. Architectures and/or host bridge
drivers can/could impose additional restrictions to the list before passing
it to pci_scan_root_bus() for example.

Best regards,
Liviu

> 
> Rob
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯