[PATCH] PCI: designware: fix asynchronous external abort in keystone PCIe h/w

[PATCH] PCI: designware: fix asynchronous external abort in keystone PCIe h/w

[Murali Karicheri]

Recent fixes for iATU unroll support introduced a bug that causes
asynchronous external abort in Keystone PCIe h/w which doesn't have
ATU port and the corresponding register. So the check should be moved
below where dw_pcie_prog_outbound_atu() is called to avoid that
being called on keystine PCIe h/w.

Here is the backtrace

[    0.771174] OF: PCI:   MEM 0x60000000..0x6fffffff -> 0x60000000
[    0.778118] Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
[    0.785548] pgd = c0003000
[    0.788347] [00000000] *pgd=80000800004003, *pmd=00000000
[    0.793864] Internal error: : 1211 [#1] PREEMPT SMP ARM
[    0.799197] Modules linked in:
[    0.802351] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-00009-g6ff59d2-dirty #7
[    0.810130] Hardware name: Keystone
[    0.813717] task: eb878000 task.stack: eb866000
[    0.818356] PC is at dw_pcie_setup_rc+0x24/0x380
[    0.823083] LR is at ks_pcie_host_init+0x10/0x170

Fixes: 416379f9ebde ("PCI: designware: Check for iATU unroll support after initializing host")
Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
Tested-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 - Applies to pci/master
 - Bug was introduced in v4.9
 - Tested on K2E EVM with SATA and DRA7-EVM with an intel PCI ethernet card

 drivers/pci/host/pcie-designware.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index bed1999..af8f6e9 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -807,11 +807,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 {
 	u32 val;
 
-	/* get iATU unroll support */
-	pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
-	dev_dbg(pp->dev, "iATU unroll: %s\n",
-		pp->iatu_unroll_enabled ? "enabled" : "disabled");
-
 	/* set the number of lanes */
 	val = dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL);
 	val &= ~PORT_LINK_MODE_MASK;
@@ -882,6 +877,11 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	 * we should not program the ATU here.
 	 */
 	if (!pp->ops->rd_other_conf) {
+		/* get iATU unroll support */
+		pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
+		dev_dbg(pp->dev, "iATU unroll: %s\n",
+			pp->iatu_unroll_enabled ? "enabled" : "disabled");
+
 		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
 					  PCIE_ATU_TYPE_MEM, pp->mem_base,
 					  pp->mem_bus_addr, pp->mem_size);
-- 
1.9.1

Re: [PATCH] PCI: designware: fix asynchronous external abort in keystone PCIe h/w

[Murali Karicheri]

On 01/04/2017 02:32 PM, Murali Karicheri wrote:
> Recent fixes for iATU unroll support introduced a bug that causes
> asynchronous external abort in Keystone PCIe h/w which doesn't have
> ATU port and the corresponding register. So the check should be moved
> below where dw_pcie_prog_outbound_atu() is called to avoid that
> being called on keystine PCIe h/w.
> 
> Here is the backtrace
> 
> [    0.771174] OF: PCI:   MEM 0x60000000..0x6fffffff -> 0x60000000
> [    0.778118] Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
> [    0.785548] pgd = c0003000
> [    0.788347] [00000000] *pgd=80000800004003, *pmd=00000000
> [    0.793864] Internal error: : 1211 [#1] PREEMPT SMP ARM
> [    0.799197] Modules linked in:
> [    0.802351] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-00009-g6ff59d2-dirty #7
> [    0.810130] Hardware name: Keystone
> [    0.813717] task: eb878000 task.stack: eb866000
> [    0.818356] PC is at dw_pcie_setup_rc+0x24/0x380
> [    0.823083] LR is at ks_pcie_host_init+0x10/0x170
> 
> Fixes: 416379f9ebde ("PCI: designware: Check for iATU unroll support after initializing host")
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> Tested-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  - Applies to pci/master
>  - Bug was introduced in v4.9
>  - Tested on K2E EVM with SATA and DRA7-EVM with an intel PCI ethernet card
> 
>  drivers/pci/host/pcie-designware.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index bed1999..af8f6e9 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -807,11 +807,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  {
>  	u32 val;
>  
> -	/* get iATU unroll support */
> -	pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
> -	dev_dbg(pp->dev, "iATU unroll: %s\n",
> -		pp->iatu_unroll_enabled ? "enabled" : "disabled");
> -
>  	/* set the number of lanes */
>  	val = dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL);
>  	val &= ~PORT_LINK_MODE_MASK;
> @@ -882,6 +877,11 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  	 * we should not program the ATU here.
>  	 */
>  	if (!pp->ops->rd_other_conf) {
> +		/* get iATU unroll support */
> +		pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
> +		dev_dbg(pp->dev, "iATU unroll: %s\n",
> +			pp->iatu_unroll_enabled ? "enabled" : "disabled");
> +
>  		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
>  					  PCIE_ATU_TYPE_MEM, pp->mem_base,
>  					  pp->mem_bus_addr, pp->mem_size);
> 
Bjorn,

A Gentle ping to merge this important fix to v4.10-rc kernel for Keystone PCI as
it is broken since v4.9.

Thanks

-- 
Murali Karicheri
Linux Kernel, Keystone

Re: [PATCH] PCI: designware: fix asynchronous external abort in keystone PCIe h/w

[Joao Pinto]

Hi,

Às 4:41 PM de 1/9/2017, Murali Karicheri escreveu:
> On 01/04/2017 02:32 PM, Murali Karicheri wrote:
>> Recent fixes for iATU unroll support introduced a bug that causes
>> asynchronous external abort in Keystone PCIe h/w which doesn't have
>> ATU port and the corresponding register. So the check should be moved
>> below where dw_pcie_prog_outbound_atu() is called to avoid that
>> being called on keystine PCIe h/w.
>>
>> Here is the backtrace
>>
>> [    0.771174] OF: PCI:   MEM 0x60000000..0x6fffffff -> 0x60000000
>> [    0.778118] Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
>> [    0.785548] pgd = c0003000
>> [    0.788347] [00000000] *pgd=80000800004003, *pmd=00000000
>> [    0.793864] Internal error: : 1211 [#1] PREEMPT SMP ARM
>> [    0.799197] Modules linked in:
>> [    0.802351] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-00009-g6ff59d2-dirty #7
>> [    0.810130] Hardware name: Keystone
>> [    0.813717] task: eb878000 task.stack: eb866000
>> [    0.818356] PC is at dw_pcie_setup_rc+0x24/0x380
>> [    0.823083] LR is at ks_pcie_host_init+0x10/0x170
>>
>> Fixes: 416379f9ebde ("PCI: designware: Check for iATU unroll support after initializing host")
>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> Tested-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  - Applies to pci/master
>>  - Bug was introduced in v4.9
>>  - Tested on K2E EVM with SATA and DRA7-EVM with an intel PCI ethernet card
>>
>>  drivers/pci/host/pcie-designware.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
>> index bed1999..af8f6e9 100644
>> --- a/drivers/pci/host/pcie-designware.c
>> +++ b/drivers/pci/host/pcie-designware.c
>> @@ -807,11 +807,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>>  {
>>  	u32 val;
>>  
>> -	/* get iATU unroll support */
>> -	pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
>> -	dev_dbg(pp->dev, "iATU unroll: %s\n",
>> -		pp->iatu_unroll_enabled ? "enabled" : "disabled");
>> -
>>  	/* set the number of lanes */
>>  	val = dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL);
>>  	val &= ~PORT_LINK_MODE_MASK;
>> @@ -882,6 +877,11 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>>  	 * we should not program the ATU here.
>>  	 */
>>  	if (!pp->ops->rd_other_conf) {
>> +		/* get iATU unroll support */
>> +		pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
>> +		dev_dbg(pp->dev, "iATU unroll: %s\n",
>> +			pp->iatu_unroll_enabled ? "enabled" : "disabled");
>> +
>>  		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
>>  					  PCIE_ATU_TYPE_MEM, pp->mem_base,
>>  					  pp->mem_bus_addr, pp->mem_size);
>>
> Bjorn,
> 
> A Gentle ping to merge this important fix to v4.10-rc kernel for Keystone PCI as
> it is broken since v4.9.
> 

In my understanding your proposal will have no impact in platform using ATU, so
for me is fine.

Acked-By: Joao Pinto <jpinto@synopsys.com>

> Thanks
> 

Re: [PATCH] PCI: designware: fix asynchronous external abort in keystone PCIe h/w

[Bjorn Helgaas]

Hi Murali,

On Wed, Jan 04, 2017 at 02:32:30PM -0500, Murali Karicheri wrote:
> Recent fixes for iATU unroll support introduced a bug that causes
> asynchronous external abort in Keystone PCIe h/w which doesn't have
> ATU port and the corresponding register. So the check should be moved
> below where dw_pcie_prog_outbound_atu() is called to avoid that
> being called on keystine PCIe h/w.
> 
> Here is the backtrace
> 
> [    0.771174] OF: PCI:   MEM 0x60000000..0x6fffffff -> 0x60000000
> [    0.778118] Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
> [    0.785548] pgd = c0003000
> [    0.788347] [00000000] *pgd=80000800004003, *pmd=00000000
> [    0.793864] Internal error: : 1211 [#1] PREEMPT SMP ARM
> [    0.799197] Modules linked in:
> [    0.802351] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-00009-g6ff59d2-dirty #7
> [    0.810130] Hardware name: Keystone
> [    0.813717] task: eb878000 task.stack: eb866000
> [    0.818356] PC is at dw_pcie_setup_rc+0x24/0x380
> [    0.823083] LR is at ks_pcie_host_init+0x10/0x170
> 
> Fixes: 416379f9ebde ("PCI: designware: Check for iATU unroll support after initializing host")

I tentatively applied this to for-linus for v4.10, with a stable tag
for v4.9+.

The patch itself mostly makes sense in terms of the code: we only use
iatu_unroll_enabled in dw_pcie_prog_outbound_atu().  We only call that
when "!pp->ops->rd_other_conf" or "!pp->ops->wr_other_conf".

So it makes sense that we only need to initialize iatu_unroll_enabled
in those cases.  But the current patch only initializes it if
"!pp->ops->rd_other_conf".

If we had "pp->ops->rd_other_conf && !pp->ops->wr_other_conf", we
would use iatu_unroll_enabled uninitialized in the
dw_pcie_wr_other_conf() path.  I suppose that's an invalid
configuration, but it'd be better if we didn't have to rely on the
host drivers to avoid that configuration.

It's not obvious how this is connected to 416379f9ebde, though.  It
*looks* like we call dw_pcie_iatu_unroll_enabled() on Keystone both
before and after that commit, so it seems like the external abort
should have happened even before it.

> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> Tested-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  - Applies to pci/master
>  - Bug was introduced in v4.9
>  - Tested on K2E EVM with SATA and DRA7-EVM with an intel PCI ethernet card
> 
>  drivers/pci/host/pcie-designware.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index bed1999..af8f6e9 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -807,11 +807,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  {
>  	u32 val;
>  
> -	/* get iATU unroll support */
> -	pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
> -	dev_dbg(pp->dev, "iATU unroll: %s\n",
> -		pp->iatu_unroll_enabled ? "enabled" : "disabled");
> -
>  	/* set the number of lanes */
>  	val = dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL);
>  	val &= ~PORT_LINK_MODE_MASK;
> @@ -882,6 +877,11 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  	 * we should not program the ATU here.
>  	 */
>  	if (!pp->ops->rd_other_conf) {
> +		/* get iATU unroll support */
> +		pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
> +		dev_dbg(pp->dev, "iATU unroll: %s\n",
> +			pp->iatu_unroll_enabled ? "enabled" : "disabled");
> +
>  		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
>  					  PCIE_ATU_TYPE_MEM, pp->mem_base,
>  					  pp->mem_bus_addr, pp->mem_size);
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PCI: designware: fix asynchronous external abort in keystone PCIe h/w

[Murali Karicheri]

On 01/10/2017 10:12 AM, Bjorn Helgaas wrote:
> Hi Murali,
> 
> On Wed, Jan 04, 2017 at 02:32:30PM -0500, Murali Karicheri wrote:
>> Recent fixes for iATU unroll support introduced a bug that causes
>> asynchronous external abort in Keystone PCIe h/w which doesn't have
>> ATU port and the corresponding register. So the check should be moved
>> below where dw_pcie_prog_outbound_atu() is called to avoid that
>> being called on keystine PCIe h/w.
>>
>> Here is the backtrace
>>
>> [    0.771174] OF: PCI:   MEM 0x60000000..0x6fffffff -> 0x60000000
>> [    0.778118] Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
>> [    0.785548] pgd = c0003000
>> [    0.788347] [00000000] *pgd=80000800004003, *pmd=00000000
>> [    0.793864] Internal error: : 1211 [#1] PREEMPT SMP ARM
>> [    0.799197] Modules linked in:
>> [    0.802351] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-00009-g6ff59d2-dirty #7
>> [    0.810130] Hardware name: Keystone
>> [    0.813717] task: eb878000 task.stack: eb866000
>> [    0.818356] PC is at dw_pcie_setup_rc+0x24/0x380
>> [    0.823083] LR is at ks_pcie_host_init+0x10/0x170
>>
>> Fixes: 416379f9ebde ("PCI: designware: Check for iATU unroll support after initializing host")
> 
> I tentatively applied this to for-linus for v4.10, with a stable tag
> for v4.9+.
> 

Ok  Thanks!

> The patch itself mostly makes sense in terms of the code: we only use
> iatu_unroll_enabled in dw_pcie_prog_outbound_atu().  We only call that
> when "!pp->ops->rd_other_conf" or "!pp->ops->wr_other_conf".
> 
> So it makes sense that we only need to initialize iatu_unroll_enabled
> in those cases.  But the current patch only initializes it if
> "!pp->ops->rd_other_conf".
> 

I think the code before also should have checked for both 
if ((!pp->ops->rd_other_conf) && (!pp->ops->wr_other_conf))

The assumption was if rd_other_conf is Null, the platform provides 
both rd_other_conf and wr_other_conf. I see I have added this API 
to support Keystone and the above assumption is true so far. 
It make sense to fix it if you agree.

> If we had "pp->ops->rd_other_conf && !pp->ops->wr_other_conf", we
> would use iatu_unroll_enabled uninitialized in the
> dw_pcie_wr_other_conf() path.  I suppose that's an invalid

Yes. So we need to fix the above to make the code correct instead
of leaving it exposed.

> configuration, but it'd be better if we didn't have to rely on the
> host drivers to avoid that configuration.

You mean to introduce the check in the designware core code above 
right?

Murali
> 
> It's not obvious how this is connected to 416379f9ebde, though.  It
> *looks* like we call dw_pcie_iatu_unroll_enabled() on Keystone both
> before and after that commit, so it seems like the external abort
> should have happened even before it.
> 

You are right. The problem was introduced by 

commit a0601a47053714eecec726aea5ebcd829f817497
Author: Joao Pinto <Joao.Pinto@synopsys.com>
Date:   Wed Aug 10 11:02:39 2016 +0100

    PCI: designware: Add iATU Unroll feature

which is fixed by commit 416379f9ebde and is again fixed by my 
commit. So probably I should have added both commits in
my description. 

Murali

>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> Tested-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  - Applies to pci/master
>>  - Bug was introduced in v4.9
>>  - Tested on K2E EVM with SATA and DRA7-EVM with an intel PCI ethernet card
>>
>>  drivers/pci/host/pcie-designware.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
>> index bed1999..af8f6e9 100644
>> --- a/drivers/pci/host/pcie-designware.c
>> +++ b/drivers/pci/host/pcie-designware.c
>> @@ -807,11 +807,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>>  {
>>  	u32 val;
>>  
>> -	/* get iATU unroll support */
>> -	pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
>> -	dev_dbg(pp->dev, "iATU unroll: %s\n",
>> -		pp->iatu_unroll_enabled ? "enabled" : "disabled");
>> -
>>  	/* set the number of lanes */
>>  	val = dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL);
>>  	val &= ~PORT_LINK_MODE_MASK;
>> @@ -882,6 +877,11 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>>  	 * we should not program the ATU here.
>>  	 */
>>  	if (!pp->ops->rd_other_conf) {
>> +		/* get iATU unroll support */
>> +		pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
>> +		dev_dbg(pp->dev, "iATU unroll: %s\n",
>> +			pp->iatu_unroll_enabled ? "enabled" : "disabled");
>> +
>>  		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
>>  					  PCIE_ATU_TYPE_MEM, pp->mem_base,
>>  					  pp->mem_bus_addr, pp->mem_size);
>> -- 
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Murali Karicheri
Linux Kernel, Keystone

Re: [PATCH] PCI: designware: fix asynchronous external abort in keystone PCIe h/w

[Bjorn Helgaas]

On Tue, Jan 10, 2017 at 12:18:29PM -0500, Murali Karicheri wrote:
> On 01/10/2017 10:12 AM, Bjorn Helgaas wrote:
> > Hi Murali,
> > 
> > On Wed, Jan 04, 2017 at 02:32:30PM -0500, Murali Karicheri wrote:
> >> Recent fixes for iATU unroll support introduced a bug that causes
> >> asynchronous external abort in Keystone PCIe h/w which doesn't have
> >> ATU port and the corresponding register. So the check should be moved
> >> below where dw_pcie_prog_outbound_atu() is called to avoid that
> >> being called on keystine PCIe h/w.
> >>
> >> Here is the backtrace
> >>
> >> [    0.771174] OF: PCI:   MEM 0x60000000..0x6fffffff -> 0x60000000
> >> [    0.778118] Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
> >> [    0.785548] pgd = c0003000
> >> [    0.788347] [00000000] *pgd=80000800004003, *pmd=00000000
> >> [    0.793864] Internal error: : 1211 [#1] PREEMPT SMP ARM
> >> [    0.799197] Modules linked in:
> >> [    0.802351] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-00009-g6ff59d2-dirty #7
> >> [    0.810130] Hardware name: Keystone
> >> [    0.813717] task: eb878000 task.stack: eb866000
> >> [    0.818356] PC is at dw_pcie_setup_rc+0x24/0x380
> >> [    0.823083] LR is at ks_pcie_host_init+0x10/0x170
> >>
> >> Fixes: 416379f9ebde ("PCI: designware: Check for iATU unroll support after initializing host")
> > 
> > I tentatively applied this to for-linus for v4.10, with a stable tag
> > for v4.9+.
> > 
> 
> Ok  Thanks!
> 
> > The patch itself mostly makes sense in terms of the code: we only use
> > iatu_unroll_enabled in dw_pcie_prog_outbound_atu().  We only call that
> > when "!pp->ops->rd_other_conf" or "!pp->ops->wr_other_conf".
> > 
> > So it makes sense that we only need to initialize iatu_unroll_enabled
> > in those cases.  But the current patch only initializes it if
> > "!pp->ops->rd_other_conf".
> > 
> 
> I think the code before also should have checked for both 
> if ((!pp->ops->rd_other_conf) && (!pp->ops->wr_other_conf))
> 
> The assumption was if rd_other_conf is Null, the platform provides 
> both rd_other_conf and wr_other_conf. I see I have added this API 
> to support Keystone and the above assumption is true so far. 
> It make sense to fix it if you agree.
> 
> > If we had "pp->ops->rd_other_conf && !pp->ops->wr_other_conf", we
> > would use iatu_unroll_enabled uninitialized in the
> > dw_pcie_wr_other_conf() path.  I suppose that's an invalid
> 
> Yes. So we need to fix the above to make the code correct instead
> of leaving it exposed.
> 
> > configuration, but it'd be better if we didn't have to rely on the
> > host drivers to avoid that configuration.
> 
> You mean to introduce the check in the designware core code above 
> right?

We *could* make the code look like:

  if (!pp->ops->rd_other_conf || !pp->ops->rd_other_conf)
    pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);

but that seems a little clunky and the connection between ATU and the
->*_other_conf() pointers is slightly obscure.  I guess it just ends
up being more detailed than I personally really want to worry about,
so I'm fine with leaving it as-is for now.

> > It's not obvious how this is connected to 416379f9ebde, though.  It
> > *looks* like we call dw_pcie_iatu_unroll_enabled() on Keystone both
> > before and after that commit, so it seems like the external abort
> > should have happened even before it.
> > 
> 
> You are right. The problem was introduced by 
> 
> commit a0601a47053714eecec726aea5ebcd829f817497
> Author: Joao Pinto <Joao.Pinto@synopsys.com>
> Date:   Wed Aug 10 11:02:39 2016 +0100
> 
>     PCI: designware: Add iATU Unroll feature
> 
> which is fixed by commit 416379f9ebde and is again fixed by my 
> commit. So probably I should have added both commits in
> my description. 

I added that.  The reason I wanted the correct "Fixes" information is
to figure out which stable kernels need this fix.  In this case, both:

  a0601a470537 ("PCI: designware: Add iATU Unroll feature") and
  416379f9ebde ("PCI: designware: Check for iATU unroll support after
    initializing host")

appeared in v4.9, so it doesn't change anything as far as the stable
tag is concerned.

The below is what I have on my for-linus branch:

commit 6fdb996a55684016de1ce639b9316b7092fde95f
Author: Murali Karicheri <m-karicheri2@ti.com>
Date:   Wed Jan 4 14:32:30 2017 -0500

    PCI: designware: Check for iATU unroll only on platforms that use ATU
    
    Previously we checked for iATU unroll support by reading PCIE_ATU_VIEWPORT
    even on platforms, e.g., Keystone, that do not have ATU ports.  This can
    cause bad behavior such as asynchronous external aborts:
    
      OF: PCI:   MEM 0x60000000..0x6fffffff -> 0x60000000
      Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
      pgd = c0003000
      [00000000] *pgd=80000800004003, *pmd=00000000
      Internal error: : 1211 [#1] PREEMPT SMP ARM
      Modules linked in:
      CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-00009-g6ff59d2-dirty #7
      Hardware name: Keystone
      task: eb878000 task.stack: eb866000
      PC is at dw_pcie_setup_rc+0x24/0x380
      LR is at ks_pcie_host_init+0x10/0x170
    
    Move the dw_pcie_iatu_unroll_enabled() check so we only call it on
    platforms that do not use the ATU.  These platforms supply their own
    ->rd_other_conf() and ->wr_other_conf() methods.
    
    [bhelgaas: changelog]
    Fixes: a0601a470537 ("PCI: designware: Add iATU Unroll feature")
    Fixes: 416379f9ebde ("PCI: designware: Check for iATU unroll support after initializing host")
    Tested-by: Kishon Vijay Abraham I <kishon@ti.com>
    Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    CC: stable@vger.kernel.org      # v4.9+

diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
index bed19994c1e9..af8f6e92e885 100644
--- a/drivers/pci/host/pcie-designware.c
+++ b/drivers/pci/host/pcie-designware.c
@@ -807,11 +807,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 {
 	u32 val;
 
-	/* get iATU unroll support */
-	pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
-	dev_dbg(pp->dev, "iATU unroll: %s\n",
-		pp->iatu_unroll_enabled ? "enabled" : "disabled");
-
 	/* set the number of lanes */
 	val = dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL);
 	val &= ~PORT_LINK_MODE_MASK;
@@ -882,6 +877,11 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
 	 * we should not program the ATU here.
 	 */
 	if (!pp->ops->rd_other_conf) {
+		/* get iATU unroll support */
+		pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
+		dev_dbg(pp->dev, "iATU unroll: %s\n",
+			pp->iatu_unroll_enabled ? "enabled" : "disabled");
+
 		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
 					  PCIE_ATU_TYPE_MEM, pp->mem_base,
 					  pp->mem_bus_addr, pp->mem_size);

Re: [PATCH] PCI: designware: fix asynchronous external abort in keystone PCIe h/w

[Murali Karicheri]

On 01/10/2017 06:06 PM, Bjorn Helgaas wrote:
> On Tue, Jan 10, 2017 at 12:18:29PM -0500, Murali Karicheri wrote:
>> On 01/10/2017 10:12 AM, Bjorn Helgaas wrote:
>>> Hi Murali,
>>>
>>> On Wed, Jan 04, 2017 at 02:32:30PM -0500, Murali Karicheri wrote:
>>>> Recent fixes for iATU unroll support introduced a bug that causes
>>>> asynchronous external abort in Keystone PCIe h/w which doesn't have
>>>> ATU port and the corresponding register. So the check should be moved
>>>> below where dw_pcie_prog_outbound_atu() is called to avoid that
>>>> being called on keystine PCIe h/w.
>>>>
>>>> Here is the backtrace
>>>>
>>>> [    0.771174] OF: PCI:   MEM 0x60000000..0x6fffffff -> 0x60000000
>>>> [    0.778118] Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
>>>> [    0.785548] pgd = c0003000
>>>> [    0.788347] [00000000] *pgd=80000800004003, *pmd=00000000
>>>> [    0.793864] Internal error: : 1211 [#1] PREEMPT SMP ARM
>>>> [    0.799197] Modules linked in:
>>>> [    0.802351] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-00009-g6ff59d2-dirty #7
>>>> [    0.810130] Hardware name: Keystone
>>>> [    0.813717] task: eb878000 task.stack: eb866000
>>>> [    0.818356] PC is at dw_pcie_setup_rc+0x24/0x380
>>>> [    0.823083] LR is at ks_pcie_host_init+0x10/0x170
>>>>
>>>> Fixes: 416379f9ebde ("PCI: designware: Check for iATU unroll support after initializing host")
>>>
>>> I tentatively applied this to for-linus for v4.10, with a stable tag
>>> for v4.9+.
>>>
>>
>> Ok  Thanks!
>>
>>> The patch itself mostly makes sense in terms of the code: we only use
>>> iatu_unroll_enabled in dw_pcie_prog_outbound_atu().  We only call that
>>> when "!pp->ops->rd_other_conf" or "!pp->ops->wr_other_conf".
>>>
>>> So it makes sense that we only need to initialize iatu_unroll_enabled
>>> in those cases.  But the current patch only initializes it if
>>> "!pp->ops->rd_other_conf".
>>>
>>
>> I think the code before also should have checked for both 
>> if ((!pp->ops->rd_other_conf) && (!pp->ops->wr_other_conf))
>>
>> The assumption was if rd_other_conf is Null, the platform provides 
>> both rd_other_conf and wr_other_conf. I see I have added this API 
>> to support Keystone and the above assumption is true so far. 
>> It make sense to fix it if you agree.
>>
>>> If we had "pp->ops->rd_other_conf && !pp->ops->wr_other_conf", we
>>> would use iatu_unroll_enabled uninitialized in the
>>> dw_pcie_wr_other_conf() path.  I suppose that's an invalid
>>
>> Yes. So we need to fix the above to make the code correct instead
>> of leaving it exposed.
>>
>>> configuration, but it'd be better if we didn't have to rely on the
>>> host drivers to avoid that configuration.
>>
>> You mean to introduce the check in the designware core code above 
>> right?
> 
> We *could* make the code look like:
> 
>   if (!pp->ops->rd_other_conf || !pp->ops->rd_other_conf)
>     pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
> 
> but that seems a little clunky and the connection between ATU and the
> ->*_other_conf() pointers is slightly obscure.  I guess it just ends
> up being more detailed than I personally really want to worry about,
> so I'm fine with leaving it as-is for now.
> 
>>> It's not obvious how this is connected to 416379f9ebde, though.  It
>>> *looks* like we call dw_pcie_iatu_unroll_enabled() on Keystone both
>>> before and after that commit, so it seems like the external abort
>>> should have happened even before it.
>>>
>>
>> You are right. The problem was introduced by 
>>
>> commit a0601a47053714eecec726aea5ebcd829f817497
>> Author: Joao Pinto <Joao.Pinto@synopsys.com>
>> Date:   Wed Aug 10 11:02:39 2016 +0100
>>
>>     PCI: designware: Add iATU Unroll feature
>>
>> which is fixed by commit 416379f9ebde and is again fixed by my 
>> commit. So probably I should have added both commits in
>> my description. 
> 
> I added that.  The reason I wanted the correct "Fixes" information is
> to figure out which stable kernels need this fix.  In this case, both:
> 
>   a0601a470537 ("PCI: designware: Add iATU Unroll feature") and
>   416379f9ebde ("PCI: designware: Check for iATU unroll support after
>     initializing host")
> 
> appeared in v4.9, so it doesn't change anything as far as the stable
> tag is concerned.
> 
> The below is what I have on my for-linus branch:
> 
> commit 6fdb996a55684016de1ce639b9316b7092fde95f
> Author: Murali Karicheri <m-karicheri2@ti.com>
> Date:   Wed Jan 4 14:32:30 2017 -0500
> 
>     PCI: designware: Check for iATU unroll only on platforms that use ATU
>     
>     Previously we checked for iATU unroll support by reading PCIE_ATU_VIEWPORT
>     even on platforms, e.g., Keystone, that do not have ATU ports.  This can
>     cause bad behavior such as asynchronous external aborts:
>     
>       OF: PCI:   MEM 0x60000000..0x6fffffff -> 0x60000000
>       Unhandled fault: asynchronous external abort (0x1211) at 0x00000000
>       pgd = c0003000
>       [00000000] *pgd=80000800004003, *pmd=00000000
>       Internal error: : 1211 [#1] PREEMPT SMP ARM
>       Modules linked in:
>       CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.9.0-00009-g6ff59d2-dirty #7
>       Hardware name: Keystone
>       task: eb878000 task.stack: eb866000
>       PC is at dw_pcie_setup_rc+0x24/0x380
>       LR is at ks_pcie_host_init+0x10/0x170
>     
>     Move the dw_pcie_iatu_unroll_enabled() check so we only call it on
>     platforms that do not use the ATU.  These platforms supply their own
>     ->rd_other_conf() and ->wr_other_conf() methods.
>     
>     [bhelgaas: changelog]
>     Fixes: a0601a470537 ("PCI: designware: Add iATU Unroll feature")
>     Fixes: 416379f9ebde ("PCI: designware: Check for iATU unroll support after initializing host")
>     Tested-by: Kishon Vijay Abraham I <kishon@ti.com>
>     Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>     Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>     CC: stable@vger.kernel.org      # v4.9+
> 
> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
> index bed19994c1e9..af8f6e92e885 100644
> --- a/drivers/pci/host/pcie-designware.c
> +++ b/drivers/pci/host/pcie-designware.c
> @@ -807,11 +807,6 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  {
>  	u32 val;
>  
> -	/* get iATU unroll support */
> -	pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
> -	dev_dbg(pp->dev, "iATU unroll: %s\n",
> -		pp->iatu_unroll_enabled ? "enabled" : "disabled");
> -
>  	/* set the number of lanes */
>  	val = dw_pcie_readl_rc(pp, PCIE_PORT_LINK_CONTROL);
>  	val &= ~PORT_LINK_MODE_MASK;
> @@ -882,6 +877,11 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  	 * we should not program the ATU here.
>  	 */
>  	if (!pp->ops->rd_other_conf) {
> +		/* get iATU unroll support */
> +		pp->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pp);
> +		dev_dbg(pp->dev, "iATU unroll: %s\n",
> +			pp->iatu_unroll_enabled ? "enabled" : "disabled");
> +
>  		dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0,
>  					  PCIE_ATU_TYPE_MEM, pp->mem_base,
>  					  pp->mem_bus_addr, pp->mem_size);
> 
Thanks Bjorn! Looks good.

-- 
Murali Karicheri
Linux Kernel, Keystone