Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933258AbaKROtZ (ORCPT ); Tue, 18 Nov 2014 09:49:25 -0500 Received: from service87.mimecast.com ([91.220.42.44]:58323 "EHLO service87.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754985AbaKROsY convert rfc822-to-8bit (ORCPT ); Tue, 18 Nov 2014 09:48:24 -0500 Date: Tue, 18 Nov 2014 14:48:19 +0000 From: Liviu Dudau To: Yijing Wang Cc: Arnd Bergmann , "linuxppc-dev@lists.ozlabs.org" , Bjorn Helgaas , Liviu Dudau , Tony Luck , Russell King , "linux-pci@vger.kernel.org" , "x86@kernel.org" , "linux-kernel@vger.kernel.org" , "huxinwei@huawei.com" , Thierry Reding , "suravee.suthikulpanit@amd.com" , "linux-ia64@vger.kernel.org" , Thomas Gleixner , Wuyun , "linux-arm-kernel@lists.infradead.org" Subject: Re: [RFC PATCH 07/16] PCI: Separate pci_host_bridge creation out of pci_create_root_bus() Message-ID: <20141118144819.GK12037@e106497-lin.cambridge.arm.com> References: <1416219710-26088-1-git-send-email-wangyijing@huawei.com> <1416219710-26088-8-git-send-email-wangyijing@huawei.com> <2507218.mHliopJb05@wuerfel> <546B041A.4060403@huawei.com> MIME-Version: 1.0 In-Reply-To: <546B041A.4060403@huawei.com> User-Agent: Mutt/1.5.22 (2013-10-16) X-OriginalArrivalTime: 18 Nov 2014 14:48:21.0067 (UTC) FILETIME=[B275E1B0:01D0033E] X-MC-Unique: 114111814482207501 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Content-Disposition: inline Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 18, 2014 at 08:32:26AM +0000, Yijing Wang wrote: > > >> +LIST_HEAD(pci_host_bridge_list); > >> +DECLARE_RWSEM(pci_host_bridge_sem); > > > > Unless the pci_host_bridge_sem is accessed thousands of times per second, > > it's normally better to use a simple mutex instead. > > OK, I will use simple mutex instead. > > > > >> +static struct resource busn_resource = { > >> + .name = "PCI busn", > >> + .start = 0, > >> + .end = 255, > >> + .flags = IORESOURCE_BUS, > >> +}; > > > > I think it would be better to require callers to pass the bus resource > > down to the function. > > Hmm, I think most of caller will provide the bus resource, but some others > will not give any bus resource, extremely, no any resources :(. But we still > need properly configure their resources for compatibility. > > > > >> +struct pci_host_bridge *pci_create_host_bridge( > >> + struct device *parent, u32 db, > >> + struct pci_ops *ops, void *sysdata, > >> + struct list_head *resources) > >> +{ > > > > Do we still need to pass the 'sysdata' in here? If we are guaranteed to > > have a device pointer, we should always be able to get the driver > > private data from dev_get_drvdata(host->dev->parent). > > We need, some platforms pass NULL pointer as host bridge parent. Yijing, May I suggest a different approach here? Rather than having to pass an opaque pointer that gets converted by the host bridge driver back to the private structure, what about promoting a new style of usage, that is similar to the way device drivers work? Lets try to promote the embedding of the generic pci_host_bridge structure in the host bridge specific structure! Then we can access the private data doing container_of(). Something like this: struct pci_controller { struct pci_host_bridge bridge; /* private host bridge data here */ ..... }; #define PCI_CONTROLLER(bus) ({ struct pci_host_bridge *hb = to_pci_host_bridge(bus->bridge); \ container_of(hb, struct pci_controller, bridge); }) Then we can retrieve the host bridge structure from everywhere we have a device. Best regards, Liviu > > > > >> + host = kzalloc(sizeof(*host), GFP_KERNEL); > >> + if (!host) > >> + return NULL; > > > > devm_kzalloc maybe? > > I don't know much detail about devm_kzalloc(), but we have no pci host driver > here, and I found no devm_kzalloc() uses in core PCI code before. > > > > >> + if (!resources) { > >> + /* Use default IO/MEM/BUS resources*/ > >> + pci_add_resource(&host->windows, &ioport_resource); > >> + pci_add_resource(&host->windows, &iomem_resource); > >> + pci_add_resource(&host->windows, &busn_resource); > >> + } else { > >> + list_for_each_entry_safe(window, n, resources, list) > >> + list_move_tail(&window->list, &host->windows); > >> + } > > > > I think we should assume that the correct resources are passed. You > > could add a wrapper around this function to convert old platforms > > though. > > OK, I will move these code out of pci_create_host_bridge, and add a wrapper > to setup the default resources. > > > > >> +EXPORT_SYMBOL(pci_create_host_bridge); > > > > EXPORT_SYMBOL_GPL() maybe? > > OK, will update it. > > > > >> diff --git a/include/linux/pci.h b/include/linux/pci.h > >> index 8b11b38..daa7f40 100644 > >> --- a/include/linux/pci.h > >> +++ b/include/linux/pci.h > >> @@ -402,7 +402,12 @@ struct pci_host_bridge_window { > >> struct pci_host_bridge { > >> struct device dev; > >> struct pci_bus *bus; /* root bus */ > >> + struct list_head list; > >> struct list_head windows; /* pci_host_bridge_windows */ > >> + int busnum; > > > > The busnum should already be implied through the bus resource. > > Yes, I will consider remove it and introduce a helper function to get the root bus number, thanks! > > Thanks! > Yijing. > > > > > Arnd > > > > . > > > > > -- > Thanks! > Yijing > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/