[PATCH] Bluetooth: Fix mgmt handling of power on failures

[PATCH] Bluetooth: Fix mgmt handling of power on failures

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

If hci_dev_open fails we need to ensure that the corresponding
mgmt_set_powered command gets an appropriate response. This patch fixes
the missing response by adding a new mgmt_set_powered_failed function
that's used to indicate a power on failure to mgmt.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
---
 include/net/bluetooth/hci_core.h |    1 +
 net/bluetooth/hci_core.c         |    6 +++++-
 net/bluetooth/mgmt.c             |   16 ++++++++++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 606615c..f77885e 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1118,6 +1118,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
 int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
 int mgmt_index_added(struct hci_dev *hdev);
 int mgmt_index_removed(struct hci_dev *hdev);
+int mgmt_set_powered_failed(struct hci_dev *hdev, int err);
 int mgmt_powered(struct hci_dev *hdev, u8 powered);
 int mgmt_discoverable(struct hci_dev *hdev, u8 discoverable);
 int mgmt_connectable(struct hci_dev *hdev, u8 connectable);
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 21ad85d..d7c2dbd 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1598,11 +1598,15 @@ static const struct rfkill_ops hci_rfkill_ops = {
 static void hci_power_on(struct work_struct *work)
 {
 	struct hci_dev *hdev = container_of(work, struct hci_dev, power_on);
+	int err;
 
 	BT_DBG("%s", hdev->name);
 
-	if (hci_dev_open(hdev->id) < 0)
+	err = hci_dev_open(hdev->id);
+	if (err < 0) {
+		mgmt_set_powered_failed(hdev, err);
 		return;
+	}
 
 	if (test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
 		queue_delayed_work(hdev->req_workqueue, &hdev->power_off,
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 5cd3aee..00b006b 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3533,6 +3533,22 @@ new_settings:
 	return err;
 }
 
+int mgmt_set_powered_failed(struct hci_dev *hdev, int err)
+{
+	struct pending_cmd *cmd;
+
+	cmd = mgmt_pending_find(MGMT_OP_SET_POWERED, hdev);
+	if (!cmd)
+		return -ENOENT;
+
+	err = cmd_status(cmd->sk, hdev->id, MGMT_OP_SET_POWERED,
+			 MGMT_STATUS_FAILED);
+
+	mgmt_pending_remove(cmd);
+
+	return err;
+}
+
 int mgmt_discoverable(struct hci_dev *hdev, u8 discoverable)
 {
 	struct cmd_lookup match = { NULL, hdev };
-- 
1.7.10.4

Re: [PATCH] Bluetooth: Fix mgmt handling of power on failures

[Anderson Lizardo]

Hi Johan,

On Tue, May 28, 2013 at 3:07 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> +int mgmt_set_powered_failed(struct hci_dev *hdev, int err)
> +{
> +       struct pending_cmd *cmd;
> +
> +       cmd = mgmt_pending_find(MGMT_OP_SET_POWERED, hdev);
> +       if (!cmd)
> +               return -ENOENT;
> +
> +       err = cmd_status(cmd->sk, hdev->id, MGMT_OP_SET_POWERED,
> +                        MGMT_STATUS_FAILED);
> +
> +       mgmt_pending_remove(cmd);
> +
> +       return err;
> +}
> +

Was the "err" parameter value supposed to be used on this function?

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH] Bluetooth: Fix mgmt handling of power on failures

[Johan Hedberg]

Hi Lizardo,

On Tue, May 28, 2013, Anderson Lizardo wrote:
> On Tue, May 28, 2013 at 3:07 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> > +int mgmt_set_powered_failed(struct hci_dev *hdev, int err)
> > +{
> > +       struct pending_cmd *cmd;
> > +
> > +       cmd = mgmt_pending_find(MGMT_OP_SET_POWERED, hdev);
> > +       if (!cmd)
> > +               return -ENOENT;
> > +
> > +       err = cmd_status(cmd->sk, hdev->id, MGMT_OP_SET_POWERED,
> > +                        MGMT_STATUS_FAILED);
> > +
> > +       mgmt_pending_remove(cmd);
> > +
> > +       return err;
> > +}
> > +
> 
> Was the "err" parameter value supposed to be used on this function?

As I mentioned in my reply to Marcel it's there so this can be easily
extended to have a special mapping to the exact mgmt error returned (and
now it seems that this mapping will be implemented before this goes
upstream).

Johan

Re: [PATCH] Bluetooth: Fix mgmt handling of power on failures

[Gustavo Padovan]

Hi Johan,

* Johan Hedberg <johan.hedberg@gmail.com> [2013-05-28 10:07:47 +0300]:

> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> If hci_dev_open fails we need to ensure that the corresponding
> mgmt_set_powered command gets an appropriate response. This patch fixes
> the missing response by adding a new mgmt_set_powered_failed function
> that's used to indicate a power on failure to mgmt.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> ---
>  include/net/bluetooth/hci_core.h |    1 +
>  net/bluetooth/hci_core.c         |    6 +++++-
>  net/bluetooth/mgmt.c             |   16 ++++++++++++++++
>  3 files changed, 22 insertions(+), 1 deletion(-)

Patch has been applied to bluetooth.git. Thanks.

	Gustavo

Re: [PATCH] Bluetooth: Fix mgmt handling of power on failures

[Marcel Holtmann]

Hi Gustavo,

>> If hci_dev_open fails we need to ensure that the corresponding
>> mgmt_set_powered command gets an appropriate response. This patch fixes
>> the missing response by adding a new mgmt_set_powered_failed function
>> that's used to indicate a power on failure to mgmt.
>> 
>> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
>> ---
>> include/net/bluetooth/hci_core.h |    1 +
>> net/bluetooth/hci_core.c         |    6 +++++-
>> net/bluetooth/mgmt.c             |   16 ++++++++++++++++
>> 3 files changed, 22 insertions(+), 1 deletion(-)
> 
> Patch has been applied to bluetooth.git. Thanks.

I rather have this fixed properly with a new error for specifically ERFKILL. Having one stable kernel return rejected and another rfkill is a bad idea.

Regards

Marcel

[PATCH v2] Bluetooth: Fix mgmt handling of power on failures

[Johan Hedberg]

From: Johan Hedberg <johan.hedberg@intel.com>

If hci_dev_open fails we need to ensure that the corresponding
mgmt_set_powered command gets an appropriate response. This patch fixes
the missing response by adding a new mgmt_set_powered_failed function
that's used to indicate a power on failure to mgmt. Since a sutuation
with the device being rfkilled may require special handling in user
space the patch uses a new dedicated mgmt status code for this.

Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
Cc: stable@vger.kernel.org
---
v2: Added dedicated mgmt error for the ERFKILL case

 include/net/bluetooth/hci_core.h |    1 +
 include/net/bluetooth/mgmt.h     |    1 +
 net/bluetooth/hci_core.c         |    6 +++++-
 net/bluetooth/mgmt.c             |   21 +++++++++++++++++++++
 4 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 606615c..f77885e 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1118,6 +1118,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
 int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
 int mgmt_index_added(struct hci_dev *hdev);
 int mgmt_index_removed(struct hci_dev *hdev);
+int mgmt_set_powered_failed(struct hci_dev *hdev, int err);
 int mgmt_powered(struct hci_dev *hdev, u8 powered);
 int mgmt_discoverable(struct hci_dev *hdev, u8 discoverable);
 int mgmt_connectable(struct hci_dev *hdev, u8 connectable);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 22980a7..9944c3e 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -42,6 +42,7 @@
 #define MGMT_STATUS_NOT_POWERED		0x0f
 #define MGMT_STATUS_CANCELLED		0x10
 #define MGMT_STATUS_INVALID_INDEX	0x11
+#define MGMT_STATUS_RFKILLED		0x12
 
 struct mgmt_hdr {
 	__le16	opcode;
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 21ad85d..d7c2dbd 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -1598,11 +1598,15 @@ static const struct rfkill_ops hci_rfkill_ops = {
 static void hci_power_on(struct work_struct *work)
 {
 	struct hci_dev *hdev = container_of(work, struct hci_dev, power_on);
+	int err;
 
 	BT_DBG("%s", hdev->name);
 
-	if (hci_dev_open(hdev->id) < 0)
+	err = hci_dev_open(hdev->id);
+	if (err < 0) {
+		mgmt_set_powered_failed(hdev, err);
 		return;
+	}
 
 	if (test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
 		queue_delayed_work(hdev->req_workqueue, &hdev->power_off,
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 5cd3aee..fedc539 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -3533,6 +3533,27 @@ new_settings:
 	return err;
 }
 
+int mgmt_set_powered_failed(struct hci_dev *hdev, int err)
+{
+	struct pending_cmd *cmd;
+	u8 status;
+
+	cmd = mgmt_pending_find(MGMT_OP_SET_POWERED, hdev);
+	if (!cmd)
+		return -ENOENT;
+
+	if (err == -ERFKILL)
+		status = MGMT_STATUS_RFKILLED;
+	else
+		status = MGMT_STATUS_FAILED;
+
+	err = cmd_status(cmd->sk, hdev->id, MGMT_OP_SET_POWERED, status);
+
+	mgmt_pending_remove(cmd);
+
+	return err;
+}
+
 int mgmt_discoverable(struct hci_dev *hdev, u8 discoverable)
 {
 	struct cmd_lookup match = { NULL, hdev };
-- 
1.7.10.4

Re: [PATCH v2] Bluetooth: Fix mgmt handling of power on failures

[Marcel Holtmann]

Hi Johan,

> If hci_dev_open fails we need to ensure that the corresponding
> mgmt_set_powered command gets an appropriate response. This patch fixes
> the missing response by adding a new mgmt_set_powered_failed function
> that's used to indicate a power on failure to mgmt. Since a situation

might want to fix this typo.

> with the device being rfkilled may require special handling in user
> space the patch uses a new dedicated mgmt status code for this.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> Cc: stable@vger.kernel.org
> ---
> v2: Added dedicated mgmt error for the ERFKILL case
> 
> include/net/bluetooth/hci_core.h |    1 +
> include/net/bluetooth/mgmt.h     |    1 +
> net/bluetooth/hci_core.c         |    6 +++++-
> net/bluetooth/mgmt.c             |   21 +++++++++++++++++++++
> 4 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 606615c..f77885e 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -1118,6 +1118,7 @@ void hci_sock_dev_event(struct hci_dev *hdev, int event);
> int mgmt_control(struct sock *sk, struct msghdr *msg, size_t len);
> int mgmt_index_added(struct hci_dev *hdev);
> int mgmt_index_removed(struct hci_dev *hdev);
> +int mgmt_set_powered_failed(struct hci_dev *hdev, int err);
> int mgmt_powered(struct hci_dev *hdev, u8 powered);
> int mgmt_discoverable(struct hci_dev *hdev, u8 discoverable);
> int mgmt_connectable(struct hci_dev *hdev, u8 connectable);
> diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
> index 22980a7..9944c3e 100644
> --- a/include/net/bluetooth/mgmt.h
> +++ b/include/net/bluetooth/mgmt.h
> @@ -42,6 +42,7 @@
> #define MGMT_STATUS_NOT_POWERED		0x0f
> #define MGMT_STATUS_CANCELLED		0x10
> #define MGMT_STATUS_INVALID_INDEX	0x11
> +#define MGMT_STATUS_RFKILLED		0x12
> 
> struct mgmt_hdr {
> 	__le16	opcode;
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 21ad85d..d7c2dbd 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -1598,11 +1598,15 @@ static const struct rfkill_ops hci_rfkill_ops = {
> static void hci_power_on(struct work_struct *work)
> {
> 	struct hci_dev *hdev = container_of(work, struct hci_dev, power_on);
> +	int err;
> 
> 	BT_DBG("%s", hdev->name);
> 
> -	if (hci_dev_open(hdev->id) < 0)
> +	err = hci_dev_open(hdev->id);
> +	if (err < 0) {
> +		mgmt_set_powered_failed(hdev, err);
> 		return;
> +	}
> 
> 	if (test_bit(HCI_AUTO_OFF, &hdev->dev_flags))
> 		queue_delayed_work(hdev->req_workqueue, &hdev->power_off,
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 5cd3aee..fedc539 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -3533,6 +3533,27 @@ new_settings:
> 	return err;
> }
> 
> +int mgmt_set_powered_failed(struct hci_dev *hdev, int err)
> +{
> +	struct pending_cmd *cmd;
> +	u8 status;
> +
> +	cmd = mgmt_pending_find(MGMT_OP_SET_POWERED, hdev);
> +	if (!cmd)
> +		return -ENOENT;
> +
> +	if (err == -ERFKILL)
> +		status = MGMT_STATUS_RFKILLED;
> +	else
> +		status = MGMT_STATUS_FAILED;
> +
> +	err = cmd_status(cmd->sk, hdev->id, MGMT_OP_SET_POWERED, status);
> +
> +	mgmt_pending_remove(cmd);
> +
> +	return err;
> +}
> +
> int mgmt_discoverable(struct hci_dev *hdev, u8 discoverable)
> {
> 	struct cmd_lookup match = { NULL, hdev };

We do not need to do this in this patch, but I think we need to get rid of all the return values for the status notification functions. Since we never use them, it is pointless to keep them around. Or are we doing anything useful with them.

Anyhow, this patch is fine with me.

Acked-by: Marcel Holtmann <marcel@holtmann.org>

Regards

Marcel

Re: [PATCH v2] Bluetooth: Fix mgmt handling of power on failures

[Gustavo Padovan]

Hi Johan,

* Johan Hedberg <johan.hedberg@gmail.com> [2013-05-29 09:51:29 +0300]:

> From: Johan Hedberg <johan.hedberg@intel.com>
> 
> If hci_dev_open fails we need to ensure that the corresponding
> mgmt_set_powered command gets an appropriate response. This patch fixes
> the missing response by adding a new mgmt_set_powered_failed function
> that's used to indicate a power on failure to mgmt. Since a sutuation
> with the device being rfkilled may require special handling in user
> space the patch uses a new dedicated mgmt status code for this.
> 
> Signed-off-by: Johan Hedberg <johan.hedberg@intel.com>
> Cc: stable@vger.kernel.org
> ---
> v2: Added dedicated mgmt error for the ERFKILL case
> 
>  include/net/bluetooth/hci_core.h |    1 +
>  include/net/bluetooth/mgmt.h     |    1 +
>  net/bluetooth/hci_core.c         |    6 +++++-
>  net/bluetooth/mgmt.c             |   21 +++++++++++++++++++++
>  4 files changed, 28 insertions(+), 1 deletion(-)

Patch has been applied, I fixed the typo too. Thanks.

	Gustavo