[PATCH] libertas: remove user-specified channel list

[PATCH] libertas: remove user-specified channel list

[Holger Schurig]

Remove the ability to specify channels to scan via debugfs

Signed-off-by: Holger Schurig <hs4233@mail.mn-solutions.de>

Index: wireless-2.6/drivers/net/wireless/libertas/README
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/libertas/README	2007-11-28 18:18:45.000000000 +0100
+++ wireless-2.6/drivers/net/wireless/libertas/README	2007-11-28 18:20:09.000000000 +0100
@@ -195,8 +195,6 @@ setuserscan
 
          where [ARGS]:
 
-      chan=[chan#][band][mode] where band is [a,b,g] and mode is
-                               blank for active or 'p' for passive
       bssid=xx:xx:xx:xx:xx:xx  specify a BSSID filter for the scan
       ssid="[SSID]"            specify a SSID filter for the scan
       keep=[0 or 1]            keep the previous scan results (1), discard (0)
@@ -204,35 +202,26 @@ setuserscan
       type=[1,2,3]             BSS type: 1 (Infra), 2(Adhoc), 3(Any)
 
     Any combination of the above arguments can be supplied on the command
-      line.  If the chan token is absent, a full channel scan will be
-      completed by the driver.  If dur tokens are absent, the driver default
-      setting will be used.  The bssid and ssid fields, if blank, will
-      produce an unfiltered scan. The type field will default to 3 (Any) and
-      the keep field will default to 0 (Discard).
+    line. If dur tokens are absent, the driver default setting will be used.
+    The bssid and ssid fields, if blank, will produce an unfiltered scan.
+    The type field will default to 3 (Any) and the keep field will default
+    to 0 (Discard).
 
     Examples:
-    1) Perform an active scan on channels 1, 6, and 11 in the 'g' band:
-            echo "chan=1g,6g,11g" > setuserscan
-
-    2) Perform a passive scan on channel 11 for 20 ms:
-            echo "chan=11gp dur=20" > setuserscan
-
-    3) Perform an active scan on channels 1, 6, and 11; and a passive scan on
-       channel 36 in the 'a' band:
-
-            echo "chan=1g,6g,11g,36ap" > setuserscan
+    1) Perform a passive scan on all channels for 20 ms per channel:
+            echo "dur=20" > setuserscan
 
-    4) Perform an active scan on channel 6 and 36 for a specific SSID:
-            echo "chan=6g,36a ssid="TestAP"" > setuserscan
+    2) Perform an active scan for a specific SSID:
+            echo "ssid="TestAP"" > setuserscan
 
-    5) Scan all available channels (B/G, A bands) for a specific BSSID, keep
+    3) Scan all available channels (B/G, A bands) for a specific BSSID, keep
        the current scan table intact, update existing or append new scan data:
             echo "bssid=00:50:43:20:12:82 keep=1" > setuserscan
 
-    6) Scan channel 6, for all infrastructure networks.
+    4) Scan for all infrastructure networks.
        Keep the previous scan table intact. Update any duplicate BSSID/SSID
        matches with the new scan data:
-            echo "chan=6g type=1 keep=1" > setuserscan
+            echo "type=1 keep=1" > setuserscan
 
     All entries in the scan table (not just the new scan data when keep=1)
     will be displayed upon completion by use of the getscantable ioctl.
Index: wireless-2.6/drivers/net/wireless/libertas/debugfs.c
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/libertas/debugfs.c	2007-11-28 18:22:36.000000000 +0100
+++ wireless-2.6/drivers/net/wireless/libertas/debugfs.c	2007-11-28 18:23:27.000000000 +0100
@@ -196,43 +196,6 @@ out_unlock:
 	return count;
 }
 
-static int lbs_parse_chan(char *buf, size_t count,
-			struct lbs_ioctl_user_scan_cfg *scan_cfg, int dur)
-{
-	char *start, *end, *hold, *str;
-	int i = 0;
-
-	start = strstr(buf, "chan=");
-	if (!start)
-		return -EINVAL;
-	start += 5;
-	end = strchr(start, ' ');
-	if (!end)
-		end = buf + count;
-	hold = kzalloc((end - start)+1, GFP_KERNEL);
-	if (!hold)
-		return -ENOMEM;
-	strncpy(hold, start, end - start);
-	hold[(end-start)+1] = '\0';
-	while(hold && (str = strsep(&hold, ","))) {
-		int chan;
-		char band, passive = 0;
-		sscanf(str, "%d%c%c", &chan, &band, &passive);
-		scan_cfg->chanlist[i].channumber = chan;
-		scan_cfg->chanlist[i].scantype = passive ? 1 : 0;
-		if (band == 'b' || band == 'g')
-			scan_cfg->chanlist[i].radiotype = 0;
-		else if (band == 'a')
-			scan_cfg->chanlist[i].radiotype = 1;
-
-		scan_cfg->chanlist[i].scantime = dur;
-		i++;
-	}
-
-	kfree(hold);
-	return i;
-}
-
 static void lbs_parse_bssid(char *buf, size_t count,
 	struct lbs_ioctl_user_scan_cfg *scan_cfg)
 {
@@ -345,7 +308,6 @@ static ssize_t lbs_setuserscan(struct fi
 	scan_cfg->bsstype = LBS_SCAN_BSS_TYPE_ANY;
 
 	dur = lbs_parse_dur(buf, count, scan_cfg);
-	lbs_parse_chan(buf, count, scan_cfg, dur);
 	lbs_parse_bssid(buf, count, scan_cfg);
 	scan_cfg->clear_bssid = lbs_parse_clear(buf, count, "clear_bssid=");
 	lbs_parse_ssid(buf, count, scan_cfg);
Index: wireless-2.6/drivers/net/wireless/libertas/scan.c
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/libertas/scan.c	2007-11-28 18:21:10.000000000 +0100
+++ wireless-2.6/drivers/net/wireless/libertas/scan.c	2007-11-28 18:23:01.000000000 +0100
@@ -415,11 +415,6 @@ lbs_scan_setup_scan_config(struct lbs_pr
 	struct mrvlietypes_ssidparamset *pssidtlv;
 	struct lbs_scan_cmd_config *pscancfgout = NULL;
 	u8 *ptlvpos;
-	int chanidx;
-	int scantype;
-	int scandur;
-	int channel;
-	int radiotype;
 
 	lbs_deb_enter(LBS_DEB_SCAN);
 
@@ -504,58 +499,8 @@ lbs_scan_setup_scan_config(struct lbs_pr
 	 */
 	*ppchantlvout = (struct mrvlietypes_chanlistparamset *) ptlvpos;
 
-	if (!puserscanin || !puserscanin->chanlist[0].channumber) {
-		/* Create a default channel scan list */
-		lbs_deb_scan("creating full region channel list\n");
-		lbs_scan_create_channel_list(priv, pscanchanlist,
-					      *pfilteredscan);
-		goto out;
-	}
-
-	for (chanidx = 0;
-	     chanidx < LBS_IOCTL_USER_SCAN_CHAN_MAX
-	     && puserscanin->chanlist[chanidx].channumber; chanidx++) {
-
-		channel = puserscanin->chanlist[chanidx].channumber;
-		(pscanchanlist + chanidx)->channumber = channel;
-
-		radiotype = puserscanin->chanlist[chanidx].radiotype;
-		(pscanchanlist + chanidx)->radiotype = radiotype;
-
-		scantype = puserscanin->chanlist[chanidx].scantype;
-
-		if (scantype == CMD_SCAN_TYPE_PASSIVE) {
-			(pscanchanlist +
-			 chanidx)->chanscanmode.passivescan = 1;
-		} else {
-			(pscanchanlist +
-			 chanidx)->chanscanmode.passivescan = 0;
-		}
-
-		if (puserscanin->chanlist[chanidx].scantime) {
-			scandur = puserscanin->chanlist[chanidx].scantime;
-		} else {
-			if (scantype == CMD_SCAN_TYPE_PASSIVE) {
-				scandur = MRVDRV_PASSIVE_SCAN_CHAN_TIME;
-			} else {
-				scandur = MRVDRV_ACTIVE_SCAN_CHAN_TIME;
-			}
-		}
-
-		(pscanchanlist + chanidx)->minscantime =
-		    cpu_to_le16(scandur);
-		(pscanchanlist + chanidx)->maxscantime =
-		    cpu_to_le16(scandur);
-	}
-
-	/* Check if we are only scanning the current channel */
-	if ((chanidx == 1) &&
-	    (puserscanin->chanlist[0].channumber ==
-			       priv->adapter->curbssparams.channel)) {
-		*pscancurrentonly = 1;
-		lbs_deb_scan("scanning current channel only");
-	}
-
+	lbs_scan_create_channel_list(priv, pscanchanlist,
+				      *pfilteredscan);
 out:
 	return pscancfgout;
 }
Index: wireless-2.6/drivers/net/wireless/libertas/scan.h
===================================================================
--- wireless-2.6.orig/drivers/net/wireless/libertas/scan.h	2007-11-28 18:20:13.000000000 +0100
+++ wireless-2.6/drivers/net/wireless/libertas/scan.h	2007-11-28 18:20:50.000000000 +0100
@@ -121,11 +121,6 @@ struct lbs_ioctl_user_scan_cfg {
 
 	/* Clear existing scan results matching this SSID */
 	u8 clear_ssid;
-
-    /**
-     *  @brief Variable number (fixed maximum) of channels to scan up
-     */
-	struct lbs_ioctl_user_scan_chan chanlist[LBS_IOCTL_USER_SCAN_CHAN_MAX];
 };
 
 /**

Re: [PATCH] libertas: remove user-specified channel list

[Dan Williams]

On Wed, 2007-11-28 at 17:30 +0100, Holger Schurig wrote:
> Remove the ability to specify channels to scan via debugfs
> 
> Signed-off-by: Holger Schurig <hs4233@mail.mn-solutions.de>

Acked-by: Dan Williams <dcbw@redhat.com>

> Index: wireless-2.6/drivers/net/wireless/libertas/README
> ===================================================================
> --- wireless-2.6.orig/drivers/net/wireless/libertas/README	2007-11-28 18:18:45.000000000 +0100
> +++ wireless-2.6/drivers/net/wireless/libertas/README	2007-11-28 18:20:09.000000000 +0100
> @@ -195,8 +195,6 @@ setuserscan
>  
>           where [ARGS]:
>  
> -      chan=[chan#][band][mode] where band is [a,b,g] and mode is
> -                               blank for active or 'p' for passive
>        bssid=xx:xx:xx:xx:xx:xx  specify a BSSID filter for the scan
>        ssid="[SSID]"            specify a SSID filter for the scan
>        keep=[0 or 1]            keep the previous scan results (1), discard (0)
> @@ -204,35 +202,26 @@ setuserscan
>        type=[1,2,3]             BSS type: 1 (Infra), 2(Adhoc), 3(Any)
>  
>      Any combination of the above arguments can be supplied on the command
> -      line.  If the chan token is absent, a full channel scan will be
> -      completed by the driver.  If dur tokens are absent, the driver default
> -      setting will be used.  The bssid and ssid fields, if blank, will
> -      produce an unfiltered scan. The type field will default to 3 (Any) and
> -      the keep field will default to 0 (Discard).
> +    line. If dur tokens are absent, the driver default setting will be used.
> +    The bssid and ssid fields, if blank, will produce an unfiltered scan.
> +    The type field will default to 3 (Any) and the keep field will default
> +    to 0 (Discard).
>  
>      Examples:
> -    1) Perform an active scan on channels 1, 6, and 11 in the 'g' band:
> -            echo "chan=1g,6g,11g" > setuserscan
> -
> -    2) Perform a passive scan on channel 11 for 20 ms:
> -            echo "chan=11gp dur=20" > setuserscan
> -
> -    3) Perform an active scan on channels 1, 6, and 11; and a passive scan on
> -       channel 36 in the 'a' band:
> -
> -            echo "chan=1g,6g,11g,36ap" > setuserscan
> +    1) Perform a passive scan on all channels for 20 ms per channel:
> +            echo "dur=20" > setuserscan
>  
> -    4) Perform an active scan on channel 6 and 36 for a specific SSID:
> -            echo "chan=6g,36a ssid="TestAP"" > setuserscan
> +    2) Perform an active scan for a specific SSID:
> +            echo "ssid="TestAP"" > setuserscan
>  
> -    5) Scan all available channels (B/G, A bands) for a specific BSSID, keep
> +    3) Scan all available channels (B/G, A bands) for a specific BSSID, keep
>         the current scan table intact, update existing or append new scan data:
>              echo "bssid=00:50:43:20:12:82 keep=1" > setuserscan
>  
> -    6) Scan channel 6, for all infrastructure networks.
> +    4) Scan for all infrastructure networks.
>         Keep the previous scan table intact. Update any duplicate BSSID/SSID
>         matches with the new scan data:
> -            echo "chan=6g type=1 keep=1" > setuserscan
> +            echo "type=1 keep=1" > setuserscan
>  
>      All entries in the scan table (not just the new scan data when keep=1)
>      will be displayed upon completion by use of the getscantable ioctl.
> Index: wireless-2.6/drivers/net/wireless/libertas/debugfs.c
> ===================================================================
> --- wireless-2.6.orig/drivers/net/wireless/libertas/debugfs.c	2007-11-28 18:22:36.000000000 +0100
> +++ wireless-2.6/drivers/net/wireless/libertas/debugfs.c	2007-11-28 18:23:27.000000000 +0100
> @@ -196,43 +196,6 @@ out_unlock:
>  	return count;
>  }
>  
> -static int lbs_parse_chan(char *buf, size_t count,
> -			struct lbs_ioctl_user_scan_cfg *scan_cfg, int dur)
> -{
> -	char *start, *end, *hold, *str;
> -	int i = 0;
> -
> -	start = strstr(buf, "chan=");
> -	if (!start)
> -		return -EINVAL;
> -	start += 5;
> -	end = strchr(start, ' ');
> -	if (!end)
> -		end = buf + count;
> -	hold = kzalloc((end - start)+1, GFP_KERNEL);
> -	if (!hold)
> -		return -ENOMEM;
> -	strncpy(hold, start, end - start);
> -	hold[(end-start)+1] = '\0';
> -	while(hold && (str = strsep(&hold, ","))) {
> -		int chan;
> -		char band, passive = 0;
> -		sscanf(str, "%d%c%c", &chan, &band, &passive);
> -		scan_cfg->chanlist[i].channumber = chan;
> -		scan_cfg->chanlist[i].scantype = passive ? 1 : 0;
> -		if (band == 'b' || band == 'g')
> -			scan_cfg->chanlist[i].radiotype = 0;
> -		else if (band == 'a')
> -			scan_cfg->chanlist[i].radiotype = 1;
> -
> -		scan_cfg->chanlist[i].scantime = dur;
> -		i++;
> -	}
> -
> -	kfree(hold);
> -	return i;
> -}
> -
>  static void lbs_parse_bssid(char *buf, size_t count,
>  	struct lbs_ioctl_user_scan_cfg *scan_cfg)
>  {
> @@ -345,7 +308,6 @@ static ssize_t lbs_setuserscan(struct fi
>  	scan_cfg->bsstype = LBS_SCAN_BSS_TYPE_ANY;
>  
>  	dur = lbs_parse_dur(buf, count, scan_cfg);
> -	lbs_parse_chan(buf, count, scan_cfg, dur);
>  	lbs_parse_bssid(buf, count, scan_cfg);
>  	scan_cfg->clear_bssid = lbs_parse_clear(buf, count, "clear_bssid=");
>  	lbs_parse_ssid(buf, count, scan_cfg);
> Index: wireless-2.6/drivers/net/wireless/libertas/scan.c
> ===================================================================
> --- wireless-2.6.orig/drivers/net/wireless/libertas/scan.c	2007-11-28 18:21:10.000000000 +0100
> +++ wireless-2.6/drivers/net/wireless/libertas/scan.c	2007-11-28 18:23:01.000000000 +0100
> @@ -415,11 +415,6 @@ lbs_scan_setup_scan_config(struct lbs_pr
>  	struct mrvlietypes_ssidparamset *pssidtlv;
>  	struct lbs_scan_cmd_config *pscancfgout = NULL;
>  	u8 *ptlvpos;
> -	int chanidx;
> -	int scantype;
> -	int scandur;
> -	int channel;
> -	int radiotype;
>  
>  	lbs_deb_enter(LBS_DEB_SCAN);
>  
> @@ -504,58 +499,8 @@ lbs_scan_setup_scan_config(struct lbs_pr
>  	 */
>  	*ppchantlvout = (struct mrvlietypes_chanlistparamset *) ptlvpos;
>  
> -	if (!puserscanin || !puserscanin->chanlist[0].channumber) {
> -		/* Create a default channel scan list */
> -		lbs_deb_scan("creating full region channel list\n");
> -		lbs_scan_create_channel_list(priv, pscanchanlist,
> -					      *pfilteredscan);
> -		goto out;
> -	}
> -
> -	for (chanidx = 0;
> -	     chanidx < LBS_IOCTL_USER_SCAN_CHAN_MAX
> -	     && puserscanin->chanlist[chanidx].channumber; chanidx++) {
> -
> -		channel = puserscanin->chanlist[chanidx].channumber;
> -		(pscanchanlist + chanidx)->channumber = channel;
> -
> -		radiotype = puserscanin->chanlist[chanidx].radiotype;
> -		(pscanchanlist + chanidx)->radiotype = radiotype;
> -
> -		scantype = puserscanin->chanlist[chanidx].scantype;
> -
> -		if (scantype == CMD_SCAN_TYPE_PASSIVE) {
> -			(pscanchanlist +
> -			 chanidx)->chanscanmode.passivescan = 1;
> -		} else {
> -			(pscanchanlist +
> -			 chanidx)->chanscanmode.passivescan = 0;
> -		}
> -
> -		if (puserscanin->chanlist[chanidx].scantime) {
> -			scandur = puserscanin->chanlist[chanidx].scantime;
> -		} else {
> -			if (scantype == CMD_SCAN_TYPE_PASSIVE) {
> -				scandur = MRVDRV_PASSIVE_SCAN_CHAN_TIME;
> -			} else {
> -				scandur = MRVDRV_ACTIVE_SCAN_CHAN_TIME;
> -			}
> -		}
> -
> -		(pscanchanlist + chanidx)->minscantime =
> -		    cpu_to_le16(scandur);
> -		(pscanchanlist + chanidx)->maxscantime =
> -		    cpu_to_le16(scandur);
> -	}
> -
> -	/* Check if we are only scanning the current channel */
> -	if ((chanidx == 1) &&
> -	    (puserscanin->chanlist[0].channumber ==
> -			       priv->adapter->curbssparams.channel)) {
> -		*pscancurrentonly = 1;
> -		lbs_deb_scan("scanning current channel only");
> -	}
> -
> +	lbs_scan_create_channel_list(priv, pscanchanlist,
> +				      *pfilteredscan);
>  out:
>  	return pscancfgout;
>  }
> Index: wireless-2.6/drivers/net/wireless/libertas/scan.h
> ===================================================================
> --- wireless-2.6.orig/drivers/net/wireless/libertas/scan.h	2007-11-28 18:20:13.000000000 +0100
> +++ wireless-2.6/drivers/net/wireless/libertas/scan.h	2007-11-28 18:20:50.000000000 +0100
> @@ -121,11 +121,6 @@ struct lbs_ioctl_user_scan_cfg {
>  
>  	/* Clear existing scan results matching this SSID */
>  	u8 clear_ssid;
> -
> -    /**
> -     *  @brief Variable number (fixed maximum) of channels to scan up
> -     */
> -	struct lbs_ioctl_user_scan_chan chanlist[LBS_IOCTL_USER_SCAN_CHAN_MAX];
>  };
>  
>  /**

Re: [PATCH] libertas: remove user-specified channel list

[Albert Cahalan]

On Nov 28, 2007 11:30 AM, Holger Schurig <hs4233@mail.mn-solutions.de> wrote:
> Remove the ability to specify channels to scan via debugfs

Why are you ripping out functionality? At the very least,
that ought to be justified in the changelog comment.

Re: [PATCH] libertas: remove user-specified channel list

[Holger Schurig]

On Thursday 29 November 2007 05:12:06 Albert Cahalan wrote:
> On Nov 28, 2007 11:30 AM, Holger Schurig 
<hs4233@mail.mn-solutions.de> wrote:
> > Remove the ability to specify channels to scan via debugfs
>
> Why are you ripping out functionality? At the very least,
> that ought to be justified in the changelog comment.

This was discussed in libertas-dev.


Basically, I posted there a RFC (request-for-comment) patch about 
a newer implementation of how scanning could be implemented. The 
current version of scan.c is a bit complicated and not easy to 
follow. I was rewriting that up to a point where it worked. But 
my RFC patch had a bunch of TODO items, and I asked in 
libertas-dev how to deal with them.

Two TODOs were about the implementation the syntax 
of "probes=XXX" and "channel=XXX" that you can use via 
debugfs/extgetscan.

Dan thought that we could easily rip this out, because no one is 
known to use this feature. And I'm seconding Dan here. Both 
debugfs-features actually just exhibits a possibility of how to 
interact with the firmware. They don't have a real value.

Simply because you cannot set number of probes or channel-list 
from "normal" user-space application. E.g. when libertas needs 
to scan because of a new ESSID set via "iwlist eth1 essid BLAH" 
it would anyway scan all channels set by the country (11, 13 or 
14 for b/g). It wouldn't honor your manually crafted last scan 
from your last access to debugfs.

So extgetscan is merely a tool to play with, but nothing more.

madwifi for example also has the ability to specify "please scan 
only on channels 1, 7, 11", but this has been implemented via 
private ioctl's and set's this "sticky". So when you scan from 
iwlist, implicitly from iwconfig XXX essid, from wpa_supplicant 
etc, it would be honored. For such a feature you have a 
real-life usage scenario, it would be much more useful than the 
current way.


However, if people come with a "this makes sense" usage scenario, 
I'd add the needed implementation of it also to the newer 
scanning code.

Re: [PATCH] libertas: remove user-specified channel list

[Dan Williams]

On Thu, 2007-11-29 at 08:44 +0100, Holger Schurig wrote:
> On Thursday 29 November 2007 05:12:06 Albert Cahalan wrote:
> > On Nov 28, 2007 11:30 AM, Holger Schurig 
> <hs4233@mail.mn-solutions.de> wrote:
> > > Remove the ability to specify channels to scan via debugfs
> >
> > Why are you ripping out functionality? At the very least,
> > that ought to be justified in the changelog comment.
> 
> This was discussed in libertas-dev.
> 
> 
> Basically, I posted there a RFC (request-for-comment) patch about 
> a newer implementation of how scanning could be implemented. The 
> current version of scan.c is a bit complicated and not easy to 
> follow. I was rewriting that up to a point where it worked. But 
> my RFC patch had a bunch of TODO items, and I asked in 
> libertas-dev how to deal with them.
> 
> Two TODOs were about the implementation the syntax 
> of "probes=XXX" and "channel=XXX" that you can use via 
> debugfs/extgetscan.
> 
> Dan thought that we could easily rip this out, because no one is 
> known to use this feature. And I'm seconding Dan here. Both 
> debugfs-features actually just exhibits a possibility of how to 
> interact with the firmware. They don't have a real value.
> 
> Simply because you cannot set number of probes or channel-list 
> from "normal" user-space application. E.g. when libertas needs 
> to scan because of a new ESSID set via "iwlist eth1 essid BLAH" 
> it would anyway scan all channels set by the country (11, 13 or 
> 14 for b/g). It wouldn't honor your manually crafted last scan 
> from your last access to debugfs.
> 
> So extgetscan is merely a tool to play with, but nothing more.
> 
> madwifi for example also has the ability to specify "please scan 
> only on channels 1, 7, 11", but this has been implemented via 
> private ioctl's and set's this "sticky". So when you scan from 
> iwlist, implicitly from iwconfig XXX essid, from wpa_supplicant 
> etc, it would be honored. For such a feature you have a 
> real-life usage scenario, it would be much more useful than the 
> current way.

Note that WEXT supports specifying the channels you'd like to scan on in
the extscan struct of the SIOCSIWSCAN call.  Libertas should support
this.  However, I don't think iwlist supports it, but that shouldn't
stop the drivers from supporting that option.

Dan