On Thursday, July 24, 2014 6:02 PM, Jingoo Han wrote:
> Am Donnerstag, den 24.07.2014, 17:55 +0900 schrieb Jingoo Han:
> > 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@xxxxxxxxxxxxxx>
> > > > > ---
> > > > > 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. :-)
> >
> I just looked this up and we we have a lot (and in some cases
> inconsistent checks) for this sprinkled through the driver. I would
> prefer to keep this patch as is and instead add this check to
> sys_to_pcie(). This way we could remove it from the various call sites
> while keeping the safety it provides. If you agree I can send a patch to
> do this.
Hi Lucas Stach,
OK, I agree with your suggestion. However, I want to get other opinions
from other CC'ed people. Thank you.
Best regards,
Jingoo Han
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
