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: <e898282f0766bdf0bd9cbb92d2a3551ff28cdf6b.camel@sipsolutions.net>
Subject: Re: [PATCH v2 2/2] mac80211: Support POWERED_ADDR_CHANGE feature
From:   Johannes Berg <johannes@sipsolutions.net>
To:     James Prestwood <prestwoj@gmail.com>,
        linux-wireless@vger.kernel.org
Date:   Thu, 25 Aug 2022 11:02:26 +0200
In-Reply-To: <20220811231338.563794-3-prestwoj@gmail.com>
References: <20220811231338.563794-1-prestwoj@gmail.com>
         <20220811231338.563794-3-prestwoj@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2022-08-11 at 16:13 -0700, James Prestwood wrote:
> @@ -217,7 +275,11 @@ static int ieee80211_change_mac(struct net_device *d=
ev, void *addr)
>  	if (ret)
>  		return ret;
> =20
> +	if (live)
> +		drv_remove_interface(local, sdata);
>  	ret =3D eth_mac_addr(dev, sa);
> +	if (live)
> +		ret =3D drv_add_interface(local, sdata);
> =20
>  	if (ret =3D=3D 0)
>  		memcpy(sdata->vif.addr, sa->sa_data, ETH_ALEN);
>=20

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

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

johannes