bus: brcmstb_gisb: add support for GISBv7 arbiter

PATCH Superseded Doug Berger 2 participants 16 messages Mar 28, 2017 – Mar 29, 2017

Subsystems (3)

Device tree bindings arm64 Broadcom BCM7XXX Arm architecture

lore

Share & tools

lore

Doug Berger

author

2017-03-28 21:34 UTC #0 lore raw

v2 cover bus: brcmstb_gisb: add support for GISBv7 arbiter

This patch set contains changes to enable the GISB arbiter driver on the latest ARM64 architecture Set-Top Box chips from Broadcom.

This driver relies on being able to hook the abort handlers of the processor core that are triggered by bus error signals generated by the GISB bus arbiter hardware found in BCM7XXX chips. The first two patches are based on the arm64/for-next/core branch to enable this functionality for the arm64 architecture.

The remaining patches correct some issues with the existing driver, add the ARM64 architecture specific support to the driver, and finally add the new register map for the GISBv7 hardware first appearing in the BCM7278 device.

Changes since v1 at [1]:
- Removed code associated with hooking SError handling in favor of a registered notifier (Thanks Mark!)
- Removed an unnecessary explicit cast (Thanks Gregory!)

[1] https://lkml.org/lkml/2017/3/24/413

Doug Berger (6):
arm64: mm: mark fault_info __ro_after_init
bus: brcmstb_gisb: Use register offsets with writes too
bus: brcmstb_gisb: Correct hooking of ARM aborts
bus: brcmstb_gisb: correct support for 64-bit address output
bus: brcmstb_gisb: add notifier handling
bus: brcmstb_gisb: update to support new revision

Florian Fainelli (2):
arm64: mm: Allow installation of memory abort handlers
bus: brcmstb_gisb: Add ARM64 support

 .../devicetree/bindings/bus/brcm,gisb-arb.txt      |   3 +-
 arch/arm64/include/asm/system_misc.h               |   3 +
 arch/arm64/mm/fault.c                              |  17 +++-
 drivers/bus/Kconfig                                |   2 +-
 drivers/bus/brcmstb_gisb.c                         | 111 ++++++++++++++++-----
 5 files changed, 106 insertions(+), 30 deletions(-)

Signature · 1 line

2.12.0

Doug Berger

author

2017-03-28 21:34 UTC #1 lore raw

PATCH v2 1/8 arm64: mm: Allow installation of memory abort handlers

From: Florian Fainelli <[email protected]>

Similarly to what the ARM/Linux kernel provides, add a hook_fault_code() function which allows drivers or other parts of the kernel to install custom memory abort handlers. This is useful when a given SoC's busing does not propagate the exact faulting physical address, but there is a way to read it through e.g: a special arbiter driver.

Signed-off-by Florian Fainelli <[email protected]>

---
 arch/arm64/include/asm/system_misc.h |  3 +++
 arch/arm64/mm/fault.c                | 15 ++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

arch/arm64/include/asm/system_misc.h +3 -0

index bc812435bc76..e05f5b8c7c1c 100644
@@ -38,6 +38,9 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
 void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
 					     struct pt_regs *),
 			   int sig, int code, const char *name);
+void hook_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
+				       struct pt_regs *),
+		     int sig, int code, const char *name);
 
 struct mm_struct;
 extern void show_pte(struct mm_struct *mm, unsigned long addr);

arch/arm64/mm/fault.c +14 -1

index 4bf899fb451b..cdf1260f1005 100644
@@ -488,7 +488,7 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
 	return 1;
 }
 
-static const struct fault_info {
+static struct fault_info {
 	int	(*fn)(unsigned long addr, unsigned int esr, struct pt_regs *regs);
 	int	sig;
 	int	code;

@@ -560,6 +560,19 @@ static const struct fault_info {
 	{ do_bad,		SIGBUS,  0,		"unknown 63"			},
 };
 
+void __init hook_fault_code(int nr,
+			    int (*fn)(unsigned long, unsigned int, struct pt_regs *),
+			    int sig, int code, const char *name)
+{
+	BUG_ON(nr < 0 || nr >= ARRAY_SIZE(fault_info));
+
+	fault_info[nr].fn	= fn;
+	fault_info[nr].sig	= sig;
+	fault_info[nr].code	= code;
+	fault_info[nr].name	= name;
+}
+
+
 static const char *fault_name(unsigned int esr)
 {
 	const struct fault_info *inf = fault_info + (esr & 63);

Signature · 1 line

2.12.0

Mark Rutland

reviewer

2017-03-29 11:32 UTC #2 lore raw

Re: [PATCH v2 1/8] arm64: mm: Allow installation of memory abort handlers

Hi,

Quoted 9 lines · Doug Berger

From: Florian Fainelli <[email protected]>
Similarly to what the ARM/Linux kernel provides, add a hook_fault_code() function which allows drivers or other parts of the kernel to install custom memory abort handlers. This is useful when a given SoC's busing does not propagate the exact faulting physical address, but there is a way to read it through e.g: a special arbiter driver.
Signed-off-by Florian Fainelli <[email protected]>

Personally, I do not think that it makes sense to allow arbitrary code to hook such low-level fault handling.

IMO, if it is truly necessary to allow drivers to handle particular faults, that should be driven by data associated with the relevant mapping (e.g. the VMA), rather than allowing code to hook *all* faults.

Quoted 1 line

From my PoV, NAK to this interface to take over low-level fault

handling.

Catalin and Will have the final say here, as the arm64 maintainers.

Thanks,
Mark.

Quoted 55 lines

---
 arch/arm64/include/asm/system_misc.h |  3 +++
 arch/arm64/mm/fault.c                | 15 ++++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

arch/arm64/include/asm/system_misc.h +3 -0

index bc812435bc76..e05f5b8c7c1c 100644
@@ -38,6 +38,9 @@ void arm64_notify_die(const char *str, struct pt_regs *regs,
 void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
 					     struct pt_regs *),
 			   int sig, int code, const char *name);
+void hook_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
+				       struct pt_regs *),
+		     int sig, int code, const char *name);
 
 struct mm_struct;
 extern void show_pte(struct mm_struct *mm, unsigned long addr);

arch/arm64/mm/fault.c +14 -1

index 4bf899fb451b..cdf1260f1005 100644
@@ -488,7 +488,7 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
 	return 1;
 }
 
-static const struct fault_info {
+static struct fault_info {
 	int	(*fn)(unsigned long addr, unsigned int esr, struct pt_regs *regs);
 	int	sig;
 	int	code;

@@ -560,6 +560,19 @@ static const struct fault_info {
 	{ do_bad,		SIGBUS,  0,		"unknown 63"			},
 };
 
+void __init hook_fault_code(int nr,
+			    int (*fn)(unsigned long, unsigned int, struct pt_regs *),
+			    int sig, int code, const char *name)
+{
+	BUG_ON(nr < 0 || nr >= ARRAY_SIZE(fault_info));
+
+	fault_info[nr].fn	= fn;
+	fault_info[nr].sig	= sig;
+	fault_info[nr].code	= code;
+	fault_info[nr].name	= name;
+}
+
+
 static const char *fault_name(unsigned int esr)
 {
 	const struct fault_info *inf = fault_info + (esr & 63);

Signature · 1 line

2.12.0

Doug Berger

author

2017-03-28 21:34 UTC #3 lore raw

PATCH v2 2/8 arm64: mm: mark fault_info __ro_after_init

In reply to Doug Berger

The fault_info table must be made writeable to allow installation of custom memory abort handlers, but it can be made read-only after initialization to provide some protection.

Signed-off-by Doug Berger <[email protected]>

---
 arch/arm64/mm/fault.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/mm/fault.c +1 -1

index cdf1260f1005..43319ed58a47 100644
@@ -493,7 +493,7 @@ static struct fault_info {
 	int	sig;
 	int	code;
 	const char *name;
-} fault_info[] = {
+} fault_info[] __ro_after_init = {
 	{ do_bad,		SIGBUS,  0,		"ttbr address size fault"	},
 	{ do_bad,		SIGBUS,  0,		"level 1 address size fault"	},
 	{ do_bad,		SIGBUS,  0,		"level 2 address size fault"	},

Signature · 1 line

2.12.0

Mark Rutland

reviewer

2017-03-29 11:23 UTC #4 lore raw

Re: [PATCH v2 2/8] arm64: mm: mark fault_info __ro_after_init

Quoted 5 lines · Doug Berger

The fault_info table must be made writeable to allow installation of custom memory abort handlers, but it can be made read-only after initialization to provide some protection.
Signed-off-by Doug Berger <[email protected]>

Thanks for putting this together. I agree that making this RO is good.

However, I also think that we should not allow arbitrary hooking of fault codes, and thus this can be const.

Thanks,
Mark.

Quoted 20 lines

---
 arch/arm64/mm/fault.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/mm/fault.c +1 -1

index cdf1260f1005..43319ed58a47 100644
@@ -493,7 +493,7 @@ static struct fault_info {
 	int	sig;
 	int	code;
 	const char *name;
-} fault_info[] = {
+} fault_info[] __ro_after_init = {
 	{ do_bad,		SIGBUS,  0,		"ttbr address size fault"	},
 	{ do_bad,		SIGBUS,  0,		"level 1 address size fault"	},
 	{ do_bad,		SIGBUS,  0,		"level 2 address size fault"	},

Signature · 1 line

2.12.0

Doug Berger

author

2017-03-28 21:34 UTC #5 lore raw

PATCH v2 3/8 bus: brcmstb_gisb: Use register offsets with writes too

In reply to Doug Berger

This commit corrects the bug introduced in commit f80835875d3d ("bus: brcmstb_gisb: Look up register offsets in a table") such that gisb_write() translates the register enumeration into an offset from the base address for writes as well as reads.

Fixes f80835875d3d ("bus: brcmstb_gisb: Look up register offsets in a table")

Signed-off-by Doug Berger <[email protected]>

Acked-by Gregory Fong <[email protected]>

---
 drivers/bus/brcmstb_gisb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/bus/brcmstb_gisb.c +3 -3

index 72fe0a5a8bf3..a94598d0945a 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2014 Broadcom Corporation
+ * Copyright (C) 2014-2017 Broadcom
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as

@@ -127,9 +127,9 @@ static void gisb_write(struct brcmstb_gisb_arb_device *gdev, u32 val, int reg)
 		return;
 
 	if (gdev->big_endian)
-		iowrite32be(val, gdev->base + reg);
+		iowrite32be(val, gdev->base + offset);
 	else
-		iowrite32(val, gdev->base + reg);
+		iowrite32(val, gdev->base + offset);
 }
 
 static ssize_t gisb_arb_get_timeout(struct device *dev,

Signature · 1 line

2.12.0

Doug Berger

author

2017-03-28 21:34 UTC #6 lore raw

PATCH v2 4/8 bus: brcmstb_gisb: Correct hooking of ARM aborts

In reply to Doug Berger

The fault status reporting in the FSR registers is different depending on whether the Long Physical Address Extension (LPAE) is being used.

This commit corrects the registerring of fault handlers for arm architecture kernels when the LPAE is enabled. It also forces the handler to report that the abort exception was unhandled so that the appropriate signal is sent to the offending user process.

Signed-off-by Doug Berger <[email protected]>

---
 drivers/bus/brcmstb_gisb.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

drivers/bus/brcmstb_gisb.c +14 -5

index a94598d0945a..9eba0143f1a4 100644
@@ -225,27 +225,29 @@ static int brcmstb_gisb_arb_decode_addr(struct brcmstb_gisb_arb_device *gdev,
 static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
 				     struct pt_regs *regs)
 {
-	int ret = 0;
 	struct brcmstb_gisb_arb_device *gdev;
 
 	/* iterate over each GISB arb registered handlers */
 	list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
-		ret |= brcmstb_gisb_arb_decode_addr(gdev, "bus error");
+		brcmstb_gisb_arb_decode_addr(gdev, "bus error");
+
+#if !defined(CONFIG_ARM_LPAE)
 	/*
 	 * If it was an imprecise abort, then we need to correct the
 	 * return address to be _after_ the instruction.
 	*/
 	if (fsr & (1 << 10))
 		regs->ARM_pc += 4;
+#endif
 
-	return ret;
+	/* Always report unhandled exception */
+	return 1;
 }
 #endif
 
 #ifdef CONFIG_MIPS
 static int brcmstb_bus_error_handler(struct pt_regs *regs, int is_fixup)
 {
-	int ret = 0;
 	struct brcmstb_gisb_arb_device *gdev;
 	u32 cap_status;

@@ -258,7 +260,7 @@ static int brcmstb_bus_error_handler(struct pt_regs *regs, int is_fixup)
 			goto out;
 		}
 
-		ret |= brcmstb_gisb_arb_decode_addr(gdev, "bus error");
+		brcmstb_gisb_arb_decode_addr(gdev, "bus error");
 	}
 out:
 	return is_fixup ? MIPS_BE_FIXUP : MIPS_BE_FATAL;

@@ -379,9 +381,16 @@ static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev)
 	list_add_tail(&gdev->next, &brcmstb_gisb_arb_device_list);
 
 #ifdef CONFIG_ARM
+#ifdef CONFIG_ARM_LPAE
+	hook_fault_code(16, brcmstb_bus_error_handler, SIGBUS, 0,
+			"synchronous external abort");
+	hook_fault_code(17, brcmstb_bus_error_handler, SIGBUS, 0,
+			"asynchronous external abort");
+#else
 	hook_fault_code(22, brcmstb_bus_error_handler, SIGBUS, 0,
 			"imprecise external abort");
 #endif
+#endif /* CONFIG_ARM */
 #ifdef CONFIG_MIPS
 	board_be_handler = brcmstb_bus_error_handler;
 #endif

Signature · 1 line

2.12.0

Doug Berger

author

2017-03-28 21:34 UTC #7 lore raw

PATCH v2 5/8 bus: brcmstb_gisb: correct support for 64-bit address output

In reply to Doug Berger

The GISB bus can support addresses beyond 32-bits. So this commit corrects support for reading a captured 64-bit address into a 64-bit variable by obtaining the high bits from the ARB_ERR_CAP_HI_ADDR register (when present) and then outputting the full 64-bit value.

It also removes unused definitions.

Fixes 44127b771d9c ("bus: add Broadcom GISB bus arbiter timeout/error handler")

Signed-off-by Doug Berger <[email protected]>

Acked-by Gregory Fong <[email protected]>

---
 drivers/bus/brcmstb_gisb.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

drivers/bus/brcmstb_gisb.c +20 ~~-16~~

index 9eba0143f1a4..edf79432f899 100644
@@ -37,8 +37,6 @@
 #define  ARB_ERR_CAP_CLEAR		(1 << 0)
 #define  ARB_ERR_CAP_STATUS_TIMEOUT	(1 << 12)
 #define  ARB_ERR_CAP_STATUS_TEA		(1 << 11)
-#define  ARB_ERR_CAP_STATUS_BS_SHIFT	(1 << 2)
-#define  ARB_ERR_CAP_STATUS_BS_MASK	0x3c
 #define  ARB_ERR_CAP_STATUS_WRITE	(1 << 1)
 #define  ARB_ERR_CAP_STATUS_VALID	(1 << 0)

@@ -47,7 +45,6 @@ enum {
 	ARB_ERR_CAP_CLR,
 	ARB_ERR_CAP_HI_ADDR,
 	ARB_ERR_CAP_ADDR,
-	ARB_ERR_CAP_DATA,
 	ARB_ERR_CAP_STATUS,
 	ARB_ERR_CAP_MASTER,
 };

@@ -57,7 +54,6 @@ static const int gisb_offsets_bcm7038[] = {
 	[ARB_ERR_CAP_CLR]	= 0x0c4,
 	[ARB_ERR_CAP_HI_ADDR]	= -1,
 	[ARB_ERR_CAP_ADDR]	= 0x0c8,
-	[ARB_ERR_CAP_DATA]	= 0x0cc,
 	[ARB_ERR_CAP_STATUS]	= 0x0d0,
 	[ARB_ERR_CAP_MASTER]	= -1,
 };

@@ -67,7 +63,6 @@ static const int gisb_offsets_bcm7400[] = {
 	[ARB_ERR_CAP_CLR]	= 0x0c8,
 	[ARB_ERR_CAP_HI_ADDR]	= -1,
 	[ARB_ERR_CAP_ADDR]	= 0x0cc,
-	[ARB_ERR_CAP_DATA]	= 0x0d0,
 	[ARB_ERR_CAP_STATUS]	= 0x0d4,
 	[ARB_ERR_CAP_MASTER]	= 0x0d8,
 };

@@ -77,7 +72,6 @@ static const int gisb_offsets_bcm7435[] = {
 	[ARB_ERR_CAP_CLR]	= 0x168,
 	[ARB_ERR_CAP_HI_ADDR]	= -1,
 	[ARB_ERR_CAP_ADDR]	= 0x16c,
-	[ARB_ERR_CAP_DATA]	= 0x170,
 	[ARB_ERR_CAP_STATUS]	= 0x174,
 	[ARB_ERR_CAP_MASTER]	= 0x178,
 };

@@ -87,7 +81,6 @@ static const int gisb_offsets_bcm7445[] = {
 	[ARB_ERR_CAP_CLR]	= 0x7e4,
 	[ARB_ERR_CAP_HI_ADDR]	= 0x7e8,
 	[ARB_ERR_CAP_ADDR]	= 0x7ec,
-	[ARB_ERR_CAP_DATA]	= 0x7f0,
 	[ARB_ERR_CAP_STATUS]	= 0x7f4,
 	[ARB_ERR_CAP_MASTER]	= 0x7f8,
 };

@@ -109,9 +102,13 @@ static u32 gisb_read(struct brcmstb_gisb_arb_device *gdev, int reg)
 {
 	int offset = gdev->gisb_offsets[reg];
 
-	/* return 1 if the hardware doesn't have ARB_ERR_CAP_MASTER */
-	if (offset == -1)
-		return 1;
+	if (offset < 0) {
+		/* return 1 if the hardware doesn't have ARB_ERR_CAP_MASTER */
+		if (reg == ARB_ERR_CAP_MASTER)
+			return 1;
+		else
+			return 0;
+	}
 
 	if (gdev->big_endian)
 		return ioread32be(gdev->base + offset);

@@ -119,6 +116,16 @@ static u32 gisb_read(struct brcmstb_gisb_arb_device *gdev, int reg)
 		return ioread32(gdev->base + offset);
 }
 
+static u64 gisb_read_address(struct brcmstb_gisb_arb_device *gdev)
+{
+	u64 value;
+
+	value = gisb_read(gdev, ARB_ERR_CAP_ADDR);
+	value |= (u64)gisb_read(gdev, ARB_ERR_CAP_HI_ADDR) << 32;
+
+	return value;
+}
+
 static void gisb_write(struct brcmstb_gisb_arb_device *gdev, u32 val, int reg)
 {
 	int offset = gdev->gisb_offsets[reg];

@@ -185,7 +192,7 @@ static int brcmstb_gisb_arb_decode_addr(struct brcmstb_gisb_arb_device *gdev,
 					const char *reason)
 {
 	u32 cap_status;
-	unsigned long arb_addr;
+	u64 arb_addr;
 	u32 master;
 	const char *m_name;
 	char m_fmt[11];

@@ -197,10 +204,7 @@ static int brcmstb_gisb_arb_decode_addr(struct brcmstb_gisb_arb_device *gdev,
 		return 1;
 
 	/* Read the address and master */
-	arb_addr = gisb_read(gdev, ARB_ERR_CAP_ADDR) & 0xffffffff;
-#if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT))
-	arb_addr |= (u64)gisb_read(gdev, ARB_ERR_CAP_HI_ADDR) << 32;
-#endif
+	arb_addr = gisb_read_address(gdev);
 	master = gisb_read(gdev, ARB_ERR_CAP_MASTER);
 
 	m_name = brcmstb_gisb_master_to_str(gdev, master);

@@ -209,7 +213,7 @@ static int brcmstb_gisb_arb_decode_addr(struct brcmstb_gisb_arb_device *gdev,
 		m_name = m_fmt;
 	}
 
-	pr_crit("%s: %s at 0x%lx [%c %s], core: %s\n",
+	pr_crit("%s: %s@0x%llx [%c %s], core: %s\n",
 		__func__, reason, arb_addr,
 		cap_status & ARB_ERR_CAP_STATUS_WRITE ? 'W' : 'R',
 		cap_status & ARB_ERR_CAP_STATUS_TIMEOUT ? "timeout" : "",

Signature · 1 line

2.12.0

Doug Berger

author

2017-03-28 21:34 UTC #8 lore raw

PATCH v2 6/8 bus: brcmstb_gisb: Add ARM64 support

In reply to Doug Berger

From: Florian Fainelli <[email protected]>

Hook to the ARM64 data abort exception #16: synchronous external abort, which is how the GISB errors will be funneled back to the ARM64 CPU in case of problems

Signed-off-by Florian Fainelli <[email protected]>

---
 drivers/bus/Kconfig        |  2 +-
 drivers/bus/brcmstb_gisb.c | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 4 deletions(-)

drivers/bus/Kconfig +1 -1

index 0a52da439abf..d2a5f1184022 100644
@@ -57,7 +57,7 @@ config ARM_CCN
 
 config BRCMSTB_GISB_ARB
 	bool "Broadcom STB GISB bus arbiter"
-	depends on ARM || MIPS
+	depends on ARM || ARM64 || MIPS
 	default ARCH_BRCMSTB || BMIPS_GENERIC
 	help
 	  Driver for the Broadcom Set Top Box System-on-a-chip internal bus

drivers/bus/brcmstb_gisb.c +12 -3

index edf79432f899..500b6bb5c739 100644
@@ -30,6 +30,11 @@
 #include <asm/signal.h>
 #endif
 
+#ifdef CONFIG_ARM64
+#include <asm/signal.h>
+#include <asm/system_misc.h>
+#endif
+
 #ifdef CONFIG_MIPS
 #include <asm/traps.h>
 #endif

@@ -225,7 +230,7 @@ static int brcmstb_gisb_arb_decode_addr(struct brcmstb_gisb_arb_device *gdev,
 	return 0;
 }
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
 static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
 				     struct pt_regs *regs)
 {

@@ -235,7 +240,7 @@ static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
 	list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
 		brcmstb_gisb_arb_decode_addr(gdev, "bus error");
 
-#if !defined(CONFIG_ARM_LPAE)
+#if defined(CONFIG_ARM) && !defined(CONFIG_ARM_LPAE)
 	/*
 	 * If it was an imprecise abort, then we need to correct the
 	 * return address to be _after_ the instruction.

@@ -247,7 +252,7 @@ static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
 	/* Always report unhandled exception */
 	return 1;
 }
-#endif
+#endif /* CONFIG_ARM || CONFIG_ARM64 */
 
 #ifdef CONFIG_MIPS
 static int brcmstb_bus_error_handler(struct pt_regs *regs, int is_fixup)

@@ -395,6 +400,10 @@ static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev)
 			"imprecise external abort");
 #endif
 #endif /* CONFIG_ARM */
+#ifdef CONFIG_ARM64
+	hook_fault_code(16, brcmstb_bus_error_handler, SIGBUS, 0,
+			"synchronous external abort");
+#endif
 #ifdef CONFIG_MIPS
 	board_be_handler = brcmstb_bus_error_handler;
 #endif

Signature · 1 line

2.12.0

Mark Rutland

reviewer

2017-03-29 11:20 UTC #9 lore raw

Re: [PATCH v2 6/8] bus: brcmstb_gisb: Add ARM64 support

Hi,

Quoted 5 lines · Doug Berger

From: Florian Fainelli <[email protected]>
Hook to the ARM64 data abort exception #16: synchronous external abort, which is how the GISB errors will be funneled back to the ARM64 CPU in case of problems

I believe that you can use a die notifier for this, and that you don't need to hook the low-level architectural fault here.

I note that the code doesn't even look at the faulting address, and likely doesn't have the information to do anything with it, so it make no sense to me for this code to hook the low-level fault.

Further, as an aside, from digging into how we handle unexpected faults it appears that we don't always treat some faults (e.g. TLB conflict, faults on PTW) sufficiently fatally when they occur at EL0, so we likely need to do so rework there.

Thanks,
Mark.

Quoted 77 lines

Signed-off-by Florian Fainelli <[email protected]>

---
 drivers/bus/Kconfig        |  2 +-
 drivers/bus/brcmstb_gisb.c | 15 ++++++++++++---
 2 files changed, 13 insertions(+), 4 deletions(-)

drivers/bus/Kconfig +1 -1

index 0a52da439abf..d2a5f1184022 100644
@@ -57,7 +57,7 @@ config ARM_CCN
 
 config BRCMSTB_GISB_ARB
 	bool "Broadcom STB GISB bus arbiter"
-	depends on ARM || MIPS
+	depends on ARM || ARM64 || MIPS
 	default ARCH_BRCMSTB || BMIPS_GENERIC
 	help
 	  Driver for the Broadcom Set Top Box System-on-a-chip internal bus

drivers/bus/brcmstb_gisb.c +12 -3

index edf79432f899..500b6bb5c739 100644
@@ -30,6 +30,11 @@
 #include <asm/signal.h>
 #endif
 
+#ifdef CONFIG_ARM64
+#include <asm/signal.h>
+#include <asm/system_misc.h>
+#endif
+
 #ifdef CONFIG_MIPS
 #include <asm/traps.h>
 #endif

@@ -225,7 +230,7 @@ static int brcmstb_gisb_arb_decode_addr(struct brcmstb_gisb_arb_device *gdev,
 	return 0;
 }
 
-#ifdef CONFIG_ARM
+#if defined(CONFIG_ARM) || defined(CONFIG_ARM64)
 static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
 				     struct pt_regs *regs)
 {

@@ -235,7 +240,7 @@ static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
 	list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
 		brcmstb_gisb_arb_decode_addr(gdev, "bus error");
 
-#if !defined(CONFIG_ARM_LPAE)
+#if defined(CONFIG_ARM) && !defined(CONFIG_ARM_LPAE)
 	/*
 	 * If it was an imprecise abort, then we need to correct the
 	 * return address to be _after_ the instruction.

@@ -247,7 +252,7 @@ static int brcmstb_bus_error_handler(unsigned long addr, unsigned int fsr,
 	/* Always report unhandled exception */
 	return 1;
 }
-#endif
+#endif /* CONFIG_ARM || CONFIG_ARM64 */
 
 #ifdef CONFIG_MIPS
 static int brcmstb_bus_error_handler(struct pt_regs *regs, int is_fixup)

@@ -395,6 +400,10 @@ static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev)
 			"imprecise external abort");
 #endif
 #endif /* CONFIG_ARM */
+#ifdef CONFIG_ARM64
+	hook_fault_code(16, brcmstb_bus_error_handler, SIGBUS, 0,
+			"synchronous external abort");
+#endif
 #ifdef CONFIG_MIPS
 	board_be_handler = brcmstb_bus_error_handler;
 #endif

Signature · 1 line

2.12.0

Doug Berger

author

2017-03-28 21:34 UTC #10 lore raw

PATCH v2 7/8 bus: brcmstb_gisb: add notifier handling

In reply to Doug Berger

Check for GISB arbitration errors through a chained notifier when a process dies or a kernel panic occurs. This allows a meaningful diagnostic message to occur along with other diagnostic information.

Notably writes to a bad GISB address on an ARM64 architecture kernel cause unrecoverable SError aborts and this allows the cause of the abort to be seen.

Signed-off-by Doug Berger <[email protected]>

---
 drivers/bus/brcmstb_gisb.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

drivers/bus/brcmstb_gisb.c +27 -0

index 500b6bb5c739..774729002b8c 100644
@@ -24,6 +24,8 @@
 #include <linux/of.h>
 #include <linux/bitops.h>
 #include <linux/pm.h>
+#include <linux/kernel.h>
+#include <linux/kdebug.h>
 
 #ifdef CONFIG_ARM
 #include <asm/bug.h>

@@ -290,6 +292,25 @@ static irqreturn_t brcmstb_gisb_tea_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/*
+ * Dump out gisb errors on die or panic.
+ */
+static int dump_gisb_error(struct notifier_block *self, unsigned long v,
+			   void *p)
+{
+	struct brcmstb_gisb_arb_device *gdev;
+
+	/* iterate over each GISB arb registered handlers */
+	list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
+		brcmstb_gisb_arb_decode_addr(gdev, "async abort");
+
+	return 0;
+}
+
+static struct notifier_block gisb_error_notifier = {
+	.notifier_call = dump_gisb_error,
+};
+
 static DEVICE_ATTR(gisb_arb_timeout, S_IWUSR | S_IRUGO,
 		gisb_arb_get_timeout, gisb_arb_set_timeout);

@@ -408,6 +429,12 @@ static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev)
 	board_be_handler = brcmstb_bus_error_handler;
 #endif
 
+	if (list_is_singular(&brcmstb_gisb_arb_device_list)) {
+		register_die_notifier(&gisb_error_notifier);
+		atomic_notifier_chain_register(&panic_notifier_list,
+					       &gisb_error_notifier);
+	}
+
 	dev_info(&pdev->dev, "registered mem: %p, irqs: %d, %d\n",
 			gdev->base, timeout_irq, tea_irq);

Signature · 1 line

2.12.0

Mark Rutland

reviewer

2017-03-29 10:13 UTC #11 lore raw

Re: [PATCH v2 7/8] bus: brcmstb_gisb: add notifier handling

Hi Doug,

Quoted 10 lines · Doug Berger

Check for GISB arbitration errors through a chained notifier when a process dies or a kernel panic occurs. This allows a meaningful diagnostic message to occur along with other diagnostic information.
Notably writes to a bad GISB address on an ARM64 architecture kernel cause unrecoverable SError aborts and this allows the cause of the abort to be seen.
Signed-off-by Doug Berger <[email protected]>

Thanks for changing this.

Quoted 24 lines

---
 drivers/bus/brcmstb_gisb.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

drivers/bus/brcmstb_gisb.c +5 -0

index 500b6bb5c739..774729002b8c 100644
@@ -24,6 +24,8 @@
 #include <linux/of.h>
 #include <linux/bitops.h>
 #include <linux/pm.h>
+#include <linux/kernel.h>
+#include <linux/kdebug.h>
 
 #ifdef CONFIG_ARM
 #include <asm/bug.h>

@@ -290,6 +292,25 @@ static irqreturn_t brcmstb_gisb_tea_handler(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+/*
+ * Dump out gisb errors on die or panic.
+ */

drivers/bus/brcmstb_gisb.c static irqreturn_t brcmstb_gisb_tea_handler(int irq, void *dev_id) · line 307

Quoted 10 lines

drivers/bus/brcmstb_gisb.c +10 -0

+static int dump_gisb_error(struct notifier_block *self, unsigned long v,
+			   void *p)
+{
+	struct brcmstb_gisb_arb_device *gdev;
+
+	/* iterate over each GISB arb registered handlers */
+	list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
+		brcmstb_gisb_arb_decode_addr(gdev, "async abort");
+
+	return 0;

I think this should be NOTIFY_OK.

Quoted 10 lines

drivers/bus/brcmstb_gisb.c +6 -0

+}
+
+static struct notifier_block gisb_error_notifier = {
+	.notifier_call = dump_gisb_error,
+};
+
 static DEVICE_ATTR(gisb_arb_timeout, S_IWUSR | S_IRUGO,
 		gisb_arb_get_timeout, gisb_arb_set_timeout);

drivers/bus/brcmstb_gisb.c static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev) · line 435

Quoted 7 lines

drivers/bus/brcmstb_gisb.c +4 -0

@@ -408,6 +429,12 @@ static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev)
 	board_be_handler = brcmstb_bus_error_handler;
 #endif
 
+	if (list_is_singular(&brcmstb_gisb_arb_device_list)) {
+		register_die_notifier(&gisb_error_notifier);
+		atomic_notifier_chain_register(&panic_notifier_list,
+					       &gisb_error_notifier);

I don't think this is quite right. A notifier_block can only be registered to one notifier chain at a time, and this has the potential to corrupt both chains.

I also think you only need to register the panic notifier. An SError should always result in a panic.

Thanks,
Mark.

Doug Berger

author

2017-03-29 17:39 UTC #12 lore raw

Re: [PATCH v2 7/8] bus: brcmstb_gisb: add notifier handling

On 03/29/2017 03:13 AM, Mark Rutland wrote:

snip

Quoted 7 lines

drivers/bus/brcmstb_gisb.c +7 -0

+/*
+ * Dump out gisb errors on die or panic.
+ */
+static int dump_gisb_error(struct notifier_block *self, unsigned long v,
+			   void *p)
+{
+	struct brcmstb_gisb_arb_device *gdev;

Quoted 9 lines

drivers/bus/brcmstb_gisb.c static irqreturn_t brcmstb_gisb_tea_handler(int irq, void *dev_id) · line 307
Quoted 6 lines
drivers/bus/brcmstb_gisb.c +6 -0
+
+	/* iterate over each GISB arb registered handlers */
+	list_for_each_entry(gdev, &brcmstb_gisb_arb_device_list, next)
+		brcmstb_gisb_arb_decode_addr(gdev, "async abort");
+
+	return 0;
I think this should be NOTIFY_OK.

I used dump_mem_limit() as a template and didn't catch this (work to do...). Upon review I think I would prefer NOTIFY_DONE since this call is opportunistic (i.e. it is taking the opportunity to check whether additional diagnostic data is available to display) and has no interest in affecting the overall handling of the event.

Quoted 13 lines

drivers/bus/brcmstb_gisb.c +6 -0

+}
+
+static struct notifier_block gisb_error_notifier = {
+	.notifier_call = dump_gisb_error,
+};
+
 static DEVICE_ATTR(gisb_arb_timeout, S_IWUSR | S_IRUGO,
 		gisb_arb_get_timeout, gisb_arb_set_timeout);

@@ -408,6 +429,12 @@ static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev)
 	board_be_handler = brcmstb_bus_error_handler;
 #endif

Quoted 9 lines

drivers/bus/brcmstb_gisb.c static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev) · line 435
Quoted 4 lines
drivers/bus/brcmstb_gisb.c +4 -0
+	if (list_is_singular(&brcmstb_gisb_arb_device_list)) {
+		register_die_notifier(&gisb_error_notifier);
+		atomic_notifier_chain_register(&panic_notifier_list,
+					       &gisb_error_notifier);
I don't think this is quite right. A notifier_block can only be registered to one notifier chain at a time, and this has the potential to corrupt both chains.

A VERY good point thanks for pointing this out.

Quoted 3 lines

I also think you only need to register the panic notifier. An SError should always result in a panic.

That was my initial thought as well. However, testing revealed that the bad mode Oops actually exits the user space process and doesn't reach the panic so there was no helpful diagnostic message. This may be in line with your comments about insufficient fatality of failures in PATCH v2 6/8, but it actually is more in line with our desired behavior for the aborted write. Setting the notify on die gave us the result we are looking for, but as noted above I should have created a separate notifier.

I had hoped that the same approach (i.e. die notifier) would remove the need for PATCH v2 6/8 as well, but I found that the Unhandled fault error didn't actually die from user mode.

Quoted 3 lines

Thanks,
Mark.

Thank you for your help with this,
Doug

Mark Rutland

reviewer

2017-03-29 18:17 UTC #13 lore raw

Re: [PATCH v2 7/8] bus: brcmstb_gisb: add notifier handling

Quoted 9 lines · Doug Berger

Quoted 7 lines · Mark Rutland
drivers/bus/brcmstb_gisb.c static irqreturn_t brcmstb_gisb_tea_handler(int irq, void *dev_id)
Quoted 4 lines
drivers/bus/brcmstb_gisb.c +3 -0
+static int dump_gisb_error(struct notifier_block *self, unsigned long v,
+			   void *p)
 
+	return 0;
I think this should be NOTIFY_OK.

Quoted 5 lines

I used dump_mem_limit() as a template and didn't catch this (work to do...). Upon review I think I would prefer NOTIFY_DONE since this call is opportunistic (i.e. it is taking the opportunity to check whether additional diagnostic data is available to display) and has no interest in affecting the overall handling of the event.

That's fine by me.

Does the distinction matter here?

Most notifer users treat NOTIFY_OK and NOTIFY_DONE as equivalent, and notifier_call_chain only terminates when it sees NOTIFY_STOP_MASK.

[...]

Quoted 22 lines

Quoted 9 lines
drivers/bus/brcmstb_gisb.c static int __init brcmstb_gisb_arb_probe(struct platform_device *pdev) · line 435
Quoted 4 lines
drivers/bus/brcmstb_gisb.c +4 -0
+	if (list_is_singular(&brcmstb_gisb_arb_device_list)) {
+		register_die_notifier(&gisb_error_notifier);
+		atomic_notifier_chain_register(&panic_notifier_list,
+					       &gisb_error_notifier);
I don't think this is quite right. A notifier_block can only be registered to one notifier chain at a time, and this has the potential to corrupt both chains.
A VERY good point thanks for pointing this out.
Quoted 3 lines
I also think you only need to register the panic notifier. An SError should always result in a panic.
That was my initial thought as well. However, testing revealed that the bad mode Oops actually exits the user space process and doesn't reach the panic so there was no helpful diagnostic message. This may be in line with your comments about insufficient fatality of failures in PATCH v2 6/8, but it actually is more in line with our desired behavior for the aborted write. Setting the notify on die gave us the result we are looking for, but as noted above I should have created a separate notifier.

Quoted 3 lines

I had hoped that the same approach (i.e. die notifier) would remove the need for PATCH v2 6/8 as well, but I found that the Unhandled fault error didn't actually die from user mode.

In my mind it's a bug that we don't treat those errors more fatally.

I'll try to dig into that.

Thanks,
Mark.

Doug Berger

author

2017-03-28 21:34 UTC #14 lore raw

PATCH v2 8/8 bus: brcmstb_gisb: update to support new revision

In reply to Doug Berger

The 7278 introduces a new version of this core. This commit adds support for that revision.

Signed-off-by Doug Berger <[email protected]>

---
 Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt |  3 ++-
 drivers/bus/brcmstb_gisb.c                              | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt +2 -1

index 1eceefb20f01..8a6c3c2e58fe 100644
@@ -3,7 +3,8 @@ Broadcom GISB bus Arbiter controller
 Required properties:
 
 - compatible:
-    "brcm,gisb-arb" or "brcm,bcm7445-gisb-arb" for 28nm chips
+    "brcm,bcm7278-gisb-arb" for V7 28nm chips
+    "brcm,gisb-arb" or "brcm,bcm7445-gisb-arb" for other 28nm chips
     "brcm,bcm7435-gisb-arb" for newer 40nm chips
     "brcm,bcm7400-gisb-arb" for older 40nm chips and all 65nm chips
     "brcm,bcm7038-gisb-arb" for 130nm chips

drivers/bus/brcmstb_gisb.c +10 -0

index 774729002b8c..6c3b0dda75f2 100644
@@ -65,6 +65,15 @@ static const int gisb_offsets_bcm7038[] = {
 	[ARB_ERR_CAP_MASTER]	= -1,
 };
 
+static const int gisb_offsets_bcm7278[] = {
+	[ARB_TIMER]		= 0x008,
+	[ARB_ERR_CAP_CLR]	= 0x7f8,
+	[ARB_ERR_CAP_HI_ADDR]	= -1,
+	[ARB_ERR_CAP_ADDR]	= 0x7e0,
+	[ARB_ERR_CAP_STATUS]	= 0x7f0,
+	[ARB_ERR_CAP_MASTER]	= 0x7f4,
+};
+
 static const int gisb_offsets_bcm7400[] = {
 	[ARB_TIMER]		= 0x00c,
 	[ARB_ERR_CAP_CLR]	= 0x0c8,

@@ -328,6 +337,7 @@ static const struct of_device_id brcmstb_gisb_arb_of_match[] = {
 	{ .compatible = "brcm,bcm7445-gisb-arb", .data = gisb_offsets_bcm7445 },
 	{ .compatible = "brcm,bcm7435-gisb-arb", .data = gisb_offsets_bcm7435 },
 	{ .compatible = "brcm,bcm7400-gisb-arb", .data = gisb_offsets_bcm7400 },
+	{ .compatible = "brcm,bcm7278-gisb-arb", .data = gisb_offsets_bcm7278 },
 	{ .compatible = "brcm,bcm7038-gisb-arb", .data = gisb_offsets_bcm7038 },
 	{ },
 };

Signature · 1 line

2.12.0

Mark Rutland

reviewer Acked-by on v2 8/8

2017-03-29 11:25 UTC #15 lore raw

Re: [PATCH v2 8/8] bus: brcmstb_gisb: update to support new revision

Quoted 14 lines · Doug Berger

The 7278 introduces a new version of this core. This commit adds support for that revision.
Signed-off-by Doug Berger <[email protected]>
---
 Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt |  3 ++-
 drivers/bus/brcmstb_gisb.c                              | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt +0 -0
index 1eceefb20f01..8a6c3c2e58fe 100644

Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt Broadcom GISB bus Arbiter controller · line 10

Quoted 9 lines

Documentation/devicetree/bindings/bus/brcm,gisb-arb.txt +2 -1

@@ -3,7 +3,8 @@ Broadcom GISB bus Arbiter controller
 Required properties:
 
 - compatible:
-    "brcm,gisb-arb" or "brcm,bcm7445-gisb-arb" for 28nm chips
+    "brcm,bcm7278-gisb-arb" for V7 28nm chips
+    "brcm,gisb-arb" or "brcm,bcm7445-gisb-arb" for other 28nm chips
     "brcm,bcm7435-gisb-arb" for newer 40nm chips
     "brcm,bcm7400-gisb-arb" for older 40nm chips and all 65nm chips
     "brcm,bcm7038-gisb-arb" for 130nm chips

For the binding:

Acked-by Mark Rutland <[email protected]>

Thanks,
Mark.