2015-05-21 14:46:40

by Loic Poulain

Subject: [PATCH 1/2] Bluetooth: Don't call shutdown when leaving user channel

Don't interfere with the user channel exclusive access.

Signed-off-by: Loic Poulain <[email protected]>
---
net/bluetooth/hci_core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 476709b..5120fea 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1557,7 +1557,8 @@ static int hci_dev_do_close(struct hci_dev *hdev)
{
BT_DBG("%s %p", hdev->name, hdev);

- if (!hci_dev_test_flag(hdev, HCI_UNREGISTER)) {
+ if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
+ !hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {
/* Execute vendor specific shutdown routine */
if (hdev->shutdown)
hdev->shutdown(hdev);
--
1.9.1

2015-05-21 19:55:29

by Marcel Holtmann

Subject: Re: [PATCH 2/2] Bluetooth: Reorder HCI user channel socket release

Hi Loic,

> The hci close method needs to know if we are in user channel context.
> Only add the index to mgmt once close is performed.
>
> Signed-off-by: Loic Poulain <[email protected]>
> ---
> net/bluetooth/hci_sock.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index 56f9edb..e1beb4e 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -503,9 +503,9 @@ static int hci_sock_release(struct socket *sock)
>
> if (hdev) {
> if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
> - mgmt_index_added(hdev);
> - hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
> hci_dev_close(hdev->id);
> + hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
> + mgmt_index_added(hdev);
> }

this way around looks so much more logical. However now I wonder if either I was confused when writing this code in the first place or I had a really good reason for it, but just forgot to document it.

Regards

Marcel

2015-05-21 14:46:41

by Loic Poulain

Subject: [PATCH 2/2] Bluetooth: Reorder HCI user channel socket release

The hci close method needs to know if we are in user channel context.
Only add the index to mgmt once close is performed.

Signed-off-by: Loic Poulain <[email protected]>
---
net/bluetooth/hci_sock.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
index 56f9edb..e1beb4e 100644
--- a/net/bluetooth/hci_sock.c
+++ b/net/bluetooth/hci_sock.c
@@ -503,9 +503,9 @@ static int hci_sock_release(struct socket *sock)

if (hdev) {
if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
- mgmt_index_added(hdev);
- hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
hci_dev_close(hdev->id);
+ hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
+ mgmt_index_added(hdev);
}

atomic_dec(&hdev->promisc);
--
1.9.1

2015-06-06 18:51:24

by Marcel Holtmann

Subject: Re: [PATCH 1/2] Bluetooth: Don't call shutdown when leaving user channel

Hi Loic,

> Don't interfere with the user channel exclusive access.
>
> Signed-off-by: Loic Poulain <[email protected]>
> ---
> net/bluetooth/hci_core.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 476709b..5120fea 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1557,7 +1557,8 @@ static int hci_dev_do_close(struct hci_dev *hdev)
> {
> BT_DBG("%s %p", hdev->name, hdev);
>
> - if (!hci_dev_test_flag(hdev, HCI_UNREGISTER)) {
> + if (!hci_dev_test_flag(hdev, HCI_UNREGISTER) &&
> + !hci_dev_test_flag(hdev, HCI_USER_CHANNEL)) {

this will conflict with the extra check we added that the device is actually HCI_UP.

> /* Execute vendor specific shutdown routine */
> if (hdev->shutdown)
> hdev->shutdown(hdev);

Regards

Marcel

2015-06-06 18:51:22

by Marcel Holtmann

Subject: Re: [PATCH 2/2] Bluetooth: Reorder HCI user channel socket release

Hi Loic,

> The hci close method needs to know if we are in user channel context.
> Only add the index to mgmt once close is performed.
>
> Signed-off-by: Loic Poulain <[email protected]>
> ---
> net/bluetooth/hci_sock.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

patch has been applied to bluetooth-next tree.

> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index 56f9edb..e1beb4e 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -503,9 +503,9 @@ static int hci_sock_release(struct socket *sock)
>
> if (hdev) {
> if (hci_pi(sk)->channel == HCI_CHANNEL_USER) {
> - mgmt_index_added(hdev);
> - hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
> hci_dev_close(hdev->id);
> + hci_dev_clear_flag(hdev, HCI_USER_CHANNEL);
> + mgmt_index_added(hdev);

However I still do not remember why I did it the other way around in the first place. Can someone please double check that this will not break anything. I do not trust my own judgment in this case. It feels suspicious.

Regards

Marcel