From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752013AbdBHW14 (ORCPT ); Wed, 8 Feb 2017 17:27:56 -0500 Received: from mail.kernel.org ([198.145.29.136]:36304 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751727AbdBHW1w (ORCPT ); Wed, 8 Feb 2017 17:27:52 -0500 Date: Wed, 8 Feb 2017 16:27:36 -0600 From: Bjorn Helgaas To: Abylay Ospan Cc: Bjorn Helgaas , Ray Jui , Scott Branden , Jon Mason , bcm-kernel-feedback-list@broadcom.com, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Shawn Lin , Wenrui Li Subject: Re: [PATCH] PCI: iproc: fix resource allocation for BCMA PCIe Message-ID: <20170208222736.GA9094@bhelgaas-glaptop.roam.corp.google.com> References: <1484265521-13497-1-git-send-email-aospan@netup.ru> <20170128204407.GA20550@bhelgaas-glaptop.roam.corp.google.com> <20170208204831.GA2206@bhelgaas-glaptop.roam.corp.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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