[PATCH 1/3] Bluetooth: Add extfeatures to struct hci_dev

[PATCH 1/3] Bluetooth: Add extfeatures to struct hci_dev

[Andre Guedes]

This new field holds the extended LMP features value. Some LE
mechanism such as discovery procedure needs to read the extended
LMP features to work properly.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci.h      |    3 +++
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_core.c         |    6 ++++++
 net/bluetooth/hci_event.c        |   21 +++++++++++++++++++++
 4 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index 65345cd..acf589c 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -676,6 +676,9 @@ struct hci_rp_read_local_features {
 } __packed;
 
 #define HCI_OP_READ_LOCAL_EXT_FEATURES	0x1004
+struct hci_cp_read_local_ext_features {
+	__u8     page;
+} __packed;
 struct hci_rp_read_local_ext_features {
 	__u8     status;
 	__u8     page;
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 26233d4..cd65f0e 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -113,6 +113,7 @@ struct hci_dev {
 	__u8		major_class;
 	__u8		minor_class;
 	__u8		features[8];
+	__u8		extfeatures[8];
 	__u8		commands[64];
 	__u8		ssp_mode;
 	__u8		hci_ver;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 0029e17..5c5ba74 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -195,6 +195,7 @@ static void hci_reset_req(struct hci_dev *hdev, unsigned long opt)
 static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
 {
 	struct hci_cp_delete_stored_link_key cp;
+	struct hci_cp_read_local_ext_features ext_cp;
 	struct sk_buff *skb;
 	__le16 param;
 	__u8 flt_type;
@@ -224,6 +225,11 @@ static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
 	/* Read Local Supported Features */
 	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
 
+	/* Read Local Extended Features */
+	ext_cp.page = 0x01;
+	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, sizeof(ext_cp),
+								&ext_cp);
+
 	/* Read Local Version */
 	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_VERSION, 0, NULL);
 
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index ac2c5e8..10d867d 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -658,6 +658,23 @@ static void hci_cc_read_local_features(struct hci_dev *hdev, struct sk_buff *skb
 					hdev->features[6], hdev->features[7]);
 }
 
+static void hci_cc_read_local_ext_features(struct hci_dev *hdev,
+							struct sk_buff *skb)
+{
+	struct hci_rp_read_local_ext_features *rp = (void *) skb->data;
+
+	BT_DBG("%s status 0x%x", hdev->name, rp->status);
+
+	if (rp->status)
+		return;
+
+	hci_dev_lock(hdev);
+
+	memcpy(hdev->extfeatures, rp->features, 8);
+
+	hci_dev_unlock(hdev);
+}
+
 static void hci_cc_read_buffer_size(struct hci_dev *hdev, struct sk_buff *skb)
 {
 	struct hci_rp_read_buffer_size *rp = (void *) skb->data;
@@ -1826,6 +1843,10 @@ static inline void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *sk
 		hci_cc_read_local_features(hdev, skb);
 		break;
 
+	case HCI_OP_READ_LOCAL_EXT_FEATURES:
+		hci_cc_read_local_ext_features(hdev, skb);
+		break;
+
 	case HCI_OP_READ_BUFFER_SIZE:
 		hci_cc_read_buffer_size(hdev, skb);
 		break;
-- 
1.7.4.1

[PATCH 2/3] Bluetooth: Write LE Host Supported command complete event

[Andre Guedes]

This patch adds a handler to Write LE Host Supported command complete
events. Once this commands has completed successfully, we should
read the extended LMP features and update the extfeatures field in
hci_dev.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci.h |    6 ++++++
 net/bluetooth/hci_event.c   |   19 +++++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index acf589c..c4f595c 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -653,6 +653,12 @@ struct hci_rp_read_local_oob_data {
 
 #define HCI_OP_READ_INQ_RSP_TX_POWER	0x0c58
 
+#define HCI_OP_WRITE_LE_HOST_SUPPORTED	0x0c6d
+struct hci_cp_write_le_host_supported {
+	__u8 le;
+	__u8 simul;
+} __packed;
+
 #define HCI_OP_READ_LOCAL_VERSION	0x1001
 struct hci_rp_read_local_version {
 	__u8     status;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 10d867d..afe1bc8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -909,6 +909,21 @@ static void hci_cc_le_ltk_neg_reply(struct hci_dev *hdev, struct sk_buff *skb)
 	hci_req_complete(hdev, HCI_OP_LE_LTK_NEG_REPLY, rp->status);
 }
 
+static inline void hci_cc_write_le_host_supported(struct hci_dev *hdev,
+							struct sk_buff *skb)
+{
+	struct hci_cp_read_local_ext_features cp;
+	__u8 status = *((__u8 *) skb->data);
+
+	BT_DBG("%s status 0x%x", hdev->name, status);
+
+	if (status)
+		return;
+
+	cp.page = 0x01;
+	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, sizeof(cp), &cp);
+}
+
 static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
 {
 	BT_DBG("%s status 0x%x", hdev->name, status);
@@ -1915,6 +1930,10 @@ static inline void hci_cmd_complete_evt(struct hci_dev *hdev, struct sk_buff *sk
 		hci_cc_le_ltk_neg_reply(hdev, skb);
 		break;
 
+	case HCI_OP_WRITE_LE_HOST_SUPPORTED:
+		hci_cc_write_le_host_supported(hdev, skb);
+		break;
+
 	default:
 		BT_DBG("%s opcode 0x%x", hdev->name, opcode);
 		break;
-- 
1.7.4.1

[PATCH 3/3] Bluetooth: Add MGMT_OP_SET_LE_SUPPORT command

[Andre Guedes]

This patch adds a new command to management interface to enable/
disable LE support. This command will be used to set LE support
according to the host configuration.

Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
---
 include/net/bluetooth/hci.h      |    1 +
 include/net/bluetooth/hci_core.h |    1 +
 include/net/bluetooth/mgmt.h     |    5 +++
 net/bluetooth/hci_event.c        |    2 +
 net/bluetooth/mgmt.c             |   73 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 82 insertions(+), 0 deletions(-)

diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c4f595c..35b3109 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -211,6 +211,7 @@ enum {
 #define LMP_EDR_3S_ESCO	0x80
 
 #define LMP_EXT_INQ	0x01
+#define LMP_SIMUL_LE_BR	0x02
 #define LMP_SIMPLE_PAIR	0x08
 #define LMP_NO_FLUSH	0x40
 
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index cd65f0e..00cbe03 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -835,6 +835,7 @@ int mgmt_device_found(u16 index, bdaddr_t *bdaddr, u8 *dev_class, s8 rssi,
 								u8 *eir);
 int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name);
 int mgmt_discovering(u16 index, u8 discovering);
+int mgmt_set_le_support_complete(u16 index, u8 status);
 
 /* HCI info for socket */
 #define hci_pi(sk) ((struct hci_pinfo *) sk)
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 45bea25..880b2e2 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -209,6 +209,11 @@ struct mgmt_cp_unblock_device {
 	bdaddr_t bdaddr;
 } __packed;
 
+#define MGMT_OP_SET_LE_SUPPORT		0x001F
+struct mgmt_cp_set_le_support {
+	__u8 enable_le;
+} __packed;
+
 #define MGMT_EV_CMD_COMPLETE		0x0001
 struct mgmt_ev_cmd_complete {
 	__le16 opcode;
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index afe1bc8..a2195ff 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -917,6 +917,8 @@ static inline void hci_cc_write_le_host_supported(struct hci_dev *hdev,
 
 	BT_DBG("%s status 0x%x", hdev->name, status);
 
+	mgmt_set_le_support_complete(hdev->id, status);
+
 	if (status)
 		return;
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 64c0418..748b8bb 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1730,6 +1730,54 @@ static int unblock_device(struct sock *sk, u16 index, unsigned char *data,
 	return err;
 }
 
+static int set_le_support(struct sock *sk, u16 index, unsigned char *data,
+								u16 len)
+{
+	struct mgmt_cp_set_le_support *mgmt_cp = (void *) data;
+	struct hci_cp_write_le_host_supported hci_cp;
+	struct pending_cmd *cmd;
+	struct hci_dev *hdev;
+	int err;
+
+	BT_DBG("hci%u ", index);
+
+	if (len != sizeof(*mgmt_cp))
+		return cmd_status(sk, index, MGMT_OP_SET_LE_SUPPORT, EINVAL);
+
+	if (mgmt_pending_find(MGMT_OP_SET_LE_SUPPORT, index))
+		return cmd_status(sk, index, MGMT_OP_SET_LE_SUPPORT,
+								EINPROGRESS);
+
+	hdev = hci_dev_get(index);
+	if (!hdev)
+		return cmd_status(sk, index, MGMT_OP_SET_LE_SUPPORT, ENODEV);
+
+	if (mgmt_cp->enable_le && !lmp_le_capable(hdev)) {
+		err = cmd_status(sk, index, MGMT_OP_SET_LE_SUPPORT,
+								EOPNOTSUPP);
+		goto failed;
+	}
+
+	cmd = mgmt_pending_add(sk, MGMT_OP_SET_LE_SUPPORT, index, data, len);
+	if (!cmd) {
+		err = -ENOMEM;
+		goto failed;
+	}
+
+	hci_cp.le = mgmt_cp->enable_le;
+	hci_cp.simul = (hdev->features[6] & LMP_SIMUL_LE_BR) ? 0x01 : 0x00;
+
+	err = hci_send_cmd(hdev, HCI_OP_WRITE_LE_HOST_SUPPORTED,
+						sizeof(hci_cp),	&hci_cp);
+	if (err < 0)
+		mgmt_pending_remove(cmd);
+
+failed:
+	hci_dev_put(hdev);
+
+	return err;
+}
+
 int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
 {
 	unsigned char *buf;
@@ -1850,6 +1898,9 @@ int mgmt_control(struct sock *sk, struct msghdr *msg, size_t msglen)
 	case MGMT_OP_UNBLOCK_DEVICE:
 		err = unblock_device(sk, index, buf + sizeof(*hdr), len);
 		break;
+	case MGMT_OP_SET_LE_SUPPORT:
+		err = set_le_support(sk, index, buf + sizeof(*hdr), len);
+		break;
 	default:
 		BT_DBG("Unknown op %u", opcode);
 		err = cmd_status(sk, index, opcode, 0x01);
@@ -2256,3 +2307,25 @@ int mgmt_discovering(u16 index, u8 discovering)
 	return mgmt_event(MGMT_EV_DISCOVERING, index, &discovering,
 						sizeof(discovering), NULL);
 }
+
+int mgmt_set_le_support_complete(u16 index, u8 status)
+{
+	struct pending_cmd *cmd;
+	int err;
+
+	cmd = mgmt_pending_find(MGMT_OP_SET_LE_SUPPORT, index);
+	if (!cmd)
+		return -ENOENT;
+
+	if (status) {
+		err = cmd_status(cmd->sk, index, MGMT_OP_SET_LE_SUPPORT, EIO);
+		goto out;
+	}
+
+	err = cmd_complete(cmd->sk, index, MGMT_OP_SET_LE_SUPPORT, NULL, 0);
+
+out:
+	mgmt_pending_remove(cmd);
+
+	return err;
+}
-- 
1.7.4.1

Re: [PATCH 3/3] Bluetooth: Add MGMT_OP_SET_LE_SUPPORT command

[Marcel Holtmann]

Hi Andre,

> This patch adds a new command to management interface to enable/
> disable LE support. This command will be used to set LE support
> according to the host configuration.

any reason why we want this separated and being under host stack control
in the first place. Should we not always enable LE if we can support it?
What are the reason to run an LE capable BlueZ on an LE capable dongle
and then not enable LE?

A similar argumentation applies for Secure Simple Pairing. We are
currently enabling this by default. Even while there are qualification
tests that would us force to run it with SSP disabled.

Maybe we should just have a generic kernel mgmt features enabling
command. One thing that I do not wanna do is to just blindly make a copy
of HCI commands for mgmt interface. The controller abstraction for the
mgmt interface is suppose to have a feature list so we can check what
the kernel supports and what not.

Regards

Marcel

Re: [PATCH 1/3] Bluetooth: Add extfeatures to struct hci_dev

[Johan Hedberg]

Hi André,

On Tue, Jun 21, 2011, Andre Guedes wrote:
> @@ -224,6 +225,11 @@ static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
>  	/* Read Local Supported Features */
>  	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
>  
> +	/* Read Local Extended Features */
> +	ext_cp.page = 0x01;
> +	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, sizeof(ext_cp),
> +								&ext_cp);

Since the extended features command is only available from 1.2 onwards I
don't think it's right to unconditionally send it in hci_init_req.
Instead, you should probably be checking for the feature bit in the
(non-extended) feature mask, i.e. in the command complete callback for
HCI_Read_Local_Features and only send the command if the feature bit is
set.

Johan

Re: [PATCH 1/3] Bluetooth: Add extfeatures to struct hci_dev

[Marcel Holtmann]

Hi Johan,

> > @@ -224,6 +225,11 @@ static void hci_init_req(struct hci_dev *hdev, unsigned long opt)
> >  	/* Read Local Supported Features */
> >  	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_FEATURES, 0, NULL);
> >  
> > +	/* Read Local Extended Features */
> > +	ext_cp.page = 0x01;
> > +	hci_send_cmd(hdev, HCI_OP_READ_LOCAL_EXT_FEATURES, sizeof(ext_cp),
> > +								&ext_cp);
> 
> Since the extended features command is only available from 1.2 onwards I
> don't think it's right to unconditionally send it in hci_init_req.
> Instead, you should probably be checking for the feature bit in the
> (non-extended) feature mask, i.e. in the command complete callback for
> HCI_Read_Local_Features and only send the command if the feature bit is
> set.

exactly and with your work on the management interface, I hope we can
get rid of hci_init_req at some point. And then do a proper async and
well defined init of the adapter. Especially with things like LE, single
mode, AMP, SSP and other factors on the init process, we need to get
this fixed once and for all.

The AMP init will look fundamentally different from the BR/EDR and init.
And so will the LE only init. And it all depends on Bluetooth adapter
version and feature bits of course.

Regards

Marcel

Re: [PATCH 3/3] Bluetooth: Add MGMT_OP_SET_LE_SUPPORT command

[Andre Guedes]

Hi Marcel,

On Tue, Jun 21, 2011 at 5:36 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> any reason why we want this separated and being under host stack control
> in the first place. Should we not always enable LE if we can support it?
> What are the reason to run an LE capable BlueZ on an LE capable dongle
> and then not enable LE?
>

I don't know the reason why, but today LE enabling is separated and is
under host control. There is an option in main.conf (EnableLE) to
enable/disable LE.

AFAIK, the reason we want to disable LE is qualification tests only. Also,
since LE support is under development we may not wanna have LE enabled
by default (maybe this is another reason).

> Maybe we should just have a generic kernel mgmt features enabling
> command. One thing that I do not wanna do is to just blindly make a copy
> of HCI commands for mgmt interface. The controller abstraction for the
> mgmt interface is suppose to have a feature list so we can check what
> the kernel supports and what not.
>

We had a little discussion here and we came up with this idea of having
new parameter to the module (e. g. enable_le) to enable/disable LE. If
the module is loaded with enable_le=y we would enable LE host support.
Once LE support is stable enough, we have enable_le=y by default.
This approach we don't need a mgmt command.

What do you think about this approach ?

BR,

Andre Guedes.

Re: [PATCH 1/3] Bluetooth: Add extfeatures to struct hci_dev

[Andre Guedes]

Hi Johan,

On Tue, Jun 21, 2011 at 6:27 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Since the extended features command is only available from 1.2 onwards I
> don't think it's right to unconditionally send it in hci_init_req.
> Instead, you should probably be checking for the feature bit in the
> (non-extended) feature mask, i.e. in the command complete callback for
> HCI_Read_Local_Features and only send the command if the feature bit is
> set.

Ok, I'll check the extended features flag.

Thanks,

Andre.

Re: [PATCH 3/3] Bluetooth: Add MGMT_OP_SET_LE_SUPPORT command

[Marcel Holtmann]

Hi Andre,

> > any reason why we want this separated and being under host stack control
> > in the first place. Should we not always enable LE if we can support it?
> > What are the reason to run an LE capable BlueZ on an LE capable dongle
> > and then not enable LE?
> >
> 
> I don't know the reason why, but today LE enabling is separated and is
> under host control. There is an option in main.conf (EnableLE) to
> enable/disable LE.
> 
> AFAIK, the reason we want to disable LE is qualification tests only. Also,
> since LE support is under development we may not wanna have LE enabled
> by default (maybe this is another reason).

for that I would have used a enable_le kernel module option since it
should be all triggered by the kernel anyway.

> > Maybe we should just have a generic kernel mgmt features enabling
> > command. One thing that I do not wanna do is to just blindly make a copy
> > of HCI commands for mgmt interface. The controller abstraction for the
> > mgmt interface is suppose to have a feature list so we can check what
> > the kernel supports and what not.
> >
> 
> We had a little discussion here and we came up with this idea of having
> new parameter to the module (e. g. enable_le) to enable/disable LE. If
> the module is loaded with enable_le=y we would enable LE host support.
> Once LE support is stable enough, we have enable_le=y by default.
> This approach we don't need a mgmt command.

See above ;)

I am open for discussion on how we might solve this in a bit more
generic way on let this control by the user. Mainly for qualification
testing and UPF testing. However we need a clear story for LE and SSP
since both are host stack driven features.

Regards

Marcel

Re: [PATCH 3/3] Bluetooth: Add MGMT_OP_SET_LE_SUPPORT command

[Andre Guedes]

Hi Marcel,

On Wed, Jun 22, 2011 at 1:57 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Andre,
>
>> > any reason why we want this separated and being under host stack control
>> > in the first place. Should we not always enable LE if we can support it?
>> > What are the reason to run an LE capable BlueZ on an LE capable dongle
>> > and then not enable LE?
>> >
>>
>> I don't know the reason why, but today LE enabling is separated and is
>> under host control. There is an option in main.conf (EnableLE) to
>> enable/disable LE.
>>
>> AFAIK, the reason we want to disable LE is qualification tests only. Also,
>> since LE support is under development we may not wanna have LE enabled
>> by default (maybe this is another reason).
>
> for that I would have used a enable_le kernel module option since it
> should be all triggered by the kernel anyway.
>
>> > Maybe we should just have a generic kernel mgmt features enabling
>> > command. One thing that I do not wanna do is to just blindly make a copy
>> > of HCI commands for mgmt interface. The controller abstraction for the
>> > mgmt interface is suppose to have a feature list so we can check what
>> > the kernel supports and what not.
>> >
>>
>> We had a little discussion here and we came up with this idea of having
>> new parameter to the module (e. g. enable_le) to enable/disable LE. If
>> the module is loaded with enable_le=y we would enable LE host support.
>> Once LE support is stable enough, we have enable_le=y by default.
>> This approach we don't need a mgmt command.
>
> See above ;)
>
> I am open for discussion on how we might solve this in a bit more
> generic way on let this control by the user. Mainly for qualification
> testing and UPF testing. However we need a clear story for LE and SSP
> since both are host stack driven features.

For LE story, the enable_le module param should be enough. IMHO, LE
support shouldn't be controlled by the user. If hardware, kernel and BlueZ
support LE then LE should be enabled.

If you have LE supported hardware, kernel and BlueZ but for some reason
(e.g. qualification tests) you want to disable LE, then you load the module
with enable_le=n parameter.

For now, while LE support is under development, enable_le default value
would be false. Once we have LE support stable enough, we change its
default value to true.

BR,

Andre

Re: [PATCH 3/3] Bluetooth: Add MGMT_OP_SET_LE_SUPPORT command

[Gustavo F. Padovan]

Hi Andre,

* Andre Guedes <andre.guedes@openbossa.org> [2011-06-22 15:33:19 -0300]:

> Hi Marcel,
> 
> On Wed, Jun 22, 2011 at 1:57 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> > Hi Andre,
> >
> >> > any reason why we want this separated and being under host stack control
> >> > in the first place. Should we not always enable LE if we can support it?
> >> > What are the reason to run an LE capable BlueZ on an LE capable dongle
> >> > and then not enable LE?
> >> >
> >>
> >> I don't know the reason why, but today LE enabling is separated and is
> >> under host control. There is an option in main.conf (EnableLE) to
> >> enable/disable LE.
> >>
> >> AFAIK, the reason we want to disable LE is qualification tests only. Also,
> >> since LE support is under development we may not wanna have LE enabled
> >> by default (maybe this is another reason).
> >
> > for that I would have used a enable_le kernel module option since it
> > should be all triggered by the kernel anyway.
> >
> >> > Maybe we should just have a generic kernel mgmt features enabling
> >> > command. One thing that I do not wanna do is to just blindly make a copy
> >> > of HCI commands for mgmt interface. The controller abstraction for the
> >> > mgmt interface is suppose to have a feature list so we can check what
> >> > the kernel supports and what not.
> >> >
> >>
> >> We had a little discussion here and we came up with this idea of having
> >> new parameter to the module (e. g. enable_le) to enable/disable LE. If
> >> the module is loaded with enable_le=y we would enable LE host support.
> >> Once LE support is stable enough, we have enable_le=y by default.
> >> This approach we don't need a mgmt command.
> >
> > See above ;)
> >
> > I am open for discussion on how we might solve this in a bit more
> > generic way on let this control by the user. Mainly for qualification
> > testing and UPF testing. However we need a clear story for LE and SSP
> > since both are host stack driven features.
> 
> For LE story, the enable_le module param should be enough. IMHO, LE
> support shouldn't be controlled by the user. If hardware, kernel and BlueZ
> support LE then LE should be enabled.
> 
> If you have LE supported hardware, kernel and BlueZ but for some reason
> (e.g. qualification tests) you want to disable LE, then you load the module
> with enable_le=n parameter.
> 
> For now, while LE support is under development, enable_le default value
> would be false. Once we have LE support stable enough, we change its
> default value to true.

Once we add enable_le I think we can also remove enable_smp, unless this is
needed for for some qualification test. But I don't think so.

	Gustavo

Re: [PATCH 3/3] Bluetooth: Add MGMT_OP_SET_LE_SUPPORT command

[Andre Guedes]

Hi Gustavo,

On Wed, Jun 22, 2011 at 4:20 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Once we add enable_le I think we can also remove enable_smp, unless this is
> needed for for some qualification test. But I don't think so.

Yes, once we have enable_le param we'll remove enable_smp.

Andre

Re: [PATCH 3/3] Bluetooth: Add MGMT_OP_SET_LE_SUPPORT command

[Marcel Holtmann]

Hi Andre,

> >> > any reason why we want this separated and being under host stack control
> >> > in the first place. Should we not always enable LE if we can support it?
> >> > What are the reason to run an LE capable BlueZ on an LE capable dongle
> >> > and then not enable LE?
> >> >
> >>
> >> I don't know the reason why, but today LE enabling is separated and is
> >> under host control. There is an option in main.conf (EnableLE) to
> >> enable/disable LE.
> >>
> >> AFAIK, the reason we want to disable LE is qualification tests only. Also,
> >> since LE support is under development we may not wanna have LE enabled
> >> by default (maybe this is another reason).
> >
> > for that I would have used a enable_le kernel module option since it
> > should be all triggered by the kernel anyway.
> >
> >> > Maybe we should just have a generic kernel mgmt features enabling
> >> > command. One thing that I do not wanna do is to just blindly make a copy
> >> > of HCI commands for mgmt interface. The controller abstraction for the
> >> > mgmt interface is suppose to have a feature list so we can check what
> >> > the kernel supports and what not.
> >> >
> >>
> >> We had a little discussion here and we came up with this idea of having
> >> new parameter to the module (e. g. enable_le) to enable/disable LE. If
> >> the module is loaded with enable_le=y we would enable LE host support.
> >> Once LE support is stable enough, we have enable_le=y by default.
> >> This approach we don't need a mgmt command.
> >
> > See above ;)
> >
> > I am open for discussion on how we might solve this in a bit more
> > generic way on let this control by the user. Mainly for qualification
> > testing and UPF testing. However we need a clear story for LE and SSP
> > since both are host stack driven features.
> 
> For LE story, the enable_le module param should be enough. IMHO, LE
> support shouldn't be controlled by the user. If hardware, kernel and BlueZ
> support LE then LE should be enabled.
> 
> If you have LE supported hardware, kernel and BlueZ but for some reason
> (e.g. qualification tests) you want to disable LE, then you load the module
> with enable_le=n parameter.
> 
> For now, while LE support is under development, enable_le default value
> would be false. Once we have LE support stable enough, we change its
> default value to true.

sounds good to me. I am always for the minimal approach. Since adding
something later is easy. Removing/deprecating is hard. So if we feel
like a mgmt command is needed, we can add it any time.

Regards

Marcel