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 for Android: free password hash cracker in your pocket

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

Message-ID: <12016e19-58d7-26d0-6964-0057435e84c5@intel.com>
Date: Sun, 20 Aug 2023 10:29:46 -0700
From: "Greenwalt, Paul" <paul.greenwalt@...el.com>
To: Simon Horman <horms@...nel.org>
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 8/20/2023 7:45 AM, Simon Horman wrote:
> 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.

Simon, thanks for for your suggestion and I apologize the warning wasn't
caught.