[01/33] ARM: BCM5301X: Add back handler ignoring external imprecise aborts

Message ID 1491286366-30720-2-git-send-email-amit.pundir@linaro.org
State Superseded
Headers show
Series
  • Stable commits picked up from lede project
Related show

Commit Message

Amit Pundir April 4, 2017, 6:12 a.m.
From: Rafał Miłecki <rafal@milecki.pl>


Since early BCM5301X days we got abort handler that was removed by
commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort
fault handler"). It assumed we need to deal only with pending aborts
left by the bootloader. Unfortunately this isn't true for BCM5301X.

When probing PCI config space (device enumeration) it is expected to
have master aborts on the PCI bus. Most bridges don't forward (or they
allow disabling it) these errors onto the AXI/AMBA bus but not the
Northstar (BCM5301X) one.

iProc PCIe controller on Northstar seems to be some older one, without
a control register for errors forwarding. It means we need to workaround
this at platform level. All newer platforms are not affected by this
issue.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

(cherry picked from commit 09f3510fb70a46c8921f2cf4a90dbcae460a6820)
Signed-off-by: Amit Pundir <amit.pundir@linaro.org>

---
 arch/arm/mach-bcm/bcm_5301x.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

-- 
2.7.4

Comments

Rafał Miłecki April 4, 2017, 9:31 a.m. | #1
Hi Amit,

On 2017-04-04 08:12, Amit Pundir wrote:
> From: Rafał Miłecki <rafal@milecki.pl>

> 

> Since early BCM5301X days we got abort handler that was removed by

> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort

> fault handler"). It assumed we need to deal only with pending aborts

> left by the bootloader. Unfortunately this isn't true for BCM5301X.

> 

> When probing PCI config space (device enumeration) it is expected to

> have master aborts on the PCI bus. Most bridges don't forward (or they

> allow disabling it) these errors onto the AXI/AMBA bus but not the

> Northstar (BCM5301X) one.

> 

> iProc PCIe controller on Northstar seems to be some older one, without

> a control register for errors forwarding. It means we need to 

> workaround

> this at platform level. All newer platforms are not affected by this

> issue.

> 

> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

> (cherry picked from commit 09f3510fb70a46c8921f2cf4a90dbcae460a6820)

> Signed-off-by: Amit Pundir <amit.pundir@linaro.org>


I'm sorry but I'm not familiar with your work and I'm missing some cover
letter explaining what these patches are about.

You seem to be sending this stuff to Greg. Do you wan to have to 
included in
some particular stable branch? Which one? Why? Some of these patches are 
clean
ups, not a really important fixes.
Amit Pundir April 4, 2017, 9:45 a.m. | #2
Hi Rafal,

On 4 April 2017 at 15:01, Rafał Miłecki <rafal@milecki.pl> wrote:
> Hi Amit,

>

> On 2017-04-04 08:12, Amit Pundir wrote:

>>

>> From: Rafał Miłecki <rafal@milecki.pl>

>>

>> Since early BCM5301X days we got abort handler that was removed by

>> commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort

>> fault handler"). It assumed we need to deal only with pending aborts

>> left by the bootloader. Unfortunately this isn't true for BCM5301X.

>>

>> When probing PCI config space (device enumeration) it is expected to

>> have master aborts on the PCI bus. Most bridges don't forward (or they

>> allow disabling it) these errors onto the AXI/AMBA bus but not the

>> Northstar (BCM5301X) one.

>>

>> iProc PCIe controller on Northstar seems to be some older one, without

>> a control register for errors forwarding. It means we need to workaround

>> this at platform level. All newer platforms are not affected by this

>> issue.

>>

>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

>> (cherry picked from commit 09f3510fb70a46c8921f2cf4a90dbcae460a6820)

>> Signed-off-by: Amit Pundir <amit.pundir@linaro.org>

>

>

> I'm sorry but I'm not familiar with your work and I'm missing some cover

> letter explaining what these patches are about.

>

> You seem to be sending this stuff to Greg. Do you wan to have to included in

> some particular stable branch? Which one? Why? Some of these patches are

> clean

> ups, not a really important fixes.


My fault indeed. Apologies for that. I did send cover letter
https://www.spinics.net/lists/stable/msg165892.html but forgot to
update the Subject line of the individual patches in this series. So
they don't explicitly mention that they are targeted for stable-4.9.
Sorry for the confusion.

I cherry-picked these patches from Lede source tree
https://github.com/lede-project/source and they seemed reasonable
enough for stable-4.9. But I'll take your word if they are indeed
stable material or not. Thanks.

Regards,
Amit Pundir
Greg Kroah-Hartman April 5, 2017, 10:12 a.m. | #3
On Tue, Apr 04, 2017 at 11:31:46AM +0200, Rafał Miłecki wrote:
> Hi Amit,

> 

> On 2017-04-04 08:12, Amit Pundir wrote:

> > From: Rafał Miłecki <rafal@milecki.pl>

> > 

> > Since early BCM5301X days we got abort handler that was removed by

> > commit 937b12306ea79 ("ARM: BCM5301X: remove workaround imprecise abort

> > fault handler"). It assumed we need to deal only with pending aborts

> > left by the bootloader. Unfortunately this isn't true for BCM5301X.

> > 

> > When probing PCI config space (device enumeration) it is expected to

> > have master aborts on the PCI bus. Most bridges don't forward (or they

> > allow disabling it) these errors onto the AXI/AMBA bus but not the

> > Northstar (BCM5301X) one.

> > 

> > iProc PCIe controller on Northstar seems to be some older one, without

> > a control register for errors forwarding. It means we need to workaround

> > this at platform level. All newer platforms are not affected by this

> > issue.

> > 

> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl>

> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

> > (cherry picked from commit 09f3510fb70a46c8921f2cf4a90dbcae460a6820)

> > Signed-off-by: Amit Pundir <amit.pundir@linaro.org>

> 

> I'm sorry but I'm not familiar with your work and I'm missing some cover

> letter explaining what these patches are about.

> 

> You seem to be sending this stuff to Greg. Do you wan to have to included in

> some particular stable branch? Which one? Why? Some of these patches are

> clean

> ups, not a really important fixes.


Are the cleanups needed for the "important fixes" that happen later in
the patch series?  If not, why would they be in the repo that Amit
pulled these from?  Do those developers just blindly backport stuff in?
(might be true, don't know who they are...)

thanks,

greg k-h

Patch

diff --git a/arch/arm/mach-bcm/bcm_5301x.c b/arch/arm/mach-bcm/bcm_5301x.c
index c8830a2..fe067f6 100644
--- a/arch/arm/mach-bcm/bcm_5301x.c
+++ b/arch/arm/mach-bcm/bcm_5301x.c
@@ -9,14 +9,42 @@ 
 #include <asm/hardware/cache-l2x0.h>
 
 #include <asm/mach/arch.h>
+#include <asm/siginfo.h>
+#include <asm/signal.h>
+
+#define FSR_EXTERNAL		(1 << 12)
+#define FSR_READ		(0 << 10)
+#define FSR_IMPRECISE		0x0406
 
 static const char *const bcm5301x_dt_compat[] __initconst = {
 	"brcm,bcm4708",
 	NULL,
 };
 
+static int bcm5301x_abort_handler(unsigned long addr, unsigned int fsr,
+				  struct pt_regs *regs)
+{
+	/*
+	 * We want to ignore aborts forwarded from the PCIe bus that are
+	 * expected and shouldn't really be passed by the PCIe controller.
+	 * The biggest disadvantage is the same FSR code may be reported when
+	 * reading non-existing APB register and we shouldn't ignore that.
+	 */
+	if (fsr == (FSR_EXTERNAL | FSR_READ | FSR_IMPRECISE))
+		return 0;
+
+	return 1;
+}
+
+static void __init bcm5301x_init_early(void)
+{
+	hook_fault_code(16 + 6, bcm5301x_abort_handler, SIGBUS, BUS_OBJERR,
+			"imprecise external abort");
+}
+
 DT_MACHINE_START(BCM5301X, "BCM5301X")
 	.l2c_aux_val	= 0,
 	.l2c_aux_mask	= ~0,
 	.dt_compat	= bcm5301x_dt_compat,
+	.init_early	= bcm5301x_init_early,
 MACHINE_END