On Mon, Nov 26, 2018 at 03:04:57PM +0530, Sarada Prasanna Garnayak wrote:
> Register callbacks to receive a new association ID for a station
> from the kernel driver using vendor cmd, which may in turn ask it
> from the firmware and that from the hardware. This AID is tied to SID
> and will need to be freed eventually during the dissociation phase.
> diff --git a/src/ap/ieee802_11.c b/src/ap/ieee802_11.c
> @@ -2130,11 +2130,14 @@ static void handle_auth(struct hostapd_data *hapd,
> }
> }
>
> -
> int hostapd_get_aid(struct hostapd_data *hapd, struct sta_info *sta)
Please do not include this type of unrelated whitespace changes (and
hostap.git follows a style where there are two empty lines between
functions, so I would not accept this even as a separate cleanup patch).
> diff --git a/src/drivers/driver.h b/src/drivers/driver.h
> @@ -3651,6 +3651,30 @@ struct wpa_driver_ops {
> + * get_aid - Receive a new association ID for a station
> + * This function is used to receive a new AID from the kernel driver,
> + * which may in turn ask it from the FW, and that from the HW.
> + * This AID is tied to SID and will need to be freed eventually.
> + */
> + int (*get_aid)(void *priv, u16 *aid, const u8 *addr);
> + * free_aid - Release an association ID
> + * This function is used to release an AID back to the kernel driver,
> + * which may release it to the FW, and that to the HW.
> + */
> + int (*free_aid)(void *priv, u16 *aid);
So the part about defining this type of interface for allowing the
driver to select AID sounds reasonable.. Though, that free_aid() case
should really use u16 aid instead of a pointer to aid (the caller should
be responsible for clearing that internal variable, if appropriate).
However, I don't really like the actual implementation of that driver
op:
> diff --git a/src/drivers/driver_nl80211.c b/src/drivers/driver_nl80211.c
> @@ -27,6 +27,7 @@
> +#include "common/intel-ltq-vendor.h"
This would bring in all the problematic src/ap/*.h inclusions into
src/drivers/*, so this is not acceptable unless those parts are removed
from intel-ltq-vendor.h as I commented on an earlier patch.
> @@ -10684,6 +10681,10 @@ const struct wpa_driver_ops wpa_driver_nl80211_ops = {
> +#ifdef CONFIG_DRIVER_NL80211_INTEL_LTQ
> + .get_aid = nl80211_get_aid,
> + .free_aid = nl80211_free_aid,
> +#endif /* CONFIG_DRIVER_NL80211_INTEL_LTQ */
And these two functions are now coming in as static inlines from
src/common/intel-ltq-vendor.h while they should really be non-inline
functions in src/drivers/driver_nl80211.c.
Furthermore, this AID management functionality by the driver sounds like
a generic design option and as such, it would be much more valuable to
define it as an upstream nl80211 functionality. Could you please
consider such implementation for this instead of this vendor specific
command?
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
