DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 007292146D
MIME-Version: 1.0
References: <20181004195906.201895-1-schuffelen@google.com> <20181005143323.ezyd2x6x5ymlb7rg@bars>
In-Reply-To: <20181005143323.ezyd2x6x5ymlb7rg@bars>
From:   Cody Schuffelen <schuffelen@google.com>
Date:   Tue, 20 Nov 2018 19:20:01 -0800
Message-ID: <CAAELVAF+-6F0Qvj3wU-sO0+EF5Qs0VXYqD9GEeV0ZuP-dKOoFA@mail.gmail.com>
Subject: Re: [PATCH net-next v3] wireless-drivers: rtnetlink wifi simulation device
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
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

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