From: Cody Schuffelen <schuffelen@google.com>
To: sergey.matyukevich.os@quantenna.com
Cc: Johannes Berg <johannes@sipsolutions.net>,
Kalle Valo <kvalo@codeaurora.org>,
"David S. Miller" <davem@davemloft.net>,
linux-kernel@vger.kernel.org, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, kernel-team@android.com
Subject: Re: [PATCH net-next v3] wireless-drivers: rtnetlink wifi simulation device
Date: Tue, 20 Nov 2018 19:20:01 -0800 [thread overview]
Message-ID: <CAAELVAF+-6F0Qvj3wU-sO0+EF5Qs0VXYqD9GEeV0ZuP-dKOoFA@mail.gmail.com> (raw)
In-Reply-To: <20181005143323.ezyd2x6x5ymlb7rg@bars>
> > + informed_bss =
> > + cfg80211_inform_bss_data(wiphy, &mock_inform_bss,
> > + CFG80211_BSS_FTYPE_PRESP,
> > + fake_router_bssid,
> > + mock_inform_bss.boottime_ns,
> > + WLAN_CAPABILITY_ESS, 0, ssid.data,
> > + sizeof(ssid), GFP_KERNEL);
>
> It is possible to simplify this part switching to cfg80211_inform_bss
> function: this function wraps your scan data in into cfg80211_inform_bss
> structure internally using some reasonable defaults, e.g. channel width.
>
> Besides, signal strength for scan entries should be passed in mBm units,
> so use DBM_TO_MBM macro. For instance, with your current code 'iw' tool
> produces the following output:
> $ sudo iw dev wlan0 scan
> ...
> signal: 0.-60 dBm
> ...
Good catch, fixed.
> > +static void virt_wifi_connect_complete(struct work_struct *work)
> > +{
> > + struct virt_wifi_priv *priv =
> > + container_of(work, struct virt_wifi_priv, connect.work);
> > + u8 *requested_bss = priv->connect_requested_bss;
> > + bool has_addr = !is_zero_ether_addr(requested_bss);
> > + bool right_addr = ether_addr_equal(requested_bss, fake_router_bssid);
> > + u16 status = WLAN_STATUS_SUCCESS;
> > +
> > + rtnl_lock();
> > + if (!priv->netdev_is_up || (has_addr && !right_addr))
> > + status = WLAN_STATUS_UNSPECIFIED_FAILURE;
> > + else
> > + priv->is_connected = true;
> > +
> > + cfg80211_connect_result(priv->netdev, requested_bss, NULL, 0, NULL, 0,
> > + status, GFP_KERNEL);
> > + rtnl_unlock();
> > +}
>
> Carrier state for wireless device depends on its connection state.
> E.g., carrier is set when wireless connection succeeds and cleared
> when device disconnects. So use netif_carrier_on/netif_carrier_off
> calls in connect/disconnect handlers to set correct carrier state.
> IIUC the following locations look reasonable:
> - netif_carrier_off on init
> - netif_carrier_on in virt_wifi_connect_complete on success
> - netif_carrier_off in virt_wifi_disconnect
Thanks, added.
> > +static void virt_wifi_disconnect_complete(struct work_struct *work)
> > +{
> > + struct virt_wifi_priv *priv =
> > + container_of(work, struct virt_wifi_priv, disconnect.work);
> > +
> > + cfg80211_disconnected(priv->netdev, priv->disconnect_reason, NULL, 0,
> > + true, GFP_KERNEL);
> > + priv->is_connected = false;
> > +}
>
> Why do you need delayed disconnect processing ? IIUC it can be dropped
> and cfg80211_disconnected call can be moved to virt_wifi_disconnect.
Done.
> > +
> > +static int virt_wifi_get_station(struct wiphy *wiphy,
> > + struct net_device *dev,
> > + const u8 *mac,
> > + struct station_info *sinfo)
> > +{
> > + wiphy_debug(wiphy, "get_station\n");
> > +
> > + if (!ether_addr_equal(mac, fake_router_bssid))
> > + return -ENOENT;
> > +
> > + sinfo->filled = BIT(NL80211_STA_INFO_TX_PACKETS) |
> > + BIT(NL80211_STA_INFO_TX_FAILED) | BIT(NL80211_STA_INFO_SIGNAL) |
> > + BIT(NL80211_STA_INFO_TX_BITRATE);
>
> Recently some of NL80211_STA_INFO_ attribute types has been modified to
> use BIT_ULL macro. Could you please check commit 22d0d2fafca93ba1d92a
> for details and modify your coded if needed.
Thanks for the the reference, updated to use BIT_ULL with the station commands.
> > + sinfo->tx_packets = 1;
>
> Only one packet, really ? Not sure if you plan to use the output of 'iw'
> or any other tool. If yes, then it probably makes sense to use stats
> from the original network link. Otherwise, your 'iw' output is
> going to look like this:
>
> $ iw dev wlan0 station dump
> ...
> tx packets: 1
> ...
>
> > + sinfo->tx_failed = 0;
>
> ...
Added bookkeeping to the net_device packet forwarded to track how many
packets were sent, and how many failed being sent due to no
connection.
> > +static int virt_wifi_dump_station(struct wiphy *wiphy,
> > + struct net_device *dev,
> > + int idx,
> > + u8 *mac,
> > + struct station_info *sinfo)
> > +{
> > + wiphy_debug(wiphy, "dump_station\n");
> > +
> > + if (idx != 0)
> > + return -ENOENT;
> > +
> > + ether_addr_copy(mac, fake_router_bssid);
> > + return virt_wifi_get_station(wiphy, dev, fake_router_bssid, sinfo);
> > +}
>
> Callback dump_station should return AP data only when STA is connected.
> Currently your driver returns fake AP data even when it is not
> connected.
Thanks, fixed.
> > +static const struct cfg80211_ops virt_wifi_cfg80211_ops = {
> > + .scan = virt_wifi_scan,
> > +
> > + .connect = virt_wifi_connect,
> > + .disconnect = virt_wifi_disconnect,
> > +
> > + .get_station = virt_wifi_get_station,
> > + .dump_station = virt_wifi_dump_station,
> > +};
>
> Hey, this minimal cfg80211 implementation works fine with wpa_supplicant
> and open AP config. By the way, if you plan to add more features, then
> I would suggest to consider the following cfg80211 callbacks:
> - change_station, get_channel
> to provide more info in connected state, e.g. compare the output
> of the following commands between your virtual interface and
> actual wireless interface:
> $ iw dev wlan0 link
> $ iw dev wlan0 info
>
> - stubs for add_key, del_key to enable encrypted AP simulation
Thanks for testing it out!
I've uploaded the new version here: https://lkml.org/lkml/2018/11/21/251
next prev parent reply other threads:[~2018-11-21 3:20 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-04 19:59 [PATCH net-next v3] wireless-drivers: rtnetlink wifi simulation device Cody Schuffelen
2018-10-05 14:33 ` Sergey Matyukevich
2018-10-05 21:05 ` Joel Fernandes
2018-10-05 21:08 ` Joel Fernandes
[not found] ` <CAAELVAGAZ-UM77P=JqzHjUGja9oJcN5y9+YvxZWKBmOXAu=zNQ@mail.gmail.com>
2018-10-06 12:01 ` Kalle Valo
2018-11-21 3:20 ` Cody Schuffelen
2018-10-08 13:56 ` Sergey Matyukevich
2018-11-21 3:20 ` Cody Schuffelen [this message]
2018-10-06 11:52 ` Kalle Valo
2018-11-21 3:20 ` Cody Schuffelen
2018-10-09 8:25 ` Johannes Berg
2018-11-21 3:20 ` Cody Schuffelen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAAELVAF+-6F0Qvj3wU-sO0+EF5Qs0VXYqD9GEeV0ZuP-dKOoFA@mail.gmail.com \
--to=schuffelen@google.com \
--cc=davem@davemloft.net \
--cc=johannes@sipsolutions.net \
--cc=kernel-team@android.com \
--cc=kvalo@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=sergey.matyukevich.os@quantenna.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).