| Message ID | 1551020901-20257-1-git-send-email-tariqt@mellanox.com |
|---|---|
| State | Superseded |
| Delegated to: | John Linville |
| Headers | show |
| Series | [ethtool] ethtool: Add support for 200Gbps (50Gbps per lane) link mode | expand |
On Sun, Feb 24, 2019 at 05:08:21PM +0200, Tariq Toukan wrote: > From: Aya Levin <ayal@mellanox.com> > index 5a26cff5fb33..64ce0711ad5f 100644 > --- a/ethtool.8.in > +++ b/ethtool.8.in > @@ -650,6 +650,11 @@ lB l lB. > 0x400000000 50000baseCR2 Full > 0x800000000 50000baseKR2 Full > 0x10000000000 50000baseSR2 Full > +0x10000000000000 50000baseKR Full > +0x20000000000000 50000baseSR Full > +0x40000000000000 50000baseCR Full > +0x80000000000000 50000baseLR_ER_FR Full > +0x100000000000000 50000baseDR Full > 0x8000000 56000baseKR4 Full > 0x10000000 56000baseCR4 Full > 0x20000000 56000baseSR4 Full > @@ -658,6 +663,16 @@ lB l lB. > 0x2000000000 100000baseSR4 Full > 0x4000000000 100000baseCR4 Full > 0x8000000000 100000baseLR4_ER4 Full > +0x200000000000000 100000baseKR2 Full > +0x400000000000000 100000baseSR2 Full > +0x800000000000000 100000baseCR2 Full > +0x1000000000000000 100000baseLR2_ER2_FR2 Full > +0x2000000000000000 100000baseDR2 Full > +0x4000000000000000 200000baseKR4 Full > +0x8000000000000000 200000baseSR4 Full > +0x10000000000000000 200000baseLR4_ER4_FR4 Full > +0x20000000000000000 200000baseDR4 Full > +0x40000000000000000 200000baseCR4 Full This is getting less friendly all the time, and it was never very friendly to start with. We have the strings which represent these link modes in the table used for dumping caps. How about allowing the user to list a comma separate list of modes. ethtool -s lan42 advertise 100000baseKR2/Full,100000baseSR2/Full,100000baseCR2/Full Andrew > + adv_bit = ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT & > + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT & > + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT & > + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT & > + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT; Maybe i'm wrong, but this looks odd. enum ethtool_link_mode_bit_indices { ETHTOOL_LINK_MODE_10baseT_Half_BIT = 0, ETHTOOL_LINK_MODE_10baseT_Full_BIT = 1, ETHTOOL_LINK_MODE_100baseT_Half_BIT = 2, ETHTOOL_LINK_MODE_100baseT_Full_BIT = 3, ETHTOOL_LINK_MODE_1000baseT_Half_BIT = 4, ETHTOOL_LINK_MODE_1000baseT_Full_BIT = 5, These are numbers, not bitmasks, so & them together does not look correct. Andrew
On Sun, Feb 24, 2019 at 05:08:21PM +0200, Tariq Toukan wrote: > From: Aya Levin <ayal@mellanox.com> > > Introduce 50Gbps per lane link modes and 200Gbps speed, update print > functions and initialization functions accordingly. > In addition, update related man page accordingly. > > Signed-off-by: Aya Levin <ayal@mellanox.com> > Signed-off-by: Tariq Toukan <tariqt@mellanox.com> > --- > ethtool-copy.h | 19 ++++++++++++++++++- > ethtool.8.in | 15 +++++++++++++++ > ethtool.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 85 insertions(+), 1 deletion(-) > > diff --git a/ethtool-copy.h b/ethtool-copy.h > index 6bfbb85f9402..de459900b2d3 100644 > --- a/ethtool-copy.h > +++ b/ethtool-copy.h > @@ -1455,15 +1455,31 @@ enum ethtool_link_mode_bit_indices { > ETHTOOL_LINK_MODE_FEC_NONE_BIT = 49, > ETHTOOL_LINK_MODE_FEC_RS_BIT = 50, > ETHTOOL_LINK_MODE_FEC_BASER_BIT = 51, > + ETHTOOL_LINK_MODE_50000baseKR_Full_BIT = 52, > + ETHTOOL_LINK_MODE_50000baseSR_Full_BIT = 53, > + ETHTOOL_LINK_MODE_50000baseCR_Full_BIT = 54, > + ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT = 55, > + ETHTOOL_LINK_MODE_50000baseDR_Full_BIT = 56, > + ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT = 57, > + ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT = 58, > + ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT = 59, > + ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT = 60, > + ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT = 61, > + ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT = 62, > + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT = 63, > + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT = 64, > + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT = 65, > + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT = 66, > > /* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit > * 31. Please do NOT define any SUPPORTED_* or ADVERTISED_* > * macro for bits > 31. The only way to use indices > 31 is to > * use the new ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API. > */ > + ETHTOOL_LINK_MODE_LAST, > > __ETHTOOL_LINK_MODE_LAST > - = ETHTOOL_LINK_MODE_FEC_BASER_BIT, > + = ETHTOOL_LINK_MODE_LAST - 1, > }; The name ETHTOOL_LINK_MODE_LAST is a bit misleading, maybe it should rather be called ETHTOOL_LINK_MODE_COUNT. Also, there should be parentheses around the expression to avoid surprises when __ETHTOOL_LINK_MODE_LAST is used in an expression. But this change is not in kernel include/uapi/linux/ethtool.h in mainline, net or net-next. As ethtool-copy.h is supposed to be a copy of sanitized kernel uapi header, you should make the change there first and then sync the header to ethtool. Michal Kubecek
On Sun, Feb 24, 2019 at 05:47:51PM +0100, Andrew Lunn wrote: > On Sun, Feb 24, 2019 at 05:08:21PM +0200, Tariq Toukan wrote: > > From: Aya Levin <ayal@mellanox.com> > > index 5a26cff5fb33..64ce0711ad5f 100644 > > --- a/ethtool.8.in > > +++ b/ethtool.8.in > > @@ -650,6 +650,11 @@ lB l lB. > > 0x400000000 50000baseCR2 Full > > 0x800000000 50000baseKR2 Full > > 0x10000000000 50000baseSR2 Full > > +0x10000000000000 50000baseKR Full > > +0x20000000000000 50000baseSR Full > > +0x40000000000000 50000baseCR Full > > +0x80000000000000 50000baseLR_ER_FR Full > > +0x100000000000000 50000baseDR Full > > 0x8000000 56000baseKR4 Full > > 0x10000000 56000baseCR4 Full > > 0x20000000 56000baseSR4 Full > > @@ -658,6 +663,16 @@ lB l lB. > > 0x2000000000 100000baseSR4 Full > > 0x4000000000 100000baseCR4 Full > > 0x8000000000 100000baseLR4_ER4 Full > > +0x200000000000000 100000baseKR2 Full > > +0x400000000000000 100000baseSR2 Full > > +0x800000000000000 100000baseCR2 Full > > +0x1000000000000000 100000baseLR2_ER2_FR2 Full > > +0x2000000000000000 100000baseDR2 Full > > +0x4000000000000000 200000baseKR4 Full > > +0x8000000000000000 200000baseSR4 Full > > +0x10000000000000000 200000baseLR4_ER4_FR4 Full > > +0x20000000000000000 200000baseDR4 Full > > +0x40000000000000000 200000baseCR4 Full > > This is getting less friendly all the time, and it was never very > friendly to start with. We have the strings which represent these link > modes in the table used for dumping caps. How about allowing the user > to list a comma separate list of modes. > > ethtool -s lan42 advertise 100000baseKR2/Full,100000baseSR2/Full,100000baseCR2/Full In my preliminary netlink patchset, I'm adding support for e.g. ethtool -s eth0 advertise 100baseT/Half off 1000baseT/Full on I'm not sure what would be more useful, if switching individual modes or listing the whole set. After all, we can also support both. But I fully agree that the numeric bitmaps are already too inconvenient. > > + adv_bit = ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT & > > + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT & > > + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT & > > + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT & > > + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT; > > Maybe i'm wrong, but this looks odd. > > enum ethtool_link_mode_bit_indices { > ETHTOOL_LINK_MODE_10baseT_Half_BIT = 0, > ETHTOOL_LINK_MODE_10baseT_Full_BIT = 1, > ETHTOOL_LINK_MODE_100baseT_Half_BIT = 2, > ETHTOOL_LINK_MODE_100baseT_Full_BIT = 3, > ETHTOOL_LINK_MODE_1000baseT_Half_BIT = 4, > ETHTOOL_LINK_MODE_1000baseT_Full_BIT = 5, > > These are numbers, not bitmasks, so & them together does not look > correct. Yes, this is wrong. Even if bit masks were used, the operator should be "|" but here adv_bit is interpreted as an index, not mask. It's obvious this part of the code was designed when speed and duplex identified the mode uniquely which is no longer the case. (Which is probably also why there are no branches for speeds over 10G.) And there is another problem: + else if (speed_wanted == SPEED_20000 && + duplex_wanted == DUPLEX_FULL) + adv_bit = ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT & + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT & + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT & + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT & + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT; The test is for SPEED_20000 but then 200G modes are added. Michal
> > This is getting less friendly all the time, and it was never very > > friendly to start with. We have the strings which represent these link > > modes in the table used for dumping caps. How about allowing the user > > to list a comma separate list of modes. > > > > ethtool -s lan42 advertise 100000baseKR2/Full,100000baseSR2/Full,100000baseCR2/Full > > In my preliminary netlink patchset, I'm adding support for e.g. > > ethtool -s eth0 advertise 100baseT/Half off 1000baseT/Full on > > I'm not sure what would be more useful, if switching individual modes or > listing the whole set. After all, we can also support both. But I fully > agree that the numeric bitmaps are already too inconvenient. Hi Michal So are you doing a read/modify/write? In that case, off/on makes sense. For a pure write, i don't see the need for off/on. I've not had to use this much, so i don't know how it is typically used. When i have used it, it is because i've got an SFP which can do 1G and 2.5G, but the peer can only do 1G. I've needed to remove the 2.5G in order to get link. So in that case, read/modify/write with an off would make sense. Andrew > And there is another problem: > > + else if (speed_wanted == SPEED_20000 && > + duplex_wanted == DUPLEX_FULL) > + adv_bit = ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT & > + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT & > + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT & > + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT & > + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT; > > The test is for SPEED_20000 but then 200G modes are added. Oh, yes. Easy to miss. Maybe we should consider adding aliases, #define SPEED_200G SPEED_200000, and #define ETHTOOL_LINK_MODE_200GbaseCR4_Full_BIT ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT Andrew
On Sun, Feb 24, 2019 at 08:40:15PM +0100, Andrew Lunn wrote: > > > This is getting less friendly all the time, and it was never very > > > friendly to start with. We have the strings which represent these link > > > modes in the table used for dumping caps. How about allowing the user > > > to list a comma separate list of modes. > > > > > > ethtool -s lan42 advertise 100000baseKR2/Full,100000baseSR2/Full,100000baseCR2/Full > > > > In my preliminary netlink patchset, I'm adding support for e.g. > > > > ethtool -s eth0 advertise 100baseT/Half off 1000baseT/Full on > > > > I'm not sure what would be more useful, if switching individual modes or > > listing the whole set. After all, we can also support both. But I fully > > agree that the numeric bitmaps are already too inconvenient. > > Hi Michal > > So are you doing a read/modify/write? In that case, off/on makes > sense. For a pure write, i don't see the need for off/on. When using netlink interface, the read/modify/write cycle is limited to kernel code and is done under rtnl_lock. The netlink interface allows userspace to send only attributes it wants to change and for bit sets (like link modes) to tell kernel which bits it wants to change so that there is no need to read current values first (and open a race window). When using ioctl interface, ethtool does read the value first even now as ETHTOOL_SLINKSETTINGS command uses struct ethtool_link_usettings which has also other members and there is no way to say we only want to change advertised link modes. Michal
> > Hi Michal > > > > So are you doing a read/modify/write? In that case, off/on makes > > sense. For a pure write, i don't see the need for off/on. > > When using netlink interface, the read/modify/write cycle is limited to > kernel code and is done under rtnl_lock. The netlink interface allows > userspace to send only attributes it wants to change and for bit sets > (like link modes) to tell kernel which bits it wants to change so that > there is no need to read current values first (and open a race window). Nice. But it would still be good to get feedback from people who use this more often. I guess that is top of rack switches with all these odd link modes. Andrew
On Sun, Feb 24, 2019 at 07:11:04PM +0100, Michal Kubecek wrote: > On Sun, Feb 24, 2019 at 05:08:21PM +0200, Tariq Toukan wrote: > > From: Aya Levin <ayal@mellanox.com> > > > > Introduce 50Gbps per lane link modes and 200Gbps speed, update print > > functions and initialization functions accordingly. > > In addition, update related man page accordingly. > > > > Signed-off-by: Aya Levin <ayal@mellanox.com> > > Signed-off-by: Tariq Toukan <tariqt@mellanox.com> > > --- > > ethtool-copy.h | 19 ++++++++++++++++++- > > ethtool.8.in | 15 +++++++++++++++ > > ethtool.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 85 insertions(+), 1 deletion(-) > > > > diff --git a/ethtool-copy.h b/ethtool-copy.h > > index 6bfbb85f9402..de459900b2d3 100644 > > --- a/ethtool-copy.h > > +++ b/ethtool-copy.h > > @@ -1455,15 +1455,31 @@ enum ethtool_link_mode_bit_indices { > > ETHTOOL_LINK_MODE_FEC_NONE_BIT = 49, > > ETHTOOL_LINK_MODE_FEC_RS_BIT = 50, > > ETHTOOL_LINK_MODE_FEC_BASER_BIT = 51, > > + ETHTOOL_LINK_MODE_50000baseKR_Full_BIT = 52, > > + ETHTOOL_LINK_MODE_50000baseSR_Full_BIT = 53, > > + ETHTOOL_LINK_MODE_50000baseCR_Full_BIT = 54, > > + ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT = 55, > > + ETHTOOL_LINK_MODE_50000baseDR_Full_BIT = 56, > > + ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT = 57, > > + ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT = 58, > > + ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT = 59, > > + ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT = 60, > > + ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT = 61, > > + ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT = 62, > > + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT = 63, > > + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT = 64, > > + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT = 65, > > + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT = 66, > > > > /* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit > > * 31. Please do NOT define any SUPPORTED_* or ADVERTISED_* > > * macro for bits > 31. The only way to use indices > 31 is to > > * use the new ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API. > > */ > > + ETHTOOL_LINK_MODE_LAST, > > > > __ETHTOOL_LINK_MODE_LAST > > - = ETHTOOL_LINK_MODE_FEC_BASER_BIT, > > + = ETHTOOL_LINK_MODE_LAST - 1, > > }; > > The name ETHTOOL_LINK_MODE_LAST is a bit misleading, maybe it should > rather be called ETHTOOL_LINK_MODE_COUNT. Also, there should be > parentheses around the expression to avoid surprises when > __ETHTOOL_LINK_MODE_LAST is used in an expression. > > But this change is not in kernel include/uapi/linux/ethtool.h in > mainline, net or net-next. As ethtool-copy.h is supposed to be a copy of > sanitized kernel uapi header, you should make the change there first and > then sync the header to ethtool. For the record, net-next tree now has simplified this even more with commit e728fdf06289 ("net: phy: improve definition of __ETHTOOL_LINK_MODE_MASK_NBITS"). Michal Kubecek
On 2/24/2019 8:40 PM, Michal Kubecek wrote: > On Sun, Feb 24, 2019 at 05:47:51PM +0100, Andrew Lunn wrote: >> On Sun, Feb 24, 2019 at 05:08:21PM +0200, Tariq Toukan wrote: >>> From: Aya Levin <ayal@mellanox.com> >>> index 5a26cff5fb33..64ce0711ad5f 100644 >>> --- a/ethtool.8.in >>> +++ b/ethtool.8.in >>> @@ -650,6 +650,11 @@ lB l lB. >>> 0x400000000 50000baseCR2 Full >>> 0x800000000 50000baseKR2 Full >>> 0x10000000000 50000baseSR2 Full >>> +0x10000000000000 50000baseKR Full >>> +0x20000000000000 50000baseSR Full >>> +0x40000000000000 50000baseCR Full >>> +0x80000000000000 50000baseLR_ER_FR Full >>> +0x100000000000000 50000baseDR Full >>> 0x8000000 56000baseKR4 Full >>> 0x10000000 56000baseCR4 Full >>> 0x20000000 56000baseSR4 Full >>> @@ -658,6 +663,16 @@ lB l lB. >>> 0x2000000000 100000baseSR4 Full >>> 0x4000000000 100000baseCR4 Full >>> 0x8000000000 100000baseLR4_ER4 Full >>> +0x200000000000000 100000baseKR2 Full >>> +0x400000000000000 100000baseSR2 Full >>> +0x800000000000000 100000baseCR2 Full >>> +0x1000000000000000 100000baseLR2_ER2_FR2 Full >>> +0x2000000000000000 100000baseDR2 Full >>> +0x4000000000000000 200000baseKR4 Full >>> +0x8000000000000000 200000baseSR4 Full >>> +0x10000000000000000 200000baseLR4_ER4_FR4 Full >>> +0x20000000000000000 200000baseDR4 Full >>> +0x40000000000000000 200000baseCR4 Full >> >> This is getting less friendly all the time, and it was never very >> friendly to start with. We have the strings which represent these link >> modes in the table used for dumping caps. How about allowing the user >> to list a comma separate list of modes. >> >> ethtool -s lan42 advertise 100000baseKR2/Full,100000baseSR2/Full,100000baseCR2/Full > > In my preliminary netlink patchset, I'm adding support for e.g. > > ethtool -s eth0 advertise 100baseT/Half off 1000baseT/Full on > > I'm not sure what would be more useful, if switching individual modes or > listing the whole set. After all, we can also support both. But I fully > agree that the numeric bitmaps are already too inconvenient. > >>> + adv_bit = ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT & >>> + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT & >>> + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT & >>> + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT & >>> + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT; >> >> Maybe i'm wrong, but this looks odd. >> >> enum ethtool_link_mode_bit_indices { >> ETHTOOL_LINK_MODE_10baseT_Half_BIT = 0, >> ETHTOOL_LINK_MODE_10baseT_Full_BIT = 1, >> ETHTOOL_LINK_MODE_100baseT_Half_BIT = 2, >> ETHTOOL_LINK_MODE_100baseT_Full_BIT = 3, >> ETHTOOL_LINK_MODE_1000baseT_Half_BIT = 4, >> ETHTOOL_LINK_MODE_1000baseT_Full_BIT = 5, >> >> These are numbers, not bitmasks, so & them together does not look >> correct. > > Yes, this is wrong. Even if bit masks were used, the operator should be > "|" but here adv_bit is interpreted as an index, not mask. It's obvious > this part of the code was designed when speed and duplex identified the > mode uniquely which is no longer the case. (Which is probably also why > there are no branches for speeds over 10G.) > > And there is another problem: > > + else if (speed_wanted == SPEED_20000 && > + duplex_wanted == DUPLEX_FULL) > + adv_bit = ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT & > + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT & > + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT & > + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT & > + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT; > > The test is for SPEED_20000 but then 200G modes are added. > > Michal > Thanks Michal and Andrew for your comments - I will fix them in the next version. The code section (setting advertisement of 200G) will be removed completely, advertisement of 200G will be set to the supported link-modes that is handled below. In addition I will make sure ethtool-copy.h is synced with current kernel version without additions. As for the man page, I agree with you completely - I thought of replacing the LONG hex values with BIT(x) but since this can not be applied in the command line - I didn't implement it. Aya
On 2/24/2019 9:40 PM, Andrew Lunn wrote: >>> This is getting less friendly all the time, and it was never very >>> friendly to start with. We have the strings which represent these link >>> modes in the table used for dumping caps. How about allowing the user >>> to list a comma separate list of modes. >>> >>> ethtool -s lan42 advertise 100000baseKR2/Full,100000baseSR2/Full,100000baseCR2/Full >> >> In my preliminary netlink patchset, I'm adding support for e.g. >> >> ethtool -s eth0 advertise 100baseT/Half off 1000baseT/Full on >> >> I'm not sure what would be more useful, if switching individual modes or >> listing the whole set. After all, we can also support both. But I fully >> agree that the numeric bitmaps are already too inconvenient. > > Hi Michal > > So are you doing a read/modify/write? In that case, off/on makes > sense. For a pure write, i don't see the need for off/on. > > I've not had to use this much, so i don't know how it is typically > used. When i have used it, it is because i've got an SFP which can do > 1G and 2.5G, but the peer can only do 1G. I've needed to remove the > 2.5G in order to get link. So in that case, read/modify/write with an > off would make sense. > > Andrew > >> And there is another problem: >> >> + else if (speed_wanted == SPEED_20000 && >> + duplex_wanted == DUPLEX_FULL) >> + adv_bit = ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT & >> + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT & >> + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT & >> + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT & >> + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT; >> >> The test is for SPEED_20000 but then 200G modes are added. > > Oh, yes. Easy to miss. Maybe we should consider adding aliases, > #define SPEED_200G SPEED_200000, and Totally agree. This will help. > > #define ETHTOOL_LINK_MODE_200GbaseCR4_Full_BIT ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT In case you do it please consider adding an underscore after the speed, so it becomes ETHTOOL_LINK_MODE_200G_baseCR4_Full_BIT. I think it's more convenient for the human eye.
diff --git a/ethtool-copy.h b/ethtool-copy.h index 6bfbb85f9402..de459900b2d3 100644 --- a/ethtool-copy.h +++ b/ethtool-copy.h @@ -1455,15 +1455,31 @@ enum ethtool_link_mode_bit_indices { ETHTOOL_LINK_MODE_FEC_NONE_BIT = 49, ETHTOOL_LINK_MODE_FEC_RS_BIT = 50, ETHTOOL_LINK_MODE_FEC_BASER_BIT = 51, + ETHTOOL_LINK_MODE_50000baseKR_Full_BIT = 52, + ETHTOOL_LINK_MODE_50000baseSR_Full_BIT = 53, + ETHTOOL_LINK_MODE_50000baseCR_Full_BIT = 54, + ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT = 55, + ETHTOOL_LINK_MODE_50000baseDR_Full_BIT = 56, + ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT = 57, + ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT = 58, + ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT = 59, + ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT = 60, + ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT = 61, + ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT = 62, + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT = 63, + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT = 64, + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT = 65, + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT = 66, /* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit * 31. Please do NOT define any SUPPORTED_* or ADVERTISED_* * macro for bits > 31. The only way to use indices > 31 is to * use the new ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API. */ + ETHTOOL_LINK_MODE_LAST, __ETHTOOL_LINK_MODE_LAST - = ETHTOOL_LINK_MODE_FEC_BASER_BIT, + = ETHTOOL_LINK_MODE_LAST - 1, }; #define __ETHTOOL_LINK_MODE_LEGACY_MASK(base_name) \ @@ -1571,6 +1587,7 @@ enum ethtool_link_mode_bit_indices { #define SPEED_50000 50000 #define SPEED_56000 56000 #define SPEED_100000 100000 +#define SPEED_200000 200000 #define SPEED_UNKNOWN -1 diff --git a/ethtool.8.in b/ethtool.8.in index 5a26cff5fb33..64ce0711ad5f 100644 --- a/ethtool.8.in +++ b/ethtool.8.in @@ -650,6 +650,11 @@ lB l lB. 0x400000000 50000baseCR2 Full 0x800000000 50000baseKR2 Full 0x10000000000 50000baseSR2 Full +0x10000000000000 50000baseKR Full +0x20000000000000 50000baseSR Full +0x40000000000000 50000baseCR Full +0x80000000000000 50000baseLR_ER_FR Full +0x100000000000000 50000baseDR Full 0x8000000 56000baseKR4 Full 0x10000000 56000baseCR4 Full 0x20000000 56000baseSR4 Full @@ -658,6 +663,16 @@ lB l lB. 0x2000000000 100000baseSR4 Full 0x4000000000 100000baseCR4 Full 0x8000000000 100000baseLR4_ER4 Full +0x200000000000000 100000baseKR2 Full +0x400000000000000 100000baseSR2 Full +0x800000000000000 100000baseCR2 Full +0x1000000000000000 100000baseLR2_ER2_FR2 Full +0x2000000000000000 100000baseDR2 Full +0x4000000000000000 200000baseKR4 Full +0x8000000000000000 200000baseSR4 Full +0x10000000000000000 200000baseLR4_ER4_FR4 Full +0x20000000000000000 200000baseDR4 Full +0x40000000000000000 200000baseCR4 Full .TE .TP .BI phyad \ N diff --git a/ethtool.c b/ethtool.c index fb4c0886ca84..1a23be818ec1 100644 --- a/ethtool.c +++ b/ethtool.c @@ -530,6 +530,21 @@ static void init_global_link_mode_masks(void) ETHTOOL_LINK_MODE_10000baseER_Full_BIT, ETHTOOL_LINK_MODE_2500baseT_Full_BIT, ETHTOOL_LINK_MODE_5000baseT_Full_BIT, + ETHTOOL_LINK_MODE_50000baseKR_Full_BIT, + ETHTOOL_LINK_MODE_50000baseSR_Full_BIT, + ETHTOOL_LINK_MODE_50000baseCR_Full_BIT, + ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT, + ETHTOOL_LINK_MODE_50000baseDR_Full_BIT, + ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT, + ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT, + ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT, + ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT, + ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT, + ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT, + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT, + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT, + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT, + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT, }; static const enum ethtool_link_mode_bit_indices additional_advertised_flags_bits[] = { @@ -689,6 +704,36 @@ static void dump_link_caps(const char *prefix, const char *an_prefix, "2500baseT/Full" }, { 0, ETHTOOL_LINK_MODE_5000baseT_Full_BIT, "5000baseT/Full" }, + { 0, ETHTOOL_LINK_MODE_50000baseKR_Full_BIT, + "50000baseKR/Full" }, + { 0, ETHTOOL_LINK_MODE_50000baseSR_Full_BIT, + "50000baseSR/Full" }, + { 0, ETHTOOL_LINK_MODE_50000baseCR_Full_BIT, + "50000baseCR/Full" }, + { 0, ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT, + "50000baseLR_ER_FR/Full" }, + { 0, ETHTOOL_LINK_MODE_50000baseDR_Full_BIT, + "50000baseDR/Full" }, + { 0, ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT, + "100000baseKR2/Full" }, + { 0, ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT, + "100000baseSR2/Full" }, + { 0, ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT, + "100000baseCR2/Full" }, + { 0, ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT, + "100000baseLR2_ER2_FR2/Full" }, + { 0, ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT, + "100000baseDR2/Full" }, + { 0, ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT, + "200000baseKR4/Full" }, + { 0, ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT, + "200000baseSR4/Full" }, + { 0, ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT, + "200000baseLR4_ER4_FR4/Full" }, + { 0, ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT, + "200000baseDR4/Full" }, + { 0, ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT, + "200000baseCR4/Full" }, }; int indent; int did1, new_line_pend, i; @@ -2958,6 +3003,13 @@ static int do_sset(struct cmd_context *ctx) else if (speed_wanted == SPEED_10000 && duplex_wanted == DUPLEX_FULL) adv_bit = ETHTOOL_LINK_MODE_10000baseT_Full_BIT; + else if (speed_wanted == SPEED_20000 && + duplex_wanted == DUPLEX_FULL) + adv_bit = ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT & + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT & + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT & + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT & + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT; if (adv_bit >= 0) { advertising_wanted = mask_advertising_wanted;