Message ID | 1490311578-18926-1-git-send-email-greearb@candelatech.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Johannes Berg |
Headers | show |
> + 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
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
> 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
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
> 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
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
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 --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)