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