Re: [PATCH net 4/8] i40e: Fix XDP program unloading while removing the driver

[Michal Kubiak <michal.kubiak@intel.com>]

On Thu, Jun 06, 2024 at 06:43:28AM -0700, Jakub Kicinski wrote:
> On Thu, 6 Jun 2024 12:02:27 +0200 Michal Kubiak wrote:
> > > Apologizes for asking a question which can be answered by studying 
> > > the code longer, but why do you need to rebuild internal data
> > > structures for a device which is *down*. Unregistering or not.  
> > 
> > Excuse me, but I don't understand why we should assume that a device is
> > *down* when that callback is being called?
> > Maybe I didn't make it clear, but the ndo_bpf can be called every time
> > when the userspace application wants to load or unload the XDP program.
> > It can happen when a device is *up* and also when the link is *up*.
> 
> The patch was adding a special case for NETREG_UNREGISTERING,
> at that point the device will be closed. Calling ndo_close is one
> of the first things core does during unregistering.
> Simplifying the handling for when the device is closed would be
> better.

I think I'm getting your point but moving the code for
NETREG_UNREGISTERING to ndo_stop wouldn't be enough and it seems to be
against the current design of 'unregister_netdevice_many_notify()'.

In 'unregister_netdevice_many_notify()' we have the call to
'dev_close_many()' (which calls ndo_stop on i40e driver) and then
'dev_xdp_uninstall()' is called (where there is the call to ndo_bpf).

'dev_xdp_uninstall()' seems to be the right function where all
activities related to XDP program unloading during unregistering are
expected from the driver.

Anyway, I analyzed that code one more time and I agree that the special
case for NETREG_UNREGISTERING makes the code more complex and I can
implement it in a simpler way.
The root cause of the problem I'm trying to fix is that __I40E_IN_REMOVE
flag is handled in a wrong way.

I will post the v2 then.


Thanks,
Michal