[v1] PCI: dwc: Fix ECAM enablement when used with vendor drivers

[PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor drivers

Posted by Krishna Chaitanya Chundru 3 months, 3 weeks ago

When the vendor configuration space is 256MB aligned, the DesignWare
PCIe host driver enables ECAM access and sets the DBI base to the start
of the config space. This causes vendor drivers to incorrectly program
iATU regions, as they rely on the DBI address for internal accesses.

To fix this, avoid overwriting the DBI base when ECAM is enabled.
Instead, introduce a custom ECAM PCI ops implementation that accesses
the DBI region directly for bus 0 and uses ECAM for other buses.

Fixes: f6fd357f7afb ("PCI: dwc: Prepare the driver for enabling ECAM mechanism using iATU 'CFG Shift Feature'")
Reported-by: Ron Economos <re@w6rz.net>
Closes: https://lore.kernel.org/all/eac81c57-1164-4d74-a1b4-6f353c577731@w6rz.net/
Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 28 +++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 20c9333bcb1c4812e2fd96047a49944574df1e6f..e92513c5bda51bde3a7157033ddbd73afa370d78 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -23,6 +23,7 @@
 #include "pcie-designware.h"
 
 static struct pci_ops dw_pcie_ops;
+static struct pci_ops dw_pcie_ecam_ops;
 static struct pci_ops dw_child_pcie_ops;
 
 #define DW_PCIE_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS		| \
@@ -471,9 +472,6 @@ static int dw_pcie_create_ecam_window(struct dw_pcie_rp *pp, struct resource *re
 	if (IS_ERR(pp->cfg))
 		return PTR_ERR(pp->cfg);
 
-	pci->dbi_base = pp->cfg->win;
-	pci->dbi_phys_addr = res->start;
-
 	return 0;
 }
 
@@ -529,7 +527,7 @@ static int dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
 		if (ret)
 			return ret;
 
-		pp->bridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;
+		pp->bridge->ops = &dw_pcie_ecam_ops;
 		pp->bridge->sysdata = pp->cfg;
 		pp->cfg->priv = pp;
 	} else {
@@ -842,12 +840,34 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
 }
 EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
 
+static void __iomem *dw_pcie_ecam_conf_map_bus(struct pci_bus *bus, unsigned int devfn, int where)
+{
+	struct pci_config_window *cfg = bus->sysdata;
+	struct dw_pcie_rp *pp = cfg->priv;
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	unsigned int busn = bus->number;
+
+	if (busn > 0)
+		return pci_ecam_map_bus(bus, devfn, where);
+
+	if (PCI_SLOT(devfn) > 0)
+		return NULL;
+
+	return pci->dbi_base + where;
+}
+
 static struct pci_ops dw_pcie_ops = {
 	.map_bus = dw_pcie_own_conf_map_bus,
 	.read = pci_generic_config_read,
 	.write = pci_generic_config_write,
 };
 
+static struct pci_ops dw_pcie_ecam_ops = {
+	.map_bus = dw_pcie_ecam_conf_map_bus,
+	.read = pci_generic_config_read,
+	.write = pci_generic_config_write,
+};
+
 static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

-- 
2.34.1

Re: [PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor drivers

Posted by Bjorn Helgaas 3 months, 3 weeks ago

On Fri, Oct 17, 2025 at 05:10:53PM +0530, Krishna Chaitanya Chundru wrote:
> When the vendor configuration space is 256MB aligned, the DesignWare
> PCIe host driver enables ECAM access and sets the DBI base to the start
> of the config space. This causes vendor drivers to incorrectly program
> iATU regions, as they rely on the DBI address for internal accesses.
> 
> To fix this, avoid overwriting the DBI base when ECAM is enabled.
> Instead, introduce a custom ECAM PCI ops implementation that accesses
> the DBI region directly for bus 0 and uses ECAM for other buses.
> 
> Fixes: f6fd357f7afb ("PCI: dwc: Prepare the driver for enabling ECAM mechanism using iATU 'CFG Shift Feature'")
> Reported-by: Ron Economos <re@w6rz.net>
> Closes: https://lore.kernel.org/all/eac81c57-1164-4d74-a1b4-6f353c577731@w6rz.net/
> Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 28 +++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 20c9333bcb1c4812e2fd96047a49944574df1e6f..e92513c5bda51bde3a7157033ddbd73afa370d78 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -23,6 +23,7 @@
>  #include "pcie-designware.h"
>  
>  static struct pci_ops dw_pcie_ops;
> +static struct pci_ops dw_pcie_ecam_ops;
>  static struct pci_ops dw_child_pcie_ops;
>  
>  #define DW_PCIE_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS		| \
> @@ -471,9 +472,6 @@ static int dw_pcie_create_ecam_window(struct dw_pcie_rp *pp, struct resource *re
>  	if (IS_ERR(pp->cfg))
>  		return PTR_ERR(pp->cfg);
>  
> -	pci->dbi_base = pp->cfg->win;
> -	pci->dbi_phys_addr = res->start;
> -
>  	return 0;
>  }
>  
> @@ -529,7 +527,7 @@ static int dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
>  		if (ret)
>  			return ret;
>  
> -		pp->bridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;
> +		pp->bridge->ops = &dw_pcie_ecam_ops;
>  		pp->bridge->sysdata = pp->cfg;
>  		pp->cfg->priv = pp;
>  	} else {
> @@ -842,12 +840,34 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
>  }
>  EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
>  
> +static void __iomem *dw_pcie_ecam_conf_map_bus(struct pci_bus *bus, unsigned int devfn, int where)
> +{
> +	struct pci_config_window *cfg = bus->sysdata;
> +	struct dw_pcie_rp *pp = cfg->priv;
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	unsigned int busn = bus->number;
> +
> +	if (busn > 0)
> +		return pci_ecam_map_bus(bus, devfn, where);

Is there a way to avoid the "root bus is bus 00" assumption here?  It
looks like something like this might work (it inverts the condition
to take care of the root bus special case first):

  if (bus == pp->bridge->bus) {
    if (PCI_SLOT(devfn) > 0)
      return NULL;

    return pci->dbi_base + where;
  }

  return pci_ecam_map_bus(bus, devfn, where);

> +	if (PCI_SLOT(devfn) > 0)
> +		return NULL;

This essentially says only one function (00.0) can be on the root bus.
I assume that someday that will be relaxed and there may be multiple
Root Ports and maybe RCiEPs on the root bus, so it would be nice if we
didn't have to have this check.

What happens without it?  Does the IP return the ~0 data that the PCI
core would interpret as "there's no device here"?

Regardless, I love this series because it removes quite a bit of code
and seems so much cleaner.

> +	return pci->dbi_base + where;
> +}
> +
>  static struct pci_ops dw_pcie_ops = {
>  	.map_bus = dw_pcie_own_conf_map_bus,
>  	.read = pci_generic_config_read,
>  	.write = pci_generic_config_write,
>  };
>  
> +static struct pci_ops dw_pcie_ecam_ops = {
> +	.map_bus = dw_pcie_ecam_conf_map_bus,
> +	.read = pci_generic_config_read,
> +	.write = pci_generic_config_write,
> +};
> +
>  static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>  {
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> 
> -- 
> 2.34.1
>

Re: [PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor drivers

Posted by Krishna Chaitanya Chundru 3 months, 3 weeks ago


On 10/18/2025 12:40 AM, Bjorn Helgaas wrote:
> On Fri, Oct 17, 2025 at 05:10:53PM +0530, Krishna Chaitanya Chundru wrote:
>> When the vendor configuration space is 256MB aligned, the DesignWare
>> PCIe host driver enables ECAM access and sets the DBI base to the start
>> of the config space. This causes vendor drivers to incorrectly program
>> iATU regions, as they rely on the DBI address for internal accesses.
>>
>> To fix this, avoid overwriting the DBI base when ECAM is enabled.
>> Instead, introduce a custom ECAM PCI ops implementation that accesses
>> the DBI region directly for bus 0 and uses ECAM for other buses.
>>
>> Fixes: f6fd357f7afb ("PCI: dwc: Prepare the driver for enabling ECAM mechanism using iATU 'CFG Shift Feature'")
>> Reported-by: Ron Economos <re@w6rz.net>
>> Closes: https://lore.kernel.org/all/eac81c57-1164-4d74-a1b4-6f353c577731@w6rz.net/
>> Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>> ---
>>   drivers/pci/controller/dwc/pcie-designware-host.c | 28 +++++++++++++++++++----
>>   1 file changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>> index 20c9333bcb1c4812e2fd96047a49944574df1e6f..e92513c5bda51bde3a7157033ddbd73afa370d78 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>> @@ -23,6 +23,7 @@
>>   #include "pcie-designware.h"
>>   
>>   static struct pci_ops dw_pcie_ops;
>> +static struct pci_ops dw_pcie_ecam_ops;
>>   static struct pci_ops dw_child_pcie_ops;
>>   
>>   #define DW_PCIE_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS		| \
>> @@ -471,9 +472,6 @@ static int dw_pcie_create_ecam_window(struct dw_pcie_rp *pp, struct resource *re
>>   	if (IS_ERR(pp->cfg))
>>   		return PTR_ERR(pp->cfg);
>>   
>> -	pci->dbi_base = pp->cfg->win;
>> -	pci->dbi_phys_addr = res->start;
>> -
>>   	return 0;
>>   }
>>   
>> @@ -529,7 +527,7 @@ static int dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
>>   		if (ret)
>>   			return ret;
>>   
>> -		pp->bridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;
>> +		pp->bridge->ops = &dw_pcie_ecam_ops;
>>   		pp->bridge->sysdata = pp->cfg;
>>   		pp->cfg->priv = pp;
>>   	} else {
>> @@ -842,12 +840,34 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
>>   }
>>   EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
>>   
>> +static void __iomem *dw_pcie_ecam_conf_map_bus(struct pci_bus *bus, unsigned int devfn, int where)
>> +{
>> +	struct pci_config_window *cfg = bus->sysdata;
>> +	struct dw_pcie_rp *pp = cfg->priv;
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	unsigned int busn = bus->number;
>> +
>> +	if (busn > 0)
>> +		return pci_ecam_map_bus(bus, devfn, where);
> 
> Is there a way to avoid the "root bus is bus 00" assumption here?  It
> looks like something like this might work (it inverts the condition
> to take care of the root bus special case first):
> 
>    if (bus == pp->bridge->bus) {
>      if (PCI_SLOT(devfn) > 0)
>        return NULL;
> 
>      return pci->dbi_base + where;
>    }
> 
>    return pci_ecam_map_bus(bus, devfn, where);
> This is working fine Bjorn, shall I send v2 with this change.

- Krishna Chaitanya.
>> +	if (PCI_SLOT(devfn) > 0)
>> +		return NULL;
> 
> This essentially says only one function (00.0) can be on the root bus.
> I assume that someday that will be relaxed and there may be multiple
> Root Ports and maybe RCiEPs on the root bus, so it would be nice if we
> didn't have to have this check.
> 
> What happens without it?  Does the IP return the ~0 data that the PCI
> core would interpret as "there's no device here"?
> 
> Regardless, I love this series because it removes quite a bit of code
> and seems so much cleaner.
> 
>> +	return pci->dbi_base + where;
>> +}
>> +
>>   static struct pci_ops dw_pcie_ops = {
>>   	.map_bus = dw_pcie_own_conf_map_bus,
>>   	.read = pci_generic_config_read,
>>   	.write = pci_generic_config_write,
>>   };
>>   
>> +static struct pci_ops dw_pcie_ecam_ops = {
>> +	.map_bus = dw_pcie_ecam_conf_map_bus,
>> +	.read = pci_generic_config_read,
>> +	.write = pci_generic_config_write,
>> +};
>> +
>>   static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>>   {
>>   	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor drivers

Posted by Bjorn Helgaas 3 months, 2 weeks ago

On Tue, Oct 21, 2025 at 05:42:39PM +0530, Krishna Chaitanya Chundru wrote:
> On 10/18/2025 12:40 AM, Bjorn Helgaas wrote:
> > On Fri, Oct 17, 2025 at 05:10:53PM +0530, Krishna Chaitanya Chundru wrote:

> > > +static void __iomem *dw_pcie_ecam_conf_map_bus(struct pci_bus *bus, unsigned int devfn, int where)
> > > +{
> > > +	struct pci_config_window *cfg = bus->sysdata;
> > > +	struct dw_pcie_rp *pp = cfg->priv;
> > > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > +	unsigned int busn = bus->number;
> > > +
> > > +	if (busn > 0)
> > > +		return pci_ecam_map_bus(bus, devfn, where);
> > 
> > Is there a way to avoid the "root bus is bus 00" assumption here?  It
> > looks like something like this might work (it inverts the condition
> > to take care of the root bus special case first):
> > 
> >    if (bus == pp->bridge->bus) {
> >      if (PCI_SLOT(devfn) > 0)
> >        return NULL;
> > 
> >      return pci->dbi_base + where;
> >    }
> > 
> >    return pci_ecam_map_bus(bus, devfn, where);
> > This is working fine Bjorn, shall I send v2 with this change.

Yes, please :)

Re: [PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor drivers

Posted by Manivannan Sadhasivam 3 months, 3 weeks ago

On Fri, Oct 17, 2025 at 02:10:05PM -0500, Bjorn Helgaas wrote:
> On Fri, Oct 17, 2025 at 05:10:53PM +0530, Krishna Chaitanya Chundru wrote:
> > When the vendor configuration space is 256MB aligned, the DesignWare
> > PCIe host driver enables ECAM access and sets the DBI base to the dw_pcie_ecam_conf_map_busstart
> > of the config space. This causes vendor drivers to incorrectly program
> > iATU regions, as they rely on the DBI address for internal accesses.
> > 
> > To fix this, avoid overwriting the DBI base when ECAM is enabled.
> > Instead, introduce a custom ECAM PCI ops implementation that accesses
> > the DBI region directly for bus 0 and uses ECAM for other buses.
> > 
> > Fixes: f6fd357f7afb ("PCI: dwc: Prepare the driver for enabling ECAM mechanism using iATU 'CFG Shift Feature'")
> > Reported-by: Ron Economos <re@w6rz.net>
> > Closes: https://lore.kernel.org/all/eac81c57-1164-4d74-a1b4-6f353c577731@w6rz.net/
> > Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
> > Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 28 +++++++++++++++++++----
> >  1 file changed, 24 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 20c9333bcb1c4812e2fd96047a49944574df1e6f..e92513c5bda51bde3a7157033ddbd73afa370d78 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -23,6 +23,7 @@
> >  #include "pcie-designware.h"
> >  
> >  static struct pci_ops dw_pcie_ops;
> > +static struct pci_ops dw_pcie_ecam_ops;
> >  static struct pci_ops dw_child_pcie_ops;
> >  
> >  #define DW_PCIE_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS		| \
> > @@ -471,9 +472,6 @@ static int dw_pcie_create_ecam_window(struct dw_pcie_rp *pp, struct resource *re
> >  	if (IS_ERR(pp->cfg))
> >  		return PTR_ERR(pp->cfg);
> >  
> > -	pci->dbi_base = pp->cfg->win;
> > -	pci->dbi_phys_addr = res->start;
> > -
> >  	return 0;
> >  }
> >  
> > @@ -529,7 +527,7 @@ static int dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
> >  		if (ret)
> >  			return ret;
> >  
> > -		pp->bridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;
> > +		pp->bridge->ops = &dw_pcie_ecam_ops;
> >  		pp->bridge->sysdata = pp->cfg;
> >  		pp->cfg->priv = pp;
> >  	} else {
> > @@ -842,12 +840,34 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
> >  }
> >  EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
> >  
> > +static void __iomem *dw_pcie_ecam_conf_map_bus(struct pci_bus *bus, unsigned int devfn, int where)
> > +{
> > +	struct pci_config_window *cfg = bus->sysdata;
> > +	struct dw_pcie_rp *pp = cfg->priv;
> > +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > +	unsigned int busn = bus->number;
> > +
> > +	if (busn > 0)
> > +		return pci_ecam_map_bus(bus, devfn, where);
> 
> Is there a way to avoid the "root bus is bus 00" assumption here?  It
> looks like something like this might work (it inverts the condition
> to take care of the root bus special case first):
> 
>   if (bus == pp->bridge->bus) {
>     if (PCI_SLOT(devfn) > 0)
>       return NULL;
> 
>     return pci->dbi_base + where;
>   }
> 
>   return pci_ecam_map_bus(bus, devfn, where);
> 

I guess it will work.

> > +	if (PCI_SLOT(devfn) > 0)
> > +		return NULL;
> 
> This essentially says only one function (00.0) can be on the root bus.
> I assume that someday that will be relaxed and there may be multiple
> Root Ports and maybe RCiEPs on the root bus, so it would be nice if we
> didn't have to have this check.
> 
> What happens without it?  Does the IP return the ~0 data that the PCI
> core would interpret as "there's no device here"?
> 

I hope the read returns ~0, but the idea is to catch the invalid access before
trying to read/write. In case of multi Root Port design, I don't think we have a
way to identify it. So maybe it is safe to remove this check.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor drivers

Posted by Krishna Chaitanya Chundru 3 months, 3 weeks ago


On 10/18/2025 8:39 AM, Manivannan Sadhasivam wrote:
> On Fri, Oct 17, 2025 at 02:10:05PM -0500, Bjorn Helgaas wrote:
>> On Fri, Oct 17, 2025 at 05:10:53PM +0530, Krishna Chaitanya Chundru wrote:
>>> When the vendor configuration space is 256MB aligned, the DesignWare
>>> PCIe host driver enables ECAM access and sets the DBI base to the dw_pcie_ecam_conf_map_busstart
>>> of the config space. This causes vendor drivers to incorrectly program
>>> iATU regions, as they rely on the DBI address for internal accesses.
>>>
>>> To fix this, avoid overwriting the DBI base when ECAM is enabled.
>>> Instead, introduce a custom ECAM PCI ops implementation that accesses
>>> the DBI region directly for bus 0 and uses ECAM for other buses.
>>>
>>> Fixes: f6fd357f7afb ("PCI: dwc: Prepare the driver for enabling ECAM mechanism using iATU 'CFG Shift Feature'")
>>> Reported-by: Ron Economos <re@w6rz.net>
>>> Closes: https://lore.kernel.org/all/eac81c57-1164-4d74-a1b4-6f353c577731@w6rz.net/
>>> Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
>>> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
>>> ---
>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 28 +++++++++++++++++++----
>>>   1 file changed, 24 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>> index 20c9333bcb1c4812e2fd96047a49944574df1e6f..e92513c5bda51bde3a7157033ddbd73afa370d78 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>> @@ -23,6 +23,7 @@
>>>   #include "pcie-designware.h"
>>>   
>>>   static struct pci_ops dw_pcie_ops;
>>> +static struct pci_ops dw_pcie_ecam_ops;
>>>   static struct pci_ops dw_child_pcie_ops;
>>>   
>>>   #define DW_PCIE_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS		| \
>>> @@ -471,9 +472,6 @@ static int dw_pcie_create_ecam_window(struct dw_pcie_rp *pp, struct resource *re
>>>   	if (IS_ERR(pp->cfg))
>>>   		return PTR_ERR(pp->cfg);
>>>   
>>> -	pci->dbi_base = pp->cfg->win;
>>> -	pci->dbi_phys_addr = res->start;
>>> -
>>>   	return 0;
>>>   }
>>>   
>>> @@ -529,7 +527,7 @@ static int dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
>>>   		if (ret)
>>>   			return ret;
>>>   
>>> -		pp->bridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;
>>> +		pp->bridge->ops = &dw_pcie_ecam_ops;
>>>   		pp->bridge->sysdata = pp->cfg;
>>>   		pp->cfg->priv = pp;
>>>   	} else {
>>> @@ -842,12 +840,34 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
>>>   }
>>>   EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
>>>   
>>> +static void __iomem *dw_pcie_ecam_conf_map_bus(struct pci_bus *bus, unsigned int devfn, int where)
>>> +{
>>> +	struct pci_config_window *cfg = bus->sysdata;
>>> +	struct dw_pcie_rp *pp = cfg->priv;
>>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> +	unsigned int busn = bus->number;
>>> +
>>> +	if (busn > 0)
>>> +		return pci_ecam_map_bus(bus, devfn, where);
>>
>> Is there a way to avoid the "root bus is bus 00" assumption here?  It
>> looks like something like this might work (it inverts the condition
>> to take care of the root bus special case first):
>>
>>    if (bus == pp->bridge->bus) {
>>      if (PCI_SLOT(devfn) > 0)
>>        return NULL;
>>
>>      return pci->dbi_base + where;
>>    }
>>
>>    return pci_ecam_map_bus(bus, devfn, where);
>>
> 
> I guess it will work.
> 
>>> +	if (PCI_SLOT(devfn) > 0)
>>> +		return NULL;
>>
>> This essentially says only one function (00.0) can be on the root bus.
>> I assume that someday that will be relaxed and there may be multiple
>> Root Ports and maybe RCiEPs on the root bus, so it would be nice if we
>> didn't have to have this check.
>>
>> What happens without it?  Does the IP return the ~0 data that the PCI
>> core would interpret as "there's no device here"?
>>
> 
> I hope the read returns ~0, but the idea is to catch the invalid access before
> trying to read/write. In case of multi Root Port design, I don't think we have a
> way to identify it. So maybe it is safe to remove this check.
>
For multi root port we may need to revisit this, currently along with
dbi there are some other registers iATU, elbi etc. So there might be
chances to read these registers like iATU as part of enumeration and
these can return non ~0 values which will have adverse effects.
So we should have this check for now.

- Krishna Chaitanya.

> - Mani
>

Re: [PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor drivers

Posted by Krishna Chaitanya Chundru 3 months, 3 weeks ago


On 10/18/2025 2:55 PM, Krishna Chaitanya Chundru wrote:
> 
> 
> On 10/18/2025 8:39 AM, Manivannan Sadhasivam wrote:
>> On Fri, Oct 17, 2025 at 02:10:05PM -0500, Bjorn Helgaas wrote:
>>> On Fri, Oct 17, 2025 at 05:10:53PM +0530, Krishna Chaitanya Chundru 
>>> wrote:
>>>> When the vendor configuration space is 256MB aligned, the DesignWare
>>>> PCIe host driver enables ECAM access and sets the DBI base to the 
>>>> dw_pcie_ecam_conf_map_busstart
>>>> of the config space. This causes vendor drivers to incorrectly program
>>>> iATU regions, as they rely on the DBI address for internal accesses.
>>>>
>>>> To fix this, avoid overwriting the DBI base when ECAM is enabled.
>>>> Instead, introduce a custom ECAM PCI ops implementation that accesses
>>>> the DBI region directly for bus 0 and uses ECAM for other buses.
>>>>
>>>> Fixes: f6fd357f7afb ("PCI: dwc: Prepare the driver for enabling ECAM 
>>>> mechanism using iATU 'CFG Shift Feature'")
>>>> Reported-by: Ron Economos <re@w6rz.net>
>>>> Closes: 
>>>> https://lore.kernel.org/all/eac81c57-1164-4d74-a1b4-6f353c577731@w6rz.net/
>>>> Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
>>>> Signed-off-by: Krishna Chaitanya Chundru 
>>>> <krishna.chundru@oss.qualcomm.com>
>>>> ---
>>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 28 
>>>> +++++++++++++++++++----
>>>>   1 file changed, 24 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c 
>>>> b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> index 
>>>> 20c9333bcb1c4812e2fd96047a49944574df1e6f..e92513c5bda51bde3a7157033ddbd73afa370d78 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>> @@ -23,6 +23,7 @@
>>>>   #include "pcie-designware.h"
>>>>   static struct pci_ops dw_pcie_ops;
>>>> +static struct pci_ops dw_pcie_ecam_ops;
>>>>   static struct pci_ops dw_child_pcie_ops;
>>>>   #define DW_PCIE_MSI_FLAGS_REQUIRED 
>>>> (MSI_FLAG_USE_DEF_DOM_OPS        | \
>>>> @@ -471,9 +472,6 @@ static int dw_pcie_create_ecam_window(struct 
>>>> dw_pcie_rp *pp, struct resource *re
>>>>       if (IS_ERR(pp->cfg))
>>>>           return PTR_ERR(pp->cfg);
>>>> -    pci->dbi_base = pp->cfg->win;
>>>> -    pci->dbi_phys_addr = res->start;
>>>> -
>>>>       return 0;
>>>>   }
>>>> @@ -529,7 +527,7 @@ static int dw_pcie_host_get_resources(struct 
>>>> dw_pcie_rp *pp)
>>>>           if (ret)
>>>>               return ret;
>>>> -        pp->bridge->ops = (struct pci_ops 
>>>> *)&pci_generic_ecam_ops.pci_ops;
>>>> +        pp->bridge->ops = &dw_pcie_ecam_ops;
>>>>           pp->bridge->sysdata = pp->cfg;
>>>>           pp->cfg->priv = pp;
>>>>       } else {
>>>> @@ -842,12 +840,34 @@ void __iomem *dw_pcie_own_conf_map_bus(struct 
>>>> pci_bus *bus, unsigned int devfn,
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
>>>> +static void __iomem *dw_pcie_ecam_conf_map_bus(struct pci_bus *bus, 
>>>> unsigned int devfn, int where)
>>>> +{
>>>> +    struct pci_config_window *cfg = bus->sysdata;
>>>> +    struct dw_pcie_rp *pp = cfg->priv;
>>>> +    struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>>> +    unsigned int busn = bus->number;
>>>> +
>>>> +    if (busn > 0)
>>>> +        return pci_ecam_map_bus(bus, devfn, where);
>>>
>>> Is there a way to avoid the "root bus is bus 00" assumption here?  It
>>> looks like something like this might work (it inverts the condition
>>> to take care of the root bus special case first):
>>>
>>>    if (bus == pp->bridge->bus) {
>>>      if (PCI_SLOT(devfn) > 0)
>>>        return NULL;
>>>
>>>      return pci->dbi_base + where;
>>>    }
>>>
>>>    return pci_ecam_map_bus(bus, devfn, where);
>>>
>>
>> I guess it will work.
>>
>>>> +    if (PCI_SLOT(devfn) > 0)
>>>> +        return NULL;
>>>
>>> This essentially says only one function (00.0) can be on the root bus.
>>> I assume that someday that will be relaxed and there may be multiple
>>> Root Ports and maybe RCiEPs on the root bus, so it would be nice if we
>>> didn't have to have this check.
>>>
>>> What happens without it?  Does the IP return the ~0 data that the PCI
>>> core would interpret as "there's no device here"?
>>>
>>
>> I hope the read returns ~0, but the idea is to catch the invalid 
>> access before
>> trying to read/write. In case of multi Root Port design, I don't think 
>> we have a
>> way to identify it. So maybe it is safe to remove this check.
>>
> For multi root port we may need to revisit this, currently along with
> dbi there are some other registers iATU, elbi etc. So there might be
> chances to read these registers like iATU as part of enumeration and
> these can return non ~0 values which will have adverse effects.
> So we should have this check for now.
> 
One more issue is some controllers may pass only 4k memory as dbi memory
so we might get unmapped address access issues also.
- Krishna Chaitanya.
> - Krishna Chaitanya.
> 
>> - Mani
>>

Re: [PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor drivers

Posted by Manivannan Sadhasivam 3 months, 3 weeks ago

On Sat, Oct 18, 2025 at 03:00:32PM +0530, Krishna Chaitanya Chundru wrote:
> 
> 
> On 10/18/2025 2:55 PM, Krishna Chaitanya Chundru wrote:
> > 
> > 
> > On 10/18/2025 8:39 AM, Manivannan Sadhasivam wrote:
> > > On Fri, Oct 17, 2025 at 02:10:05PM -0500, Bjorn Helgaas wrote:
> > > > On Fri, Oct 17, 2025 at 05:10:53PM +0530, Krishna Chaitanya
> > > > Chundru wrote:
> > > > > When the vendor configuration space is 256MB aligned, the DesignWare
> > > > > PCIe host driver enables ECAM access and sets the DBI base
> > > > > to the dw_pcie_ecam_conf_map_busstart
> > > > > of the config space. This causes vendor drivers to incorrectly program
> > > > > iATU regions, as they rely on the DBI address for internal accesses.
> > > > > 
> > > > > To fix this, avoid overwriting the DBI base when ECAM is enabled.
> > > > > Instead, introduce a custom ECAM PCI ops implementation that accesses
> > > > > the DBI region directly for bus 0 and uses ECAM for other buses.
> > > > > 
> > > > > Fixes: f6fd357f7afb ("PCI: dwc: Prepare the driver for
> > > > > enabling ECAM mechanism using iATU 'CFG Shift Feature'")
> > > > > Reported-by: Ron Economos <re@w6rz.net>
> > > > > Closes: https://lore.kernel.org/all/eac81c57-1164-4d74-a1b4-6f353c577731@w6rz.net/
> > > > > Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
> > > > > Signed-off-by: Krishna Chaitanya Chundru
> > > > > <krishna.chundru@oss.qualcomm.com>
> > > > > ---
> > > > >   drivers/pci/controller/dwc/pcie-designware-host.c | 28
> > > > > +++++++++++++++++++----
> > > > >   1 file changed, 24 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git
> > > > > a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > index 20c9333bcb1c4812e2fd96047a49944574df1e6f..e92513c5bda51bde3a7157033ddbd73afa370d78
> > > > > 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > @@ -23,6 +23,7 @@
> > > > >   #include "pcie-designware.h"
> > > > >   static struct pci_ops dw_pcie_ops;
> > > > > +static struct pci_ops dw_pcie_ecam_ops;
> > > > >   static struct pci_ops dw_child_pcie_ops;
> > > > >   #define DW_PCIE_MSI_FLAGS_REQUIRED
> > > > > (MSI_FLAG_USE_DEF_DOM_OPS        | \
> > > > > @@ -471,9 +472,6 @@ static int
> > > > > dw_pcie_create_ecam_window(struct dw_pcie_rp *pp, struct
> > > > > resource *re
> > > > >       if (IS_ERR(pp->cfg))
> > > > >           return PTR_ERR(pp->cfg);
> > > > > -    pci->dbi_base = pp->cfg->win;
> > > > > -    pci->dbi_phys_addr = res->start;
> > > > > -
> > > > >       return 0;
> > > > >   }
> > > > > @@ -529,7 +527,7 @@ static int
> > > > > dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
> > > > >           if (ret)
> > > > >               return ret;
> > > > > -        pp->bridge->ops = (struct pci_ops
> > > > > *)&pci_generic_ecam_ops.pci_ops;
> > > > > +        pp->bridge->ops = &dw_pcie_ecam_ops;
> > > > >           pp->bridge->sysdata = pp->cfg;
> > > > >           pp->cfg->priv = pp;
> > > > >       } else {
> > > > > @@ -842,12 +840,34 @@ void __iomem
> > > > > *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int
> > > > > devfn,
> > > > >   }
> > > > >   EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
> > > > > +static void __iomem *dw_pcie_ecam_conf_map_bus(struct
> > > > > pci_bus *bus, unsigned int devfn, int where)
> > > > > +{
> > > > > +    struct pci_config_window *cfg = bus->sysdata;
> > > > > +    struct dw_pcie_rp *pp = cfg->priv;
> > > > > +    struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > > +    unsigned int busn = bus->number;
> > > > > +
> > > > > +    if (busn > 0)
> > > > > +        return pci_ecam_map_bus(bus, devfn, where);
> > > > 
> > > > Is there a way to avoid the "root bus is bus 00" assumption here?  It
> > > > looks like something like this might work (it inverts the condition
> > > > to take care of the root bus special case first):
> > > > 
> > > >    if (bus == pp->bridge->bus) {
> > > >      if (PCI_SLOT(devfn) > 0)
> > > >        return NULL;
> > > > 
> > > >      return pci->dbi_base + where;
> > > >    }
> > > > 
> > > >    return pci_ecam_map_bus(bus, devfn, where);
> > > > 
> > > 
> > > I guess it will work.
> > > 
> > > > > +    if (PCI_SLOT(devfn) > 0)
> > > > > +        return NULL;
> > > > 
> > > > This essentially says only one function (00.0) can be on the root bus.
> > > > I assume that someday that will be relaxed and there may be multiple
> > > > Root Ports and maybe RCiEPs on the root bus, so it would be nice if we
> > > > didn't have to have this check.
> > > > 
> > > > What happens without it?  Does the IP return the ~0 data that the PCI
> > > > core would interpret as "there's no device here"?
> > > > 
> > > 
> > > I hope the read returns ~0, but the idea is to catch the invalid
> > > access before
> > > trying to read/write. In case of multi Root Port design, I don't
> > > think we have a
> > > way to identify it. So maybe it is safe to remove this check.
> > > 
> > For multi root port we may need to revisit this, currently along with
> > dbi there are some other registers iATU, elbi etc. So there might be
> > chances to read these registers like iATU as part of enumeration and
> > these can return non ~0 values which will have adverse effects.
> > So we should have this check for now.
> > 
> One more issue is some controllers may pass only 4k memory as dbi memory
> so we might get unmapped address access issues also.

Oh yes. The single Root Port controllers might supply only 4KiB of the DBI
space, so without the check, the driver could read/write past the mapped space.

Good catch!

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor drivers

Posted by Ron Economos 3 months, 3 weeks ago

On 10/17/25 04:40, Krishna Chaitanya Chundru wrote:
> When the vendor configuration space is 256MB aligned, the DesignWare
> PCIe host driver enables ECAM access and sets the DBI base to the start
> of the config space. This causes vendor drivers to incorrectly program
> iATU regions, as they rely on the DBI address for internal accesses.
>
> To fix this, avoid overwriting the DBI base when ECAM is enabled.
> Instead, introduce a custom ECAM PCI ops implementation that accesses
> the DBI region directly for bus 0 and uses ECAM for other buses.
>
> Fixes: f6fd357f7afb ("PCI: dwc: Prepare the driver for enabling ECAM mechanism using iATU 'CFG Shift Feature'")
> Reported-by: Ron Economos <re@w6rz.net>
> Closes: https://lore.kernel.org/all/eac81c57-1164-4d74-a1b4-6f353c577731@w6rz.net/
> Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>   drivers/pci/controller/dwc/pcie-designware-host.c | 28 +++++++++++++++++++----
>   1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 20c9333bcb1c4812e2fd96047a49944574df1e6f..e92513c5bda51bde3a7157033ddbd73afa370d78 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -23,6 +23,7 @@
>   #include "pcie-designware.h"
>   
>   static struct pci_ops dw_pcie_ops;
> +static struct pci_ops dw_pcie_ecam_ops;
>   static struct pci_ops dw_child_pcie_ops;
>   
>   #define DW_PCIE_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS		| \
> @@ -471,9 +472,6 @@ static int dw_pcie_create_ecam_window(struct dw_pcie_rp *pp, struct resource *re
>   	if (IS_ERR(pp->cfg))
>   		return PTR_ERR(pp->cfg);
>   
> -	pci->dbi_base = pp->cfg->win;
> -	pci->dbi_phys_addr = res->start;
> -
>   	return 0;
>   }
>   
> @@ -529,7 +527,7 @@ static int dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
>   		if (ret)
>   			return ret;
>   
> -		pp->bridge->ops = (struct pci_ops *)&pci_generic_ecam_ops.pci_ops;
> +		pp->bridge->ops = &dw_pcie_ecam_ops;
>   		pp->bridge->sysdata = pp->cfg;
>   		pp->cfg->priv = pp;
>   	} else {
> @@ -842,12 +840,34 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
>   }
>   EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
>   
> +static void __iomem *dw_pcie_ecam_conf_map_bus(struct pci_bus *bus, unsigned int devfn, int where)
> +{
> +	struct pci_config_window *cfg = bus->sysdata;
> +	struct dw_pcie_rp *pp = cfg->priv;
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	unsigned int busn = bus->number;
> +
> +	if (busn > 0)
> +		return pci_ecam_map_bus(bus, devfn, where);
> +
> +	if (PCI_SLOT(devfn) > 0)
> +		return NULL;
> +
> +	return pci->dbi_base + where;
> +}
> +
>   static struct pci_ops dw_pcie_ops = {
>   	.map_bus = dw_pcie_own_conf_map_bus,
>   	.read = pci_generic_config_read,
>   	.write = pci_generic_config_write,
>   };
>   
> +static struct pci_ops dw_pcie_ecam_ops = {
> +	.map_bus = dw_pcie_ecam_conf_map_bus,
> +	.read = pci_generic_config_read,
> +	.write = pci_generic_config_write,
> +};
> +
>   static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>   {
>   	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>
Works good on the SiFive FU740 controller.

Tested-by: Ron Economos <re@w6rz.net>

[PATCH 1/2] PCI: dwc: Fix ECAM enablement when used with vendor drivers
[PATCH 2/2] PCI: dwc: qcom: Revert "PCI: qcom: Prepare for the DWC ECAM enablement"