Message-ID: <20251226205239.GA4138276@bhelgaas>
Date: Fri, 26 Dec 2025 14:52:39 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Cc: Jingoo Han <jingoohan1@...il.com>,
	Manivannan Sadhasivam <mani@...nel.org>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kwilczynski@...nel.org>,
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
	Frank Li <Frank.Li@....com>, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, macro@...am.me.uk
Subject: Re: [PATCH 2/2] PCI: dwc: Fix missing iATU setup when ECAM is enabled

On Wed, Dec 03, 2025 at 11:50:15AM +0530, Krishna Chaitanya Chundru wrote:
> When ECAM is enabled, the driver skipped calling dw_pcie_iatu_setup()
> before configuring ECAM iATU entries. This left IO and MEM outbound
> windows unprogrammed, resulting in broken IO transactions. Additionally,
> dw_pcie_config_ecam_iatu() was only called during host initialization,
> so ECAM-related iATU entries were not restored after suspend/resume,
> leading to failures in configuration space access

Thanks for fixing this bug.

> To resolve these issues, the ECAM iATU configuration is moved into
> dw_pcie_setup_rc(). At the same time, dw_pcie_iatu_setup() is invoked
> when ECAM is enabled.
> 
> Rename msg_atu_index to ob_atu_index to track the next available outbound
> iATU index for ECAM and MSG TLP windows. Furthermore, an error check is
> added in dw_pcie_prog_outbound_atu() to avoid programming beyond
> num_ob_windows.
> 
> Fixes: f6fd357f7afb ("PCI: dwc: Prepare the driver for enabling ECAM mechanism using iATU 'CFG Shift Feature'")
> Reported-by: Maciej W. Rozycki <macro@...am.me.uk>
> Closes: https://lore.kernel.org/all/alpine.DEB.2.21.2511280256260.36486@angie.orcam.me.uk/
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 37 ++++++++++++++---------
>  drivers/pci/controller/dwc/pcie-designware.c      |  3 ++
>  drivers/pci/controller/dwc/pcie-designware.h      |  2 +-
>  3 files changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 4fb6331fbc2b322c1a1b6a8e4fe08f92e170da19..22c6ec7bff8840d935803f0b96749413b1c3f905 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -433,7 +433,7 @@ static int dw_pcie_config_ecam_iatu(struct dw_pcie_rp *pp)
>  	 * Immediate bus under Root Bus, needs type 0 iATU configuration and
>  	 * remaining buses need type 1 iATU configuration.

Tangent: I think this comment should say "Immediate bus under Root
Port needs type 0" or "Root bus needs type 0".

>  	 */
> -	atu.index = 0;
> +	atu.index = pp->ob_atu_index;

Previously we used entries 0 and 1 for config space accesses; now we
program the entries for bridge->windows first and use later entries for
config space.  But the implicit reservation of entry 0 persists below.

>  	atu.type = PCIE_ATU_TYPE_CFG0;
>  	atu.parent_bus_addr = pp->cfg0_base + SZ_1M;
>  	/* 1MiB is to cover 1 (bus) * 32 (devices) * 8 (functions) */
> @@ -443,6 +443,8 @@ static int dw_pcie_config_ecam_iatu(struct dw_pcie_rp *pp)
>  	if (ret)
>  		return ret;
>  
> +	pp->ob_atu_index++;
> +
>  	bus_range_max = resource_size(bus->res);
>  
>  	if (bus_range_max < 2)
> @@ -455,7 +457,11 @@ static int dw_pcie_config_ecam_iatu(struct dw_pcie_rp *pp)
>  	atu.size = (SZ_1M * bus_range_max) - SZ_2M;
>  	atu.ctrl2 = PCIE_ATU_CFG_SHIFT_MODE_ENABLE;
>  
> -	return dw_pcie_prog_outbound_atu(pci, &atu);
> +	ret = dw_pcie_prog_outbound_atu(pci, &atu);
> +	if (!ret)
> +		pp->ob_atu_index++;
> +
> +	return ret;

This would be better like this to match the type 0 case:

    ret = dw_pcie_prog_outbound_atu(pci, &atu);
    if (ret)
        return ret;

    pp->ob_atu_index++;
    return 0;

>  }
>  
>  static int dw_pcie_create_ecam_window(struct dw_pcie_rp *pp, struct resource *res)
> @@ -630,14 +636,6 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
>  	if (ret)
>  		goto err_free_msi;
>  
> -	if (pp->ecam_enabled) {
> -		ret = dw_pcie_config_ecam_iatu(pp);
> -		if (ret) {
> -			dev_err(dev, "Failed to configure iATU in ECAM mode\n");
> -			goto err_free_msi;
> -		}
> -	}
> -
>  	/*
>  	 * Allocate the resource for MSG TLP before programming the iATU
>  	 * outbound window in dw_pcie_setup_rc(). Since the allocation depends
> @@ -942,7 +940,7 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)

Earlier in dw_pcie_iatu_setup(), we have this:

    i = 0;
    resource_list_for_each_entry(entry, &pp->bridge->windows) {
        if (pci->num_ob_windows <= ++i)
            break;

        atu.index = i;
        dw_pcie_prog_outbound_atu(pci, &atu);

I think this starts at 1 because of the implicit reservation of entry
0 for config access, so now I think entry 0 is never used.

Since dw_pcie_prog_outbound_atu() now checks against
pci->num_ob_windows, I don't think we should also do it here.

The use of preincrement also makes this harder to read than it should
be.  You could do something like this:

    i = 0;
    resource_list_for_each_entry(entry, &pp->bridge->windows) {
        atu.index = i++;   # i++ better matches inbound ATU setup
        ret = dw_pcie_prog_outbound_atu(pci, &atu);
        if (ret)
            return ret;
        pci->ob_atu_index = i;
    }

>  		dev_warn(pci->dev, "Ranges exceed outbound iATU size (%d)\n",
>  			 pci->num_ob_windows);
>  
> -	pp->msg_atu_index = ++i;
> +	pp->ob_atu_index = ++i;
>  
>  	i = 0;
>  	resource_list_for_each_entry(entry, &pp->bridge->dma_ranges) {
> @@ -1084,14 +1082,23 @@ int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
>  	/*
>  	 * If the platform provides its own child bus config accesses, it means
>  	 * the platform uses its own address translation component rather than
> -	 * ATU, so we should not program the ATU here.
> +	 * ATU, so we should not program the ATU here. If ECAM is enabled, config
> +	 * space access goes through ATU, so setup ATU here.

Wrap comment to fit in 80 columns.

s/setup ATU/set up ATU/

>  	 */
> -	if (pp->bridge->child_ops == &dw_child_pcie_ops) {
> +	if (pp->bridge->child_ops == &dw_child_pcie_ops || pp->ecam_enabled) {
>  		ret = dw_pcie_iatu_setup(pp);
>  		if (ret)
>  			return ret;
>  	}
>  
> +	if (pp->ecam_enabled) {
> +		ret = dw_pcie_config_ecam_iatu(pp);
> +		if (ret) {
> +			dev_err(pci->dev, "Failed to configure iATU in ECAM mode\n");
> +			return ret;
> +		}
> +	}
> +
>  	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0);
>  
>  	/* Program correct class for RC */
> @@ -1113,7 +1120,7 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
>  	void __iomem *mem;
>  	int ret;
>  
> -	if (pci->num_ob_windows <= pci->pp.msg_atu_index)
> +	if (pci->num_ob_windows <= pci->pp.ob_atu_index)
>  		return -ENOSPC;

You added basically the same check in dw_pcie_prog_outbound_atu(), so
I don't think this check is needed, is it?

>  	if (!pci->pp.msg_res)
> @@ -1123,7 +1130,7 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
>  	atu.routing = PCIE_MSG_TYPE_R_BC;
>  	atu.type = PCIE_ATU_TYPE_MSG;
>  	atu.size = resource_size(pci->pp.msg_res);
> -	atu.index = pci->pp.msg_atu_index;
> +	atu.index = pci->pp.ob_atu_index;
>  
>  	atu.parent_bus_addr = pci->pp.msg_res->start - pci->parent_bus_offset;

It worries me a bit that we don't log any messages when this function
fails.  If it ever does fail, whether we failed to allocate pp.msg_res
or there are no available ATU entries, it looks like suspend might
silently fail with no clue in the logs, which sounds like a hassle to
debug.

> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index c644216995f69cbf065e61a0392bf1e5e32cf56e..f9f3c2f3532e0d0e9f8e4f42d8c5c9db68d55272 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -476,6 +476,9 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
>  	u32 retries, val;
>  	u64 limit_addr;
>  
> +	if (atu->index > pci->num_ob_windows)
> +		return -ENOSPC;
> +
>  	limit_addr = parent_bus_addr + atu->size - 1;
>  
>  	if ((limit_addr & ~pci->region_limit) != (parent_bus_addr & ~pci->region_limit) ||
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index e995f692a1ecd10130d3be3358827f801811387f..69d0bd8b3c57353763f98999e5d39527861fe1a7 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -423,8 +423,8 @@ struct dw_pcie_rp {
>  	struct pci_host_bridge  *bridge;
>  	raw_spinlock_t		lock;
>  	DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
> +	int			ob_atu_index;

The number of outbound ATU entries (num_ob_windows) is apparently a
Root Complex property, not a Root Port property, so it's in struct
dw_pcie.  If ob_atu_index tracks how many of those entries are in use,
I think it should be in struct dw_pci too instead of being in struct
dw_pcie_rp.

>  	bool			use_atu_msg;
> -	int			msg_atu_index;
>  	struct resource		*msg_res;
>  	bool			use_linkup_irq;
>  	struct pci_eq_presets	presets;
> 
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists