Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <90ae00044bc0834d87d3f9fb75ce63dce4cfadd5.camel@gmail.com>
Subject: Re: [PATCH 2/2] mac80211: Support LIVE_ADDRESS_CHANGE feature
From:   James Prestwood <prestwoj@gmail.com>
To:     Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless@vger.kernel.org
Date:   Fri, 04 Oct 2019 09:25:37 -0700
In-Reply-To: <c6835b5c5d2a97fa82b0fb21f7b7f0056aa42e1b.camel@sipsolutions.net>
References: <20190913195908.7871-1-prestwoj@gmail.com>
         <20190913195908.7871-2-prestwoj@gmail.com>
         (sfid-20190913_220113_985031_7C3A66BD) <c6835b5c5d2a97fa82b0fb21f7b7f0056aa42e1b.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Fri, 2019-10-04 at 13:56 +0200, Johannes Berg wrote:
> Hi,
> 
> I was tempted to apply this (sans the feature advertisement part that
> I
> don't think should be in nl80211), but:
> 
> > 
> > Signed-off-by: James Prestwood <prestwoj@gmail.com>
> 
> Please add a commit log.
> 
> > +static int ieee80211_can_live_addr_change(struct
> > ieee80211_sub_if_data *sdata)
> > +{
> > +	if (netif_carrier_ok(sdata->dev))
> > +		return -EBUSY;
> > +
> > +	switch (sdata->vif.type) {
> > +	case NL80211_IFTYPE_AP:
> > +	case NL80211_IFTYPE_P2P_GO:
> > +	case NL80211_IFTYPE_AP_VLAN:
> > +	case NL80211_IFTYPE_WDS:
> > +	case NL80211_IFTYPE_MESH_POINT:
> > +	case NL80211_IFTYPE_MONITOR:
> > +	case NL80211_IFTYPE_OCB:
> > +		/* No further checking required, when started or UP
> > these
> > +		 * interface types set carrier
> > +		 */
> > +		break;
> > +	case NL80211_IFTYPE_ADHOC:
> > +		if (sdata->u.ibss.ssid_len != 0)
> > +			return -EBUSY;
> 
> Can you please document why this is there? Maybe all of the
> conditions,
> for that matter.
> 
> I'm not even entirely sure it _is_ needed - if we've still not
> created
> the IBSS but are scanning for it or trying to merge the MAC address
> won't really matter yet? Probably?

I guess its just paranoia, rather be safe than sorry. I can take this
out, but is "Probably?" a good reason? ;)

> 
> > +		break;
> > +	case NL80211_IFTYPE_STATION:
> > +	case NL80211_IFTYPE_P2P_CLIENT:
> > +		if (!list_empty(&sdata->local->roc_list) ||
> > +					!sdata->local->scanning)
> > +			return -EBUSY;
> 
> AP, mesh and other interfaces *can* scan, so that test should be
> pulled
> out to be generic - but then in fact all of them should probably be
> generic - ROC maybe can't be done on other interfaces yet, but unless
> you're going to check *which* interface is actually doing the ROC,
> you
> should just make that a generic check that applies to all interfaces.

Ok so no switch statement, simply just check that we aren't offchannel
or scanning. I guess this would then cover the IBSS case too.

> 
> If you do care about this being more granular then you should check
> *which* interface is scanning, and then you can still switch the MAC
> address for *other* interfaces - but I'd still argue it should be
> independent of interface type.

I think maybe in the future we might want this, but for now lets not
worry about it. But just to make sure we are on the same page, your
talking about e.g. hardware with multiple radios so you could be doing
offchannel work/scanning/connecting simultaneously without having to
wait for the current operation to complete?

> 
> And, I'm confused, but isn't the polarity of the scanning check
> wrong?

Ah yeah, after you pointed that out I realized 'scanning' is a bit
field. I should be doing:

test_bit(SCAN_HW_SCANNING, &sdata->local->scanning)

Feel free the merge this, but I haven t had a chance yet to look into
adding a flag to RTNL (based on what you said in your previous email).
Without some way of telling userspace this is supported, its kinda
useless IMO.

Either way I'll send another patch with these things addressed.

Thanks,
James

> 
> johannes
>