On Thursday 09 July 2020 15:47:01 Lorenzo Pieralisi wrote: > On Thu, Jul 09, 2020 at 02:22:08PM +0200, Pali Rohár wrote: > > On Thursday 09 July 2020 12:35:09 Lorenzo Pieralisi wrote: > > > On Thu, Jul 02, 2020 at 10:30:36AM +0200, Pali Rohár wrote: > > > > When there is no PCIe card connected and advk_pcie_rd_conf() or > > > > advk_pcie_wr_conf() is called for PCI bus which doesn't belong to emulated > > > > root bridge, the aardvark driver throws the following error message: > > > > > > > > advk-pcie d0070000.pcie: config read/write timed out > > > > > > > > Obviously accessing PCIe registers of disconnected card is not possible. > > > > > > > > Extend check in advk_pcie_valid_device() function for validating > > > > availability of PCIe bus. If PCIe link is down, then the device is marked > > > > as Not Found and the driver does not try to access these registers. > > > > > > > > This is just an optimization to prevent accessing PCIe registers when card > > > > is disconnected. Trying to access PCIe registers of disconnected card does > > > > not cause any crash, kernel just needs to wait for a timeout. So if card > > > > disappear immediately after checking for PCIe link (before accessing PCIe > > > > registers), it does not cause any problems. > > > > > > > > Signed-off-by: Pali Rohár <pali@xxxxxxxxxx> > > > > > > > > --- > > > > Changes in V3: > > > > * Add comment to the code > > > > Changes in V2: > > > > * Update commit message, mention that this is optimization > > > > --- > > > > drivers/pci/controller/pci-aardvark.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > > > > index 90ff291c24f0..d18f389b36a1 100644 > > > > --- a/drivers/pci/controller/pci-aardvark.c > > > > +++ b/drivers/pci/controller/pci-aardvark.c > > > > @@ -644,6 +644,13 @@ static bool advk_pcie_valid_device(struct advk_pcie *pcie, struct pci_bus *bus, > > > > if ((bus->number == pcie->root_bus_nr) && PCI_SLOT(devfn) != 0) > > > > return false; > > > > > > > > + /* > > > > + * If the link goes down after we check for link-up, nothing bad > > > > + * happens but the config access times out. > > > > + */ > > > > + if (bus->number != pcie->root_bus_nr && !advk_pcie_link_up(pcie)) > > > > + return false; > > > > + > > > > return true; > > > > } > > > > > > Question: this basically means that you can only effectively enumerate > > > bus number == root_bus_nr and AFAICS if at probe the link did not > > > come up it will never do, will it ? > > > > > > Isn't this equivalent to limiting the bus numbers the bridge is capable > > > of handling ? > > > > > > Reworded: if in advk_pcie_setup_hw() the link does not come up, what's > > > the point of trying to enumerate the bus hierarchy below the root bus ? > > > > Hello Lorenzo! > > > > PCIe link can theoretically come up even after boot, but aardvark driver > > currently does not support link detection at runtime. So it checks and > > enumerate device only at probe time. > > If the link is not up at probe enumerating devices below the root > bus is basically useless and that's actually what is causing the > delays you are fixing. Is this correct ? Yes, this is one (but not the only one) delay. > > I do not know if hardware has some mechanism to inform kernel that PCIe > > link come up (or down) and re-enumeration is required. Or the only > > option is polling via advk_pcie_link_up(). > > > > So if device is not visible at the probe time then it would not appear > > in system and cannot be used. This is current state. > > > > Just to note that our hardware does not support physical hotplug of > > mPCIe cards. You need to connect card when board is powered off. > > > > So if at the aardvark probe time PCIe link is not up then trying to > > enumerate devices under (software) root bridge is not needed. But it is > > needed to register/enumerate software root bridge device and currently > > both is done by one (recursive) call pci_host_probe(). > > I understand that but the bridge bus resource can be trimmed to just > contain the root bus because that's the only one where there is a > chance you can enumerate a device. It is possible to register only root bridge without endpoint? > I would like to get Bjorn's opinion on this, I don't like these "link is > up" checks in config accessors (they are racy and honestly it is a > run-time check that does not make much sense, either it is always > true/false or it is inevitably racy) It is runtime check, but does not have to be always true/false. I have tested more Compex wifi cards and under certain conditions they "disappear" from the bus during usage. So I think it still make sense to do this "fast" check as it is only optimization. > I was wondering if we can find an > alternative solution but I am not sure the one I suggested above is > better than this patch. I do not know if it helps in situation when card disappear from bus on runtime...
