Re: [PATCH] wifi: mac80211: restore monitor injection when coexisting with another VIF

[Lachlan Hodges]

Hi,

I will leave implementation discussion to Johannes, but I have some
generic feedback;

> + * Context: Must be called with wiphy->mtx held.
> + *          Always process context - GFP_KERNEL is safe and appropriate.

This comment seems redundant given the lockdep assert?

> + */
> +void ieee80211_update_sole_chandef(struct ieee80211_local *local)
> +{
> +	struct ieee80211_chanctx      *ctx, *found = NULL;
> +	struct ieee80211_sole_chandef *snap = NULL;
> +	struct ieee80211_sole_chandef *old;

Don't align the local names i.e

	struct ieee80211_chanctx *ctx, *found = NULL;
	struct ieee80211_sole_chandef *snap = NULL;
	...

> +	if (found) {
> +		snap = kmalloc(sizeof(*snap), GFP_KERNEL);
> +		if (snap)
> +			snap->def = found->conf.def;
> +		/* alloc failure -> snap == NULL -> publish NULL below */

Same here - this comment adds no value

>  struct ieee80211_chanctx_user_iter {
>  	struct ieee80211_chan_req *chanreq;
>  	struct ieee80211_sub_if_data *sdata;
> @@ -729,6 +784,9 @@ static void ieee80211_change_chanctx(struct ieee80211_local *local,
>  				     const struct ieee80211_chan_req *chanreq)
>  {
>  	_ieee80211_change_chanctx(local, ctx, old_ctx, chanreq, NULL);
> +
> +	/* Hook 4/4: channel parameters changed; refresh snapshot */
> +	ieee80211_update_sole_chandef(local);

Without the context of this patch, there is no way to understand
what this is doing (same with 3/4). The comment doesn't help and
the general idea of a "sole chandef" seems strange. hw.conf.chandef
is also a sole chandef?

> +/* Defined in chan.c */
> +void ieee80211_update_sole_chandef(struct ieee80211_local *local);

Another comment not needed
 
> +	/*
> +	 * All interfaces are gone by this point, so every chanctx has been
> +	 * freed and ieee80211_update_sole_chandef() has already published
> +	 * NULL. Assert the invariant.
> +	 */
> +	WARN_ON_ONCE(rcu_access_pointer(local->sole_chandef));

Seems unnecessary?
 
> -	if (chanctx_conf)
> +	if (chanctx_conf) {
>  		chandef = &chanctx_conf->def;
> -	else
> -		goto fail_rcu;
> +	} else {
> +		/*
> +		 * Real-chanctx drivers (e.g. mt76) do not assign a chanctx to
> +		 * the monitor VIF, so vif.bss_conf.chanctx_conf is NULL here.
> +		 * Fall back to the sole_chandef snapshot maintained by
> +		 * ieee80211_update_sole_chandef(). NULL means MCC or no active
> +		 * channel - drop the frame.
> +		 *
> +		 * The snapshot is valid for this whole function: it is freed
> +		 * via kfree_rcu() after a full grace period, and we are inside
> +		 * rcu_read_lock() throughout.
> +		 */

is the bottom half of this comment really needed?

> +		struct ieee80211_sole_chandef *sole =
> +			rcu_dereference(local->sole_chandef);
> +		chandef = sole ? &sole->def : NULL;

nit: space after local declaration

> +		list_for_each_entry_rcu(tmp_sdata, &local->interfaces, list) {
> +			struct ieee80211_chanctx_conf *tx_conf;
> +
> +			if (!ieee80211_sdata_running(tmp_sdata))
> +				continue;
> +			if (tmp_sdata->vif.type == NL80211_IFTYPE_MONITOR ||
> +			    tmp_sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
> +				continue;
> +
> +			tx_conf = rcu_dereference(tmp_sdata->vif.bss_conf.chanctx_conf);
> +

nit: remove this space after tx_conf = ..

lachlan