On Mon, Jul 06, 2020 at 09:56:03PM +0200, Andrew Lunn wrote: > On Mon, Jul 06, 2020 at 11:40:00AM -0700, Jakub Kicinski wrote: > > On Sun, 5 Jul 2020 21:27:58 -0700 Florian Fainelli wrote: > > > + ops = ethtool_phy_ops; > > > + if (!ops || !ops->start_cable_test) { > > > > nit: don't think member-by-member checking is necessary. We don't > > expect there to be any alternative versions of the ops, right? > > I would not like to see anything else registering an ops. So i think > taking an Opps would be a good indication somebody is doing something > wrong and needs fixing. > > > We could even risk a direct call: > > > > #if IS_REACHABLE(CONFIG_PHYLIB) > > static inline int do_x() > > { > > return __do_x(); > > } > > #else > > static inline int do_x() > > { > > if (!ops) > > return -EOPNOTSUPP; > > return ops->do_x(); > > } > > #endif > > > > But that's perhaps doing too much... > > I would say it is too far. Two ways of doing the same thing requires > twice as much testing. And these are not hot paths where we want to > eliminate as many instructions and trampolines as possible.
Agreed, it seems a bit over the top. Michal