| Message ID | 20200712221625.287763-1-f.fainelli@gmail.com |
|---|---|
| Headers | show |
| Series | net: Preserve netdev_ops equality tests | expand |
On Sun, 12 Jul 2020 15:16:22 -0700 Florian Fainelli wrote: > Hi David, Jakub, > > This patch series addresses a long standing with no known impact today > with the overloading of netdev_ops done by the DSA layer. Do you plan to make use of this comparison? Or trying to protect the MAC driver from misbehaving because it's unaware the DSA may replace its ops? For non-DSA experts I think it may be worth stating :) > First we introduce a ndo_equal netdev_ops function pointer, then we have > DSA utilize it, and finally all in tree users are converted to using > either netdev_ops_equal() or __netdev_ops_equal() (for const struct > net_device reference). The experience with TCP ULPs made me dislike hijacking ops :( Maybe it's just my limited capability to comprehend complex systems but the moment there is more than one entity that tries to insert itself as a proxy, ordering etc. gets quite hairy.. Perhaps we have some well understood rules for ndo replacement but if that's not the case I prefer the interception to be done explicitly in the caller. (e.g. add separate dsa_ops to struct net_device and call that prior to/ /instead of calling the ndo). At the very least I'd think it's better to create an explicit hierarchy of the ops by linking them (add "const struct net_device_ops *base_ops" to ndos) rather than one-off callback for comparisons. That's just my 2ยข..
On 7/13/2020 1:09 PM, Jakub Kicinski wrote: > On Sun, 12 Jul 2020 15:16:22 -0700 Florian Fainelli wrote: >> Hi David, Jakub, >> >> This patch series addresses a long standing with no known impact today >> with the overloading of netdev_ops done by the DSA layer. > > Do you plan to make use of this comparison? Or trying to protect the > MAC driver from misbehaving because it's unaware the DSA may replace > its ops? For non-DSA experts I think it may be worth stating :) We have at least one network device driver that is always used in a DSA set-up (bcmsysport.c) which does check DSA notifiers against its own netdev_ops, however this happens before the mangling of DSA netdev_ops is done, so the check is not defeated there. > >> First we introduce a ndo_equal netdev_ops function pointer, then we have >> DSA utilize it, and finally all in tree users are converted to using >> either netdev_ops_equal() or __netdev_ops_equal() (for const struct >> net_device reference). > > The experience with TCP ULPs made me dislike hijacking ops :( > Maybe it's just my limited capability to comprehend complex systems > but the moment there is more than one entity that tries to insert > itself as a proxy, ordering etc. gets quite hairy.. Perhaps we > have some well understood rules for ndo replacement but if that's not > the case I prefer the interception to be done explicitly in the caller. > (e.g. add separate dsa_ops to struct net_device and call that prior to/ > /instead of calling the ndo). > > At the very least I'd think it's better to create an explicit hierarchy > of the ops by linking them (add "const struct net_device_ops *base_ops" > to ndos) rather than one-off callback for comparisons. Initially I was going to introduce a way to do recursive operations, but the problem with that approach is that you need to impose an ordering within the core about how operations are invoked. The idea to add dsa_ops as a singleton that DSA would provide (very much like the recent ethtool_phy_ops introduction) is probably the sanest approach. Thanks for the suggestions and review Jakub!