[PATCH 1/2] firewire: catch self_id_count == 0

[PATCH 1/2] firewire: catch self_id_count == 0

[Stefan Richter]

fw_core_handle_bus_reset() incorrectly relied on the assumption that
self_id_count > 0.  The added check should fix one aspect of
http://bugzilla.kernel.org/show_bug.cgi?id=10128

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

Index: linux/drivers/firewire/fw-topology.c
===================================================================
--- linux.orig/drivers/firewire/fw-topology.c
+++ linux/drivers/firewire/fw-topology.c
@@ -513,6 +513,11 @@ fw_core_handle_bus_reset(struct fw_card 
 
 	fw_flush_transactions(card);
 
+	if (self_id_count == 0) {
+		fw_destroy_nodes(card);
+		return;
+	}
+
 	spin_lock_irqsave(&card->lock, flags);
 
 	/*

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

[PATCH 2/2] firewire: insist on successive self ID complete events

[Stefan Richter]

The whole topology code only works if the old and new topologies which
are compared come from immediately successive self ID complete events.

If there happened bus resets without self ID complete events in the
meantime, or self ID complete events with invalid selfIDs, the topology
comparison could identify nodes wrongly, or more likely just corrupt
kernel memory or panic right away.

We new discard all nodes of the old topology and treat all current nodes
as new ones if the current self ID generation is not the previous one
plus 1.  This should fix another aspect of
http://bugzilla.kernel.org/show_bug.cgi?id=10128

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

Index: linux/drivers/firewire/fw-topology.c
===================================================================
--- linux.orig/drivers/firewire/fw-topology.c
+++ linux/drivers/firewire/fw-topology.c
@@ -518,6 +518,17 @@ fw_core_handle_bus_reset(struct fw_card 
 		return;
 	}
 
+	/*
+	 * If the selfID buffer is not the immediate successor of the
+	 * previously processed one, we cannot reliably compare the
+	 * old and new topologies.
+	 */
+	if ((generation & 0xff) != ((card->generation + 1) & 0xff)) {
+		fw_notify("skipped bus generations, destroying all nodes\n");
+		fw_destroy_nodes(card);
+		card->bm_retries = 0;
+	}
+
 	spin_lock_irqsave(&card->lock, flags);
 
 	/*

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

Re: [PATCH 1/2] firewire: catch self_id_count == 0

[Stefan Richter]

I wrote:
> --- linux.orig/drivers/firewire/fw-topology.c
> +++ linux/drivers/firewire/fw-topology.c
> @@ -513,6 +513,11 @@ fw_core_handle_bus_reset(struct fw_card 
>  
>  	fw_flush_transactions(card);
>  
> +	if (self_id_count == 0) {
> +		fw_destroy_nodes(card);
> +		return;
> +	}

We may also want to update some other card-> members, pull the topology
map down, and whatnot...
-- 
Stefan Richter
-=====-==--- --== =--==
http://arcgraph.de/sr/

Re: [PATCH 1/2] firewire: catch self_id_count == 0

[Stefan Richter]

> I wrote:
>> --- linux.orig/drivers/firewire/fw-topology.c
>> +++ linux/drivers/firewire/fw-topology.c
>> @@ -513,6 +513,11 @@ fw_core_handle_bus_reset(struct fw_card 
>>  
>>  	fw_flush_transactions(card);
>>  
>> +	if (self_id_count == 0) {
>> +		fw_destroy_nodes(card);
>> +		return;
>> +	}
> 
> We may also want to update some other card-> members, pull the topology
> map down, and whatnot...

Or instead of all this:
Don't call fw_core_handle_bus_reset at all if self_id_count == 0?
-- 
Stefan Richter
-=====-==--- --== =--==
http://arcgraph.de/sr/

[PATCH] firewire: fw-ohci: add self ID error check

[Stefan Richter]

Discard self ID buffer contents if
  - the selfIDError flag is set,
  - any of the self ID packets has bit errors.

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

Index: linux/drivers/firewire/fw-ohci.c
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.c
+++ linux/drivers/firewire/fw-ohci.c
@@ -1060,20 +1060,26 @@ static void bus_reset_tasklet(unsigned l
 	ohci->node_id = reg & (OHCI1394_NodeID_busNumber |
 			       OHCI1394_NodeID_nodeNumber);
 
+	reg = reg_read(ohci, OHCI1394_SelfIDCount);
+	if (reg & OHCI1394_SelfIDCount_selfIDError) {
+		fw_error("inconsistent self IDs\n");
+		return;
+	}
 	/*
 	 * The count in the SelfIDCount register is the number of
 	 * bytes in the self ID receive buffer.  Since we also receive
 	 * the inverted quadlets and a header quadlet, we shift one
 	 * bit extra to get the actual number of self IDs.
 	 */
-
-	self_id_count = (reg_read(ohci, OHCI1394_SelfIDCount) >> 3) & 0x3ff;
+	self_id_count = (reg >> 3) & 0x3ff;
 	generation = (cond_le32_to_cpu(ohci->self_id_cpu[0]) >> 16) & 0xff;
 	rmb();
 
 	for (i = 1, j = 0; j < self_id_count; i += 2, j++) {
-		if (ohci->self_id_cpu[i] != ~ohci->self_id_cpu[i + 1])
+		if (ohci->self_id_cpu[i] != ~ohci->self_id_cpu[i + 1]) {
 			fw_error("inconsistent self IDs\n");
+			return;
+		}
 		ohci->self_id_buffer[j] =
 				cond_le32_to_cpu(ohci->self_id_cpu[i]);
 	}
Index: linux/drivers/firewire/fw-ohci.h
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.h
+++ linux/drivers/firewire/fw-ohci.h
@@ -30,6 +30,7 @@
 #define  OHCI1394_HCControl_softReset		0x00010000
 #define OHCI1394_SelfIDBuffer                 0x064
 #define OHCI1394_SelfIDCount                  0x068
+#define  OHCI1394_SelfIDCount_selfIDError	0x80000000
 #define OHCI1394_IRMultiChanMaskHiSet         0x070
 #define OHCI1394_IRMultiChanMaskHiClear       0x074
 #define OHCI1394_IRMultiChanMaskLoSet         0x078

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

Re: [PATCH] firewire: fw-ohci: add self ID error check

[Stefan Richter]

On 19 Mar, Stefan Richter wrote:
> Discard self ID buffer contents if
>   - the selfIDError flag is set,
>   - any of the self ID packets has bit errors.
> 
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> ---
>  drivers/firewire/fw-ohci.c |   12 +++++++++---
>  drivers/firewire/fw-ohci.h |    1 +
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> Index: linux/drivers/firewire/fw-ohci.c
> ===================================================================
> --- linux.orig/drivers/firewire/fw-ohci.c
> +++ linux/drivers/firewire/fw-ohci.c
> @@ -1060,20 +1060,26 @@ static void bus_reset_tasklet(unsigned l
>  	ohci->node_id = reg & (OHCI1394_NodeID_busNumber |
>  			       OHCI1394_NodeID_nodeNumber);
>  
> +	reg = reg_read(ohci, OHCI1394_SelfIDCount);
> +	if (reg & OHCI1394_SelfIDCount_selfIDError) {
> +		fw_error("inconsistent self IDs\n");
> +		return;
> +	}
>  	/*
>  	 * The count in the SelfIDCount register is the number of
>  	 * bytes in the self ID receive buffer.  Since we also receive
>  	 * the inverted quadlets and a header quadlet, we shift one
>  	 * bit extra to get the actual number of self IDs.
>  	 */
> -
> -	self_id_count = (reg_read(ohci, OHCI1394_SelfIDCount) >> 3) & 0x3ff;
> +	self_id_count = (reg >> 3) & 0x3ff;
>  	generation = (cond_le32_to_cpu(ohci->self_id_cpu[0]) >> 16) & 0xff;
>  	rmb();
>  
>  	for (i = 1, j = 0; j < self_id_count; i += 2, j++) {
> -		if (ohci->self_id_cpu[i] != ~ohci->self_id_cpu[i + 1])
> +		if (ohci->self_id_cpu[i] != ~ohci->self_id_cpu[i + 1]) {
>  			fw_error("inconsistent self IDs\n");
> +			return;
> +		}
>  		ohci->self_id_buffer[j] =
>  				cond_le32_to_cpu(ohci->self_id_cpu[i]);
>  	}
> Index: linux/drivers/firewire/fw-ohci.h
> ===================================================================
> --- linux.orig/drivers/firewire/fw-ohci.h
> +++ linux/drivers/firewire/fw-ohci.h
> @@ -30,6 +30,7 @@
>  #define  OHCI1394_HCControl_softReset		0x00010000
>  #define OHCI1394_SelfIDBuffer                 0x064
>  #define OHCI1394_SelfIDCount                  0x068
> +#define  OHCI1394_SelfIDCount_selfIDError	0x80000000
>  #define OHCI1394_IRMultiChanMaskHiSet         0x070
>  #define OHCI1394_IRMultiChanMaskHiClear       0x074
>  #define OHCI1394_IRMultiChanMaskLoSet         0x078
> 

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

[PATCH update] firewire: fw-ohci: add self ID error check

[Stefan Richter]

On 19 Mar, Stefan Richter wrote:
> On 19 Mar, Stefan Richter wrote:
[slippery mouse button... actual reply follows now]


From: Stefan Richter <stefanr@s5r6.in-berlin.de>
Subject: firewire: fw-ohci: add self ID error check

Discard self ID buffer contents if
  - the selfIDError flag is set,
  - any of the self ID packets has bit errors.

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

Update:  Demote log level from KERN_ERR to KERN_NOTICE.

 drivers/firewire/fw-ohci.c |   14 ++++++++++----
 drivers/firewire/fw-ohci.h |    1 +
 2 files changed, 11 insertions(+), 4 deletions(-)

Index: linux/drivers/firewire/fw-ohci.c
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.c
+++ linux/drivers/firewire/fw-ohci.c
@@ -1060,20 +1060,26 @@ static void bus_reset_tasklet(unsigned l
 	ohci->node_id = reg & (OHCI1394_NodeID_busNumber |
 			       OHCI1394_NodeID_nodeNumber);
 
+	reg = reg_read(ohci, OHCI1394_SelfIDCount);
+	if (reg & OHCI1394_SelfIDCount_selfIDError) {
+		fw_notify("inconsistent self IDs\n");
+		return;
+	}
 	/*
 	 * The count in the SelfIDCount register is the number of
 	 * bytes in the self ID receive buffer.  Since we also receive
 	 * the inverted quadlets and a header quadlet, we shift one
 	 * bit extra to get the actual number of self IDs.
 	 */
-
-	self_id_count = (reg_read(ohci, OHCI1394_SelfIDCount) >> 3) & 0x3ff;
+	self_id_count = (reg >> 3) & 0x3ff;
 	generation = (cond_le32_to_cpu(ohci->self_id_cpu[0]) >> 16) & 0xff;
 	rmb();
 
 	for (i = 1, j = 0; j < self_id_count; i += 2, j++) {
-		if (ohci->self_id_cpu[i] != ~ohci->self_id_cpu[i + 1])
-			fw_error("inconsistent self IDs\n");
+		if (ohci->self_id_cpu[i] != ~ohci->self_id_cpu[i + 1]) {
+			fw_notify("inconsistent self IDs\n");
+			return;
+		}
 		ohci->self_id_buffer[j] =
 				cond_le32_to_cpu(ohci->self_id_cpu[i]);
 	}
Index: linux/drivers/firewire/fw-ohci.h
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.h
+++ linux/drivers/firewire/fw-ohci.h
@@ -30,6 +30,7 @@
 #define  OHCI1394_HCControl_softReset		0x00010000
 #define OHCI1394_SelfIDBuffer                 0x064
 #define OHCI1394_SelfIDCount                  0x068
+#define  OHCI1394_SelfIDCount_selfIDError	0x80000000
 #define OHCI1394_IRMultiChanMaskHiSet         0x070
 #define OHCI1394_IRMultiChanMaskHiClear       0x074
 #define OHCI1394_IRMultiChanMaskLoSet         0x078


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

[PATCH 2/2 update] firewire: insist on successive self ID complete events

[Stefan Richter]

The whole topology code only works if the old and new topologies which
are compared come from immediately successive self ID complete events.

If there happened bus resets without self ID complete events in the
meantime, or self ID complete events with invalid selfIDs, the topology
comparison could identify nodes wrongly, or more likely just corrupt
kernel memory or panic right away.

We new discard all nodes of the old topology and treat all current nodes
as new ones if the current self ID generation is not the previous one
plus 1.

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

Update:
  - Doesn't help with http://bugzilla.kernel.org/show_bug.cgi?id=10128.
  - Suppress spurious "destroying all nodes" if there are none, in
    particular when loading the driver.

 drivers/firewire/fw-topology.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Index: linux/drivers/firewire/fw-topology.c
===================================================================
--- linux.orig/drivers/firewire/fw-topology.c
+++ linux/drivers/firewire/fw-topology.c
@@ -513,6 +513,18 @@ fw_core_handle_bus_reset(struct fw_card 
 
 	fw_flush_transactions(card);
 
+	/*
+	 * If the selfID buffer is not the immediate successor of the
+	 * previously processed one, we cannot reliably compare the
+	 * old and new topologies.
+	 */
+	if ((generation & 0xff) != ((card->generation + 1) & 0xff) &&
+	    card->local_node != NULL) {
+		fw_notify("skipped bus generations, destroying all nodes\n");
+		fw_destroy_nodes(card);
+		card->bm_retries = 0;
+	}
+
 	spin_lock_irqsave(&card->lock, flags);
 
 	/*

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

[PATCH] firewire: fw-ohci: catch self_id_count == 0

[Stefan Richter]

fw_core_handle_bus_reset() incorrectly relied on the assumption that
self_id_count > 0.

We check early in fw-ohci and discard the self ID complete event if
self_id_count == 0 because a valid event always has at least one self ID
packet in it (the one of the local node).  Hence treat self_id_count ==
0 like any other kind of invalid self ID buffer.

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

Applies after patch "firewire: fw-ohci: add self ID error check".
Supersedes patch "firewire: catch self_id_count == 0".

 drivers/firewire/fw-ohci.c |    4 ++++
 1 file changed, 4 insertions(+)

Index: linux/drivers/firewire/fw-ohci.c
===================================================================
--- linux.orig/drivers/firewire/fw-ohci.c
+++ linux/drivers/firewire/fw-ohci.c
@@ -1072,6 +1072,10 @@ static void bus_reset_tasklet(unsigned l
 	 * bit extra to get the actual number of self IDs.
 	 */
 	self_id_count = (reg >> 3) & 0x3ff;
+	if (self_id_count == 0) {
+		fw_notify("inconsistent self IDs\n");
+		return;
+	}
 	generation = (cond_le32_to_cpu(ohci->self_id_cpu[0]) >> 16) & 0xff;
 	rmb();
 

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

Re: [PATCH] firewire: fw-ohci: catch self_id_count == 0

[Stefan Richter]

Stefan Richter wrote:
> fw_core_handle_bus_reset() incorrectly relied on the assumption that
> self_id_count > 0.

Even though
     firewire: insist on successive self ID complete events
     firewire: fw-ohci: add self ID error check
     firewire: fw-ohci: catch self_id_count == 0
didn't fix any of Jarod's weird setups, I plan on submitting these 
patches nevertheless (after 2.6.25) because they IMO fix real 
possibilities for panics in the topology code.
-- 
Stefan Richter
-=====-==--- --== =-==-
http://arcgraph.de/sr/

Re: [PATCH] firewire: fw-ohci: catch self_id_count == 0

[Jarod Wilson]

Stefan Richter wrote:
> Stefan Richter wrote:
>> fw_core_handle_bus_reset() incorrectly relied on the assumption that
>> self_id_count > 0.
> 
> Even though
>     firewire: insist on successive self ID complete events
>     firewire: fw-ohci: add self ID error check
>     firewire: fw-ohci: catch self_id_count == 0
> didn't fix any of Jarod's weird setups, I plan on submitting these 
> patches nevertheless (after 2.6.25) because they IMO fix real 
> possibilities for panics in the topology code.

Agreed, they all looked valid and didn't cause any regressions in my own 
prodding. So go ahead with all three:

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

-- 
Jarod Wilson
jwilson@redhat.com

Re: [PATCH 2/2 update] firewire: insist on successive self ID complete events

[Stefan Richter]

I wrote on 2008-03-19:
> The whole topology code only works if the old and new topologies which
> are compared come from immediately successive self ID complete events.
> 
> If there happened bus resets without self ID complete events in the
> meantime, or self ID complete events with invalid selfIDs, the topology
> comparison could identify nodes wrongly, or more likely just corrupt
> kernel memory or panic right away.
> 
> We new discard all nodes of the old topology and treat all current nodes
> as new ones if the current self ID generation is not the previous one
> plus 1.
[...]
> --- linux.orig/drivers/firewire/fw-topology.c
> +++ linux/drivers/firewire/fw-topology.c
> @@ -513,6 +513,18 @@ fw_core_handle_bus_reset(struct fw_card 
>  
>  	fw_flush_transactions(card);
>  
> +	/*
> +	 * If the selfID buffer is not the immediate successor of the
> +	 * previously processed one, we cannot reliably compare the
> +	 * old and new topologies.
> +	 */
> +	if ((generation & 0xff) != ((card->generation + 1) & 0xff) &&
> +	    card->local_node != NULL) {
> +		fw_notify("skipped bus generations, destroying all nodes\n");
> +		fw_destroy_nodes(card);
> +		card->bm_retries = 0;
> +	}
> +
>  	spin_lock_irqsave(&card->lock, flags);
>  
>  	/*
> 

Some a-posteriori thoughts:

Situations like this happen quite regularly when camcorders are plugged 
in or out or switched on or off, or when bus-powered hubs are plugged 
in, and similar situations --- depending on the PHYs on the bus.

The conclusion that we have to discard the old topology data in this 
situation still stands.

However, although we have to discard node data, we should not discard 
_device_ data.  Chances are that some devices remained on the bus.  At 
least the local node's device will obviously still be there.

Destroying the device representations (and recreating them)
   - causes unnecessary terminal connection loss for userspace drivers,
   - causes unnecessary terminal connection loss to SBP-2 devices with
     the possible result of data loss,
   - will unnecessarily disturb hypothetical future kernelspace firewire
     drivers.  (There will be at least one more of those eventually, i.e.
     IP over 1394.)
   - is a regression relative to the current firewire-core and relative
     to ieee1394.

I don't think it will be hard to prevent the premature destruction of 
device representations.  I will work on it soon and am holding off 
upstream submission of the above quoted patch until I have the device 
preserving code implemented an tested.

Until then, firewire-core will keep panicking in rare border cases due 
to bogus topology comparisons.¹  But it will on the other hand not lose 
connection in the not quite uncommon situations outlined above.

¹) There is no proof yet that it does, but there are some suspicious 
reports.  And I don't remember any panics by hotplugging anymore since I 
am using the above patch myself.
-- 
Stefan Richter
-=====-==--- -=-- =--=-
http://arcgraph.de/sr/