Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1cdf35f95aca2a65d0d738544fb04079125b9581.camel@gmail.com>
Subject: Re: [PATCH v2 2/2] mac80211: Support POWERED_ADDR_CHANGE feature
From:   James Prestwood <prestwoj@gmail.com>
To:     Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless@vger.kernel.org
Date:   Thu, 25 Aug 2022 09:13:56 -0700
In-Reply-To: <e898282f0766bdf0bd9cbb92d2a3551ff28cdf6b.camel@sipsolutions.net>
References: <20220811231338.563794-1-prestwoj@gmail.com>
         <20220811231338.563794-3-prestwoj@gmail.com>
         <e898282f0766bdf0bd9cbb92d2a3551ff28cdf6b.camel@sipsolutions.net>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.40.4 (3.40.4-5.fc34) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Johannes,

On Thu, 2022-08-25 at 11:02 +0200, Johannes Berg wrote:
> On Thu, 2022-08-11 at 16:13 -0700, James Prestwood wrote:
> > @@ -217,7 +275,11 @@ static int ieee80211_change_mac(struct
> > net_device *dev, void *addr)
> >         if (ret)
> >                 return ret;
> >  
> > +       if (live)
> > +               drv_remove_interface(local, sdata);
> >         ret = eth_mac_addr(dev, sa);
> > +       if (live)
> > +               ret = drv_add_interface(local, sdata);
> >  
> >         if (ret == 0)
> >                 memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN);
> > 
> 
> I still don't like the (lack of) error checking here. As far as I know,
> eth_mac_addr() can very happily fail if the passed address is invalid,
> so we really shouldn't overwrite the ret value by drv_add_interface().

Ah yes, that was an oversight. I assume we do want to add_interface
even if eth_mac_addr fails though. So my only question is about the
return from drv_add_interface(). Is this unlikely to fail? If so would
just a WARN_ON be sufficient and return the value from eth_mac_addr()?

So something like:

	if (live)
		drv_remove_interface(local, sdata);
	ret = eth_mac_addr(dev, sa);
	if (ret == 0)
		memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN);

	if (live)
		WARN_ON(drv_add_interface(local, sdata));

	return ret;

> 
> Also, it seems like we should only add the interface again after
> updating sdata->vif.addr (last context line), so that the driver
> actually knows ... otherwise I'm not sure how this patch would have
> much
> effect (unless it updates the FW all the time like iwlwifi, which I
> guess is where you tested it, based on the rationale...)

Yes, this is where I tested it. But yeah makes sense, I'll move copying
the address to before add_interface.

Thanks,
James

> 
> johannes