diff mbox

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

Message ID 1490311578-18926-1-git-send-email-greearb@candelatech.com (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show

Commit Message

Ben Greear March 23, 2017, 11:26 p.m. UTC
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>
---

v3:  Rebased against latest wireless-testing, renamed "CH_FREQ" to CENTER_FREQ,
don't send center_freq2 if value is zero.

Compile tested, not run-time tested since original patches were written.

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

Comments

Johannes Berg March 29, 2017, 8:42 a.m. UTC | #1
> +	if (data->center_freq2)
> +		if (nla_put_u32(skb, HWSIM_ATTR_CENTER_FREQ2, data-
> >center_freq2))
> +			goto nla_put_failure;
> 

That could be simplified (but isn't really interesting)

> + * @HWSIM_ATTR_CENTER_FREQ2: Configured center-freq-2.  Not reported
> if value is zero.

The value 0 really just means "unused" - so this should say "if used"
or so.

> + * @HWSIM_ATTR_CH_WIDTH: Configured channel width.

That should point to &enum nl80211_chan_width I guess.

Anyway, all of that is minor. The biggest problem I have with this
patch upon close review is that it only handles one case, and actually
ties that case to the nl80211 API.

hwsim also can deal with channel contexts already, is there much point
in making the userspace API ignorant of that?

johannes
Ben Greear March 29, 2017, 3:35 p.m. UTC | #2
On 03/29/2017 01:42 AM, Johannes Berg wrote:
>
>> +	if (data->center_freq2)
>> +		if (nla_put_u32(skb, HWSIM_ATTR_CENTER_FREQ2, data-
>>> center_freq2))
>> +			goto nla_put_failure;
>>
>
> That could be simplified (but isn't really interesting)
>
>> + * @HWSIM_ATTR_CENTER_FREQ2: Configured center-freq-2.  Not reported
>> if value is zero.
>
> The value 0 really just means "unused" - so this should say "if used"
> or so.
>
>> + * @HWSIM_ATTR_CH_WIDTH: Configured channel width.
>
> That should point to &enum nl80211_chan_width I guess.
>
> Anyway, all of that is minor. The biggest problem I have with this
> patch upon close review is that it only handles one case, and actually
> ties that case to the nl80211 API.
>
> hwsim also can deal with channel contexts already, is there much point
> in making the userspace API ignorant of that?

The patch appeared to solve my problem, and it seems an improvement
over what is there currently.  Whoever wants it to support even more things
can add that in the future?

If you remember, the first iteration of my patch had the NL API defines
more general, so that someone could just add more data members
as the needs grew.  It can still grow, however.

Thanks,
Ben
Johannes Berg March 29, 2017, 4:51 p.m. UTC | #3
> The patch appeared to solve my problem, and it seems an improvement
> over what is there currently.  Whoever wants it to support even more
> things can add that in the future?

Yes, that's kinda true. However, it also means that wmediumd (or
similar) would have to support the two APIs. It'd be simpler for them
to just have to support a single one, no?

johannes
Ben Greear March 29, 2017, 5:11 p.m. UTC | #4
On 03/29/2017 09:51 AM, Johannes Berg wrote:
>
>> The patch appeared to solve my problem, and it seems an improvement
>> over what is there currently.  Whoever wants it to support even more
>> things can add that in the future?
>
> Yes, that's kinda true. However, it also means that wmediumd (or
> similar) would have to support the two APIs. It'd be simpler for them
> to just have to support a single one, no?

I really don't understand what change you are suggesting.  Anything that
uses a new API needs to change, and as API is further improved, then the
app will need to change again.  Netlink's saving grace is that it is easy to
add new data members and so support new API in a forward/backward compatible manner.

Thanks,
Ben
Johannes Berg March 31, 2017, 11:48 a.m. UTC | #5
> I really don't understand what change you are suggesting.  Anything
> that uses a new API needs to change, and as API is further improved,
> then the app will need to change again.  Netlink's saving grace is
> that it is easy to add new data members and so support new API in a
> forward/backward compatible manner.

That's true, but it'd still mean you have to update all the time, and
perhaps then we'll find out it'd be easier to break the API, etc.?

OTOH, supporting (on both sides) channel contexts doesn't seem so hard
- perhaps something like
 * add chanctx
 * remove chanctx
 * change chanctx config

as hwsim commands, and then we can pretend to add/remove in start/stop
if we have non-chanctx-hwsim?

johannes
Ben Greear March 31, 2017, 1:33 p.m. UTC | #6
On 03/31/2017 04:48 AM, Johannes Berg wrote:
>
>> I really don't understand what change you are suggesting.  Anything
>> that uses a new API needs to change, and as API is further improved,
>> then the app will need to change again.  Netlink's saving grace is
>> that it is easy to add new data members and so support new API in a
>> forward/backward compatible manner.
>
> That's true, but it'd still mean you have to update all the time, and
> perhaps then we'll find out it'd be easier to break the API, etc.?

In my experience, the big problem with netlink is that if you write
a patch that cannot make it upstream (or takes forever), then the netlink
IDs conflict as upstream adds more stuff.

Other than that, it is easy to add new members, or completely new commands.

User-space can drop old API and simply not fully work against older kernels
if it wants, especially for something as specialized as a simulated
wifi radio.

 >
> OTOH, supporting (on both sides) channel contexts doesn't seem so hard
> - perhaps something like
>  * add chanctx
>  * remove chanctx
>  * change chanctx config
>
> as hwsim commands, and then we can pretend to add/remove in start/stop
> if we have non-chanctx-hwsim?

The project I did this work for is basically done and frozen.  I can tweak
some small API changes to sync up with new netlink ID numbers, but I have no
time to completely re-do the API (and more importantly, to test it all).

So, if my patch can go in as is or with small tweaks, then I'm happy to
keep working on it.  If it needs a complete re-write, then it will have to
wait for someone else or some later date.

Thanks,
Ben
Johannes Berg April 18, 2017, 9:58 a.m. UTC | #7
On Fri, 2017-03-31 at 06:33 -0700, Ben Greear wrote:
> 
> In my experience, the big problem with netlink is that if you write
> a patch that cannot make it upstream (or takes forever), then the
> netlink IDs conflict as upstream adds more stuff.

Sure, that's a common problem we all run into :)

> Other than that, it is easy to add new members, or completely new
> commands.
> 
> User-space can drop old API and simply not fully work against older
> kernels if it wants, especially for something as specialized as a
> simulated wifi radio.

Yeah, but the kernel will have to maintain both versions, and strictly
speaking shouldn't be breaking old userspace - but that would be
impossible as one moves to chanctx, perhaps even by default.

This is the problem I have with it - chanctx code already exists and is
used, so there's no technical reason not to support both now.

> So, if my patch can go in as is or with small tweaks, then I'm happy
> to keep working on it.  If it needs a complete re-write, then it will
> have to wait for someone else or some later date.

Ok, that's fair. I think I'll leave it out then though, because I
really do think that we should aim to support chanctx from the start
with this, it's well-established by now.

johannes
diff mbox

Patch

diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 67fc91d..f1ad908 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -533,6 +533,10 @@  struct mac80211_hwsim_data {
 		      ARRAY_SIZE(hwsim_channels_5ghz)];
 
 	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;
@@ -1004,6 +1008,65 @@  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_CENTER_FREQ1, data->center_freq1))
+		goto nla_put_failure;
+
+	if (data->center_freq2)
+		if (nla_put_u32(skb, HWSIM_ATTR_CENTER_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)
@@ -1631,6 +1694,11 @@  static int mac80211_hwsim_config(struct ieee80211_hw *hw, u32 changed)
 	} else {
 		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;
+
 	mutex_unlock(&data->mutex);
 
 	data->power_level = conf->power_level;
@@ -1646,6 +1714,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 3f5eda5..523fbc4 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_CENTER_FREQ1: Configured center-freq-1.
+ * @HWSIM_ATTR_CENTER_FREQ2: Configured center-freq-2.  Not reported if value is zero.
+ * @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_CENTER_FREQ1,
+	HWSIM_ATTR_CENTER_FREQ2,
+	HWSIM_ATTR_CH_WIDTH,
 	__HWSIM_ATTR_MAX,
 };
 #define HWSIM_ATTR_MAX (__HWSIM_ATTR_MAX - 1)