[PATCH 0/3] irqchip: orion: bridge irq fixes for v3.14-rc1

[PATCH 0/3] irqchip: orion: bridge irq fixes for v3.14-rc1

[Sebastian Hesselbarth]

This is a small patch set to fix some shortcomings how Orion bridge
irqs are handled. The patches are based on v3.13-rc8 and should go
into v3.14. They can possibly also marked for -stable down to v3.10.

This patches are the result of a discussion about a stale watchdog irq,
that can accidentially trigger the watchdog's irq handler and cause a
reset [1].

The first patch will add a write to clear already pending interrupts
on init. The second patch replaces handle_level_irq with handle_edge_irq
which is more appropriate for bridge irqs which are edge-triggered.
The last patch finally, fixes stale interrupts by installing an
.irq_enable callback, that will clear a possible pending interrupt
before unmasking it.

[1] http://www.spinics.net/lists/arm-kernel/msg302106.html

Sebastian Hesselbarth (3):
  irqchip: orion: clear bridge cause register on init
  irqchip: orion: use handle_edge_irq on bridge irqs
  irqchip: orion: clear stale interrupts in irq_enable

 drivers/irqchip/irq-orion.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

---
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
-- 
1.8.5.2

[PATCH 1/3] irqchip: orion: clear bridge cause register on init

[Sebastian Hesselbarth]

It is good practice to mask and clear pending irqs on init. We already
mask all irqs, so also clear the bridge irq cause register.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/irqchip/irq-orion.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
index e51d40031884..4137c3d15284 100644
--- a/drivers/irqchip/irq-orion.c
+++ b/drivers/irqchip/irq-orion.c
@@ -180,8 +180,9 @@ static int __init orion_bridge_irq_init(struct device_node *np,
 	gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
 	gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
 
-	/* mask all interrupts */
+	/* mask and clear all interrupts */
 	writel(0, gc->reg_base + ORION_BRIDGE_IRQ_MASK);
+	writel(0, gc->reg_base + ORION_BRIDGE_IRQ_CAUSE);
 
 	irq_set_handler_data(irq, domain);
 	irq_set_chained_handler(irq, orion_bridge_irq_handler);
-- 
1.8.5.2

[PATCH 2/3] irqchip: orion: use handle_edge_irq on bridge irqs

[Sebastian Hesselbarth]

Bridge irqs are edge-triggered, i.e. they get asserted on low-to-high
transitions and not on the level of the downstream interrupt line.
This replaces handle_level_irq by the more appropriate handle_edge_irq.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/irqchip/irq-orion.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
index 4137c3d15284..1f636f719065 100644
--- a/drivers/irqchip/irq-orion.c
+++ b/drivers/irqchip/irq-orion.c
@@ -143,7 +143,7 @@ static int __init orion_bridge_irq_init(struct device_node *np,
 	}
 
 	ret = irq_alloc_domain_generic_chips(domain, nrirqs, 1, np->name,
-			     handle_level_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE);
+			     handle_edge_irq, clr, 0, IRQ_GC_INIT_MASK_CACHE);
 	if (ret) {
 		pr_err("%s: unable to alloc irq domain gc\n", np->name);
 		return ret;
-- 
1.8.5.2

[PATCH 3/3] irqchip: orion: clear stale interrupts in irq_enable

[Sebastian Hesselbarth]

Bridge IRQ_CAUSE bits are asserted regardless of the corresponding bit in
IRQ_MASK register. To avoid interrupt events on stale irqs, we have to clear
them before unmask. This installs an .irq_enable callback to ensure stale
irqs are cleared before initial unmask.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/irqchip/irq-orion.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
index 1f636f719065..80b13c1a0947 100644
--- a/drivers/irqchip/irq-orion.c
+++ b/drivers/irqchip/irq-orion.c
@@ -123,6 +123,18 @@ static void orion_bridge_irq_handler(unsigned int irq, struct irq_desc *desc)
 	}
 }
 
+/*
+ * Bridge IRQ_CAUSE is asserted regardless of IRQ_MASK register.
+ * To avoid interrupt events on stale irqs, we clear them before unmask.
+ */
+static void orion_bridge_irq_enable(struct irq_data *d)
+{
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+
+	ct->chip.irq_ack(d);
+	ct->chip.irq_unmask(d);
+}
+
 static int __init orion_bridge_irq_init(struct device_node *np,
 					struct device_node *parent)
 {
@@ -176,6 +188,7 @@ static int __init orion_bridge_irq_init(struct device_node *np,
 
 	gc->chip_types[0].regs.ack = ORION_BRIDGE_IRQ_CAUSE;
 	gc->chip_types[0].regs.mask = ORION_BRIDGE_IRQ_MASK;
+	gc->chip_types[0].chip.irq_enable = orion_bridge_irq_enable;
 	gc->chip_types[0].chip.irq_ack = irq_gc_ack_clr_bit;
 	gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
 	gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
-- 
1.8.5.2

[PATCH 3/3] irqchip: orion: clear stale interrupts in irq_enable

[Jason Gunthorpe]

On Thu, Jan 23, 2014 at 11:38:06PM +0100, Sebastian Hesselbarth wrote:
> Bridge IRQ_CAUSE bits are asserted regardless of the corresponding bit in
> IRQ_MASK register. To avoid interrupt events on stale irqs, we have to clear
> them before unmask. This installs an .irq_enable callback to ensure stale
> irqs are cleared before initial unmask.

I'm not sure if putting this in irq_enable is correct. I think this
should only happen at irq_startup.

The question boils down to what is supposed to happen with this code
sequence:

disable_irq(..);
write(.. something to cause an interrupt edge ..);
.. synchronize ..
enable_irq(..);

Do we get the interrupt or not?

I found this message from Linus long ago:
 http://yarchive.net/comp/linux/edge_triggered_interrupts.html
> Btw, the "disable_irq()/enable_irq()" subsystem has been written so that
> when you disable an edge-triggered interrupt, and the edge happens while
> the interrupt is disabled, we will re-play the interrupt at enable time.
> Exactly so that drivers can have an easier time and don't have to
> normally worry about whether something is edge or level-triggered.

And found this note in Documentation/DocBook/genericirq.tmpl:

> This prevents losing edge interrupts on hardware which does
> not store an edge interrupt event while the interrupt is disabled at
> the hardware level. 

So I think it is very clear that the chip driver should not discard
edges that happened while the interrupt was disabled.

Regards,
Jason

[PATCH 3/3] irqchip: orion: clear stale interrupts in irq_enable

[Sebastian Hesselbarth]

On 01/23/2014 11:52 PM, Jason Gunthorpe wrote:
> On Thu, Jan 23, 2014 at 11:38:06PM +0100, Sebastian Hesselbarth wrote:
>> Bridge IRQ_CAUSE bits are asserted regardless of the corresponding bit in
>> IRQ_MASK register. To avoid interrupt events on stale irqs, we have to clear
>> them before unmask. This installs an .irq_enable callback to ensure stale
>> irqs are cleared before initial unmask.
>
> I'm not sure if putting this in irq_enable is correct. I think this
> should only happen at irq_startup.
>
> The question boils down to what is supposed to happen with this code
> sequence:
>
> disable_irq(..);
> write(.. something to cause an interrupt edge ..);
> .. synchronize ..
> enable_irq(..);
>
> Do we get the interrupt or not?

Jason,

I get the point and actually I'd chosen .irq_enable because using
.irq_startup didn't work. I rechecked this and now it works.. maybe
it is getting too late for me. I'll send a v2 of this patch shortly.

Sebastian

> I found this message from Linus long ago:
>   http://yarchive.net/comp/linux/edge_triggered_interrupts.html
>> Btw, the "disable_irq()/enable_irq()" subsystem has been written so that
>> when you disable an edge-triggered interrupt, and the edge happens while
>> the interrupt is disabled, we will re-play the interrupt at enable time.
>> Exactly so that drivers can have an easier time and don't have to
>> normally worry about whether something is edge or level-triggered.
>
> And found this note in Documentation/DocBook/genericirq.tmpl:
>
>> This prevents losing edge interrupts on hardware which does
>> not store an edge interrupt event while the interrupt is disabled at
>> the hardware level.
>
> So I think it is very clear that the chip driver should not discard
> edges that happened while the interrupt was disabled.
>
> Regards,
> Jason
>

[PATCH v2 3/3] irqchip: orion: clear stale interrupts in irq_startup

[Sebastian Hesselbarth]

Bridge IRQ_CAUSE bits are asserted regardless of the corresponding bit in
IRQ_MASK register. To avoid interrupt events on stale irqs, we have to clear
them before unmask. This installs an .irq_startup callback to ensure stale
irqs are cleared before initial unmask.

Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Changelog:
v1->v2:
- use .irq_startup instead of .irq_enable (Reported by Jason Gunthorpe)

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: linux-arm-kernel at lists.infradead.org
Cc: linux-kernel at vger.kernel.org
---
 drivers/irqchip/irq-orion.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
index 1f636f719065..0dfdc5c824a1 100644
--- a/drivers/irqchip/irq-orion.c
+++ b/drivers/irqchip/irq-orion.c
@@ -123,6 +123,19 @@ static void orion_bridge_irq_handler(unsigned int irq, struct irq_desc *desc)
 	}
 }
 
+/*
+ * Bridge IRQ_CAUSE is asserted regardless of IRQ_MASK register.
+ * To avoid interrupt events on stale irqs, we clear them before unmask.
+ */
+static unsigned int orion_bridge_irq_startup(struct irq_data *d)
+{
+	struct irq_chip_type *ct = irq_data_get_chip_type(d);
+
+	ct->chip.irq_ack(d);
+	ct->chip.irq_unmask(d);
+	return 0;
+}
+
 static int __init orion_bridge_irq_init(struct device_node *np,
 					struct device_node *parent)
 {
@@ -176,6 +189,7 @@ static int __init orion_bridge_irq_init(struct device_node *np,
 
 	gc->chip_types[0].regs.ack = ORION_BRIDGE_IRQ_CAUSE;
 	gc->chip_types[0].regs.mask = ORION_BRIDGE_IRQ_MASK;
+	gc->chip_types[0].chip.irq_startup = orion_bridge_irq_startup;
 	gc->chip_types[0].chip.irq_ack = irq_gc_ack_clr_bit;
 	gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
 	gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
-- 
1.8.5.2

[PATCH 3/3] irqchip: orion: clear stale interrupts in irq_enable

[Russell King - ARM Linux]

On Thu, Jan 23, 2014 at 03:52:08PM -0700, Jason Gunthorpe wrote:
> On Thu, Jan 23, 2014 at 11:38:06PM +0100, Sebastian Hesselbarth wrote:
> > Bridge IRQ_CAUSE bits are asserted regardless of the corresponding bit in
> > IRQ_MASK register. To avoid interrupt events on stale irqs, we have to clear
> > them before unmask. This installs an .irq_enable callback to ensure stale
> > irqs are cleared before initial unmask.
> 
> I'm not sure if putting this in irq_enable is correct. I think this
> should only happen at irq_startup.
> 
> The question boils down to what is supposed to happen with this code
> sequence:
> 
> disable_irq(..);
> write(.. something to cause an interrupt edge ..);
> .. synchronize ..
> enable_irq(..);
> 
> Do we get the interrupt or not?

The answer is... yes, the interrupt should be delivered after the
interrupt is re-enabled.

> I found this message from Linus long ago:
>  http://yarchive.net/comp/linux/edge_triggered_interrupts.html
> > Btw, the "disable_irq()/enable_irq()" subsystem has been written so that
> > when you disable an edge-triggered interrupt, and the edge happens while
> > the interrupt is disabled, we will re-play the interrupt at enable time.
> > Exactly so that drivers can have an easier time and don't have to
> > normally worry about whether something is edge or level-triggered.
> 
> And found this note in Documentation/DocBook/genericirq.tmpl:
> 
> > This prevents losing edge interrupts on hardware which does
> > not store an edge interrupt event while the interrupt is disabled at
> > the hardware level. 
> 
> So I think it is very clear that the chip driver should not discard
> edges that happened while the interrupt was disabled.

Correct.

-- 
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up.  Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".

[PATCH 1/3] irqchip: orion: clear bridge cause register on init

[Ezequiel Garcia]

On Thu, Jan 23, 2014 at 11:38:04PM +0100, Sebastian Hesselbarth wrote:
> It is good practice to mask and clear pending irqs on init. We already
> mask all irqs, so also clear the bridge irq cause register.
> 
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Cc: linux-arm-kernel at lists.infradead.org
> Cc: linux-kernel at vger.kernel.org
> ---
>  drivers/irqchip/irq-orion.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-orion.c b/drivers/irqchip/irq-orion.c
> index e51d40031884..4137c3d15284 100644
> --- a/drivers/irqchip/irq-orion.c
> +++ b/drivers/irqchip/irq-orion.c
> @@ -180,8 +180,9 @@ static int __init orion_bridge_irq_init(struct device_node *np,
>  	gc->chip_types[0].chip.irq_mask = irq_gc_mask_clr_bit;
>  	gc->chip_types[0].chip.irq_unmask = irq_gc_mask_set_bit;
>  
> -	/* mask all interrupts */
> +	/* mask and clear all interrupts */
>  	writel(0, gc->reg_base + ORION_BRIDGE_IRQ_MASK);
> +	writel(0, gc->reg_base + ORION_BRIDGE_IRQ_CAUSE);
>  

This looks a bit bogus to me, now that we are clearing the cause upon
irq_startup(). Don't have a strong opinion, it's just that I fail to see
why we'd want or need this change...
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 0/3] irqchip: orion: bridge irq fixes for v3.14-rc1

[Jason Cooper]

On Thu, Jan 23, 2014 at 11:38:03PM +0100, Sebastian Hesselbarth wrote:
> This is a small patch set to fix some shortcomings how Orion bridge
> irqs are handled. The patches are based on v3.13-rc8 and should go
> into v3.14. They can possibly also marked for -stable down to v3.10.
> 
> This patches are the result of a discussion about a stale watchdog irq,
> that can accidentially trigger the watchdog's irq handler and cause a
> reset [1].
> 
> The first patch will add a write to clear already pending interrupts
> on init. The second patch replaces handle_level_irq with handle_edge_irq
> which is more appropriate for bridge irqs which are edge-triggered.
> The last patch finally, fixes stale interrupts by installing an
> .irq_enable callback, that will clear a possible pending interrupt
> before unmasking it.
> 
> [1] http://www.spinics.net/lists/arm-kernel/msg302106.html
> 
> Sebastian Hesselbarth (3):
>   irqchip: orion: clear bridge cause register on init
>   irqchip: orion: use handle_edge_irq on bridge irqs
>   irqchip: orion: clear stale interrupts in irq_enable
> 
>  drivers/irqchip/irq-orion.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)

Whole series applied to mvebu-next/irqchip-fixes (v2 for 3/3).  Once I
see that the outstanding pull requests for mvebu/irqchip have been
merged into mainline, I'll be changing the name of this branch to
mvebu/irqchip-fixes.  I just don't want to upset the applecart atm.

Oh yeah, Cc'd for stable back to v3.10.

thx,

Jason.

[PATCH 0/3] irqchip: orion: bridge irq fixes for v3.14-rc1

[Ezequiel Garcia]

On Tue, Feb 04, 2014 at 11:54:15PM -0500, Jason Cooper wrote:
> On Thu, Jan 23, 2014 at 11:38:03PM +0100, Sebastian Hesselbarth wrote:
> > This is a small patch set to fix some shortcomings how Orion bridge
> > irqs are handled. The patches are based on v3.13-rc8 and should go
> > into v3.14. They can possibly also marked for -stable down to v3.10.
> > 
> > This patches are the result of a discussion about a stale watchdog irq,
> > that can accidentially trigger the watchdog's irq handler and cause a
> > reset [1].
> > 
> > The first patch will add a write to clear already pending interrupts
> > on init. The second patch replaces handle_level_irq with handle_edge_irq
> > which is more appropriate for bridge irqs which are edge-triggered.
> > The last patch finally, fixes stale interrupts by installing an
> > .irq_enable callback, that will clear a possible pending interrupt
> > before unmasking it.
> > 
> > [1] http://www.spinics.net/lists/arm-kernel/msg302106.html
> > 
> > Sebastian Hesselbarth (3):
> >   irqchip: orion: clear bridge cause register on init
> >   irqchip: orion: use handle_edge_irq on bridge irqs
> >   irqchip: orion: clear stale interrupts in irq_enable
> > 
> >  drivers/irqchip/irq-orion.c | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> Whole series applied to mvebu-next/irqchip-fixes (v2 for 3/3).  Once I
> see that the outstanding pull requests for mvebu/irqchip have been
> merged into mainline, I'll be changing the name of this branch to
> mvebu/irqchip-fixes.  I just don't want to upset the applecart atm.
> 
> Oh yeah, Cc'd for stable back to v3.10.
> 

If you want to pick this:

Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

Tested Kirkwood Topkick and Dove Cubox. Quite frankly, I haven't checked
this series prevents an "errant watchdog", but just pursued a boot test.

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH 0/3] irqchip: orion: bridge irq fixes for v3.14-rc1

[Jason Cooper]

On Thu, Feb 06, 2014 at 02:10:40PM -0300, Ezequiel Garcia wrote:
> On Tue, Feb 04, 2014 at 11:54:15PM -0500, Jason Cooper wrote:
> > On Thu, Jan 23, 2014 at 11:38:03PM +0100, Sebastian Hesselbarth wrote:
> > > This is a small patch set to fix some shortcomings how Orion bridge
> > > irqs are handled. The patches are based on v3.13-rc8 and should go
> > > into v3.14. They can possibly also marked for -stable down to v3.10.
> > > 
> > > This patches are the result of a discussion about a stale watchdog irq,
> > > that can accidentially trigger the watchdog's irq handler and cause a
> > > reset [1].
> > > 
> > > The first patch will add a write to clear already pending interrupts
> > > on init. The second patch replaces handle_level_irq with handle_edge_irq
> > > which is more appropriate for bridge irqs which are edge-triggered.
> > > The last patch finally, fixes stale interrupts by installing an
> > > .irq_enable callback, that will clear a possible pending interrupt
> > > before unmasking it.
> > > 
> > > [1] http://www.spinics.net/lists/arm-kernel/msg302106.html
> > > 
> > > Sebastian Hesselbarth (3):
> > >   irqchip: orion: clear bridge cause register on init
> > >   irqchip: orion: use handle_edge_irq on bridge irqs
> > >   irqchip: orion: clear stale interrupts in irq_enable
> > > 
> > >  drivers/irqchip/irq-orion.c | 18 ++++++++++++++++--
> > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > 
> > Whole series applied to mvebu-next/irqchip-fixes (v2 for 3/3).  Once I
> > see that the outstanding pull requests for mvebu/irqchip have been
> > merged into mainline, I'll be changing the name of this branch to
> > mvebu/irqchip-fixes.  I just don't want to upset the applecart atm.
> > 
> > Oh yeah, Cc'd for stable back to v3.10.
> > 
> 
> If you want to pick this:
> 
> Tested-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

Added, thanks for testing!

thx,

Jason.