Alright so I looked at this a little bit.
> +static int wil_cfg80211_set_txpower(struct wiphy *wiphy,
> +static int wil_cfg80211_get_txpower(struct wiphy *wiphy, int *dbm)
> +static int wil_cfg80211_mgmt_tx(struct wiphy *wiphy,
> + struct wireless_dev *wdev,
> + struct ieee80211_channel *chan, bool offchan,
> + enum nl80211_channel_type channel_type,
> + bool channel_type_valid, unsigned int wait,
> + const u8 *buf, size_t len, bool no_cck,
> + bool dont_wait_for_ack, u64 *cookie)
> +static void wil_cfg80211_mgmt_frame_register(struct wiphy *wiphy,
> + struct wireless_dev *wdev,
> + u16 frame_type, bool reg)
Don't implement things you don't support.
> +static int wil_cfg80211_start_ap(struct wiphy *wiphy,
> + struct net_device *ndev,
> + struct cfg80211_ap_settings *info)
> +{
> + int rc = 0;
> + struct wil6210_priv *wil = wiphy_to_wil(wiphy);
> + struct ieee80211_channel *channel = info->channel;
> +
> + if (!channel) {
> + wil_err(wil, "No channel???\n");
> + return -EINVAL;
> + }
> +
> + rc = wmi_set_ssid(wil, info->ssid_len, info->ssid);
> + if (rc)
> + return rc;
> +
> + rc = wmi_set_channel(wil, channel->hw_value);
> + if (rc)
> + return rc;
> +
> + /* TODO: complete implementation */
> +
> + return 0;
Again, this doesn't seem right? How would it work?
> +static int wil_cfg80211_change_beacon(struct wiphy *wiphy,
> + struct net_device *ndev,
> + struct cfg80211_beacon_data *beacon)
> +{
> + return 0;
Really?
> +static int wil_cfg80211_stop_ap(struct wiphy *wiphy,
> + struct net_device *ndev)
> +static int wil_cfg80211_del_station(struct wiphy *wiphy,
> + struct net_device *ndev,
> + u8 *mac)
> +static int wil_cfg80211_change_station(struct wiphy *wiphy,
> + struct net_device *ndev,
> + u8 *mac,
> + struct station_parameters *params)
So obviously don't implement AP mode yet...
> +static int wil_cfg80211_probe_client(struct wiphy *wiphy,
> + struct net_device *dev,
> + const u8 *peer, u64 *cookie)
> +static int wil_cfg80211_set_pmksa(struct wiphy *wiphy,
> + struct net_device *netdev,
> + struct cfg80211_pmksa *pmksa)
> +static int wil_cfg80211_del_pmksa(struct wiphy *wiphy,
> + struct net_device *netdev,
> + struct cfg80211_pmksa *pmksa)
> +static int wil_cfg80211_flush_pmksa(struct wiphy *wiphy,
> + struct net_device *netdev)
Again, don't implement things you don't support.
And never ever just "return 0", pretending that you actually did
something.
> +static int wil_cfg80211_add_key(struct wiphy *wiphy,
> + struct net_device *ndev,
> + u8 key_index, bool pairwise,
> + const u8 *mac_addr,
> + struct key_params *params)
> +{
> + struct wil6210_priv *wil = wiphy_to_wil(wiphy);
> +
> + /* group key is not used */
> + if (!pairwise)
> + return 0;
Return an error then?
> + return wmi_add_cipher_key(wil, key_index, mac_addr,
> + params->key_len, params->key);
> +}
> +
> +static int wil_cfg80211_del_key(struct wiphy *wiphy,
> + struct net_device *ndev,
> + u8 key_index, bool pairwise,
> + const u8 *mac_addr)
> +{
> + struct wil6210_priv *wil = wiphy_to_wil(wiphy);
> +
> + /* group key is not used */
> + if (!pairwise)
> + return 0;
Ditto
> +static int wil_cfg80211_get_key(struct wiphy *wiphy,
> + struct net_device *ndev,
> + u8 key_index, bool pairwise,
> + const u8 *mac_addr, void *cookie,
> + void (*callback) (void *cookie,
> + struct key_params *))
> +{
> + return -ENOENT;
No need to implement this then?
> +static int wil_cfg80211_set_default_key(struct wiphy *wiphy,
> + struct net_device *ndev,
> + u8 key_index, bool unicast,
> + bool multicast)
> +{
> + return 0;
Ok this is possibly the only case where I could think you might need to
implement it and do nothing, although it does seem strange too.
> + wil->fw_code_blob.data = (void * __force)wil->csr + 0x40000;
> + wil->fw_code_blob.size = 0x40000;
> + wil_debugfs_create_ioblob("blob_fw_code", S_IRUGO, dbg,
> + &wil->fw_code_blob);
> +
> + wil->fw_data_blob.data = (void * __force)wil->csr + 0x80000;
> + wil->fw_data_blob.size = 0x8000;
> + wil_debugfs_create_ioblob("blob_fw_data", S_IRUGO, dbg,
> + &wil->fw_data_blob);
This seems a bit strange, why export the blobs that came from the
filesystem to start with (request_firmware)?
> +static int __wil_up(struct wil6210_priv *wil)
> +{
> + /* Apply profile in the following order: */
> + /* SSID and channel for the AP */
> + switch (wdev->iftype) {
> + case NL80211_IFTYPE_AP:
> + case NL80211_IFTYPE_P2P_GO:
> + if (wil->ssid_len == 0) {
> + wil_err(wil, "SSID not set\n");
> + return -EINVAL;
> + }
> + wmi_set_ssid(wil, wil->ssid_len, wil->ssid);
> + if (wil->channel)
> + wmi_set_channel(wil, wil->channel->hw_value);
> + break;
I don't think so? The interface shouldn't come up with any
configuration, that's what start_ap() is used for.
It also seems too early:
> + /* MAC address - pre-requisite for other commands */
> + wmi_set_mac_address(wil, ndev->dev_addr);
> +
> + /* Set up beaconing if required. */
> + rc = wmi_set_bcon(wil, bi, wmi_nettype);
> + if (rc)
> + return rc;
That also should be in start_ap(). Maybe that's the TODO there.
> +static int use_msi = 1;
> +module_param(use_msi, int, S_IRUGO);
> +MODULE_PARM_DESC(use_msi,
> + " Use MSI interrupt: "
> + "0 - don't, 1 - (default) - single, or 3");
"or 3"?
> + rc = wil6210_sysfs_init(wil);
> + if (rc)
> + goto if_remove;
> + /* rollback to sysfs_fini */
Do you really need sysfs in the driver?
> @@ -0,0 +1,134 @@
> +/* attribute: SSID */
> +/* attribute: bf */
> +/* attribute: secure_pcp */
None of this looks like it belongs into sysfs, I think you should remove
sysfs from this entirely.
> + if (rtap_include_phy_info) {
> + rtap_vendor->rtap.rthdr.it_present |= cpu_to_le32(1 <<
> + IEEE80211_RADIOTAP_VENDOR_NAMESPACE);
> + /* OUI for Wilocity 04:ce:14 */
> + rtap_vendor->vendor_oui[0] = 0x04;
> + rtap_vendor->vendor_oui[1] = 0xce;
> + rtap_vendor->vendor_oui[2] = 0x14;
> + rtap_vendor->vendor_ns = 1;
woot, cool! :-)
Finally somebody using vendor namespaces
> +int wmi_tx_eapol(struct wil6210_priv *wil, struct sk_buff *skb)
Why is EAPOL special?
johannes
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
