i40e (gcc13): synchronize allocate/free functions return type & values

Message ID 20221031114456.10482-1-jirislaby@kernel.org
State New
Headers
Series i40e (gcc13): synchronize allocate/free functions return type & values |

Commit Message

Jiri Slaby Oct. 31, 2022, 11:44 a.m. UTC
  i40e allocate/free functions generate a valid warning with gcc-13:
  drivers/net/ethernet/intel/i40e/i40e_main.c:129:5: error: conflicting types for 'i40e_allocate_dma_mem_d' due to enum/integer mismatch; have 'int(struct i40e_hw *, struct i40e_dma_mem *, u64,  u32)' {aka 'int(struct i40e_hw *, struct i40e_dma_mem *, long long unsigned int,  unsigned int)'} [-Werror=enum-int-mismatch]
  drivers/net/ethernet/intel/i40e/i40e_osdep.h:40:25: note: previous declaration of 'i40e_allocate_dma_mem_d' with type 'i40e_status(struct i40e_hw *, struct i40e_dma_mem *, u64,  u32)' {aka 'enum i40e_status_code(struct i40e_hw *, struct i40e_dma_mem *, long long unsigned int,  unsigned int)'}
...

I.e. the type of their return value in the definition is int, while the
declaration spell enum i40e_status. Synchronize the definitions to the
latter.

And make sure proper values are returned. I.e. I40E_SUCCESS and not 0,
I40E_ERR_NO_MEMORY and not -ENOMEM.

Cc: Martin Liska <mliska@suse.cz>
Cc: Jesse Brandeburg <jesse.brandeburg@intel.com>
Cc: Tony Nguyen <anthony.l.nguyen@intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: intel-wired-lan@lists.osuosl.org
Cc: netdev@vger.kernel.org
Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/net/ethernet/intel/i40e/i40e_main.c | 25 +++++++++++----------
 1 file changed, 13 insertions(+), 12 deletions(-)
  

Comments

Jakub Kicinski Nov. 3, 2022, 3:41 a.m. UTC | #1
On Mon, 31 Oct 2022 12:44:56 +0100 Jiri Slaby (SUSE) wrote:
> I.e. the type of their return value in the definition is int, while the
> declaration spell enum i40e_status. Synchronize the definitions to the
> latter.
> 
> And make sure proper values are returned. I.e. I40E_SUCCESS and not 0,
> I40E_ERR_NO_MEMORY and not -ENOMEM.

Let's go the opposite way, towards using standard errno.
  
Jiri Slaby Nov. 3, 2022, 12:03 p.m. UTC | #2
On 03. 11. 22, 4:41, Jakub Kicinski wrote:
> On Mon, 31 Oct 2022 12:44:56 +0100 Jiri Slaby (SUSE) wrote:
>> I.e. the type of their return value in the definition is int, while the
>> declaration spell enum i40e_status. Synchronize the definitions to the
>> latter.
>>
>> And make sure proper values are returned. I.e. I40E_SUCCESS and not 0,
>> I40E_ERR_NO_MEMORY and not -ENOMEM.
> 
> Let's go the opposite way, towards using standard errno.

This is propagated several layers up throughout the whole i40e driver. 
It would be a mass change which I'd rather leave up to the driver 
maintainers -- I don't even have the HW to test.

thanks,
  
Tony Nguyen Nov. 4, 2022, 6:33 p.m. UTC | #3
On 11/3/2022 5:03 AM, Jiri Slaby wrote:
> On 03. 11. 22, 4:41, Jakub Kicinski wrote:
>> On Mon, 31 Oct 2022 12:44:56 +0100 Jiri Slaby (SUSE) wrote:
>>> I.e. the type of their return value in the definition is int, while the
>>> declaration spell enum i40e_status. Synchronize the definitions to the
>>> latter.
>>>
>>> And make sure proper values are returned. I.e. I40E_SUCCESS and not 0,
>>> I40E_ERR_NO_MEMORY and not -ENOMEM.
>>
>> Let's go the opposite way, towards using standard errno.
> 
> This is propagated several layers up throughout the whole i40e driver. 
> It would be a mass change which I'd rather leave up to the driver 
> maintainers -- I don't even have the HW to test.

Hi Jakub,

As Jiri mentioned, this is propagated up throughout the driver. We could 
change this function to return int but all the callers would then need 
to convert these errors to i40e_status to propagate. This doesn't really 
gain much other than having this function return int. To adjust the 
entire call chain is going to take more work. As this is resolving a 
valid warning and returning what is currently expected, what are your 
thoughts on taking this now to resolve the issue and our i40e team will 
take the work on to convert the functions to use the standard errnos?

Thanks,
Tony
  
Jakub Kicinski Nov. 4, 2022, 6:47 p.m. UTC | #4
On Fri, 4 Nov 2022 11:33:07 -0700 Tony Nguyen wrote:
> As Jiri mentioned, this is propagated up throughout the driver. We could 
> change this function to return int but all the callers would then need 
> to convert these errors to i40e_status to propagate. This doesn't really 
> gain much other than having this function return int. To adjust the 
> entire call chain is going to take more work. As this is resolving a 
> valid warning and returning what is currently expected, what are your 
> thoughts on taking this now to resolve the issue and our i40e team will 
> take the work on to convert the functions to use the standard errnos?

My thoughts on your OS abstraction layers should be pretty evident.
If anything I'd like to be more vigilant about less flagrant cases.

I don't think this is particularly difficult, let's patch it up
best we can without letting the "status" usage grow.
  
Tony Nguyen Nov. 4, 2022, 8:28 p.m. UTC | #5
On 11/4/2022 11:47 AM, Jakub Kicinski wrote:
> On Fri, 4 Nov 2022 11:33:07 -0700 Tony Nguyen wrote:
>> As Jiri mentioned, this is propagated up throughout the driver. We could
>> change this function to return int but all the callers would then need
>> to convert these errors to i40e_status to propagate. This doesn't really
>> gain much other than having this function return int. To adjust the
>> entire call chain is going to take more work. As this is resolving a
>> valid warning and returning what is currently expected, what are your
>> thoughts on taking this now to resolve the issue and our i40e team will
>> take the work on to convert the functions to use the standard errnos?
> 
> My thoughts on your OS abstraction layers should be pretty evident.
> If anything I'd like to be more vigilant about less flagrant cases.
> 
> I don't think this is particularly difficult, let's patch it up
> best we can without letting the "status" usage grow.

Ok thanks will do.
  
Jiri Slaby Dec. 12, 2022, 11:55 a.m. UTC | #6
On 04. 11. 22, 21:28, Tony Nguyen wrote:
> 
> 
> On 11/4/2022 11:47 AM, Jakub Kicinski wrote:
>> On Fri, 4 Nov 2022 11:33:07 -0700 Tony Nguyen wrote:
>>> As Jiri mentioned, this is propagated up throughout the driver. We could
>>> change this function to return int but all the callers would then need
>>> to convert these errors to i40e_status to propagate. This doesn't really
>>> gain much other than having this function return int. To adjust the
>>> entire call chain is going to take more work. As this is resolving a
>>> valid warning and returning what is currently expected, what are your
>>> thoughts on taking this now to resolve the issue and our i40e team will
>>> take the work on to convert the functions to use the standard errnos?
>>
>> My thoughts on your OS abstraction layers should be pretty evident.
>> If anything I'd like to be more vigilant about less flagrant cases.
>>
>> I don't think this is particularly difficult, let's patch it up
>> best we can without letting the "status" usage grow.
> 
> Ok thanks will do.

Just heads-up: have you managed to remove the abstraction yet?

thanks,
  
Tony Nguyen Dec. 12, 2022, 5:51 p.m. UTC | #7
On 12/12/2022 3:55 AM, Jiri Slaby wrote:
> On 04. 11. 22, 21:28, Tony Nguyen wrote:
>>
>>
>> On 11/4/2022 11:47 AM, Jakub Kicinski wrote:
>>> On Fri, 4 Nov 2022 11:33:07 -0700 Tony Nguyen wrote:
>>>> As Jiri mentioned, this is propagated up throughout the driver. We 
>>>> could
>>>> change this function to return int but all the callers would then need
>>>> to convert these errors to i40e_status to propagate. This doesn't 
>>>> really
>>>> gain much other than having this function return int. To adjust the
>>>> entire call chain is going to take more work. As this is resolving a
>>>> valid warning and returning what is currently expected, what are your
>>>> thoughts on taking this now to resolve the issue and our i40e team will
>>>> take the work on to convert the functions to use the standard errnos?
>>>
>>> My thoughts on your OS abstraction layers should be pretty evident.
>>> If anything I'd like to be more vigilant about less flagrant cases.
>>>
>>> I don't think this is particularly difficult, let's patch it up
>>> best we can without letting the "status" usage grow.
>>
>> Ok thanks will do.
> 
> Just heads-up: have you managed to remove the abstraction yet?


Hi Jiri,

It's being worked on: 
https://lore.kernel.org/intel-wired-lan/20221207144800.1257060-1-jan.sokolowski@intel.com/

Thanks,
Tony
  

Patch

diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 1a1fab94205d..92fd4db7195f 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -126,8 +126,9 @@  static void netdev_hw_addr_refcnt(struct i40e_mac_filter *f,
  * @size: size of memory requested
  * @alignment: what to align the allocation to
  **/
-int i40e_allocate_dma_mem_d(struct i40e_hw *hw, struct i40e_dma_mem *mem,
-			    u64 size, u32 alignment)
+i40e_status i40e_allocate_dma_mem_d(struct i40e_hw *hw,
+				    struct i40e_dma_mem *mem, u64 size,
+				    u32 alignment)
 {
 	struct i40e_pf *pf = (struct i40e_pf *)hw->back;
 
@@ -135,9 +136,9 @@  int i40e_allocate_dma_mem_d(struct i40e_hw *hw, struct i40e_dma_mem *mem,
 	mem->va = dma_alloc_coherent(&pf->pdev->dev, mem->size, &mem->pa,
 				     GFP_KERNEL);
 	if (!mem->va)
-		return -ENOMEM;
+		return I40E_ERR_NO_MEMORY;
 
-	return 0;
+	return I40E_SUCCESS;
 }
 
 /**
@@ -145,7 +146,7 @@  int i40e_allocate_dma_mem_d(struct i40e_hw *hw, struct i40e_dma_mem *mem,
  * @hw:   pointer to the HW structure
  * @mem:  ptr to mem struct to free
  **/
-int i40e_free_dma_mem_d(struct i40e_hw *hw, struct i40e_dma_mem *mem)
+i40e_status i40e_free_dma_mem_d(struct i40e_hw *hw, struct i40e_dma_mem *mem)
 {
 	struct i40e_pf *pf = (struct i40e_pf *)hw->back;
 
@@ -154,7 +155,7 @@  int i40e_free_dma_mem_d(struct i40e_hw *hw, struct i40e_dma_mem *mem)
 	mem->pa = 0;
 	mem->size = 0;
 
-	return 0;
+	return I40E_SUCCESS;
 }
 
 /**
@@ -163,16 +164,16 @@  int i40e_free_dma_mem_d(struct i40e_hw *hw, struct i40e_dma_mem *mem)
  * @mem:  ptr to mem struct to fill out
  * @size: size of memory requested
  **/
-int i40e_allocate_virt_mem_d(struct i40e_hw *hw, struct i40e_virt_mem *mem,
-			     u32 size)
+i40e_status i40e_allocate_virt_mem_d(struct i40e_hw *hw,
+				     struct i40e_virt_mem *mem, u32 size)
 {
 	mem->size = size;
 	mem->va = kzalloc(size, GFP_KERNEL);
 
 	if (!mem->va)
-		return -ENOMEM;
+		return I40E_ERR_NO_MEMORY;
 
-	return 0;
+	return I40E_SUCCESS;
 }
 
 /**
@@ -180,14 +181,14 @@  int i40e_allocate_virt_mem_d(struct i40e_hw *hw, struct i40e_virt_mem *mem,
  * @hw:   pointer to the HW structure
  * @mem:  ptr to mem struct to free
  **/
-int i40e_free_virt_mem_d(struct i40e_hw *hw, struct i40e_virt_mem *mem)
+i40e_status i40e_free_virt_mem_d(struct i40e_hw *hw, struct i40e_virt_mem *mem)
 {
 	/* it's ok to kfree a NULL pointer */
 	kfree(mem->va);
 	mem->va = NULL;
 	mem->size = 0;
 
-	return 0;
+	return I40E_SUCCESS;
 }
 
 /**