Re: [PATCH] hv_netvsc:Register VF in netvsc_probe if NET_DEVICE_REGISTER missed

[Shradha Gupta]

On Tue, Jan 30, 2024 at 08:13:21PM +0000, Dexuan Cui wrote:
> > From: Shradha Gupta <shradhagupta@xxxxxxxxxxxxxxxxxxx>
> > Sent: Monday, January 29, 2024 11:19 PM
> >  [...]
> > If hv_netvsc driver is removed and reloaded, the NET_DEVICE_REGISTER
> 
> s/removed/unloaded/
> unloaded looks more accurate to me :-)
> 
> > [...]
> > Tested-on: Ubuntu22
> > Testcases: LISA testsuites
> > 	   verify_reload_hyperv_modules, perf_tcp_ntttcp_sriov
> IMO the 3 lines can be removed: this bug is not specific to Ubuntu, and the
> test case names don't provide extra value to help understand the issue
> here and they might cause more questions unnecessarily, e.g. what's LISA,
> what exactly do the test cases do.
> 
> > +/* Macros to define the context of vf registration */
> > +#define VF_REG_IN_PROBE		1
> > +#define VF_REG_IN_RECV_CBACK	2
> 
> I think VF_REG_IN_NOTIFIER is a better name?
> RECV_CBALL looks inaccurate to me.
> 
> > @@ -2205,8 +2209,11 @@ static int netvsc_vf_join(struct net_device
> > *vf_netdev,
> >  			   ndev->name, ret);
> >  		goto upper_link_failed;
> >  	}
> > -
> > -	schedule_delayed_work(&ndev_ctx->vf_takeover,
> > VF_TAKEOVER_INT);
> > +	/* If this registration is called from probe context vf_takeover
> > +	 * is taken care of later in probe itself.
> I suspect "later in probe itself" is not accurate.
> If 'context' is VF_REG_IN_PROBE, I suppose what happens here is:
> after netvsc_probe() finishes, the netvsc interface becomes available,
> so the user space will ifup it, and netvsc_open() will UP the VF interface,
> and netvsc_netdev_event() is called for the VF with event ==
> NETDEV_POST_INIT (?) and NETDEV_CHANGE, and the data path is
> switched to the VF.
> 
> If my understanding is correct, I think in the case of 'context' ==
> VF_REG_IN_PROBE, I suspect the "Align MTU of VF with master"
> and the "sync address list from ndev to VF" in __netvsc_vf_setup() are
> omitted? If so, should this be fixed? e.g. Not sure if the below is an issue or not:
> 1) a VF is bound to a NetVSC interface, and a user sets the MTUs to 1024.
> 2) rmmod hv_netvsc
> 3) modprobe hv_netvsc
> 4) the netvsc interface uses MTU=1500 (the default), and the VF still uses 1024.
> 
> > @@ -2597,6 +2604,34 @@ static int netvsc_probe(struct hv_device *dev,
> >  	}
> > 
> >  	list_add(&net_device_ctx->list, &netvsc_dev_list);
> > +
> > +	/* When the hv_netvsc driver is removed and readded, the
> 
> s/removed and readded/unloaded and reloaded/
> 
> > +	 * NET_DEVICE_REGISTER for the vf device is replayed before
> > probe
> > +	 * is complete. This is because register_netdevice_notifier() gets
> > +	 * registered before vmbus_driver_register() so that callback func
> > +	 * is set before probe and we don't miss events like
> > NETDEV_POST_INIT
> > +	 * So, in this section we try to register each matching
> 
> Looks like there are spaces at the end of the line. We can move up a few words
> from the next line :-)
> 
> s/each matching/the matching/
> A NetVSC interface has only 1 matching VF device.
> 
> > +	 * vf device that is present as a netdevice, knowing that it's register
> 
> s/it's/its/
> 
> > +	 * call is not processed in the netvsc_netdev_notifier(as probing is
> > +	 * progress and get_netvsc_byslot fails).
> > +	 */
> > +	for_each_netdev(dev_net(net), vf_netdev) {
> > +		if (vf_netdev->netdev_ops == &device_ops)
> > +			continue;
> > +
> > +		if (vf_netdev->type != ARPHRD_ETHER)
> > +			continue;
> > +
> > +		if (is_vlan_dev(vf_netdev))
> > +			continue;
> > +
> > +		if (netif_is_bond_master(vf_netdev))
> > +			continue;
> 
> The code above is duplicated from netvsc_netdev_event().
> Can we add a new helper function is_matching_vf() to avoid the duplication?
Sure, I will do that. Thanks
> 
> > +		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?
> 
> > +	}
> >  	rtnl_unlock();