[PATCH] firewire: wait until PHY configuration packet was transmitted

[PATCH] firewire: wait until PHY configuration packet was transmitted

[Stefan Richter]

We now exit fw_send_phy_config /after/ the PHY config packet has been
transmitted, instead of before.  A subsequent fw_core_initiate_bus_reset
will therefore not overlap with the transmission.  This is meant to make
the send PHY config packet + reset bus routine more deterministic.

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

Index: linux/drivers/firewire/fw-transaction.c
===================================================================
--- linux.orig/drivers/firewire/fw-transaction.c
+++ linux/drivers/firewire/fw-transaction.c
@@ -18,6 +18,7 @@
  * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
  */
 
+#include <linux/completion.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -294,42 +295,40 @@ fw_send_request(struct fw_card *card, st
 }
 EXPORT_SYMBOL(fw_send_request);
 
+struct fw_phy_packet {
+	struct fw_packet packet;
+	struct completion done;
+};
+
 static void
 transmit_phy_packet_callback(struct fw_packet *packet,
 			     struct fw_card *card, int status)
 {
-	kfree(packet);
-}
-
-static void send_phy_packet(struct fw_card *card, u32 data, int generation)
-{
-	struct fw_packet *packet;
+	struct fw_phy_packet *p =
+			container_of(packet, struct fw_phy_packet, packet);
 
-	packet = kzalloc(sizeof(*packet), GFP_ATOMIC);
-	if (packet == NULL)
-		return;
-
-	packet->header[0] = data;
-	packet->header[1] = ~data;
-	packet->header_length = 8;
-	packet->payload_length = 0;
-	packet->speed = SCODE_100;
-	packet->generation = generation;
-	packet->callback = transmit_phy_packet_callback;
-
-	card->driver->send_request(card, packet);
+	complete(&p->done);
 }
 
 void fw_send_phy_config(struct fw_card *card,
 			int node_id, int generation, int gap_count)
 {
-	u32 q;
-
-	q = PHY_IDENTIFIER(PHY_PACKET_CONFIG) |
-		PHY_CONFIG_ROOT_ID(node_id) |
-		PHY_CONFIG_GAP_COUNT(gap_count);
+	struct fw_phy_packet p;
+	u32 data = PHY_IDENTIFIER(PHY_PACKET_CONFIG) |
+		   PHY_CONFIG_ROOT_ID(node_id) |
+		   PHY_CONFIG_GAP_COUNT(gap_count);
+
+	p.packet.header[0] = data;
+	p.packet.header[1] = ~data;
+	p.packet.header_length = 8;
+	p.packet.payload_length = 0;
+	p.packet.speed = SCODE_100;
+	p.packet.generation = generation;
+	p.packet.callback = transmit_phy_packet_callback;
+	init_completion(&p.done);
 
-	send_phy_packet(card, q, generation);
+	card->driver->send_request(card, &p.packet);
+	wait_for_completion(&p.done);
 }
 
 void fw_flush_transactions(struct fw_card *card)

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

Re: [PATCH] firewire: wait until PHY configuration packet was transmitted

[Jarod Wilson]

On Thursday 20 March 2008 06:48:23 pm Stefan Richter wrote:
> We now exit fw_send_phy_config /after/ the PHY config packet has been
> transmitted, instead of before.  A subsequent fw_core_initiate_bus_reset
> will therefore not overlap with the transmission.  This is meant to make
> the send PHY config packet + reset bus routine more deterministic.
>
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>

Will double-check Monday that this solves never-ending (until panic) bus 
resets with a jmicron controller in the office, but this does indeed prevent 
a very similar looking case with a via vt6307 ohci 1.0 controller + iogear 
hub + unibrain fire-i camera:

  http://bugzilla.kernel.org/show_bug.cgi?id=10128

Wondering if this will be the ticket for nForce2 controllers as well. Still 
trying to track one down to poke at myself, although we can probably find 
someone out there to test. Also wondering how we hold up against a bus reset 
storm initiated by another host, thinking maybe we can still trigger the 
list_add corruption in that bug, but stopping the endless bus reset loops is 
definitely an improvement.

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

-- 
Jarod Wilson
jwilson@redhat.com

Re: [PATCH] firewire: wait until PHY configuration packet was transmitted

[Stefan Richter]

Jarod Wilson wrote:
> Will double-check Monday that this solves never-ending (until panic) bus

s/that/whether/? :-)

> resets with a jmicron controller in the office, but this does indeed prevent 
> a very similar looking case with a via vt6307 ohci 1.0 controller + iogear 
> hub + unibrain fire-i camera:
> 
>   http://bugzilla.kernel.org/show_bug.cgi?id=10128

The wonders of brainstorming on IRC.  Let's implement all far-off and 
weird ideas we can possibly get, distill those which don't cause 
regressions, and we will end up with perfectly working drivers. :-)
-- 
Stefan Richter
-=====-==--- --== =-==-
http://arcgraph.de/sr/

Re: [PATCH] firewire: wait until PHY configuration packet was transmitted

[Jarod Wilson]

Stefan Richter wrote:
> Jarod Wilson wrote:
>> Will double-check Monday that this solves never-ending (until panic) bus
> 
> s/that/whether/? :-)

Either one works here. I went with "that", because I'm reasonably 
certain that it does solve the problem and I'll just be confirming it, 
vs. suspecting it might and finding out whether it does or not... Or 
something like that. :)

>> resets with a jmicron controller in the office, but this does indeed 
>> prevent a very similar looking case with a via vt6307 ohci 1.0 
>> controller + iogear hub + unibrain fire-i camera:
>>
>>   http://bugzilla.kernel.org/show_bug.cgi?id=10128
> 
> The wonders of brainstorming on IRC.  Let's implement all far-off and 
> weird ideas we can possibly get, distill those which don't cause 
> regressions, and we will end up with perfectly working drivers. :-)

Definitely! We've certainly been getting closer and closer, anyhow...

-- 
Jarod Wilson
jwilson@redhat.com

Re: [PATCH] firewire: wait until PHY configuration packet was transmitted

[Jarod Wilson]

On Saturday 22 March 2008 11:38:30 pm Jarod Wilson wrote:
> Stefan Richter wrote:
> > Jarod Wilson wrote:
> >> Will double-check Monday that this solves never-ending (until panic) bus
> >
> > s/that/whether/? :-)
>
> Either one works here. I went with "that", because I'm reasonably
> certain that it does solve the problem and I'll just be confirming it,
> vs. suspecting it might and finding out whether it does or not... Or
> something like that. :)
[...]
> >> resets with a jmicron controller in the office

And it does in fact solve the constant bus reset problem on the jmicron 
controller.

-- 
Jarod Wilson
jwilson@redhat.com