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

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

[Thomas Petazzoni]

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)) {
 		*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

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

[Bjorn Helgaas]

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

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

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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

[Bjorn Helgaas]

[+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

[PATCH v2 2/7] PCI: aardvark: set PIO_ADDR_LS correctly in advk_pcie_rd_conf()

[Thomas Petazzoni]

From: Victor Gu <xigu@marvell.com>

When setting the PIO_ADDR_LS register during a configuration read, we
were properly passing the device number, function number and register
number, but not the bus number, causing issues when reading the
configuration of PCIe devices.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
index da2881ba7737..af7a9c4a61a4 100644
--- a/drivers/pci/host/pci-aardvark.c
+++ b/drivers/pci/host/pci-aardvark.c
@@ -459,7 +459,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
 	advk_writel(pcie, reg, PIO_CTRL);
 
 	/* Program the address registers */
-	reg = PCIE_BDF(devfn) | PCIE_CONF_REG(where);
+	reg = PCIE_CONF_ADDR(bus->number, devfn, where);
 	advk_writel(pcie, reg, PIO_ADDR_LS);
 	advk_writel(pcie, 0, PIO_ADDR_MS);
 
-- 
2.13.5

Re: [PATCH v2 2/7] PCI: aardvark: set PIO_ADDR_LS correctly in advk_pcie_rd_conf()

[Bjorn Helgaas]

On Thu, Sep 28, 2017 at 02:58:33PM +0200, Thomas Petazzoni wrote:
> From: Victor Gu <xigu@marvell.com>
> 
> When setting the PIO_ADDR_LS register during a configuration read, we
> were properly passing the device number, function number and register
> number, but not the bus number, causing issues when reading the
> configuration of PCIe devices.
> 
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
> index da2881ba7737..af7a9c4a61a4 100644
> --- a/drivers/pci/host/pci-aardvark.c
> +++ b/drivers/pci/host/pci-aardvark.c
> @@ -459,7 +459,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
>  	advk_writel(pcie, reg, PIO_CTRL);
>  
>  	/* Program the address registers */
> -	reg = PCIE_BDF(devfn) | PCIE_CONF_REG(where);
> +	reg = PCIE_CONF_ADDR(bus->number, devfn, where);

I think PCIE_BDF() is now unused and should be removed.

>  	advk_writel(pcie, reg, PIO_ADDR_LS);
>  	advk_writel(pcie, 0, PIO_ADDR_MS);
>  
> -- 
> 2.13.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/7] PCI: aardvark: set PIO_ADDR_LS correctly in advk_pcie_rd_conf()

[Thomas Petazzoni]

Hello,

On Thu, 5 Oct 2017 12:25:45 -0500, Bjorn Helgaas wrote:

> > diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
> > index da2881ba7737..af7a9c4a61a4 100644
> > --- a/drivers/pci/host/pci-aardvark.c
> > +++ b/drivers/pci/host/pci-aardvark.c
> > @@ -459,7 +459,7 @@ static int advk_pcie_rd_conf(struct pci_bus *bus, u32 devfn,
> >  	advk_writel(pcie, reg, PIO_CTRL);
> >  
> >  	/* Program the address registers */
> > -	reg = PCIE_BDF(devfn) | PCIE_CONF_REG(where);
> > +	reg = PCIE_CONF_ADDR(bus->number, devfn, where);  
> 
> I think PCIE_BDF() is now unused and should be removed.

True, I'll fix this in v3.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH v2 7/7] PCI: aardvark: define IRQ related hooks in pci_host_bridge

[Thomas Petazzoni]

Commit 769b461fc0c0 ("arm64: PCI: Drop DT IRQ allocation from
pcibios_alloc_irq()") was assuming all PCI host controller drivers had
been converted to use ->map_irq(), but that wasn't the case:
pci-aardvark had not been converted. Due to this, it broke the support
for legacy PCI interrupts when using the pci-aardvark driver (used on
Marvell Armada 3720 platforms).

In order to fix this, we make sure the ->map_irq and ->swizzle_irq
fields of pci_host_bridge are properly filled in.

Fixes: 769b461fc0c0 ("arm64: PCI: Drop DT IRQ allocation from pcibios_alloc_irq()")
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: <stable@vger.kernel.org> # v4.13+
---
 drivers/pci/host/pci-aardvark.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
index c1093b023e48..249a088b463c 100644
--- a/drivers/pci/host/pci-aardvark.c
+++ b/drivers/pci/host/pci-aardvark.c
@@ -998,6 +998,8 @@ static int advk_pcie_probe(struct platform_device *pdev)
 	bridge->sysdata = pcie;
 	bridge->busnr = 0;
 	bridge->ops = &advk_pcie_ops;
+	bridge->map_irq = of_irq_parse_and_map_pci;
+	bridge->swizzle_irq = pci_common_swizzle;
 
 	ret = pci_scan_root_bus_bridge(bridge);
 	if (ret < 0) {
-- 
2.13.5

Re: [PATCH v2 7/7] PCI: aardvark: define IRQ related hooks in pci_host_bridge

[Bjorn Helgaas]

[+cc Lorenzo]

On Thu, Sep 28, 2017 at 02:58:38PM +0200, Thomas Petazzoni wrote:
> Commit 769b461fc0c0 ("arm64: PCI: Drop DT IRQ allocation from
> pcibios_alloc_irq()") was assuming all PCI host controller drivers had
> been converted to use ->map_irq(), but that wasn't the case:
> pci-aardvark had not been converted. Due to this, it broke the support
> for legacy PCI interrupts when using the pci-aardvark driver (used on
> Marvell Armada 3720 platforms).

Lorenzo is pretty thorough, but maybe pci-aardvark.c got overlooked.
Cc'ing him just to make sure there's nothing deeper than that.

> In order to fix this, we make sure the ->map_irq and ->swizzle_irq
> fields of pci_host_bridge are properly filled in.
> 
> Fixes: 769b461fc0c0 ("arm64: PCI: Drop DT IRQ allocation from pcibios_alloc_irq()")
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: <stable@vger.kernel.org> # v4.13+
> ---
>  drivers/pci/host/pci-aardvark.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pci/host/pci-aardvark.c b/drivers/pci/host/pci-aardvark.c
> index c1093b023e48..249a088b463c 100644
> --- a/drivers/pci/host/pci-aardvark.c
> +++ b/drivers/pci/host/pci-aardvark.c
> @@ -998,6 +998,8 @@ static int advk_pcie_probe(struct platform_device *pdev)
>  	bridge->sysdata = pcie;
>  	bridge->busnr = 0;
>  	bridge->ops = &advk_pcie_ops;
> +	bridge->map_irq = of_irq_parse_and_map_pci;
> +	bridge->swizzle_irq = pci_common_swizzle;
>  
>  	ret = pci_scan_root_bus_bridge(bridge);
>  	if (ret < 0) {
> -- 
> 2.13.5
> 

Re: [PATCH v2 7/7] PCI: aardvark: define IRQ related hooks in pci_host_bridge

[Thomas Petazzoni]

Hello,

On Thu, 5 Oct 2017 12:55:47 -0500, Bjorn Helgaas wrote:

> On Thu, Sep 28, 2017 at 02:58:38PM +0200, Thomas Petazzoni wrote:
> > Commit 769b461fc0c0 ("arm64: PCI: Drop DT IRQ allocation from
> > pcibios_alloc_irq()") was assuming all PCI host controller drivers had
> > been converted to use ->map_irq(), but that wasn't the case:
> > pci-aardvark had not been converted. Due to this, it broke the support
> > for legacy PCI interrupts when using the pci-aardvark driver (used on
> > Marvell Armada 3720 platforms).  
> 
> Lorenzo is pretty thorough, but maybe pci-aardvark.c got overlooked.
> Cc'ing him just to make sure there's nothing deeper than that.

I talked about this issue with Lorenzo on IRC and then briefly in real
life at Linux Plumbers, and I'm pretty sure he agreed that it was just
due to the Aardvark driver having been overlooked.

But I'll let him confirm.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com