Hi Marcel,
On Wed, Oct 02, 2013, Marcel Holtmann wrote:
> > +static int set_bredr(struct sock *sk, struct hci_dev *hdev, void *data, u16 len)
> > +{
> > + struct mgmt_mode *cp = data;
> > + struct pending_cmd *cmd;
> > + struct hci_request req;
> > + u8 val, enabled;
> > + int err;
> > +
> > + BT_DBG("request for %s", hdev->name);
> > +
> > + if (!lmp_bredr_capable(hdev) || !lmp_le_capable(hdev))
> > + return cmd_status(sk, hdev->id, MGMT_OP_SET_BREDR,
> > + MGMT_STATUS_REJECTED);
>
> this should MGMT_STATUS_NOT_SUPPORTED to be in sync with how we do it
> for other commands.
Fixed in my latest set.
> > + if (val == enabled) {
> > + err = send_settings_rsp(sk, MGMT_OP_SET_BREDR, hdev);
> > + goto unlock;
> > + }
> > +
> > + if (!hdev_is_powered(hdev)) {
> > + if (!val) {
> > + clear_bit(HCI_CONNECTABLE, &hdev->dev_flags);
> > + clear_bit(HCI_DISCOVERABLE, &hdev->dev_flags);
> > + clear_bit(HCI_SSP_ENABLED, &hdev->dev_flags);
> > + clear_bit(HCI_LINK_SECURITY, &hdev->dev_flags);
> > + clear_bit(HCI_FAST_CONNECTABLE, &hdev->dev_flags);
> > + clear_bit(HCI_HS_ENABLED, &hdev->dev_flags);
> > + }
> > +
> > + change_bit(HCI_BREDR_ENABLED, &hdev->dev_flags);
> > +
> > + err = send_settings_rsp(sk, MGMT_OP_SET_BREDR, hdev);
> > + if (err < 0)
> > + goto unlock;
> > +
> > + err = new_settings(hdev, sk);
> > + goto unlock;
> > + }
> > +
> > + /* Reject disabling when powered on */
> > + if (!val) {
> > + err = cmd_status(sk, hdev->id, MGMT_OP_SET_BREDR,
> > + MGMT_STATUS_REJECTED);
> > + goto unlock;
> > + }
>
> I would have done this the other way around with the if clauses.
>
> if (val) {
> …
> } else {
> if (hdev_is_powered())
> fail
>
> …
> }
>
I've left this as is since it actually produces simpler code imo. I
believe your suggestion was influenced by the set_hs code, however
that's a bit different since you don't need to send any HCI commands
there.
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
