[PATCH] PCI: rockchip: Fix bus checks in rockchip_pcie_valid_device()

[PATCH] PCI: rockchip: Fix bus checks in rockchip_pcie_valid_device()

[Lorenzo Pieralisi]

The root bus checks rework in:

commit d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check if bus is root bus")

caused a regression whereby in rockchip_pcie_valid_device() if
the bus parameter is the root bus and the dev value == 0 the
function should return 1 (ie true) without checking if the
bus->parent pointer is a root bus because that triggers a NULL
pointer dereference.

Fix this by streamlining the root bus detection.

Fixes: d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check if bus is root bus")
Reported-by: Samuel Dionne-Riel <samuel@dionne-riel.com>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Shawn Lin <shawn.lin@rock-chips.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index 0bb2fb3e8a0b..9705059523a6 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -71,16 +71,13 @@ static void rockchip_pcie_update_txcredit_mui(struct rockchip_pcie *rockchip)
 static int rockchip_pcie_valid_device(struct rockchip_pcie *rockchip,
 				      struct pci_bus *bus, int dev)
 {
-	/* access only one slot on each root port */
-	if (pci_is_root_bus(bus) && dev > 0)
-		return 0;
-
 	/*
-	 * do not read more than one device on the bus directly attached
+	 * Access only one slot on each root port.
+	 * Do not read more than one device on the bus directly attached
 	 * to RC's downstream side.
 	 */
-	if (pci_is_root_bus(bus->parent) && dev > 0)
-		return 0;
+	if (pci_is_root_bus(bus) || pci_is_root_bus(bus->parent))
+		return dev == 0;
 
 	return 1;
 }
-- 
2.26.1

Re: [PATCH] PCI: rockchip: Fix bus checks in rockchip_pcie_valid_device()

[Rob Herring]

On Fri, Sep 4, 2020 at 8:09 AM Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> The root bus checks rework in:
>
> commit d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check if bus is root bus")
>
> caused a regression whereby in rockchip_pcie_valid_device() if
> the bus parameter is the root bus and the dev value == 0 the
> function should return 1 (ie true) without checking if the
> bus->parent pointer is a root bus because that triggers a NULL
> pointer dereference.
>
> Fix this by streamlining the root bus detection.
>
> Fixes: d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check if bus is root bus")
> Reported-by: Samuel Dionne-Riel <samuel@dionne-riel.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)

Even better than my broken version.

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH] PCI: rockchip: Fix bus checks in rockchip_pcie_valid_device()

[Lorenzo Pieralisi]

On Fri, Sep 04, 2020 at 03:09:04PM +0100, Lorenzo Pieralisi wrote:
> The root bus checks rework in:
> 
> commit d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check if bus is root bus")
> 
> caused a regression whereby in rockchip_pcie_valid_device() if
> the bus parameter is the root bus and the dev value == 0 the
> function should return 1 (ie true) without checking if the
> bus->parent pointer is a root bus because that triggers a NULL
> pointer dereference.
> 
> Fix this by streamlining the root bus detection.
> 
> Fixes: d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check if bus is root bus")
> Reported-by: Samuel Dionne-Riel <samuel@dionne-riel.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)

Hi Samuel,

I would kindly ask you please to test it since I changed the code,
I need your Tested-by before asking Bjorn to merge it.

Thanks,
Lorenzo

> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index 0bb2fb3e8a0b..9705059523a6 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -71,16 +71,13 @@ static void rockchip_pcie_update_txcredit_mui(struct rockchip_pcie *rockchip)
>  static int rockchip_pcie_valid_device(struct rockchip_pcie *rockchip,
>  				      struct pci_bus *bus, int dev)
>  {
> -	/* access only one slot on each root port */
> -	if (pci_is_root_bus(bus) && dev > 0)
> -		return 0;
> -
>  	/*
> -	 * do not read more than one device on the bus directly attached
> +	 * Access only one slot on each root port.
> +	 * Do not read more than one device on the bus directly attached
>  	 * to RC's downstream side.
>  	 */
> -	if (pci_is_root_bus(bus->parent) && dev > 0)
> -		return 0;
> +	if (pci_is_root_bus(bus) || pci_is_root_bus(bus->parent))
> +		return dev == 0;
>  
>  	return 1;
>  }
> -- 
> 2.26.1
> 

Re: [PATCH] PCI: rockchip: Fix bus checks in rockchip_pcie_valid_device()

[Samuel Dionne-Riel]

On Mon, 7 Sep 2020 11:20:16 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:

> On Fri, Sep 04, 2020 at 03:09:04PM +0100, Lorenzo Pieralisi wrote:
> > The root bus checks rework in:
> > 
> > commit d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check
> > if bus is root bus")
> > 
> > caused a regression whereby in rockchip_pcie_valid_device() if
> > the bus parameter is the root bus and the dev value == 0 the
> > function should return 1 (ie true) without checking if the
> > bus->parent pointer is a root bus because that triggers a NULL
> > pointer dereference.
> > 
> > Fix this by streamlining the root bus detection.
> > 
> > Fixes: d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check
> > if bus is root bus") Reported-by: Samuel Dionne-Riel
> > <samuel@dionne-riel.com> Signed-off-by: Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Shawn Lin <shawn.lin@rock-chips.com>
> > ---
> >  drivers/pci/controller/pcie-rockchip-host.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)  
> 
> Hi Samuel,
> 
> I would kindly ask you please to test it since I changed the code,
> I need your Tested-by before asking Bjorn to merge it.
> 

Hi,

I'm sorry, I had tested it, but didn't reply back as it worked. Not
being familiar with the customs of the mailing list.

Again, just in case, verified to work and fix the issue on top of
v5.9-rc3.

-- 
Samuel Dionne-Riel

Re: [PATCH] PCI: rockchip: Fix bus checks in rockchip_pcie_valid_device()

[Lorenzo Pieralisi]

On Fri, Sep 04, 2020 at 03:09:04PM +0100, Lorenzo Pieralisi wrote:
> The root bus checks rework in:
> 
> commit d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check if bus is root bus")
> 
> caused a regression whereby in rockchip_pcie_valid_device() if
> the bus parameter is the root bus and the dev value == 0 the
> function should return 1 (ie true) without checking if the
> bus->parent pointer is a root bus because that triggers a NULL
> pointer dereference.
> 
> Fix this by streamlining the root bus detection.
> 
> Fixes: d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check if bus is root bus")
> Reported-by: Samuel Dionne-Riel <samuel@dionne-riel.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Shawn Lin <shawn.lin@rock-chips.com>
> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)

Hi Bjorn,

this is a fix for a patch we merged in the last merge window, can
we send it for one of the upcoming -rcX please ?

Thanks,
Lorenzo

> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index 0bb2fb3e8a0b..9705059523a6 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -71,16 +71,13 @@ static void rockchip_pcie_update_txcredit_mui(struct rockchip_pcie *rockchip)
>  static int rockchip_pcie_valid_device(struct rockchip_pcie *rockchip,
>  				      struct pci_bus *bus, int dev)
>  {
> -	/* access only one slot on each root port */
> -	if (pci_is_root_bus(bus) && dev > 0)
> -		return 0;
> -
>  	/*
> -	 * do not read more than one device on the bus directly attached
> +	 * Access only one slot on each root port.
> +	 * Do not read more than one device on the bus directly attached
>  	 * to RC's downstream side.
>  	 */
> -	if (pci_is_root_bus(bus->parent) && dev > 0)
> -		return 0;
> +	if (pci_is_root_bus(bus) || pci_is_root_bus(bus->parent))
> +		return dev == 0;
>  
>  	return 1;
>  }
> -- 
> 2.26.1
> 

Re: [PATCH] PCI: rockchip: Fix bus checks in rockchip_pcie_valid_device()

[Bjorn Helgaas]

On Tue, Sep 08, 2020 at 11:02:31AM +0100, Lorenzo Pieralisi wrote:
> On Fri, Sep 04, 2020 at 03:09:04PM +0100, Lorenzo Pieralisi wrote:
> > The root bus checks rework in:
> > 
> > commit d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check if bus is root bus")
> > 
> > caused a regression whereby in rockchip_pcie_valid_device() if
> > the bus parameter is the root bus and the dev value == 0 the
> > function should return 1 (ie true) without checking if the
> > bus->parent pointer is a root bus because that triggers a NULL
> > pointer dereference.
> > 
> > Fix this by streamlining the root bus detection.
> > 
> > Fixes: d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check if bus is root bus")
> > Reported-by: Samuel Dionne-Riel <samuel@dionne-riel.com>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Shawn Lin <shawn.lin@rock-chips.com>
> > ---
> >  drivers/pci/controller/pcie-rockchip-host.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> Hi Bjorn,
> 
> this is a fix for a patch we merged in the last merge window, can
> we send it for one of the upcoming -rcX please ?

Sure.  I added Samuel's tested-by and put this on for-linus for v5.9.

But is there any chance we can figure out a way to make all these
"valid_device" functions look more similar?  They're a real potpourri
of styles:

  - Most return bool, a couple return int.

  - Some take PCI_SLOT(devfn); others take devfn.

  - Some reject "devfn > 0", others reject only "dev > 0".  Maybe this
    is a real difference, I dunno.

  - A few do unusual things that *look* like pci_is_root_bus():
      bus->primary == to_pci_host_bridge(bus->bridge)->busnr
      bus->number == cfg->busr.start
      bus->number == pcie->root_bus_nr

  - Some check for a negated condition first ("!pci_is_root_bus()"),
    i.e., I always prefer something like this:

      if (pci_is_root_bus(bus))
        return devfn == 0;

      return pcie_link_up();

    over this (from nwl_pcie_valid_device()):

      if (!pci_is_root_bus(bus)) {
        if (!pcie_link_up())
          return false;
      } else if (devfn > 0)
	return false;

      return true;

  - About half check whether the link is up.

  - The comments are pointlessly different.

> > diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> > index 0bb2fb3e8a0b..9705059523a6 100644
> > --- a/drivers/pci/controller/pcie-rockchip-host.c
> > +++ b/drivers/pci/controller/pcie-rockchip-host.c
> > @@ -71,16 +71,13 @@ static void rockchip_pcie_update_txcredit_mui(struct rockchip_pcie *rockchip)
> >  static int rockchip_pcie_valid_device(struct rockchip_pcie *rockchip,
> >  				      struct pci_bus *bus, int dev)
> >  {
> > -	/* access only one slot on each root port */
> > -	if (pci_is_root_bus(bus) && dev > 0)
> > -		return 0;
> > -
> >  	/*
> > -	 * do not read more than one device on the bus directly attached
> > +	 * Access only one slot on each root port.
> > +	 * Do not read more than one device on the bus directly attached
> >  	 * to RC's downstream side.
> >  	 */
> > -	if (pci_is_root_bus(bus->parent) && dev > 0)
> > -		return 0;
> > +	if (pci_is_root_bus(bus) || pci_is_root_bus(bus->parent))
> > +		return dev == 0;
> >  
> >  	return 1;
> >  }
> > -- 
> > 2.26.1
> > 

Re: [PATCH] PCI: rockchip: Fix bus checks in rockchip_pcie_valid_device()

[Rob Herring]

On Tue, Sep 8, 2020 at 4:08 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Tue, Sep 08, 2020 at 11:02:31AM +0100, Lorenzo Pieralisi wrote:
> > On Fri, Sep 04, 2020 at 03:09:04PM +0100, Lorenzo Pieralisi wrote:
> > > The root bus checks rework in:
> > >
> > > commit d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check if bus is root bus")
> > >
> > > caused a regression whereby in rockchip_pcie_valid_device() if
> > > the bus parameter is the root bus and the dev value == 0 the
> > > function should return 1 (ie true) without checking if the
> > > bus->parent pointer is a root bus because that triggers a NULL
> > > pointer dereference.
> > >
> > > Fix this by streamlining the root bus detection.
> > >
> > > Fixes: d84c572de1a3 ("PCI: rockchip: Use pci_is_root_bus() to check if bus is root bus")
> > > Reported-by: Samuel Dionne-Riel <samuel@dionne-riel.com>
> > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > > Cc: Rob Herring <robh@kernel.org>
> > > Cc: Shawn Lin <shawn.lin@rock-chips.com>
> > > ---
> > >  drivers/pci/controller/pcie-rockchip-host.c | 11 ++++-------
> > >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > Hi Bjorn,
> >
> > this is a fix for a patch we merged in the last merge window, can
> > we send it for one of the upcoming -rcX please ?
>
> Sure.  I added Samuel's tested-by and put this on for-linus for v5.9.
>
> But is there any chance we can figure out a way to make all these
> "valid_device" functions look more similar?  They're a real potpourri
> of styles:

I'm definitely trying to head in that direction, but trying to make
the all the copy-n-paste the same is an exercise in frustration.
Really, we need to factor out the copy-n-paste.

I expect now (in the DWC cleanup series) that we can support different
ops for root and child buses, we can refactor these a bit. For
Rockchip in particular, it looks like it is actually a Cadence
controller, so I'd like to get the Cadence driver working on Rockchip
and we can remove this one. Interestingly, the Cadence driver has no
such 'Do not read more than one device on the bus directly attached to
RC's downstream side.' check, so I suspect that's not really needed.
Though it could also be the same issue as the Neoverse N1 quirk. I
need to get a different Rockchip board because it seems PCIe doesn't
work on the Rock960c I have.

>   - Most return bool, a couple return int.
>
>   - Some take PCI_SLOT(devfn); others take devfn.
>
>   - Some reject "devfn > 0", others reject only "dev > 0".  Maybe this
>     is a real difference, I dunno.

If not just poor naming, it's just limited testing I'd guess. Or the
root bridge is always a single function? I imagine lots of these Arm
drivers are never tested with more than a handful of single devices
(most h/w is a single soldered device or M2 slot). There were numerous
cases I found where only 0 for root bus number would have worked.
Those should be fixed now.

Given filtering out root bus 'dev > 0' is fairly common, I'm wondering
if it should just be a bridge feature/quirk flag that the PCI core
could handle.

>   - A few do unusual things that *look* like pci_is_root_bus():
>       bus->primary == to_pci_host_bridge(bus->bridge)->busnr
>       bus->number == cfg->busr.start
>       bus->number == pcie->root_bus_nr

I think I've cleaned-up most of these. At least how we check for root
bus is consistent.

The checks with bus->primary are the oddballs which I don't really understand.

>   - Some check for a negated condition first ("!pci_is_root_bus()"),
>     i.e., I always prefer something like this:
>
>       if (pci_is_root_bus(bus))
>         return devfn == 0;
>
>       return pcie_link_up();
>
>     over this (from nwl_pcie_valid_device()):
>
>       if (!pci_is_root_bus(bus)) {
>         if (!pcie_link_up())
>           return false;
>       } else if (devfn > 0)
>         return false;
>
>       return true;
>
>   - About half check whether the link is up.

I think we agree that's a pointless, racy check. Happy to go rip those
out. Of course, the testing probably won't happen until a -rc1. :(

Rob