Message-ID: <20200709150959.wq6zfkcy4m6hvvpl@pali>
Date:   Thu, 9 Jul 2020 17:09:59 +0200
From:   Pali Rohár <pali@...nel.org>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc:     Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Andrew Murray <amurray@...goodpenguin.co.uk>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Marek Behún <marek.behun@....cz>,
        Remi Pommarel <repk@...plefau.lt>,
        Tomasz Maciej Nowak <tmn505@...il.com>,
        Xogium <contact@...ium.me>, linux-pci@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card
 connected

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@...nel.org>
> > > > 
> > > > ---
> > > > 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...

Powered by blists - more mailing lists