Hi all,
On Sat, Feb 25, 2012 at 12:52:04PM -0300, Ulisses Furquim wrote:
> >> <snip>
> >>
> >> - if (bt_cb(skb)->retries == 1) {
> >> - chan->unacked_frames++;
> >> + l2cap_chan_hold(chan);
> >> + sock_hold(chan->sk);
> >> + tx_skb->sk = chan->sk;
> >>
> >> Do we really need these? Do we always have chan->sk? (We have that in
> >> l2cap_ertm_send() and l2cap_ertm_resend()).
> >
> > The upstream ERTM code still relies on having chan->sk, so I didn't try to
> > finish splitting channels & sockets within this patch. skb destructors
> > expect to have an sk pointer, so it is critical to modify these reference
> > counts so the socket and chan are around when the skb leaves the hci tx
> > queue.
>
> If I'm not mistaken the skb destructor relies on sk only to be able to
> access chan, right? It'd be good if we could not rely on sk here.
> Andrei, what do you think?
I believe that ERTM should not use sk. The places where we still use sk
shall be corrected IMO.
> >> - int ret;
> >> + struct l2cap_chan *chan =
> >> + container_of(work, struct l2cap_chan, tx_work);
> >>
> >> - if (!skb_queue_empty(&chan->tx_q))
> >> - chan->tx_send_head = chan->tx_q.next;
> >> + BT_DBG("%p", chan);
> >>
> >> - chan->next_tx_seq = chan->expected_ack_seq;
> >> - ret = l2cap_ertm_send(chan);
> >> - return ret;
> >> + lock_sock(chan->sk);
> >> + l2cap_ertm_send(chan);
> >> + release_sock(chan->sk);
> >>
> >> Can't we just use l2cap_chan_lock()/l2cap_chan_unlock() here? (I'm
> >> assuming you saw the patches creating a lock in l2cap_chan to protect
> >> it) Do we always have sk?
> >
> > l2cap_chan_lock() is pretty new! Yes, I should use that to guard the ERTM
> > state.
> >
> > Right now, we do still always have sk, but that is changing (as it should).
>
> Ok. If we could not rely on sk here as well would be good.
Good that we agree here.
Best regards
Andrei Emeltchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
