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

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]>
> ---
> Cc: "Krzysztof Wilczyński" <[email protected]>
> Cc: Bjorn Helgaas <[email protected]>
> Cc: Geert Uytterhoeven <[email protected]>
> Cc: Koichiro Den <[email protected]>
> Cc: Lorenzo Pieralisi <[email protected]>
> Cc: Magnus Damm <[email protected]>
> Cc: Manivannan Sadhasivam <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Yoshihiro Shimoda <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> ---
>  drivers/pci/controller/dwc/pcie-rcar-gen4.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index fbe465a29068f..b47281b30db96 100644
> --- 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);
> +

Hi Marek,

Thanks for adding me to Cc here. I tested this patch on S4 Spider and saw no
regression. I have two questions though.

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?

>  	ret = rcar_gen4_pcie_common_init(rcar);
>  	if (ret)
>  		return ret;
> @@ -571,7 +573,7 @@ static int rcar_gen4_add_dw_pcie_ep(struct rcar_gen4_pcie *rcar)
>  
>  	ret = dw_pcie_ep_init(ep);
>  	if (ret) {
> -		rcar_gen4_pcie_ep_deinit(rcar);
> +		rcar_gen4_pcie_common_deinit(rcar);

2. It seems to me dw_pcie_ep_init() can fail:
   - before pre_init() has run,
   - from inside common_init(), or
   - after pre_init() has succeeded and written PCIEDMAINTSTSEN_INIT (e.g.
     pci_epc_mem_init() fails)

   The caller cannot tell which case returned the error. In the first case,
   common_init() has not run, while in the last case PCIEDMAINTSTSEN is not 0.
   So this seems to me different from what the commit message says. Is
   common_deinit() intended to be safe in all three cases?

I may be missing some detail here. Could you clarify this for me?

Best regards,
Koichiro

>  		return ret;
>  	}
>  
> -- 
> 2.53.0
>