Hi,
During porting the STA management to nl80211 it turned out that it would
be much nicer in the kernel if we would always set all the flags on a
STA that the kernel needs to get from hostapd.
To implement that, it would be good to simply pass the sta_info
structure, e.g. we currently have this code:
if (sta->flags & WLAN_STA_WME)
hostapd_sta_set_flags(hapd, sta->addr, WLAN_STA_WME, ~0);
else
hostapd_sta_set_flags(hapd, sta->addr, 0, ~WLAN_STA_WME);
and that would become
hostapd_sta_set_flags(hapd, sta, WLAN_STA_WME);
or
hostapd_sta_set_flags(hapd, sta->flags, WLAN_STA_WME);
where the third parameter indicates which flag may have changed and the
nl80211 driver would simply use only the flags.
Do you see any issues with that? Are there sometimes flag changes that
we do not want to commit to the kernel?
johannes
How about this then?
---
hostapd/driver.h | 8 ++++----
hostapd/driver_bsd.c | 3 ++-
hostapd/driver_devicescape.c | 6 +++---
hostapd/driver_hostap.c | 4 ++--
hostapd/driver_madwifi.c | 3 ++-
hostapd/driver_prism54.c | 3 ++-
hostapd/ieee802_11.c | 4 ++--
hostapd/ieee802_1x.c | 4 ++--
hostapd/wme.c | 6 ++++--
9 files changed, 23 insertions(+), 18 deletions(-)
--- hostap.orig/hostapd/driver.h 2007-09-28 14:20:48.000000000 +0200
+++ hostap/hostapd/driver.h 2007-09-28 14:21:37.000000000 +0200
@@ -92,7 +92,7 @@ struct wpa_driver_ops {
int (*get_retry)(void *priv, int *short_retry, int *long_retry);
int (*sta_set_flags)(void *priv, const u8 *addr,
- int flags_or, int flags_and);
+ int total_flags, int flags_or, int flags_and);
int (*set_rate_sets)(void *priv, int *supp_rates, int *basic_rates,
int mode);
int (*set_channel_flag)(void *priv, int mode, int chan, int flag,
@@ -427,12 +427,12 @@ hostapd_get_retry(struct hostapd_data *h
static inline int
hostapd_sta_set_flags(struct hostapd_data *hapd, u8 *addr,
- int flags_or, int flags_and)
+ int total_flags, int flags_or, int flags_and)
{
if (hapd->driver == NULL || hapd->driver->sta_set_flags == NULL)
return 0;
- return hapd->driver->sta_set_flags(hapd->drv_priv, addr, flags_or,
- flags_and);
+ return hapd->driver->sta_set_flags(hapd->drv_priv, addr, total_flags,
+ flags_or, flags_and);
}
static inline int
--- hostap.orig/hostapd/driver_bsd.c 2007-09-28 14:21:55.000000000 +0200
+++ hostap/hostapd/driver_bsd.c 2007-09-28 14:22:14.000000000 +0200
@@ -322,7 +322,8 @@ bsd_set_sta_authorized(void *priv, const
}
static int
-bsd_sta_set_flags(void *priv, const u8 *addr, int flags_or, int flags_and)
+bsd_sta_set_flags(void *priv, const u8 *addr, int total_flags, int flags_or,
+ int flags_and)
{
/* For now, only support setting Authorized flag */
if (flags_or & WLAN_STA_AUTHORIZED)
--- hostap.orig/hostapd/driver_devicescape.c 2007-09-28 14:22:25.000000000 +0200
+++ hostap/hostapd/driver_devicescape.c 2007-09-28 14:22:43.000000000 +0200
@@ -75,7 +75,7 @@ struct i802_driver_data {
#define HAPD_DECL struct hostapd_data *hapd = iface->bss[0]
static int i802_sta_set_flags(void *priv, const u8 *addr,
- int flags_or, int flags_and);
+ int total_flags, int flags_or, int flags_and);
static int hostapd_set_iface_flags(struct i802_driver_data *drv, int dev_up)
@@ -764,7 +764,7 @@ static int i802_sta_remove(void *priv, c
struct i802_driver_data *drv = priv;
struct prism2_hostapd_param param;
- i802_sta_set_flags(drv, addr, 0, ~WLAN_STA_AUTHORIZED);
+ i802_sta_set_flags(drv, addr, 0, 0, ~WLAN_STA_AUTHORIZED);
memset(¶m, 0, sizeof(param));
param.cmd = PRISM2_HOSTAPD_REMOVE_STA;
@@ -776,7 +776,7 @@ static int i802_sta_remove(void *priv, c
static int i802_sta_set_flags(void *priv, const u8 *addr,
- int flags_or, int flags_and)
+ int total_flags, int flags_or, int flags_and)
{
struct i802_driver_data *drv = priv;
struct prism2_hostapd_param param;
--- hostap.orig/hostapd/driver_hostap.c 2007-09-28 14:22:54.000000000 +0200
+++ hostap/hostapd/driver_hostap.c 2007-09-28 14:23:15.000000000 +0200
@@ -374,7 +374,7 @@ static int hostap_send_eapol(void *priv,
static int hostap_sta_set_flags(void *priv, const u8 *addr,
- int flags_or, int flags_and)
+ int total_flags, int flags_or, int flags_and)
{
struct hostap_driver_data *drv = priv;
struct prism2_hostapd_param param;
@@ -694,7 +694,7 @@ static int hostap_sta_remove(void *priv,
struct hostap_driver_data *drv = priv;
struct prism2_hostapd_param param;
- hostap_sta_set_flags(drv, addr, 0, ~WLAN_STA_AUTHORIZED);
+ hostap_sta_set_flags(drv, addr, 0, 0, ~WLAN_STA_AUTHORIZED);
memset(¶m, 0, sizeof(param));
param.cmd = PRISM2_HOSTAPD_REMOVE_STA;
--- hostap.orig/hostapd/driver_madwifi.c 2007-09-28 14:23:15.000000000 +0200
+++ hostap/hostapd/driver_madwifi.c 2007-09-28 14:23:31.000000000 +0200
@@ -410,7 +410,8 @@ madwifi_set_sta_authorized(void *priv, c
}
static int
-madwifi_sta_set_flags(void *priv, const u8 *addr, int flags_or, int flags_and)
+madwifi_sta_set_flags(void *priv, const u8 *addr, int total_flags,
+ int flags_or, int flags_and)
{
/* For now, only support setting Authorized flag */
if (flags_or & WLAN_STA_AUTHORIZED)
--- hostap.orig/hostapd/driver_prism54.c 2007-09-28 14:23:35.000000000 +0200
+++ hostap/hostapd/driver_prism54.c 2007-09-28 14:23:49.000000000 +0200
@@ -187,7 +187,8 @@ static int prism54_set_sta_authorized(vo
static int
-prism54_sta_set_flags(void *priv, const u8 *addr, int flags_or, int flags_and)
+prism54_sta_set_flags(void *priv, const u8 *addr, int total_flags,
+ int flags_or, int flags_and)
{
/* For now, only support setting Authorized flag */
if (flags_or & WLAN_STA_AUTHORIZED)
--- hostap.orig/hostapd/ieee802_11.c 2007-09-28 14:23:51.000000000 +0200
+++ hostap/hostapd/ieee802_11.c 2007-09-28 14:24:04.000000000 +0200
@@ -1625,10 +1625,10 @@ static void handle_assoc_cb(struct hosta
ap_sta_bind_vlan(hapd, sta, 0);
}
if (sta->flags & WLAN_STA_SHORT_PREAMBLE) {
- hostapd_sta_set_flags(hapd, sta->addr,
+ hostapd_sta_set_flags(hapd, sta->addr, sta->flags,
WLAN_STA_SHORT_PREAMBLE, ~0);
} else {
- hostapd_sta_set_flags(hapd, sta->addr,
+ hostapd_sta_set_flags(hapd, sta->addr, sta->flags,
0, ~WLAN_STA_SHORT_PREAMBLE);
}
--- hostap.orig/hostapd/ieee802_1x.c 2007-09-28 14:24:04.000000000 +0200
+++ hostap/hostapd/ieee802_1x.c 2007-09-28 14:24:14.000000000 +0200
@@ -94,13 +94,13 @@ void ieee802_1x_set_sta_authorized(struc
if (authorized) {
sta->flags |= WLAN_STA_AUTHORIZED;
- res = hostapd_sta_set_flags(hapd, sta->addr,
+ res = hostapd_sta_set_flags(hapd, sta->addr, sta->flags,
WLAN_STA_AUTHORIZED, ~0);
hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_IEEE8021X,
HOSTAPD_LEVEL_DEBUG, "authorizing port");
} else {
sta->flags &= ~WLAN_STA_AUTHORIZED;
- res = hostapd_sta_set_flags(hapd, sta->addr,
+ res = hostapd_sta_set_flags(hapd, sta->addr, sta->flags,
0, ~WLAN_STA_AUTHORIZED);
hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_IEEE8021X,
HOSTAPD_LEVEL_DEBUG, "unauthorizing port");
--- hostap.orig/hostapd/wme.c 2007-09-28 14:24:16.000000000 +0200
+++ hostap/hostapd/wme.c 2007-09-28 14:24:33.000000000 +0200
@@ -110,9 +110,11 @@ int hostapd_wme_sta_config(struct hostap
{
/* update kernel STA data for WME related items (WLAN_STA_WPA flag) */
if (sta->flags & WLAN_STA_WME)
- hostapd_sta_set_flags(hapd, sta->addr, WLAN_STA_WME, ~0);
+ hostapd_sta_set_flags(hapd, sta->addr, sta->flags,
+ WLAN_STA_WME, ~0);
else
- hostapd_sta_set_flags(hapd, sta->addr, 0, ~WLAN_STA_WME);
+ hostapd_sta_set_flags(hapd, sta->addr, sta->flags,
+ 0, ~WLAN_STA_WME);
return 0;
}
On Thu, Sep 27, 2007 at 06:46:39PM +0200, Johannes Berg wrote:
> During porting the STA management to nl80211 it turned out that it would
> be much nicer in the kernel if we would always set all the flags on a
> STA that the kernel needs to get from hostapd.
That I might be able to support at least partially by merging together
some operations, but still, I would expect there to be valid scenarios
where it would be useful to be able to just toggle a single flag.
> To implement that, it would be good to simply pass the sta_info
> structure, e.g. we currently have this code:
This is not going to happen. struct sta_info (the one in hostapd) is
internal for the core implementation and the driver wrappers should
never touch it directly, i.e., pointer to this is not passed on purpose.
If changes to the operation between the driver wrapper and kernel is
desired, it will need to be done in a different way.
--
Jouni Malinen PGP id EFC895FA
Something like this.
--- hostap.orig/hostapd/driver.h 2007-09-27 18:41:09.000000000 +0200
+++ hostap/hostapd/driver.h 2007-09-27 19:26:52.000000000 +0200
@@ -64,12 +64,12 @@ struct wpa_driver_ops {
size_t elem_len);
int (*read_sta_data)(void *priv, struct hostap_sta_driver_data *data,
- const u8 *addr);
+ const struct sta_info *sta);
int (*send_eapol)(void *priv, const u8 *addr, const u8 *data,
size_t data_len, int encrypt, const u8 *own_addr);
int (*sta_deauth)(void *priv, const u8 *addr, int reason);
int (*sta_disassoc)(void *priv, const u8 *addr, int reason);
- int (*sta_remove)(void *priv, const u8 *addr);
+ int (*sta_remove)(void *priv, const struct sta_info *sta);
int (*get_ssid)(const char *ifname, void *priv, u8 *buf, int len);
int (*set_ssid)(const char *ifname, void *priv, const u8 *buf,
int len);
@@ -80,8 +80,8 @@ struct wpa_driver_ops {
int (*sta_add)(const char *ifname, void *priv, const u8 *addr, u16 aid,
u16 capability, u8 *supp_rates, size_t supp_rates_len,
int flags);
- int (*get_inact_sec)(void *priv, const u8 *addr);
- int (*sta_clear_stats)(void *priv, const u8 *addr);
+ int (*get_inact_sec)(void *priv, const struct sta_info *sta);
+ int (*sta_clear_stats)(void *priv, const struct sta_info *sta);
int (*set_freq)(void *priv, int mode, int freq);
int (*set_rts)(void *priv, int rts);
@@ -91,7 +91,7 @@ struct wpa_driver_ops {
int (*set_retry)(void *priv, int short_retry, int long_retry);
int (*get_retry)(void *priv, int *short_retry, int *long_retry);
- int (*sta_set_flags)(void *priv, const u8 *addr,
+ int (*sta_set_flags)(void *priv, const struct sta_info *sta,
int flags_or, int flags_and);
int (*set_rate_sets)(void *priv, int *supp_rates, int *basic_rates,
int mode);
@@ -142,8 +142,8 @@ struct wpa_driver_ops {
char *ifname, const u8 *addr);
int (*if_remove)(void *priv, enum hostapd_driver_if_type type,
const char *ifname, const u8 *addr);
- int (*set_sta_vlan)(void *priv, const u8 *addr, const char *ifname,
- int vlan_id);
+ int (*set_sta_vlan)(void *priv, const struct sta_info *sta,
+ const char *ifname, int vlan_id);
/**
* commit - Optional commit changes handler
* @priv: driver private data
@@ -263,11 +263,12 @@ hostapd_set_generic_elem(struct hostapd_
static inline int
hostapd_read_sta_data(struct hostapd_data *hapd,
- struct hostap_sta_driver_data *data, const u8 *addr)
+ struct hostap_sta_driver_data *data,
+ const struct sta_info *sta)
{
if (hapd->driver == NULL || hapd->driver->read_sta_data == NULL)
return -1;
- return hapd->driver->read_sta_data(hapd->drv_priv, data, addr);
+ return hapd->driver->read_sta_data(hapd->drv_priv, data, sta);
}
static inline int
@@ -297,11 +298,11 @@ hostapd_sta_disassoc(struct hostapd_data
}
static inline int
-hostapd_sta_remove(struct hostapd_data *hapd, const u8 *addr)
+hostapd_sta_remove(struct hostapd_data *hapd, const struct sta_info *sta)
{
if (hapd->driver == NULL || hapd->driver->sta_remove == NULL)
return 0;
- return hapd->driver->sta_remove(hapd->drv_priv, addr);
+ return hapd->driver->sta_remove(hapd->drv_priv, sta);
}
static inline int
@@ -360,11 +361,11 @@ hostapd_sta_add(const char *ifname, stru
}
static inline int
-hostapd_get_inact_sec(struct hostapd_data *hapd, const u8 *addr)
+hostapd_get_inact_sec(struct hostapd_data *hapd, const struct sta_info *sta)
{
if (hapd->driver == NULL || hapd->driver->get_inact_sec == NULL)
return 0;
- return hapd->driver->get_inact_sec(hapd->drv_priv, addr);
+ return hapd->driver->get_inact_sec(hapd->drv_priv, sta);
}
static inline int
@@ -426,12 +427,12 @@ hostapd_get_retry(struct hostapd_data *h
}
static inline int
-hostapd_sta_set_flags(struct hostapd_data *hapd, u8 *addr,
+hostapd_sta_set_flags(struct hostapd_data *hapd, const struct sta_info *sta,
int flags_or, int flags_and)
{
if (hapd->driver == NULL || hapd->driver->sta_set_flags == NULL)
return 0;
- return hapd->driver->sta_set_flags(hapd->drv_priv, addr, flags_or,
+ return hapd->driver->sta_set_flags(hapd->drv_priv, sta, flags_or,
flags_and);
}
@@ -484,11 +485,11 @@ hostapd_set_ieee80211d(struct hostapd_da
}
static inline int
-hostapd_sta_clear_stats(struct hostapd_data *hapd, const u8 *addr)
+hostapd_sta_clear_stats(struct hostapd_data *hapd, const struct sta_info *sta)
{
if (hapd->driver == NULL || hapd->driver->sta_clear_stats == NULL)
return 0;
- return hapd->driver->sta_clear_stats(hapd->drv_priv, addr);
+ return hapd->driver->sta_clear_stats(hapd->drv_priv, sta);
}
static inline int
@@ -654,11 +655,11 @@ hostapd_get_hw_feature_data(struct hosta
static inline int
hostapd_set_sta_vlan(const char *ifname, struct hostapd_data *hapd,
- const u8 *addr, int vlan_id)
+ const struct sta_info *sta, int vlan_id)
{
if (hapd->driver == NULL || hapd->driver->set_sta_vlan == NULL)
return 0;
- return hapd->driver->set_sta_vlan(hapd->drv_priv, addr, ifname, vlan_id);
+ return hapd->driver->set_sta_vlan(hapd->drv_priv, sta, ifname, vlan_id);
}
static inline int
--- hostap.orig/hostapd/accounting.c 2007-09-27 19:27:31.000000000 +0200
+++ hostap/hostapd/accounting.c 2007-09-27 19:27:43.000000000 +0200
@@ -185,7 +185,7 @@ static int accounting_sta_update_stats(s
struct sta_info *sta,
struct hostap_sta_driver_data *data)
{
- if (hostapd_read_sta_data(hapd, data, sta->addr))
+ if (hostapd_read_sta_data(hapd, data, sta))
return -1;
if (sta->last_rx_bytes > data->rx_bytes)
@@ -237,7 +237,7 @@ void accounting_sta_start(struct hostapd
time(&sta->acct_session_start);
sta->last_rx_bytes = sta->last_tx_bytes = 0;
sta->acct_input_gigawords = sta->acct_output_gigawords = 0;
- hostapd_sta_clear_stats(hapd, sta->addr);
+ hostapd_sta_clear_stats(hapd, sta);
if (!hapd->conf->radius->acct_server)
return;
--- hostap.orig/hostapd/ieee802_1x.c 2007-09-27 19:27:08.000000000 +0200
+++ hostap/hostapd/ieee802_1x.c 2007-09-27 19:27:18.000000000 +0200
@@ -94,13 +94,13 @@ void ieee802_1x_set_sta_authorized(struc
if (authorized) {
sta->flags |= WLAN_STA_AUTHORIZED;
- res = hostapd_sta_set_flags(hapd, sta->addr,
+ res = hostapd_sta_set_flags(hapd, sta,
WLAN_STA_AUTHORIZED, ~0);
hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_IEEE8021X,
HOSTAPD_LEVEL_DEBUG, "authorizing port");
} else {
sta->flags &= ~WLAN_STA_AUTHORIZED;
- res = hostapd_sta_set_flags(hapd, sta->addr,
+ res = hostapd_sta_set_flags(hapd, sta,
0, ~WLAN_STA_AUTHORIZED);
hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_IEEE8021X,
HOSTAPD_LEVEL_DEBUG, "unauthorizing port");
--- hostap.orig/hostapd/sta_info.c 2007-09-27 19:27:50.000000000 +0200
+++ hostap/hostapd/sta_info.c 2007-09-27 19:28:16.000000000 +0200
@@ -117,7 +117,7 @@ void ap_free_sta(struct hostapd_data *ha
if (!ap_sta_in_other_bss(hapd, sta, WLAN_STA_ASSOC) &&
!(sta->flags & WLAN_STA_PREAUTH))
- hostapd_sta_remove(hapd, sta->addr);
+ hostapd_sta_remove(hapd, sta);
ap_sta_hash_del(hapd, sta);
ap_sta_list_del(hapd, sta);
@@ -209,7 +209,7 @@ void ap_handle_timer(void *eloop_ctx, vo
HOSTAPD_DEBUG(HOSTAPD_DEBUG_MINIMAL,
"Checking STA " MACSTR " inactivity:\n",
MAC2STR(sta->addr));
- inactive_sec = hostapd_get_inact_sec(hapd, sta->addr);
+ inactive_sec = hostapd_get_inact_sec(hapd, sta);
if (inactive_sec == -1) {
printf(" Could not get station info from kernel "
"driver for " MACSTR ".\n",
@@ -408,7 +408,7 @@ static int ap_sta_remove(struct hostapd_
HOSTAPD_DEBUG(HOSTAPD_DEBUG_MINIMAL, "Removing STA " MACSTR
" from kernel driver\n", MAC2STR(sta->addr));
- if (hostapd_sta_remove(hapd, sta->addr) &&
+ if (hostapd_sta_remove(hapd, sta) &&
sta->flags & WLAN_STA_ASSOC) {
printf("Could not remove station " MACSTR " from kernel "
"driver.\n", MAC2STR(sta->addr));
@@ -580,5 +580,5 @@ int ap_sta_bind_vlan(struct hostapd_data
if (wpa_auth_sta_set_vlan(sta->wpa_sm, sta->vlan_id) < 0)
wpa_printf(MSG_INFO, "Failed to update VLAN-ID for WPA");
- return hostapd_set_sta_vlan(iface, hapd, sta->addr, sta->vlan_id);
+ return hostapd_set_sta_vlan(iface, hapd, sta, sta->vlan_id);
}
--- hostap.orig/hostapd/wme.c 2007-09-27 19:28:23.000000000 +0200
+++ hostap/hostapd/wme.c 2007-09-27 19:28:28.000000000 +0200
@@ -110,9 +110,9 @@ int hostapd_wme_sta_config(struct hostap
{
/* update kernel STA data for WME related items (WLAN_STA_WPA flag) */
if (sta->flags & WLAN_STA_WME)
- hostapd_sta_set_flags(hapd, sta->addr, WLAN_STA_WME, ~0);
+ hostapd_sta_set_flags(hapd, sta, WLAN_STA_WME, ~0);
else
- hostapd_sta_set_flags(hapd, sta->addr, 0, ~WLAN_STA_WME);
+ hostapd_sta_set_flags(hapd, sta, 0, ~WLAN_STA_WME);
return 0;
}
On Thu, 2007-09-27 at 18:16 -0700, Jouni Malinen wrote:
> > During porting the STA management to nl80211 it turned out that it would
> > be much nicer in the kernel if we would always set all the flags on a
> > STA that the kernel needs to get from hostapd.
>
> That I might be able to support at least partially by merging together
> some operations, but still, I would expect there to be valid scenarios
> where it would be useful to be able to just toggle a single flag.
Are there really? I can only see three flags in the kernel being set by
hostapd, those being "authorized", "wme capable" and "shortslot
capable". Some additional flags for the userspace MLME maybe, I need to
look into that later.
Hence, I don't see any place where we'd ever need to set a single flag,
we need to keep track of all those flags anyway.
It is certainly possible to have selective flags updating but I don't
quite see the point.
> This is not going to happen. struct sta_info (the one in hostapd) is
> internal for the core implementation and the driver wrappers should
> never touch it directly, i.e., pointer to this is not passed on purpose.
> If changes to the operation between the driver wrapper and kernel is
> desired, it will need to be done in a different way.
Ok. I'm still thinking about the best way to do VLAN/STA finding but I
may just do that by the AP interface VLANs are tied to. Then the only
thing I really need changed is the above flags change.
johannes