From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753568AbaIYSPF (ORCPT ); Thu, 25 Sep 2014 14:15:05 -0400 Received: from mail-ig0-f181.google.com ([209.85.213.181]:42184 "EHLO mail-ig0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752130AbaIYSPC (ORCPT ); Thu, 25 Sep 2014 14:15:02 -0400 MIME-Version: 1.0 In-Reply-To: <1411498874-9864-7-git-send-email-Liviu.Dudau@arm.com> References: <1411498874-9864-1-git-send-email-Liviu.Dudau@arm.com> <1411498874-9864-7-git-send-email-Liviu.Dudau@arm.com> Date: Thu, 25 Sep 2014 11:15:01 -0700 X-Google-Sender-Auth: KbIRxvxWXoV6ziGSBvz-2xAeejQ Message-ID: Subject: Re: [PATCH v12 06/12] PCI: Create pci_host_bridge before its associated bus in pci_create_root_bus. From: Yinghai Lu To: Liviu Dudau Cc: Bjorn Helgaas , Arnd Bergmann , Rob Herring , Jason Gunthorpe , Benjamin Herrenschmidt , Catalin Marinas , Will Deacon , Russell King , linux-pci , Linus Walleij , Tanmay Inamdar , Grant Likely , Sinan Kaya , Jingoo Han , Kukjin Kim , Suravee Suthikulanit , linux-arch , LKML , Device Tree ML , LAKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 23, 2014 at 12:01 PM, Liviu Dudau wrote: > Before commit 7b5436635800 the pci_host_bridge was created before the root bus. > As that commit has added a needless dependency on the bus for pci_alloc_host_bridge() > the creation order has been changed for no good reason. Revert the order of > creation as we are going to depend on the pci_host_bridge structure to retrieve the > domain number of the root bus. > > Cc: Bjorn Helgaas > Signed-off-by: Liviu Dudau > Acked-by: Grant Likely > Reviewed-by: Catalin Marinas > Tested-by: Tanmay Inamdar > --- > drivers/pci/probe.c | 32 +++++++++++++++++--------------- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index e3cf8a2..5ff72ec 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -515,7 +515,7 @@ static void pci_release_host_bridge_dev(struct device *dev) > kfree(bridge); > } > > -static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b) > +static struct pci_host_bridge *pci_alloc_host_bridge(void) > { > struct pci_host_bridge *bridge; > > @@ -524,7 +524,8 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b) > return NULL; > > INIT_LIST_HEAD(&bridge->windows); > - bridge->bus = b; > + bridge->dev.release = pci_release_host_bridge_dev; > + > return bridge; > } > > @@ -1761,9 +1762,15 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > char bus_addr[64]; > char *fmt; > > + bridge = pci_alloc_host_bridge(); > + if (!bridge) > + return NULL; > + > + bridge->dev.parent = parent; > + > b = pci_alloc_bus(); > if (!b) > - return NULL; > + goto err_out; > > b->sysdata = sysdata; > b->ops = ops; > @@ -1772,26 +1779,19 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > if (b2) { > /* If we already got to this bus through a different bridge, ignore it */ > dev_dbg(&b2->dev, "bus already known\n"); > - goto err_out; > + goto err_bus_out; > } > > - bridge = pci_alloc_host_bridge(b); > - if (!bridge) > - goto err_out; > - > - bridge->dev.parent = parent; > - bridge->dev.release = pci_release_host_bridge_dev; > + bridge->bus = b; > dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); > error = pcibios_root_bridge_prepare(bridge); > - if (error) { > - kfree(bridge); > + if (error) > goto err_out; > - } > > error = device_register(&bridge->dev); > if (error) { > put_device(&bridge->dev); > - goto err_out; > + goto err_bus_out; > } > b->bridge = get_device(&bridge->dev); > device_enable_async_suspend(b->bridge); > @@ -1848,8 +1848,10 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > class_dev_reg_err: > put_device(&bridge->dev); > device_unregister(&bridge->dev); > -err_out: > +err_bus_out: > kfree(b); > +err_out: > + kfree(bridge); > return NULL; > } Hi Bjorn, Noticed that you put this one into pci/next. This patch has problem for the class_dev_reg_err path. as device_unregister(&bridge->dev) will trigger pci_release_host_bridge_dev. So will have double free for bridge or else. Please drop this patch from pci/next. Also looks strange that the patch have long CC list, but just omit me and Jiang Liu that touched those code before. Thanks Yinghai From mboxrd@z Thu Jan 1 00:00:00 1970 From: yinghai@kernel.org (Yinghai Lu) Date: Thu, 25 Sep 2014 11:15:01 -0700 Subject: [PATCH v12 06/12] PCI: Create pci_host_bridge before its associated bus in pci_create_root_bus. In-Reply-To: <1411498874-9864-7-git-send-email-Liviu.Dudau@arm.com> References: <1411498874-9864-1-git-send-email-Liviu.Dudau@arm.com> <1411498874-9864-7-git-send-email-Liviu.Dudau@arm.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Tue, Sep 23, 2014 at 12:01 PM, Liviu Dudau wrote: > Before commit 7b5436635800 the pci_host_bridge was created before the root bus. > As that commit has added a needless dependency on the bus for pci_alloc_host_bridge() > the creation order has been changed for no good reason. Revert the order of > creation as we are going to depend on the pci_host_bridge structure to retrieve the > domain number of the root bus. > > Cc: Bjorn Helgaas > Signed-off-by: Liviu Dudau > Acked-by: Grant Likely > Reviewed-by: Catalin Marinas > Tested-by: Tanmay Inamdar > --- > drivers/pci/probe.c | 32 +++++++++++++++++--------------- > 1 file changed, 17 insertions(+), 15 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index e3cf8a2..5ff72ec 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -515,7 +515,7 @@ static void pci_release_host_bridge_dev(struct device *dev) > kfree(bridge); > } > > -static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b) > +static struct pci_host_bridge *pci_alloc_host_bridge(void) > { > struct pci_host_bridge *bridge; > > @@ -524,7 +524,8 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b) > return NULL; > > INIT_LIST_HEAD(&bridge->windows); > - bridge->bus = b; > + bridge->dev.release = pci_release_host_bridge_dev; > + > return bridge; > } > > @@ -1761,9 +1762,15 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > char bus_addr[64]; > char *fmt; > > + bridge = pci_alloc_host_bridge(); > + if (!bridge) > + return NULL; > + > + bridge->dev.parent = parent; > + > b = pci_alloc_bus(); > if (!b) > - return NULL; > + goto err_out; > > b->sysdata = sysdata; > b->ops = ops; > @@ -1772,26 +1779,19 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > if (b2) { > /* If we already got to this bus through a different bridge, ignore it */ > dev_dbg(&b2->dev, "bus already known\n"); > - goto err_out; > + goto err_bus_out; > } > > - bridge = pci_alloc_host_bridge(b); > - if (!bridge) > - goto err_out; > - > - bridge->dev.parent = parent; > - bridge->dev.release = pci_release_host_bridge_dev; > + bridge->bus = b; > dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); > error = pcibios_root_bridge_prepare(bridge); > - if (error) { > - kfree(bridge); > + if (error) > goto err_out; > - } > > error = device_register(&bridge->dev); > if (error) { > put_device(&bridge->dev); > - goto err_out; > + goto err_bus_out; > } > b->bridge = get_device(&bridge->dev); > device_enable_async_suspend(b->bridge); > @@ -1848,8 +1848,10 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > class_dev_reg_err: > put_device(&bridge->dev); > device_unregister(&bridge->dev); > -err_out: > +err_bus_out: > kfree(b); > +err_out: > + kfree(bridge); > return NULL; > } Hi Bjorn, Noticed that you put this one into pci/next. This patch has problem for the class_dev_reg_err path. as device_unregister(&bridge->dev) will trigger pci_release_host_bridge_dev. So will have double free for bridge or else. Please drop this patch from pci/next. Also looks strange that the patch have long CC list, but just omit me and Jiang Liu that touched those code before. Thanks Yinghai