All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Pali Rohár" <pali@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Andrew Murray" <amurray@thegoodpenguin.co.uk>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Marek Behún" <marek.behun@nic.cz>,
	"Remi Pommarel" <repk@triplefau.lt>,
	"Tomasz Maciej Nowak" <tmn505@gmail.com>,
	Xogium <contact@xogium.me>,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected
Date: Tue, 30 Jun 2020 16:04:20 +0200	[thread overview]
Message-ID: <20200630140420.7qkuq7wturmaafzf@pali> (raw)
In-Reply-To: <20200630135127.GA3414186@bjorn-Precision-5520>

On Tuesday 30 June 2020 08:51:27 Bjorn Helgaas wrote:
> On Thu, May 28, 2020 at 06:38:09PM +0200, Pali Rohár wrote:
> > On Thursday 28 May 2020 11:26:04 Bjorn Helgaas wrote:
> > > On Thu, May 28, 2020 at 04:31:41PM +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.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > ---
> > > >  drivers/pci/controller/pci-aardvark.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > index 90ff291c24f0..53a4cfd7d377 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -644,6 +644,9 @@ 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 (bus->number != pcie->root_bus_nr && !advk_pcie_link_up(pcie))
> > > > +		return false;
> > > 
> > > I don't think this is the right fix.  This makes it racy because the
> > > link may go down after we call advk_pcie_valid_device() but before we
> > > perform the config read.
> > 
> > Yes, it is racy, but I do not think it cause problems. Trying to read
> > PCIe registers when device is not connected cause just those timeouts,
> > printing error message and increased delay in advk_pcie_wait_pio() due
> > to polling loop. This patch reduce unnecessary access to PCIe registers
> > when advk_pcie_wait_pio() polling just fail.
> 
> What happens when the device is removed after advk_pcie_link_up()
> returns true, but before we actually do the config access?

Do you mean to remove device physically at runtime? I was told that our
board would crash or issue reset. Removing device from mini PCIe slot
without power off is not supported.

Anyway, currently we are trying to read from device registers even when
no device is connected. So when advk_pcie_link_up() returns true and
after that device is not connected (somehow board and kernel would be
still alive) I guess that it would behave as without applying this
patch. So kernel starts reading from register and would wait until
timeout expires. As device is not connected there would be no answer,
so kernel print error message to dmesg (same as in commit message) and
returns error that read failed.

WARNING: multiple messages have this Message-ID
From: "Pali Rohár" <pali@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Tomasz Maciej Nowak" <tmn505@gmail.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	linux-pci@vger.kernel.org, Xogium <contact@xogium.me>,
	linux-kernel@vger.kernel.org, "Marek Behún" <marek.behun@nic.cz>,
	"Remi Pommarel" <repk@triplefau.lt>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	linux-arm-kernel@lists.infradead.org,
	"Andrew Murray" <amurray@thegoodpenguin.co.uk>
Subject: Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected
Date: Tue, 30 Jun 2020 16:04:20 +0200	[thread overview]
Message-ID: <20200630140420.7qkuq7wturmaafzf@pali> (raw)
In-Reply-To: <20200630135127.GA3414186@bjorn-Precision-5520>

On Tuesday 30 June 2020 08:51:27 Bjorn Helgaas wrote:
> On Thu, May 28, 2020 at 06:38:09PM +0200, Pali Rohár wrote:
> > On Thursday 28 May 2020 11:26:04 Bjorn Helgaas wrote:
> > > On Thu, May 28, 2020 at 04:31:41PM +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.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > ---
> > > >  drivers/pci/controller/pci-aardvark.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > index 90ff291c24f0..53a4cfd7d377 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -644,6 +644,9 @@ 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 (bus->number != pcie->root_bus_nr && !advk_pcie_link_up(pcie))
> > > > +		return false;
> > > 
> > > I don't think this is the right fix.  This makes it racy because the
> > > link may go down after we call advk_pcie_valid_device() but before we
> > > perform the config read.
> > 
> > Yes, it is racy, but I do not think it cause problems. Trying to read
> > PCIe registers when device is not connected cause just those timeouts,
> > printing error message and increased delay in advk_pcie_wait_pio() due
> > to polling loop. This patch reduce unnecessary access to PCIe registers
> > when advk_pcie_wait_pio() polling just fail.
> 
> What happens when the device is removed after advk_pcie_link_up()
> returns true, but before we actually do the config access?

Do you mean to remove device physically at runtime? I was told that our
board would crash or issue reset. Removing device from mini PCIe slot
without power off is not supported.

Anyway, currently we are trying to read from device registers even when
no device is connected. So when advk_pcie_link_up() returns true and
after that device is not connected (somehow board and kernel would be
still alive) I guess that it would behave as without applying this
patch. So kernel starts reading from register and would wait until
timeout expires. As device is not connected there would be no answer,
so kernel print error message to dmesg (same as in commit message) and
returns error that read failed.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2020-06-30 14:04 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-28 14:31 [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected Pali Rohár
2020-05-28 14:31 ` Pali Rohár
2020-05-28 16:26 ` Bjorn Helgaas
2020-05-28 16:26   ` Bjorn Helgaas
2020-05-28 16:38   ` Pali Rohár
2020-05-28 16:38     ` Pali Rohár
2020-05-28 16:49     ` Bjorn Helgaas
2020-05-28 16:49       ` Bjorn Helgaas
2020-05-29  8:30       ` Pali Rohár
2020-05-29  8:30         ` Pali Rohár
2020-06-30 12:31         ` Pali Rohár
2020-06-30 12:31           ` Pali Rohár
2020-06-30 13:51     ` Bjorn Helgaas
2020-06-30 13:51       ` Bjorn Helgaas
2020-06-30 14:04       ` Pali Rohár [this message]
2020-06-30 14:04         ` Pali Rohár
2020-06-30 14:58         ` Bjorn Helgaas
2020-06-30 14:58           ` Bjorn Helgaas
2020-07-01  8:08           ` Pali Rohár
2020-07-01  8:08             ` Pali Rohár
2020-07-01  8:20 ` [PATCH v2] " Pali Rohár
2020-07-01  8:20   ` Pali Rohár
2020-07-01 21:34   ` Bjorn Helgaas
2020-07-01 21:34     ` Bjorn Helgaas
2020-07-02  8:23     ` Pali Rohár
2020-07-02  8:23       ` Pali Rohár
2020-07-02  8:30 ` [PATCH v3] " Pali Rohár
2020-07-02  8:30   ` Pali Rohár
2020-07-09 11:35   ` Lorenzo Pieralisi
2020-07-09 11:35     ` Lorenzo Pieralisi
2020-07-09 12:22     ` Pali Rohár
2020-07-09 12:22       ` Pali Rohár
2020-07-09 14:47       ` Lorenzo Pieralisi
2020-07-09 14:47         ` Lorenzo Pieralisi
2020-07-09 15:09         ` Pali Rohár
2020-07-09 15:09           ` Pali Rohár
2020-07-10  9:18           ` Lorenzo Pieralisi
2020-07-10  9:18             ` Lorenzo Pieralisi
2020-07-10 15:44             ` Pali Rohár
2020-07-10 15:44               ` Pali Rohár
2020-07-10 16:08               ` Bjorn Helgaas
2020-07-10 16:08                 ` Bjorn Helgaas
2020-07-10 19:30                 ` Pali Rohár
2020-07-10 19:30                   ` Pali Rohár
2020-07-10 20:08                   ` Bjorn Helgaas
2020-07-10 20:08                     ` Bjorn Helgaas
2020-07-13  8:27             ` Pali Rohár
2020-07-13  8:27               ` Pali Rohár
2020-07-13 11:23               ` Lorenzo Pieralisi
2020-07-13 11:23                 ` Lorenzo Pieralisi
2020-07-13 14:50                 ` Pali Rohár
2020-07-13 14:50                   ` Pali Rohár
2020-07-13 16:41                   ` Lorenzo Pieralisi
2020-07-13 16:41                     ` Lorenzo Pieralisi
2020-07-14  7:38                     ` Pali Rohár
2020-07-14  7:38                       ` Pali Rohár
2020-07-15 12:17                 ` Pali Rohár
2020-07-15 12:17                   ` Pali Rohár
2020-07-15 16:21                   ` Lorenzo Pieralisi
2020-07-15 16:21                     ` Lorenzo Pieralisi
2020-07-21  8:57                     ` Pali Rohár
2020-07-21  8:57                       ` Pali Rohár
2020-07-21 10:48                       ` Lorenzo Pieralisi
2020-07-21 10:48                         ` Lorenzo Pieralisi

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=20200630140420.7qkuq7wturmaafzf@pali \
    --to=pali@kernel.org \
    --cc=amurray@thegoodpenguin.co.uk \
    --cc=bhelgaas@google.com \
    --cc=contact@xogium.me \
    --cc=helgaas@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=marek.behun@nic.cz \
    --cc=repk@triplefau.lt \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=tmn505@gmail.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.