Messages in this thread | | | From | Haiyang Zhang <> | Subject | RE: [PATCH] hv_netvsc:Register VF in netvsc_probe if NET_DEVICE_REGISTER missed | Date | Wed, 31 Jan 2024 20:05:39 +0000 |
| |
> -----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
| |