| Message ID | 20231213181554.4741-2-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: add define to describe link speed modes | expand |
NAK. You *clearly* didn't look before you leaped. On Wed, Dec 13, 2023 at 07:15:53PM +0100, Christian Marangi wrote: > +enum ethtool_link_speeds { > + SPEED_10 = 0, > + SPEED_100, > + SPEED_1000, ... and from the context immediately below, included in your patch: > #define SPEED_10 10 ^^^^^^^^ > #define SPEED_100 100 ^^^^^^^^^ > #define SPEED_1000 1000 ^^^^^^^^^^ Your enumerated values will be overridden by the preprocessor definitions. Moreover, SPEED_xxx is an already taken namespace and part of the UAPI, and thus can _not_ be changed. Convention is that SPEED_x will be defined as the numeric speed.
On Wed, Dec 13, 2023 at 08:10:42PM +0000, Russell King (Oracle) wrote: > NAK. > > You *clearly* didn't look before you leaped. > > On Wed, Dec 13, 2023 at 07:15:53PM +0100, Christian Marangi wrote: > > +enum ethtool_link_speeds { > > + SPEED_10 = 0, > > + SPEED_100, > > + SPEED_1000, > ... > > and from the context immediately below, included in your patch: > > #define SPEED_10 10 > ^^^^^^^^ > > #define SPEED_100 100 > ^^^^^^^^^ > > #define SPEED_1000 1000 > ^^^^^^^^^^ > > Your enumerated values will be overridden by the preprocessor > definitions. > > Moreover, SPEED_xxx is an already taken namespace and part of the UAPI, > and thus can _not_ be changed. Convention is that SPEED_x will be > defined as the numeric speed. > Well yes that is the idea of having the enum to count them and then redefining them to the correct value. (wasn't trying to introduce new define for the speed and trying to assign incremental values) Any idea how to handle this without the enum - redefine thing? Was trying to find a more automated way than defining the raw number of the current modes. (but maybe this is not that bad? since on adding more modes, other values has to be changed so it would be just another value to document in the comment)
On Wed, Dec 13, 2023 at 09:15:27PM +0100, Christian Marangi wrote: > On Wed, Dec 13, 2023 at 08:10:42PM +0000, Russell King (Oracle) wrote: > > NAK. > > > > You *clearly* didn't look before you leaped. > > > > On Wed, Dec 13, 2023 at 07:15:53PM +0100, Christian Marangi wrote: > > > +enum ethtool_link_speeds { > > > + SPEED_10 = 0, > > > + SPEED_100, > > > + SPEED_1000, > > ... > > > > and from the context immediately below, included in your patch: > > > #define SPEED_10 10 > > ^^^^^^^^ > > > #define SPEED_100 100 > > ^^^^^^^^^ > > > #define SPEED_1000 1000 > > ^^^^^^^^^^ > > > > Your enumerated values will be overridden by the preprocessor > > definitions. > > > > Moreover, SPEED_xxx is an already taken namespace and part of the UAPI, > > and thus can _not_ be changed. Convention is that SPEED_x will be > > defined as the numeric speed. > > > > Well yes that is the idea of having the enum to count them and then redefining > them to the correct value. (wasn't trying to introduce new define for > the speed and trying to assign incremental values) > > Any idea how to handle this without the enum - redefine thing? > > Was trying to find a more automated way than defining the raw number of > the current modes. (but maybe this is not that bad? since on adding more > modes, other values has to be changed so it would be just another value > to document in the comment) I think my comment on patch 2 gives some ideas! :D
Hi Christian,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/net-ethtool-add-define-for-link-speed-mode-number/20231214-021806
base: net-next/main
patch link: https://lore.kernel.org/r/20231213181554.4741-2-ansuelsmth%40gmail.com
patch subject: [net-next PATCH 1/2] net: ethtool: add define for link speed mode number
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20231214/202312141531.bEtUmIwG-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231214/202312141531.bEtUmIwG-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312141531.bEtUmIwG-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/net/ethernet/intel/igb/e1000_hw.h:13,
from drivers/net/ethernet/intel/igb/e1000_mac.h:7,
from drivers/net/ethernet/intel/igb/e1000_82575.c:14:
>> drivers/net/ethernet/intel/igb/e1000_defines.h:256:21: error: expected identifier before numeric constant
256 | #define SPEED_10 10
| ^~
include/uapi/linux/ethtool.h:1888:9: note: in expansion of macro 'SPEED_10'
1888 | SPEED_10 = 0,
| ^~~~~~~~
--
In file included from drivers/net/ethernet/intel/igc/igc_hw.h:12,
from drivers/net/ethernet/intel/igc/igc_base.c:6:
>> drivers/net/ethernet/intel/igc/igc_defines.h:231:33: error: expected identifier before numeric constant
231 | #define SPEED_10 10
| ^~
include/uapi/linux/ethtool.h:1888:9: note: in expansion of macro 'SPEED_10'
1888 | SPEED_10 = 0,
| ^~~~~~~~
vim +256 drivers/net/ethernet/intel/igb/e1000_defines.h
9d5c824399dea8 drivers/net/igb/e1000_defines.h Auke Kok 2008-01-24 255
9d5c824399dea8 drivers/net/igb/e1000_defines.h Auke Kok 2008-01-24 @256 #define SPEED_10 10
9d5c824399dea8 drivers/net/igb/e1000_defines.h Auke Kok 2008-01-24 257 #define SPEED_100 100
9d5c824399dea8 drivers/net/igb/e1000_defines.h Auke Kok 2008-01-24 258 #define SPEED_1000 1000
ceb5f13b70cd6e drivers/net/ethernet/intel/igb/e1000_defines.h Carolyn Wyborny 2013-04-18 259 #define SPEED_2500 2500
9d5c824399dea8 drivers/net/igb/e1000_defines.h Auke Kok 2008-01-24 260 #define HALF_DUPLEX 1
9d5c824399dea8 drivers/net/igb/e1000_defines.h Auke Kok 2008-01-24 261 #define FULL_DUPLEX 2
9d5c824399dea8 drivers/net/igb/e1000_defines.h Auke Kok 2008-01-24 262
9d5c824399dea8 drivers/net/igb/e1000_defines.h Auke Kok 2008-01-24 263
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index f7fba0dc87e5..59f394a663ab 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h @@ -1884,6 +1884,28 @@ enum ethtool_link_mode_bit_indices { * Update drivers/net/phy/phy.c:phy_speed_to_str() and * drivers/net/bonding/bond_3ad.c:__get_link_speed() when adding new values. */ +enum ethtool_link_speeds { + SPEED_10 = 0, + SPEED_100, + SPEED_1000, + SPEED_2500, + SPEED_5000, + SPEED_10000, + SPEED_14000, + SPEED_20000, + SPEED_25000, + SPEED_40000, + SPEED_50000, + SPEED_56000, + SPEED_100000, + SPEED_200000, + SPEED_400000, + SPEED_800000, + + /* must be last entry */ + __LINK_SPEEDS_NUM, +}; + #define SPEED_10 10 #define SPEED_100 100 #define SPEED_1000 1000
Add define to reference the number of link speed mode defined in the system. This can be handy for generic parsing of the different link speed mode. Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> --- include/uapi/linux/ethtool.h | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)