Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 1/1] net/mlx5: Call pci_disable_sriov() on remove
From:   Niklas Schnelle <schnelle@linux.ibm.com>
To:     Saeed Mahameed <saeedm@mellanox.com>
Cc:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "leon@kernel.org" <leon@kernel.org>
References: <20200430120308.92773-1-schnelle@linux.ibm.com>
 <20200430120308.92773-2-schnelle@linux.ibm.com>
 <2409e7071482b8d05447b8660abcac15987ad399.camel@mellanox.com>
 <36de00e7-cccb-7de8-bd93-84cf647d6d39@linux.ibm.com>
Message-ID: <0c59cb62-3156-54bb-0f36-837369adf220@linux.ibm.com>
Date:   Fri, 1 May 2020 00:31:53 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <36de00e7-cccb-7de8-bd93-84cf647d6d39@linux.ibm.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 4/30/20 9:47 PM, Niklas Schnelle wrote:
> 
> 
> On 4/30/20 5:58 PM, Saeed Mahameed wrote:
>> On Thu, 2020-04-30 at 14:03 +0200, Niklas Schnelle wrote:
>>> as described in Documentation/PCI/pci-iov-howto.rst a driver with SR-
>>> IOV
>>> support should call pci_disable_sriov() in the remove handler.
>>
>> Hi Niklas,
>>
>> looking at the documentation, it doesn't say "should" it just gives the
>> code as example.
>>
>>> Otherwise removing a PF (e.g. via pci_stop_and_remove_bus_device())
>>> with
>>> attached VFs does not properly shut the VFs down before shutting down
>>> the PF. This leads to the VF drivers handling defunct devices and
>>> accompanying error messages.
>>>
>>
>> Which should be the admin responsibility .. if the admin want to do
>> this, then let it be.. why block him ? 
>>
>> our mlx5 driver in the vf handles this gracefully and once pf
>> driver/device is back online the vf driver quickly recovers.
> See my answer to your other answer ;-)
>>
>>> In the current code pci_disable_sriov() is already called in
>>> mlx5_sriov_disable() but not in mlx5_sriov_detach() which is called
>>> from
>>> the remove handler. Fix this by moving the pci_disable_sriov() call
>>> into
>>> mlx5_device_disable_sriov() which is called by both.
>>>
>>> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
>>> ---
>>>  drivers/net/ethernet/mellanox/mlx5/core/sriov.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/sriov.c
>>> b/drivers/net/ethernet/mellanox/mlx5/core/sriov.c
>>> index 3094d20297a9..2401961c9f5b 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/sriov.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/sriov.c
>>> @@ -114,6 +114,8 @@ mlx5_device_disable_sriov(struct mlx5_core_dev
>>> *dev, int num_vfs, bool clear_vf)
>>>  	int err;
>>>  	int vf;
>>>  
>>> +	pci_disable_sriov(dev->pdev);
>>> +
>>>  	for (vf = num_vfs - 1; vf >= 0; vf--) {
>>>  		if (!sriov->vfs_ctx[vf].enabled)
>>>  			continue;
>>> @@ -156,7 +158,6 @@ static void mlx5_sriov_disable(struct pci_dev
>>> *pdev)
>>>  	struct mlx5_core_dev *dev  = pci_get_drvdata(pdev);
>>>  	int num_vfs = pci_num_vf(dev->pdev);
>>>  
>>> -	pci_disable_sriov(pdev);
>>
>> this patch is no good as it breaks code symmetry.. and could lead to
>> many new issues.
> Ah you're right I totally missed that there is a matching pci_enable_sriov() in
> mlx5_enable_sriov() haven't used these myself before and since it wasn't in the
> documentation example I somehow expected it to happen in non-driver code,
aaand it actually is in the documentation example and I definitely sent this
when it wasn't ready, sorry again…
> so for symmetry that would also have to move to mlx5_device_enable_sriov(),
> sorry for the oversight.
>>
>>
>>>  	mlx5_device_disable_sriov(dev, num_vfs, true);
>>>  }
>>>  
>>