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

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

[Stefan Richter]

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.

Reported-by: Clemens Ladisch <clemens@ladisch.de> (missing 1394a check)
Reported-by: H. S. (issue with Sony DCR-TRV25)
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---

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
===================================================================
--- a/drivers/firewire/core-card.c
+++ 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/

Re: [PATCH] firewire: core: check for 1394a compliant IRM, fix inaccessibility of Sony camcorder

[Stefan Richter]

> 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.

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/

Re: [PATCH] firewire: core: check for 1394a compliant IRM, fix inaccessibility of Sony camcorder

[Stefan Richter]

H.S. wrote:
> Stefan Richter wrote:
> <SNIP>
>> 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?
> 
> Yes, I can give it a shot. Please pardon this basic question, should I
> be downloading the vanilla kernel source, patching it, compiling it and
> then testing it?

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.

> Or is there a precompiled binary available for download
> (i368 or amd64) with this patch included?

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

> I admit I have never patched
> and compiled a kernel before for testing a patch. I have, however,
> compiled kernels numerous times quite a while ago (just for learning
> objectives).

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/

Re: [PATCH] firewire: core: check for 1394a compliant IRM, fix inaccessibility of Sony camcorder

[H.S.]

H.S. wrote:
> 
> I will report back how it goes as soon as I am able to do the experiments.
> 


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.

Re: [PATCH] firewire: core: check for 1394a compliant IRM, fix inaccessibility of Sony camcorder

[Stefan Richter]

H.S. wrote:
> H.S. wrote:
>> So, what is verdict?
> 
> Should have been "So, what is the verdict?"

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.

> Anyway, just wanted to give a little additional information, in case
> this is significant. While testing the camcorder on a different Debian
> machine running Unstable and 2.6.32-5-686-bigmem kernel, I did not face
> the face problem I reported earlier. On this new machine, I was able to
> connect the camera and grab a footage using dvgrab. The syslog had this
> in it:
> May 30 19:31:42 blue kernel: [ 5203.926694] firewire_core: skipped bus generations, destroying all nodes
> May 30 19:31:42 blue kernel: [ 5204.424019] firewire_core: rediscovered device fw0
> May 30 19:31:42 blue kernel: [ 5204.516343] firewire_core: created device fw1: GUID 08004601024aca36, S100
> May 30 19:31:42 blue kernel: [ 5204.516346] firewire_core: phy config: card 0, new root=ffc1, gap_count=5
> May 30 19:31:42 blue kernel: [ 5204.525209] firewire_core: skipped bus generations, destroying all nodes
> May 30 19:31:43 blue kernel: [ 5205.024019] firewire_core: rediscovered device fw0
> May 30 19:31:43 blue kernel: [ 5205.116912] firewire_core: rediscovered device fw1

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/

Re: [PATCH] firewire: core: check for 1394a compliant IRM, fix inaccessibility of Sony camcorder

[H.S.]

On 02/06/10 02:26 PM, Stefan Richter wrote:
> 
> 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.

Great! I am glad I was able to help.

> 
> 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.


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.

[PATCH] firewire: core: fix unstable I/O with Canon camcorder

[Stefan Richter]

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.

Reported-by: Ralf Lange
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: <stable@kernel.org> # 2.6.32.y and newer
---
 drivers/firewire/core-card.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Index: b/drivers/firewire/core-card.c
===================================================================
--- a/drivers/firewire/core-card.c
+++ 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/