[PATCH 099/306] mac80211-hwsim: notify user-space about channel change.

[PATCH 099/306] 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>
---
 drivers/net/wireless/mac80211_hwsim.c | 48 +++++++++++++++++++++++++++++++++++
 drivers/net/wireless/mac80211_hwsim.h |  6 +++++
 2 files changed, 54 insertions(+)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index d3bad57..af53317 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -1005,6 +1005,52 @@ 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 = ACCESS_ONCE(data->wmediumd);
+
+	if (!_portid)
+		return;
+
+	skb = genlmsg_new(GENLMSG_DEFAULT_SIZE, GFP_ATOMIC);
+	if (skb == NULL)
+		goto err_print;
+
+	msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0,
+			       HWSIM_CMD_NOTIFY);
+	if (msg_head == NULL) {
+		printk(KERN_DEBUG "mac80211_hwsim: problem with msg_head, notify\n");
+		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;
+
+	genlmsg_end(skb, msg_head);
+	if (genlmsg_unicast(&init_net, skb, _portid))
+		goto err_print;
+
+	return;
+
+nla_put_failure:
+	nlmsg_free(skb);
+err_print:
+	printk(KERN_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)
@@ -1622,6 +1668,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..73646b1 100644
--- a/drivers/net/wireless/mac80211_hwsim.h
+++ b/drivers/net/wireless/mac80211_hwsim.h
@@ -71,6 +71,11 @@ 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: notify user-space about driver changes.  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_ADDR_TRANSMITTER # ID which radio we are notifying about.
  * @__HWSIM_CMD_MAX: enum limit
  */
 enum {
@@ -81,6 +86,7 @@ enum {
 	HWSIM_CMD_NEW_RADIO,
 	HWSIM_CMD_DEL_RADIO,
 	HWSIM_CMD_GET_RADIO,
+	HWSIM_CMD_NOTIFY,
 	__HWSIM_CMD_MAX,
 };
 #define HWSIM_CMD_MAX (_HWSIM_CMD_MAX - 1)
-- 
2.4.11

[PATCH 100/306] 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, 1 insertion(+), 1 deletion(-)

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

Re: [PATCH 100/306] mac80211-hwsim: remove dmesg spam about get-survey.

[Johannes Berg]

On Thu, 2017-02-23 at 16:28 -0800, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> This message just fills up dmesg and/or kernel logs and does
> not provide any useful information.

Fine, but don't just comment it out.

johannes

[PATCH 101/306] mac80211-hwsim: Pass rate-ctrl flags and tx-power to user-space

[greearb]

From: Ben Greear <greearb@candelatech.com>

The flags field allows user-space to properly decode what the
rate->idx really means.  The tx-power field is useful as well,
in case user-space is interested in doing something clever with
path-loss calculations.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/mac80211_hwsim.c | 21 ++++++++++++++++++++-
 drivers/net/wireless/mac80211_hwsim.h |  8 ++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index b8189b9..853c7ae 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -629,6 +629,9 @@ static const struct nla_policy hwsim_genl_policy[HWSIM_ATTR_MAX + 1] = {
 	[HWSIM_ATTR_RADIO_NAME] = { .type = NLA_STRING },
 	[HWSIM_ATTR_NO_VIF] = { .type = NLA_FLAG },
 	[HWSIM_ATTR_FREQ] = { .type = NLA_U32 },
+	[HWSIM_ATTR_TX_INFO2] = { .type = NLA_UNSPEC,
+				  .len = IEEE80211_TX_MAX_RATES *
+				         sizeof(struct hwsim_tx_rate2)},
 };
 
 static void mac80211_hwsim_tx_frame(struct ieee80211_hw *hw,
@@ -1064,6 +1067,7 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
 	int i;
 	struct hwsim_tx_rate tx_attempts[IEEE80211_TX_MAX_RATES];
 	uintptr_t cookie;
+	struct hwsim_tx_rate2 tx_attempts2[IEEE80211_TX_MAX_RATES];
 
 	if (data->ps != PS_DISABLED)
 		hdr->frame_control |= cpu_to_le16(IEEE80211_FCTL_PM);
@@ -1115,6 +1119,8 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
 	for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
 		tx_attempts[i].idx = info->status.rates[i].idx;
 		tx_attempts[i].count = info->status.rates[i].count;
+		tx_attempts2[i].rc_flags = info->status.rates[i].flags;
+		tx_attempts2[i].power_level = data->power_level;
 	}
 
 	if (nla_put(skb, HWSIM_ATTR_TX_INFO,
@@ -1122,6 +1128,11 @@ static void mac80211_hwsim_tx_frame_nl(struct ieee80211_hw *hw,
 		    tx_attempts))
 		goto nla_put_failure;
 
+	if (nla_put(skb, HWSIM_ATTR_TX_INFO2,
+		    sizeof(struct hwsim_tx_rate2)*IEEE80211_TX_MAX_RATES,
+		    tx_attempts2))
+		goto nla_put_failure;
+
 	/* We create a cookie to identify this skb */
 	data->pending_cookie++;
 	cookie = data->pending_cookie;
@@ -2898,6 +2909,7 @@ static int hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
 	struct ieee80211_tx_info *txi;
 	struct hwsim_tx_rate *tx_attempts;
 	u64 ret_skb_cookie;
+	struct hwsim_tx_rate2 *tx_attempts2;
 	struct sk_buff *skb, *tmp;
 	const u8 *src;
 	unsigned int hwsim_flags;
@@ -2949,6 +2961,12 @@ static int hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
 	tx_attempts = (struct hwsim_tx_rate *)nla_data(
 		       info->attrs[HWSIM_ATTR_TX_INFO]);
 
+	if (info->attrs[HWSIM_ATTR_TX_INFO2])
+		tx_attempts2 = (struct hwsim_tx_rate2 *)nla_data(
+		       info->attrs[HWSIM_ATTR_TX_INFO2]);
+	else
+		tx_attempts2 = NULL;
+
 	/* now send back TX status */
 	txi = IEEE80211_SKB_CB(skb);
 
@@ -2957,7 +2975,8 @@ static int hwsim_tx_info_frame_received_nl(struct sk_buff *skb_2,
 	for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
 		txi->status.rates[i].idx = tx_attempts[i].idx;
 		txi->status.rates[i].count = tx_attempts[i].count;
-		/*txi->status.rates[i].flags = 0;*/
+		if (tx_attempts2)
+			txi->status.rates[i].flags = tx_attempts2[i].rc_flags;
 	}
 
 	txi->status.ack_signal = nla_get_u32(info->attrs[HWSIM_ATTR_SIGNAL]);
diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
index 73646b1..480c0a7 100644
--- a/drivers/net/wireless/mac80211_hwsim.h
+++ b/drivers/net/wireless/mac80211_hwsim.h
@@ -129,6 +129,7 @@ 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_TX_INFO2: hwsim_tx_rate2 array
  * @__HWSIM_ATTR_MAX: enum limit
  */
 
@@ -155,6 +156,7 @@ enum {
 	HWSIM_ATTR_NO_VIF,
 	HWSIM_ATTR_FREQ,
 	HWSIM_ATTR_PAD,
+	HWSIM_ATTR_TX_INFO2,
 	__HWSIM_ATTR_MAX,
 };
 #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
@@ -177,4 +179,10 @@ struct hwsim_tx_rate {
 	u8 count;
 } __packed;
 
+/* Auxilary info to allow user-space to better understand the rate */
+struct hwsim_tx_rate2 {
+	u16 rc_flags; /* rate-ctrl flags (see mac80211_rate_control_flags) */
+	s16 power_level;
+} __packed;
+
 #endif /* __MAC80211_HWSIM_H */
-- 
2.4.11

Re: [PATCH 101/306] mac80211-hwsim: Pass rate-ctrl flags and tx-power to user-space

[Johannes Berg]

> +/* Auxilary info to allow user-space to better understand the rate
> */
> +struct hwsim_tx_rate2 {
> +	u16 rc_flags; /* rate-ctrl flags (see
> mac80211_rate_control_flags) */
> +	s16 power_level;
> +} __packed;

Nope, not going to happen - I'm not tying the user-space visible API to
the mac80211 internals that directly.

johannes

[PATCH 102/306] mac80211-hwsim: enable better rx-status when using netlink.

[greearb]

From: Ben Greear <greearb@candelatech.com>

This allows proper rx-status reporting for packets received
from the netlink api.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/mac80211_hwsim.c | 10 ++++++++++
 drivers/net/wireless/mac80211_hwsim.h | 18 ++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 853c7ae..8d494ac 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -3068,6 +3068,16 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2,
 	rx_status.rate_idx = nla_get_u32(info->attrs[HWSIM_ATTR_RX_RATE]);
 	rx_status.signal = nla_get_u32(info->attrs[HWSIM_ATTR_SIGNAL]);
 
+	if (info->attrs[HWSIM_ATTR_RX_INFO]) {
+		struct hwsim_rx_info *r;
+		r = (struct hwsim_rx_info *)nla_data(
+			info->attrs[HWSIM_ATTR_RX_INFO]);
+		rx_status.flag = r->rx_flags;
+		rx_status.vht_flag = r->vht_flags;
+		rx_status.vht_nss = r->vht_nss;
+		rx_status.ampdu_reference = r->ampdu_reference;
+	}
+
 	memcpy(IEEE80211_SKB_RXCB(skb), &rx_status, sizeof(rx_status));
 	data2->rx_pkts++;
 	data2->rx_bytes += skb->len;
diff --git a/drivers/net/wireless/mac80211_hwsim.h b/drivers/net/wireless/mac80211_hwsim.h
index 480c0a7..d9dd2f8 100644
--- a/drivers/net/wireless/mac80211_hwsim.h
+++ b/drivers/net/wireless/mac80211_hwsim.h
@@ -130,6 +130,7 @@ enum {
  * @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_TX_INFO2: hwsim_tx_rate2 array
+ * @HWSIM_ATTR_RX_INFO: hwsim_rx_info
  * @__HWSIM_ATTR_MAX: enum limit
  */
 
@@ -157,6 +158,7 @@ enum {
 	HWSIM_ATTR_FREQ,
 	HWSIM_ATTR_PAD,
 	HWSIM_ATTR_TX_INFO2,
+	HWSIM_ATTR_RX_INFO,
 	__HWSIM_ATTR_MAX,
 };
 #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)
@@ -185,4 +187,20 @@ struct hwsim_tx_rate2 {
 	s16 power_level;
 } __packed;
 
+/**
+ * This relates to the ieee80211_rx_status struct in mac80211.h
+ * @rx_flags: %RX_FLAG_* (see  mac80211_rx_flags)
+ * @vht_flags: %RX_VHT_FLAG_*
+ * @vht_nss: number of streams (VHT only)
+ * @ampdu_reference: A-MPDU reference number, must be a different value for
+ *	each A-MPDU but the same for each subframe within one A-MPDU
+ */
+struct hwsim_rx_info {
+	u32 rx_flags;
+	u8 vht_flags;
+	u8 vht_nss;
+	u16 unused_pad; /* pad to 32-bits, and space for growth */
+	u32 ampdu_reference;
+} __packed;
+
 #endif /* __MAC80211_HWSIM_H */
-- 
2.4.11

Re: [PATCH 102/306] mac80211-hwsim: enable better rx-status when using netlink.

[Johannes Berg]

> +/**
> + * This relates to the ieee80211_rx_status struct in mac80211.h
> + * @rx_flags: %RX_FLAG_* (see  mac80211_rx_flags)
> + * @vht_flags: %RX_VHT_FLAG_*
> + * @vht_nss: number of streams (VHT only)
> + * @ampdu_reference: A-MPDU reference number, must be a different
> value for
> + *	each A-MPDU but the same for each subframe within one A-
> MPDU
> + */
> +struct hwsim_rx_info {
> +	u32 rx_flags;
> +	u8 vht_flags;
> +	u8 vht_nss;
> +	u16 unused_pad; /* pad to 32-bits, and space for growth */
> +	u32 ampdu_reference;
> +} __packed;

Same as in the previous patch.

johannes

[PATCH 160/306] 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>
---
 drivers/net/wireless/mac80211_hwsim.c | 42 ++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 8d494ac..aaba126 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -3010,8 +3010,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 (net_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]);
@@ -3019,29 +3022,50 @@ 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 (net_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 (net_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 (net_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 (net_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) {
+		if (net_ratelimit())
+			printk(KERN_DEBUG " hwsim rx-nl: radio %pM idle: %d or not started: %d\n",
+			       dst, data2->idle, !data2->started);
 		goto out;
+	}
 
 	/* A frame is received from user space */
 	memset(&rx_status, 0, sizeof(rx_status));
@@ -3084,8 +3108,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

Re: [PATCH 160/306] mac80211-hwsim: add rate-limited debugging for rx-netlink

[Johannes Berg]

> +	    !info->attrs[HWSIM_ATTR_SIGNAL]) {
> +		if (net_ratelimit())
> +			printk(KERN_DEBUG " hwsim rx-nl: Missing
> required attribute\n");

I'm not convinced net_ratelimit() is a good idea, that's a global rate
limiter.

johannes

Re: [PATCH 160/306] mac80211-hwsim: add rate-limited debugging for rx-netlink

[Ben Greear]

On 02/23/2017 10:39 PM, Johannes Berg wrote:
>
>> +	    !info->attrs[HWSIM_ATTR_SIGNAL]) {
>> +		if (net_ratelimit())
>> +			printk(KERN_DEBUG " hwsim rx-nl: Missing
>> required attribute\n");
>
> I'm not convinced net_ratelimit() is a good idea, that's a global rate
> limiter.

Is there a better rate-limiter w/out hand-crafting something?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 160/306] mac80211-hwsim: add rate-limited debugging for rx-netlink

[Johannes Berg]

On Fri, 2017-02-24 at 07:27 -0800, Ben Greear wrote:
> 
> On 02/23/2017 10:39 PM, Johannes Berg wrote:
> > 
> > > +	    !info->attrs[HWSIM_ATTR_SIGNAL]) {
> > > +		if (net_ratelimit())
> > > +			printk(KERN_DEBUG " hwsim rx-nl: Missing
> > > required attribute\n");
> > 
> > I'm not convinced net_ratelimit() is a good idea, that's a global
> > rate limiter.
> 
> Is there a better rate-limiter w/out hand-crafting something?

You can use include/linux/ratelimit.h to do that?

johannes

[PATCH 161/306] mac80211-hwsim: Improve logging.

[greearb]

From: Ben Greear <greearb@candelatech.com>

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

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index aaba126..48ddf5d 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -1643,7 +1643,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,
@@ -1654,7 +1654,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),
@@ -3061,9 +3061,12 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2,
 	/* check if radio is configured properly */
 
 	if (data2->idle || !data2->started) {
-		if (net_ratelimit())
-			printk(KERN_DEBUG " hwsim rx-nl: radio %pM idle: %d or not started: %d\n",
-			       dst, 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) && net_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;
 	}
 
-- 
2.4.11

Re: [PATCH 161/306] mac80211-hwsim: Improve logging.

[Johannes Berg]

> +		static unsigned int cnt = 0;
> +		/* This is fairly common, and usually not a
> bug.  So, print errors
> +		   rarely. */
> +		if (((cnt++ & 0x3FF) == 0x3FF) && net_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;
>  	}

You just added that in the previous patch...

Please take a bit more care, or I'll eventually stop looking at your
patches since things like that seem to happen over and over again. I
don't get a feeling that you actually care about getting things
upstream much anyway.

johannes

Re: [PATCH 161/306] mac80211-hwsim: Improve logging.

[Ben Greear]

On 02/23/2017 10:42 PM, Johannes Berg wrote:
>
>> +		static unsigned int cnt = 0;
>> +		/* This is fairly common, and usually not a
>> bug.  So, print errors
>> +		   rarely. */
>> +		if (((cnt++ & 0x3FF) == 0x3FF) && net_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;
>>  	}
>
> You just added that in the previous patch...
>
> Please take a bit more care, or I'll eventually stop looking at your
> patches since things like that seem to happen over and over again. I
> don't get a feeling that you actually care about getting things
> upstream much anyway.

I wrote these patches over a long period of time.  Some, like the binary
API things I already knew you did not want, but I posted them since
it seems several people are looking at this sort of thing and maybe
they will have a good idea how to add similar behaviour in an efficient
manner.  I think your previous suggestion was that I should map each
flag in some translate code and/or bloat up netlink API, and neither
of those options seemed like an efficient use of CPU time.

I'll work to fix the cosmetic problems and squash these logging patches
and re-submit those.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

[PATCH 162/306] 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>
---
 drivers/net/wireless/mac80211_hwsim.c | 43 +++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 48ddf5d..3a96933 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -3020,25 +3020,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 (net_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 (net_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) {
@@ -3067,9 +3048,33 @@ static int hwsim_cloned_frame_received_nl(struct sk_buff *skb_2,
 		if (((cnt++ & 0x3FF) == 0x3FF) && net_ratelimit())
 			printk(KERN_DEBUG " hwsim rx-nl: radio %pM idle: %d or not started: %d cnt: %d\n",
 			       dst, data2->idle, !data2->started, cnt);
+		/* Fail silently, no need to bug user-space about this, since lots of races
+		 * in up/down interface, and the user-space app cannot keep perfectly
+		 * in sync.
+		 */
+		return 0;
+	}
+
+	if (frame_data_len > IEEE80211_MAX_DATA_LEN) {
+		if (net_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 (net_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 162/306] mac80211-hwsim: add length checks before allocating skb.

[Johannes Berg]

And here's the third patch in a row modifying the same code ...

johannes

Re: [PATCH 162/306] mac80211-hwsim: add length checks before allocating skb.

[Andrew Zaborowski]

On 24 February 2017 at 01:28,  <greearb@candelatech.com> wrote:
> 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.

You probably want to return some error code anyway because 0, if you
compare to the kernel medium, currently maps to the ack returned bit
and is possibly the only way for userspace to set the
HWSIM_TX_STAT_ACK flag in a meaningful way.

Best regards

Re: [PATCH 162/306] mac80211-hwsim: add length checks before allocating skb.

[Ben Greear]

On 02/24/2017 12:45 AM, Andrew Zaborowski wrote:
> On 24 February 2017 at 01:28,  <greearb@candelatech.com> wrote:
>> 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.
>
> You probably want to return some error code anyway because 0, if you
> compare to the kernel medium, currently maps to the ack returned bit
> and is possibly the only way for userspace to set the
> HWSIM_TX_STAT_ACK flag in a meaningful way.

Maybe there is a way to return a specific error code so that
the user-space doesn't get concerned when radio is idle.  I
didn't want to spam logs in user-space app...

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 099/306] mac80211-hwsim: notify user-space about channel change.

[Johannes Berg]

> +	msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0,
> +			       HWSIM_CMD_NOTIFY);

I think you should use a more specific command name.

> +	if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER,
> +		    ETH_ALEN, data->addresses[1].addr))
> +		goto nla_put_failure;

and at least also add a more specific identifier like the radio ID.

> +	if (data->channel)
> +		center_freq = data->channel->center_freq;
> +
> +	if (nla_put_u32(skb, HWSIM_ATTR_FREQ, center_freq))
> +		goto nla_put_failure;

and have the full channel definition


Also the indentation in the documentation didn't match the convention
used there.

johannes

Re: [PATCH 099/306] mac80211-hwsim: notify user-space about channel change.

[Ben Greear]

On 02/23/2017 10:36 PM, Johannes Berg wrote:
>
>
>> +	msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0,
>> +			       HWSIM_CMD_NOTIFY);
>
> I think you should use a more specific command name.

My idea was that other attributes could be added over time without
having to add a new cmd-id, so that is why I left it general.  If you
still want a different command, do you want it to be something like
'HWSIM_CMD_CHANNEL_CHANGE' ?

Thanks,
Ben

>
>> +	if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER,
>> +		    ETH_ALEN, data->addresses[1].addr))
>> +		goto nla_put_failure;
>
> and at least also add a more specific identifier like the radio ID.
>
>> +	if (data->channel)
>> +		center_freq = data->channel->center_freq;
>> +
>> +	if (nla_put_u32(skb, HWSIM_ATTR_FREQ, center_freq))
>> +		goto nla_put_failure;
>
> and have the full channel definition
>
>
> Also the indentation in the documentation didn't match the convention
> used there.
>
> johannes
>

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 099/306] mac80211-hwsim: notify user-space about channel change.

[Johannes Berg]

On Fri, 2017-02-24 at 07:39 -0800, Ben Greear wrote:
> 
> On 02/23/2017 10:36 PM, Johannes Berg wrote:
> > 
> > 
> > > +	msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0,
> > > +			       HWSIM_CMD_NOTIFY);
> > 
> > I think you should use a more specific command name.
> 
> My idea was that other attributes could be added over time without
> having to add a new cmd-id, so that is why I left it general.  If you
> still want a different command, do you want it to be something like
> 'HWSIM_CMD_CHANNEL_CHANGE' ?

We won't run out of command IDs any time soon, so I don't really see a
problem with using a new one for all of those things if needed.

But having a general NOTIFY just means that the application will have
to parse the attributes and understand what means what - that seems
like a case of the cure being worse than the disease?

johannes

Re: [PATCH 099/306] mac80211-hwsim: notify user-space about channel change.

[Ben Greear]

On 02/23/2017 10:36 PM, Johannes Berg wrote:
>
>
>> +	msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0,
>> +			       HWSIM_CMD_NOTIFY);
>
> I think you should use a more specific command name.
>
>> +	if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER,
>> +		    ETH_ALEN, data->addresses[1].addr))
>> +		goto nla_put_failure;
>
> and at least also add a more specific identifier like the radio ID.
>
>> +	if (data->channel)
>> +		center_freq = data->channel->center_freq;
>> +
>> +	if (nla_put_u32(skb, HWSIM_ATTR_FREQ, center_freq))
>> +		goto nla_put_failure;
>
> and have the full channel definition

You want chandef.center_freq1,
	 chandef.center_freq2,
	 chandef.width?


Anything else?

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 099/306] mac80211-hwsim: notify user-space about channel change.

[Johannes Berg]

On Mon, 2017-02-27 at 12:48 -0800, Ben Greear wrote:
> On 02/23/2017 10:36 PM, Johannes Berg wrote:
> > 
> > 
> > > +	msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0,
> > > +			       HWSIM_CMD_NOTIFY);
> > 
> > I think you should use a more specific command name.
> > 
> > > +	if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER,
> > > +		    ETH_ALEN, data->addresses[1].addr))
> > > +		goto nla_put_failure;
> > 
> > and at least also add a more specific identifier like the radio ID.
> > 
> > > +	if (data->channel)
> > > +		center_freq = data->channel->center_freq;
> > > +
> > > +	if (nla_put_u32(skb, HWSIM_ATTR_FREQ, center_freq))
> > > +		goto nla_put_failure;
> > 
> > and have the full channel definition
> 
> You want chandef.center_freq1,
> 	 chandef.center_freq2,
> 	 chandef.width?
> 
> 
> Anything else?

The control channel center_freq is already there so that should be the
full chandef. I guess center_freq2 should be conditional on being non-
zero.

johannes

Re: [PATCH 099/306] mac80211-hwsim: notify user-space about channel change.

[Ben Greear]

On 03/02/2017 12:38 AM, Johannes Berg wrote:
> On Mon, 2017-02-27 at 12:48 -0800, Ben Greear wrote:
>> On 02/23/2017 10:36 PM, Johannes Berg wrote:
>>>
>>>
>>>> +	msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0,
>>>> +			       HWSIM_CMD_NOTIFY);
>>>
>>> I think you should use a more specific command name.
>>>
>>>> +	if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER,
>>>> +		    ETH_ALEN, data->addresses[1].addr))
>>>> +		goto nla_put_failure;
>>>
>>> and at least also add a more specific identifier like the radio ID.
>>>
>>>> +	if (data->channel)
>>>> +		center_freq = data->channel->center_freq;
>>>> +
>>>> +	if (nla_put_u32(skb, HWSIM_ATTR_FREQ, center_freq))
>>>> +		goto nla_put_failure;
>>>
>>> and have the full channel definition
>>
>> You want chandef.center_freq1,
>> 	 chandef.center_freq2,
>> 	 chandef.width?
>>
>>
>> Anything else?
>
> The control channel center_freq is already there so that should be the
> full chandef. I guess center_freq2 should be conditional on being non-
> zero.

I posted a new patch series a day or two ago...please let me know
if that looks right to you.  I un-conditionally included freq2, but
I think that is cleaner code all around.  Still, I'll make it conditional
if that is important to you.

Thanks,
Ben

>
> johannes
>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 099/306] mac80211-hwsim: notify user-space about channel change.

[Ben Greear]

On 02/23/2017 10:36 PM, Johannes Berg wrote:
>
>
>> +	msg_head = genlmsg_put(skb, 0, 0, &hwsim_genl_family, 0,
>> +			       HWSIM_CMD_NOTIFY);
>
> I think you should use a more specific command name.
>
>> +	if (nla_put(skb, HWSIM_ATTR_ADDR_TRANSMITTER,
>> +		    ETH_ALEN, data->addresses[1].addr))
>> +		goto nla_put_failure;
>
> and at least also add a more specific identifier like the radio ID.
>
>> +	if (data->channel)
>> +		center_freq = data->channel->center_freq;
>> +
>> +	if (nla_put_u32(skb, HWSIM_ATTR_FREQ, center_freq))
>> +		goto nla_put_failure;
>
> and have the full channel definition
>
>
> Also the indentation in the documentation didn't match the convention
> used there.

Looks like there are two conventions used (see HWSIM_CMD_TX_INFO_FRAME,
and HWSIM_CMD_NEW_RADIO).  I guess you want it indented like the NEW_RADIO
command?

Thanks,
Ben

>
> johannes
>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH 099/306] mac80211-hwsim: notify user-space about channel change.

[Johannes Berg]

> Looks like there are two conventions used (see
> HWSIM_CMD_TX_INFO_FRAME, and HWSIM_CMD_NEW_RADIO).  I guess you want
> it indented like the NEW_RADIO command?

Huh, sorry - yes, indented like NEW_RADIO please, I'll fix the others.

johannes