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

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

[Artemii Morozov]

From: Artemii Morozov <Artemii.Morozov@arknetworks.am>

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

Signed-off-by: Artemii Morozov <artemii.morozov@arknetworks.am>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
 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++;
-- 
2.34.1

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

[Artemii Morozov]

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

Signed-off-by: Artemii Morozov <artemii.morozov@arknetworks.am>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
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++;
-- 
2.34.1

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

[Ferruh Yigit]

On 6/1/2023 7:30 AM, Artemii Morozov wrote:
> If the prefix for the rss hash was not chosen the
> ENOTSUP error should be returned.
> 
> Signed-off-by: Artemii Morozov <artemii.morozov@arknetworks.am>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
> ---
> 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++;

This is fixing return code of the function, right? Before update success
was returned at this point event it failed.

If so, can you please update commit log as fix commit and add proper
fixes tags?

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

[Artemii Morozov]

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

Signed-off-by: Artemii Morozov <artemii.morozov@arknetworks.am>
Reviewed-by: Andy Moreton <amoreton@xilinx.com>
---
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++;
-- 
2.34.1

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

[Andrew Rybchenko]

On 6/22/23 12:13, Artemii Morozov wrote:
> If the prefix for the rss hash was not chosen the

rss -> RSS

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

I guess Cc: stable@dpdk.org is missing

> 
> Signed-off-by: Artemii Morozov <artemii.morozov@arknetworks.am>
> Reviewed-by: Andy Moreton <amoreton@xilinx.com>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

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

[Artemii Morozov]

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

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

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

[Ferruh Yigit]

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 v5] common/sfc_efx/base: fix Rx queue creation without RSS hash prefix

[Artemii Morozov]

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: 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>
---
v5: update commit message

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

Re: [PATCH v5] common/sfc_efx/base: fix Rx queue creation without RSS hash prefix

[Ferruh Yigit]

On 6/22/2023 1:31 PM, Artemii Morozov wrote:
> 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.
> 

It looks like you used sample commit log as it is, so can you please
confirm this is the actual case (because this was my assumption without
really knowing the actual problem), problem is not crash or undefined
behavior because of 'erpp' was not set etc..?

> 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>
> ---
> v5: update commit message
> 
> 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++;

Re: [PATCH v5] common/sfc_efx/base: fix Rx queue creation without RSS hash prefix

[Artemii Morozov]

On 6/22/23 17:13, Ferruh Yigit wrote:
> On 6/22/2023 1:31 PM, Artemii Morozov wrote:
>> 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.
>>
> It looks like you used sample commit log as it is, so can you please
> confirm this is the actual case (because this was my assumption without
> really knowing the actual problem), problem is not crash or undefined
> behavior because of 'erpp' was not set etc..?
The main problem is that if the RSS prefix was requested and an error 
occurred when creating the Rx queue, then this is ignored. Now the 
commit message reflects this.
>
>> 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>
>> ---
>> v5: update commit message
>>
>> 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++;

Re: [PATCH v5] common/sfc_efx/base: fix Rx queue creation without RSS hash prefix

[Ferruh Yigit]

On 6/22/2023 1:31 PM, Artemii Morozov wrote:
> 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: 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>
>

Applied to dpdk-next-net/main, thanks.