diff mbox

[v2,1/7] PCI: aardvark: fix logic in PCI configuration read/write functions

Message ID 20170928125838.11887-2-thomas.petazzoni@free-electrons.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Thomas Petazzoni Sept. 28, 2017, 12:58 p.m. UTC 
From: Victor Gu <xigu@marvell.com>

The PCI configuration space read/write functions were special casing
the situation where PCI_SLOT(devfn) != 0, and returned
PCIBIOS_DEVICE_NOT_FOUND in this case.

However, will this is what is intended for the root bus, it is not
intended for the child busses, as it prevents discovering devices with
PCI_SLOT(x) != 0. Therefore, we return PCIBIOS_DEVICE_NOT_FOUND only
if we're on the root bus.

Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Victor Gu <xigu@marvell.com>
Reviewed-by: Wilson Ding <dingwei@marvell.com>
Reviewed-by: Nadav Haklai <nadavh@marvell.com>
[Thomas: tweak commit log.]
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/pci/host/pci-aardvark.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Bjorn Helgaas Oct. 5, 2017, 5:23 p.m. UTC | #1 
On Thu, Sep 28, 2017 at 02:58:32PM +0200, Thomas Petazzoni wrote:
> From: Victor Gu <xigu@marvell.com>
> 
> The PCI configuration space read/write functions were special casing
> the situation where PCI_SLOT(devfn) != 0, and returned
> PCIBIOS_DEVICE_NOT_FOUND in this case.
> 
> However, will this is what is intended for the root bus, it is not
> intended for the child busses, as it prevents discovering devices with
> PCI_SLOT(x) != 0. Therefore, we return PCIBIOS_DEVICE_NOT_FOUND only
> if we're on the root bus.
> 
> Fixes: 8c39d710363c1 ("PCI: aardvark: Add Aardvark PCI host controller driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Victor Gu <xigu@marvell.com>
> Reviewed-by: Wilson Ding <dingwei@marvell.com>
> Reviewed-by: Nadav Haklai <nadavh@marvell.com>
> [Thomas: tweak commit log.]
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/pci/host/pci-aardvark.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
> index 89f4e3d072d7..da2881ba7737 100644
> --- a/drivers/pci/host/pci-aardvark.c
> +++ b/drivers/pci/host/pci-aardvark.c
> @@ -440,7 +440,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  	u32 reg;
>  	int ret;
>  
> -	if (PCI_SLOT(devfn) != 0) {
> +	if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0)) {

I'm fine with this, but please take a look at these:

  8e7ca8ca5fd8 PCI: xilinx: Relax device number checking to allow SR-IOV
  e18934b5e9c7 PCI: designware: Relax device number checking to allow SR-IOV
  d99e30b7936a PCI: altera: Relax device number checking to allow SR-IOV

and make sure that reasoning doesn't apply here, too.

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8e7ca8ca5fd8
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e18934b5e9c7
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d99e30b7936a

>  		*val = 0xffffffff;
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  	}
> @@ -494,7 +494,7 @@ static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>  	int offset;
>  	int ret;
>  
> -	if (PCI_SLOT(devfn) != 0)
> +	if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0))
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  
>  	if (where % size)
> -- 
> 2.13.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Mason Oct. 9, 2017, 7:59 a.m. UTC | #2 
[ NB: CC list trimmed ]

On 28/09/2017 14:58, Thomas Petazzoni wrote:

> From: Victor Gu <xigu@marvell.com>
> 
> The PCI configuration space read/write functions were special casing
> the situation where PCI_SLOT(devfn) != 0, and returned
> PCIBIOS_DEVICE_NOT_FOUND in this case.
> 
> However, will this is what is intended for the root bus, it is not

*while* this is what is intended?
Thomas Petazzoni Jan. 9, 2018, 4:49 p.m. UTC | #3 
Hello Bjorn,

Again, reviving this very old thread :-)

On Thu, 5 Oct 2017 12:23:30 -0500, Bjorn Helgaas wrote:

> > -	if (PCI_SLOT(devfn) != 0) {
> > +	if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0)) {  
> 
> I'm fine with this, but please take a look at these:
> 
>   8e7ca8ca5fd8 PCI: xilinx: Relax device number checking to allow SR-IOV
>   e18934b5e9c7 PCI: designware: Relax device number checking to allow SR-IOV
>   d99e30b7936a PCI: altera: Relax device number checking to allow SR-IOV
> 
> and make sure that reasoning doesn't apply here, too.
> 
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8e7ca8ca5fd8
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e18934b5e9c7
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d99e30b7936a

The original code for xilinx/designware/altera was doing:

 	if (bus->number == port->root_busno && devfn > 0)
 		return false;
 
	if (bus->primary == port->root_busno && devfn > 0)
		return false;

I.e, it was checking both if bus->number *and* bus->primary were equal
to port->root_busno.

The commit you points removed the check on bus->primary, keeping the
check on bus->number.

Your patch for the Aadvark driver only adds a check on bus->number, i.e
exactly what the xilinx/designware/altera code is still doing today:

Altera:

        /* access only one slot on each root port */
        if (bus->number == pcie->root_bus_nr && dev > 0)
                return false;

Designware:

        /* access only one slot on each root port */
        if (bus->number == pp->root_bus_nr && dev > 0)
                return 0;

Xilinx:

        /* Only one device down on each root port */
        if (bus->number == port->root_busno && devfn > 0)
                return false;

Aardvark (with our patch):

        if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0)) {
                *val = 0xffffffff;
                return PCIBIOS_DEVICE_NOT_FOUND;
        }

So we're doing exactly the same thing.

Do you agree ?

Best regards,

Thomas Petazzoni
Bjorn Helgaas Jan. 10, 2018, 1:11 a.m. UTC | #4 
[+cc Lorenzo, since he takes care of this now]

On Tue, Jan 09, 2018 at 05:49:18PM +0100, Thomas Petazzoni wrote:
> Hello Bjorn,
> 
> Again, reviving this very old thread :-)
> 
> On Thu, 5 Oct 2017 12:23:30 -0500, Bjorn Helgaas wrote:
> 
> > > -	if (PCI_SLOT(devfn) != 0) {
> > > +	if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0)) {  
> > 
> > I'm fine with this, but please take a look at these:
> > 
> >   8e7ca8ca5fd8 PCI: xilinx: Relax device number checking to allow SR-IOV
> >   e18934b5e9c7 PCI: designware: Relax device number checking to allow SR-IOV
> >   d99e30b7936a PCI: altera: Relax device number checking to allow SR-IOV
> > 
> > and make sure that reasoning doesn't apply here, too.
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=8e7ca8ca5fd8
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=e18934b5e9c7
> > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d99e30b7936a
> 
> The original code for xilinx/designware/altera was doing:
> 
>  	if (bus->number == port->root_busno && devfn > 0)
>  		return false;
>  
> 	if (bus->primary == port->root_busno && devfn > 0)
> 		return false;
> 
> I.e, it was checking both if bus->number *and* bus->primary were equal
> to port->root_busno.
> 
> The commit you points removed the check on bus->primary, keeping the
> check on bus->number.
> 
> Your patch for the Aadvark driver only adds a check on bus->number, i.e
> exactly what the xilinx/designware/altera code is still doing today:

This is a long time ago and I could have forgotten, but I don't think
this is *my* patch, is it?  

> Altera:
> 
>         /* access only one slot on each root port */
>         if (bus->number == pcie->root_bus_nr && dev > 0)
>                 return false;
> 
> Designware:
> 
>         /* access only one slot on each root port */
>         if (bus->number == pp->root_bus_nr && dev > 0)
>                 return 0;
> 
> Xilinx:
> 
>         /* Only one device down on each root port */
>         if (bus->number == port->root_busno && devfn > 0)
>                 return false;
> 
> Aardvark (with our patch):
> 
>         if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0)) {
>                 *val = 0xffffffff;
>                 return PCIBIOS_DEVICE_NOT_FOUND;
>         }
> 
> So we're doing exactly the same thing.
> 
> Do you agree ?

I do agree.  I can't remember what I was thinking when I first
responded.

I *would* suggest making an advk_pcie_valid_device() so your structure
matches the other drivers.  I know it seems trivial when you're mostly
looking at one driver, but it really helps those who pay attention to
all of them.

Bjorn
diff mbox

Patch

diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
index 89f4e3d072d7..da2881ba7737 100644
--- a/drivers/pci/host/pci-aardvark.c
+++ b/drivers/pci/host/pci-aardvark.c
@@ -440,7 +440,7 @@  static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	u32 reg;
 	int ret;
 
-	if (PCI_SLOT(devfn) != 0) {
+	if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0)) {
 		*val = 0xffffffff;
 		return PCIBIOS_DEVICE_NOT_FOUND;
 	}
@@ -494,7 +494,7 @@  static int advk_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 	int offset;
 	int ret;
 
-	if (PCI_SLOT(devfn) != 0)
+	if ((bus->number == pcie->root_bus_nr) && (PCI_SLOT(devfn) != 0))
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	if (where % size)