On Wed, Feb 08, 2017 at 05:07:27PM -0500, Abylay Ospan wrote:
> Hi Bjorn,
>
> I have checked first listed driver
> (drivers/pci/host/pcie-designware.c). Seems like into
> 'devm_request_pci_bus_resources' we supply same stack allocated 'res'
> (actual insert of this pointer to 'iomem_resource' was done inside
> '__request_resource'). This 'res' is not changed inside
> 'of_pci_get_host_bridge_resources'.
> I don't have this platforms on hand and cannot test it on real
> hadrware (to 100% verify). But investigating this code I see that the
> problem exist.
>
> Here is a summary of flow for 'res' to show the problem:
>
> pcie-designware.c:
> LIST_HEAD(res);
> ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &res,
> &pp->io_base); <--- 'res' not changing here
> ret = devm_request_pci_bus_resources(&pdev->dev, &res);
>
> drivers/pci/bus.c:
> err = devm_request_resource(dev, parent, res);
>
> kernel/resource.c:
> conflict = request_resource_conflict(root, new);
> conflict = __request_resource(root, new);
> *p = new; <--- here we introduce stack allocated res into
> global 'iomem_resource'
>
>
> Please check and correct me if i'm wrong ?
The "res" in dw_pcie_host_init() is a list_head (not a struct
resource) and is on the stack.
When we call of_pci_get_host_bridge_resources(), we pass a pointer
("&res") to the empty list. It kzallocs a struct resource for the bus
range and more for any bridge windows, and adds them to the list.
When we call devm_request_pci_bus_resources(), we pass a pointer
("&res") to the list, which is no longer empty. It iterates through
the list and calls devm_request_resource() for each resource. Inside
devm_request_pci_bus_resources(), "res" is the pointer to the resource
(not the list_head), and this resource is the one we kzalloc'd above.
When devm_request_resource() calls request_resource_conflict(), it
passes that pointer to the kzalloc'd resource (the pointer is called
"new" in devm_request_resource()).)
So when __request_resource() assigns "*p = new", it is copying a
pointer to a kzalloc'd struct resource.
This is certainly a twisty maze of similar names for different things,
but I think it is OK if the list_head is on the stack as long as the
struct resources are kzalloc'd.
> > dw_pcie_host_init
> > LIST_HEAD(res) # on stack
> > of_pci_get_host_bridge_resources(np, 0, 0xff, &res, &pp->io_base)
> > res = kzalloc() # different "res" from above!
> > pci_add_resource_offset(resources, res, ...)
> > devm_request_pci_bus_resources(&pdev->dev, &res)
> > pci_scan_root_bus(pp->dev, pp->root_bus_nr, &dw_pcie_ops, pp, &res)
> > error:
> > pci_free_resource_list(&res)
>
> > This looks good to me, but I don't think it's necessary to keep the
> > list_head in the struct iproc_pcie. It should be safe to use
> > "LIST_HEAD(res)" on the stack like the other drivers do. Can you
> > verify that and get an ack from Ray, Scott, or Jon?
>
> if my investigation above is true then we need to keep 'res' all the
> time we working with the driver (or find another way to fix this
> issue).
>
> --
> Abylay Ospan,
> NetUP Inc.
> http://www.netup.tv
