Hi Johan,
Sorry the delay to reply, I was out of the office last week.
On Tue, 2013-12-10 at 15:21 +0200, Johan Hedberg wrote:
> Hi Andre,
>
> On Fri, Dec 06, 2013, Andre Guedes wrote:
> > Some LE controllers don't support scanning and creating a connection
> > at the same time. So we should always stop scanning in order to
> > establish the connection.
> >
> > Since we may prematurely stop the discovery procedure in favor of
> > the connection establishment, we should also cancel hdev->le_scan_
> > disable delayed work and set the discovery state to DISCOVERY_STOPPED.
> >
> > This change does a small improvement since it is not mandatory the
> > user stops scanning before connecting anymore. Moreover, this change
> > is required by upcoming LE auto connection mechanism in order to work
> > properly with controllers that don't support background scanning and
> > connection establishment at the same time.
> >
> > In future, we might want to do a small optimization by checking if
> > controller is able to scan and connect at the same time. For now,
> > we want the simplest approach so we always stop scanning (even if
> > the controller is able to carry out both operations).
> >
> > Signed-off-by: Andre Guedes <andre.guedes@xxxxxxxxxxxxx>
> > ---
> > net/bluetooth/hci_conn.c | 23 ++++++++++++++++++++++-
> > 1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
> > index b5c3ebff..750a39d 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -518,8 +518,17 @@ static void create_le_conn_complete(struct hci_dev *hdev, u8 status)
> > {
> > struct hci_conn *conn;
> >
> > - if (status == 0)
> > + if (status == 0) {
> > + /* If the discovery procedure was running, we prematurely
> > + * stopped it. So we have to change the discovery state.
> > + */
> > + if (hdev->discovery.state == DISCOVERY_FINDING) {
> > + cancel_delayed_work(&hdev->le_scan_disable);
> > + hci_discovery_set_state(hdev, DISCOVERY_STOPPED);
> > + }
> > +
> > return;
> > + }
> >
> > BT_ERR("HCI request failed to create LE connection: status 0x%2.2x",
> > status);
> > @@ -552,6 +561,18 @@ static int hci_create_le_conn(struct hci_conn *conn)
> >
> > hci_req_init(&req, hdev);
> >
> > + /* If controller is scanning, we stop it since some controllers are
> > + * not able to scan and connect at the same time.
> > + */
> > + if (test_bit(HCI_LE_SCAN, &hdev->dev_flags)) {
> > + struct hci_cp_le_set_scan_enable enable_cp;
> > +
> > + memset(&enable_cp, 0, sizeof(enable_cp));
> > + enable_cp.enable = LE_SCAN_DISABLE;
> > + hci_req_add(&req, HCI_OP_LE_SET_SCAN_ENABLE, sizeof(enable_cp),
> > + &enable_cp);
> > + }
> > +
> > memset(&cp, 0, sizeof(cp));
> > cp.scan_interval = cpu_to_le16(hdev->le_scan_interval);
> > cp.scan_window = cpu_to_le16(hdev->le_scan_window);
>
> The way this all hangs together with a discovery process started through
> mgmt_start_discovery feels a bit flimsy to me. Particularly, have you
> ensured that everything is fine if you've got inquiry ongoing when you
> do hci_discovery_set_state(hdev, DISCOVERY_STOPPED). Also, are there any
> risks of race conditions here, e.g. is it fine to let the
> cancel_delayed_work() call be in create_le_conn_complete() instead of
> doing it in hci_create_le_conn()?
Yes, I did lots of testing, including the inquiry test you mentioned,
and I didn't find any issues.
I failed to see any race conditons since cancel_delayed_work() does not
block. So it is fine to call cancel_delayed_work() in
create_le_conn_complete() as well as in hci_create_le_conn().
> What also makes this hard to track is that the condition you're testing
> for first is the HCI_LE_SCAN bit, but then later you look at
> discovery.state == DISCOVERY_FINDING. For the casual reader there's no
> direct indication of how these two are releated for the various types of
> discovery that are possible (LE-only, BR/EDR-only and interleaved).
Yes, I see your point. However, we can't check HCI_LE_SCAN in
create_le_conn_complete() because the flag was already cleared in
hci_cc_le_set_scan_enable().
> I don't mean to say that this is a nack for the patch, but I'd like to
> know that you've considered and tested this kind of cases. I had to
> spend quite some time looking through the existing code and this patch
> and still couldn't arrive at absolute confidence of its correctness,
> meaning there should hopefully be some room for simplification.
So I think we can do some simplification by moving this discovery
handling from create_le_conn_complete() to hci_create_le_conn(), at
least I think this code will become a bit easier to follow. What do you
think?
Regards,
Andre
--
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
