ARM: BCM5301X: Add back handler ignoring external imprecise aborts

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>
---
 arch/arm/mach-bcm/bcm_5301x.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Hi Rafal,


On 16-10-29 04:12 AM, Rafał Miłecki 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.
Should we only add this workaround code if CONFIG_PCI is on then?

>
> 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>
> ---
>  arch/arm/mach-bcm/bcm_5301x.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> 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
>

Regards,
Scott

On 10/31/2016 07:08 PM, Scott Branden wrote:
> Hi Rafal,
> 
> 
> On 16-10-29 04:12 AM, Rafał Miłecki 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.
> Should we only add this workaround code if CONFIG_PCI is on then?

I think all the supported northstar devices have a PCIe controller. We
could add such a CONFIG_PCI check, but I do not see a big advantage.

>> 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>
>> ---
>>  arch/arm/mach-bcm/bcm_5301x.c | 28 ++++++++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>
>> 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;

How often does this happen? Would it be useful to add a log message here?

>> +
>> +    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
>>
> 
> Regards,
> Scott

On 10/31/2016 01:59 PM, Hauke Mehrtens wrote:
> 
> 
> On 10/31/2016 07:08 PM, Scott Branden wrote:
>> Hi Rafal,
>>
>>
>> On 16-10-29 04:12 AM, Rafał Miłecki 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.
>> Should we only add this workaround code if CONFIG_PCI is on then?
> 
> I think all the supported northstar devices have a PCIe controller. We
> could add such a CONFIG_PCI check, but I do not see a big advantage.

Actually, I do see a couple disadvantages if we gate this with
CONFIG_PCI: if this problem shows up irrespective of your kernel
configuration, you want the error handler to clear it, not rely on
CONFIG_PCI to be enabled for the error to go away and also, without an
additional ifdef, additional compiler coverage.

On 16-10-31 02:01 PM, Florian Fainelli wrote:
> On 10/31/2016 01:59 PM, Hauke Mehrtens wrote:
>>
>>
>> On 10/31/2016 07:08 PM, Scott Branden wrote:
>>> Hi Rafal,
>>>
>>>
>>> On 16-10-29 04:12 AM, Rafał Miłecki 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.
>>> Should we only add this workaround code if CONFIG_PCI is on then?
>>
>> I think all the supported northstar devices have a PCIe controller. We
>> could add such a CONFIG_PCI check, but I do not see a big advantage.
>
> Actually, I do see a couple disadvantages if we gate this with
> CONFIG_PCI: if this problem shows up irrespective of your kernel
> configuration, you want the error handler to clear it, not rely on
> CONFIG_PCI to be enabled for the error to go away and also, without an
> additional ifdef, additional compiler coverage.
>
A problem with reintroducing this change is that all imprecise data 
aborts are ignored, not just PCI.  So if you don't actually use PCI in 
your system and want to debug other aborts you are unable to.  I don't 
know if we care about such situation.

On 10/31/2016 02:46 PM, Scott Branden wrote:
> 
> 
> On 16-10-31 02:01 PM, Florian Fainelli wrote:
>> On 10/31/2016 01:59 PM, Hauke Mehrtens wrote:
>>>
>>>
>>> On 10/31/2016 07:08 PM, Scott Branden wrote:
>>>> Hi Rafal,
>>>>
>>>>
>>>> On 16-10-29 04:12 AM, Rafał Miłecki 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.
>>>> Should we only add this workaround code if CONFIG_PCI is on then?
>>>
>>> I think all the supported northstar devices have a PCIe controller. We
>>> could add such a CONFIG_PCI check, but I do not see a big advantage.
>>
>> Actually, I do see a couple disadvantages if we gate this with
>> CONFIG_PCI: if this problem shows up irrespective of your kernel
>> configuration, you want the error handler to clear it, not rely on
>> CONFIG_PCI to be enabled for the error to go away and also, without an
>> additional ifdef, additional compiler coverage.
>>
> A problem with reintroducing this change is that all imprecise data
> aborts are ignored, not just PCI.  So if you don't actually use PCI in
> your system and want to debug other aborts you are unable to.  I don't
> know if we care about such situation.

Considering that any abort is pretty much fatal, the options are:

- update the freaking bootloader to a version where there are no such
aborts generated, not an option on consumer devices, unclear which
version (if any) fixes that

- fixups the aborts externally, via a boot wrapper, which is going to
take some time to develop, causes additional burden on the distributors
to provide instructions/build images on how to do it

- fixups the aborts in the kernel, irrespective of where they come from,
simple and easy

- fixups the aborts in the kernel, look where they come from, by using
some bit of magic, looking at PCIe registers and whatnot (provided that
is even possible), not too hard, but can take a while, and is error prone

I can certainly advocate that option 3 is the one that gives a working
device, and this is what matters from a distribution perspective like
LEDE/OpenWrt.

On 31 October 2016 at 22:56, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 10/31/2016 02:46 PM, Scott Branden wrote:
>> A problem with reintroducing this change is that all imprecise data
>> aborts are ignored, not just PCI.  So if you don't actually use PCI in
>> your system and want to debug other aborts you are unable to.  I don't
>> know if we care about such situation.
>
> Considering that any abort is pretty much fatal, the options are:
>
> - update the freaking bootloader to a version where there are no such
> aborts generated, not an option on consumer devices, unclear which
> version (if any) fixes that
>
> - fixups the aborts externally, via a boot wrapper, which is going to
> take some time to develop, causes additional burden on the distributors
> to provide instructions/build images on how to do it

In practice updating bootloader is not possible (as you noticed) but I
don't think it's even possible to fix this problem at bootloader /
extra loader level. If this was a matter of hardware setup, we could
handle it in kernel as well I believe. Ray actually verified it's
controller limitation on Northstar platform.
It sounds a bit like you're thinking about MMU and Dcache Northstar
problem when writing that e-mail.


> - fixups the aborts in the kernel, irrespective of where they come from,
> simple and easy
>
> - fixups the aborts in the kernel, look where they come from, by using
> some bit of magic, looking at PCIe registers and whatnot (provided that
> is even possible), not too hard, but can take a while, and is error prone
>
> I can certainly advocate that option 3 is the one that gives a working
> device, and this is what matters from a distribution perspective like
> LEDE/OpenWrt.

On 16-10-31 02:56 PM, Florian Fainelli wrote:
> On 10/31/2016 02:46 PM, Scott Branden wrote:
>>
>>
>> On 16-10-31 02:01 PM, Florian Fainelli wrote:
>>> On 10/31/2016 01:59 PM, Hauke Mehrtens wrote:
>>>>
>>>>
>>>> On 10/31/2016 07:08 PM, Scott Branden wrote:
>>>>> Hi Rafal,
>>>>>
>>>>>
>>>>> On 16-10-29 04:12 AM, Rafał Miłecki 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.
>>>>> Should we only add this workaround code if CONFIG_PCI is on then?
>>>>
>>>> I think all the supported northstar devices have a PCIe controller. We
>>>> could add such a CONFIG_PCI check, but I do not see a big advantage.
>>>
>>> Actually, I do see a couple disadvantages if we gate this with
>>> CONFIG_PCI: if this problem shows up irrespective of your kernel
>>> configuration, you want the error handler to clear it, not rely on
>>> CONFIG_PCI to be enabled for the error to go away and also, without an
>>> additional ifdef, additional compiler coverage.
>>>
>> A problem with reintroducing this change is that all imprecise data
>> aborts are ignored, not just PCI.  So if you don't actually use PCI in
>> your system and want to debug other aborts you are unable to.  I don't
>> know if we care about such situation.
>
> Considering that any abort is pretty much fatal, the options are:
>
> - update the freaking bootloader to a version where there are no such
> aborts generated, not an option on consumer devices, unclear which
> version (if any) fixes that
>
> - fixups the aborts externally, via a boot wrapper, which is going to
> take some time to develop, causes additional burden on the distributors
> to provide instructions/build images on how to do it
I think the abort is already fixed in the kernel now on boot so option 1 
and 2 were not needed - and abort handler was removed as detailed by 
Rafal in the commit.  Only outstanding issue is now this new PCI 
enumeration issue.
>
> - fixups the aborts in the kernel, irrespective of where they come from,
> simple and easy
>
> - fixups the aborts in the kernel, look where they come from, by using
> some bit of magic, looking at PCIe registers and whatnot (provided that
> is even possible), not too hard, but can take a while, and is error prone
Option 4 sounds like the proper solution - check the range the abort is 
due to the PCI device enumeration and only ignore those aborts.
>
> I can certainly advocate that option 3 is the one that gives a working
> device, and this is what matters from a distribution perspective like
> LEDE/OpenWrt.
>
Since I don't use BCM5301x option 3 is fine by me.

On 31 October 2016 at 23:05, Scott Branden <scott.branden@broadcom.com> wrote:
> On 16-10-31 02:56 PM, Florian Fainelli wrote:
>> - fixups the aborts in the kernel, look where they come from, by using
>> some bit of magic, looking at PCIe registers and whatnot (provided that
>> is even possible), not too hard, but can take a while, and is error prone
>
> Option 4 sounds like the proper solution - check the range the abort is due
> to the PCI device enumeration and only ignore those aborts.

This was already suggested by Arnd:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422873.html
and Ray was checking some internal datasheets, but I'm afraid he never
found info on checking if error was caused by PCIe controller.

Maybe we could think about some BCM5301X API (extra symbol exported by
arch code) for starting ignoring aborts and stopping that. We could
ignore aborts during scanning only but honestly it sounds like a bit
hacky solution to me.


>> I can certainly advocate that option 3 is the one that gives a working
>> device, and this is what matters from a distribution perspective like
>> LEDE/OpenWrt.
>>
> Since I don't use BCM5301x option 3 is fine by me.

So for it seems like the best solution to me, but I'm open for changes
if someone points another that is clean & better one.

Hi Rafal,

On 10/31/2016 3:16 PM, Rafał Miłecki wrote:
> On 31 October 2016 at 23:05, Scott Branden <scott.branden@broadcom.com> wrote:
>> On 16-10-31 02:56 PM, Florian Fainelli wrote:
>>> - fixups the aborts in the kernel, look where they come from, by using
>>> some bit of magic, looking at PCIe registers and whatnot (provided that
>>> is even possible), not too hard, but can take a while, and is error prone
>>
>> Option 4 sounds like the proper solution - check the range the abort is due
>> to the PCI device enumeration and only ignore those aborts.
> 
> This was already suggested by Arnd:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-April/422873.html
> and Ray was checking some internal datasheets, but I'm afraid he never
> found info on checking if error was caused by PCIe controller.
> 

Correct. Based on response from our ASIC team, on Northstar, 1) we
cannot distinguish between various external imprecise aborts; 2) nor can
we use the internal PCIe controller register to disable unsupported
request from being forwarded as APB bus error (and therefore causes the
abort).

All subsequent iProc SoCs can disable this error forwarding from the
iProc PCIe controller.

> Maybe we could think about some BCM5301X API (extra symbol exported by
> arch code) for starting ignoring aborts and stopping that. We could
> ignore aborts during scanning only but honestly it sounds like a bit
> hacky solution to me.
> 
> 

Correct, one possible solution is to only ignoring the aborts during
condiguration space register access of an endpoint device. Below is the
logic how I only disable the APB bus error forwarding during these
accesses (for all other iProc SoCs):

+/**
+ * APB error forwarding can be disabled during access of configuration
+ * registers of the endpoint device, to prevent unsupported requests
+ * (typically seen during enumeration with multi-function devices) from
+ * triggering a system exception
+ */
+static inline void iproc_pcie_apb_err_disable(struct pci_bus *bus,
+					      bool disable)
+{
+	struct iproc_pcie *pcie = iproc_data(bus);
+	u32 val;
+
+	if (bus->number && pcie->has_apb_err_disable) {
+		val = iproc_pcie_read_reg(pcie, IPROC_PCIE_APB_ERR_EN);
+		if (disable)
+			val &= ~APB_ERR_EN;
+		else
+			val |= APB_ERR_EN;
+		iproc_pcie_write_reg(pcie, IPROC_PCIE_APB_ERR_EN, val);
+	}
+}
+
[...]
+static int iproc_pcie_config_read32(struct pci_bus *bus, unsigned int
devfn,
+				    int where, int size, u32 *val)
+{
+	int ret;
+
+	iproc_pcie_apb_err_disable(bus, true);
+	ret = pci_generic_config_read32(bus, devfn, where, size, val);
+	iproc_pcie_apb_err_disable(bus, false);
+
+	return ret;
+}
+
+static int iproc_pcie_config_write32(struct pci_bus *bus, unsigned int
devfn,
+				     int where, int size, u32 val)
+{
+	int ret;
+
+	iproc_pcie_apb_err_disable(bus, true);
+	ret = pci_generic_config_write32(bus, devfn, where, size, val);
+	iproc_pcie_apb_err_disable(bus, false);
+
+	return ret;
+}
+
 static struct pci_ops iproc_pcie_ops = {
 	.map_bus = iproc_pcie_map_cfg_bus,
-	.read = pci_generic_config_read32,
-	.write = pci_generic_config_write32,
+	.read = iproc_pcie_config_read32,
+	.write = iproc_pcie_config_write32,
 };

>>> I can certainly advocate that option 3 is the one that gives a working
>>> device, and this is what matters from a distribution perspective like
>>> LEDE/OpenWrt.
>>>
>> Since I don't use BCM5301x option 3 is fine by me.
> 
> So for it seems like the best solution to me, but I'm open for changes
> if someone points another that is clean & better one.
> 

I agree with that option 3 is probably the best solution for now, from a
distribution's perspective.

Thanks,

Ray