On Tue, 2016-03-29 at 12:35 +0300, Emmanuel Grumbach wrote:
> + * @rm_nan_func: Remove a nan function. The driver must call
> + * ieee80211_nan_func_terminated() with
> + * NL80211_NAN_FUNC_TERM_REASON_USER_REQUEST reason code upon
> removal.
bad indentation.
Also here: nan -> NAN.
> + /* Only set max_nan_de_entries as available to honor the
> device's
> + * limitations
> + */
> + bitmap_set(sdata->u.nan.func_ids, 1,
> + sdata->local->hw.max_nan_de_entries);
That doesn't make a lot of sense to me. What are you trying to do?
> + inst_id = find_first_bit(sdata->u.nan.func_ids,
> + IEEE80211_MAX_NAN_INSTANCE_ID + 1);
> + if (inst_id == IEEE80211_MAX_NAN_INSTANCE_ID + 1)
> + return -ENOBUFS;
Wouldn't you use max_nan_de_entries here instead of
MAX_NAN_INSTANCE_ID, after validating that the former is actually
smaller than or equal to the latter?
Also, the logic says that the variable should be called
"available_func_ids", although I'd prefer "used_func_ids" after
adjusting the logic according to the comments above.
> + cfg80211_clone_nan_func_members(&func->func, nan_func);
This is quite obviously missing error checking, but see the discussion
in the patch that introduced it.
> + spin_lock_bh(&sdata->u.nan.func_lock);
> + clear_bit(inst_id, sdata->u.nan.func_ids);
> + list_add(&func->list, &sdata->u.nan.functions_list);
> + spin_unlock_bh(&sdata->u.nan.func_lock);
> +
> + ret = drv_add_nan_func(sdata->local, sdata, nan_func);
> + if (ret) {
> + spin_lock_bh(&sdata->u.nan.func_lock);
> + set_bit(inst_id, sdata->u.nan.func_ids);
> + list_del(&func->list);
> + spin_unlock_bh(&sdata->u.nan.func_lock);
> +
> + cfg80211_free_nan_func_members(&func->func);
> + kfree(func);
> + }
> +
> + return ret;
> +}
> +static struct ieee80211_nan_func *
> +ieee80211_find_nan_func(struct ieee80211_sub_if_data *sdata, u8
> instance_id)
> +{
> + struct ieee80211_nan_func *func;
> +
> + lockdep_assert_held(&sdata->u.nan.func_lock);
> +
> + list_for_each_entry(func, &sdata->u.nan.functions_list,
> list) {
> + if (func->func.instance_id == instance_id)
> + return func;
> + }
> +
> + return NULL;
> +}
Arguably though, this whole thing just screams "idr" [1] and then you
don't even need the list_head in the cfg80211 struct.
[1] include/linux/idr.h
> +static struct ieee80211_nan_func *
> +ieee80211_find_nan_func_by_cookie(struct ieee80211_sub_if_data
> *sdata,
> + u64 cookie)
> +{
> + struct ieee80211_nan_func *func;
> +
> + lockdep_assert_held(&sdata->u.nan.func_lock);
> +
> + list_for_each_entry(func, &sdata->u.nan.functions_list,
> list) {
> + if (func->func.cookie == cookie)
> + return func;
> + }
> +
> + return NULL;
> +}
Although this might be more difficult then, but you always
have idr_for_each_entry().
> + case NL80211_IFTYPE_NAN:
> + /* clean all the functions */
> + spin_lock_bh(&sdata->u.nan.func_lock);
> + list_for_each_entry_safe(func, tmp_func,
> + &sdata-
> >u.nan.functions_list, list) {
> + list_del(&func->list);
> + cfg80211_free_nan_func_members(&func->func);
> + kfree(func);
> + }
> + spin_unlock_bh(&sdata->u.nan.func_lock);
> + break;
> case NL80211_IFTYPE_P2P_DEVICE:
> /* relies on synchronize_rcu() below */
> RCU_INIT_POINTER(local->p2p_sdata, NULL);
> /* fall through */
> - case NL80211_IFTYPE_NAN:
any particular reason you're moving the case label?
> default:
> cancel_work_sync(&sdata->work);
> /*
> @@ -1453,9 +1464,15 @@ static void ieee80211_setup_sdata(struct
> ieee80211_sub_if_data *sdata,
> case NL80211_IFTYPE_WDS:
> sdata->vif.bss_conf.bssid = NULL;
> break;
> + case NL80211_IFTYPE_NAN:
> + bitmap_zero(sdata->u.nan.func_ids,
> + IEEE80211_MAX_NAN_INSTANCE_ID + 1);
> + INIT_LIST_HEAD(&sdata->u.nan.functions_list);
> + spin_lock_init(&sdata->u.nan.func_lock);
> + sdata->vif.bss_conf.bssid = sdata->vif.addr;
> + break;
> case NL80211_IFTYPE_AP_VLAN:
> case NL80211_IFTYPE_P2P_DEVICE:
> - case NL80211_IFTYPE_NAN:
also here?
> + if (!local->hw.max_nan_de_entries)
> + local->hw.max_nan_de_entries =
> IEEE80211_MAX_NAN_INSTANCE_ID;
Need a max check also, I guess?
> +/* TODO: record more fields */
...
but isn't cfg80211 recording them anyway?
> +static int ieee80211_reconfig_nan(struct ieee80211_sub_if_data
> *sdata)
> +{
> + struct ieee80211_nan_func *func, *ftmp;
> + LIST_HEAD(tmp_list);
> + int res;
> +
> + res = drv_start_nan(sdata->local, sdata,
> + &sdata->u.nan.nan_conf);
> + if (WARN_ON(res))
> + return res;
> +
> + /* Add all the functions:
> + * This is a little bit ugly. We need to call a potentially
> sleeping
> + * callback for each entry in the list, so we can't hold the
> spinlock.
Nobody forced you to use a spinlock though??
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
