Sam Leffler wrote: > 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. It sounds like the work that luigi and you have both done in this area is well worth continuing. I would like to encourage Luigi to consider that his exercise has proven a good thing and that it be done in earnest. We should probably better document the interface "interface". if we are going to (as Sam suggests) do some cleanups we might as well consider what other changes should be put in at the same time. > > Sam > _______________________________________________ > freebsd-current_at_freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-current > To unsubscribe, send any mail to > "freebsd-current-unsubscribe_at_freebsd.org"Received on Tue Jan 24 2006 - 18:33:52 UTC
This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:38:51 UTC