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

From: "Greenwalt, Paul" <paul.greenwalt@intel.com>
To: Simon Horman <horms@kernel.org>
Cc: andrew@lunn.ch, aelior@marvell.com, manishc@marvell.com,
	netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v2 2/9] ethtool: Add forced speed to supported link modes maps
Date: Sun, 20 Aug 2023 10:29:46 -0700	[thread overview]
Message-ID: <12016e19-58d7-26d0-6964-0057435e84c5@intel.com> (raw)
In-Reply-To: <ZOIm8TJz/ykcI+PR@vergenet.net>

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@lunn.ch>
>> Signed-off-by: Paul Greenwalt <paul.greenwalt@intel.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.
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan