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

Lachlan Hodges
2026-04-25T01:47:33Z

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