On Tue, 1 May 2007, Samuel Ortiz wrote:

But I will definitely spend some time this week running my IrDA stack 
with this patch applied. I didn't bother to do that earlier as you first 
reported some oops with this patch applied.

I think, what I reported was not an Oops, but the race that we're also 
fixing ATM - the one in the state machine. So, unrelated.

On Mon, Apr 30, 2007 at 03:24:05PM +0200, Guennadi Liakhovetski wrote:

quoted

in irttp_dup() (remember spinlock_init()?:-)), otherwise it oopses.

good catch, again...Yes, I do remember the irttp_dup bug ;-)

I've put a tsap_init() function that does those common initialisation 
calls, so we have a smaller chance of forgetting the dup() path next 
time...

quoted

I will be 
testing it too, but don't know how much longer and how intensively. Do you 
think we might get some new problems with this new context?

It seems quite safe to me. But more importantly, I thing we do want to call
the flow indication routine asynchronously in that specific case. 
There is one thing that this patch is missing though: we should probably
clean the worqueue right before we destroy the TTP instance. The work routine
checks for NULL pointer, but still...

I thought about it too, but the only thing you can do is 
flush_scheduled_work(), is this what you mean?

The test with your patch stopped inside a ftp transfer - the ftp client 
was doing a get, and it stopped half-way through. On the client side only 
the control tcp connection was still open, on the client both ftp-server 
forked children were dead, no connection open. No errors in logs. Weird. 
With my "ugly" patch it ran the weekend through.

...and it just stopped again - this time after just 13 minutes.

A couple of comments to your patch:

quoted

quoted

+static void irttp_flow_restart(struct work_struct *work)
+{
+	struct tsap_cb * self =
+		container_of(work, struct tsap_cb, irnet_flow_work);
+
+	if (self == NULL)
+		return;

self will not be NULL here. How about

+	IRDA_ASSERT(work != NULL, return;);
+	IRDA_ASSERT(self->magic == TTP_TSAP_MAGIC, return;);

quoted

quoted

-	/* Check if we can accept more frames from client.
-	 * We don't want to wait until the todo timer to do that, and we
-	 * can't use tasklets (grr...), so we are obliged to give control
-	 * to client. That's ok, this test will be true not too often
-	 * (max once per LAP window) and we are called from places
-	 * where we can spend a bit of time doing stuff. - Jean II */
 	if ((self->tx_sdu_busy) &&
 	    (skb_queue_len(&self->tx_queue) < TTP_TX_LOW_THRESHOLD) &&
 	    (!self->close_pend))

Is this test still needed here? You do it again in the work-function, and 
the conditions can change, so, should we schedule_work unconditionally 
here?

Thanks
Guennadi
---------------------------------
Guennadi Liakhovetski, Ph.D.
DSA Daten- und Systemtechnik GmbH
Pascalstr. 28
D-52076 Aachen
Germany