[PATCH v2 2/6] mac80211: allow creating wiphy devices with suggested name.

[greearb-my8/4N5VtI7c+]

From: Ben Greear <greearb-my8/4N5VtI7c+***@public.gmane.org>

Support creating wiphy devices with an optional name.
This will be used by hwsim to have better automated control
over virtual radio creation/deletion.

Signed-off-by: Ben Greear <greearb-my8/4N5VtI7c+***@public.gmane.org>
---
include/net/mac80211.h | 25 ++++++++++++++++++++++++-
net/mac80211/main.c | 9 +++++----
2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 9bb2fc7..a206a56 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -3069,11 +3069,34 @@ struct ieee80211_ops {
*
* @priv_data_len: length of private data
* @ops: callbacks for this device
+ * @requested_name: Requested name for this device.
*
* Return: A pointer to the new hardware device, or %NULL on error.
*/
+struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
+ const struct ieee80211_ops *ops,
+ const char *requested_name);
+
+/**
+ * ieee80211_alloc_hw - Allocate a new hardware device
+ *
+ * This must be called once for each hardware device. The returned pointer
+ * must be used to refer to this device when calling other functions.
+ * mac80211 allocates a private data area for the driver pointed to by
+ * @priv in &struct ieee80211_hw, the size of this area is given as
+ * @priv_data_len.
+ *
+ * @priv_data_len: length of private data
+ * @ops: callbacks for this device
+ *
+ * Return: A pointer to the new hardware device, or %NULL on error.
+ */
+static inline
struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
- const struct ieee80211_ops *ops);
+ const struct ieee80211_ops *ops)
+{
+ return ieee80211_alloc_hw_nm(priv_data_len, ops, NULL);
+}

/**
* ieee80211_register_hw - Register hardware device
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 9e322dc..b189122 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -478,8 +478,9 @@ static const struct ieee80211_vht_cap mac80211_vht_capa_mod_mask = {
},
};

-struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
- const struct ieee80211_ops *ops)
+struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
+ const struct ieee80211_ops *ops,
+ const char *requested_name)
{
struct ieee80211_local *local;
int priv_size, i;
@@ -519,7 +520,7 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
*/
priv_size = ALIGN(sizeof(*local), NETDEV_ALIGN) + priv_data_len;

- wiphy = wiphy_new(&mac80211_config_ops, priv_size);
+ wiphy = wiphy_new_nm(&mac80211_config_ops, priv_size, requested_name);

if (!wiphy)
return NULL;
@@ -649,7 +650,7 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,

return &local->hw;
}
-EXPORT_SYMBOL(ieee80211_alloc_hw);
+EXPORT_SYMBOL(ieee80211_alloc_hw_nm);

static int ieee80211_init_cipher_suites(struct ieee80211_local *local)
{

[greearb-my8/4N5VtI7c+]

From: Ben Greear <greearb-my8/4N5VtI7c+***@public.gmane.org>

Good for automated testing, where user can create wlan
interfaces with specified names.

Signed-off-by: Ben Greear <greearb-my8/4N5VtI7c+***@public.gmane.org>
---
drivers/net/wireless/mac80211_hwsim.c | 13 ++++++++++---
drivers/net/wireless/mac80211_hwsim.h | 2 ++
2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index ef793d5..fc5695d 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -2023,7 +2023,7 @@ static int mac80211_hwsim_create_radio(int channels, const char *reg_alpha2,
const struct ieee80211_regdomain *regd,
bool reg_strict, bool p2p_device,
bool use_chanctx, bool destroy_on_close,
- u32 portid, char *hwname)
+ u32 portid, char *hwname, bool no_vdev)
{
int err;
u8 addr[ETH_ALEN];
@@ -2230,6 +2230,9 @@ static int mac80211_hwsim_create_radio(int channels, const char *reg_alpha2,
schedule_timeout_interruptible(1);
}

+ if (no_vdev)
+ hw->flags |= IEEE80211_HW_NO_AUTO_VDEV;
+
err = ieee80211_register_hw(hw);
if (err < 0) {
printk(KERN_DEBUG "mac80211_hwsim: ieee80211_register_hw failed (%d)\n",
@@ -2516,11 +2519,15 @@ static int hwsim_create_radio_nl(struct sk_buff *msg, struct genl_info *info)
bool p2p_device = info->attrs[HWSIM_ATTR_SUPPORT_P2P_DEVICE];
bool destroy_on_close = info->attrs[HWSIM_ATTR_DESTROY_RADIO_ON_CLOSE];
bool use_chanctx;
+ bool no_vdev = false;
char *hwname = NULL;

if (info->attrs[HWSIM_ATTR_CHANNELS])
chans = nla_get_u32(info->attrs[HWSIM_ATTR_CHANNELS]);

+ if (info->attrs[HWSIM_ATTR_NO_VDEV])
+ no_vdev = true;
+
if (info->attrs[HWSIM_ATTR_RADIO_NAME])
hwname = nla_data(info->attrs[HWSIM_ATTR_RADIO_NAME]);

@@ -2543,7 +2550,7 @@ static int hwsim_create_radio_nl(struct sk_buff *msg, struct genl_info *info)
return mac80211_hwsim_create_radio(chans, alpha2, regd, reg_strict,
p2p_device, use_chanctx,
destroy_on_close, info->snd_portid,
- hwname);
+ hwname, no_vdev);
}

static int hwsim_destroy_radio_nl(struct sk_buff *msg, struct genl_info *info)
@@ -2800,7 +2807,7 @@ static int __init init_mac80211_hwsim(void)
err = mac80211_hwsim_create_radio(channels, reg_alpha2,
regd, reg_strict,
support_p2p_device,
- channels > 1, false, 0, NULL);
+ channels > 1, false, 0, NULL, false);
if (err < 0)
goto out_free_radios;
}
diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
index 98c6998..71bf498 100644
--- a/drivers/net/wireless/mac80211_hwsim.h
+++ b/drivers/net/wireless/mac80211_hwsim.h
@@ -114,6 +114,7 @@ enum {
* @HWSIM_ATTR_DESTROY_RADIO_ON_CLOSE: used with the %HWSIM_CMD_CREATE_RADIO
* command to force radio removal when process that created the radio dies
* @HWSIM_ATTR_RADIO_NAME: Name of radio, e.g. phy666
+ * @HWSIM_ATTR_NO_VDEV: Do not create vdev (wlanX) when creating radio.
* @__HWSIM_ATTR_MAX: enum limit
*/

@@ -137,6 +138,7 @@ enum {
HWSIM_ATTR_USE_CHANCTX,
HWSIM_ATTR_DESTROY_RADIO_ON_CLOSE,
HWSIM_ATTR_RADIO_NAME,
+ HWSIM_ATTR_NO_VDEV,
__HWSIM_ATTR_MAX,
};
#define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)

[Johannes Berg]

Post by greearb-my8/4N5VtI7c+
+ u32 portid, char *hwname, bool no_vdev)

same comment regarding "vdev" throughout this patch.

Also, there shouldn't be a . at the end of the subject :-)

johannes


--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[greearb-my8/4N5VtI7c+]

From: Ben Greear <greearb-my8/4N5VtI7c+***@public.gmane.org>

Otherwise, it can be very difficult to know which is which
if you are trying to do detailed testing.

Signed-off-by: Ben Greear <greearb-my8/4N5VtI7c+***@public.gmane.org>
---
drivers/net/wireless/mac80211_hwsim.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 00c7b3c..ef793d5 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -2023,7 +2023,7 @@ static int mac80211_hwsim_create_radio(int channels, const char *reg_alpha2,
const struct ieee80211_regdomain *regd,
bool reg_strict, bool p2p_device,
bool use_chanctx, bool destroy_on_close,
- u32 portid)
+ u32 portid, char *hwname)
{
int err;
u8 addr[ETH_ALEN];
@@ -2042,7 +2042,7 @@ static int mac80211_hwsim_create_radio(int channels, const char *reg_alpha2,

if (use_chanctx)
ops = &mac80211_hwsim_mchan_ops;
- hw = ieee80211_alloc_hw(sizeof(*data), ops);
+ hw = ieee80211_alloc_hw_nm(sizeof(*data), ops, hwname);
if (!hw) {
printk(KERN_DEBUG "mac80211_hwsim: ieee80211_alloc_hw failed\n");
err = -ENOMEM;
@@ -2516,10 +2516,14 @@ static int hwsim_create_radio_nl(struct sk_buff *msg, struct genl_info *info)
bool p2p_device = info->attrs[HWSIM_ATTR_SUPPORT_P2P_DEVICE];
bool destroy_on_close = info->attrs[HWSIM_ATTR_DESTROY_RADIO_ON_CLOSE];
bool use_chanctx;
+ char *hwname = NULL;

if (info->attrs[HWSIM_ATTR_CHANNELS])
chans = nla_get_u32(info->attrs[HWSIM_ATTR_CHANNELS]);

+ if (info->attrs[HWSIM_ATTR_RADIO_NAME])
+ hwname = nla_data(info->attrs[HWSIM_ATTR_RADIO_NAME]);
+
if (info->attrs[HWSIM_ATTR_USE_CHANCTX])
use_chanctx = true;
else
@@ -2538,7 +2542,8 @@ static int hwsim_create_radio_nl(struct sk_buff *msg, struct genl_info *info)

return mac80211_hwsim_create_radio(chans, alpha2, regd, reg_strict,
p2p_device, use_chanctx,
- destroy_on_close, info->snd_portid);
+ destroy_on_close, info->snd_portid,
+ hwname);
}

static int hwsim_destroy_radio_nl(struct sk_buff *msg, struct genl_info *info)
@@ -2795,7 +2800,7 @@ static int __init init_mac80211_hwsim(void)
err = mac80211_hwsim_create_radio(channels, reg_alpha2,
regd, reg_strict,
support_p2p_device,
- channels > 1, false, 0);
+ channels > 1, false, 0, NULL);
if (err < 0)
goto out_free_radios;
}

[Johannes Berg]

Post by greearb-my8/4N5VtI7c+
@@ -2023,7 +2023,7 @@ static int mac80211_hwsim_create_radio(int channels, const char *reg_alpha2,
const struct ieee80211_regdomain *regd,
bool reg_strict, bool p2p_device,
bool use_chanctx, bool destroy_on_close,
- u32 portid)
+ u32 portid, char *hwname)

could also be const?

Post by greearb-my8/4N5VtI7c+
+ char *hwname = NULL;

const as well then, I guess.

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[greearb-my8/4N5VtI7c+]

From: Ben Greear <greearb-my8/4N5VtI7c+***@public.gmane.org>

This will be helpful when using the mac80211_hwsim
wiphys and automated testing. Let user create the
wlan devs as needed, and named as expected.

Signed-off-by: Ben Greear <greearb-my8/4N5VtI7c+***@public.gmane.org>
---
include/net/mac80211.h | 7 ++++++-
net/mac80211/main.c | 3 ++-
2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index a206a56..a5236d2 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1579,6 +1579,10 @@ struct ieee80211_tx_control {
* a virtual monitor interface when monitor interfaces are the only
* active interfaces.
*
+ * @IEEE80211_HW_NO_AUTO_VDEV: The driver would like for no wlanX to
+ * be created. It is expected user-space will create vdevs as
+ * desired (and thus have them named as desired).
+ *
* @IEEE80211_HW_QUEUE_CONTROL: The driver wants to control per-interface
* queue mapping in order to use different queues (not just one per AC)
* for different virtual interfaces. See the doc section on HW queue
@@ -1625,7 +1629,8 @@ enum ieee80211_hw_flags {
IEEE80211_HW_SUPPORTS_DYNAMIC_PS = 1<<12,
IEEE80211_HW_MFP_CAPABLE = 1<<13,
IEEE80211_HW_WANT_MONITOR_VIF = 1<<14,
- /* free slots */
+ IEEE80211_HW_NO_AUTO_VDEV = 1<<15,
+ /* free slot */
IEEE80211_HW_SUPPORTS_UAPSD = 1<<17,
IEEE80211_HW_REPORTS_TX_ACK_STATUS = 1<<18,
IEEE80211_HW_CONNECTION_MONITOR = 1<<19,
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index b189122..830bfec 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -1023,7 +1023,8 @@ int ieee80211_register_hw(struct ieee80211_hw *hw)
}

/* add one default STA interface if supported */
- if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_STATION)) {
+ if (local->hw.wiphy->interface_modes & BIT(NL80211_IFTYPE_STATION) &&
+ !(hw->flags & IEEE80211_HW_NO_AUTO_VDEV)) {
result = ieee80211_if_add(local, "wlan%d", NULL,
NL80211_IFTYPE_STATION, NULL);
if (result)

[Johannes Berg]

This isn't a cfg80211 patch (subject)

Post by greearb-my8/4N5VtI7c+
+ * be created. It is expected user-space will create vdevs as
+ * desired (and thus have them named as desired).

"VDEV" isn't a term mac80211 uses anywhere, please use "VIF" or similar
- no need to add more terminology.
johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[greearb-my8/4N5VtI7c+]

From: Ben Greear <greearb-my8/4N5VtI7c+***@public.gmane.org>

This is useful when creating virtual-stations, and probably
APs and other devices as well. Keeps udev from mucking with
things it shouldn't, since the default MAC is never seen
by udev when specified on the cmd-line during creation.

Signed-off-by: Ben Greear <greearb-my8/4N5VtI7c+***@public.gmane.org>
---
include/net/cfg80211.h | 6 +++---
include/uapi/linux/nl80211.h | 3 +++
net/mac80211/iface.c | 5 ++++-
net/mac80211/main.c | 1 +
net/wireless/nl80211.c | 2 +-
5 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 5252f80..ba8a196 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -319,9 +319,9 @@ struct ieee80211_supported_band {
/**
* struct vif_params - describes virtual interface parameters
* @use_4addr: use 4-address frames
- * @macaddr: address to use for this virtual interface. This will only
- * be used for non-netdevice interfaces. If this parameter is set
- * to zero address the driver may determine the address as needed.
+ * @macaddr: address to use for this virtual interface.
+ * If this parameter is set to zero address the driver may
+ * determine the address as needed.
*/
struct vif_params {
int use_4addr;
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index b553c48..c7e35ae 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -4052,6 +4052,8 @@ enum nl80211_ap_sme_features {
* multiplexing powersave, ie. can turn off all but one chain
* and then wake the rest up as required after, for example,
* rts/cts handshake.
+ * @NL80211_FEATURE_MAC_ON_CREATE: Device supports configuring
+ * the vdev's MAC address upon creation.
*/
enum nl80211_feature_flags {
NL80211_FEATURE_SK_TX_STATUS = 1 << 0,
@@ -4080,6 +4082,7 @@ enum nl80211_feature_flags {
NL80211_FEATURE_ACKTO_ESTIMATION = 1 << 23,
NL80211_FEATURE_STATIC_SMPS = 1 << 24,
NL80211_FEATURE_DYNAMIC_SMPS = 1 << 25,
+ NL80211_FEATURE_MAC_ON_CREATE = 1 << 26,
};

/**
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index e469b33..1ffcc07 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1671,7 +1671,10 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
}

ieee80211_assign_perm_addr(local, ndev->perm_addr, type);
- memcpy(ndev->dev_addr, ndev->perm_addr, ETH_ALEN);
+ if (params && is_valid_ether_addr(params->macaddr))
+ memcpy(ndev->dev_addr, params->macaddr, ETH_ALEN);
+ else
+ memcpy(ndev->dev_addr, ndev->perm_addr, ETH_ALEN);
SET_NETDEV_DEV(ndev, wiphy_dev(local->hw.wiphy));

/* don't use IEEE80211_DEV_TO_SUB_IF -- it checks too much */
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 830bfec..43d57db 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -542,6 +542,7 @@ struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
NL80211_FEATURE_SAE |
NL80211_FEATURE_HT_IBSS |
NL80211_FEATURE_VIF_TXPOWER |
+ NL80211_FEATURE_MAC_ON_CREATE |
NL80211_FEATURE_USERSPACE_MPM;

if (!ops->hw_scan)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 0c0f204..9cbc277 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -2605,7 +2605,7 @@ static int nl80211_new_interface(struct sk_buff *skb, struct genl_info *info)
!(rdev->wiphy.interface_modes & (1 << type)))
return -EOPNOTSUPP;

- if (type == NL80211_IFTYPE_P2P_DEVICE && info->attrs[NL80211_ATTR_MAC]) {
+ if (info->attrs[NL80211_ATTR_MAC]) {
nla_memcpy(params.macaddr, info->attrs[NL80211_ATTR_MAC],
ETH_ALEN);
if (!is_valid_ether_addr(params.macaddr))

[Johannes Berg]

Same comment regarding "vdev", here starting from the subject.

Also, this patch isn't purely cfg80211 - should probably split it into
cfg80211 and mac80211 parts?

Post by greearb-my8/4N5VtI7c+
This is useful when creating virtual-stations, and probably
APs and other devices as well.

It clearly is - imho worth rephrasing as just "when creating virtual
interfaces" :-)

Post by greearb-my8/4N5VtI7c+
+++ b/include/net/cfg80211.h
@@ -319,9 +319,9 @@ struct ieee80211_supported_band {
/**
* struct vif_params - describes virtual interface parameters
- * be used for non-netdevice interfaces. If this parameter is set
- * to zero address the driver may determine the address as needed.
+ * If this parameter is set to zero address the driver may
+ * determine the address as needed.

Maybe point to the feature flag here.

Post by greearb-my8/4N5VtI7c+
+ * the vdev's MAC address upon creation.

Maybe point to the attribute that's used for that here?

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Johannes Berg]

missing colon, missing documentation that it can be NULL (for no
request)

also missing in the documentation is the name change of the function

if (unlikely(rdev->wiphy_idx < 0)) {
+ /* TODO: Fix this someday. */

That doesn't belong into this patch :)

/* give it a proper name */
- dev_set_name(&rdev->wiphy.dev, PHY_NAME "%d", rdev->wiphy_idx);
+ if (requested_name && requested_name[0]) {
+ struct cfg80211_registered_device *rdev2;
+ int wiphy_idx, taken = -1, result, digits;
+
+ /* Code below is from cfg80211_dev_rename */

can you refactor this then please?

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

[Ben Greear]

Post by Johannes Berg

/* give it a proper name */
- dev_set_name(&rdev->wiphy.dev, PHY_NAME "%d", rdev->wiphy_idx);
+ if (requested_name && requested_name[0]) {
+ struct cfg80211_registered_device *rdev2;
+ int wiphy_idx, taken = -1, result, digits;
+
+ /* Code below is from cfg80211_dev_rename */

can you refactor this then please?

I'll see what I can do on this, as well as take care of
the rest of the comments.

Thanks,
Ben

[Ben Greear]

Post by Ben Greear

Post by Johannes Berg

/* give it a proper name */
- dev_set_name(&rdev->wiphy.dev, PHY_NAME "%d", rdev->wiphy_idx);
+ if (requested_name && requested_name[0]) {
+ struct cfg80211_registered_device *rdev2;
+ int wiphy_idx, taken = -1, result, digits;
+
+ /* Code below is from cfg80211_dev_rename */

can you refactor this then please?

I'll see what I can do on this, as well as take care of
the rest of the comments.

I was thinking, this could be made simpler if we reduced restrictions on
the naming. Basically, let user (re)name the wiphy however they want, as
long as it name is not currently in use.

Might require a bit of extra locking and checking instead of just assuming the
atomic-inc and phy%d is unique, but maybe that is worth the tradeoff?

I'll get started on trying to refactor with existing constraints, but
let me know how you feel about relaxing the restrictions...

Thanks,
Ben

Post by Ben Greear
Thanks,
Ben

[Johannes Berg]

There seem to be two spaces in the subject - please fix that since we
need another spin anyway.
please also just have one space here
also same comments as with cfg80211 - document NULL and the function
name change

Post by greearb-my8/4N5VtI7c+
+struct ieee80211_hw *ieee80211_alloc_hw_nm(size_t priv_data_len,
+ const struct ieee80211_ops *ops,
+ const char *requested_name);
+
+/**
+ * ieee80211_alloc_hw - Allocate a new hardware device

another place where you have two spaces :)

johannes

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html