| Message ID | 20241129-potin-revert-ncsi-set-mac-addr-v1-1-94ea2cb596af@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Revert "net/ncsi: change from ndo_set_mac_address to dev_set_mac_address" | expand |
On Fri, Nov 29, 2024 at 05:12:56PM +0800, Potin Lai wrote: > From: Potin Lai <potin.lai@quantatw.com> > > This reverts commit 790071347a0a1a89e618eedcd51c687ea783aeb3. > > We are seeing kernel panic when enabling two NCSI interfaces at same > time. It looks like mutex lock is being used in softirq caused the > issue. So a revert does make sense, you are seeing a real problem from that commit. However with the revert, is the code actually correct? Or is it missing some locking? Normally dev_addr_sem is used to protect against two calls to change the MAC address at once. Is this protection needed? It would also be typical to hold RTNL while changing the MAC address. So it would be nice to see an analysis of the locking, and maybe the revert commit message says this gets you from a broken state to a less broken state, and the real fix will be submitted soon? Andrew
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c index e28be33bdf2c..0cd7b916d3f8 100644 --- a/net/ncsi/ncsi-rsp.c +++ b/net/ncsi/ncsi-rsp.c @@ -629,6 +629,7 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id) { struct ncsi_dev_priv *ndp = nr->ndp; struct net_device *ndev = ndp->ndev.dev; + const struct net_device_ops *ops = ndev->netdev_ops; struct ncsi_rsp_oem_pkt *rsp; struct sockaddr saddr; u32 mac_addr_off = 0; @@ -655,9 +656,7 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id) /* Set the flag for GMA command which should only be called once */ ndp->gma_flag = 1; - rtnl_lock(); - ret = dev_set_mac_address(ndev, &saddr, NULL); - rtnl_unlock(); + ret = ops->ndo_set_mac_address(ndev, &saddr); if (ret < 0) netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");