Re: [PATCH wireless-next 14/35] wifi: mm81x: add mac.c

[Johannes Berg]

On Fri, 2026-02-27 at 15:10 +1100, Lachlan Hodges wrote:
> 
> +static int mm81x_mac_ops_hw_scan(struct ieee80211_hw *hw,
> +				 struct ieee80211_vif *vif,
> +				 struct ieee80211_scan_request *hw_req)
> +{
> +	int ret = 0;
> +	struct mm81x *mm = hw->priv;
> +	struct cfg80211_scan_request *req = &hw_req->req;
> +	struct mm81x_hw_scan_params *params;
> +	struct ieee80211_channel **chans = hw_req->req.channels;
> +

> +	mutex_lock(&mm->lock);

Seeing this, I wonder about two things:

 1) Do you even need a mutex, given that the wiphy mutex covers all of
    this pretty much? I can say from experience that a _lot_ of things
    get quite significantly simpler without a separate driver mutex.

 2) Are you going to incur the wrath of mm/ folks, where instances of
    'struct mm_struct' are commonly called 'mm'? I can find a few
    examples of others (struct drm_buddy *mm, struct mqd_manager *mm),
    but you'd double the instances.

> +	UNUSED(hw);
> +	UNUSED(ctx);

I think you should remove these (and the macro.)

> +	/*
> +	 * mm81x only support changing/setting the channel
> +	 * when we create an interface.
> +	 */
> +	if (WARN_ON(changed & IEEE80211_CHANCTX_CHANGE_CHANNEL))
> +		mm81x_err(mm, "Changing channel via chanctx not supported");

Wait, what, why do you have chanctx support then? This seems highly
questionable, how do you not run into this all the time?

If it just has a single, wouldn't the chanctx emulation suit the driver
better, and that'd make this more obvious? Hmm, but you _do_ support
multiple vifs? I'm confused.

> +static int mm81x_mac_ops_sta_state(struct ieee80211_hw *hw,
> +				   struct ieee80211_vif *vif,
> +				   struct ieee80211_sta *sta,
> +				   enum ieee80211_sta_state old_state,
> +				   enum ieee80211_sta_state new_state)
> +{
> +	u16 aid;
> +	int ret = 0;

nit: that =0 assignment is unused. I (we?) tend to not add them so the
compiler can warn if the remaining code changes.

> +		WARN_ON((key->flags & IEEE80211_KEY_FLAG_PAIRWISE));

nit: extra parentheses

> + * The firmware passes up NULL vifs for broadcast management frames. Find
> + * the first interface that best fits the frame we are rx'ing. This
> + * has the clear downside if we have two vifs with the same interface type
> + * the 2nd vif will never be targeted. For now, this will have to do.

Why do you need this? Curious, because mac80211 ought to sort out the
right vif (or even send it to multiple) anyway?

The only user _appears_ to be mm81x_rx_h_update_sta() which seems you
could just skip entirely for broadcast mgmt frames, since it's just
statistics?
Or look up the STA not the VIF (ieee80211_find_sta_by_ifaddr() can take
a NULL ifaddr)?

Anyway, not really important.

> +	ieee80211_rx_irqsafe(hw, skb);

This seems a bit pointless, you're coming from a worker already, so why
jump through a tasklet again? Seems ieee80211_rx() would do, unless you
have some assumptions on how fast the work must process? (but then you
should probably document those.)

(I'm not going to look in this much detail at the other stuff, this just
because of the mac80211 interface.)

johannes