Re: [PATCHv1 1/2] Bluetooth: Add refcnt to l2cap_conn

[Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com>]

Hi Gustavo,

On Tue, Jul 24, 2012 at 08:26:18PM -0300, Gustavo Padovan wrote:
...
> > +static int l2cap_conn_put(struct l2cap_conn *conn)
> > +{
> > +	/* conn might be NULL, was checked before in l2cap_conn_del */
> > +	if (!conn) {
> > +		BT_DBG("conn == NULL");
> > +		return 1;
> 
> 1? What does that mean?

this would mean that conn == NULL but I will make this function void.

> > +	}
> > +
> > +	BT_DBG("conn %p orig refcnt %d", conn,
> > +	       atomic_read(&conn->kref.refcount));
> > +
> > +	return kref_put(&conn->kref, &l2cap_conn_destroy);
> > +}
> > +
> > +void l2cap_conn_set_timer(struct l2cap_conn *conn, struct delayed_work *work,
> > +			  long timeout)
> > +{
> > +	BT_DBG("conn %p timeout %ld", conn, timeout);
> > +
> > +	/* If delayed work cancelled do not hold(conn)
> > +	   since it is already done with previous set_timer */
> > +	if (!cancel_delayed_work(work))
> > +		l2cap_conn_get(conn);
> > +
> > +	schedule_delayed_work(work, timeout);
> > +}
> > +
> > +bool l2cap_conn_clear_timer(struct l2cap_conn *conn, struct delayed_work *work)
> > +{
> > +	bool ret;
> > +
> > +	BT_DBG("conn %p", conn);
> > +
> > +	/* put(conn) if delayed work cancelled otherwise it
> > +	   is done in delayed work function */
> > +	ret = cancel_delayed_work(work);
> > +	if (ret)
> > +		l2cap_conn_put(conn);
> > +
> > +	return ret;
> > +}
> >  
> >  /* ---- L2CAP channels ---- */
> >  
> > @@ -976,7 +1038,8 @@ static void l2cap_do_start(struct l2cap_chan *chan)
> >  		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_SENT;
> >  		conn->info_ident = l2cap_get_ident(conn);
> >  
> > -		schedule_delayed_work(&conn->info_timer, L2CAP_INFO_TIMEOUT);
> > +		l2cap_conn_set_timer(conn, &conn->info_timer,
> > +				     L2CAP_INFO_TIMEOUT);
> >  
> >  		l2cap_send_cmd(conn, conn->info_ident,
> >  					L2CAP_INFO_REQ, sizeof(req), &req);
> > @@ -1258,12 +1321,14 @@ static void l2cap_conn_unreliable(struct l2cap_conn *conn, int err)
> >  static void l2cap_info_timeout(struct work_struct *work)
> >  {
> >  	struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
> > -							info_timer.work);
> > +					       info_timer.work);
> >  
> >  	conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
> >  	conn->info_ident = 0;
> >  
> >  	l2cap_conn_start(conn);
> > +
> > +	l2cap_conn_put(conn);
> >  }
> >  
> >  static void l2cap_conn_del(struct hci_conn *hcon, int err)
> > @@ -1297,13 +1362,8 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
> >  
> >  	hci_chan_del(conn->hchan);
> >  
> > -	if (conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT)
> > -		cancel_delayed_work_sync(&conn->info_timer);
> > -
> > -	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags)) {
> > -		cancel_delayed_work_sync(&conn->security_timer);
> > +	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &hcon->flags))
> 
> So where are you cleaning these timers? I don't see it. You are change the
> code flow here.

Yes this change code flow. Those would be handled gracefully with refcounting.

> >  		smp_chan_destroy(conn);
> > -	}
> >  
> >  	hcon->l2cap_data = NULL;
> >  	kfree(conn);
> > @@ -1312,14 +1372,11 @@ static void l2cap_conn_del(struct hci_conn *hcon, int err)
> >  static void security_timeout(struct work_struct *work)
> >  {
> >  	struct l2cap_conn *conn = container_of(work, struct l2cap_conn,
> > -						security_timer.work);
> > +					       security_timer.work);
> >  
> >  	BT_DBG("conn %p", conn);
> >  
> > -	if (test_and_clear_bit(HCI_CONN_LE_SMP_PEND, &conn->hcon->flags)) {
> > -		smp_chan_destroy(conn);
> > -		l2cap_conn_del(conn->hcon, ETIMEDOUT);
> > -	}
> > +	l2cap_conn_put(conn);
> 
> You are loosing the error code here (and in some other places in the this
> patch). We need to carry the error code so we can do proper error report in
> teardown()

Maybe we need to set error code somewhere here.

> >  }
> >  
> >  static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
> > @@ -1361,6 +1418,8 @@ static struct l2cap_conn *l2cap_conn_add(struct hci_conn *hcon, u8 status)
> >  
> >  	INIT_LIST_HEAD(&conn->chan_l);
> >  
> > +	kref_init(&conn->kref);
> > +`
> >  	if (hcon->type == LE_LINK)
> >  		INIT_DELAYED_WORK(&conn->security_timer, security_timeout);
> >  	else
> > @@ -3318,8 +3377,8 @@ static inline int l2cap_command_rej(struct l2cap_conn *conn, struct l2cap_cmd_hd
> >  		return 0;
> >  
> >  	if ((conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_SENT) &&
> > -					cmd->ident == conn->info_ident) {
> > -		cancel_delayed_work(&conn->info_timer);
> > +	    cmd->ident == conn->info_ident) {
> > +		l2cap_conn_clear_timer(conn, &conn->info_timer);
> >  
> >  		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
> >  		conn->info_ident = 0;
> > @@ -3433,10 +3492,11 @@ sendresp:
> >  		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_SENT;
> >  		conn->info_ident = l2cap_get_ident(conn);
> >  
> > -		schedule_delayed_work(&conn->info_timer, L2CAP_INFO_TIMEOUT);
> > +		l2cap_conn_set_timer(conn, &conn->info_timer,
> > +				     L2CAP_INFO_TIMEOUT);
> >  
> >  		l2cap_send_cmd(conn, conn->info_ident,
> > -					L2CAP_INFO_REQ, sizeof(info), &info);
> > +			       L2CAP_INFO_REQ, sizeof(info), &info);
> >  	}
> >  
> >  	if (chan && !test_bit(CONF_REQ_SENT, &chan->conf_state) &&
> > @@ -3889,7 +3949,7 @@ static inline int l2cap_information_rsp(struct l2cap_conn *conn, struct l2cap_cm
> >  			conn->info_state & L2CAP_INFO_FEAT_MASK_REQ_DONE)
> >  		return 0;
> >  
> > -	cancel_delayed_work(&conn->info_timer);
> > +	l2cap_conn_clear_timer(conn, &conn->info_timer);
> >  
> >  	if (result != L2CAP_IR_SUCCESS) {
> >  		conn->info_state |= L2CAP_INFO_FEAT_MASK_REQ_DONE;
> > @@ -5277,8 +5337,10 @@ static void l2cap_recv_frame(struct l2cap_conn *conn, struct sk_buff *skb)
> >  		break;
> >  
> >  	case L2CAP_CID_SMP:
> > -		if (smp_sig_channel(conn, skb))
> > -			l2cap_conn_del(conn->hcon, EACCES);
> > +		if (smp_sig_channel(conn, skb)) {
> > +			l2cap_conn_clear_timer(conn, &conn->security_timer);
> > +			l2cap_conn_put(conn);
> > +		}
> >  		break;
> >  
> >  	default:
> > @@ -5330,8 +5392,14 @@ int l2cap_connect_cfm(struct hci_conn *hcon, u8 status)
> >  		conn = l2cap_conn_add(hcon, status);
> >  		if (conn)
> >  			l2cap_conn_ready(conn);
> > -	} else
> > -		l2cap_conn_del(hcon, bt_to_errno(status));
> > +	} else {
> > +		conn = hcon->l2cap_data;
> > +
> > +		if (hcon->type == LE_LINK)
> > +			l2cap_conn_clear_timer(conn, &conn->security_timer);
> 
> Can you explain why this change is here?

IIRC this the result of changing l2cap_conn_del -> l2cap_conn_put

Best regards 
Andrei Emeltchenko