Re: [RFC PATCH] PCI: designware: add host_init() error handling

[Joao Pinto]

Às 11:51 AM de 12/2/2016, Srinivas Kandagatla escreveu:
> 
> 
> On 02/12/16 10:32, Joao Pinto wrote:
>>
>> Hi Srinivas,
>>
>> Às 11:51 AM de 12/1/2016, Srinivas Kandagatla escreveu:
>>>  drivers/pci/host/pci-dra7xx.c   |  4 +++-
>>>  drivers/pci/host/pci-exynos.c   |  4 +++-
>>>  drivers/pci/host/pci-imx6.c |  4 +++-
>>>  drivers/pci/host/pci-keystone.c |  4 +++-
>>>  drivers/pci/host/pci-layerscape.c   | 12 
>>>  drivers/pci/host/pcie-armada8k.c|  4 +++-
>>>  drivers/pci/host/pcie-designware-plat.c |  4 +++-
>>>  drivers/pci/host/pcie-designware.c  |  4 +++-
>>>  drivers/pci/host/pcie-designware.h  |  2 +-
>>>  drivers/pci/host/pcie-qcom.c|  6 --
>>>  drivers/pci/host/pcie-spear13xx.c   |  4 +++-
>>>  11 files changed, 37 insertions(+), 15 deletions(-)
>>>
>>
>> Thanks for the patch!
>>
>> In my opinion your idea is good but only qcom driver is able to detect 
>> failure
>> in the specific host init routine, all others have a 'return 0' even if
>> something not well init. I would recomend that we take this issue a bit 
>> further
>> and add the error checking to all specific pci drivers in order to make them 
>> as
>> robust as qcom'.
> I totally agree with you, I can give this a go in next version.

Sure, but I think it would be better to finish now since we are on top of the
task. I can help you if you need.

Thanks Joao

> 
> Thanks,
> srini
> 
>>
>> Thanks,
>> Joao
>>

Re: [RFC PATCH] PCI: designware: add host_init() error handling

[Srinivas Kandagatla]

On 02/12/16 10:32, Joao Pinto wrote:


Hi Srinivas,

Às 11:51 AM de 12/1/2016, Srinivas Kandagatla escreveu:

 drivers/pci/host/pci-dra7xx.c   |  4 +++-
 drivers/pci/host/pci-exynos.c   |  4 +++-
 drivers/pci/host/pci-imx6.c |  4 +++-
 drivers/pci/host/pci-keystone.c |  4 +++-
 drivers/pci/host/pci-layerscape.c   | 12 
 drivers/pci/host/pcie-armada8k.c|  4 +++-
 drivers/pci/host/pcie-designware-plat.c |  4 +++-
 drivers/pci/host/pcie-designware.c  |  4 +++-
 drivers/pci/host/pcie-designware.h  |  2 +-
 drivers/pci/host/pcie-qcom.c|  6 --
 drivers/pci/host/pcie-spear13xx.c   |  4 +++-
 11 files changed, 37 insertions(+), 15 deletions(-)



Thanks for the patch!

In my opinion your idea is good but only qcom driver is able to detect failure
in the specific host init routine, all others have a 'return 0' even if
something not well init. I would recomend that we take this issue a bit further
and add the error checking to all specific pci drivers in order to make them as
robust as qcom'.

I totally agree with you, I can give this a go in next version.

Thanks,
srini



Thanks,
Joao

Re: [RFC PATCH] PCI: designware: add host_init() error handling

[Joao Pinto]

Hi Srinivas,

Às 11:51 AM de 12/1/2016, Srinivas Kandagatla escreveu:
>  drivers/pci/host/pci-dra7xx.c   |  4 +++-
>  drivers/pci/host/pci-exynos.c   |  4 +++-
>  drivers/pci/host/pci-imx6.c |  4 +++-
>  drivers/pci/host/pci-keystone.c |  4 +++-
>  drivers/pci/host/pci-layerscape.c   | 12 
>  drivers/pci/host/pcie-armada8k.c|  4 +++-
>  drivers/pci/host/pcie-designware-plat.c |  4 +++-
>  drivers/pci/host/pcie-designware.c  |  4 +++-
>  drivers/pci/host/pcie-designware.h  |  2 +-
>  drivers/pci/host/pcie-qcom.c|  6 --
>  drivers/pci/host/pcie-spear13xx.c   |  4 +++-
>  11 files changed, 37 insertions(+), 15 deletions(-)
> 

Thanks for the patch!

In my opinion your idea is good but only qcom driver is able to detect failure
in the specific host init routine, all others have a 'return 0' even if
something not well init. I would recomend that we take this issue a bit further
and add the error checking to all specific pci drivers in order to make them as
robust as qcom'.

Thanks,
Joao

[RFC PATCH] PCI: designware: add host_init() error handling

[Srinivas Kandagatla]

This patch add support to return value from host_init() callback from
drivers,so that the designware library can handle or pass it to proper
place. Issue with void return type is that errors or error handling
within host_init() callback are never know to designware code, which
could go ahead and access registers even in error cases.

Typical case in qcom controller driver is to turn off clks in case of
errors, if designware code continues to read/write register when clocks
are turned off the board would reboot/lockup.

Signed-off-by: Srinivas Kandagatla 
---

Currently designware code does not have a way return errors generated
as part of host_init() callback in controller drivers. This is an issue
with controller drivers like qcom which turns off the clocks in error
handling path. As the dw core is un aware of this would continue to
access registers which faults resulting in board reboots/hangs. 

There are two ways to solve this issue,
one is remove error handling in the qcom controller host_init() function
other is to handle error and pass back to dw core code which would then
pass back to controller driver as part of dw_pcie_host_init() return value.

Second option seems more sensible and correct way to fix the issue,
this patch does the same.

As part of this change to host_init() return type I had to patch other
host controller drivers which use dw core. Most of these changes are
harmless as I have not added any extra code other then returning 0
from host_init().

 drivers/pci/host/pci-dra7xx.c   |  4 +++-
 drivers/pci/host/pci-exynos.c   |  4 +++-
 drivers/pci/host/pci-imx6.c |  4 +++-
 drivers/pci/host/pci-keystone.c |  4 +++-
 drivers/pci/host/pci-layerscape.c   | 12 
 drivers/pci/host/pcie-armada8k.c|  4 +++-
 drivers/pci/host/pcie-designware-plat.c |  4 +++-
 drivers/pci/host/pcie-designware.c  |  4 +++-
 drivers/pci/host/pcie-designware.h  |  2 +-
 drivers/pci/host/pcie-qcom.c|  6 --
 drivers/pci/host/pcie-spear13xx.c   |  4 +++-
 11 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c
index 9595fad..f63491c 100644
--- a/drivers/pci/host/pci-dra7xx.c
+++ b/drivers/pci/host/pci-dra7xx.c
@@ -127,7 +127,7 @@ static void dra7xx_pcie_enable_interrupts(struct 
dra7xx_pcie *dra7xx)
   LEG_EP_INTERRUPTS);
 }
 
-static void dra7xx_pcie_host_init(struct pcie_port *pp)
+static int dra7xx_pcie_host_init(struct pcie_port *pp)
 {
struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pp);
 
@@ -142,6 +142,8 @@ static void dra7xx_pcie_host_init(struct pcie_port *pp)
if (IS_ENABLED(CONFIG_PCI_MSI))
dw_pcie_msi_init(pp);
dra7xx_pcie_enable_interrupts(dra7xx);
+
+   return 0;
 }
 
 static struct pcie_host_ops dra7xx_pcie_host_ops = {
diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c
index f1c544b..b5a8f11 100644
--- a/drivers/pci/host/pci-exynos.c
+++ b/drivers/pci/host/pci-exynos.c
@@ -458,12 +458,14 @@ static int exynos_pcie_link_up(struct pcie_port *pp)
return 0;
 }
 
-static void exynos_pcie_host_init(struct pcie_port *pp)
+static int exynos_pcie_host_init(struct pcie_port *pp)
 {
struct exynos_pcie *exynos_pcie = to_exynos_pcie(pp);
 
exynos_pcie_establish_link(exynos_pcie);
exynos_pcie_enable_interrupts(exynos_pcie);
+
+   return 0;
 }
 
 static struct pcie_host_ops exynos_pcie_host_ops = {
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c
index c8cefb0..4844367 100644
--- a/drivers/pci/host/pci-imx6.c
+++ b/drivers/pci/host/pci-imx6.c
@@ -550,7 +550,7 @@ static int imx6_pcie_establish_link(struct imx6_pcie 
*imx6_pcie)
return ret;
 }
 
-static void imx6_pcie_host_init(struct pcie_port *pp)
+static int imx6_pcie_host_init(struct pcie_port *pp)
 {
struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
 
@@ -562,6 +562,8 @@ static void imx6_pcie_host_init(struct pcie_port *pp)
 
if (IS_ENABLED(CONFIG_PCI_MSI))
dw_pcie_msi_init(pp);
+
+   return 0;
 }
 
 static int imx6_pcie_link_up(struct pcie_port *pp)
diff --git a/drivers/pci/host/pci-keystone.c b/drivers/pci/host/pci-keystone.c
index 043c19a..fded7f9 100644
--- a/drivers/pci/host/pci-keystone.c
+++ b/drivers/pci/host/pci-keystone.c
@@ -260,7 +260,7 @@ static int keystone_pcie_fault(unsigned long addr, unsigned 
int fsr,
return 0;
 }
 
-static void __init ks_pcie_host_init(struct pcie_port *pp)
+static int __init ks_pcie_host_init(struct pcie_port *pp)
 {
struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
u32 val;
@@ -287,6 +287,8 @@ static void __init ks_pcie_host_init(struct pcie_port *pp)
 */
hook_fault_code(17, keystone_pcie_fault, SIGBUS, 0,
"Asynchronous external abort");
+
+   return 0;
 }
 
 static struct