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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> > Cc: Rob Herring <robh@xxxxxxxxxx>
> > Cc: Shawn Lin <shawn.lin@xxxxxxxxxxxxxx>
> > ---
> > 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
> >
