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
