[rfc PATCH] ieee1394: ohci1394: delete bogus spinlock, flush MMIO writes

[rfc PATCH] ieee1394: ohci1394: delete bogus spinlock, flush MMIO writes

[Stefan Richter]

Remove a per-host spinlock which was only taken by the IRQ handler,
i.e. where no concurrency was involved.

All MMIO writes which were surrounded by the spinlock as well as the
very last MMIO write of the IRQ handler are now explicitly flushed by
MMIO reads of the respective register.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---

Some of the flushes may be unnecessary. Comments?


 drivers/ieee1394/ohci1394.c |   24 ++++++++++--------------
 drivers/ieee1394/ohci1394.h |    1 -
 2 files changed, 10 insertions(+), 15 deletions(-)

Index: linux-2.6.19-rc6/drivers/ieee1394/ohci1394.h
===================================================================
--- linux-2.6.19-rc6.orig/drivers/ieee1394/ohci1394.h
+++ linux-2.6.19-rc6/drivers/ieee1394/ohci1394.h
@@ -215,7 +215,6 @@ struct ti_ohci {
         int phyid, isroot;
 
         spinlock_t phy_reg_lock;
-	spinlock_t event_lock;
 
 	int self_id_errors;
 
Index: linux-2.6.19-rc6/drivers/ieee1394/ohci1394.c
===================================================================
--- linux-2.6.19-rc6.orig/drivers/ieee1394/ohci1394.c
+++ linux-2.6.19-rc6/drivers/ieee1394/ohci1394.c
@@ -2307,17 +2307,15 @@ static irqreturn_t ohci_irq_handler(int 
 	int phyid = -1, isroot = 0;
 	unsigned long flags;
 
-	/* Read and clear the interrupt event register.  Don't clear
-	 * the busReset event, though. This is done when we get the
-	 * selfIDComplete interrupt. */
-	spin_lock_irqsave(&ohci->event_lock, flags);
 	event = reg_read(ohci, OHCI1394_IntEventClear);
-	reg_write(ohci, OHCI1394_IntEventClear, event & ~OHCI1394_busReset);
-	spin_unlock_irqrestore(&ohci->event_lock, flags);
-
 	if (!event)
 		return IRQ_NONE;
 
+	/* Clear the interrupt event register except for the busReset event.
+	 * This is done when we handle the selfIDComplete event. */
+	reg_write(ohci, OHCI1394_IntEventClear, event & ~OHCI1394_busReset);
+	reg_read(ohci, OHCI1394_IntEventClear); /* flush */
+
 	/* If event is ~(u32)0 cardbus card was ejected.  In this case
 	 * we just return, and clean up in the ohci1394_pci_remove
 	 * function. */
@@ -2399,8 +2397,8 @@ static irqreturn_t ohci_irq_handler(int 
 		/* The busReset event bit can't be cleared during the
 		 * selfID phase, so we disable busReset interrupts, to
 		 * avoid burying the cpu in interrupt requests. */
-		spin_lock_irqsave(&ohci->event_lock, flags);
 		reg_write(ohci, OHCI1394_IntMaskClear, OHCI1394_busReset);
+		reg_read(ohci, OHCI1394_IntMaskClear); /* flush */
 
 		if (ohci->check_busreset) {
 			int loop_count = 0;
@@ -2409,10 +2407,9 @@ static irqreturn_t ohci_irq_handler(int 
 
 			while (reg_read(ohci, OHCI1394_IntEventSet) & OHCI1394_busReset) {
 				reg_write(ohci, OHCI1394_IntEventClear, OHCI1394_busReset);
+				reg_read(ohci, OHCI1394_IntEventClear); /* flush */
 
-				spin_unlock_irqrestore(&ohci->event_lock, flags);
 				udelay(10);
-				spin_lock_irqsave(&ohci->event_lock, flags);
 
 				/* The loop counter check is to prevent the driver
 				 * from remaining in this state forever. For the
@@ -2429,7 +2426,7 @@ static irqreturn_t ohci_irq_handler(int 
 				loop_count++;
 			}
 		}
-		spin_unlock_irqrestore(&ohci->event_lock, flags);
+
 		if (!host->in_bus_reset) {
 			DBGMSG("irq_handler: Bus reset requested");
 
@@ -2520,10 +2517,10 @@ static irqreturn_t ohci_irq_handler(int 
 
 			/* Clear the bus reset event and re-enable the
 			 * busReset interrupt.  */
-			spin_lock_irqsave(&ohci->event_lock, flags);
 			reg_write(ohci, OHCI1394_IntEventClear, OHCI1394_busReset);
+			reg_read(ohci, OHCI1394_IntEventClear); /* flush */
 			reg_write(ohci, OHCI1394_IntMaskSet, OHCI1394_busReset);
-			spin_unlock_irqrestore(&ohci->event_lock, flags);
+			reg_read(ohci, OHCI1394_IntMaskSet); /* flush */
 
 			/* Turn on phys dma reception.
 			 *
@@ -3398,7 +3395,6 @@ static int __devinit ohci1394_pci_probe(
 	/* We hopefully don't have to pre-allocate IT DMA like we did
 	 * for IR DMA above. Allocate it on-demand and mark inactive. */
 	ohci->it_legacy_context.ohci = NULL;
-	spin_lock_init(&ohci->event_lock);
 
 	/*
 	 * interrupts are disabled, all right, but... due to IRQF_SHARED we


-- 
Stefan Richter
-=====-=-==- =-== ===--
http://arcgraph.de/sr/

Re: [rfc PATCH] ieee1394: ohci1394: delete bogus spinlock, flush MMIO writes

[Alan]

On Tue, 28 Nov 2006 22:24:11 +0100 (CET)
Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> All MMIO writes which were surrounded by the spinlock as well as the
> very last MMIO write of the IRQ handler are now explicitly flushed by
> MMIO reads of the respective register.

MMIO is ordered anyway on the bus, you just need mmiowb() to force
ordering to the bus controller in case you are on a big numa box.

Re: [rfc PATCH] ieee1394: ohci1394: delete bogus spinlock, flush MMIO writes

[Stefan Richter]

Alan wrote:
> On Tue, 28 Nov 2006 22:24:11 +0100 (CET)
> Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>> All MMIO writes which were surrounded by the spinlock as well as the
>> very last MMIO write of the IRQ handler are now explicitly flushed by
>> MMIO reads of the respective register.
> 
> MMIO is ordered anyway on the bus, you just need mmiowb() to force
> ordering to the bus controller in case you are on a big numa box.

The mmiowb is a checkpoint to ensure ordering between different threads
of MMIO writes; i.e. it doesn't halt the thread until the write actually
reached the device like a read would do, right?
-- 
Stefan Richter
-=====-=-==- =-== ===-=
http://arcgraph.de/sr/

Re: [rfc PATCH] ieee1394: ohci1394: delete bogus spinlock, flush MMIO writes

[Alan]

On Wed, 29 Nov 2006 00:50:43 +0100
Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Alan wrote:
> > On Tue, 28 Nov 2006 22:24:11 +0100 (CET)
> > Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> >> All MMIO writes which were surrounded by the spinlock as well as the
> >> very last MMIO write of the IRQ handler are now explicitly flushed by
> >> MMIO reads of the respective register.
> > 
> > MMIO is ordered anyway on the bus, you just need mmiowb() to force
> > ordering to the bus controller in case you are on a big numa box.
> 
> The mmiowb is a checkpoint to ensure ordering between different threads
> of MMIO writes; i.e. it doesn't halt the thread until the write actually
> reached the device like a read would do, right?

It guarantees that no other mmio will sneak past it from another thread
but doesn't guarantee the previous I/O has hit the hardware. It's a much
weaker (and thus far faster) guarantee which is usually sufficient as it
can be combined with spin_unlock to enforce I/O ordering matching the
lock ordering.

Alan