Re: [PATCH net] sfc: fix error process in efx_ef100_pci_sriov_enable()

[Edward Cree <ecree.xilinx@gmail.com>]

On 25/11/2022 07:19, Zhengchao Shao wrote:
> There are two issues in efx_ef100_pci_sriov_enable():
> 1. When it doesn't have MAE Privilege, it doesn't disable pci sriov.
> 2. When creating VF successfully, it should return vf nums instead of 0.

A single patch should do one thing.  If these two issues were
 valid, they ought to be fixed separately by two commits.

> Compiled test only.
> 
> Fixes: 08135eecd07f ("sfc: add skeleton ef100 VF representors")
> Fixes: 78a9b3c47bef ("sfc: add EF100 VF support via a write to sriov_numvfs")
> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com>
> ---
>  drivers/net/ethernet/sfc/ef100_sriov.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/ef100_sriov.c b/drivers/net/ethernet/sfc/ef100_sriov.c
> index 94bdbfcb47e8..adf7fb09940e 100644
> --- a/drivers/net/ethernet/sfc/ef100_sriov.c
> +++ b/drivers/net/ethernet/sfc/ef100_sriov.c
> @@ -25,15 +25,17 @@ static int efx_ef100_pci_sriov_enable(struct efx_nic *efx, int num_vfs)
>  	if (rc)
>  		goto fail1;
>  
> -	if (!nic_data->grp_mae)
> +	if (!nic_data->grp_mae) {
> +		pci_disable_sriov(dev);
>  		return 0;
> +	}

NACK to this part; if we don't have MAE privilege, that means
 someone else (e.g. the embedded SoC) is in charge of the MAE
 and is responsible for configuring switching behaviour for any
 VFs we create.
Thus, the existing behaviour — create the VFs without creating
 any corresponding representors — is intended.

>  
>  	for (i = 0; i < num_vfs; i++) {
>  		rc = efx_ef100_vfrep_create(efx, i);
>  		if (rc)
>  			goto fail2;
>  	}
> -	return 0;
> +	return num_vfs;

NACK to this too: this is not returned directly to the PCI
 core but to ef100_pci_sriov_configure(), which already does
 the translation from 0 (success) to num_vfs.  So changing it
 here is unnecessary.

-ed

>  
>  fail2:
>  	list_for_each_entry_safe(efv, next, &efx->vf_reps, list)
>