| Message ID | 20221031114456.10482-1-jirislaby@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | i40e (gcc13): synchronize allocate/free functions return type & values | expand |
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.
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,
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
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.
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.
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,
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
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; } /**
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(-)