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, _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 
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, _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.

[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

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, _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 
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, _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 
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, _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 
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, _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

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

[greearb]

From: Ben Greear 

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 
---
 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, _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(_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