Re: [PATCH 17/19] PCI: rcar-gen2: Convert to use modern host bridge probe functions

[Rob Herring <robh@kernel.org>]

On Tue, Aug 4, 2020 at 6:12 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Rob,
>
> On Wed, Jul 22, 2020 at 4:26 AM Rob Herring <robh@kernel.org> wrote:
> > The rcar-gen2 host driver still uses the old Arm PCI setup function
> > pci_common_init_dev(). Let's update it to use the modern
> > devm_pci_alloc_host_bridge(), pci_parse_request_of_pci_ranges() and
> > pci_host_probe() functions.
> >
> > Cc: Marek Vasut <marek.vasut+renesas@gmail.com>
> > Cc: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: linux-renesas-soc@vger.kernel.org
> > Signed-off-by: Rob Herring <robh@kernel.org>
>
> This is now commit 92d69cc6275845a7 ("PCI: rcar-gen2: Convert to use
> modern host bridge probe functions"), and causes a crash on r8a7791/koelsch:

Can't say I'm surprised, this was a big change. Thanks for testing.


>     Unable to handle kernel NULL pointer dereference at virtual address 00000008
>     pgd = (ptrval)
>     [00000008] *pgd=00000000
>     Internal error: Oops: 5 [#1] SMP ARM
>     CPU: 0 PID: 1 Comm: swapper/0 Not tainted
> 5.8.0-rc1-shmobile-00035-g92d69cc6275845a7 #645
>     Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
>     PC is at rcar_pci_probe+0x154/0x340
>     LR is at _raw_spin_unlock_irqrestore+0x18/0x20
>
> > --- a/drivers/pci/controller/pci-rcar-gen2.c
> > +++ b/drivers/pci/controller/pci-rcar-gen2.c
> > @@ -189,19 +170,33 @@ static inline void rcar_pci_setup_errirq(struct rcar_pci_priv *priv) { }
> >  #endif
> >
> >  /* PCI host controller setup */
> > -static int rcar_pci_setup(int nr, struct pci_sys_data *sys)
> > +static void rcar_pci_setup(struct rcar_pci_priv *priv)
> >  {
> > -       struct rcar_pci_priv *priv = sys->private_data;
> > +       struct pci_host_bridge *bridge = pci_host_bridge_from_priv(priv);
> >         struct device *dev = priv->dev;
> >         void __iomem *reg = priv->reg;
> > +       struct resource_entry *entry;
> > +       unsigned long window_size;
> > +       unsigned long window_addr;
> > +       unsigned long window_pci;
> >         u32 val;
> > -       int ret;
> > +
> > +       entry = resource_list_first_type(&bridge->dma_ranges, IORESOURCE_MEM);
>
> bridge->dma_ranges is not initialized => BOOM.
>
> >  static int rcar_pci_probe(struct platform_device *pdev)
> >  {
> >         struct device *dev = &pdev->dev;
> >         struct resource *cfg_res, *mem_res;
> >         struct rcar_pci_priv *priv;
> > +       struct pci_host_bridge *bridge;
> >         void __iomem *reg;
> > -       struct hw_pci hw;
> > -       void *hw_private[1];
> > +       int ret;
> > +
> > +       bridge = devm_pci_alloc_host_bridge(dev, sizeof(*priv));
> > +       if (!bridge)
> > +               return -ENOMEM;
> > +
> > +       priv = pci_host_bridge_priv(bridge);
>
> This is the "new" priv instance.
>
> > +       bridge->sysdata = priv;
> >
> >         cfg_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >         reg = devm_ioremap_resource(dev, cfg_res);
>
> However, the "old" instance is still allocated below, and should be removed.
>
> I've send a patch to fix that, which should appear soon at
> https://lore.kernel.org/r/20200804120430.9253-1-geert+renesas@glider.be
>
> BTW, the conversion has the following impact on r8a7791/koelsch:
>
>     -pci-rcar-gen2 ee090000.pci: PCI: bus0 revision 11
>     +pci-rcar-gen2 ee090000.pci: host bridge /soc/pci@ee090000 ranges:
>     +pci-rcar-gen2 ee090000.pci:      MEM 0x00ee080000..0x00ee08ffff
> -> 0x00ee080000
>     +pci-rcar-gen2 ee090000.pci: PCI: revision 11
>      pci-rcar-gen2 ee090000.pci: PCI host bridge to bus 0000:00
>     -pci_bus 0000:00: root bus resource [mem 0xee080000-0xee0810ff]
>                                              ^^^^^^^^^^^^^^^^^^^^^
>     -pci_bus 0000:00: No busn resource found for root bus, will use [bus 00-ff]
>     +pci_bus 0000:00: root bus resource [bus 00]
>     +pci_bus 0000:00: root bus resource [mem 0xee080000-0xee08ffff]
>                                              ^^^^^^^^^^^^^^^^^^^^^
>
>     pci0: pci@ee090000 {
>             ...
>             reg = <0 0xee090000 0 0xc00>,
>                   <0 0xee080000 0 0x1100>;
>             ...
>             ranges = <0x02000000 0 0xee080000 0 0xee080000 0 0x00010000>;
>             ...
>             usb@1,0 {
>                     reg = <0x800 0 0 0 0>;
>                     ...
>             };
>
>             usb@2,0 {
>                     reg = <0x1000 0 0 0 0>;
>                     ...
>             };
>
>     }
>
> The old root bus resource size was based on the "reg" property.
> The new root bus resource size is based on the "ranges" property.

That was wrong to have PCI memory space in 'reg', but the driver could
always adjust the size to 0x1100 if needed.

BTW, the binding description seems to have the 'reg' description reversed.

> Given the only children are hardcoded, I guess that is OK?

In the sense that those are the only 2 devices and you know their
memory fits, yes. However, the memory space itself isn't hardcoded. If
you wanted to do that, then the children really need
'assigned-addresses' properties. I guess it happens to work because
memory space is allocated from the start and goes in order of device
addresses. But if that changed to top down allocation?

Rob