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

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

[Zhengchao Shao]

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.
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;
+	}
 
 	for (i = 0; i < num_vfs; i++) {
 		rc = efx_ef100_vfrep_create(efx, i);
 		if (rc)
 			goto fail2;
 	}
-	return 0;
+	return num_vfs;
 
 fail2:
 	list_for_each_entry_safe(efv, next, &efx->vf_reps, list)
-- 
2.17.1

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

[Edward Cree]

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)
> 

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

[Martin Habets]

On Fri, Nov 25, 2022 at 03:19:58PM +0800, 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.

This function returns 0 on success, or an error code. It is the higher level
function ef100_pci_sriov_configure() in ef100.c that returns num_vfs if things
are ok.

Martin

> 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;
> +	}
>  
>  	for (i = 0; i < num_vfs; i++) {
>  		rc = efx_ef100_vfrep_create(efx, i);
>  		if (rc)
>  			goto fail2;
>  	}
> -	return 0;
> +	return num_vfs;
>  
>  fail2:
>  	list_for_each_entry_safe(efv, next, &efx->vf_reps, list)
> -- 
> 2.17.1

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

[shaozhengchao]

On 2022/11/29 16:19, Edward Cree wrote:
> 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
> 
Thank you for your review. My misunderstood. You are right.
>>   
>>   fail2:
>>   	list_for_each_entry_safe(efv, next, &efx->vf_reps, list)
>>