[PATCH v2 2/4] mac80211-hwsim: remove dmesg spam about get-survey.

[PATCH v2 2/4] mac80211-hwsim: remove dmesg spam about get-survey.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This message just fills up dmesg and/or kernel logs and does
not provide any useful information.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/mac80211_hwsim.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 12e9333..635ad03 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -1860,8 +1860,6 @@ static int mac80211_hwsim_get_survey(
 {
 	struct ieee80211_conf *conf = &hw->conf;
 
-	wiphy_debug(hw->wiphy, "%s (idx=%d)\n", __func__, idx);
-
 	if (idx != 0)
 		return -ENOENT;
 
-- 
2.4.11

[PATCH v2 1/4] mac80211-hwsim: notify user-space about channel change.

[greearb]

From: Ben Greear <greearb@candelatech.com>

The goal is to allow the user-space application to properly
filter packets before sending them down to the kernel.  This
should more closely mimic what a real piece of hardware would
do.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
v2:  Change IDs, add new ones for width, freq1, freq2.

 drivers/net/wireless/mac80211_hwsim.c | 67 +++++++++++++++++++++++++++++++++++
 drivers/net/wireless/mac80211_hwsim.h | 16 +++++++++
 2 files changed, 83 insertions(+)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 0150747..12e9333 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -527,6 +527,10 @@ struct mac80211_hwsim_data {
 	u8 scan_addr[ETH_ALEN];
 
 	struct ieee80211_channel *channel;
+	enum nl80211_chan_width ch_width;
+	u32 center_freq1;
+	u32 center_freq2;
+
 	u64 beacon_int	/* beacon interval in us */;
 	unsigned int rx_filter;
 	bool started, idle, scanning;
@@ -998,6 +1002,64 @@ static int hwsim_unicast_netgroup(struct mac80211_hwsim_data *data,
 	return res;
 }
 
+static void mac80211_hwsim_check_nl_notify(struct mac80211_hwsim_data *data)
+{
+	struct sk_buff *skb;
+	u32 center_freq = 0;
+	u32 _portid;
+	void *msg_head;
+
+	/* wmediumd mode check */
+	_portid = READ_ONCE(data->wmediumd);
+
+	if (!_portid)
+		return;
+
+	skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_ATOMIC);
+	if (!skb)
+		goto err_print;
+
+	msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0,
+			       HWSIM_CMD_NOTIFY_CH_CHANGE);
+	if (!msg_head) {
+		pr_debug("mac80211_hwsim: problem with msg_head, notify-ch-change\n");
+		goto nla_put_failure;
+	}
+
+	if (nla_put_u32(skb, HWSIM_ATTR_RADIO_ID, data->idx))
+		goto nla_put_failure;
+
+	if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER,
+		    ETH_ALEN, data->addresses[1].addr))
+		goto nla_put_failure;
+
+	if (data->channel)
+		center_freq = data->channel->center_freq;
+
+	if (nla_put_u32(skb, HWSIM_ATTR_FREQ, center_freq))
+		goto nla_put_failure;
+
+	if (nla_put_u32(skb, HWSIM_ATTR_CH_FREQ1, data->center_freq1))
+		goto nla_put_failure;
+
+	if (nla_put_u32(skb, HWSIM_ATTR_CH_FREQ2, data->center_freq2))
+		goto nla_put_failure;
+
+	if (nla_put_u32(skb, HWSIM_ATTR_CH_WIDTH, data->ch_width))
+		goto nla_put_failure;
+
+	genlmsg_end(skb, msg_head);
+	if (genlmsg_unicast(&init_net, skb, _portid))
+		goto err_print;
+
+	return;
+
+nla_put_failure:
+	nlmsg_free(skb);
+err_print:
+	pr_debug("mac80211_hwsim: error occurred in %s\n", __func__);
+}
+
 static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
 				       struct sk_buff *my_skb,
 				       int dst_portid)
@@ -1599,6 +1661,9 @@ static int mac80211_hwsim_config(struct ieee80211_hw *hw, u32 changed)
 	data->idle = !!(conf->flags & IEEE80211_CONF_IDLE);
 
 	data->channel = conf->chandef.chan;
+	data->ch_width = conf->chandef.width;
+	data->center_freq1 = conf->chandef.center_freq1;
+	data->center_freq2 = conf->chandef.center_freq2;
 
 	WARN_ON(data->channel && data->use_chanctx);
 
@@ -1615,6 +1680,8 @@ static int mac80211_hwsim_config(struct ieee80211_hw *hw, u32 changed)
 				      HRTIMER_MODE_REL);
 	}
 
+	mac80211_hwsim_check_nl_notify(data);
+
 	return 0;
 }
 
diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
index 39f2246..1251fdd 100644
--- a/drivers/net/wireless/mac80211_hwsim.h
+++ b/drivers/net/wireless/mac80211_hwsim.h
@@ -71,6 +71,15 @@ enum hwsim_tx_control_flags {
  * @HWSIM_CMD_DEL_RADIO: destroy a radio, reply is multicasted
  * @HWSIM_CMD_GET_RADIO: fetch information about existing radios, uses:
  *	%HWSIM_ATTR_RADIO_ID
+ * @HWSIM_CMD_NOTIFY_CH_CHANGE: notify user-space about channel-change.  This is
+ *	designed to help the user-space app better emulate radio hardware.
+ *	This command uses:
+ *	%HWSIM_ATTR_FREQ # Notify current operating center frequency.
+ *	%HWSIM_ATTR_CH_FREQ1 # Configured center-freq-1.
+ *	%HWSIM_ATTR_CH_FREQ2 # Configured center-freq-2.
+ *	%HWSIM_ATTR_CH_WIDTH # Configured channel width.
+ *	%HWSIM_ATTR_ADDR_TRANSMITTER # ID which radio we are notifying about.
+ *	%HWSIM_ATTR_ADDR_ID # Numeric Radio ID.
  * @__HWSIM_CMD_MAX: enum limit
  */
 enum {
@@ -81,6 +90,7 @@ enum {
 	HWSIM_CMD_NEW_RADIO,
 	HWSIM_CMD_DEL_RADIO,
 	HWSIM_CMD_GET_RADIO,
+	HWSIM_CMD_NOTIFY_CH_CHANGE,
 	__HWSIM_CMD_MAX,
 };
 #define HWSIM_CMD_MAX (_HWSIM_CMD_MAX - 1)
@@ -123,6 +133,9 @@ enum {
  * @HWSIM_ATTR_RADIO_NAME: Name of radio, e.g. phy666
  * @HWSIM_ATTR_NO_VIF:  Do not create vif (wlanX) when creating radio.
  * @HWSIM_ATTR_FREQ: Frequency at which packet is transmitted or received.
+ * @HWSIM_ATTR_CH_FREQ1: Configured center-freq-1.
+ * @HWSIM_ATTR_CH_FREQ2: Configured center-freq-2.
+ * @HWSIM_ATTR_CH_WIDTH: Configured channel width.
  * @__HWSIM_ATTR_MAX: enum limit
  */
 
@@ -149,6 +162,9 @@ enum {
 	HWSIM_ATTR_NO_VIF,
 	HWSIM_ATTR_FREQ,
 	HWSIM_ATTR_PAD,
+	HWSIM_ATTR_CH_FREQ1,
+	HWSIM_ATTR_CH_FREQ2,
+	HWSIM_ATTR_CH_WIDTH,
 	__HWSIM_ATTR_MAX,
 };
 #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
-- 
2.4.11

[PATCH v2 3/4] mac80211-hwsim: add rate-limited debugging for rx-netlink

[greearb]

From: Ben Greear <greearb@candelatech.com>

This makes it easier to understand why wmediumd (or similar)
is getting errors when sending frames to the kernel.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---

v2:  Add and use hwsim_ratelimit() instead of net_ratelimit.
     Squash two patches into this one.

 drivers/net/wireless/mac80211_hwsim.c | 55 +++++++++++++++++++++++++++--------
 1 file changed, 43 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 635ad03..c259b99 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -137,6 +137,12 @@ static int regtest = HWSIM_REGTEST_DISABLED;
 module_param(regtest, int, 0444);
 MODULE_PARM_DESC(regtest, "The type of regulatory test we want to run");
 
+DEFINE_RATELIMIT_STATE(hwsim_ratelimit_state, 5 * HZ, 10);
+int hwsim_ratelimit(void)
+{
+	return __ratelimit(&hwsim_ratelimit_state);
+}
+
 static const char *hwsim_alpha2s[] = {
 	"FI",
 	"AL",
@@ -1641,7 +1647,7 @@ static int mac80211_hwsim_config(struct ieee80211_hw *hw, u32 changed)
 
 	if (conf->chandef.chan)
 		wiphy_debug(hw->wiphy,
-			    "%s (freq=%d(%d - %d)/%s idle=%d ps=%d smps=%s)\n",
+			    "%s (chandef-chan freq=%d(%d - %d)/%s idle=%d ps=%d smps=%s)\n",
 			    __func__,
 			    conf->chandef.chan->center_freq,
 			    conf->chandef.center_freq1,
@@ -1652,7 +1658,7 @@ static int mac80211_hwsim_config(struct ieee80211_hw *hw, u32 changed)
 			    smps_modes[conf->smps_mode]);
 	else
 		wiphy_debug(hw->wiphy,
-			    "%s (freq=0 idle=%d ps=%d smps=%s)\n",
+			    "%s (no-chandef-chan freq=0 idle=%d ps=%d smps=%s)\n",
 			    __func__,
 			    !!(conf->flags & IEEE80211_CONF_IDLE),
 			    !!(conf->flags & IEEE80211_CONF_PS),
@@ -3018,8 +3024,11 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2,
 	if (!info->attrs[HWSIM_ATTR_ADDR_RECEIVER] ||
 	    !info->attrs[HWSIM_ATTR_FRAME] ||
 	    !info->attrs[HWSIM_ATTR_RX_RATE] ||
-	    !info->attrs[HWSIM_ATTR_SIGNAL])
+	    !info->attrs[HWSIM_ATTR_SIGNAL]) {
+		if (hwsim_ratelimit())
+			printk(KERN_DEBUG " hwsim rx-nl: Missing required attribute\n");
 		goto out;
+	}
 
 	dst = (void *)nla_data(info->attrs[HWSIM_ATTR_ADDR_RECEIVER]);
 	frame_data_len = nla_len(info->attrs[HWSIM_ATTR_FRAME]);
@@ -3027,29 +3036,53 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2,
 
 	/* Allocate new skb here */
 	skb = alloc_skb(frame_data_len, GFP_KERNEL);
-	if (skb == NULL)
-		goto err;
+	if (skb == NULL) {
+		if (hwsim_ratelimit())
+			printk(KERN_DEBUG " hwsim rx-nl: skb alloc failed, len: %d\n",
+			       frame_data_len);
+		goto out;
+	}
 
-	if (frame_data_len > IEEE80211_MAX_DATA_LEN)
-		goto err;
+	if (frame_data_len > IEEE80211_MAX_DATA_LEN) {
+		if (hwsim_ratelimit())
+			printk(KERN_DEBUG " hwsim rx-nl: data lenth error: %d  max: %d\n",
+			       frame_data_len, IEEE80211_MAX_DATA_LEN);
+		goto out;
+	}
 
 	/* Copy the data */
 	memcpy(skb_put(skb, frame_data_len), frame_data, frame_data_len);
 
 	data2 = get_hwsim_data_ref_from_addr(dst);
-	if (!data2)
+
+	if (!data2) {
+		if (hwsim_ratelimit())
+			printk(KERN_DEBUG " hwsim rx-nl: Cannot find radio %pM\n",
+			       dst);
 		goto out;
+	}
 
 	if (hwsim_net_get_netgroup(genl_info_net(info)) != data2->netgroup)
 		goto out;
 
-	if (info->snd_portid != data2->wmediumd)
+	if (info->snd_portid != data2->wmediumd) {
+		if (hwsim_ratelimit())
+			printk(KERN_DEBUG " hwsim rx-nl: portid mismatch, snd_portid: %d  portid %d\n",
+			       info->snd_portid, data2->wmediumd);
 		goto out;
+	}
 
 	/* check if radio is configured properly */
 
-	if (data2->idle || !data2->started)
+	if (data2->idle || !data2->started) {
+		static unsigned int cnt = 0;
+		/* This is fairly common, and usually not a bug.  So, print errors
+		   rarely. */
+		if (((cnt++ & 0x3FF) == 0x3FF) && hwsim_ratelimit())
+			printk(KERN_DEBUG " hwsim rx-nl: radio %pM idle: %d or not started: %d cnt: %d\n",
+			       dst, data2->idle, !data2->started, cnt);
 		goto out;
+	}
 
 	/* A frame is received from user space */
 	memset(&rx_status, 0, sizeof(rx_status));
@@ -3082,8 +3115,6 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2,
 	ieee80211_rx_irqsafe(data2->hw, skb);
 
 	return 0;
-err:
-	printk(KERN_DEBUG "mac80211_hwsim: error occurred in %s\n", __func__);
 out:
 	dev_kfree_skb(skb);
 	return -EINVAL;
-- 
2.4.11

[PATCH v2 4/4] mac80211-hwsim: add length checks before allocating skb.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Modify the receive-from-user-space logic to do length
and 'is-down' checks before trying to allocate an skb.

And, if we are going to ignore the pkt due to radio idle,
then do not return an error code to user-space.  User-space
cannot reliably know exactly when a radio is idle or not.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---

v2:  Don't return success when radio is idle, but do return unique
     error code (ENETDOWN) in hopes user-space can make a distinction.

 drivers/net/wireless/mac80211_hwsim.c | 41 +++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index c259b99..73dc627 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -3020,6 +3020,7 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2,
 	int frame_data_len;
 	void *frame_data;
 	struct sk_buff *skb = NULL;
+	int rv = -EINVAL;
 
 	if (!info->attrs[HWSIM_ATTR_ADDR_RECEIVER] ||
 	    !info->attrs[HWSIM_ATTR_FRAME] ||
@@ -3034,25 +3035,6 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2,
 	frame_data_len = nla_len(info->attrs[HWSIM_ATTR_FRAME]);
 	frame_data = (void *)nla_data(info->attrs[HWSIM_ATTR_FRAME]);
 
-	/* Allocate new skb here */
-	skb = alloc_skb(frame_data_len, GFP_KERNEL);
-	if (skb == NULL) {
-		if (hwsim_ratelimit())
-			printk(KERN_DEBUG " hwsim rx-nl: skb alloc failed, len: %d\n",
-			       frame_data_len);
-		goto out;
-	}
-
-	if (frame_data_len > IEEE80211_MAX_DATA_LEN) {
-		if (hwsim_ratelimit())
-			printk(KERN_DEBUG " hwsim rx-nl: data lenth error: %d  max: %d\n",
-			       frame_data_len, IEEE80211_MAX_DATA_LEN);
-		goto out;
-	}
-
-	/* Copy the data */
-	memcpy(skb_put(skb, frame_data_len), frame_data, frame_data_len);
-
 	data2 = get_hwsim_data_ref_from_addr(dst);
 
 	if (!data2) {
@@ -3081,9 +3063,30 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2,
 		if (((cnt++ & 0x3FF) == 0x3FF) && hwsim_ratelimit())
 			printk(KERN_DEBUG " hwsim rx-nl: radio %pM idle: %d or not started: %d cnt: %d\n",
 			       dst, data2->idle, !data2->started, cnt);
+		rv = -ENETDOWN;
 		goto out;
 	}
 
+	if (frame_data_len > IEEE80211_MAX_DATA_LEN) {
+		if (hwsim_ratelimit())
+			printk(KERN_DEBUG " hwsim rx-nl: data lenth error: %d  max: %d\n",
+			       frame_data_len, IEEE80211_MAX_DATA_LEN);
+		goto out;
+	}
+
+
+	/* Allocate new skb here */
+	skb = alloc_skb(frame_data_len, GFP_KERNEL);
+	if (skb == NULL) {
+		if (hwsim_ratelimit())
+			printk(KERN_DEBUG " hwsim rx-nl: skb alloc failed, len: %d\n",
+			       frame_data_len);
+		goto out;
+	}
+
+	/* Copy the data */
+	memcpy(skb_put(skb, frame_data_len), frame_data, frame_data_len);
+
 	/* A frame is received from user space */
 	memset(&rx_status, 0, sizeof(rx_status));
 	if (info->attrs[HWSIM_ATTR_FREQ]) {
-- 
2.4.11

Re: [PATCH v2 3/4] mac80211-hwsim: add rate-limited debugging for rx-netlink

[Joe Perches]

On Mon, 2017-02-27 at 13:56 -0800, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> This makes it easier to understand why wmediumd (or similar)
> is getting errors when sending frames to the kernel.
> 
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
> 
> v2:  Add and use hwsim_ratelimit() instead of net_ratelimit.
>      Squash two patches into this one.
> 
>  drivers/net/wireless/mac80211_hwsim.c | 55 +++++++++++++++++++++++++++--------
>  1 file changed, 43 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
[]
> @@ -137,6 +137,12 @@ static int regtest = HWSIM_REGTEST_DISABLED;
>  module_param(regtest, int, 0444);
>  MODULE_PARM_DESC(regtest, "The type of regulatory test we want to run");
>  
> +DEFINE_RATELIMIT_STATE(hwsim_ratelimit_state, 5 * HZ, 10);
> +int hwsim_ratelimit(void)
> +{
> +	return __ratelimit(&hwsim_ratelimit_state);
> +}

Maybe it'd be better to add a function like

__printf(1, 2)
static void hwsim_dbg_ratelimited(const char *fmt, ...)
{
	struct va_format vaf;
	va_list args;

	if (!__ratelimit(&hwsim_ratelimit_state))
		return;

	va_start(args, fmt);

	vaf.fmt = fmt;
	vaf.va = &args;

	printk(KERN_DEBUG "hwsim rx-nl: %pV", &vaf);

	va_end(args);	
}

[]
> @@ -3018,8 +3024,11 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2,
>  	if (!info->attrs[HWSIM_ATTR_ADDR_RECEIVER] ||
>  	    !info->attrs[HWSIM_ATTR_FRAME] ||
>  	    !info->attrs[HWSIM_ATTR_RX_RATE] ||
> -	    !info->attrs[HWSIM_ATTR_SIGNAL])
> +	    !info->attrs[HWSIM_ATTR_SIGNAL]) {
> +		if (hwsim_ratelimit())
> +			printk(KERN_DEBUG " hwsim rx-nl: Missing required attribute\n");

so this becomes:

		hwsim_dbg_ratelimited("Missing required attribute\n");

and why are these currently indented with a space?

>  		goto out;
> +	}
>  
>  	dst = (void *)nla_data(info->attrs[HWSIM_ATTR_ADDR_RECEIVER]);
>  	frame_data_len = nla_len(info->attrs[HWSIM_ATTR_FRAME]);
> @@ -3027,29 +3036,53 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2,
>  
>  	/* Allocate new skb here */
>  	skb = alloc_skb(frame_data_len, GFP_KERNEL);
> -	if (skb == NULL)
> -		goto err;
> +	if (skb == NULL) {
> +		if (hwsim_ratelimit())
> +			printk(KERN_DEBUG " hwsim rx-nl: skb alloc failed, len: %d\n",
> +			       frame_data_len);

		hwsim_dbg_ratelimited("skb alloc failed, len: %d\n",
				      frame_data_len);

etc...

Re: [PATCH v2 1/4] mac80211-hwsim: notify user-space about channel change.

[Johannes Berg]

> +	if (nla_put_u32(skb, HWSIM_ATTR_CH_FREQ2, data-
> >center_freq2))
> +		goto nla_put_failure;

I'd prefer this to be conditional on it being non-zero.

I'd have fixed it, but none of these patches apply cleanly to my tree.

johannes

Re: [PATCH v2 1/4] mac80211-hwsim: notify user-space about channel change.

[Johannes Berg]

> + * @HWSIM_ATTR_CH_FREQ1: Configured center-freq-1.

CENTER_FREQ1 might be better? CH will be equated with "channel", and
that's kinda wrong.

johannes