Hi Jose,
On 09/13/2017 04:24 AM, Jose Abreu wrote:
> By default __iormb() and __iowmb() translate into a do { } while(0) for
> AXS10x platform. As ARC700 supports the sync op we can use the standard
> memory barriers that are supplied by asm-generic headers.
>
> Signed-off-by: Jose Abreu <joabreu at synopsys.com>
> Cc: Vineet Gupta <vgupta at synopsys.com>
> Cc: Alexey Brodkin <abrodkin at synopsys.com>
> Cc: Joao Pinto <jpinto at synopsys.com>
> ---
> Hi Vineet,
>
> This is the final patch for the series which should fix all the stacktracing
> mechanism for Bus Error messages. In this one we force memory barriers for
> all IO operations
SYNC is a performance killer - it makes the processor wait for all memory
operations to complete before proceeding. So the justification for adding it to
all io accessors has to be more than getting correct stack traces.
> which will prevent op reordering by gcc and which will
> *really* correct blink and eret regs to show where exactly the error happened.
I don't think this is the precise reason why it works. Adding the SYNC likely adds
enough delay between the IO r/w such that CPU doesn't move to the next instruction.
As you would know very well, Bus Errors on ARC are imprecise and that is the root
of the issue. The bus fabric can take arbitrary amount of time to come back with
an error and ARC would have moved on to next instruction(s) when it is actually
notified. Thus the exception is "charged" to whatever the current PC was when it
is actually taken, which accounts for the bogus ERET and hence broken call chains
etc. Note that sdding SYNC delays things just enough but it is still not
guaranteed that you will correct ERET, depending on your bus network.
Hence the reason this patch is not acceptable.
Apologies for the delay in responding..
Thx,
-Vineet
> With this fix I get a correct stacktrace upon a readl() from a non-existent
> register which causes a Bus Error. Without this, I would get non-correct
> blink and eret addresses because the ld operation would launch a bus error
> way after we performed readl().
>
> I am sending this but I'm not exactly sure if all platforms support
> the sync op. Could you confirm this?
>
> Best regards,
> Jose Miguel Abreu
> ---
> arch/arc/include/asm/io.h | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/arch/arc/include/asm/io.h b/arch/arc/include/asm/io.h
> index c22b181..712defd 100644
> --- a/arch/arc/include/asm/io.h
> +++ b/arch/arc/include/asm/io.h
> @@ -12,15 +12,10 @@
> #include <linux/types.h>
> #include <asm/byteorder.h>
> #include <asm/page.h>
> -
> -#ifdef CONFIG_ISA_ARCV2
> #include <asm/barrier.h>
> +
> #define __iormb() rmb()
> #define __iowmb() wmb()
> -#else
> -#define __iormb() do { } while (0)
> -#define __iowmb() do { } while (0)
> -#endif
>
> extern void __iomem *ioremap(phys_addr_t paddr, unsigned long size);
> extern void __iomem *ioremap_prot(phys_addr_t paddr, unsigned long size,
>
