Hi Marcel,
On Tue, Oct 23, 2012, Marcel Holtmann wrote:
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -786,6 +786,9 @@ static void hci_set_le_support(struct hci_dev *hdev)
> > if (cp.le != !!(hdev->host_features[0] & LMP_HOST_LE))
> > hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED, sizeof(cp),
> > &cp);
> > + else if (test_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags))
> > + hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE, sizeof(cp.le),
> > + &cp.le);
>
> What is this doing and why it is correct? I am failing to see this. We
> need to enable LE host supported first anyway.
The first if-statement checks if the host feature bits are what we want
them to be, and if not send the WRITE_LE_HOST_SUPPORTED command. The
else if part (if the host features are already correct and don't need
updating) enables advertising if necessary. There's a similar check for
HCI_LE_PERIPHERAL in the command complete handler for
HCI_OP_WRITE_LE_HOST_SUPPORTED in case the first if statement was
triggered. FWIW, I have actually tested this both with hciconfig and
btmgmt and it works fine.
> > +static void hci_cc_le_set_adv_enable(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > + __u8 *sent, status = *((__u8 *) skb->data);
> > +
> > + BT_DBG("%s status 0x%2.2x", hdev->name, status);
> > +
> > + sent = hci_sent_cmd_data(hdev, HCI_OP_LE_SET_ADV_ENABLE);
> > + if (!sent)
> > + return;
> > +
> > + hci_dev_lock(hdev);
> > +
> > + if (!status) {
> > + if (*sent)
> > + set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > + else
> > + clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > + } else {
> > + if (*sent)
> > + clear_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > + else
> > + set_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> > + }
>
> Are you sure we want to based LE peripheral enabling based on if we
> advertise or not. I can see cases where we are a peripheral, but might
> want to not always advertise.
These set_bit/clear_bit calls are actually all no-op in the case that
the mgmt interface is used (since the LE_PERIPHERAL flag is already
modified when receiving the mgmt command). The only reason I put them
here is so that the reported mgmt settings remain correct and we reject
connection initiation and scanning if "hciconfig hci0 leadv" or similar
is used.
The simplistic way I thought we'd initially keep peripheral mode is
that it always implies advertising (unless a connection exists).
Otherwise the user would need to switch to LE-off or Central mode.
> > @@ -1213,48 +1242,71 @@ static int set_le(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> > goto unlock;
> > }
> >
> > - val = !!cp->val;
> > - enabled = !!(hdev->host_features[0] & LMP_HOST_LE);
> > + if (!valid_le_mode(cp->val)) {
> > + err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > + MGMT_STATUS_INVALID_PARAMS);
> > + goto unlock;
> > + }
> >
> > - if (!hdev_is_powered(hdev) || val == enabled) {
> > - bool changed = false;
> > + if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
> > + err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > + MGMT_STATUS_BUSY);
> > + goto unlock;
> > + }
> > +
> > + new_mode = cp->val;
> > + old_mode = get_le_mode(hdev);
> > +
> > + BT_DBG("%s old_mode %u new_mode %u", hdev->name, old_mode, new_mode);
> > +
> > + if (new_mode == old_mode) {
> > + err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
> > + goto unlock;
> > + }
> > +
> > + if (old_mode == MGMT_LE_PERIPHERAL || new_mode == MGMT_LE_PERIPHERAL)
> > + change_bit(HCI_LE_PERIPHERAL, &hdev->dev_flags);
> >
> > - if (val != test_bit(HCI_LE_ENABLED, &hdev->dev_flags)) {
> > + if (!hdev_is_powered(hdev)) {
> > + if (old_mode == MGMT_LE_OFF || new_mode == MGMT_LE_OFF)
> > change_bit(HCI_LE_ENABLED, &hdev->dev_flags);
> > - changed = true;
> > - }
> >
> > err = send_settings_rsp(sk, MGMT_OP_SET_LE, hdev);
> > if (err < 0)
> > goto unlock;
> >
> > - if (changed)
> > - err = new_settings(hdev, sk);
> > + err = new_settings(hdev, sk);
> >
> > goto unlock;
> > }
> >
> > - if (mgmt_pending_find(MGMT_OP_SET_LE, hdev)) {
> > - err = cmd_status(sk, hdev->id, MGMT_OP_SET_LE,
> > - MGMT_STATUS_BUSY);
> > - goto unlock;
> > - }
> > -
> > cmd = mgmt_pending_add(sk, MGMT_OP_SET_LE, hdev, data, len);
> > if (!cmd) {
> > err = -ENOMEM;
> > goto unlock;
> > }
> >
> > + if ((old_mode != MGMT_LE_OFF && new_mode == MGMT_LE_PERIPHERAL) ||
> > + old_mode == MGMT_LE_PERIPHERAL) {
> > + u8 adv_enable = (new_mode == MGMT_LE_PERIPHERAL);
> > +
> > + err = hci_send_cmd(hdev, HCI_OP_LE_SET_ADV_ENABLE,
> > + sizeof(adv_enable), &adv_enable);
> > + if (err < 0)
> > + mgmt_pending_remove(cmd);
> > +
> > + goto unlock;
> > + }
> > +
>
> this gets a bit complicated. We either need more comments or split this
> out in separate helper functions.
I actually spent over a half a day trying to figure out an easy way to
simplify this, but the best I got was those two simple helper functions.
So I think I'll opt for just adding good code comments.
Johan
--
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
