| Message ID | 20200904140904.944-1-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | e338eecf3fe79054e8a31b8c39a1234b5acfdabe |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: rockchip: Fix bus checks in rockchip_pcie_valid_device() | expand |
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>
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 >
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.
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 >
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 > >
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
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; }
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(-)