From: Jingoo Han <jg1.han@samsung.com>
To: 'Lucas Stach' <l.stach@pengutronix.de>
Cc: 'Bjorn Helgaas' <bhelgaas@google.com>,
linux-pci@vger.kernel.org, 'Richard Zhu' <r65037@freescale.com>,
'Mohit Kumar' <mohit.kumar@st.com>, 'Marek Vasut' <marex@denx.de>,
'Kishon Vijay Abraham I' <kishon@ti.com>,
kernel@pengutronix.de, 'Jingoo Han' <jg1.han@samsung.com>
Subject: Re: [PATCH 2/4] PCI: designware: get rid of pci_scan_root_bus
Date: Thu, 24 Jul 2014 17:55:46 +0900 [thread overview]
Message-ID: <002d01cfa71d$0f5a2590$2e0e70b0$%han@samsung.com> (raw)
In-Reply-To: <1406190729.4590.8.camel@weser.hi.pengutronix.de>
On Thursday, July 24, 2014 5:32 PM, Lucas Stach wrote:
> Am Donnerstag, den 24.07.2014, 13:11 +0900 schrieb Jingoo Han:
> > On Thursday, July 24, 2014 2:53 AM, Lucas Stach wrote:
> > >
> > > Use pci_create_root_bus() similar to other pci
> > > host controller drivers.
> > >
> > > The main problem with pci_scan_root_bus() is that
> > > not only it will create the root bus, but also try
> > > to activate all devices on the bus. This triggers
> > > PCI device drivers probe, which try to use the device
> > > but fail as resource allocation isn't done at that
> > > point in time.
> > >
> > > To work around this we made sure that the host
> > > controller driver is probed early and finishes resource
> > > allocation before any other device drivers are registered.
> > > Switching to pci_create_root_bus() allows us to get rid
> > > of this special handling.
> > >
> > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > ---
> > > drivers/pci/host/pcie-designware.c | 15 +++++++--------
> > > 1 file changed, 7 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> > > index b13a830c8b0f..b869e202367c 100644
> > > --- a/drivers/pci/host/pcie-designware.c
> > > +++ b/drivers/pci/host/pcie-designware.c
> > > @@ -801,14 +801,13 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
> > > struct pci_bus *bus;
> > > struct pcie_port *pp = sys_to_pcie(sys);
> > >
> > > - if (pp) {
> > > - pp->root_bus_nr = sys->busnr;
> > > - bus = pci_scan_root_bus(pp->dev, sys->busnr, &dw_pcie_ops,
> > > - sys, &sys->resources);
> > > - } else {
> > > - bus = NULL;
> > > - BUG();
> > > - }
> > > + pp->root_bus_nr = sys->busnr;
> > > + bus = pci_create_root_bus(pp->dev, sys->busnr,
> > > + &dw_pcie_ops, sys, &sys->resources);
> > > + if (!bus)
> > > + return NULL;
> > > +
> > > + pci_scan_child_bus(bus);
> >
> >
> > I think that we still need to check if pp is NULL.
> > Then, how about adding NULL check as below?
> >
> No I don't think it's needed as it doesn't provide any additional
> security.
>
> The only code path that is calling this dw_pci_scan_bus() is in
> pcibios_init_hw() which always fills sys->private_data with
> hw_pci->private_data. As we are always filling hw_pci->private_data with
> a pointer to our pcie_port structure in dw_pcie_host_init() the only way
> for pp to be NULL at this point would be for pp to be NULL in
> dw_pcie_host_init(). This would obviously crash a lot earlier.
>
> So this check is just adding to LOC without adding any value.
Hi Lucas,
I see what you mean. However, pcie-designware.c has been handled by
various engineers. So, it is not sure that the current sequence will
not change in the future. Anyway, thank you for your comment. :-)
Best regards,
Jingoo Han
>
> Regards,
> Lucas
>
> --
> Pengutronix e.K. | Lucas Stach |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
next prev parent reply other threads:[~2014-07-24 8:55 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-23 17:52 [PATCH 0/4] PCI: designware: init order/resource alloc fixes Lucas Stach
2014-07-23 17:52 ` [PATCH 1/4] PCI: designware: start parsing bus-range Lucas Stach
2014-08-16 12:26 ` Pratyush Anand
2014-09-03 18:37 ` Bjorn Helgaas
2014-07-23 17:52 ` [PATCH 2/4] PCI: designware: get rid of pci_scan_root_bus Lucas Stach
2014-07-24 4:11 ` Jingoo Han
2014-07-24 8:32 ` Lucas Stach
2014-07-24 8:55 ` Jingoo Han [this message]
2014-07-24 9:02 ` Lucas Stach
2014-07-25 0:39 ` Jingoo Han
2014-08-16 12:27 ` Pratyush Anand
2014-07-23 17:52 ` [PATCH 3/4] PCI: designware: remove pci_assign_unassigned_resources call Lucas Stach
2014-08-16 13:13 ` Pratyush Anand
2014-08-17 2:59 ` Yinghai Lu
2014-08-17 6:20 ` Pratyush Anand
2014-08-18 15:39 ` Murali Karicheri
2014-07-23 17:52 ` [PATCH 4/4] PCI: imx6: move to module_init Lucas Stach
2014-07-24 6:32 ` Hong-Xing.Zhu
2014-07-23 18:08 ` [PATCH 0/4] PCI: designware: init order/resource alloc fixes Marek Vasut
2014-09-02 23:10 ` Bjorn Helgaas
2014-09-03 4:19 ` Mohit KUMAR DCG
2014-09-03 18:33 ` Bjorn Helgaas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='002d01cfa71d$0f5a2590$2e0e70b0$%han@samsung.com' \
--to=jg1.han@samsung.com \
--cc=bhelgaas@google.com \
--cc=kernel@pengutronix.de \
--cc=kishon@ti.com \
--cc=l.stach@pengutronix.de \
--cc=linux-pci@vger.kernel.org \
--cc=marex@denx.de \
--cc=mohit.kumar@st.com \
--cc=r65037@freescale.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).