firewire: core: check for 1394a compliant IRM, fix inaccessibility of Sony camcorder

Per IEEE 1394 clause 8.4.2.3, a contender for the IRM role shall check whether the current IRM complies to 1394a-2000 or later. If not force a compliant node (e.g. itself) to become IRM. This was implemented in the older ieee1394 driver but not yet in firewire-core.

An older Sony camcorder (Sony DCR-TRV25) which implements 1394-1995 IRM but neither 1394a-2000 IRM nor BM was now found to cause an interoperability bug:
- Camcorder becomes root node when plugged in, hence gets IRM role.
- firewire-core successfully contends for BM role, proceeds to perform gap count optimization and resets the bus.
- Sony camcorder ignores presence of a BM (against the spec, this is a firmware bug), performs its idea of gap count optimization and resets the bus.
- Preceding two steps are repeated endlessly, bus never settles, regular I/O is practically impossible.
http://thread.gmane.org/gmane.linux.kernel.firewire.user/3913

This is an interoperability regression from the old to the new drivers. Fix it indirectly by adding the 1394a IRM check. The spec suggests three and a half methods to determine 1394a compliance of a remote IRM; we choose the method of testing the Config_ROM.Bus_Info.generation field. This is data that firewire-core should have readily available at this point, i.e. does not require extra I/O.

---

The patch was generated against the latest kernel sources but is applicable with harmless line offsets to the 2.6.32.y stable kernel series too. Hence it should be applicable to Debian's 2.6.32 sources as well.

H. S., could you please test this? *If* this fixes the issue, may I add your mail address in a Tested-By signature to the changelog as described at
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=Documentation/SubmittingPatches;h=72651f788f4e3536149ef5e7ddfbed96a8f14d2f;hb=HEAD#l412
?

 drivers/firewire/core-card.c |   24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

Index: b/drivers/firewire/core-card.c
===================================================================
@@ -231,7 +231,7 @@ void fw_schedule_bm_work(struct fw_card 
 static void fw_card_bm_work(struct work_struct *work)
 {
 	struct fw_card *card = container_of(work, struct fw_card, work.work);
-	struct fw_device *root_device;
+	struct fw_device *root_device, *irm_device;
 	struct fw_node *root_node;
 	unsigned long flags;
 	int root_id, new_root_id, irm_id, local_id;

@@ -239,6 +239,7 @@ static void fw_card_bm_work(struct work_
 	bool do_reset = false;
 	bool root_device_is_running;
 	bool root_device_is_cmc;
+	bool irm_is_1394_1995_only;
 
 	spin_lock_irqsave(&card->lock, flags);
 

@@ -248,12 +249,18 @@ static void fw_card_bm_work(struct work_
 	}
 
 	generation = card->generation;
+
 	root_node = card->root_node;
 	fw_node_get(root_node);
 	root_device = root_node->data;
 	root_device_is_running = root_device &&
 			atomic_read(&root_device->state) == FW_DEVICE_RUNNING;
 	root_device_is_cmc = root_device && root_device->cmc;
+
+	irm_device = card->irm_node->data;
+	irm_is_1394_1995_only = irm_device && irm_device->config_rom &&
+			(irm_device->config_rom[2] & 0x000000f0) == 0;
+
 	root_id  = root_node->node_id;
 	irm_id   = card->irm_node->node_id;
 	local_id = card->local_node->node_id;

@@ -276,8 +283,15 @@ static void fw_card_bm_work(struct work_
 
 		if (!card->irm_node->link_on) {
 			new_root_id = local_id;
-			fw_notify("IRM has link off, making local node (%02x) root.\n",
-				  new_root_id);
+			fw_notify("%s, making local node (%02x) root.\n",
+				  "IRM has link off", new_root_id);
+			goto pick_me;
+		}
+
+		if (irm_is_1394_1995_only) {
+			new_root_id = local_id;
+			fw_notify("%s, making local node (%02x) root.\n",
+				  "IRM is not 1394a compliant", new_root_id);
 			goto pick_me;
 		}
 

@@ -316,8 +330,8 @@ static void fw_card_bm_work(struct work_
 			 * root, and thus, IRM.
 			 */
 			new_root_id = local_id;
-			fw_notify("BM lock failed, making local node (%02x) root.\n",
-				  new_root_id);
+			fw_notify("%s, making local node (%02x) root.\n",
+				  "BM lock failed", new_root_id);
 			goto pick_me;
 		}
 	} else if (card->bm_generation != generation) {

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

While I don't have an IRM that features the DCR-TRV25 bug, I did at least quick tests of the patch with the following consistent results:
- Joined three nodes to a bus, only one node of them IRM capable,

    but being a 1394-1995-only implementation.

- Plugged in Linux node into this bus, got the following log:
firewire_ohci: isochronous cycle inconsistent
firewire_core: created device fw8: GUID 00d04b21010212c8, S400
firewire_core: created device fw9: GUID 008088030960484b, S100
firewire_core: phy config: card 4, new root=ffc3, gap_count=7
firewire_core: IRM is not 1394a compliant, making local node (ffc1) root.
firewire_core: phy config: card 4, new root=ffc1, gap_count=7
and all is well as far as kernel, gscanbus, and Kino can tell.

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

Either this or the sources of the Debian kernel that you are currently running. The latter may perhaps be preferable for an easier switch between distributed kernel and patched kernel.

Alas not.
(I could build such a package if I had Debian myself, but I don't.)

OK; the patching as additional step is going to be the easiest one.

A quick web search turned up the following pointers for using a custom kernel on Debian:
http://www.debian.org/doc/FAQ/ch-kernel.en.html
http://www.debian.org/releases/stable/i386/ch08s06.html.en
(I hope the described method provides an already configured kernel source tree with Debian's configuration as baseline.)

Before you start compilation, save the email with the patch somewhere, change into the top-level directory of the kernel sources, and apply the patch with "patch -p1 < /the/path/to/the/email".

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

Well, good news. The patch seems to be worked as you designed.

Compiled the kernel with your patch, rebooted the machine and plugged in the camcorder to the firewire port. This is what the log said:

May 31 11:06:47 red kernel: [  924.228439] firewire_ohci: isochronous
cycle inconsistent
May 31 11:06:48 red kernel: [  924.821383] firewire_core: created device
fw1: GUID 08004601024aca36, S100
May 31 11:06:48 red kernel: [  924.821396] firewire_core: phy config:
card 0, new root=ffc1, gap_count=5
May 31 11:06:48 red kernel: [  924.833768] firewire_core: IRM is not
1394a compliant, making local node (ffc0) root.
May 31 11:06:48 red kernel: [  924.833777] firewire_core: phy config:

card 0, new root=ffc0, gap_count=5

And that is all it said ever after I captured a footage of some seconds. I used the following command:
$> dvgrab -a -f raw -t video-

Then I switched off the camera, and still there were no messages in the log.

So, what is verdict?

Meanwhile, I will continue using this kernel.

Thanks.

Please reply to this list only. I read this list on its corresponding
newsgroup on gmane.org. Replies sent to my email address are just
filtered to a folder in my mailbox and get periodically deleted without
ever having been read.

I committed the patch to linux1394-2.6.git now, exposed it in the linux-next branch, and will send a pull request sometime next week.

I marked the commit to be back-merged into the stable kernel branches (2.6.32.y and newer). Distributors will pick it up from there at their leisure, or earlier if they get a distro bug report that points them to the commit.

Thanks for testing the patched kernel.

I suppose either the local node became root node (node with highest node ID) and thus the camcorder stopped doing bus management things, or the camcorder chose the optimum gap count 5 for some reason and thus the Linux node saw no reason to do its bus management routine. If you are very curious, you can look at node IDs and gap counts and more with the FireWire inspection utility gscanbus.

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

Great! I am glad I was able to help.

Okay. Thanks again for the explanations.

Warm regards.

Please reply to this list only. I read this list on its corresponding
newsgroup on gmane.org. Replies sent to my email address are just
filtered to a folder in my mailbox and get periodically deleted without
ever having been read.

Regression since commit 10389536742c, "firewire: core: check for 1394a compliant IRM, fix inaccessibility of Sony camcorder":

The camcorder Canon MV5i generates lots of bus resets when asynchronous requests are sent to it (e.g. Config ROM read requests or FCP Command write requests) if the camcorder is not root node. This causes drop- outs in videos or makes the camcorder entirely inaccessible.
https://bugzilla.redhat.com/show_bug.cgi?id=633260

Fix this by allowing any Canon device, even if it is a pre-1394a IRM like MV5i are, to remain root node (if it is at least Cycle Master capable). With the FireWire controller cards that I tested, MV5i always becomes root node when plugged in and left to its own devices.

---
 drivers/firewire/core-card.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Index: b/drivers/firewire/core-card.c
===================================================================
@@ -75,6 +75,8 @@ static size_t config_rom_length = 1 + 4 
 #define BIB_IRMC		((1) << 31)
 #define NODE_CAPABILITIES	0x0c0083c0 /* per IEEE 1394 clause 8.3.2.6.5.2 */
 
+#define CANON_OUI		0x000085
+
 static void generate_config_rom(struct fw_card *card, __be32 *config_rom)
 {
 	struct fw_descriptor *desc;

@@ -284,6 +286,7 @@ static void bm_work(struct work_struct *
 	bool root_device_is_running;
 	bool root_device_is_cmc;
 	bool irm_is_1394_1995_only;
+	bool keep_this_irm;
 
 	spin_lock_irq(&card->lock);
 

@@ -305,6 +308,10 @@ static void bm_work(struct work_struct *
 	irm_is_1394_1995_only = irm_device && irm_device->config_rom &&
 			(irm_device->config_rom[2] & 0x000000f0) == 0;
 
+	/* Canon MV5i works unreliably if it is not root node. */
+	keep_this_irm = irm_device && irm_device->config_rom &&
+			irm_device->config_rom[3] >> 8 == CANON_OUI;
+
 	root_id  = root_node->node_id;
 	irm_id   = card->irm_node->node_id;
 	local_id = card->local_node->node_id;

@@ -333,7 +340,7 @@ static void bm_work(struct work_struct *
 			goto pick_me;
 		}
 
-		if (irm_is_1394_1995_only) {
+		if (irm_is_1394_1995_only && !keep_this_irm) {
 			new_root_id = local_id;
 			fw_notify("%s, making local node (%02x) root.\n",
 				  "IRM is not 1394a compliant", new_root_id);

@@ -382,7 +389,7 @@ static void bm_work(struct work_struct *
 
 		spin_lock_irq(&card->lock);
 
-		if (rcode != RCODE_COMPLETE) {
+		if (rcode != RCODE_COMPLETE && !keep_this_irm) {
 			/*
 			 * The lock request failed, maybe the IRM
 			 * isn't really IRM capable after all. Let's

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