RE: [PATCH] ixgbe: Remove bimodal SR-IOV disabling

[Rose, Gregory V]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
> Sent: Friday, July 10, 2015 2:32 PM
> To: intel-wired-lan@xxxxxxxxxxxxxxxx; Kirsher, Jeffrey T
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Rose, Gregory V
> Subject: [PATCH] ixgbe: Remove bimodal SR-IOV disabling
> 
> When unbinding an SR-IOV device with VFs configured from ixgbe, the driver
> behaves in one of two ways.  If max_vfs was specified, the SR-IOV state is
> disabled, removing the VFs.  The occurs regardless of whether the VF count
> was later modified through sysfs.  If however max_vfs is zero, such as by
> not specifying the module parameter, the VFs persist after the PF is
> unbound from ixgbe.  If the PF is then bound to vfio-pci to be assigned to
> a VM, the PF is non-functional.
> 
> >From the comment, commit da36b64736cf ("ixgbe: Implement PCI SR-IOV
> sysfs callback operation") clearly intended this alternate behavior, but
> probably didn't realize the PF doesn't work in this mode.
> 
> This bimodal behavior is confusing to users and results in a state where
> the PF is broken for other uses unless the user sets sriov_numvfs to zero
> prior to unbinding the device.  Remove this behavior so that VFs are
> removed and the PF is functional for other uses after unbind, regardless
> of the way VFs are enabled.
> 
> Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Cc: Greg Rose <gregory.v.rose@xxxxxxxxx>
> Cc: Jeff Kirsher <jeffrey.t.kirsher@xxxxxxxxx>
> ---
> 
> I can only think that not disabling SR-IOV was meant to enable some sort
> of persistence for VFs, but that's probably better accomplished with
> either udev rules and/or modprobe.d install scripts.
> 
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 5be12a0..de04e3e 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -8810,12 +8810,7 @@ static void ixgbe_remove(struct pci_dev *pdev)
>  		unregister_netdev(netdev);
> 
>  #ifdef CONFIG_PCI_IOV
> -	/*
> -	 * Only disable SR-IOV on unload if the user specified the now
> -	 * deprecated max_vfs module parameter.
> -	 */
> -	if (max_vfs)
> -		ixgbe_disable_sriov(adapter);
> +	ixgbe_disable_sriov(adapter);
>  #endif
>  	ixgbe_clear_interrupt_scheme(adapter);
> 

Please remove max_vfs module parameter - it is deprecated and should be removed from upstream builds.  Dave let us get away with a kernel module a few years ago because the other necessary infrastructure to enable SR-IOV virtual functions via the PCIe interface was not available.  Now that it's there it should be removed and vendors/end users should be forced to move away from this.

Thanks,

- Greg Rose
Intel Corp.
Networking Division