| Message ID | 20240828100044.53870-1-shenlichuan@vivo.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1] sfc: Convert to use ERR_CAST() | expand |
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.
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>
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.
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
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...
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!
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);
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(-)