[PATCH] SIBYTE: fix locking in set_irq_affinity

[PATCH] SIBYTE: fix locking in set_irq_affinity

[Thomas Bogendoerfer]

Locking of irq_desc is now done in irq_set_affinity; Don't lock it
again in chip specific set_affinity function.

Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
---

 arch/mips/sibyte/bcm1480/irq.c |    7 ++-----
 arch/mips/sibyte/sb1250/irq.c  |    7 ++-----
 2 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/mips/sibyte/bcm1480/irq.c b/arch/mips/sibyte/bcm1480/irq.c
index 352352b..c147c4b 100644
--- a/arch/mips/sibyte/bcm1480/irq.c
+++ b/arch/mips/sibyte/bcm1480/irq.c
@@ -113,7 +113,6 @@ static void bcm1480_set_affinity(unsigned int irq, const struct cpumask *mask)
 {
 	int i = 0, old_cpu, cpu, int_on, k;
 	u64 cur_ints;
-	struct irq_desc *desc = irq_desc + irq;
 	unsigned long flags;
 	unsigned int irq_dirty;
 
@@ -127,8 +126,7 @@ static void bcm1480_set_affinity(unsigned int irq, const struct cpumask *mask)
 	cpu = cpu_logical_map(i);
 
 	/* Protect against other affinity changers and IMR manipulation */
-	spin_lock_irqsave(&desc->lock, flags);
-	spin_lock(&bcm1480_imr_lock);
+	spin_lock_irqsave(&bcm1480_imr_lock, flags);
 
 	/* Swizzle each CPU's IMR (but leave the IP selection alone) */
 	old_cpu = bcm1480_irq_owner[irq];
@@ -153,8 +151,7 @@ static void bcm1480_set_affinity(unsigned int irq, const struct cpumask *mask)
 			____raw_writeq(cur_ints, IOADDR(A_BCM1480_IMR_MAPPER(cpu) + R_BCM1480_IMR_INTERRUPT_MASK_H + (k*BCM1480_IMR_HL_SPACING)));
 		}
 	}
-	spin_unlock(&bcm1480_imr_lock);
-	spin_unlock_irqrestore(&desc->lock, flags);
+	spin_unlock_irqrestore(&bcm1480_imr_lock, flags);
 }
 #endif
 
diff --git a/arch/mips/sibyte/sb1250/irq.c b/arch/mips/sibyte/sb1250/irq.c
index c08ff58..38cb998 100644
--- a/arch/mips/sibyte/sb1250/irq.c
+++ b/arch/mips/sibyte/sb1250/irq.c
@@ -107,7 +107,6 @@ static void sb1250_set_affinity(unsigned int irq, const struct cpumask *mask)
 {
 	int i = 0, old_cpu, cpu, int_on;
 	u64 cur_ints;
-	struct irq_desc *desc = irq_desc + irq;
 	unsigned long flags;
 
 	i = cpumask_first(mask);
@@ -121,8 +120,7 @@ static void sb1250_set_affinity(unsigned int irq, const struct cpumask *mask)
 	cpu = cpu_logical_map(i);
 
 	/* Protect against other affinity changers and IMR manipulation */
-	spin_lock_irqsave(&desc->lock, flags);
-	spin_lock(&sb1250_imr_lock);
+	spin_lock_irqsave(&sb1250_imr_lock, flags);
 
 	/* Swizzle each CPU's IMR (but leave the IP selection alone) */
 	old_cpu = sb1250_irq_owner[irq];
@@ -144,8 +142,7 @@ static void sb1250_set_affinity(unsigned int irq, const struct cpumask *mask)
 		____raw_writeq(cur_ints, IOADDR(A_IMR_MAPPER(cpu) +
 					R_IMR_INTERRUPT_MASK));
 	}
-	spin_unlock(&sb1250_imr_lock);
-	spin_unlock_irqrestore(&desc->lock, flags);
+	spin_unlock_irqrestore(&sb1250_imr_lock, flags);
 }
 #endif
 

Re: [PATCH] SIBYTE: fix locking in set_irq_affinity

[Ralf Baechle]

On Mon, May 04, 2009 at 11:51:54PM +0200, Thomas Bogendoerfer wrote:

> Locking of irq_desc is now done in irq_set_affinity; Don't lock it
> again in chip specific set_affinity function.
> 
> Signed-off-by: Thomas Bogendoerfer <tsbogend@alpha.franken.de>

Funny locking code, indeed.  I've applied your patch to master and all
-stable branches but I'm holding back on pushing for the moment so
somebody else has a chance to comment.

Thanks,

  Ralf

Re: [PATCH] SIBYTE: fix locking in set_irq_affinity

[Martin Michlmayr]

* Thomas Bogendoerfer <tsbogend@alpha.franken.de> [2009-05-04 23:51]:
> Locking of irq_desc is now done in irq_set_affinity; Don't lock it
> again in chip specific set_affinity function.

SWARM boots with this patch, but dmesg shows:

[17179570.260000] attempted to set irq affinity for irq 8 to multiple CPUs
[17179570.484000] attempted to set irq affinity for irq 8 to multiple CPUs
[17179570.500000] attempted to set irq affinity for irq 8 to multiple CPUs

-- 
Martin Michlmayr
http://www.cyrius.com/

Re: [PATCH] SIBYTE: fix locking in set_irq_affinity

[Thomas Bogendoerfer]

On Tue, May 12, 2009 at 11:55:57PM +0200, Martin Michlmayr wrote:
> * Thomas Bogendoerfer <tsbogend@alpha.franken.de> [2009-05-04 23:51]:
> > Locking of irq_desc is now done in irq_set_affinity; Don't lock it
> > again in chip specific set_affinity function.
> 
> SWARM boots with this patch, but dmesg shows:
> 
> [17179570.260000] attempted to set irq affinity for irq 8 to multiple CPUs
> [17179570.484000] attempted to set irq affinity for irq 8 to multiple CPUs
> [17179570.500000] attempted to set irq affinity for irq 8 to multiple CPUs

I saw them as well, either the caller of set_irq_affinity does something
illegal or the API has changed and the message just should go away...

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] SIBYTE: fix locking in set_irq_affinity

[Ralf Baechle]

On Thu, May 14, 2009 at 03:20:30PM +0200, Thomas Bogendoerfer wrote:

> > [17179570.260000] attempted to set irq affinity for irq 8 to multiple CPUs
> > [17179570.484000] attempted to set irq affinity for irq 8 to multiple CPUs
> > [17179570.500000] attempted to set irq affinity for irq 8 to multiple CPUs
> 
> I saw them as well, either the caller of set_irq_affinity does something
> illegal or the API has changed and the message just should go away...

Several things here:

- I would be interested in the caller of set_irq_affinity here.  It seems
  like the IDE code is doing this - but why?
- the message should go away or at least be changed depend on DEBUG being
  defined.  Normal usage errors should not result in error messages to the
  console.  Heck, the kernel console is not stderr.
- the kernel's idea of interrupt mask is a different one from the hardware.
  In the kernel it is the set of CPUs one of which should process an
  interrupt.  The Sibyte hardware interprets it as the set of CPUs which
  (all!) will receive the interrupt to process.

  Ralf