| Message ID | 1458590472-677-3-git-send-email-qca_merez@qca.qualcomm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
"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" :)
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;