[v4] common/sfc_efx/base: set return code in case of the error

Message ID 20230622093110.252817-1-artemii.morozov@arknetworks.am (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v4] common/sfc_efx/base: set return code in case of the error |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Functional success Functional PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS

Commit Message

Artemii Morozov June 22, 2023, 9:31 a.m. UTC
  If the prefix for the RSS hash was not chosen the
ENOTSUP error should be returned. Before this patch
the zero code was returned in case of an error.

Fixes: f784cdc5cbb1 ("common/sfc_efx/base: provide control to deliver RSS hash")
Cc: stable@dpdk.org

Signed-off-by: Artemii Morozov <artemii.morozov@arknetworks.am>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
---
v4: add Cc: stable@dpdk.org and transform rss to RSS
  
v3: update commit log as fix commit

v2: don't use capital letters in email

 drivers/common/sfc_efx/base/efx_rx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
  

Comments

Ferruh Yigit June 22, 2023, 11:27 a.m. UTC | #1
On 6/22/2023 10:31 AM, Artemii Morozov wrote:
> If the prefix for the RSS hash was not chosen the
> ENOTSUP error should be returned. Before this patch
> the zero code was returned in case of an error.
> 

What is the impact of the problem to user?

Previously 'efx_rx_qcreate_internal()' was returning success although it
fails, and 'erpp' was not set.

There are multiple callers of 'efx_rx_qcreate_internal()', those
functions will assume functions succeed when it failed, what kind of
problem is this cause?
Does it cause a crash, or failure in the Rx queue creation, or no impact
at all, can you please describe?

Impact information is not always easy to understand from code change or
low level details, but most of the times that is what users are interested.
Like fixing a return value can be a simple refactoring without any
functional impact at all, or it can be causing a crash and making driver
completely unusable for some cases, although code change can be similar
these two cases has different priority.
So commit message/title should highlight the impact, starting from 'fix'
keyword.

Assume it cause Rx queue creation failure, commit can be something like:
```
 common/sfc_efx/base: fix Rx queue creation without RSS hash prefix

 If the prefix for the RSS hash was not chosen the ENOTSUP error should
 be returned.

 Before this patch success was returned for this case causing Rx queue
 creation to fail.

 Fixing return value to indicate failure.

 Fixes: ...
 Cc: ...

 ...
```


> Fixes: f784cdc5cbb1 ("common/sfc_efx/base: provide control to deliver RSS hash")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Artemii Morozov <artemii.morozov@arknetworks.am>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> ---
> v4: add Cc: stable@dpdk.org and transform rss to RSS
>   
> v3: update commit log as fix commit
> 
> v2: don't use capital letters in email
> 
>  drivers/common/sfc_efx/base/efx_rx.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/common/sfc_efx/base/efx_rx.c b/drivers/common/sfc_efx/base/efx_rx.c
> index 68f42f5cac..61726a9f0b 100644
> --- a/drivers/common/sfc_efx/base/efx_rx.c
> +++ b/drivers/common/sfc_efx/base/efx_rx.c
> @@ -937,8 +937,10 @@ efx_rx_qcreate_internal(
>  
>  		rss_hash_field =
>  		    &erplp->erpl_fields[EFX_RX_PREFIX_FIELD_RSS_HASH];
> -		if (rss_hash_field->erpfi_width_bits == 0)
> +		if (rss_hash_field->erpfi_width_bits == 0) {
> +			rc = ENOTSUP;
>  			goto fail5;
> +		}
>  	}
>  
>  	enp->en_rx_qcount++;
  

Patch

diff --git a/drivers/common/sfc_efx/base/efx_rx.c b/drivers/common/sfc_efx/base/efx_rx.c
index 68f42f5cac..61726a9f0b 100644
--- a/drivers/common/sfc_efx/base/efx_rx.c
+++ b/drivers/common/sfc_efx/base/efx_rx.c
@@ -937,8 +937,10 @@  efx_rx_qcreate_internal(
 
 		rss_hash_field =
 		    &erplp->erpl_fields[EFX_RX_PREFIX_FIELD_RSS_HASH];
-		if (rss_hash_field->erpfi_width_bits == 0)
+		if (rss_hash_field->erpfi_width_bits == 0) {
+			rc = ENOTSUP;
 			goto fail5;
+		}
 	}
 
 	enp->en_rx_qcount++;