On Wed. 27 Jul. 2022 at 17:20, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote: > On 26.07.2022 18:59:18, Vincent MAILHOL wrote: > > > > Does it make sense? > > > > > > I have already used this scheme in the c_can driver. I used this > > > scheme because I saw that it was used a lot > > > (git grep set_ethtool_ops) in the kernel. > > > > | $ git grep "void .*_set_ethtool_ops.*;" | wc -l > > | 46 > > | $ git grep "extern const struct ethtool_ops" | wc -l > > | 43 > > > > I did not know it was a good practice, but you are right, both schemes > > are roughly as popular (with yours slightly more popular by a small > > margin). > > > > > By doing so you can define > > > slcan_ethtool_ops as a static variable > > > and if possible I prefer to export functions rather than data. But it > > > can be a matter of taste. > > > > My taste is to export the data (to remove a function call), but as the > > maintainer, your opinion should prevail here. > > I think with exporting the data instead of the function, the resulting > module will be a bit smaller. As we don't use LTO by default there's no > optimization between object files. The size of the resulting modules can > be checked with: Yes, the additional function call goes with additional assembly instruction. I did not mention it but this goes in pairs. > | ./scripts/bloat-o-meter old.o new.o $ ./scripts/bloat-o-meter drivers/net/can/slcan/slcan.old.o drivers/net/can/slcan/slcan.o add/remove: 0/1 grow/shrink: 1/0 up/down: 15/-29 (-14) Function old new delta slcan_open 1010 1025 +15 slcan_set_ethtool_ops 29 - -29 Total: Before=11115, After=11101, chg -0.13% slcan.old.o is the current one which uses the set_ethtool_ops() function, the slcan.o exports the structure instead. It saves 14 bytes (and a function call). > > And thanks for the explanation. > > > > I will also fix those two drivers: > > > > | $ git grep "void .*_set_ethtool_ops.*;" drivers/net/can/ > > | drivers/net/can/c_can/c_can.h:void c_can_set_ethtool_ops(struct > > net_device *dev); > > | drivers/net/can/flexcan/flexcan.h:void > > flexcan_set_ethtool_ops(struct net_device *dev); > > In the mcp251xfd driver there is mcp251xfd_ethtool_init(). This function > sets the ethtool_ops, but also initializes the parameters that can be > configured by ethtool (ring layout and coalescing) to default values. Yes, also the mcp251xfd is already fixed in v3. > Other drivers that have a dedicated function that assigns ethtool_ops > only can be optimized IMHO. Yes, but this is not related to the timestamp series. So if I do it, I will do it separately (and I do not commit that I will do it). Yours sincerely, Vincent Mailhol
