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

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

[jaikumar Ganesh]

 Folks,
     I have a device which supports only SCO connections.  So when the
application calls sco_sock_connect -> sco_connect, the conn->state is
set to BT_CONNECT and then bt_sock_state is called waiting for the
conn->sk->sk_state to be set to BT_CONNECTED.
     When the HCI Connection Complete event comes, the conn->state is
set to BT_CONNECTED.

     In 2.6.25 ->  hci_proto_connect_cfm(conn, ev->status) was called
at the end of hci_conn_complete_evt which used to set the
conn->sk->sk_state to BT_CONNECTED in sco_conn_ready.
     In 2.6.27->  hci_proto_connect_cfm(conn, ev->status) is called
only if the ev_status is 1.
                      Hence, the conn->sk->sk_state doesn't get set to
BT_CONNECTED and hence the connect() times out.

    Should we be calling sco_conn_ready in sco_connect similar to
sco_connect_cfm if conn != null ?
    I didn't see anyone else complain so am I missing something obvious ?


     * I am new to this list so please excuse me if the above doesn't
make sense.

 Thanks
 Jaikumar

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

[jaikumar Ganesh]

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.

//hci_conn_complete_evt
        if (ev->status) {
                hci_proto_connect_cfm(conn, ev->status);
                hci_conn_del(conn);
        }

unlock:
        hci_dev_unlock(hdev);
        hci_conn_check_pending(hdev);
}

//hci_sync_conn_complete_evt.

        hci_proto_connect_cfm(conn, ev->status);
        if (ev->status)
                hci_conn_del(conn);
unlock:
        hci_dev_unlock(hdev);
}

Any pointers / hints ?

Thanks
Jaikumar
On Wed, Dec 17, 2008 at 2:07 PM, jaikumar Ganesh <jaikumarg@gmail.com> wrote:
>  Folks,
>     I have a device which supports only SCO connections.  So when the
> application calls sco_sock_connect -> sco_connect, the conn->state is
> set to BT_CONNECT and then bt_sock_state is called waiting for the
> conn->sk->sk_state to be set to BT_CONNECTED.
>     When the HCI Connection Complete event comes, the conn->state is
> set to BT_CONNECTED.
>
>     In 2.6.25 ->  hci_proto_connect_cfm(conn, ev->status) was called
> at the end of hci_conn_complete_evt which used to set the
> conn->sk->sk_state to BT_CONNECTED in sco_conn_ready.
>     In 2.6.27->  hci_proto_connect_cfm(conn, ev->status) is called
> only if the ev_status is 1.
>                      Hence, the conn->sk->sk_state doesn't get set to
> BT_CONNECTED and hence the connect() times out.
>
>    Should we be calling sco_conn_ready in sco_connect similar to
> sco_connect_cfm if conn != null ?
>    I didn't see anyone else complain so am I missing something obvious ?
>
>
>     * I am new to this list so please excuse me if the above doesn't
> make sense.
>
>  Thanks
>  Jaikumar
>

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

[Marcel Holtmann]

Hi Jaikumar,

> 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.

Regards

Marcel

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

[jaikumar Ganesh]

Hi Marcel,

On Fri, Dec 19, 2008 at 10:58 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Jaikumar,
>
>> 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.

Thanks
Jaikumar

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

[Nick Pelly]

On Fri, Dec 19, 2008 at 10:28 AM, jaikumar Ganesh <jaikumarg@gmail.com> wrote:
> Hi Marcel,
>
> On Fri, Dec 19, 2008 at 10:58 AM, Marcel Holtmann <marcel@holtmann.org> wrote:
>> Hi Jaikumar,
>>
>>> 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.

Thanks,
Nick



commit 654488a822615b645c43605ab24f0305b56b40dc
Author: Jaikumar Ganesh <jaikumar@google.com>
Date:   Fri Dec 19 14:17:53 2008 -0800

    [Bluetooth] Fix SCO connection issue

    When the AG supports only SCO connections, on receipt of the
    HCI_EV_CONN_COMPLETE packet, the connect state is changed to
    BT_CONNECTED but the socket state is not updated. Hence, the
    connect() call times out even though the SCO connection has
    been established.

    Signed-off-by: Jaikumar Ganesh <jaikumar@google.com>

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index ad7a553..3cba788 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -916,8 +916,8 @@ static inline void hci_conn_complete_evt(struct
hci_dev *hdev, struct sk_buff *s
                }
        }

+       hci_proto_connect_cfm(conn, ev->status);
        if (ev->status) {
-               hci_proto_connect_cfm(conn, ev->status);
                hci_conn_del(conn);
        }

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

[Marcel Holtmann]

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

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

[jaikumar Ganesh]

Hi Marcel,

On Mon, Dec 22, 2008 at 2:18 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> 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
>
>
>
Sorry for not sending the patch before, we did the patch only on Friday.
Will change it according to your suggestion and submit.

Thanks
Jaikumar