[Guennadi Liakhovetski <gl@ds...>]

On Fri, 27 Apr 2007, Samuel Ortiz wrote:
> After looking a bit more carefully at it, I have a remark regarding this
> patch:
> 
> > > diff -p -u -r1.1.1.8.4.1 irlap_frame.c
> > > --- a/net/irda/irlap_frame.c	2 Apr 2007 10:36:28 -0000	1.1.1.8.4.1
> > > +++ b/net/irda/irlap_frame.c	19 Apr 2007 13:20:36 -0000
> > > @@ -796,6 +796,7 @@ void irlap_send_data_primary_poll(struct
> > >  		self->vs = (self->vs + 1) % 8;
> > >  		self->ack_required = FALSE;
> > >  
> > > +		irlap_next_state(self, LAP_NRM_P);
> > >  		irlap_send_i_frame(self, tx_skb, CMD_FRAME);
> > >  	} else {
> > >  		IRDA_DEBUG(4, "%s(), sending unreliable frame\n", __FUNCTION__);
> > > @@ -808,6 +809,7 @@ void irlap_send_data_primary_poll(struct
> > >  			skb->data[1] |= PF_BIT;
> > >  			irlap_send_ui_frame(self, skb_get(skb), self->caddr, CMD_FRAME);
> > >  		}
> > > +		irlap_next_state(self, LAP_NRM_P);
> > >  	}
> Here you're calling irlap_next_state() after the last frame has been sent.
> If irlap_send_data_primary_poll() gets preempted, and we receive the peer's
> answer before we call irlap_next_state(), then we may hit the same bug you are
> trying to fix.

Yes, I know. It was a deliberate decision. I couldn't reproduce a race in 
those branches, and, therefore couldn't test any fix. So, I preferred to 
go the "least impact" way.

So, what is the logic we are trying to implement now? Set the LAP_NRM_P 
state just before sending the packet with the pf-bit? Or can we set it as 
long as we know that we're about to send such a packet?

I.e., would it be safe maybe to just put the call to 
irlap_next_state(self, LAP_NRM_P); just after 
del_timer(&self->poll_timer); (btw, shouldn't it be del_timer_sync()? But 
it can also be called from a softirq, so, perhaps not) or maybe even just 
put it above the call to irlap_send_data_primary_poll(self, skb); in 
irlap_state_xmit_p()? In fact, as I now look at this patch again, I've 
already changed the behaviour in the error case with it: if tx_skb = 
skb_clone(skb, GFP_ATOMIC); fails. Before, it would fail cloning the skb, 
wouldn't send the frame, and still just set the state to LAP_NRM_P and 
just hang there indefinitely. Now it aborts and stays in the LAP_XMIT_P 
state, which is correct?

> This is how I'd see it, let me know if you're ok with it:
> 
> diff --git a/net/irda/irlap_frame.c b/net/irda/irlap_frame.c
> index 3c5a68e..3013c49 100644
> --- a/net/irda/irlap_frame.c
> +++ b/net/irda/irlap_frame.c
> @@ -798,16 +798,19 @@ void irlap_send_data_primary_poll(struct irlap_cb *self, struct sk_buff *skb)
>  		self->vs = (self->vs + 1) % 8;
>  		self->ack_required = FALSE;
>  
> +		irlap_next_state(self, LAP_NRM_P);
>  		irlap_send_i_frame(self, tx_skb, CMD_FRAME);
>  	} else {
>  		IRDA_DEBUG(4, "%s(), sending unreliable frame\n", __FUNCTION__);
>  
>  		if (self->ack_required) {
>  			irlap_send_ui_frame(self, skb_get(skb), self->caddr, CMD_FRAME);
> +			irlap_next_state(self, LAP_NRM_P);
>  			irlap_send_rr_frame(self, CMD_FRAME);
>  			self->ack_required = FALSE;
>  		} else {
>  			skb->data[1] |= PF_BIT;
> +			irlap_next_state(self, LAP_NRM_P);
>  			irlap_send_ui_frame(self, skb_get(skb), self->caddr, CMD_FRAME);
>  		}
>  	}

So, as I said, the whole IrDA stack is, well, non-trivial. After about 2 
months of almost full-time debugging it I still don't understand many 
things like how are all those numerous asynchronous contexts (timers / 
softirqs, user, irq, work-queues...) are locking the common data, etc.

Therefore, I would prefer to keep places that we cannot currently test as 
close to "unmodified" as possible. But if you are sure this change is 
correct, well, we can try it too.

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