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: | ./scripts/bloat-o-meter old.o new.o > 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. Other drivers that have a dedicated function that assigns ethtool_ops only can be optimized IMHO. regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: PGP signature
