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

[Ferruh Yigit <ferruh.yigit@amd.com>]

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++;