diff mbox series

[net-next,1/2] net: ethtool: add define for link speed mode number

Message ID 20231213181554.4741-2-ansuelsmth@gmail.com
State New
Headers
Series net: add define to describe link speed modes |

Commit Message

Christian Marangi Dec. 13, 2023, 6:15 p.m. UTC 
  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(+)

Comments

Russell King (Oracle) Dec. 13, 2023, 8:10 p.m. UTC | #1 
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.
Christian Marangi Dec. 13, 2023, 8:15 p.m. UTC | #2 
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)
Russell King (Oracle) Dec. 13, 2023, 8:23 p.m. UTC | #3 
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
kernel test robot Dec. 14, 2023, 7:35 a.m. UTC | #4 
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 mbox series

Patch

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