[PATCH update 2] firewire: fw-ohci: don't append to AT context when its not active

[PATCH update 2] firewire: fw-ohci: don't append to AT context when its not active

[Stefan Richter]

From: Jarod Wilson <jwilson@redhat.com>

I finally tracked down the issues with this JMicron PCI-e card in my
possession to a failure to comply with section 7.2.3.2 of the OHCI 1.1
specification (thanks to Kristian for the pointer to illustrate that it
is indeed a flaw in this card, not the driver). The controller should
simply flush the packets we've appended to its AT queue if a bus reset
occurs before they've been transmitted and we'll try again, but
something goes wrong and the controller winds up hung.

However, we can avoid the problem by simply checking if the
IntEvent.busReset register had been set before we try appending to the
AT context. When busReset is set, the AT context is completely halted
until busReset is cleared, so there's no point in appending AT packets
until the register is cleared. So at_context_queue_packet() now checks
for busReset being set, and bails with an RCODE_GENERATION packet ack,
which results in us trying to append the packet again after recognizing
the fact there has been a bus reset, and clearing busReset.

Signed-off-by: Jarod Wilson <jwilson@redhat.com>

Split out busReset event logging to do it safer as a separate patch.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/fw-ohci.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Index: linux/drivers/firewire/fw-ohci.c
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.c
+++ linux/drivers/firewire/fw-ohci.c
@@ -950,8 +950,19 @@ at_context_queue_packet(struct context *
 				     DESCRIPTOR_IRQ_ALWAYS |
 				     DESCRIPTOR_BRANCH_ALWAYS);
 
-	/* FIXME: Document how the locking works. */
-	if (ohci->generation != packet->generation) {
+	/*
+	 * If the controller and packet generations don't match, we need to
+	 * bail out and try again.  If IntEvent.busReset is set, the AT context
+	 * is halted, so appending to the context and trying to run it is
+	 * futile.  Most controllers do the right thing and just flush the AT
+	 * queue (per section 7.2.3.2 of the OHCI 1.1 specification), but
+	 * some controllers (like a JMicron JMB381 PCI-e) misbehave and wind
+	 * up stalling out.  So we just bail out in software and try again
+	 * later, and everyone is happy.
+	 * FIXME: Document how the locking works.
+	 */
+	if (ohci->generation != packet->generation ||
+	    reg_read(ohci, OHCI1394_IntEventSet) & OHCI1394_busReset) {
 		if (packet->payload_length > 0)
 			dma_unmap_single(ohci->card.device, payload_bus,
 					 packet->payload_length, DMA_TO_DEVICE);

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

[PATCH] firewire: fw-ohci: conditionally log busReset interrupts

[Stefan Richter]

Add a debug option to watch bus reset interrupt events.  Half of this
patch is taken from Jarod Wilson's first version of the JMicron fix.

BusReset interrupts are only generated if the respective module
parameter flag was set before the controller is being initialized.  We
keep this event masked otherwise to reduce IRQ load in normal operation
and to avoid potential problems with buggy chips.

Note, this is unlike the other IRQ events whose logging can be enabled
any time after chip initialization.  This and the influence on what
interrupts the chip generates is why I added an extra flag for it.

Also, reorder the debug parameter flags according to their perceived
usefulness.

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

This could be merged into "firewire: debug interrupt events" before
mainline commit.

 drivers/firewire/fw-ohci.c |   28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

Index: linux/drivers/firewire/fw-ohci.c
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.c
+++ linux/drivers/firewire/fw-ohci.c
@@ -240,24 +240,32 @@ static char ohci_driver_name[] = KBUILD_
 
 #ifdef CONFIG_FIREWIRE_OHCI_DEBUG
 
-#define OHCI_PARAM_DEBUG_IRQS		1
+#define OHCI_PARAM_DEBUG_AT_AR		1
 #define OHCI_PARAM_DEBUG_SELFIDS	2
-#define OHCI_PARAM_DEBUG_AT_AR		4
+#define OHCI_PARAM_DEBUG_IRQS		4
+#define OHCI_PARAM_DEBUG_BUSRESETS	8 /* only effective before chip init */
 
 static int param_debug;
 module_param_named(debug, param_debug, int, 0644);
 MODULE_PARM_DESC(debug, "Verbose logging (default = 0"
-	", IRQs = "		__stringify(OHCI_PARAM_DEBUG_IRQS)
-	", self-IDs = "		__stringify(OHCI_PARAM_DEBUG_SELFIDS)
 	", AT/AR events = "	__stringify(OHCI_PARAM_DEBUG_AT_AR)
+	", self-IDs = "		__stringify(OHCI_PARAM_DEBUG_SELFIDS)
+	", IRQs = "		__stringify(OHCI_PARAM_DEBUG_IRQS)
+	", busReset events = "	__stringify(OHCI_PARAM_DEBUG_BUSRESETS)
 	", or a combination, or all = -1)");
 
 static void log_irqs(u32 evt)
 {
-	if (likely(!(param_debug & OHCI_PARAM_DEBUG_IRQS)))
+	if (likely(!(param_debug &
+			(OHCI_PARAM_DEBUG_IRQS | OHCI_PARAM_DEBUG_BUSRESETS))))
+		return;
+
+	if (!(param_debug & OHCI_PARAM_DEBUG_IRQS) &&
+	    !(evt & OHCI1394_busReset))
 		return;
 
-	printk(KERN_DEBUG KBUILD_MODNAME ": IRQ %08x%s%s%s%s%s%s%s%s%s%s%s%s\n",
+	printk(KERN_DEBUG KBUILD_MODNAME ": IRQ "
+	       "%08x%s%s%s%s%s%s%s%s%s%s%s%s%s\n",
 	       evt,
 	       evt & OHCI1394_selfIDComplete	? " selfID"		: "",
 	       evt & OHCI1394_RQPkt		? " AR_req"		: "",
@@ -270,12 +278,13 @@ static void log_irqs(u32 evt)
 	       evt & OHCI1394_cycleTooLong	? " cycleTooLong"	: "",
 	       evt & OHCI1394_cycle64Seconds	? " cycle64Seconds"	: "",
 	       evt & OHCI1394_regAccessFail	? " regAccessFail"	: "",
+	       evt & OHCI1394_busReset		? " busReset"		: "",
 	       evt & ~(OHCI1394_selfIDComplete | OHCI1394_RQPkt |
 		       OHCI1394_RSPkt | OHCI1394_reqTxComplete |
 		       OHCI1394_respTxComplete | OHCI1394_isochRx |
 		       OHCI1394_isochTx | OHCI1394_postedWriteErr |
 		       OHCI1394_cycleTooLong | OHCI1394_cycle64Seconds |
-		       OHCI1394_regAccessFail)
+		       OHCI1394_regAccessFail | OHCI1394_busReset)
 						? " ?"			: "");
 }
 
@@ -1328,7 +1337,8 @@ static irqreturn_t irq_handler(int irq, 
 	if (!event || !~event)
 		return IRQ_NONE;
 
-	reg_write(ohci, OHCI1394_IntEventClear, event);
+	/* busReset must not be cleared yet, see OHCI 1.1 clause 7.2.3.2 */
+	reg_write(ohci, OHCI1394_IntEventClear, event & ~OHCI1394_busReset);
 	log_irqs(event);
 
 	if (event & OHCI1394_selfIDComplete)
@@ -1467,6 +1477,8 @@ static int ohci_enable(struct fw_card *c
 		  OHCI1394_postedWriteErr | OHCI1394_cycleTooLong |
 		  OHCI1394_cycle64Seconds | OHCI1394_regAccessFail |
 		  OHCI1394_masterIntEnable);
+	if (param_debug & OHCI_PARAM_DEBUG_BUSRESETS)
+		reg_write(ohci, OHCI1394_IntMaskSet, OHCI1394_busReset);
 
 	/* Activate link_on bit and contender bit in our self ID packets.*/
 	if (ohci_update_phy_reg(card, 4, 0,

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

Re: [PATCH] firewire: fw-ohci: conditionally log busReset interrupts

[Jarod Wilson]

On Monday 07 April 2008 04:33:35 pm Stefan Richter wrote:
> Add a debug option to watch bus reset interrupt events.  Half of this
> patch is taken from Jarod Wilson's first version of the JMicron fix.
> 
> BusReset interrupts are only generated if the respective module
> parameter flag was set before the controller is being initialized.  We
> keep this event masked otherwise to reduce IRQ load in normal operation
> and to avoid potential problems with buggy chips.
> 
> Note, this is unlike the other IRQ events whose logging can be enabled
> any time after chip initialization.  This and the influence on what
> interrupts the chip generates is why I added an extra flag for it.
> 
> Also, reorder the debug parameter flags according to their perceived
> usefulness.
> 
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> ---

Indeed, after discussing, this looks much safer.

Signed-off-by: Jarod Wilson <jwilson@redhat.com>


-- 
Jarod Wilson
jwilson@redhat.com