[PATCH 1/2] Bluetooth: Rename mgmt_inquiry_failed()

[PATCH 1/2] Bluetooth: Rename mgmt_inquiry_failed()

[Andre Guedes]

This patch renames the function mgmt_inquiry_failed() to
mgmt_start_discovery_failed(). This function is more related
to MGMT_OP_START_DISCOVERY command handling than to inquiry.
Besides, this functions will be reused by LE based discovery
procedures in case of failure.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e6071d0..4e00ae0 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -931,7 +931,7 @@ int mgmt_read_local_oob_data_reply_complete(u16 index, u8 *hash, u8 *randomizer,
 int mgmt_device_found(u16 index, bdaddr_t *bdaddr, u8 type, u8 *dev_class,
 							s8 rssi, u8 *eir);
 int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name);
-int mgmt_inquiry_failed(u16 index, u8 status);
+int mgmt_start_discovery_failed(u16 index, u8 status);
 int mgmt_discovering(u16 index, u8 discovering);
 int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr);
 int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 2fced8c..e29a3b1 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -986,7 +986,7 @@ static inline void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
 		hci_req_complete(hdev, HCI_OP_INQUIRY, status);
 		hci_conn_check_pending(hdev);
 		if (test_bit(HCI_MGMT, &hdev->flags))
-			mgmt_inquiry_failed(hdev->id, status);
+			mgmt_start_discovery_failed(hdev->id, status);
 		return;
 	}
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 4cb2f95..dc938dc 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2379,7 +2379,7 @@ int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name)
 	return mgmt_event(MGMT_EV_REMOTE_NAME, index, &ev, sizeof(ev), NULL);
 }
 
-int mgmt_inquiry_failed(u16 index, u8 status)
+int mgmt_start_discovery_failed(u16 index, u8 status)
 {
 	struct pending_cmd *cmd;
 	int err;
-- 
1.7.7.1

[PATCH 2/2] Bluetooth: mgmt_stop_discovery_failed()

[Andre Guedes]

This patches creates mgmt_stop_discovery_failed() which removes
pending MGMT_OP_STOP_DISCOVERY commands and sends proper command
status events.

This patch also fixes the MGMT_OP_STOP_DISCOVERY command leak in
case cancel inquiry fails.

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

diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 4e00ae0..dba10c6 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -932,6 +932,7 @@ int mgmt_device_found(u16 index, bdaddr_t *bdaddr, u8 type, u8 *dev_class,
 							s8 rssi, u8 *eir);
 int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name);
 int mgmt_start_discovery_failed(u16 index, u8 status);
+int mgmt_stop_discovery_failed(u16 index, u8 status);
 int mgmt_discovering(u16 index, u8 discovering);
 int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr);
 int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr);
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index e29a3b1..78ba9c8 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -55,8 +55,10 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
 
 	BT_DBG("%s status 0x%x", hdev->name, status);
 
-	if (status)
+	if (status) {
+		mgmt_stop_discovery_failed(hdev->id, status);
 		return;
+	}
 
 	clear_bit(HCI_INQUIRY, &hdev->flags);
 
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index dc938dc..d840600 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -2394,6 +2394,21 @@ int mgmt_start_discovery_failed(u16 index, u8 status)
 	return err;
 }
 
+int mgmt_stop_discovery_failed(u16 index, u8 status)
+{
+	struct pending_cmd *cmd;
+	int err;
+
+	cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index);
+	if (!cmd)
+		return -ENOENT;
+
+	err = cmd_status(cmd->sk, index, cmd->opcode, status);
+	mgmt_pending_remove(cmd);
+
+	return err;
+}
+
 int mgmt_discovering(u16 index, u8 discovering)
 {
 	struct pending_cmd *cmd;
-- 
1.7.7.1

Re: [PATCH 2/2] Bluetooth: mgmt_stop_discovery_failed()

[Marcel Holtmann]

Hi Andre,

> This patches creates mgmt_stop_discovery_failed() which removes
> pending MGMT_OP_STOP_DISCOVERY commands and sends proper command
> status events.
> 
> This patch also fixes the MGMT_OP_STOP_DISCOVERY command leak in
> case cancel inquiry fails.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    1 +
>  net/bluetooth/hci_event.c        |    4 +++-
>  net/bluetooth/mgmt.c             |   15 +++++++++++++++
>  3 files changed, 19 insertions(+), 1 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index 4e00ae0..dba10c6 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -932,6 +932,7 @@ int mgmt_device_found(u16 index, bdaddr_t *bdaddr, u8 type, u8 *dev_class,
>  							s8 rssi, u8 *eir);
>  int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name);
>  int mgmt_start_discovery_failed(u16 index, u8 status);
> +int mgmt_stop_discovery_failed(u16 index, u8 status);
>  int mgmt_discovering(u16 index, u8 discovering);
>  int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr);
>  int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr);
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index e29a3b1..78ba9c8 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -55,8 +55,10 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
>  
>  	BT_DBG("%s status 0x%x", hdev->name, status);
>  
> -	if (status)
> +	if (status) {
> +		mgmt_stop_discovery_failed(hdev->id, status);
>  		return;
> +	}
>  
>  	clear_bit(HCI_INQUIRY, &hdev->flags);
>  
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index dc938dc..d840600 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2394,6 +2394,21 @@ int mgmt_start_discovery_failed(u16 index, u8 status)
>  	return err;
>  }
>  
> +int mgmt_stop_discovery_failed(u16 index, u8 status)
> +{
> +	struct pending_cmd *cmd;
> +	int err;
> +
> +	cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index);
> +	if (!cmd)
> +		return -ENOENT;
> +
> +	err = cmd_status(cmd->sk, index, cmd->opcode, status);
> +	mgmt_pending_remove(cmd);
> +
> +	return err;
> +}
> +

this looks like something that we might repeat over and over again.
Would it not make sense to have a proper helper for this?

Regards

Marcel

Re: [PATCH 2/2] Bluetooth: mgmt_stop_discovery_failed()

[Andre Guedes]

Hi Marcel,

On Nov 9, 2011, at 4:39 AM, Marcel Holtmann wrote:

> Hi Andre,
> 
>> This patches creates mgmt_stop_discovery_failed() which removes
>> pending MGMT_OP_STOP_DISCOVERY commands and sends proper command
>> status events.
>> 
>> This patch also fixes the MGMT_OP_STOP_DISCOVERY command leak in
>> case cancel inquiry fails.
>> 
>> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
>> ---
>> include/net/bluetooth/hci_core.h |    1 +
>> net/bluetooth/hci_event.c        |    4 +++-
>> net/bluetooth/mgmt.c             |   15 +++++++++++++++
>> 3 files changed, 19 insertions(+), 1 deletions(-)
>> 
>> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
>> index 4e00ae0..dba10c6 100644
>> --- a/include/net/bluetooth/hci_core.h
>> +++ b/include/net/bluetooth/hci_core.h
>> @@ -932,6 +932,7 @@ int mgmt_device_found(u16 index, bdaddr_t *bdaddr, u8 type, u8 *dev_class,
>> 							s8 rssi, u8 *eir);
>> int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name);
>> int mgmt_start_discovery_failed(u16 index, u8 status);
>> +int mgmt_stop_discovery_failed(u16 index, u8 status);
>> int mgmt_discovering(u16 index, u8 discovering);
>> int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr);
>> int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr);
>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>> index e29a3b1..78ba9c8 100644
>> --- a/net/bluetooth/hci_event.c
>> +++ b/net/bluetooth/hci_event.c
>> @@ -55,8 +55,10 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
>> 
>> 	BT_DBG("%s status 0x%x", hdev->name, status);
>> 
>> -	if (status)
>> +	if (status) {
>> +		mgmt_stop_discovery_failed(hdev->id, status);
>> 		return;
>> +	}
>> 
>> 	clear_bit(HCI_INQUIRY, &hdev->flags);
>> 
>> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
>> index dc938dc..d840600 100644
>> --- a/net/bluetooth/mgmt.c
>> +++ b/net/bluetooth/mgmt.c
>> @@ -2394,6 +2394,21 @@ int mgmt_start_discovery_failed(u16 index, u8 status)
>> 	return err;
>> }
>> 
>> +int mgmt_stop_discovery_failed(u16 index, u8 status)
>> +{
>> +	struct pending_cmd *cmd;
>> +	int err;
>> +
>> +	cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index);
>> +	if (!cmd)
>> +		return -ENOENT;
>> +
>> +	err = cmd_status(cmd->sk, index, cmd->opcode, status);
>> +	mgmt_pending_remove(cmd);
>> +
>> +	return err;
>> +}
>> +
> 
> this looks like something that we might repeat over and over again.
> Would it not make sense to have a proper helper for this?

Yes, it seems we have three mgmt_*_failed functions that do the
same thing: find command, send cmd_status and remove command.
They are:

mgmt_disconnect_failed()
mgmt_start_discovery_failed()
mgmt_stop_discovery_failed()

We can create a helper function like mgmt_cmd_failed() that would
remove the command and would send the command status event. Then
we can do some refactoring in those three functions so they use
the mgmt_cmd_failed() function.

Since this patch fixes a mgmt command leak in the pending list
and the refactoring is not critical, IMO, we should apply it and
do the refactoring thing later.

BR,

Andre

Re: [PATCH 2/2] Bluetooth: mgmt_stop_discovery_failed()

[Marcel Holtmann]

Hi Andre,

> >> This patches creates mgmt_stop_discovery_failed() which removes
> >> pending MGMT_OP_STOP_DISCOVERY commands and sends proper command
> >> status events.
> >> 
> >> This patch also fixes the MGMT_OP_STOP_DISCOVERY command leak in
> >> case cancel inquiry fails.
> >> 
> >> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> >> ---
> >> include/net/bluetooth/hci_core.h |    1 +
> >> net/bluetooth/hci_event.c        |    4 +++-
> >> net/bluetooth/mgmt.c             |   15 +++++++++++++++
> >> 3 files changed, 19 insertions(+), 1 deletions(-)
> >> 
> >> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> >> index 4e00ae0..dba10c6 100644
> >> --- a/include/net/bluetooth/hci_core.h
> >> +++ b/include/net/bluetooth/hci_core.h
> >> @@ -932,6 +932,7 @@ int mgmt_device_found(u16 index, bdaddr_t *bdaddr, u8 type, u8 *dev_class,
> >> 							s8 rssi, u8 *eir);
> >> int mgmt_remote_name(u16 index, bdaddr_t *bdaddr, u8 *name);
> >> int mgmt_start_discovery_failed(u16 index, u8 status);
> >> +int mgmt_stop_discovery_failed(u16 index, u8 status);
> >> int mgmt_discovering(u16 index, u8 discovering);
> >> int mgmt_device_blocked(u16 index, bdaddr_t *bdaddr);
> >> int mgmt_device_unblocked(u16 index, bdaddr_t *bdaddr);
> >> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> >> index e29a3b1..78ba9c8 100644
> >> --- a/net/bluetooth/hci_event.c
> >> +++ b/net/bluetooth/hci_event.c
> >> @@ -55,8 +55,10 @@ static void hci_cc_inquiry_cancel(struct hci_dev *hdev, struct sk_buff *skb)
> >> 
> >> 	BT_DBG("%s status 0x%x", hdev->name, status);
> >> 
> >> -	if (status)
> >> +	if (status) {
> >> +		mgmt_stop_discovery_failed(hdev->id, status);
> >> 		return;
> >> +	}
> >> 
> >> 	clear_bit(HCI_INQUIRY, &hdev->flags);
> >> 
> >> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> >> index dc938dc..d840600 100644
> >> --- a/net/bluetooth/mgmt.c
> >> +++ b/net/bluetooth/mgmt.c
> >> @@ -2394,6 +2394,21 @@ int mgmt_start_discovery_failed(u16 index, u8 status)
> >> 	return err;
> >> }
> >> 
> >> +int mgmt_stop_discovery_failed(u16 index, u8 status)
> >> +{
> >> +	struct pending_cmd *cmd;
> >> +	int err;
> >> +
> >> +	cmd = mgmt_pending_find(MGMT_OP_STOP_DISCOVERY, index);
> >> +	if (!cmd)
> >> +		return -ENOENT;
> >> +
> >> +	err = cmd_status(cmd->sk, index, cmd->opcode, status);
> >> +	mgmt_pending_remove(cmd);
> >> +
> >> +	return err;
> >> +}
> >> +
> > 
> > this looks like something that we might repeat over and over again.
> > Would it not make sense to have a proper helper for this?
> 
> Yes, it seems we have three mgmt_*_failed functions that do the
> same thing: find command, send cmd_status and remove command.
> They are:
> 
> mgmt_disconnect_failed()
> mgmt_start_discovery_failed()
> mgmt_stop_discovery_failed()
> 
> We can create a helper function like mgmt_cmd_failed() that would
> remove the command and would send the command status event. Then
> we can do some refactoring in those three functions so they use
> the mgmt_cmd_failed() function.
> 
> Since this patch fixes a mgmt command leak in the pending list
> and the refactoring is not critical, IMO, we should apply it and
> do the refactoring thing later.

if you send the re-factoring patch right away, I am fine if we go ahead
with this patch. However some time later is not acceptable. If we do
that, then this gets forgotten and next time you add another function
that does exactly the same thing.

Regards

Marcel

Re: [PATCH 1/2] Bluetooth: Rename mgmt_inquiry_failed()

[Marcel Holtmann]

Hi Andre,

> This patch renames the function mgmt_inquiry_failed() to
> mgmt_start_discovery_failed(). This function is more related
> to MGMT_OP_START_DISCOVERY command handling than to inquiry.
> Besides, this functions will be reused by LE based discovery
> procedures in case of failure.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    2 +-
>  net/bluetooth/hci_event.c        |    2 +-
>  net/bluetooth/mgmt.c             |    2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

the name change makes sense to be. I hope it also leads to avoiding code
duplication here in the long run.

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

Regards

Marcel

Re: [PATCH 1/2] Bluetooth: Rename mgmt_inquiry_failed()

[Gustavo Padovan]

Hi Andre,

* Andre Guedes <andre.guedes@openbossa.org> [2011-11-08 19:40:01 -0300]:

> This patch renames the function mgmt_inquiry_failed() to
> mgmt_start_discovery_failed(). This function is more related
> to MGMT_OP_START_DISCOVERY command handling than to inquiry.
> Besides, this functions will be reused by LE based discovery
> procedures in case of failure.
> 
> Signed-off-by: Andre Guedes <andre.guedes@openbossa.org>
> ---
>  include/net/bluetooth/hci_core.h |    2 +-
>  net/bluetooth/hci_event.c        |    2 +-
>  net/bluetooth/mgmt.c             |    2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)

It doesn't apply anymore, please rebase on top of bluetooth-next and resend.
Thanks.

	Gustavo