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

[Lachlan Hodges]

On Fri, Mar 06, 2026 at 10:04:53AM +0100, Johannes Berg wrote:
> 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.

Gave this a look and you are right, wiphy mutex covers pretty much
everything so will just remove.

>  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.

This.. is definitely something I did not think of. I have no issue with
renaming to something else.. maybe mx? I'm not sure.

> > +	/*
> > +	 * 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.

We originally used chanctx emulation.. but I suppose in an effort to
be "modern" we use chanctx. It's probably best to switch back to the
chanctx emulation anyway. As for why we don't run into this is due
to no channel switch support yet, iirc mac80211 I think needs a minor
tweak to work with S1G (which further reinforces the idea that we
should just emulate chanctx)

--

Thanks for the review. On the other thread [1] you mentioned sending a
pull request once reviews settle down, as per the documentation in [2]
(which I should have read earlier... :) ), can we confirm that this means
we are to submit subsequent patchset revisions in the same per-file
format until everyone is happy with the driver, and then raise the PR?

[1] https://lore.kernel.org/linux-wireless/b71d0932b10b5c446681cef588cfcf6f869f3fca.camel@xxxxxxxxxxxxxxxx/
[2] https://wireless.docs.kernel.org/en/latest/en/developers/documentation/submittingpatches.html#new-driver

lachlan