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:
> > 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...
> > 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:
> > > +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;);
> > > - /* 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
-
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
