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

[Rob Herring]

On Mon, Sep 22, 2014 at 12:55 PM, Liviu Dudau <Liviu.Dudau@xxxxxxx> wrote:
> 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@xxxxxxxxxx>
>> >> Cc: Arnd Bergmann <arnd@xxxxxxxx>
>> >> Cc: Grant Likely <grant.likely@xxxxxxxxxx>
>> >> Cc: Rob Herring <robh+dt@xxxxxxxxxx>
>> >> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> >> Signed-off-by: Liviu Dudau <Liviu.Dudau@xxxxxxx>
>> >> ---
>>
>> [...]
>>
>> >> +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.

Okay, as long as the caller has a single clean-up responsibility, I
guess it is fine.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/