Re: [PATCH 3/3 v1] Bluetooth: Fix stop discovery while in STARTING state

[Gustavo Padovan <gustavo@padovan.org>]

Hi Jaganath,

* Jaganath Kanakkassery <jaganath.k@samsung.com> [2012-12-06 16:35:58 +0530]:

> If stop_discovery() is called when discovery state is STARTING, it
> will be failed currently. This patch fixes this.
> 
> Signed-off-by: Jaganath Kanakkassery <jaganath.k@samsung.com>
> ---
>  include/net/bluetooth/hci_core.h |    1 +
>  net/bluetooth/hci_core.c         |    9 ++++++++-
>  net/bluetooth/hci_event.c        |   15 +++++++++++++--
>  net/bluetooth/mgmt.c             |    9 +++++++++
>  4 files changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
> index bd63a9f..d09777a 100644
> --- a/include/net/bluetooth/hci_core.h
> +++ b/include/net/bluetooth/hci_core.h
> @@ -64,6 +64,7 @@ struct discovery_state {
>  		DISCOVERY_RESOLVING,
>  		DISCOVERY_STOPPING,
>  	} state;
> +	int prev_state;
>  	struct list_head	all;	/* All devices found during inquiry */
>  	struct list_head	unknown;	/* Name state not known */
>  	struct list_head	resolve;	/* Name needs to be resolved */
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index c6c67b2..93d0261 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -391,7 +391,13 @@ void hci_discovery_set_state(struct hci_dev *hdev, int state)
>  
>  	switch (state) {
>  	case DISCOVERY_STOPPED:
> -		if (hdev->discovery.state != DISCOVERY_STARTING)
> +		if (hdev->discovery.state == DISCOVERY_STARTING)
> +			break;
> +
> +		if (hdev->discovery.state == DISCOVERY_STOPPING &&
> +		    hdev->discovery.prev_state == DISCOVERY_STARTING)
> +			mgmt_stop_discovery_complete(hdev, 0);
> +		else
>  			mgmt_discovering(hdev, 0);

I don't think we need to be this complicated here, I don't think that add
discovery.prev_state is a good a idea. I would prefer if you add
discovery.discovering that will tell if we are discovering or not, then check
inside mgmt_discovering() if the value is the same and return right away.
You would just need to add the mgmt_stop_discovery_complete() in case state is
DISCOVERY_STOPPING.

>  		break;
>  	case DISCOVERY_STARTING:
> @@ -405,6 +411,7 @@ void hci_discovery_set_state(struct hci_dev *hdev, int state)
>  		break;
>  	}
>  
> +	hdev->discovery.prev_state = hdev->discovery.state;
>  	hdev->discovery.state = state;
>  }
>  
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index 1fadba5..3f9f317 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -1085,6 +1085,8 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>  		if (status) {
>  			hci_dev_lock(hdev);
>  			mgmt_start_discovery_failed(hdev, status);
> +			if (hdev->discovery.state == DISCOVERY_STOPPING)
> +				mgmt_stop_discovery_complete(hdev, status);

I think this case will be handled by hci_discovery_set_state(), and you could
just remove this from here. And why you return the set_scan_enable status
error to the stop discovery command?

>  			hci_dev_unlock(hdev);
>  			return;
>  		}
> @@ -1092,7 +1094,10 @@ static void hci_cc_le_set_scan_enable(struct hci_dev *hdev,
>  		set_bit(HCI_LE_SCAN, &hdev->dev_flags);
>  
>  		hci_dev_lock(hdev);
> -		hci_discovery_set_state(hdev, DISCOVERY_FINDING);
> +		if (hdev->discovery.state == DISCOVERY_STOPPING)
> +			hci_cancel_le_scan(hdev);
> +		else
> +			hci_discovery_set_state(hdev, DISCOVERY_FINDING);
>  		hci_dev_unlock(hdev);
>  		break;
>  
> @@ -1180,8 +1185,11 @@ static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
>  		hci_req_complete(hdev, HCI_OP_INQUIRY, status);
>  		hci_conn_check_pending(hdev);
>  		hci_dev_lock(hdev);
> -		if (test_bit(HCI_MGMT, &hdev->dev_flags))
> +		if (test_bit(HCI_MGMT, &hdev->dev_flags)) {
>  			mgmt_start_discovery_failed(hdev, status);
> +			if (hdev->discovery.state == DISCOVERY_STOPPING)
> +				mgmt_stop_discovery_complete(hdev, status);
> +		}

Saome thing as the previous comment, you don't need this.
mgmt_start_discovery_failed() calls hci_discovery_set_state().

>  		hci_dev_unlock(hdev);
>  		return;
>  	}
> @@ -1189,6 +1197,9 @@ static void hci_cs_inquiry(struct hci_dev *hdev, __u8 status)
>  	set_bit(HCI_INQUIRY, &hdev->flags);
>  
>  	hci_dev_lock(hdev);
> +	if (hdev->discovery.state == DISCOVERY_STOPPING)
> +		hci_cancel_inquiry(hdev);
> +	else
>  	hci_discovery_set_state(hdev, DISCOVERY_FINDING);

Wrong indentation level inside the else.

>  	hci_dev_unlock(hdev);
>  }
> diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
> index 332152b..a645494 100644
> --- a/net/bluetooth/mgmt.c
> +++ b/net/bluetooth/mgmt.c
> @@ -2387,6 +2387,11 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>  
>  	hci_dev_lock(hdev);
>  
> +	if (hdev->discovery.state == DISCOVERY_STARTING) {
> +		err = 0;
> +		goto proceed;
> +	}
> +
>  	if (!hci_discovery_active(hdev)) {

I would do something like this instead:

        if (hdev->discovery.state != DISCOVERY_STARTING &&
	            !hci_discovery_active(hdev)) {


And remove the label jump from here...

>  		err = cmd_complete(sk, hdev->id, MGMT_OP_STOP_DISCOVERY,
>  				   MGMT_STATUS_REJECTED, &mgmt_cp->type,
> @@ -2401,6 +2406,7 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>  		goto unlock;
>  	}
>  
> +proceed:
>  	cmd = mgmt_pending_add(sk, MGMT_OP_STOP_DISCOVERY, hdev, NULL, 0);
>  	if (!cmd) {
>  		err = -ENOMEM;
> @@ -2435,6 +2441,9 @@ static int stop_discovery(struct sock *sk, struct hci_dev *hdev, void *data,
>  
>  		break;
>  
> +	case DISCOVERY_STARTING:

and add err = 0 here.

	Gustavo