Re: [PATCH 2/3] mac80211: filter multicast data packets on AP / AP_VLAN

[Johannes Berg <johannes@sipsolutions.net>]

Please also add "v3" to the subject line, e.g. using --subject-prefix
'PATCH v3' when using git send-email.

> +static void add_vlan_files(struct ieee80211_sub_if_data *sdata)
> +{
> +	// add num_mcast_sta_vlan using name num_mcast_sta

Please don't use // style comments.

> +static inline void
> +ieee80211_vif_inc_num_mcast(struct ieee80211_sub_if_data *sdata)
> +{
> +	if (sdata->vif.type != NL80211_IFTYPE_AP &&
> +	    sdata->vif.type != NL80211_IFTYPE_AP_VLAN)
> +		return;

That's pointless, given this:

> +	if (sdata->vif.type == NL80211_IFTYPE_AP)
> +		atomic_inc(&sdata->u.ap.num_mcast_sta);
> +	else if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
> +		atomic_inc(&sdata->u.vlan.num_mcast_sta);
> +}
> +
> +static inline void
> +ieee80211_vif_dec_num_mcast(struct ieee80211_sub_if_data *sdata)
> +{
> +	if (sdata->vif.type != NL80211_IFTYPE_AP &&
> +	    sdata->vif.type != NL80211_IFTYPE_AP_VLAN)
> +		return;
> +
> +	if (sdata->vif.type == NL80211_IFTYPE_AP)
> +		atomic_dec(&sdata->u.ap.num_mcast_sta);
> +	else if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
> +		atomic_dec(&sdata->u.vlan.num_mcast_sta);

Same here.

> +}
> +
> +/* This function returns the number of multicast stations connected
> to this
> + * interface. It returns -1 if that number is not tracked, that is
> for netdevs
> + * not in AP or AP_VLAN mode or when using 4addr. */
> +static inline int
> +ieee80211_vif_get_num_mcast_if(struct ieee80211_sub_if_data *sdata)
> +{
> +	if (sdata->vif.type == NL80211_IFTYPE_AP)
> +		return atomic_read(&sdata->u.ap.num_mcast_sta);
> +	else if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN &&
> +		 !sdata->u.vlan.sta)
> +		return atomic_read(&sdata->u.vlan.num_mcast_sta);
> +	else
> +		return -1;

All the "else" branches are useless since you return immediately inside
each if.

> -       } else if (unlikely(tx->sdata->vif.type == NL80211_IFTYPE_AP &&
> -                           ieee80211_is_data(hdr->frame_control) &&
> -                           !atomic_read(&tx->sdata->u.ap.num_mcast_sta))) {
> +       } else if (unlikely(ieee80211_vif_get_num_mcast_if(tx->sdata) == 0 &&
> +                           ieee80211_is_data(hdr->frame_control))) {

any particular reason to invert the order of the checks? seems checking
for data first should be faster/cheaper from the cache, than accessing
the counters?

johannes