[PATCH iwl-net] ice: add missing napi deletion

[PATCH iwl-net] ice: add missing napi deletion

[Maciej Fijalkowski]

Error path from ice_probe() is missing ice_napi_del() calls, add it to
ice_deinit_eth() as ice_init_eth() was the one to add napi instances. It
is also a good habit to delete napis when removing driver and this also
addresses that. FWIW ice_napi_del() had no callsites.

Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
index cd562856f23a..f6b041490154 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -4485,6 +4485,7 @@ static void ice_deinit_eth(struct ice_pf *pf)
 	if (!vsi)
 		return;
 
+	ice_napi_del(vsi);
 	ice_vsi_close(vsi);
 	ice_unregister_netdev(vsi);
 	ice_devlink_destroy_pf_port(pf);
-- 
2.34.1

Re: [Intel-wired-lan] [PATCH iwl-net] ice: add missing napi deletion

[Przemek Kitszel]

On 6/20/23 10:24, Maciej Fijalkowski wrote:
> Error path from ice_probe() is missing ice_napi_del() calls, add it to
> ice_deinit_eth() as ice_init_eth() was the one to add napi instances. It
> is also a good habit to delete napis when removing driver and this also
> addresses that. FWIW ice_napi_del() had no callsites.
> 
> Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> ---
>   drivers/net/ethernet/intel/ice/ice_main.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> index cd562856f23a..f6b041490154 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -4485,6 +4485,7 @@ static void ice_deinit_eth(struct ice_pf *pf)
>   	if (!vsi)
>   		return;
>   
> +	ice_napi_del(vsi);
>   	ice_vsi_close(vsi);
>   	ice_unregister_netdev(vsi);
>   	ice_devlink_destroy_pf_port(pf);

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Re: [PATCH iwl-net] ice: add missing napi deletion

[Jakub Kicinski]

On Tue, 20 Jun 2023 10:24:54 +0200 Maciej Fijalkowski wrote:
> Error path from ice_probe() is missing ice_napi_del() calls, add it to
> ice_deinit_eth() as ice_init_eth() was the one to add napi instances. It
> is also a good habit to delete napis when removing driver and this also
> addresses that. FWIW ice_napi_del() had no callsites.
> 
> Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

Is there user visible impact? I agree that it's a good habit, but
since unregister cleans up NAPI instances automatically the patch
is not necessarily a fix.

Re: [PATCH iwl-net] ice: add missing napi deletion

[Maciej Fijalkowski]

On Tue, Jun 20, 2023 at 09:53:35AM -0700, Jakub Kicinski wrote:
> On Tue, 20 Jun 2023 10:24:54 +0200 Maciej Fijalkowski wrote:
> > Error path from ice_probe() is missing ice_napi_del() calls, add it to
> > ice_deinit_eth() as ice_init_eth() was the one to add napi instances. It
> > is also a good habit to delete napis when removing driver and this also
> > addresses that. FWIW ice_napi_del() had no callsites.
> > 
> > Fixes: 6624e780a577 ("ice: split ice_vsi_setup into smaller functions")
> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> 
> Is there user visible impact? I agree that it's a good habit, but
> since unregister cleans up NAPI instances automatically the patch
> is not necessarily a fix.

It's rather free_netdev() not unregistering per se, no? I sent this patch
as I found that cited commit didn't delete napis on ice_probe()'s error
path - I just saw that as a regression. 

But as you're saying when getting rid of netdev we actually do
netif_napi_del() - it seems redundant to do explicit napi delete on remove
path as it is supposed do free the netdev. Does it mean that many drivers
should be verified against that? Sorta tired so might be missing
something, pardon. If not, I'll send a v2 that just removes
ice_napi_del().

Re: [PATCH iwl-net] ice: add missing napi deletion

[Jakub Kicinski]

On Tue, 20 Jun 2023 19:22:01 +0200 Maciej Fijalkowski wrote:
> On Tue, Jun 20, 2023 at 09:53:35AM -0700, Jakub Kicinski wrote:
> > Is there user visible impact? I agree that it's a good habit, but
> > since unregister cleans up NAPI instances automatically the patch
> > is not necessarily a fix.  
> 
> It's rather free_netdev() not unregistering per se, no? I sent this patch
> as I found that cited commit didn't delete napis on ice_probe()'s error
> path - I just saw that as a regression. 
> 
> But as you're saying when getting rid of netdev we actually do
> netif_napi_del() - it seems redundant to do explicit napi delete on remove
> path as it is supposed do free the netdev. Does it mean that many drivers
> should be verified against that? Sorta tired so might be missing
> something, pardon. If not, I'll send a v2 that just removes
> ice_napi_del().

I personally prefer to keep track of my resources, so I avoid devm_*
and delete NAPI instances by hand. It's up to the author and/or
maintainer of the driver in question.

My only real ask is to no route this via net and drop the Fixes tag.
Whether you prefer to keep the patch as is or drop ice_napi_del() --
up to you.

Re: [PATCH iwl-net] ice: add missing napi deletion

[Maciej Fijalkowski]

On Tue, Jun 20, 2023 at 10:49:11AM -0700, Jakub Kicinski wrote:
> On Tue, 20 Jun 2023 19:22:01 +0200 Maciej Fijalkowski wrote:
> > On Tue, Jun 20, 2023 at 09:53:35AM -0700, Jakub Kicinski wrote:
> > > Is there user visible impact? I agree that it's a good habit, but
> > > since unregister cleans up NAPI instances automatically the patch
> > > is not necessarily a fix.  
> > 
> > It's rather free_netdev() not unregistering per se, no? I sent this patch
> > as I found that cited commit didn't delete napis on ice_probe()'s error
> > path - I just saw that as a regression. 
> > 
> > But as you're saying when getting rid of netdev we actually do
> > netif_napi_del() - it seems redundant to do explicit napi delete on remove
> > path as it is supposed do free the netdev. Does it mean that many drivers
> > should be verified against that? Sorta tired so might be missing
> > something, pardon. If not, I'll send a v2 that just removes
> > ice_napi_del().
> 
> I personally prefer to keep track of my resources, so I avoid devm_*
> and delete NAPI instances by hand. It's up to the author and/or
> maintainer of the driver in question.

Hmm I am not a fan of devm either but I didn't mean that in my response at
all.

There are quite a few drivers that do this:

net/core/dev.c:
void free_netdev(struct net_device *dev)
{
	(...)
	list_for_each_entry_safe(p, n, &dev->napi_list, dev_list)
		netif_napi_del(p);
	(...)
}

static inline void netif_napi_del(struct napi_struct *napi)
{
	__netif_napi_del(napi);
	synchronize_net();
}

drivers/net/ethernet/xxxcorp/xxx/xxx_main.c:
static void xxx_remove(struct pci_dev *pdev)
{
	// retrieve net_device and napi_struct ptrs

	netif_napi_del(napi); // redundant, covered below
	(...)
	free_netdev(netdev);
	(...)
}

I believe this is what you were referring to originally and I said that
after a short drivers audit there is a bunch going via flow shown
above...plus my patch was trying to introduce that :)

Although in such case __netif_napi_del() will exit early as
NAPI_STATE_LISTED bit was cleared, if driver holds multiple napi instances
we will be going unnecessarily via synchronize_net() calls.

> 
> My only real ask is to no route this via net and drop the Fixes tag.
> Whether you prefer to keep the patch as is or drop ice_napi_del() --
> up to you.

I'll go through -next and remove ice_napi_del(), given the above
explanation what I meant previously.

>