Re: if_flags usage etc.

Luigi Rizzo wrote:
> approx 1 month ago there was some discussion on the usage of
> if_flags and if_drv_flags in the stack
> As an exercise, i tried to identify the places these flags
> are used by taking a -current snapshot, splitting if_flags
> into two sets (those created at init time, those modified
> during normal operation) called if_flags_i and if_flags_n,
> and renaming the flags IFF_i_* and IFF_n_* respectively.
> Also, to point out where the IFF_DRV_* flags are used, i
> have renamed them IFF_d_DRV_*.
> 
> just to see how it looks, the patch that allows GENERIC to compile is at
> http://info.iet.unipi.it/~luigi/FreeBSD/if_flags.20060114.diff
> (i know it is missing netgraph, carp, vlan and atm, but
> covers the majority of drivers).
> No big suprises here but a few interesting things:
> 
> - because if_drv_flags only contains RUNNING and OACTIVE, it
>   could be manipulated by simple assignments by the drivers,
>   instead of using bitwise ops;
> 
> 	-       ifp->if_drv_flags |= IFF_d_DRV_RUNNING;   
> 	-       ifp->if_drv_flags &= ~IFF_d_DRV_OACTIVE;
> 	+       ifp->if_drv_flags = IFF_d_DRV_RUNNING;   /* clear OACTIVE */
> 
> 	...
> 	
> 	-	ifp->if_drv_flags &= ~(IFF_d_DRV_RUNNING | IFF_d_DRV_OACTIVE);
> 	+	ifp->if_drv_flags = 0;
> 
>   and so on;
> 
> - the check for IFF_UP is written so often that i wonder if it wouldn't
>   be the case to wrap it around a macro, as e.g. it is done in
>   net80211/ieee80211_ioctl.c for something similar.
> 
> - IFF_CANTCHANGE still has masks for IFF_DRV_RUNNING and IFF_DRV_OACTIVE,
>   that are no more in if_flags so should probably be removed.
> 
> - very few places outside drivers look at IFF_DRV_RUNNING. Basically.
>   net/if_ethersubr.c, netinet/ip_fastfwd.c , net80211/ieee80211_ioctl.c
>   in all cases checking that both IFF_UP and IFF_RUNNING are set.
>   I am not sure why we need both.

IFF_UP is set by administrative choice and is different from IFF_RUNNING 
which means the device/driver is operational.  We'd need to look at the 
cases where both are checked; it's likely the decision making is 
muddled.  I know when I did the virtual ap stuff that the distinction 
between the two became very important so I believe it is good to keep both.

> 
> - some drivers try to manipulate flags that they should not e.g.
> 
> 	if_bge
> 		sets	IFF_UP
> 
> 	if_ed
> 		sets IFF_ALTPHYS (IFF_LINK*, but it is almost a capability here)
> 
> 	if_fwe
> 		sets IFF_PROMISC
> 
> 	if_ie
> 		resets IFF_UP
> 		sets IFF_UP
> 		sets IFF_ALLMULTI (commented as broken)
> 
> 	if_plip
> 		sets IFF_UP
> 
> 	if_re
> 		resets IFF_PROMISC
> 		sets IFF_PROMISC
> 		resets IFF_UP
> 	if_vge
> 		resets IFF_UP
> 	if_faith
> 		sets IFF_UP
> 	if_gif
> 		sets IFF_UP
> 	if_loop
> 		sets IFF_UP
> 
> 	if_ppp
> 		resets IFF_UP
> 	if_sl
> 		resets IFF_UP
> 		sets IFF_UP
> 	if_tun
> 		sets IFF_UP
> 
> 	if_de
> 		resets IFF_UP
> 
>    this can be probably fixedin many cases.

This list looks very incomplete; I'm aware of many drivers that alter 
IFF_UP incorrectly.  One reason drivers touch IFF_UP is because if_init 
has no return value so drivers use IFF_UP to indicate whether or not the 
device initialization was successful.  I held off changing the type 
signature for this method but think it's time to change it in HEAD.

> 
> - in most drivers, the logic for SIOCSIFFLAGS is very
>   convoluted and could be cleaned up a lot. if_dc.c has a
>   reasonable version. if_xl.c has a bad example, unfortunately
>   copied a few times around.

I agree but haven't looked at these drivers are good examples.

Another area where there is major confusion is in handling ioctls that 
change driver/device state.  The basic model appears to be that an upper 
layer changes state then calls the device if_init method to push state 
into the hardware.  However this puts the onus on the driver to identify 
what has changed if it wants to optimize this work; otherwise it has to 
push everything to the device.  The net80211 layer is especially hurt by 
this as many times this means reseting the 802.11 state machine which 
can be very slow.  I've done work in my vap branch in p4 to split out 
driver callbacks to help with this but it would be good to consider 
wired drivers as well as wireless in anything that goes into the main tree.

	Sam