Re: [PATCH net-next v3] wireless-drivers: rtnetlink wifi simulation device

[Sergey Matyukevich]

Hi Cody,

>  drivers/net/wireless/Kconfig     |   7 +
>  drivers/net/wireless/Makefile    |   2 +
>  drivers/net/wireless/virt_wifi.c | 618 +++++++++++++++++++++++++++++++
>  3 files changed, 627 insertions(+)
>  create mode 100644 drivers/net/wireless/virt_wifi.c

I did a quick check of your patch using checkpatch kernel tool,
here is a summary of its output:

$ ./scripts/checkpatch.pl --strict test.patch
...
total: 165 errors, 428 warnings, 9 checks, 634 lines checked

Most part of those complaints is about either whitespaces or code
idents. I am not sure whether this is a patch itself or email client.
So could you please take a look and run checkpatch on your side.

> +static void virt_wifi_scan_result(struct work_struct *work)
> +{
> +       const union {
> +               struct {
> +                       u8 tag;
> +                       u8 len;
> +                       u8 ssid[8];
> +               } __packed parts;
> +               u8 data[10];
> +       } ssid = { .parts = {
> +               .tag = WLAN_EID_SSID, .len = 8, .ssid = "VirtWifi" }
> +       };
> +       struct cfg80211_bss *informed_bss;
> +       struct virt_wifi_priv *priv =
> +               container_of(work, struct virt_wifi_priv,
> +                            scan_result.work);
> +       struct wiphy *wiphy = priv_to_wiphy(priv);
> +       struct cfg80211_inform_bss mock_inform_bss = {
> +               .chan = &channel_5ghz,
> +               .scan_width = NL80211_BSS_CHAN_WIDTH_20,
> +               .signal = -60,
> +               .boottime_ns = ktime_get_boot_ns(),
> +       };
> +       struct cfg80211_scan_info scan_info = {};
> +
> +       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
	...

> +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

> +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.

> +
> +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.

> +       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;

...

> +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.

> +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

Regards,
Sergey