ARC: IOC: panic if kernel was started with previously enabled IOC

Message ID 20181004131212.16405-1-Eugeniy.Paltsev@synopsys.com
State New
Headers show
Series
  • ARC: IOC: panic if kernel was started with previously enabled IOC
Related show

Commit Message

Eugeniy Paltsev Oct. 4, 2018, 1:12 p.m.
Disabling and reconfiguring of IOC are quite a tricky actions because
nobody knows what happens if there're IOC-ahndled tarnsactions in flight
when we're disabling IOC.

And the problem is external DMA masters [that were initialized and set
in a bootlaoder that was executed before we got here] might continue
to send data to memory even at this point and we have no way to prevent
that.

That said it's much safer to not enable IOC at all anywhere before Linux
kernel.

As we can build u-boot with IOC support (mostly for debugging purposes)
let's check that kernel was started with disabled IOC regardless of
our plans to use it or not.

Also while I'm at it, replace hardcoded bits in ARC_REG_IO_COH_PARTIAL
and ARC_REG_IO_COH_ENABLE registers by definitions.

Inspired by: https://lkml.org/lkml/2018/1/19/557

Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
 arch/arc/include/asm/cache.h |  2 ++
 arch/arc/mm/cache.c          | 25 ++++++++++++++++++++++---
 2 files changed, 24 insertions(+), 3 deletions(-)

Comments

Vineet Gupta Oct. 5, 2018, 10:56 p.m. | #1
On 10/04/2018 06:12 AM, Eugeniy Paltsev wrote:
>  
> diff --git a/arch/arc/mm/cache.c b/arch/arc/mm/cache.c
> index f2701c13a66b..ee7b63e9c5e3 100644
> --- a/arch/arc/mm/cache.c
> +++ b/arch/arc/mm/cache.c
> @@ -1144,6 +1144,25 @@ noinline void __init arc_ioc_setup(void)
>  {
>  	unsigned int ioc_base, mem_sz;
>  
> +	/*
> +	 * Disabling and reconfiguring of IOC are quite a tricky actions because
> +	 * nobody knows what happens if there're IOC-ahndled tarnsactions in
> +	 * flight when we're disabling IOC.
> +	 *
> +	 * And the problem is external DMA masters [that were initialized and
> +	 * set in a bootlaoder that was executed before we got here] might
> +	 * continue to send data to memory even at this point and we have
> +	 * no way to prevent that.
> +	 *
> +	 * That said it's much safer to not enable IOC at all anywhere before
> +	 * Linux kernel.
> +	 */
> +	if (read_aux_reg(ARC_REG_IO_COH_ENABLE) & ARC_IO_COH_ENABLE_BIT)
> +		panic("kernel was started with previously enabled IOC!\n");

While I understand the needs, this seems excessive, should we warm the user,
instead of panic ? Did you run into specific issue to warrant this !

OTOH in recent past more than 1 person ran into some hsdk uboot shenanigans, where
we had to upgrade the uboot to get it working with prebuit images - is that what
you are trying to prevent here - panic early instead of random user errors / hangs
later ?

-Vineet
Eugeniy Paltsev Oct. 9, 2018, 12:23 p.m. | #2
On Fri, 2018-10-05 at 22:56 +0000, Vineet Gupta wrote:
> On 10/04/2018 06:12 AM, Eugeniy Paltsev wrote:
> >  
> > diff --git a/arch/arc/mm/cache.c b/arch/arc/mm/cache.c
> > index f2701c13a66b..ee7b63e9c5e3 100644
> > --- a/arch/arc/mm/cache.c
> > +++ b/arch/arc/mm/cache.c
> > @@ -1144,6 +1144,25 @@ noinline void __init arc_ioc_setup(void)
> >  {
> >  	unsigned int ioc_base, mem_sz;
> >  
> > +	/*
> > +	 * Disabling and reconfiguring of IOC are quite a tricky actions because
> > +	 * nobody knows what happens if there're IOC-ahndled tarnsactions in
> > +	 * flight when we're disabling IOC.
> > +	 *
> > +	 * And the problem is external DMA masters [that were initialized and
> > +	 * set in a bootlaoder that was executed before we got here] might
> > +	 * continue to send data to memory even at this point and we have
> > +	 * no way to prevent that.
> > +	 *
> > +	 * That said it's much safer to not enable IOC at all anywhere before
> > +	 * Linux kernel.
> > +	 */
> > +	if (read_aux_reg(ARC_REG_IO_COH_ENABLE) & ARC_IO_COH_ENABLE_BIT)
> > +		panic("kernel was started with previously enabled IOC!\n");
> 
> While I understand the needs, this seems excessive, should we warm the user,
> instead of panic ? Did you run into specific issue to warrant this !

Yes we've run into this.
Remember, we have IOC disabled (actually 'not enabled') in most of HSDK linux demos
(https://github.com/foss-for-synopsys-dwc-arc-processors/buildroot-demos/releases)

So we faced several times that this demos crashes different weird ways when we
accidentally launch them using old u-boot (which enables IOC by default!)

As we don't want to forcibly disable IOC during linux launch
(https://lkml.org/lkml/2018/1/19/557) 
I guess panic is what we need here.

> OTOH in recent past more than 1 person ran into some hsdk uboot shenanigans, where
> we had to upgrade the uboot to get it working with prebuit images - is that what
> you are trying to prevent here - panic early instead of random user errors / hangs
> later ?

Yes, I prefer to panic early instead of random errors / crashes later.
The problem with warning here is that it isn't very clear / easy to match warning
message with some random error.

> -Vineet
Vineet Gupta Oct. 16, 2018, 5:27 p.m. | #3
On 10/04/2018 06:12 AM, Eugeniy Paltsev wrote:
> Disabling and reconfiguring of IOC are quite a tricky actions because
> nobody knows what happens if there're IOC-ahndled tarnsactions in flight
> when we're disabling IOC.
>
> And the problem is external DMA masters [that were initialized and set
> in a bootlaoder that was executed before we got here] might continue
> to send data to memory even at this point and we have no way to prevent
> that.
>
> That said it's much safer to not enable IOC at all anywhere before Linux
> kernel.
>
> As we can build u-boot with IOC support (mostly for debugging purposes)
> let's check that kernel was started with disabled IOC regardless of
> our plans to use it or not.
>
> Also while I'm at it, replace hardcoded bits in ARC_REG_IO_COH_PARTIAL
> and ARC_REG_IO_COH_ENABLE registers by definitions.
>
> Inspired by: https://lkml.org/lkml/2018/1/19/557
>

Applied with updated changelog !

-Vineet

Patch

diff --git a/arch/arc/include/asm/cache.h b/arch/arc/include/asm/cache.h
index ff7d3232764a..f393b663413e 100644
--- a/arch/arc/include/asm/cache.h
+++ b/arch/arc/include/asm/cache.h
@@ -113,7 +113,9 @@  extern unsigned long perip_base, perip_end;
 
 /* IO coherency related Auxiliary registers */
 #define ARC_REG_IO_COH_ENABLE	0x500
+#define ARC_IO_COH_ENABLE_BIT	BIT(0)
 #define ARC_REG_IO_COH_PARTIAL	0x501
+#define ARC_IO_COH_PARTIAL_BIT	BIT(0)
 #define ARC_REG_IO_COH_AP0_BASE	0x508
 #define ARC_REG_IO_COH_AP0_SIZE	0x509
 
diff --git a/arch/arc/mm/cache.c b/arch/arc/mm/cache.c
index f2701c13a66b..ee7b63e9c5e3 100644
--- a/arch/arc/mm/cache.c
+++ b/arch/arc/mm/cache.c
@@ -1144,6 +1144,25 @@  noinline void __init arc_ioc_setup(void)
 {
 	unsigned int ioc_base, mem_sz;
 
+	/*
+	 * Disabling and reconfiguring of IOC are quite a tricky actions because
+	 * nobody knows what happens if there're IOC-ahndled tarnsactions in
+	 * flight when we're disabling IOC.
+	 *
+	 * And the problem is external DMA masters [that were initialized and
+	 * set in a bootlaoder that was executed before we got here] might
+	 * continue to send data to memory even at this point and we have
+	 * no way to prevent that.
+	 *
+	 * That said it's much safer to not enable IOC at all anywhere before
+	 * Linux kernel.
+	 */
+	if (read_aux_reg(ARC_REG_IO_COH_ENABLE) & ARC_IO_COH_ENABLE_BIT)
+		panic("kernel was started with previously enabled IOC!\n");
+
+	if (!ioc_enable)
+		return;
+
 	/*
 	 * As for today we don't support both IOC and ZONE_HIGHMEM enabled
 	 * simultaneously. This happens because as of today IOC aperture covers
@@ -1187,8 +1206,8 @@  noinline void __init arc_ioc_setup(void)
 		panic("IOC Aperture start must be aligned to the size of the aperture");
 
 	write_aux_reg(ARC_REG_IO_COH_AP0_BASE, ioc_base >> 12);
-	write_aux_reg(ARC_REG_IO_COH_PARTIAL, 1);
-	write_aux_reg(ARC_REG_IO_COH_ENABLE, 1);
+	write_aux_reg(ARC_REG_IO_COH_PARTIAL, ARC_IO_COH_PARTIAL_BIT);
+	write_aux_reg(ARC_REG_IO_COH_ENABLE, ARC_IO_COH_ENABLE_BIT);
 
 	/* Re-enable L1 dcache */
 	__dc_enable();
@@ -1265,7 +1284,7 @@  void __init arc_cache_init_master(void)
 	if (is_isa_arcv2() && l2_line_sz && !slc_enable)
 		arc_slc_disable();
 
-	if (is_isa_arcv2() && ioc_enable)
+	if (is_isa_arcv2())
 		arc_ioc_setup();
 
 	if (is_isa_arcv2() && l2_line_sz && slc_enable) {