diff mbox

[1/3] pno: Change sched_scan_stopped event to handle pending PNO properly

Message ID 1392283441-13612-1-git-send-email-ilan.peer@intel.com
State Superseded
Headers show

Commit Message

Ilan Peer Feb. 13, 2014, 9:23 a.m. UTC 
From: Alexander Bondar <alexander.bondar@intel.com>

1. Move pno_start and pno_stop to scan.c as more relevant location. Rename
   them to wpa_supplicant_start_pno/stop_pno.
2. Change wpa_supplicant_stop_pno so that nothing will be done if PNO is
   not in progress.
3. When a sched_scan_stopped event is received and there is a pending PNO,
   it uses regular scheduled scan parameters instead of PNO specific
   parameters. Change it by calling wpa_supplicant_start_pno.

Signed-hostap: Alexander Bondar <alexander.bondar@intel.com>
---
 wpa_supplicant/ctrl_iface.c |  106 +------------------------------------------
 wpa_supplicant/events.c     |   15 ++----
 wpa_supplicant/scan.c       |  106 +++++++++++++++++++++++++++++++++++++++++++
 wpa_supplicant/scan.h       |    2 +
 4 files changed, 115 insertions(+), 114 deletions(-)

Comments

Jouni Malinen Feb. 25, 2014, 1:52 p.m. UTC | #1 
On Thu, Feb 13, 2014 at 11:23:59AM +0200, Ilan Peer wrote:
> From: Alexander Bondar <alexander.bondar@intel.com>
> 
> 1. Move pno_start and pno_stop to scan.c as more relevant location. Rename
>    them to wpa_supplicant_start_pno/stop_pno.
> 2. Change wpa_supplicant_stop_pno so that nothing will be done if PNO is
>    not in progress.
> 3. When a sched_scan_stopped event is received and there is a pending PNO,
>    it uses regular scheduled scan parameters instead of PNO specific
>    parameters. Change it by calling wpa_supplicant_start_pno.

Could you please split this type of changes into two or more commits? I
want to be able to see the real changes separately both now when
reviewing this and when looking at commits later. Moving functions in
the same commit while changing them makes this unnecessarily painful. In
addition, I'm moving more and more towards using "wpas_" prefix instead
of "wpa_supplicant_" to avoid long function names.
Ilan Peer Feb. 25, 2014, 2:11 p.m. UTC | #2 
> -----Original Message-----
> From: Jouni Malinen [mailto:j@w1.fi]
> Sent: Tuesday, February 25, 2014 15:52
> To: Peer, Ilan
> Cc: hostap@lists.shmoo.com
> Subject: Re: [PATCH 1/3] pno: Change sched_scan_stopped event to handle
> pending PNO properly
> 
> On Thu, Feb 13, 2014 at 11:23:59AM +0200, Ilan Peer wrote:
> > From: Alexander Bondar <alexander.bondar@intel.com>
> >
> > 1. Move pno_start and pno_stop to scan.c as more relevant location.
> Rename
> >    them to wpa_supplicant_start_pno/stop_pno.
> > 2. Change wpa_supplicant_stop_pno so that nothing will be done if PNO is
> >    not in progress.
> > 3. When a sched_scan_stopped event is received and there is a pending
> PNO,
> >    it uses regular scheduled scan parameters instead of PNO specific
> >    parameters. Change it by calling wpa_supplicant_start_pno.
> 
> Could you please split this type of changes into two or more commits? I want
> to be able to see the real changes separately both now when reviewing this
> and when looking at commits later. Moving functions in the same commit
> while changing them makes this unnecessarily painful. In addition, I'm
> moving more and more towards using "wpas_" prefix instead of
> "wpa_supplicant_" to avoid long function names.
> 


Np. You want this to for future patches or this one as well?

Thanks,

Ilan.
Jouni Malinen Feb. 25, 2014, 2:57 p.m. UTC | #3 
On Tue, Feb 25, 2014 at 02:11:24PM +0000, Peer, Ilan wrote:
> Np. You want this to for future patches or this one as well?

In general, so both included. I would end up having to manually undo the
function move and renaming here to be able to review this in the first
place.. I may have done that in the past myself for some contributions,
but I'm getting lazier.. :)
Ilan Peer Feb. 25, 2014, 3:01 p.m. UTC | #4 
> -----Original Message-----
> From: Jouni Malinen [mailto:j@w1.fi]
> Sent: Tuesday, February 25, 2014 16:58
> To: Peer, Ilan
> Cc: hostap@lists.shmoo.com
> Subject: Re: [PATCH 1/3] pno: Change sched_scan_stopped event to handle
> pending PNO properly
> 
> On Tue, Feb 25, 2014 at 02:11:24PM +0000, Peer, Ilan wrote:
> > Np. You want this to for future patches or this one as well?
> 
> In general, so both included. I would end up having to manually undo the
> function move and renaming here to be able to review this in the first place..
> I may have done that in the past myself for some contributions, but I'm
> getting lazier.. :)
> 

Hehe ... np, will resubmit as 2 different patches. 

Thanks,

Ilan.
diff mbox

Patch

diff --git a/wpa_supplicant/ctrl_iface.c b/wpa_supplicant/ctrl_iface.c
index ddddad3..4047b64 100644
--- a/wpa_supplicant/ctrl_iface.c
+++ b/wpa_supplicant/ctrl_iface.c
@@ -46,108 +46,6 @@  static int wpa_supplicant_global_iface_list(struct wpa_global *global,
 static int wpa_supplicant_global_iface_interfaces(struct wpa_global *global,
 						  char *buf, int len);
 
-
-static int pno_start(struct wpa_supplicant *wpa_s)
-{
-	int ret, interval;
-	size_t i, num_ssid;
-	struct wpa_ssid *ssid;
-	struct wpa_driver_scan_params params;
-
-	if (wpa_s->pno || wpa_s->pno_sched_pending)
-		return 0;
-
-	if ((wpa_s->wpa_state > WPA_SCANNING) &&
-	    (wpa_s->wpa_state <= WPA_COMPLETED)) {
-		wpa_printf(MSG_ERROR, "PNO: In assoc process");
-		return -EAGAIN;
-	}
-
-	if (wpa_s->wpa_state == WPA_SCANNING) {
-		wpa_supplicant_cancel_scan(wpa_s);
-		if (wpa_s->sched_scanning) {
-			wpa_printf(MSG_DEBUG, "Schedule PNO on completion of "
-				   "ongoing sched scan");
-			wpa_supplicant_cancel_sched_scan(wpa_s);
-			wpa_s->pno_sched_pending = 1;
-			return 0;
-		}
-	}
-
-	os_memset(&params, 0, sizeof(params));
-
-	num_ssid = 0;
-	ssid = wpa_s->conf->ssid;
-	while (ssid) {
-		if (!wpas_network_disabled(wpa_s, ssid))
-			num_ssid++;
-		ssid = ssid->next;
-	}
-	if (num_ssid > WPAS_MAX_SCAN_SSIDS) {
-		wpa_printf(MSG_DEBUG, "PNO: Use only the first %u SSIDs from "
-			   "%u", WPAS_MAX_SCAN_SSIDS, (unsigned int) num_ssid);
-		num_ssid = WPAS_MAX_SCAN_SSIDS;
-	}
-
-	if (num_ssid == 0) {
-		wpa_printf(MSG_DEBUG, "PNO: No configured SSIDs");
-		return -1;
-	}
-
-	params.filter_ssids = os_malloc(sizeof(struct wpa_driver_scan_filter) *
-					num_ssid);
-	if (params.filter_ssids == NULL)
-		return -1;
-	i = 0;
-	ssid = wpa_s->conf->ssid;
-	while (ssid) {
-		if (!wpas_network_disabled(wpa_s, ssid)) {
-			params.ssids[i].ssid = ssid->ssid;
-			params.ssids[i].ssid_len = ssid->ssid_len;
-			params.num_ssids++;
-			os_memcpy(params.filter_ssids[i].ssid, ssid->ssid,
-				  ssid->ssid_len);
-			params.filter_ssids[i].ssid_len = ssid->ssid_len;
-			params.num_filter_ssids++;
-			i++;
-			if (i == num_ssid)
-				break;
-		}
-		ssid = ssid->next;
-	}
-
-	if (wpa_s->conf->filter_rssi)
-		params.filter_rssi = wpa_s->conf->filter_rssi;
-
-	interval = wpa_s->conf->sched_scan_interval ?
-		wpa_s->conf->sched_scan_interval : 10;
-
-	ret = wpa_supplicant_start_sched_scan(wpa_s, &params, interval);
-	os_free(params.filter_ssids);
-	if (ret == 0)
-		wpa_s->pno = 1;
-	return ret;
-}
-
-
-static int pno_stop(struct wpa_supplicant *wpa_s)
-{
-	int ret = 0;
-
-	if (wpa_s->pno || wpa_s->sched_scanning) {
-		wpa_s->pno = 0;
-		ret = wpa_supplicant_stop_sched_scan(wpa_s);
-	}
-
-	wpa_s->pno_sched_pending = 0;
-
-	if (wpa_s->wpa_state == WPA_SCANNING)
-		wpa_supplicant_req_scan(wpa_s, 0, 0);
-
-	return ret;
-}
-
-
 static int set_bssid_filter(struct wpa_supplicant *wpa_s, char *val)
 {
 	char *pos;
@@ -391,9 +289,9 @@  static int wpa_supplicant_ctrl_iface_set(struct wpa_supplicant *wpa_s,
 #endif /* CONFIG_TDLS */
 	} else if (os_strcasecmp(cmd, "pno") == 0) {
 		if (atoi(value))
-			ret = pno_start(wpa_s);
+			ret = wpa_supplicant_start_pno(wpa_s);
 		else
-			ret = pno_stop(wpa_s);
+			ret = wpa_supplicant_stop_pno(wpa_s);
 	} else if (os_strcasecmp(cmd, "radio_disabled") == 0) {
 		int disabled = atoi(value);
 		if (wpa_drv_radio_disable(wpa_s, disabled) < 0)
diff --git a/wpa_supplicant/events.c b/wpa_supplicant/events.c
index a72f2fa..182b473 100644
--- a/wpa_supplicant/events.c
+++ b/wpa_supplicant/events.c
@@ -3300,17 +3300,12 @@  void wpa_supplicant_event(void *ctx, enum wpa_event_type event,
 		 * Start a new sched scan to continue searching for more SSIDs
 		 * either if timed out or PNO schedule scan is pending.
 		 */
-		if (wpa_s->sched_scan_timed_out || wpa_s->pno_sched_pending) {
-
-			if (wpa_supplicant_req_sched_scan(wpa_s) < 0 &&
-			    wpa_s->pno_sched_pending) {
-				wpa_msg(wpa_s, MSG_ERROR, "Failed to schedule PNO");
-			} else if (wpa_s->pno_sched_pending) {
-				wpa_s->pno_sched_pending = 0;
-				wpa_s->pno = 1;
-			}
+		if (wpa_s->sched_scan_timed_out)
+			wpa_supplicant_req_sched_scan(wpa_s);
+		else if (wpa_s->pno_sched_pending) {
+			wpa_s->pno_sched_pending = 0;
+			wpa_supplicant_start_pno(wpa_s);
 		}
-
 		break;
 	case EVENT_WPS_BUTTON_PUSHED:
 #ifdef CONFIG_WPS
diff --git a/wpa_supplicant/scan.c b/wpa_supplicant/scan.c
index 18d243e..2d1eb2a 100644
--- a/wpa_supplicant/scan.c
+++ b/wpa_supplicant/scan.c
@@ -1801,3 +1801,109 @@  void wpa_scan_free_params(struct wpa_driver_scan_params *params)
 	os_free(params->filter_ssids);
 	os_free(params);
 }
+
+int wpa_supplicant_start_pno(struct wpa_supplicant *wpa_s)
+{
+	int ret, interval;
+	size_t i, num_ssid;
+	struct wpa_ssid *ssid;
+	struct wpa_driver_scan_params params;
+
+	if (!wpa_s->sched_scan_supported)
+		return -1;
+
+	if (wpa_s->pno || wpa_s->pno_sched_pending)
+		return 0;
+
+	if ((wpa_s->wpa_state > WPA_SCANNING) &&
+	    (wpa_s->wpa_state <= WPA_COMPLETED)) {
+		wpa_printf(MSG_ERROR, "PNO: In assoc process");
+		return -EAGAIN;
+	}
+
+	if (wpa_s->wpa_state == WPA_SCANNING) {
+		wpa_supplicant_cancel_scan(wpa_s);
+		if (wpa_s->sched_scanning) {
+			wpa_printf(MSG_DEBUG, "Schedule PNO on completion of "
+				   "ongoing sched scan");
+			wpa_supplicant_cancel_sched_scan(wpa_s);
+			wpa_s->pno_sched_pending = 1;
+			return 0;
+		}
+	}
+
+	os_memset(&params, 0, sizeof(params));
+
+	num_ssid = 0;
+	ssid = wpa_s->conf->ssid;
+	while (ssid) {
+		if (!wpas_network_disabled(wpa_s, ssid))
+			num_ssid++;
+		ssid = ssid->next;
+	}
+	if (num_ssid > WPAS_MAX_SCAN_SSIDS) {
+		wpa_printf(MSG_DEBUG, "PNO: Use only the first %u SSIDs from "
+			   "%u", WPAS_MAX_SCAN_SSIDS, (unsigned int) num_ssid);
+		num_ssid = WPAS_MAX_SCAN_SSIDS;
+	}
+
+	if (num_ssid == 0) {
+		wpa_printf(MSG_DEBUG, "PNO: No configured SSIDs");
+		return -1;
+	}
+
+	params.filter_ssids = os_malloc(sizeof(struct wpa_driver_scan_filter) *
+					num_ssid);
+	if (params.filter_ssids == NULL)
+		return -1;
+	i = 0;
+	ssid = wpa_s->conf->ssid;
+	while (ssid) {
+		if (!wpas_network_disabled(wpa_s, ssid)) {
+			params.ssids[i].ssid = ssid->ssid;
+			params.ssids[i].ssid_len = ssid->ssid_len;
+			params.num_ssids++;
+			os_memcpy(params.filter_ssids[i].ssid, ssid->ssid,
+				  ssid->ssid_len);
+			params.filter_ssids[i].ssid_len = ssid->ssid_len;
+			params.num_filter_ssids++;
+			i++;
+			if (i == num_ssid)
+				break;
+		}
+		ssid = ssid->next;
+	}
+
+	if (wpa_s->conf->filter_rssi)
+		params.filter_rssi = wpa_s->conf->filter_rssi;
+
+	interval = wpa_s->conf->sched_scan_interval ?
+		wpa_s->conf->sched_scan_interval : 10;
+
+	ret = wpa_supplicant_start_sched_scan(wpa_s, &params, interval);
+	os_free(params.filter_ssids);
+	if (ret == 0)
+		wpa_s->pno = 1;
+	else
+		wpa_msg(wpa_s, MSG_ERROR, "Failed to schedule PNO");
+	return ret;
+}
+
+
+int wpa_supplicant_stop_pno(struct wpa_supplicant *wpa_s)
+{
+	int ret = 0;
+
+	if (!wpa_s->pno)
+		return 0;
+
+	ret = wpa_supplicant_stop_sched_scan(wpa_s);
+
+	wpa_s->pno = 0;
+	wpa_s->pno_sched_pending = 0;
+
+	if (wpa_s->wpa_state == WPA_SCANNING)
+		wpa_supplicant_req_scan(wpa_s, 0, 0);
+
+	return ret;
+}
diff --git a/wpa_supplicant/scan.h b/wpa_supplicant/scan.h
index e4c8989..aae0b62 100644
--- a/wpa_supplicant/scan.h
+++ b/wpa_supplicant/scan.h
@@ -46,5 +46,7 @@  int wpa_supplicant_stop_sched_scan(struct wpa_supplicant *wpa_s);
 struct wpa_driver_scan_params *
 wpa_scan_clone_params(const struct wpa_driver_scan_params *src);
 void wpa_scan_free_params(struct wpa_driver_scan_params *params);
+int wpa_supplicant_start_pno(struct wpa_supplicant *wpa_s);
+int wpa_supplicant_stop_pno(struct wpa_supplicant *wpa_s);
 
 #endif /* SCAN_H */