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

[Koichiro Den]

On Wed, Aug 19, 2026 at 08:28:39AM +0200, Marek Vasut wrote:
> On 8/18/26 5:09 PM, Koichiro Den wrote:
> 
> Hello Den-san,
> 
> > 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.
> 
> This is clearly my error, the commit message is poorly written.
> 
> The rcar_gen4_add_dw_pcie_ep() contains two calls which can fail,
> dw_pcie_ep_init() and dw_pcie_ep_init_registers().
> 
> The first one, dw_pcie_ep_init(), internally calls .pre_init callback, which
> is implemented in rcar_gen4_pcie_ep_pre_init() on R-Car Gen4 .
> 
> The rcar_gen4_pcie_ep_pre_init() internally calls
> rcar_gen4_pcie_common_init(), which may succeed or fail. At this point, two
> possibilities can happen, the fail one is of interest:
> - If rcar_gen4_pcie_common_init() succeeds , then clk_bulk_prepare_enable()
> in rcar_gen4_pcie_common_init() did enable all dw->core_clks and those clock
> are kept enabled on exit from rcar_gen4_pcie_common_init() . The return
> value from rcar_gen4_pcie_common_init() is 0. OK.
> - If rcar_gen4_pcie_common_init() failed, then clk_bulk_disable_unprepare()
> is called in rcar_gen4_pcie_common_init() fail path and dw->core_clks clock
> are disabled. Return value from rcar_gen4_pcie_common_init() is non-zero.
> NG.
> 
> Back in rcar_gen4_pcie_ep_pre_init():
> - If rcar_gen4_pcie_ep_pre_init() succeeded , then write PCIEDMAINTSTSEN and
> enable edma_int , and exit with return value 0, OK.
> - If rcar_gen4_pcie_common_init() failed, then immediately exit with return
> value non-zero. Do not write PCIEDMAINTSTSEN and do not enable the
> edma_interrupts, do depend on previously set content of PCIEDMAINTSTSEN
> register, which was zeroed out at the beginning of this function instead.
> NG.
> 
> Finally, back in rcar_gen4_add_dw_pcie_ep():
> - If dw_pcie_ep_init() failed, then dw->core_clks have to be disabled at
> this point (*), and PCIEDMAINTSTSEN register is 0, therefore, do not call
> rcar_gen4_pcie_ep_deinit() which writes PCIEDMAINTSTSEN register to 0
> (again) and calls rcar_gen4_pcie_common_deinit(), directly call
> rcar_gen4_pcie_common_deinit() (**).
> 
> But as I wrote this part, I realized the (*) is not true, and
> rcar_gen4_pcie_common_deinit() is not the correct function to call in case
> dw_pcie_ep_init() fails.
> 
> I think we might need this kind of a patch:
> 
> "
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c
> b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index de8ee3db43601..a5801a74dec67 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -1553,7 +1553,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
>                                ep->page_size);
>         if (ret < 0) {
>                 dev_err(dev, "Failed to initialize address space\n");
> -               return ret;
> +               goto err_deinit;
>         }
> 
>         ep->msi_mem = pci_epc_mem_alloc_addr(epc, &ep->msi_mem_phys,
> @@ -1568,6 +1568,9 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
> 
>  err_exit_epc_mem:
>         pci_epc_mem_exit(epc);
> +err_deinit:
> +       if (ep->ops->pre_deinit)
> +               ep->ops->pre_deinit(ep);
> 
>         return ret;
>  }
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h
> b/drivers/pci/controller/dwc/pcie-designware.h
> index 0735ae9409240..733d8b6ad6d2b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -475,6 +475,7 @@ struct dw_pcie_rp {
> 
>  struct dw_pcie_ep_ops {
>         int     (*pre_init)(struct dw_pcie_ep *ep);
> +       void    (*pre_deinit)(struct dw_pcie_ep *ep);
>         int     (*init)(struct dw_pcie_ep *ep);
>         int     (*raise_irq)(struct dw_pcie_ep *ep, u8 func_no,
>                              unsigned int type, u16 interrupt_num);
> "
> 
> Then implement .pre_deinit callback for R-Car Gen4 such, that it would
> assert reset and stop the dw->core_clks by calling
> rcar_gen4_pcie_common_deinit(). Then we are sure (*) is true.
> 
> Finally, if dw_pcie_ep_init() call in rcar_gen4_add_dw_pcie_ep() fails, then
> with the aforementioned deinit implementation, I think it will be possible
> to simply do the following to cover (**).
> 
> ret = dw_pcie_ep_init(ep);
> if (ret)
>   return ret;
> 
> (I am also attaching the entire example as a diff, compile tested only thus
> far)
> 
> What do you think ?

Hi Marek,

Thanks for sharing your thoughts.

I think the callback idea is much cleaner. I wonder if .post_deinit might
describe its role more clearly.


P.S. I took a quick look through the related call paths, and it made me wonder
if we could take this a little further. Perhaps dw_pcie_ep_deinit() could call
the same hook at the end (I use .post_deinit as a tentative name below):

  dw_pcie_ep_init()
    -> .pre_init()
    -> generic intialization
    error:
      -> generic unwind
      -> .post_deinit()

  dw_pcie_ep_deinit()
    -> generic teardown
    -> .post_deinit()

Then would let the generic init/deinit pair own both hooks, and the
pcie-rcar-gen4 driver would no longer need to call rcar_gen4_pcie_ep_deinit()
separately. After dropping those explicit calls, the function could naturally be
renamed rcar_gen4_pcie_ep_post_deinit() and be used only as the hook.

This might be too much for this small fix. Even without this wider change,
however, I think rcar_gen4_pcie_ep_pre_deinit() should do the same operations as
rcar_gen4_pcie_ep_deinit(), rather than only calling common_deinit(). For
example, if pci_epc_mem_init() fails after .pre_init() succeeds, PCIEDMAINTSTSEN
is already 0xffff, and common_deinit() does not clear it.

Best regards,
Koichiro

> 
> > 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.
> 
> I think this ought to be addressed by the pre_deinit above.
> 
> > - When I injected a failure after pre_init() had succeeded, PCIEDMAINTSTSEN read
> >    back as 0xffff both before and after common_deinit().
> 
> I think that with the aforementioned pre_deinit implementation, in case
> pre_init succeeded and a failure occurred in dw_pcie_ep_init_registers()
> instead, the PCIEDMAINTSTSEN will be cleared in rcar_gen4_pcie_ep_deinit()
> correctly.
> 
> Does that cover this concern or is there another fail path which I missed ?
> 
> > 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.
> Thank you for your feedback, it allowed me to find an error in my train of
> thoughts, it is very much appreciated. I hope we can also find a good
> solution.
> 
> Thank you for your help !
> 
> -- 
> Best regards,
> Marek Vasut