Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected
@ 2020-05-28 14:31 Pali Rohár
  2020-05-28 16:26 ` Bjorn Helgaas
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Pali Rohár @ 2020-05-28 14:31 UTC (permalink / raw)
  To: Thomas Petazzoni, Lorenzo Pieralisi, Andrew Murray,
	Bjorn Helgaas, Marek Behún, Remi Pommarel,
	Tomasz Maciej Nowak, Xogium
  Cc: linux-pci, linux-kernel, linux-arm-kernel

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;
+
 	return true;
 }
 
-- 
2.20.1


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-05-28 14:31 [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected Pali Rohár
@ 2020-05-28 16:26 ` Bjorn Helgaas
  2020-05-28 16:38   ` Pali Rohár
  2020-07-01  8:20 ` [PATCH v2] " Pali Rohár
  2020-07-02  8:30 ` [PATCH v3] " Pali Rohár
  2 siblings, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2020-05-28 16:26 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Tomasz Maciej Nowak, Lorenzo Pieralisi, linux-pci, Xogium,
	linux-kernel, Marek Behún, Remi Pommarel, Thomas Petazzoni,
	Bjorn Helgaas, linux-arm-kernel, Andrew Murray

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.

I have no objection to removing the "config read/write timed out"
message.  The "return PCIBIOS_SET_FAILED" in the read case probably
should be augmented by setting "*val = 0xffffffff".

>  	return true;
>  }
>  
> -- 
> 2.20.1
> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-05-28 16:26 ` Bjorn Helgaas
@ 2020-05-28 16:38   ` Pali Rohár
  2020-05-28 16:49     ` Bjorn Helgaas
  2020-06-30 13:51     ` Bjorn Helgaas
  0 siblings, 2 replies; 16+ messages in thread
From: Pali Rohár @ 2020-05-28 16:38 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Tomasz Maciej Nowak, Lorenzo Pieralisi, linux-pci, Xogium,
	linux-kernel, Marek Behún, Remi Pommarel, Thomas Petazzoni,
	Bjorn Helgaas, linux-arm-kernel, Andrew Murray

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.

I think it is a good idea to not call blocking advk_pcie_wait_pio() when
it is not needed. We could have faster enumeration of PCIe buses when
card is not connected.

> I have no objection to removing the "config read/write timed out"
> message.  The "return PCIBIOS_SET_FAILED" in the read case probably
> should be augmented by setting "*val = 0xffffffff".
> 
> >  	return true;
> >  }
> >  
> > -- 
> > 2.20.1
> > 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-05-28 16:38   ` Pali Rohár
@ 2020-05-28 16:49     ` Bjorn Helgaas
  2020-05-29  8:30       ` Pali Rohár
  2020-06-30 13:51     ` Bjorn Helgaas
  1 sibling, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2020-05-28 16:49 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Tomasz Maciej Nowak, Lorenzo Pieralisi, linux-pci, Xogium,
	linux-kernel, Marek Behún, Remi Pommarel, Thomas Petazzoni,
	Bjorn Helgaas, linux-arm-kernel, Andrew Murray

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.
> 
> I think it is a good idea to not call blocking advk_pcie_wait_pio() when
> it is not needed. We could have faster enumeration of PCIe buses when
> card is not connected.

Maybe advk_pcie_check_pio_status() and advk_pcie_wait_pio() could be
combined so we could get the correct error status as soon as it's
available, without waiting for a timeout?

In any event, the "return PCIBIOS_SET_FAILED" needs to be fixed.  Most
callers of config read do not check for failure, but most of the ones
that do, check for "val == ~0".  Only a few check for a status of
other than PCIBIOS_SUCCESSFUL.

> > I have no objection to removing the "config read/write timed out"
> > message.  The "return PCIBIOS_SET_FAILED" in the read case probably
> > should be augmented by setting "*val = 0xffffffff".
> > 
> > >  	return true;
> > >  }
> > >  
> > > -- 
> > > 2.20.1
> > > 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-05-28 16:49     ` Bjorn Helgaas
@ 2020-05-29  8:30       ` Pali Rohár
  2020-06-30 12:31         ` Pali Rohár
  0 siblings, 1 reply; 16+ messages in thread
From: Pali Rohár @ 2020-05-29  8:30 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Tomasz Maciej Nowak, Lorenzo Pieralisi, linux-pci, Xogium,
	linux-kernel, Marek Behún, Remi Pommarel, Thomas Petazzoni,
	Bjorn Helgaas, linux-arm-kernel, Andrew Murray

On Thursday 28 May 2020 11:49:38 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.
> > 
> > I think it is a good idea to not call blocking advk_pcie_wait_pio() when
> > it is not needed. We could have faster enumeration of PCIe buses when
> > card is not connected.
> 
> Maybe advk_pcie_check_pio_status() and advk_pcie_wait_pio() could be
> combined so we could get the correct error status as soon as it's
> available, without waiting for a timeout?

Any idea how to achieve it?

First call is polling function advk_pcie_wait_pio() and second call is
advk_pcie_check_pio_status() which just reads status register and prints
error message to dmesg.

So for me it looks like that combining these two functions into one does
not change anything. We always need to call polling code prior to
checking status register. And therefore need to wait for timeout. Unless
something like in this proposed patch is not used (to skip whole
register access if it would fail).

> In any event, the "return PCIBIOS_SET_FAILED" needs to be fixed.  Most
> callers of config read do not check for failure, but most of the ones
> that do, check for "val == ~0".  Only a few check for a status of
> other than PCIBIOS_SUCCESSFUL.
> 
> > > I have no objection to removing the "config read/write timed out"
> > > message.  The "return PCIBIOS_SET_FAILED" in the read case probably
> > > should be augmented by setting "*val = 0xffffffff".

Now I see, "*val = 0xffffffff" should be really set when function
advk_pcie_rd_conf() fails.

> > > >  	return true;
> > > >  }
> > > >  
> > > > -- 
> > > > 2.20.1
> > > > 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-05-29  8:30       ` Pali Rohár
@ 2020-06-30 12:31         ` Pali Rohár
  0 siblings, 0 replies; 16+ messages in thread
From: Pali Rohár @ 2020-06-30 12:31 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Tomasz Maciej Nowak, Lorenzo Pieralisi, linux-pci, Xogium,
	linux-kernel, Marek Behún, Remi Pommarel, Thomas Petazzoni,
	Bjorn Helgaas, linux-arm-kernel, Andrew Murray

Hello!

On Friday 29 May 2020 10:30:13 Pali Rohár wrote:
> On Thursday 28 May 2020 11:49:38 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.
> > > 
> > > I think it is a good idea to not call blocking advk_pcie_wait_pio() when
> > > it is not needed. We could have faster enumeration of PCIe buses when
> > > card is not connected.
> > 
> > Maybe advk_pcie_check_pio_status() and advk_pcie_wait_pio() could be
> > combined so we could get the correct error status as soon as it's
> > available, without waiting for a timeout?
> 
> Any idea how to achieve it?
> 
> First call is polling function advk_pcie_wait_pio() and second call is
> advk_pcie_check_pio_status() which just reads status register and prints
> error message to dmesg.
> 
> So for me it looks like that combining these two functions into one does
> not change anything. We always need to call polling code prior to
> checking status register. And therefore need to wait for timeout. Unless
> something like in this proposed patch is not used (to skip whole
> register access if it would fail).

So to answer your question, correct status is possible to retrieve only
after waiting for timeout. As status would be available only after
timeout expires.

Therefore my proposed patch in this (or some other) form is needed if we
want to prevent trying to read from registers and waiting for answer
when card is disconnected.

I would really like to see this issue fixed, so booting linux kernel on
board without connected PCIe card would not be delayed.

Thomas, Lorenzo, Bjorn: do you have any idea how to fix it differently?
Or if not, could be my proposed patch accepted in some form?

> > In any event, the "return PCIBIOS_SET_FAILED" needs to be fixed.  Most
> > callers of config read do not check for failure, but most of the ones
> > that do, check for "val == ~0".  Only a few check for a status of
> > other than PCIBIOS_SUCCESSFUL.
> > 
> > > > I have no objection to removing the "config read/write timed out"
> > > > message.  The "return PCIBIOS_SET_FAILED" in the read case probably
> > > > should be augmented by setting "*val = 0xffffffff".
> 
> Now I see, "*val = 0xffffffff" should be really set when function
> advk_pcie_rd_conf() fails.

I have already sent separate patch which fixes this issue.

> > > > >  	return true;
> > > > >  }
> > > > >  
> > > > > -- 
> > > > > 2.20.1
> > > > > 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-05-28 16:38   ` Pali Rohár
  2020-05-28 16:49     ` Bjorn Helgaas
@ 2020-06-30 13:51     ` Bjorn Helgaas
  2020-06-30 14:04       ` Pali Rohár
  1 sibling, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2020-06-30 13:51 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Tomasz Maciej Nowak, Lorenzo Pieralisi, linux-pci, Xogium,
	linux-kernel, Marek Behún, Remi Pommarel, Thomas Petazzoni,
	Bjorn Helgaas, linux-arm-kernel, Andrew Murray

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?

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-06-30 13:51     ` Bjorn Helgaas
@ 2020-06-30 14:04       ` Pali Rohár
  2020-06-30 14:58         ` Bjorn Helgaas
  0 siblings, 1 reply; 16+ messages in thread
From: Pali Rohár @ 2020-06-30 14:04 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Tomasz Maciej Nowak, Lorenzo Pieralisi, linux-pci, Xogium,
	linux-kernel, Marek Behún, Remi Pommarel, Thomas Petazzoni,
	Bjorn Helgaas, linux-arm-kernel, Andrew Murray

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-06-30 14:04       ` Pali Rohár
@ 2020-06-30 14:58         ` Bjorn Helgaas
  2020-07-01  8:08           ` Pali Rohár
  0 siblings, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2020-06-30 14:58 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Tomasz Maciej Nowak, Lorenzo Pieralisi, linux-pci, Xogium,
	linux-kernel, Marek Behún, Remi Pommarel, Thomas Petazzoni,
	Bjorn Helgaas, linux-arm-kernel, Andrew Murray

On Tue, Jun 30, 2020 at 04:04:20PM +0200, Pali Rohár wrote:
> 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.

Right, I don't think PCIe mini cards support hotplug.

> 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.

OK, so if I understand correctly, checking advk_pcie_link_up() is
strictly an optimization.  If we guess wrong (e.g., after calling
advk_pcie_link_up(), the link went down because the card was removed,
DPC triggered, etc), the only bad thing is that we wait for a timeout;
it never causes a crash.

If that's the case, I'm fine with this.  But please add a comment to
that effect.

I think several other drivers check for the link being up because we
actually crash if we try to read config space when the link is down.
That's what I was trying to avoid here.

Bjorn

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-06-30 14:58         ` Bjorn Helgaas
@ 2020-07-01  8:08           ` Pali Rohár
  0 siblings, 0 replies; 16+ messages in thread
From: Pali Rohár @ 2020-07-01  8:08 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Tomasz Maciej Nowak, Lorenzo Pieralisi, linux-pci, Xogium,
	linux-kernel, Marek Behún, Remi Pommarel, Thomas Petazzoni,
	Bjorn Helgaas, linux-arm-kernel, Andrew Murray

On Tuesday 30 June 2020 09:58:48 Bjorn Helgaas wrote:
> On Tue, Jun 30, 2020 at 04:04:20PM +0200, Pali Rohár wrote:
> > 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.
> 
> Right, I don't think PCIe mini cards support hotplug.
> 
> > 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.
> 
> OK, so if I understand correctly, checking advk_pcie_link_up() is
> strictly an optimization.  If we guess wrong (e.g., after calling
> advk_pcie_link_up(), the link went down because the card was removed,
> DPC triggered, etc), the only bad thing is that we wait for a timeout;
> it never causes a crash.

Yes.

> If that's the case, I'm fine with this.  But please add a comment to
> that effect.

Ok, I will send V2 with updated commit message.

> I think several other drivers check for the link being up because we
> actually crash if we try to read config space when the link is down.
> That's what I was trying to avoid here.
> 
> Bjorn

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-05-28 14:31 [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected Pali Rohár
  2020-05-28 16:26 ` Bjorn Helgaas
@ 2020-07-01  8:20 ` Pali Rohár
  2020-07-01 21:34   ` Bjorn Helgaas
  2020-07-02  8:30 ` [PATCH v3] " Pali Rohár
  2 siblings, 1 reply; 16+ messages in thread
From: Pali Rohár @ 2020-07-01  8:20 UTC (permalink / raw)
  To: Thomas Petazzoni, Lorenzo Pieralisi, Andrew Murray,
	Bjorn Helgaas, Marek Behún, Remi Pommarel,
	Tomasz Maciej Nowak, Xogium
  Cc: linux-pci, linux-kernel, linux-arm-kernel

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

---
Changes in V2:
* Update commit message, mention that this is optimization
---
 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;
+
 	return true;
 }
 
-- 
2.20.1


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-07-01  8:20 ` [PATCH v2] " Pali Rohár
@ 2020-07-01 21:34   ` Bjorn Helgaas
  2020-07-02  8:23     ` Pali Rohár
  0 siblings, 1 reply; 16+ messages in thread
From: Bjorn Helgaas @ 2020-07-01 21:34 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Tomasz Maciej Nowak, Lorenzo Pieralisi, linux-pci, Xogium,
	linux-kernel, Marek Behún, Remi Pommarel, Thomas Petazzoni,
	Bjorn Helgaas, linux-arm-kernel, Andrew Murray

On Wed, Jul 01, 2020 at 10:20:44AM +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.

Thanks, this is good.  I'd really like a short comment in the code as
well, because this sort of link-up check tends to get copied to new
drivers where it shouldn't be used, e.g., something like this:

  /*
   * If the link goes down after we check for link-up, nothing bad
   * happens but the config access times out.
   */

> Signed-off-by: Pali Rohár <pali@kernel.org>
> 
> ---
> Changes in V2:
> * Update commit message, mention that this is optimization
> ---
>  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;
> +
>  	return true;
>  }
>  
> -- 
> 2.20.1
> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-07-01 21:34   ` Bjorn Helgaas
@ 2020-07-02  8:23     ` Pali Rohár
  0 siblings, 0 replies; 16+ messages in thread
From: Pali Rohár @ 2020-07-02  8:23 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Tomasz Maciej Nowak, Lorenzo Pieralisi, linux-pci, Xogium,
	linux-kernel, Marek Behún, Remi Pommarel, Thomas Petazzoni,
	Bjorn Helgaas, linux-arm-kernel, Andrew Murray

On Wednesday 01 July 2020 16:34:42 Bjorn Helgaas wrote:
> On Wed, Jul 01, 2020 at 10:20:44AM +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.
> 
> Thanks, this is good.  I'd really like a short comment in the code as
> well, because this sort of link-up check tends to get copied to new
> drivers where it shouldn't be used, e.g., something like this:
> 
>   /*
>    * If the link goes down after we check for link-up, nothing bad
>    * happens but the config access times out.
>    */

Ok, it makes sense! I will send a new patch version.

> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > 
> > ---
> > Changes in V2:
> > * Update commit message, mention that this is optimization
> > ---
> >  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;
> > +
> >  	return true;
> >  }
> >  
> > -- 
> > 2.20.1
> > 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-05-28 14:31 [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected Pali Rohár
  2020-05-28 16:26 ` Bjorn Helgaas
  2020-07-01  8:20 ` [PATCH v2] " Pali Rohár
@ 2020-07-02  8:30 ` Pali Rohár
  2020-07-09 11:35   ` Lorenzo Pieralisi
  2 siblings, 1 reply; 16+ messages in thread
From: Pali Rohár @ 2020-07-02  8:30 UTC (permalink / raw)
  To: Thomas Petazzoni, Lorenzo Pieralisi, Andrew Murray,
	Bjorn Helgaas, Marek Behún, Remi Pommarel,
	Tomasz Maciej Nowak, Xogium
  Cc: linux-pci, linux-kernel, linux-arm-kernel

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@kernel.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;
 }
 
-- 
2.20.1


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-07-02  8:30 ` [PATCH v3] " Pali Rohár
@ 2020-07-09 11:35   ` Lorenzo Pieralisi
  2020-07-09 12:22     ` Pali Rohár
  0 siblings, 1 reply; 16+ messages in thread
From: Lorenzo Pieralisi @ 2020-07-09 11:35 UTC (permalink / raw)
  To: Pali Rohár
  Cc: Tomasz Maciej Nowak, linux-pci, Xogium, linux-kernel,
	Marek Behún, Remi Pommarel, Thomas Petazzoni, Bjorn Helgaas,
	linux-arm-kernel, Andrew Murray

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@kernel.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 ?

Thanks,
Lorenzo

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v3] PCI: aardvark: Don't touch PCIe registers if no card connected
  2020-07-09 11:35   ` Lorenzo Pieralisi
@ 2020-07-09 12:22     ` Pali Rohár
  0 siblings, 0 replies; 16+ messages in thread
From: Pali Rohár @ 2020-07-09 12:22 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Tomasz Maciej Nowak, linux-pci, Xogium, linux-kernel,
	Marek Behún, Remi Pommarel, Thomas Petazzoni, Bjorn Helgaas,
	linux-arm-kernel, Andrew Murray

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@kernel.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.

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().

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-28 14:31 [PATCH] PCI: aardvark: Don't touch PCIe registers if no card connected Pali Rohár
2020-05-28 16:26 ` Bjorn Helgaas
2020-05-28 16:38   ` Pali Rohár
2020-05-28 16:49     ` Bjorn Helgaas
2020-05-29  8:30       ` Pali Rohár
2020-06-30 12:31         ` Pali Rohár
2020-06-30 13:51     ` Bjorn Helgaas
2020-06-30 14:04       ` Pali Rohár
2020-06-30 14:58         ` Bjorn Helgaas
2020-07-01  8:08           ` Pali Rohár
2020-07-01  8:20 ` [PATCH v2] " Pali Rohár
2020-07-01 21:34   ` Bjorn Helgaas
2020-07-02  8:23     ` Pali Rohár
2020-07-02  8:30 ` [PATCH v3] " Pali Rohár
2020-07-09 11:35   ` Lorenzo Pieralisi
2020-07-09 12:22     ` Pali Rohár

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git