netdev - Re: [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Sun, 20 Aug 2023 16:45:05 +0200
From: Simon Horman <horms@...nel.org>
To: Paul Greenwalt <paul.greenwalt@...el.com>
Cc: intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
	pawel.chmielewski@...el.com, andrew@...n.ch, aelior@...vell.com,
	manishc@...vell.com
Subject: Re: [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported
 link modes maps

On Sat, Aug 19, 2023 at 02:39:41AM -0700, Paul Greenwalt wrote:
> The need to map Ethtool forced speeds to  Ethtool supported link modes is
> common among drivers. To support this move the supported link modes maps
> implementation from the qede driver. This is an efficient solution
> introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes
> maps on module init") for qede driver.
> 
> ethtool_forced_speed_maps_init() should be called during driver init
> with an array of struct ethtool_forced_speed_map to populate the
> mapping. The macro ETHTOOL_FORCED_SPEED_MAP is a helper to initialized
> the struct ethtool_forced_speed_map.
> 
> The qede driver was compile tested only.
> 
> Suggested-by: Andrew Lunn <andrew@...n.ch>
> Signed-off-by: Paul Greenwalt <paul.greenwalt@...el.com>
> ---
> v2: move qede Ethtool speed to link modes mapping to be shared by other
> drivers (Andrew)

Hi Paul,

thanks for your efforts in adding a mechanism to share code.
It's a great step in the right direction.

Perhaps I am on the wrong track here, but it seems to me that the approach
you have taken, which is to move the definitions of the symbols into a
header file, is, perhaps, not the best. For one thing it will end up with
duplicate definitions of the symbols - one for each object in which they
are included.

For another, and this more a symtom than an actual problem,
a (W=1) build now complains about symbols that are defined but not used.

./include/linux/ethtool.h:1190:18: warning: 'ethtool_forced_speed_800000' defined but not used [-Wunused-const-variable=]  1190 | static const u32 ethtool_forced_speed_800000[] __initconst = {
...

I suspect a better approach is to leave the symbol definitions in
a .c file, one that is linked in such a way that it is available
to code that uses it - be it modules or built-in. And to make
declarations of those symbols available via a header file.