[PATCH 0/2] wil6210 changes

[PATCH 0/2] wil6210 changes

[Maya Erez]

The below patches include a bug fix in debugfs and the ability to determine the interface name.

Lior David (2):
  wil6210: allow empty WMI commands in debugfs wmi_send
  wil6210: add module parameter for alternate interface name

 drivers/net/wireless/ath/wil6210/debugfs.c | 4 ++--
 drivers/net/wireless/ath/wil6210/netdev.c  | 8 +++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

-- 
1.8.5.2

[PATCH 1/2] wil6210: allow empty WMI commands in debugfs wmi_send

[Maya Erez]

From: Lior David <qca_liord@qca.qualcomm.com>

There are many valid WMI commands with only header without any
additional payload. Such WMI commands could not be sent using
the debugfs wmi_send facility. Fix the code to allow sending
of such commands.

Signed-off-by: Lior David <qca_liord@qca.qualcomm.com>
Signed-off-by: Maya Erez <qca_merez@qca.qualcomm.com>
---
 drivers/net/wireless/ath/wil6210/debugfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/wil6210/debugfs.c b/drivers/net/wireless/ath/wil6210/debugfs.c
index a4d3f70..b338a09 100644
--- a/drivers/net/wireless/ath/wil6210/debugfs.c
+++ b/drivers/net/wireless/ath/wil6210/debugfs.c
@@ -832,7 +832,7 @@ static ssize_t wil_write_file_wmi(struct file *file, const char __user *buf,
 	u16 cmdid;
 	int rc, rc1;
 
-	if (cmdlen <= 0)
+	if (cmdlen < 0)
 		return -EINVAL;
 
 	wmi = kmalloc(len, GFP_KERNEL);
@@ -845,7 +845,7 @@ static ssize_t wil_write_file_wmi(struct file *file, const char __user *buf,
 		return rc;
 	}
 
-	cmd = &wmi[1];
+	cmd = (cmdlen > 0) ? &wmi[1] : NULL;
 	cmdid = le16_to_cpu(wmi->command_id);
 
 	rc1 = wmi_send(wil, cmdid, cmd, cmdlen);
-- 
1.8.5.2

Re: [PATCH 1/2] wil6210: allow empty WMI commands in debugfs wmi_send

[Kalle Valo]

Maya Erez <qca_merez@qca.qualcomm.com> writes:

> From: Lior David <qca_liord@qca.qualcomm.com>
>
> There are many valid WMI commands with only header without any
> additional payload. Such WMI commands could not be sent using
> the debugfs wmi_send facility. Fix the code to allow sending
> of such commands.
>
> Signed-off-by: Lior David <qca_liord@qca.qualcomm.com>
> Signed-off-by: Maya Erez <qca_merez@qca.qualcomm.com>

Patch 1 applied to ath.git, thanks.

-- 
Kalle Valo

[PATCH 2/2] wil6210: add module parameter for alternate interface name

[Maya Erez]

From: Lior David <qca_liord@qca.qualcomm.com>

Add a module parameter alt_ifname that when set, will name
the primary network interface wigig<N> instead of the default
wlan<N>. This helps platforms such as android where we need to
clearly separate the WIGIG interface from the default wireless
interface.

Signed-off-by: Lior David <qca_liord@qca.qualcomm.com>
Signed-off-by: Maya Erez <qca_merez@qca.qualcomm.com>
---
 drivers/net/wireless/ath/wil6210/netdev.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/wil6210/netdev.c b/drivers/net/wireless/ath/wil6210/netdev.c
index 3bc0e26..f78ea91 100644
--- a/drivers/net/wireless/ath/wil6210/netdev.c
+++ b/drivers/net/wireless/ath/wil6210/netdev.c
@@ -14,10 +14,15 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
+#include <linux/moduleparam.h>
 #include <linux/etherdevice.h>
 #include "wil6210.h"
 #include "txrx.h"
 
+static bool alt_ifname; /* = false; */
+module_param(alt_ifname, bool, S_IRUGO);
+MODULE_PARM_DESC(alt_ifname, " use an alternate interface name wigigN instead of wlanN");
+
 static int wil_open(struct net_device *ndev)
 {
 	struct wil6210_priv *wil = ndev_to_wil(ndev);
@@ -136,6 +141,7 @@ void *wil_if_alloc(struct device *dev)
 	struct wil6210_priv *wil;
 	struct ieee80211_channel *ch;
 	int rc = 0;
+	const char *ifname = alt_ifname ? "wigig%d" : "wlan%d";
 
 	wdev = wil_cfg80211_init(dev);
 	if (IS_ERR(wdev)) {
@@ -160,7 +166,7 @@ void *wil_if_alloc(struct device *dev)
 	ch = wdev->wiphy->bands[IEEE80211_BAND_60GHZ]->channels;
 	cfg80211_chandef_create(&wdev->preset_chandef, ch, NL80211_CHAN_NO_HT);
 
-	ndev = alloc_netdev(0, "wlan%d", NET_NAME_UNKNOWN, wil_dev_setup);
+	ndev = alloc_netdev(0, ifname, NET_NAME_UNKNOWN, wil_dev_setup);
 	if (!ndev) {
 		dev_err(dev, "alloc_netdev_mqs failed\n");
 		rc = -ENOMEM;
-- 
1.8.5.2

Re: [PATCH 2/2] wil6210: add module parameter for alternate interface name

[Kalle Valo]

Maya Erez <qca_merez@qca.qualcomm.com> writes:

> From: Lior David <qca_liord@qca.qualcomm.com>
>
> Add a module parameter alt_ifname that when set, will name
> the primary network interface wigig<N> instead of the default
> wlan<N>. This helps platforms such as android where we need to
> clearly separate the WIGIG interface from the default wireless
> interface.
>
> Signed-off-by: Lior David <qca_liord@qca.qualcomm.com>
> Signed-off-by: Maya Erez <qca_merez@qca.qualcomm.com>
> ---
>  drivers/net/wireless/ath/wil6210/netdev.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/wil6210/netdev.c b/drivers/net/wireless/ath/wil6210/netdev.c
> index 3bc0e26..f78ea91 100644
> --- a/drivers/net/wireless/ath/wil6210/netdev.c
> +++ b/drivers/net/wireless/ath/wil6210/netdev.c
> @@ -14,10 +14,15 @@
>   * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>   */
>  
> +#include <linux/moduleparam.h>
>  #include <linux/etherdevice.h>
>  #include "wil6210.h"
>  #include "txrx.h"
>  
> +static bool alt_ifname; /* = false; */
> +module_param(alt_ifname, bool, S_IRUGO);
> +MODULE_PARM_DESC(alt_ifname, " use an alternate interface name wigigN instead of wlanN");
> +
>  static int wil_open(struct net_device *ndev)
>  {
>  	struct wil6210_priv *wil = ndev_to_wil(ndev);
> @@ -136,6 +141,7 @@ void *wil_if_alloc(struct device *dev)
>  	struct wil6210_priv *wil;
>  	struct ieee80211_channel *ch;
>  	int rc = 0;
> +	const char *ifname = alt_ifname ? "wigig%d" : "wlan%d";
>  
>  	wdev = wil_cfg80211_init(dev);
>  	if (IS_ERR(wdev)) {
> @@ -160,7 +166,7 @@ void *wil_if_alloc(struct device *dev)
>  	ch = wdev->wiphy->bands[IEEE80211_BAND_60GHZ]->channels;
>  	cfg80211_chandef_create(&wdev->preset_chandef, ch, NL80211_CHAN_NO_HT);
>  
> -	ndev = alloc_netdev(0, "wlan%d", NET_NAME_UNKNOWN, wil_dev_setup);
> +	ndev = alloc_netdev(0, ifname, NET_NAME_UNKNOWN, wil_dev_setup);
>  	if (!ndev) {
>  		dev_err(dev, "alloc_netdev_mqs failed\n");
>  		rc = -ENOMEM;

To me this looks like an ugly hack and I hope there is a better way to
handle the problem this patch is fixing. I think interface names
shouldn't matter from functionality point of view, anything requiring
certain naming is broken.

But if the interface name is so important why not use "wigig%d" always?
The user space can rename the interface name anyway.

-- 
Kalle Valo

Re: [PATCH 2/2] wil6210: add module parameter for alternate interface name

[Lior David]

On 3/23/2016 5:27 PM, Kalle Valo wrote:
> Maya Erez <qca_merez@qca.qualcomm.com> writes:
> 
>> From: Lior David <qca_liord@qca.qualcomm.com>
>>
>> Add a module parameter alt_ifname that when set, will name
>> the primary network interface wigig<N> instead of the default
>> wlan<N>. This helps platforms such as android where we need to
>> clearly separate the WIGIG interface from the default wireless
>> interface.
>>
>> Signed-off-by: Lior David <qca_liord@qca.qualcomm.com>
>> Signed-off-by: Maya Erez <qca_merez@qca.qualcomm.com>
>> ---
>>  drivers/net/wireless/ath/wil6210/netdev.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/wil6210/netdev.c b/drivers/net/wireless/ath/wil6210/netdev.c
>> index 3bc0e26..f78ea91 100644
>> --- a/drivers/net/wireless/ath/wil6210/netdev.c
>> +++ b/drivers/net/wireless/ath/wil6210/netdev.c
>> @@ -14,10 +14,15 @@
>>   * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>>   */
>>  
>> +#include <linux/moduleparam.h>
>>  #include <linux/etherdevice.h>
>>  #include "wil6210.h"
>>  #include "txrx.h"
>>  
>> +static bool alt_ifname; /* = false; */
>> +module_param(alt_ifname, bool, S_IRUGO);
>> +MODULE_PARM_DESC(alt_ifname, " use an alternate interface name wigigN instead of wlanN");
>> +
>>  static int wil_open(struct net_device *ndev)
>>  {
>>  	struct wil6210_priv *wil = ndev_to_wil(ndev);
>> @@ -136,6 +141,7 @@ void *wil_if_alloc(struct device *dev)
>>  	struct wil6210_priv *wil;
>>  	struct ieee80211_channel *ch;
>>  	int rc = 0;
>> +	const char *ifname = alt_ifname ? "wigig%d" : "wlan%d";
>>  
>>  	wdev = wil_cfg80211_init(dev);
>>  	if (IS_ERR(wdev)) {
>> @@ -160,7 +166,7 @@ void *wil_if_alloc(struct device *dev)
>>  	ch = wdev->wiphy->bands[IEEE80211_BAND_60GHZ]->channels;
>>  	cfg80211_chandef_create(&wdev->preset_chandef, ch, NL80211_CHAN_NO_HT);
>>  
>> -	ndev = alloc_netdev(0, "wlan%d", NET_NAME_UNKNOWN, wil_dev_setup);
>> +	ndev = alloc_netdev(0, ifname, NET_NAME_UNKNOWN, wil_dev_setup);
>>  	if (!ndev) {
>>  		dev_err(dev, "alloc_netdev_mqs failed\n");
>>  		rc = -ENOMEM;
> 
> To me this looks like an ugly hack and I hope there is a better way to
> handle the problem this patch is fixing. I think interface names
> shouldn't matter from functionality point of view, anything requiring
> certain naming is broken.
> 
> But if the interface name is so important why not use "wigig%d" always?
> The user space can rename the interface name anyway.

The problem we try to solve is with Android. Android uses hard-coded "wlan0"
interface name for wifi. We have a platform where wigig(11ad) is used
alongside wifi(11ac). Both are independent and managed by separate services,
but started at boot at roughly the same time. Sometimes 11ad will get wlan0
interface name and 11ac will get wlan1, and wifi will not work.
We considered using "wigig%d" always as you suggested, but it may break an
unknown number of existing tools/scripts that rely on wlan0 interface name.

As I see it, it is an issue of classification. Network drivers use a default
interface name prefix depending on the device type. Ethernet drivers get "eth%d",
wireless drivers get "wlan%d" and so on. There are even existing drivers
that give different prefixes based on other conditions, for example see
drivers/s390/net/ctcm_main.c, ctcm_init_netdevice.
For our 11ad device, in many platforms it is used as a wifi replacement, so
the wlan%d name is appropriate, in other platforms it is used as
a different wireless device for different purposes, so the default "wigig%d"
prefix seems appropriate.

Thanks,
Lior

Re: [PATCH 2/2] wil6210: add module parameter for alternate interface name

[Kalle Valo]

Lior David <liord@codeaurora.org> writes:

>>> +	const char *ifname = alt_ifname ? "wigig%d" : "wlan%d";
>>>  
>>>  	wdev = wil_cfg80211_init(dev);
>>>  	if (IS_ERR(wdev)) {
>>> @@ -160,7 +166,7 @@ void *wil_if_alloc(struct device *dev)
>>>  	ch = wdev->wiphy->bands[IEEE80211_BAND_60GHZ]->channels;
>>>  	cfg80211_chandef_create(&wdev->preset_chandef, ch, NL80211_CHAN_NO_HT);
>>>  
>>> -	ndev = alloc_netdev(0, "wlan%d", NET_NAME_UNKNOWN, wil_dev_setup);
>>> +	ndev = alloc_netdev(0, ifname, NET_NAME_UNKNOWN, wil_dev_setup);
>>>  	if (!ndev) {
>>>  		dev_err(dev, "alloc_netdev_mqs failed\n");
>>>  		rc = -ENOMEM;
>> 
>> To me this looks like an ugly hack and I hope there is a better way to
>> handle the problem this patch is fixing. I think interface names
>> shouldn't matter from functionality point of view, anything requiring
>> certain naming is broken.
>> 
>> But if the interface name is so important why not use "wigig%d" always?
>> The user space can rename the interface name anyway.
>
> The problem we try to solve is with Android. Android uses hard-coded "wlan0"
> interface name for wifi. We have a platform where wigig(11ad) is used
> alongside wifi(11ac). Both are independent and managed by separate services,
> but started at boot at roughly the same time. Sometimes 11ad will get wlan0
> interface name and 11ac will get wlan1, and wifi will not work.
> We considered using "wigig%d" always as you suggested, but it may break an
> unknown number of existing tools/scripts that rely on wlan0 interface
> name.

I got the idea why this is done, I just don't think that the way the
issue is solved is a good one. Can't you just rename the interface
during boot? Like older Ubuntu versions had a udev rule at
/etc/udev/rules.d/70-persistent-net.rules to name the interfaces (no
idea if it's still available after systemd was taken into use).

> As I see it, it is an issue of classification. Network drivers use a default
> interface name prefix depending on the device type. Ethernet drivers get "eth%d",
> wireless drivers get "wlan%d" and so on. There are even existing drivers
> that give different prefixes based on other conditions, for example see
> drivers/s390/net/ctcm_main.c, ctcm_init_netdevice.
> For our 11ad device, in many platforms it is used as a wifi replacement, so
> the wlan%d name is appropriate, in other platforms it is used as
> a different wireless device for different purposes, so the default "wigig%d"
> prefix seems appropriate.

Every platform works differently, even systemd even has it's own weird
naming scheme:

https://www.freedesktop.org/wiki/Software/systemd/PredictableNetworkInterfaceNames/

I really don't think that we should start working around interface
naming problems by selecting the name with module parameters. But having
"wigig%d" as the default sounds like a good idea to me.

-- 
Kalle Valo

Re: [PATCH 2/2] wil6210: add module parameter for alternate interface name

[Kalle Valo]

"Erez, Maya" <merez@codeaurora.org> writes:

> On 3/30/2016 3:13 PM, Kalle Valo wrote:
>
>     Lior David <liord@codeaurora.org> writes:
>
>
>         As I see it, it is an issue of classification. Network drivers use a
>
>     default
>
>         interface name prefix depending on the device type. Ethernet drivers get
>
>     "eth%d",
>
>         wireless drivers get "wlan%d" and so on. There are even existing drivers
>         that give different prefixes based on other conditions, for example see
>         drivers/s390/net/ctcm_main.c, ctcm_init_netdevice.
>         For our 11ad device, in many platforms it is used as a wifi replacement,
>
>     so
>
>         the wlan%d name is appropriate, in other platforms it is used as
>         a different wireless device for different purposes, so the default
>
>     "wigig%d"
>
>         prefix seems appropriate.
>
>     Every platform works differently, even systemd even has it's own weird
>     naming scheme:
>
>     https://www.freedesktop.org/wiki/Software/systemd/PredictableNetworkInterf
>     aceNames/
>
>     I really don't think that we should start working around interface
>     naming problems by selecting the name with module parameters. But having
>     "wigig%d" as the default sounds like a good idea to me.
>
>
> Thanks, Kalle. We will check your suggestion to change the interface name to
> "wigig%d".
>
> Can you drop this patch for now?

Ok, patch 2 dropped. Patch 1 is still on my queue and will be applied
"really soon now" :)

-- 
Kalle Valo