-----Original Message----- From: Dexuan Cui decui@microsoft.com Sent: Wednesday, January 31, 2024 12:40 PM To: Haiyang Zhang haiyangz@microsoft.com; Shradha Gupta shradhagupta@linux.microsoft.com Cc: KY Srinivasan kys@microsoft.com; Wei Liu wei.liu@kernel.org; David S. Miller davem@davemloft.net; Eric Dumazet edumazet@google.com; Jakub Kicinski kuba@kernel.org; Paolo Abeni pabeni@redhat.com; Wojciech Drewek wojciech.drewek@intel.com; linux- hyperv@vger.kernel.org; netdev@vger.kernel.org; linux- kernel@vger.kernel.org; Shradha Gupta shradhagupta@microsoft.com; stable@vger.kernel.org Subject: RE: [PATCH] hv_netvsc:Register VF in netvsc_probe if NET_DEVICE_REGISTER missed
From: Haiyang Zhang haiyangz@microsoft.com Sent: Wednesday, January 31, 2024 8:46 AM [...]
From: Shradha Gupta shradhagupta@linux.microsoft.com Sent: Wednesday, January 31, 2024 2:54 AM
[...]
netvsc_prepare_bonding(vf_netdev);netvsc_register_vf(vf_netdev, VF_REG_IN_PROBE);__netvsc_vf_setup(net, vf_netdev);add a "break;' ?
With MANA devices and multiport support there, the individual ports
are
also net_devices. Wouldn't this be needed for such scenario(where we have multiple mana port net devices) to register them all?
Each device has separate probe() call, so only one VF will match in one netvsc_probe().
netvsc_prepare_bonding() & netvsc_register_vf() have get_netvsc_byslot(vf_netdev), but __netvsc_vf_setup() doesn't have. So, in case of multi-Vfs, this code will run "this" netvsc NIC with
multiple VFs by
__netvsc_vf_setup() which isn't correct.
You need to add the following lines before netvsc_prepare_bonding(vf_netdev) in netvsc_probe() to skip non-matching VFs:
if (net != get_netvsc_byslot(vf_netdev)) continue;
Haiyang is correct. I think it's still good to add a "break;", e.g. my understanding is something like the below (this is untested):
+static struct net_device *get_matching_netvsc_dev(net_device *event_ndev) +{
/* Skip NetVSC interfaces */if (event_ndev->netdev_ops == &device_ops)return NULL;/* Avoid non-Ethernet type devices */if (event_ndev->type != ARPHRD_ETHER)return NULL;/* Avoid Vlan dev with same MAC registering as VF */if (is_vlan_dev(event_ndev))return NULL;/* Avoid Bonding master dev with same MAC registering as VF */if (netif_is_bond_master(event_ndev))return NULL;return get_netvsc_byslot(event_ndev);+}
Looks good. But, like you said before, the four if's can be moved into a new function, and shared by two callers: netvsc_probe() & netvsc_netdev_event().
- for_each_netdev(dev_net(net), vf_netdev) {
if (get_matching_netvsc_dev(event_dev) != net)continue;netvsc_prepare_bonding(vf_netdev);netvsc_register_vf(vf_netdev, VF_REG_IN_PROBE);__netvsc_vf_setup(net, vf_netdev);break;- }
We can also use get_matching_netvsc_dev() in netvsc_netdev_event().
netvsc_netdev_event() >> netvsc_unregister_vf() uses get_netvsc_byref(vf_netdev) instead of get_netvsc_byslot(). So probably just re-factoring the four if's is better.
Thanks, -Haiyang