Re: [PATCH] Revert "net: linkwatch: add check for netdevice being present to linkwatch_do_dev"

[Nikolay Aleksandrov]

On Mon, 2020-09-14 at 09:40 +0200, Geert Uytterhoeven wrote:
> Hi David,
> 
> CC bridge
> 
> On Sun, Sep 13, 2020 at 3:34 AM David Miller <davem@xxxxxxxxxxxxx> wrote:
> > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > Date: Sat, 12 Sep 2020 14:33:59 +0200
> > 
> > > "dev" is not the bridge device, but the physical Ethernet interface, which
> > > may already be suspended during s2ram.
> > 
> > Hmmm, ok.
> > 
> > Looking more deeply NETDEV_CHANGE causes br_port_carrier_check() to run which
> > exits early if netif_running() is false, which is going to be true if
> > netif_device_present() is false:
> > 
> >         *notified = false;
> >         if (!netif_running(br->dev))
> >                 return;
> > 
> > The only other work the bridge notifier does is:
> > 
> >         if (event != NETDEV_UNREGISTER)
> >                 br_vlan_port_event(p, event);
> > 
> > and:
> > 
> >         /* Events that may cause spanning tree to refresh */
> >         if (!notified && (event == NETDEV_CHANGEADDR || event == NETDEV_UP ||
> >                           event == NETDEV_CHANGE || event == NETDEV_DOWN))
> >                 br_ifinfo_notify(RTM_NEWLINK, NULL, p);
> > 
> > So some vlan stuff, and emitting a netlink message to any available
> > listeners.
> > 
> > Should we really do all of this for a device which is not even
> > present?
> > 
> > This whole situation seems completely illogical.  The device is
> > useless, it logically has no link or other state that can be managed
> > or used, while it is not present.
> > 
> > So all of these bridge operations should only happen when the device
> > transitions back to present again.
> 
> Thanks for your analysis!
> I'd like to defer this to the bridge people (CC).
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Hi,
Sorry for the delay. Interesting problem. :)
Thanks for the analysis, I don't see any issues with checking if the device
isn't present. It will have to go through some testing, but no obvious
objections/issues. Have you tried if it fixes your case?
I have briefly gone over drivers' use of net_device_detach(), mostly it's used
for suspends, but there are a few cases which use it on IO error or when a
device is actually detaching (VF detach). The vlan port event is for vlan
devices on top of the bridge when BROPT_VLAN_BRIDGE_BINDING is enabled and their
carrier is changed based on vlan participating ports' state.

Thanks,
 Nik