netdev - Re: [PATCH net] sfc: fix error process in efx_ef100_pci_sriov

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3adfad23-b2dd-3bd9-40da-d4dfaa78b435@gmail.com>
Date:   Tue, 29 Nov 2022 08:19:50 +0000
From:   Edward Cree <ecree.xilinx@...il.com>
To:     Zhengchao Shao <shaozhengchao@...wei.com>, netdev@...r.kernel.org,
        habetsm.xilinx@...il.com, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com
Cc:     pieter.jansen-van-vuuren@....com, weiyongjun1@...wei.com,
        yuehaibing@...wei.com
Subject: Re: [PATCH net] sfc: fix error process in
 efx_ef100_pci_sriov_enable()

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@...wei.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)
>