Re: [PATCH 0/1] net/mlx5: Call pci_disable_sriov() on remove

[Saeed Mahameed <saeedm@mellanox.com>]

On Thu, 2020-04-30 at 14:03 +0200, Niklas Schnelle wrote:
> Hello,
> 
> I'm currently working on improvements in PF-VF handling on s390. One
> thing that
> may be a bit special for us is that the s390 hotplug code supports
> shutting
> down a single PF of a multi-function device such as a ConnectX-5.
> During testing I found that the mlx5_core driver does not call
> pci_disable_sriov() in its remove handler which is called on the PF
> via
> pci_stop_and_remove_bus_device() on an orderly hot unplug.

Actually what happens if you call pci_disable_sriov() when there are
VFs attached to VMs ? 

> 
> Following is a patch to fix this, I want to point out however that
> I'm not
> entirely sure about the effect of clear_vfs that distinguishes
> mlx5_sriov_detach() from mlx5_sriov_disable() only that the former is
> called
> from the remove handler and it doesn't call pci_disable_sriov().
> That however is required according to Documentation/PCI/pci-iov-
> howto.rst
> 

The Doc doesn't say "required", so the question is, is it really
required ? 

because the way i see it, it is the admin responsibility to clear out
vfs before shutting down the PF driver.

as explained in the patch review, mlx5 vf driver is resilient of such
behavior and once the device is back online the vf driver quickly
recovers, so it is actually a feature and not a bug :)

There are many reasons why the admin would want to do this:

1) Fast Firmware upgrade
2) Fast Hyper-visor upgrade/refresh
3) PF debugging 

So you can quickly reset the PF driver without the need to wait for all
VFs or manually detach-attach them.


> I've only tested this on top of my currently still internal PF-VF
> rework so
> I might also be totally missing something here in that case excuse my
> ignorance.
> 
> Best regards,
> Niklas Schnelle
> 
> Niklas Schnelle (1):
>   net/mlx5: Call pci_disable_sriov() on remove
> 
>  drivers/net/ethernet/mellanox/mlx5/core/sriov.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>