Re: [PATCH] PCI: dwc: rcar-gen4: Fix potential unclocked access in rcar_gen4_pcie_ep_deinit()

On Tue, Aug 18, 2026 at 08:57:55AM +0200, Geert Uytterhoeven wrote:
> Hi Den-san,
> 
> On Tue, 18 Aug 2026 at 07:55, Koichiro Den <[email protected]> wrote:
> > On Thu, Aug 13, 2026 at 11:18:22AM +0200, Marek Vasut wrote:
> > > Initialize PCIEDMAINTSTSEN in rcar_gen4_pcie_ep_pre_init() to 0,
> > > and in case rcar_gen4_pcie_common_init() fails and the result
> > > propagates to dw_pcie_ep_init(), the invoke common deinit instead
> > > of endpoint deinit, because we are certain the PCIEDMAINTSTSEN
> > > register is already 0 and the common deinit does not do any register
> > > accesses.
> > >
> > > Signed-off-by: Marek Vasut <[email protected]>
> 
> > > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > > @@ -487,6 +487,8 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
> > >       struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> > >       int ret;
> > >
> > > +     writel(0, rcar->base + PCIEDMAINTSTSEN);
> > > +
> >
> > 1. The new writel(0, PCIEDMAINTSTSEN) runs before clk_bulk_prepare_enable().
> >    Is MSTP024 expected to be already released at this point? I added the quick
> >    check like below on Spider, and confirmed that the clock was already enabled.
> >
> >    --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> >    +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> >    @@ -9,6 +9,7 @@
> >      */
> >
> >     #include <linux/delay.h>
> >    +#include <linux/clk-provider.h>
> >     #include <linux/firmware.h>
> >     #include <linux/interrupt.h>
> >     #include <linux/io.h>
> >    @@ -487,6 +488,10 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
> >            struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> >            int ret;
> >
> >    +       if (__clk_is_enabled(dw->core_clks[DW_PCIE_CORE_CLK].clk))
> >    +               dev_info(dw->dev,
> >    +                        "PCIEC module clock was already enabled\n");
> >    +
> >            writel(0, rcar->base + PCIEDMAINTSTSEN);
> >
> >            ret = rcar_gen4_pcie_common_init(rcar);
> >
> >    If that is not guaranteed, wouldn't this write rely on firmware or some
> >    earlier user leaving the module clock enabled?
> 
> The module clock is enabled through Runtime PM:
> 
>     rcar_gen4_pcie_probe
>     -> rcar_gen4_pcie_prepare
>             -> pm_runtime_resume_and_get

Hi Geert,

I see, CPG_MOD 624. Thanks! Then I am not sure the original writel() is actually
unclocked. It runs before pm_runtime_put(), so runtime PM still keeps that clock
enabled. If "unclocked" means ref clock instead, the new writel() also runs
before clk_bulk_prepare_enable(). Probably I'm missing something again. I would
appreciate it if you could shed light on this.


P.S. I also ran two error injection experiments on an S4 Spider. This is also
related to question #2 in my previous comment.

- When I injected a failure inside common_init(), its error path unwound the
  bulk clocks itself. Calling common_deinit() afterwards triggered "already
  disabled/unprepared" warnings.
- When I injected a failure after pre_init() had succeeded, PCIEDMAINTSTSEN read
  back as 0xffff both before and after common_deinit().

So I think clean-up needs to know whether pre_init() completed, rather than
always calling common_deinit(). Something like this (field declaration omitted):

--- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
+++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
@@ -487,17 +487,24 @@ static int rcar_gen4_pcie_ep_pre_init(struct dw_pcie_ep *ep)
        struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
        int ret;

        ret = rcar_gen4_pcie_common_init(rcar);
        if (ret)
                return ret;

        writel(PCIEDMAINTSTSEN_INIT, rcar->base + PCIEDMAINTSTSEN);
+       rcar->ep_pre_init_done = true;

        return 0;
 }

 static void rcar_gen4_pcie_ep_deinit(struct rcar_gen4_pcie *rcar)
 {
+       if (!rcar->ep_pre_init_done)
+               return;
+
+       rcar->ep_pre_init_done = false;
        writel(0, rcar->base + PCIEDMAINTSTSEN);
        rcar_gen4_pcie_common_deinit(rcar);
 }

I may be missing something again, so please just take it with a pinch of salt.

Best regards,
Koichiro

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds