[PATCH v1] sfc: Convert to use ERR_CAST()

[PATCH v1] sfc: Convert to use ERR_CAST()

[Shen Lichuan]

As opposed to open-code, using the ERR_CAST macro clearly indicates that 
this is a pointer to an error value and a type conversion was performed.

Signed-off-by: Shen Lichuan <shenlichuan@vivo.com>
---
 drivers/net/ethernet/sfc/tc_counters.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/sfc/tc_counters.c b/drivers/net/ethernet/sfc/tc_counters.c
index c44088424323..76d32641202b 100644
--- a/drivers/net/ethernet/sfc/tc_counters.c
+++ b/drivers/net/ethernet/sfc/tc_counters.c
@@ -249,7 +249,7 @@ struct efx_tc_counter_index *efx_tc_flower_get_counter_index(
 					       &ctr->linkage,
 					       efx_tc_counter_id_ht_params);
 			kfree(ctr);
-			return (void *)cnt; /* it's an ERR_PTR */
+			return ERR_CAST(cnt); /* it's an ERR_PTR */
 		}
 		ctr->cnt = cnt;
 		refcount_set(&ctr->ref, 1);
-- 
2.17.1

Re: [PATCH v1] sfc: Convert to use ERR_CAST()

[Edward Cree]

On 28/08/2024 11:00, Shen Lichuan wrote:
> As opposed to open-code, using the ERR_CAST macro clearly indicates that 
> this is a pointer to an error value and a type conversion was performed.
> 
> Signed-off-by: Shen Lichuan <shenlichuan@vivo.com>
> ---
>  drivers/net/ethernet/sfc/tc_counters.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/sfc/tc_counters.c b/drivers/net/ethernet/sfc/tc_counters.c
> index c44088424323..76d32641202b 100644
> --- a/drivers/net/ethernet/sfc/tc_counters.c
> +++ b/drivers/net/ethernet/sfc/tc_counters.c
> @@ -249,7 +249,7 @@ struct efx_tc_counter_index *efx_tc_flower_get_counter_index(
>  					       &ctr->linkage,
>  					       efx_tc_counter_id_ht_params);
>  			kfree(ctr);
> -			return (void *)cnt; /* it's an ERR_PTR */
> +			return ERR_CAST(cnt); /* it's an ERR_PTR */

May as well remove the now superfluous comment.
Other than that this lgtm.

Re: [PATCH v1] sfc: Convert to use ERR_CAST()

[Jacob Keller]

On 8/28/2024 6:23 AM, Edward Cree wrote:
> On 28/08/2024 11:00, Shen Lichuan wrote:
>> As opposed to open-code, using the ERR_CAST macro clearly indicates that 
>> this is a pointer to an error value and a type conversion was performed.
>>
>> Signed-off-by: Shen Lichuan <shenlichuan@vivo.com>
>> ---
>>  drivers/net/ethernet/sfc/tc_counters.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/sfc/tc_counters.c b/drivers/net/ethernet/sfc/tc_counters.c
>> index c44088424323..76d32641202b 100644
>> --- a/drivers/net/ethernet/sfc/tc_counters.c
>> +++ b/drivers/net/ethernet/sfc/tc_counters.c
>> @@ -249,7 +249,7 @@ struct efx_tc_counter_index *efx_tc_flower_get_counter_index(
>>  					       &ctr->linkage,
>>  					       efx_tc_counter_id_ht_params);
>>  			kfree(ctr);

I was confused because I was wondering why you would kfree the error
value.. until I realized that this function has both "ctr" and "ctn".

>> -			return (void *)cnt; /* it's an ERR_PTR */
>> +			return ERR_CAST(cnt); /* it's an ERR_PTR */
> 
> May as well remove the now superfluous comment.
> Other than that this lgtm.
> 

Somewhat unrelated but you could cleanup some of the confusion by using
__free(kfree) annotation from <linux/cleanup.h> to avoid needing to
manually free ctr in error paths, and just use return_ptr() return the
value at the end.

Anyways, with the removal of the comment suggested by Edward,

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Re: [PATCH v1] sfc: Convert to use ERR_CAST()

[Jakub Kicinski]

On Wed, 28 Aug 2024 15:31:08 -0700 Jacob Keller wrote:
> Somewhat unrelated but you could cleanup some of the confusion by using
> __free(kfree) annotation from <linux/cleanup.h> to avoid needing to
> manually free ctr in error paths, and just use return_ptr() return the
> value at the end.

Please don't send people towards __free(). In general, but especially as
part of random cleanups.

Re: [PATCH v1] sfc: Convert to use ERR_CAST()

[Alejandro Lucero Palau]

On 8/29/24 00:01, Jakub Kicinski wrote:
> On Wed, 28 Aug 2024 15:31:08 -0700 Jacob Keller wrote:
>> Somewhat unrelated but you could cleanup some of the confusion by using
>> __free(kfree) annotation from <linux/cleanup.h> to avoid needing to
>> manually free ctr in error paths, and just use return_ptr() return the
>> value at the end.
> Please don't send people towards __free(). In general, but especially as
> part of random cleanups.


Hi Kuba,


Could you explain why or point to a discussion about it?

Thanks

Re: [PATCH v1] sfc: Convert to use ERR_CAST()

[Jakub Kicinski]

On Thu, 29 Aug 2024 07:47:34 +0100 Alejandro Lucero Palau wrote:
> On 8/29/24 00:01, Jakub Kicinski wrote:
> > On Wed, 28 Aug 2024 15:31:08 -0700 Jacob Keller wrote:  
> >> Somewhat unrelated but you could cleanup some of the confusion by using
> >> __free(kfree) annotation from <linux/cleanup.h> to avoid needing to
> >> manually free ctr in error paths, and just use return_ptr() return the
> >> value at the end.  
> > Please don't send people towards __free(). In general, but especially as
> > part of random cleanups.  
> 
> Could you explain why or point to a discussion about it?

It was discussed multiple times on the list and various community calls,
someone was supposed to document it but didn't. So I guess I should...

Re: [PATCH v1] sfc: Convert to use ERR_CAST()

[Alejandro Lucero Palau]

On 8/29/24 16:09, Jakub Kicinski wrote:
> On Thu, 29 Aug 2024 07:47:34 +0100 Alejandro Lucero Palau wrote:
>> On 8/29/24 00:01, Jakub Kicinski wrote:
>>> On Wed, 28 Aug 2024 15:31:08 -0700 Jacob Keller wrote:
>>>> Somewhat unrelated but you could cleanup some of the confusion by using
>>>> __free(kfree) annotation from <linux/cleanup.h> to avoid needing to
>>>> manually free ctr in error paths, and just use return_ptr() return the
>>>> value at the end.
>>> Please don't send people towards __free(). In general, but especially as
>>> part of random cleanups.
>> Could you explain why or point to a discussion about it?
> It was discussed multiple times on the list and various community calls,
> someone was supposed to document it but didn't. So I guess I should...


I have seen your quick reaction with the cleanup.h guidance patch.

Honestly, I have never been comfortable with some of the automatic 
cleanup approaches ...


Thank you!