[RFC] Bluetooth: AMP: Do not set name for AMP ctrl

[RFC] Bluetooth: AMP: Do not set name for AMP ctrl

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

AMP controllers do not understand this command

...
< HCI Command: Write Local Name (0x03|0x0013) plen 248
   name ''
> HCI Event: Command Complete (0x0e) plen 4
   Write Local Name (0x03|0x0013) ncmd 1
   status 0x01
   Error: Unknown HCI Command
...

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 net/bluetooth/mgmt.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 958f764..9afcc84 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2135,6 +2135,9 @@ static int set_local_name(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	BT_DBG("");
 
+	if (hdev->amp_type != HCI_BREDR)
+		return -ENOTSUPP;
+
 	hci_dev_lock(hdev);
 
 	memcpy(hdev->short_name, cp->short_name, sizeof(hdev->short_name));
-- 
1.7.9.5

Re: [RFC] Bluetooth: AMP: Do not set name for AMP ctrl

[Johan Hedberg]

Hi Andrei,

On Wed, Jun 06, 2012, Andrei Emeltchenko wrote:
> AMP controllers do not understand this command
> 
> ...
> < HCI Command: Write Local Name (0x03|0x0013) plen 248
>    name ''
> > HCI Event: Command Complete (0x0e) plen 4
>    Write Local Name (0x03|0x0013) ncmd 1
>    status 0x01
>    Error: Unknown HCI Command
> ...
> 
> Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> ---
>  net/bluetooth/mgmt.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 958f764..9afcc84 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2135,6 +2135,9 @@ static int set_local_name(struct sock *sk, struct hci_dev *hdev, void *data,
>  
>  	BT_DBG("");
>  
> +	if (hdev->amp_type != HCI_BREDR)
> +		return -ENOTSUPP;
> +
>  	hci_dev_lock(hdev);
>  
>  	memcpy(hdev->short_name, cp->short_name, sizeof(hdev->short_name));

This will essentially make write() fail on the mgmt socket which isn't
necessarily the most intuitive behavior (one might think there's
something wrong with the socket). What would probably make more sense is
to send a proper cmd_status reply with MGMT_STATUS_NOT_SUPPORTED.

Johan

Re: [RFC] Bluetooth: AMP: Do not set name for AMP ctrl

[Johan Hedberg]

Hi,

On Wed, Jun 06, 2012, Johan Hedberg wrote:
> On Wed, Jun 06, 2012, Andrei Emeltchenko wrote:
> > AMP controllers do not understand this command
> > 
> > ...
> > < HCI Command: Write Local Name (0x03|0x0013) plen 248
> >    name ''
> > > HCI Event: Command Complete (0x0e) plen 4
> >    Write Local Name (0x03|0x0013) ncmd 1
> >    status 0x01
> >    Error: Unknown HCI Command
> > ...
> > 
> > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > ---
> >  net/bluetooth/mgmt.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > index 958f764..9afcc84 100644
> > --- a/net/bluetooth/mgmt.c
> > +++ b/net/bluetooth/mgmt.c
> > @@ -2135,6 +2135,9 @@ static int set_local_name(struct sock *sk, struct hci_dev *hdev, void *data,
> >  
> >  	BT_DBG("");
> >  
> > +	if (hdev->amp_type != HCI_BREDR)
> > +		return -ENOTSUPP;
> > +
> >  	hci_dev_lock(hdev);
> >  
> >  	memcpy(hdev->short_name, cp->short_name, sizeof(hdev->short_name));
> 
> This will essentially make write() fail on the mgmt socket which isn't
> necessarily the most intuitive behavior (one might think there's
> something wrong with the socket). What would probably make more sense is
> to send a proper cmd_status reply with MGMT_STATUS_NOT_SUPPORTED.

Actually, should we even be exposing AMP controllers to begin with
through mgmt? Maybe we shouldn't even tell user space about them through
mgmt?

Johan

Re: [RFC] Bluetooth: AMP: Do not set name for AMP ctrl

[Andrei Emeltchenko]

Hi Johan,

On Wed, Jun 06, 2012 at 11:27:36PM +0800, Johan Hedberg wrote:
> Hi,
> 
> On Wed, Jun 06, 2012, Johan Hedberg wrote:
> > On Wed, Jun 06, 2012, Andrei Emeltchenko wrote:
> > > AMP controllers do not understand this command
> > > 
> > > ...
> > > < HCI Command: Write Local Name (0x03|0x0013) plen 248
> > >    name ''
> > > > HCI Event: Command Complete (0x0e) plen 4
> > >    Write Local Name (0x03|0x0013) ncmd 1
> > >    status 0x01
> > >    Error: Unknown HCI Command
> > > ...
> > > 
> > > Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> > > ---
> > >  net/bluetooth/mgmt.c |    3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> > > index 958f764..9afcc84 100644
> > > --- a/net/bluetooth/mgmt.c
> > > +++ b/net/bluetooth/mgmt.c
> > > @@ -2135,6 +2135,9 @@ static int set_local_name(struct sock *sk, struct hci_dev *hdev, void *data,
> > >  
> > >  	BT_DBG("");
> > >  
> > > +	if (hdev->amp_type != HCI_BREDR)
> > > +		return -ENOTSUPP;
> > > +
> > >  	hci_dev_lock(hdev);
> > >  
> > >  	memcpy(hdev->short_name, cp->short_name, sizeof(hdev->short_name));
> > 
> > This will essentially make write() fail on the mgmt socket which isn't
> > necessarily the most intuitive behavior (one might think there's
> > something wrong with the socket). What would probably make more sense is
> > to send a proper cmd_status reply with MGMT_STATUS_NOT_SUPPORTED.
> 
> Actually, should we even be exposing AMP controllers to begin with
> through mgmt? Maybe we shouldn't even tell user space about them through
> mgmt?

This seems to be right. BTW: What is the best way to disable mgmt for
device?
 - Clear HCI_MGMT flag ?
 - Do not run mgmt_powered ?

BTW: Where that flag HCI_MGMT is set and how this check supposed to be
working?

test_and_set_bit(HCI_MGMT, &hdev->dev_flags))

Best regards 
Andrei Emeltchenko  

Re: [RFC] Bluetooth: AMP: Do not set name for AMP ctrl

[Johan Hedberg]

Hi Andrei,

On Thu, Jun 07, 2012, Andrei Emeltchenko wrote:
> > Actually, should we even be exposing AMP controllers to begin with
> > through mgmt? Maybe we shouldn't even tell user space about them through
> > mgmt?
> 
> This seems to be right. BTW: What is the best way to disable mgmt for
> device?
>  - Clear HCI_MGMT flag ?
>  - Do not run mgmt_powered ?
> 
> BTW: Where that flag HCI_MGMT is set and how this check supposed to be
> working?
> 
> test_and_set_bit(HCI_MGMT, &hdev->dev_flags))

The HCI_MGMT flag indicates that there's a mgmt-capable user-space that
is aware of the hci_dev.

The first thing that comes to mind is to add checks to mgmt_index_added,
mgmt_index_removed, mgmt_get_index_list and the place in mgmt_control
that calls hci_dev_get. Since all of these places essentially would need
to do the the same check it might be worth to wrap it into a macro, e.g.
mgmt_valid_hdev().

Johan

[PATCH] Bluetooth: mgmt: Fix managing AMP device

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

AMP device shall not be managed by user space for now.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/hci_core.h |    2 +-
 net/bluetooth/hci_core.c         |    6 ++++--
 net/bluetooth/mgmt.c             |   20 ++++++++++++++++++++
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 288eaa8..78d5fe2 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1080,7 +1080,7 @@ int mgmt_discovering(struct hci_dev *hdev, u8 discovering);
 int mgmt_interleaved_discovery(struct hci_dev *hdev);
 int mgmt_device_blocked(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type);
 int mgmt_device_unblocked(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type);
-
+bool mgmt_valid_hdev(struct hci_dev *hdev);
 int mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, u8 persistent);
 
 /* HCI info for socket */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 88d996b..25a86cd 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -699,7 +699,8 @@ int hci_dev_open(__u16 dev)
 		hci_dev_hold(hdev);
 		set_bit(HCI_UP, &hdev->flags);
 		hci_notify(hdev, HCI_DEV_UP);
-		if (!test_bit(HCI_SETUP, &hdev->dev_flags)) {
+		if (!test_bit(HCI_SETUP, &hdev->dev_flags) &&
+		    mgmt_valid_hdev(hdev)) {
 			hci_dev_lock(hdev);
 			mgmt_powered(hdev, 1);
 			hci_dev_unlock(hdev);
@@ -801,7 +802,8 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 	 * and no tasks are scheduled. */
 	hdev->close(hdev);
 
-	if (!test_and_clear_bit(HCI_AUTO_OFF, &hdev->dev_flags)) {
+	if (!test_and_clear_bit(HCI_AUTO_OFF, &hdev->dev_flags) &&
+	    mgmt_valid_hdev(hdev)) {
 		hci_dev_lock(hdev);
 		mgmt_powered(hdev, 0);
 		hci_dev_unlock(hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 958f764..5ea1305 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -193,6 +193,11 @@ static u8 mgmt_status_table[] = {
 	MGMT_STATUS_CONNECT_FAILED,	/* MAC Connection Failed */
 };
 
+bool mgmt_valid_hdev(struct hci_dev *hdev)
+{
+	return hdev->amp_type == HCI_BREDR;
+}
+
 static u8 mgmt_status(u8 hci_status)
 {
 	if (hci_status < ARRAY_SIZE(mgmt_status_table))
@@ -329,6 +334,9 @@ static int read_index_list(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	count = 0;
 	list_for_each(p, &hci_dev_list) {
+		if (!mgmt_valid_hdev(hdev))
+			continue;
+
 		count++;
 	}
 
@@ -346,6 +354,9 @@ static int read_index_list(struct sock *sk, struct hci_dev *hdev, void *data,
 		if (test_bit(HCI_SETUP, &d->dev_flags))
 			continue;
 
+		if (!mgmt_valid_hdev(hdev))
+			continue;
+
 		rp->index[i++] = cpu_to_le16(d->id);
 		BT_DBG("Added hci%u", d->id);
 	}
@@ -2135,6 +2146,9 @@ static int set_local_name(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	BT_DBG("");
 
+	if (hdev->amp_type != HCI_BREDR)
+		return -ENOTSUPP;
+
 	hci_dev_lock(hdev);
 
 	memcpy(hdev->short_name, cp->short_name, sizeof(hdev->short_name));
@@ -2813,6 +2827,9 @@ static void cmd_status_rsp(struct pending_cmd *cmd, void *data)
 
 int mgmt_index_added(struct hci_dev *hdev)
 {
+	if (!mgmt_valid_hdev(hdev))
+		return -ENOTSUPP;
+
 	return mgmt_event(MGMT_EV_INDEX_ADDED, hdev, NULL, 0, NULL);
 }
 
@@ -2820,6 +2837,9 @@ int mgmt_index_removed(struct hci_dev *hdev)
 {
 	u8 status = MGMT_STATUS_INVALID_INDEX;
 
+	if (!mgmt_valid_hdev(hdev))
+		return -ENOTSUPP;
+
 	mgmt_pending_foreach(0, hdev, cmd_status_rsp, &status);
 
 	return mgmt_event(MGMT_EV_INDEX_REMOVED, hdev, NULL, 0, NULL);
-- 
1.7.9.5

[PATCHv2] Bluetooth: mgmt: Fix managing AMP device

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

AMP device shall not be managed by user space for now.

Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
---
 include/net/bluetooth/hci_core.h |    2 +-
 net/bluetooth/hci_core.c         |    6 ++++--
 net/bluetooth/mgmt.c             |   23 +++++++++++++++++++++--
 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 05d3340..40ce2fa 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1074,7 +1074,7 @@ int mgmt_discovering(struct hci_dev *hdev, u8 discovering);
 int mgmt_interleaved_discovery(struct hci_dev *hdev);
 int mgmt_device_blocked(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type);
 int mgmt_device_unblocked(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 type);
-
+bool mgmt_valid_hdev(struct hci_dev *hdev);
 int mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, u8 persistent);
 
 /* HCI info for socket */
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index f02325d..43ca889 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -698,7 +698,8 @@ int hci_dev_open(__u16 dev)
 		hci_dev_hold(hdev);
 		set_bit(HCI_UP, &hdev->flags);
 		hci_notify(hdev, HCI_DEV_UP);
-		if (!test_bit(HCI_SETUP, &hdev->dev_flags)) {
+		if (!test_bit(HCI_SETUP, &hdev->dev_flags) &&
+		    mgmt_valid_hdev(hdev)) {
 			hci_dev_lock(hdev);
 			mgmt_powered(hdev, 1);
 			hci_dev_unlock(hdev);
@@ -799,7 +800,8 @@ static int hci_dev_do_close(struct hci_dev *hdev)
 	 * and no tasks are scheduled. */
 	hdev->close(hdev);
 
-	if (!test_and_clear_bit(HCI_AUTO_OFF, &hdev->dev_flags)) {
+	if (!test_and_clear_bit(HCI_AUTO_OFF, &hdev->dev_flags) &&
+	    mgmt_valid_hdev(hdev)) {
 		hci_dev_lock(hdev);
 		mgmt_powered(hdev, 0);
 		hci_dev_unlock(hdev);
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index c72307c..68cc079 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -193,6 +193,11 @@ static u8 mgmt_status_table[] = {
 	MGMT_STATUS_CONNECT_FAILED,	/* MAC Connection Failed */
 };
 
+bool mgmt_valid_hdev(struct hci_dev *hdev)
+{
+	return hdev->amp_type == HCI_BREDR;
+}
+
 static u8 mgmt_status(u8 hci_status)
 {
 	if (hci_status < ARRAY_SIZE(mgmt_status_table))
@@ -317,7 +322,6 @@ static int read_index_list(struct sock *sk, struct hci_dev *hdev, void *data,
 			   u16 data_len)
 {
 	struct mgmt_rp_read_index_list *rp;
-	struct list_head *p;
 	struct hci_dev *d;
 	size_t rp_len;
 	u16 count;
@@ -328,7 +332,10 @@ static int read_index_list(struct sock *sk, struct hci_dev *hdev, void *data,
 	read_lock(&hci_dev_list_lock);
 
 	count = 0;
-	list_for_each(p, &hci_dev_list) {
+	list_for_each_entry(d, &hci_dev_list, list) {
+		if (!mgmt_valid_hdev(d))
+			continue;
+
 		count++;
 	}
 
@@ -346,6 +353,9 @@ static int read_index_list(struct sock *sk, struct hci_dev *hdev, void *data,
 		if (test_bit(HCI_SETUP, &d->dev_flags))
 			continue;
 
+		if (!mgmt_valid_hdev(d))
+			continue;
+
 		rp->index[i++] = cpu_to_le16(d->id);
 		BT_DBG("Added hci%u", d->id);
 	}
@@ -2153,6 +2163,9 @@ static int set_local_name(struct sock *sk, struct hci_dev *hdev, void *data,
 
 	BT_DBG("");
 
+	if (hdev->amp_type != HCI_BREDR)
+		return -ENOTSUPP;
+
 	hci_dev_lock(hdev);
 
 	memcpy(hdev->short_name, cp->short_name, sizeof(hdev->short_name));
@@ -2831,6 +2844,9 @@ static void cmd_status_rsp(struct pending_cmd *cmd, void *data)
 
 int mgmt_index_added(struct hci_dev *hdev)
 {
+	if (!mgmt_valid_hdev(hdev))
+		return -ENOTSUPP;
+
 	return mgmt_event(MGMT_EV_INDEX_ADDED, hdev, NULL, 0, NULL);
 }
 
@@ -2838,6 +2854,9 @@ int mgmt_index_removed(struct hci_dev *hdev)
 {
 	u8 status = MGMT_STATUS_INVALID_INDEX;
 
+	if (!mgmt_valid_hdev(hdev))
+		return -ENOTSUPP;
+
 	mgmt_pending_foreach(0, hdev, cmd_status_rsp, &status);
 
 	return mgmt_event(MGMT_EV_INDEX_REMOVED, hdev, NULL, 0, NULL);
-- 
1.7.9.5