Re: conn->state vs conn->sk->sk_state

[Marcel Holtmann <marcel@holtmann.org>]

Hi Nick,

> >>> Looking at this a bit further, I see a discrepancy between the code in
> >>> function  hci_conn_complete_evt and hci_sync_conn_complete_evt.
> >>> Is this intentional or a bug ? Like I said below, using
> >>> hci_conn_complete_evt (when the AG doesn't support eSCO) results in
> >>> the connect() never returning because the socket state is not updated.
> >>
> >> I still have no idea what's your problem. It doesn't matter if the
> >> remote side supports eSCO or not. The sync setup commands can be used
> >> even for setting up SCO channels.
> >>
> >
> >  So here is the problem:
> >     The  AG doesn't support eSCO i.e disable_esco is Y and
> > lmp_esco_capable(dev) evaluates to 0, so we try to establish a SCO
> > channel irrespective of whether the remote support eSCO or not.
> >
> >  So sco_connect is called, and after the hci_connect, hcon->state is
> > BT_CONNECT and the sk->sk_state = BT_CONNECT.
> >  When we get a response from the remote device
> > hci_connect_complete_evt is called which sets the conn->state to
> > BT_CONNECTED. However, as
> >  hci_proto_connect_cfm is not called (this was being called in
> > 2.6.25) ,  sk->sk_state remained in BT_CONNECT state. Hence, we get
> > stuck in bt_sock_wait_state
> >  and the connect() times out.
> 
> 
> Explained another way:
> 
> JK found that in 2.6.27 with "echo Y >
> /sys/module/sco/parameters/disable_esco" the socket state is never
> updated to BT_CONNECTED. The userspace connect() call then times out,
> even though the transport is connected. This appears to be because
> hci_proto_connect_cfm() is not called when the connection complete
> event arrives. Below is a patch that resolves this issue for us.
> 
> This is a request for comment as to whether this patch is correct. It
> contradicts Marcel's change 769be974 which explicitly moved
> hci_proto_connect_cfm() into the ev->status conditional.

now I get what you are talking about. You guys should have sent the
patch from the beginning since that makes it clear what you are trying
to achieve.

So the patch is wrong, because it breaks Simple Pairing. What you have
to do is check if it is an ACL link, then the code is doing the right
thing and doing features requests first.

        if (ev->status) {
                hci_proto_connect_cfm(conn, ev->status);
                hci_conn_del(conn);
        } else if (ev->link_type != ACL_LINK)
                hci_proto_connect_cfm(conn, ev->status);

Regards

Marcel