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

[Bjorn Helgaas <helgaas@kernel.org>]

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

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