* Broken link partner advertised reporting in ethtool @ 2020-07-27 15:47 Jamie Gloudon 2020-07-27 19:19 ` Jacob Keller 0 siblings, 1 reply; 13+ messages in thread From: Jamie Gloudon @ 2020-07-27 15:47 UTC (permalink / raw) To: netdev Hey, While having a discussion with Sasha from Intel. I noticed link partner advertised support is broken in ethtool 5.7. Sasha hinted to me, the new API that ethtool is using. I see the actual cause in dump_peer_modes() in netlink/settings.c, that the mask parameter is set to false for dump_link_modes, dump_pause and bitset_get_bit. Regards, Jamie Gloudon ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Broken link partner advertised reporting in ethtool 2020-07-27 15:47 Broken link partner advertised reporting in ethtool Jamie Gloudon @ 2020-07-27 19:19 ` Jacob Keller 2020-07-27 20:09 ` Jamie Gloudon 0 siblings, 1 reply; 13+ messages in thread From: Jacob Keller @ 2020-07-27 19:19 UTC (permalink / raw) To: Jamie Gloudon, netdev On 7/27/2020 8:47 AM, Jamie Gloudon wrote: > Hey, > > While having a discussion with Sasha from Intel. I noticed link partner > advertised support is broken in ethtool 5.7. Sasha hinted to me, the > new API that ethtool is using. > > I see the actual cause in dump_peer_modes() in netlink/settings.c, that > the mask parameter is set to false for dump_link_modes, dump_pause and > bitset_get_bit. > > Regards, > Jamie Gloudon > Hi, Seems like more detail here would be useful. This is about the ethtool application. Answering the following questions would help: - what you wanted to achieve; - what you did (including what versions of software and the command sequence to reproduce the behavior); - what you saw happen; - what you expected to see; and - how the last two are different. The mask parameter for dump_link_modes is used to select between displaying the mask and the value for a bitset. According to the source in filling the LINKMODES_PEER, we actually don't send a mask at all with this setting, so using true for the mask in dump_link_modes here seems like it would be wrong. It appears that to get link partner settings your driver must fill in lp_advertising. If you're referring to an Intel driver, a quick search over drivers/net/ethernet/intel shows that only the ice driver currently supports reporting this information. Given this, I am not convinced there is a bug in ethtool. Thanks, Jake ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Broken link partner advertised reporting in ethtool 2020-07-27 19:19 ` Jacob Keller @ 2020-07-27 20:09 ` Jamie Gloudon 2020-07-27 20:42 ` Michal Kubecek 0 siblings, 1 reply; 13+ messages in thread From: Jamie Gloudon @ 2020-07-27 20:09 UTC (permalink / raw) To: Jacob Keller; +Cc: netdev On Mon, Jul 27, 2020 at 12:19:13PM -0700, Jacob Keller wrote: > > > On 7/27/2020 8:47 AM, Jamie Gloudon wrote: > > Hey, > > > > While having a discussion with Sasha from Intel. I noticed link partner > > advertised support is broken in ethtool 5.7. Sasha hinted to me, the > > new API that ethtool is using. > > > > I see the actual cause in dump_peer_modes() in netlink/settings.c, that > > the mask parameter is set to false for dump_link_modes, dump_pause and > > bitset_get_bit. > > > > Regards, > > Jamie Gloudon > > > > Hi, > > Seems like more detail here would be useful. This is about the ethtool > application. > > Answering the following questions would help: > > - what you wanted to achieve; > > - what you did (including what versions of software and the command > sequence to reproduce the behavior); > > - what you saw happen; > > - what you expected to see; and > > - how the last two are different. > > The mask parameter for dump_link_modes is used to select between > displaying the mask and the value for a bitset. > > According to the source in filling the LINKMODES_PEER, we actually don't > send a mask at all with this setting, so using true for the mask in > dump_link_modes here seems like it would be wrong. > > It appears that to get link partner settings your driver must fill in > lp_advertising. If you're referring to an Intel driver, a quick search > over drivers/net/ethernet/intel shows that only the ice driver currently > supports reporting this information. > > Given this, I am not convinced there is a bug in ethtool. > > Thanks, > Jake I am using r8169 with phy driver which actually fills lp_advertising. I recompiled ethtool v5.7 with --disable-netlink and "Link partner advertised link modes" works as it should. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Broken link partner advertised reporting in ethtool 2020-07-27 20:09 ` Jamie Gloudon @ 2020-07-27 20:42 ` Michal Kubecek 2020-07-27 21:01 ` Andrew Lunn 0 siblings, 1 reply; 13+ messages in thread From: Michal Kubecek @ 2020-07-27 20:42 UTC (permalink / raw) To: Jamie Gloudon; +Cc: Jacob Keller, netdev On Mon, Jul 27, 2020 at 04:09:12PM -0400, Jamie Gloudon wrote: > On Mon, Jul 27, 2020 at 12:19:13PM -0700, Jacob Keller wrote: > > > > > > On 7/27/2020 8:47 AM, Jamie Gloudon wrote: > > > Hey, > > > > > > While having a discussion with Sasha from Intel. I noticed link partner > > > advertised support is broken in ethtool 5.7. Sasha hinted to me, the > > > new API that ethtool is using. > > > > > > I see the actual cause in dump_peer_modes() in netlink/settings.c, that > > > the mask parameter is set to false for dump_link_modes, dump_pause and > > > bitset_get_bit. > > > > > > Regards, > > > Jamie Gloudon > > > > > > > Hi, > > > > Seems like more detail here would be useful. This is about the ethtool > > application. > > > > Answering the following questions would help: > > > > - what you wanted to achieve; > > > > - what you did (including what versions of software and the command > > sequence to reproduce the behavior); > > > > - what you saw happen; > > > > - what you expected to see; and > > > > - how the last two are different. > > > > The mask parameter for dump_link_modes is used to select between > > displaying the mask and the value for a bitset. > > > > According to the source in filling the LINKMODES_PEER, we actually don't > > send a mask at all with this setting, so using true for the mask in > > dump_link_modes here seems like it would be wrong. > > > > It appears that to get link partner settings your driver must fill in > > lp_advertising. If you're referring to an Intel driver, a quick search > > over drivers/net/ethernet/intel shows that only the ice driver currently > > supports reporting this information. > > > > Given this, I am not convinced there is a bug in ethtool. > > > > Thanks, > > Jake > > I am using r8169 with phy driver which actually fills lp_advertising. > I recompiled ethtool v5.7 with --disable-netlink and "Link partner > advertised link modes" works as it should. Please keep in mind that unlike you, we are not familiar with the issue and we don't know what exactly you did, what output you expected and what you got. Also, the issue may be reproducible only with a specific hardware. What would be definitely helpful would be - the exact command you ran (including arguments) - expected output (or at least the relevant part) - actual output (or at least the relevant part) - output with dump of netlink messages, you can get it by enabling debugging flags, e.g. "ethtool --debug 0x12 eth0" Michal ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Broken link partner advertised reporting in ethtool 2020-07-27 20:42 ` Michal Kubecek @ 2020-07-27 21:01 ` Andrew Lunn 2020-07-27 21:08 ` Michal Kubecek 2020-07-27 21:18 ` Jacob Keller 0 siblings, 2 replies; 13+ messages in thread From: Andrew Lunn @ 2020-07-27 21:01 UTC (permalink / raw) To: Michal Kubecek; +Cc: Jamie Gloudon, Jacob Keller, netdev > - the exact command you ran (including arguments) > - expected output (or at least the relevant part) > - actual output (or at least the relevant part) > - output with dump of netlink messages, you can get it by enabling > debugging flags, e.g. "ethtool --debug 0x12 eth0" Hi Michal See if this helps. This is a Marvel Ethernet switch port using an Marvell PHY. $ dpkg -l ethtool Desired=Unknown/Install/Remove/Purge/Hold | Status=Not/Inst/Conf-files/Unpacked/halF-conf/Half-inst/trig-aWait/Trig-pend |/ Err?=(none)/Reinst-required (Status,Err: uppercase=bad) ||/ Name Version Architecture Description +++-==============-============-============-========================================== ii ethtool 1:5.4-1 amd64 display or change Ethernet device settings root@rap:~# ethtool green Settings for green: Supported ports: [ TP MII ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Supported pause frame use: Symmetric Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Advertised pause frame use: Symmetric Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Link partner advertised pause frame use: No Link partner advertised auto-negotiation: Yes Link partner advertised FEC modes: Not reported Speed: 1000Mb/s Duplex: Full Port: MII PHYAD: 4 Transceiver: internal Auto-negotiation: on Supports Wake-on: d Wake-on: d Link detected: yes And now ethtool from git 4e02c55227c9958184d5941de73d9cf1cd49bf2e. root@rap:/home/andrew/ethtool# /home/andrew/ethtool/ethtool green Settings for green: Supported ports: [ TP MII ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Supported pause frame use: Symmetric Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Advertised pause frame use: Symmetric Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: Not reported Link partner advertised pause frame use: No Link partner advertised auto-negotiation: No Link partner advertised FEC modes: No Speed: 1000Mb/s Duplex: Full Auto-negotiation: on Port: MII PHYAD: 4 Transceiver: external Supports Wake-on: d Wake-on: d Link detected: yes So they are definitely missing. Here are the netlink messages. sending genetlink packet (32 bytes): msg length 32 genl-ctrl CTRL_CMD_GETFAMILY CTRL_ATTR_FAMILY_NAME = "ethtool" ... ... sending genetlink packet (36 bytes): msg length 36 ethool ETHTOOL_MSG_LINKMODES_GET ETHTOOL_MSG_LINKMODES_GET ETHTOOL_A_LINKMODES_HEADER ETHTOOL_A_HEADER_DEV_NAME = "green" received genetlink packet (572 bytes): msg length 572 ethool ETHTOOL_MSG_LINKMODES_GET_REPLY ETHTOOL_MSG_LINKMODES_GET_REPLY ETHTOOL_A_LINKMODES_HEADER ETHTOOL_A_HEADER_DEV_INDEX = 8 ETHTOOL_A_HEADER_DEV_NAME = "green" ETHTOOL_A_LINKMODES_AUTONEG = on ETHTOOL_A_LINKMODES_OURS ETHTOOL_A_BITSET_SIZE = 90 ETHTOOL_A_BITSET_BITS ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 0 ETHTOOL_A_BITSET_BIT_NAME = "10baseT/Half" ETHTOOL_A_BITSET_BIT_VALUE = true ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 1 ETHTOOL_A_BITSET_BIT_NAME = "10baseT/Full" ETHTOOL_A_BITSET_BIT_VALUE = true ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 2 ETHTOOL_A_BITSET_BIT_NAME = "100baseT/Half" ETHTOOL_A_BITSET_BIT_VALUE = true ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 3 ETHTOOL_A_BITSET_BIT_NAME = "100baseT/Full" ETHTOOL_A_BITSET_BIT_VALUE = true ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 5 ETHTOOL_A_BITSET_BIT_NAME = "1000baseT/Full" ETHTOOL_A_BITSET_BIT_VALUE = true ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 6 ETHTOOL_A_BITSET_BIT_NAME = "Autoneg" ETHTOOL_A_BITSET_BIT_VALUE = true ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 7 ETHTOOL_A_BITSET_BIT_NAME = "TP" ETHTOOL_A_BITSET_BIT_VALUE = true ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 9 ETHTOOL_A_BITSET_BIT_NAME = "MII" ETHTOOL_A_BITSET_BIT_VALUE = true ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 13 ETHTOOL_A_BITSET_BIT_NAME = "Pause" ETHTOOL_A_BITSET_BIT_VALUE = true ETHTOOL_A_LINKMODES_PEER ETHTOOL_A_BITSET_NOMASK = true ETHTOOL_A_BITSET_SIZE = 90 ETHTOOL_A_BITSET_BITS ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 0 ETHTOOL_A_BITSET_BIT_NAME = "10baseT/Half" ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 1 ETHTOOL_A_BITSET_BIT_NAME = "10baseT/Full" ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 2 ETHTOOL_A_BITSET_BIT_NAME = "100baseT/Half" ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 3 ETHTOOL_A_BITSET_BIT_NAME = "100baseT/Full" ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 5 ETHTOOL_A_BITSET_BIT_NAME = "1000baseT/Full" ETHTOOL_A_BITSET_BITS_BIT ETHTOOL_A_BITSET_BIT_INDEX = 6 ETHTOOL_A_BITSET_BIT_NAME = "Autoneg" ETHTOOL_A_LINKMODES_SPEED = 1000 ETHTOOL_A_LINKMODES_DUPLEX = 1 Settings for green: Supported ports: [ TP MII ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Supported pause frame use: Symmetric Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Advertised pause frame use: Symmetric Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: Not reported Link partner advertised pause frame use: No Link partner advertised auto-negotiation: No Link partner advertised FEC modes: No Speed: 1000Mb/s Duplex: Full Auto-negotiation: on received genetlink packet (36 bytes): msg length 36 error errno=0 sending genetlink packet (36 bytes): msg length 36 ethool ETHTOOL_MSG_LINKINFO_GET ETHTOOL_MSG_LINKINFO_GET ETHTOOL_A_LINKINFO_HEADER ETHTOOL_A_HEADER_DEV_NAME = "green" received genetlink packet (84 bytes): msg length 84 ethool ETHTOOL_MSG_LINKINFO_GET_REPLY ETHTOOL_MSG_LINKINFO_GET_REPLY ETHTOOL_A_LINKINFO_HEADER ETHTOOL_A_HEADER_DEV_INDEX = 8 ETHTOOL_A_HEADER_DEV_NAME = "green" ETHTOOL_A_LINKINFO_PORT = 2 ETHTOOL_A_LINKINFO_PHYADDR = 4 ETHTOOL_A_LINKINFO_TP_MDIX = 0 ETHTOOL_A_LINKINFO_TP_MDIX_CTRL = 0 ETHTOOL_A_LINKINFO_TRANSCEIVER = 1 Port: MII PHYAD: 4 Transceiver: external received genetlink packet (36 bytes): msg length 36 error errno=0 sending genetlink packet (36 bytes): msg length 36 ethool ETHTOOL_MSG_WOL_GET ETHTOOL_MSG_WOL_GET ETHTOOL_A_WOL_HEADER ETHTOOL_A_HEADER_DEV_NAME = "green" received genetlink packet (60 bytes): msg length 60 ethool ETHTOOL_MSG_WOL_GET_REPLY ETHTOOL_MSG_WOL_GET_REPLY ETHTOOL_A_WOL_HEADER ETHTOOL_A_HEADER_DEV_INDEX = 8 ETHTOOL_A_HEADER_DEV_NAME = "green" ETHTOOL_A_WOL_MODES ETHTOOL_A_BITSET_SIZE = 8 ETHTOOL_A_BITSET_BITS Supports Wake-on: d Wake-on: d received genetlink packet (36 bytes): msg length 36 error errno=0 sending genetlink packet (36 bytes): msg length 36 ethool ETHTOOL_MSG_DEBUG_GET ETHTOOL_MSG_DEBUG_GET ETHTOOL_A_DEBUG_HEADER ETHTOOL_A_HEADER_DEV_NAME = "green" received genetlink packet (56 bytes): msg length 56 error errno=-95 offending message: ETHTOOL_MSG_DEBUG_GET ETHTOOL_A_DEBUG_HEADER ETHTOOL_A_HEADER_DEV_NAME = "green" sending genetlink packet (36 bytes): msg length 36 ethool ETHTOOL_MSG_LINKSTATE_GET ETHTOOL_MSG_LINKSTATE_GET ETHTOOL_A_LINKSTATE_HEADER ETHTOOL_A_HEADER_DEV_NAME = "green" received genetlink packet (52 bytes): msg length 52 ethool ETHTOOL_MSG_LINKSTATE_GET_REPLY ETHTOOL_MSG_LINKSTATE_GET_REPLY ETHTOOL_A_LINKSTATE_HEADER ETHTOOL_A_HEADER_DEV_INDEX = 8 ETHTOOL_A_HEADER_DEV_NAME = "green" ETHTOOL_A_LINKSTATE_LINK = on Link detected: yes received genetlink packet (36 bytes): msg length 36 error errno=0 I also get similar results from a USB-Ethernet Dongle: # ethtool enx0050b61b0207 Settings for enx0050b61b0207: Supported ports: [ TP MII ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Supported pause frame use: No Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Advertised pause frame use: Symmetric Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Link partner advertised pause frame use: Symmetric Link partner advertised auto-negotiation: Yes Link partner advertised FEC modes: Not reported Speed: 100Mb/s Duplex: Full Port: MII PHYAD: 16 Transceiver: internal Auto-negotiation: on Supports Wake-on: pg Wake-on: p Current message level: 0x00000007 (7) drv probe link Link detected: yes # /home/andrew/ethtool/ethtool enx0050b61b0207 Settings for enx0050b61b0207: Supported ports: [ TP MII ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Supported pause frame use: No Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Advertised pause frame use: No Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: Not reported Link partner advertised pause frame use: No Link partner advertised auto-negotiation: No Link partner advertised FEC modes: No Speed: 100Mb/s Duplex: Full Auto-negotiation: on Port: MII PHYAD: 16 Transceiver: internal Supports Wake-on: pg Wake-on: p Current message level: 0x00000007 (7) drv probe link Link detected: yes ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Broken link partner advertised reporting in ethtool 2020-07-27 21:01 ` Andrew Lunn @ 2020-07-27 21:08 ` Michal Kubecek 2020-07-27 21:25 ` Andrew Lunn 2020-07-27 21:27 ` Jacob Keller 2020-07-27 21:18 ` Jacob Keller 1 sibling, 2 replies; 13+ messages in thread From: Michal Kubecek @ 2020-07-27 21:08 UTC (permalink / raw) To: Andrew Lunn; +Cc: Jamie Gloudon, Jacob Keller, netdev On Mon, Jul 27, 2020 at 11:01:41PM +0200, Andrew Lunn wrote: > > - the exact command you ran (including arguments) > > - expected output (or at least the relevant part) > > - actual output (or at least the relevant part) > > - output with dump of netlink messages, you can get it by enabling > > debugging flags, e.g. "ethtool --debug 0x12 eth0" > > Hi Michal > > See if this helps. > > This is a Marvel Ethernet switch port using an Marvell PHY. Thank you. I think I can see the problem. Can you try the patch below? Michal diff --git a/netlink/settings.c b/netlink/settings.c index 35ba2f5dd6d5..60523ad6edf5 100644 --- a/netlink/settings.c +++ b/netlink/settings.c @@ -280,6 +280,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset, const struct nlattr *bit; bool first = true; int prev = -2; + bool nomask; int ret; ret = mnl_attr_parse_nested(bitset, attr_cb, &bitset_tb_info); @@ -338,6 +339,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset, goto after; } + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK]; printf("\t%s", before); mnl_attr_for_each_nested(bit, bits) { const struct nlattr *tb[ETHTOOL_A_BITSET_BIT_MAX + 1] = {}; @@ -354,7 +356,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset, if (!tb[ETHTOOL_A_BITSET_BIT_INDEX] || !tb[ETHTOOL_A_BITSET_BIT_NAME]) goto err; - if (!mask && !tb[ETHTOOL_A_BITSET_BIT_VALUE]) + if (!mask && !nomask && !tb[ETHTOOL_A_BITSET_BIT_VALUE]) continue; idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]); ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: Broken link partner advertised reporting in ethtool 2020-07-27 21:08 ` Michal Kubecek @ 2020-07-27 21:25 ` Andrew Lunn 2020-07-27 21:30 ` Jacob Keller 2020-07-27 21:42 ` Jacob Keller 2020-07-27 21:27 ` Jacob Keller 1 sibling, 2 replies; 13+ messages in thread From: Andrew Lunn @ 2020-07-27 21:25 UTC (permalink / raw) To: Michal Kubecek; +Cc: Jamie Gloudon, Jacob Keller, netdev On Mon, Jul 27, 2020 at 11:08:43PM +0200, Michal Kubecek wrote: > On Mon, Jul 27, 2020 at 11:01:41PM +0200, Andrew Lunn wrote: > > > - the exact command you ran (including arguments) > > > - expected output (or at least the relevant part) > > > - actual output (or at least the relevant part) > > > - output with dump of netlink messages, you can get it by enabling > > > debugging flags, e.g. "ethtool --debug 0x12 eth0" > > > > Hi Michal > > > > See if this helps. > > > > This is a Marvel Ethernet switch port using an Marvell PHY. > > Thank you. I think I can see the problem. Can you try the patch below? Hi Michal This is better. Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Link partner advertised pause frame use: No Link partner advertised auto-negotiation: No Link partner advertised FEC modes: No However, the Debian version gives: Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Link partner advertised pause frame use: No Link partner advertised auto-negotiation: Yes Link partner advertised FEC modes: Not reported For the USB-Ethernet dongle: netlink: Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Link partner advertised pause frame use: No Link partner advertised auto-negotiation: No Link partner advertised FEC modes: No IOCTL Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Link partner advertised pause frame use: Symmetric Link partner advertised auto-negotiation: Yes Link partner advertised FEC modes: Not reported Andrew ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Broken link partner advertised reporting in ethtool 2020-07-27 21:25 ` Andrew Lunn @ 2020-07-27 21:30 ` Jacob Keller 2020-07-27 21:42 ` Jacob Keller 1 sibling, 0 replies; 13+ messages in thread From: Jacob Keller @ 2020-07-27 21:30 UTC (permalink / raw) To: Andrew Lunn, Michal Kubecek; +Cc: Jamie Gloudon, netdev On 7/27/2020 2:25 PM, Andrew Lunn wrote: > On Mon, Jul 27, 2020 at 11:08:43PM +0200, Michal Kubecek wrote: >> On Mon, Jul 27, 2020 at 11:01:41PM +0200, Andrew Lunn wrote: >>>> - the exact command you ran (including arguments) >>>> - expected output (or at least the relevant part) >>>> - actual output (or at least the relevant part) >>>> - output with dump of netlink messages, you can get it by enabling >>>> debugging flags, e.g. "ethtool --debug 0x12 eth0" >>> >>> Hi Michal >>> >>> See if this helps. >>> >>> This is a Marvel Ethernet switch port using an Marvell PHY. >> >> Thank you. I think I can see the problem. Can you try the patch below? > > Hi Michal > > This is better. > I got similar results testing with a modified netdevsim driver. I think the correct solution requires checking for the NOMASK flag and then behaving differently in all three of dump_link_modes, dump_pause, and bitset_get_bit. > Link partner advertised link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > 1000baseT/Full > Link partner advertised pause frame use: No > Link partner advertised auto-negotiation: No > Link partner advertised FEC modes: No > > However, the Debian version gives: > > Link partner advertised link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > 1000baseT/Full > Link partner advertised pause frame use: No > Link partner advertised auto-negotiation: Yes > Link partner advertised FEC modes: Not reported > > For the USB-Ethernet dongle: > > netlink: > Link partner advertised link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > Link partner advertised pause frame use: No > Link partner advertised auto-negotiation: No > Link partner advertised FEC modes: No > IOCTL > Link partner advertised link modes: 10baseT/Half 10baseT/Full > 100baseT/Half 100baseT/Full > Link partner advertised pause frame use: Symmetric > Link partner advertised auto-negotiation: Yes > Link partner advertised FEC modes: Not reported > > Andrew > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Broken link partner advertised reporting in ethtool 2020-07-27 21:25 ` Andrew Lunn 2020-07-27 21:30 ` Jacob Keller @ 2020-07-27 21:42 ` Jacob Keller 1 sibling, 0 replies; 13+ messages in thread From: Jacob Keller @ 2020-07-27 21:42 UTC (permalink / raw) To: Andrew Lunn, Michal Kubecek; +Cc: Jamie Gloudon, netdev On 7/27/2020 2:25 PM, Andrew Lunn wrote: > On Mon, Jul 27, 2020 at 11:08:43PM +0200, Michal Kubecek wrote: >> On Mon, Jul 27, 2020 at 11:01:41PM +0200, Andrew Lunn wrote: >>>> - the exact command you ran (including arguments) >>>> - expected output (or at least the relevant part) >>>> - actual output (or at least the relevant part) >>>> - output with dump of netlink messages, you can get it by enabling >>>> debugging flags, e.g. "ethtool --debug 0x12 eth0" >>> >>> Hi Michal >>> >>> See if this helps. >>> >>> This is a Marvel Ethernet switch port using an Marvell PHY. >> >> Thank you. I think I can see the problem. Can you try the patch below? > > Hi Michal > > This is better. > I have a proposed patch, about to send it. Thanks, Jake ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Broken link partner advertised reporting in ethtool 2020-07-27 21:08 ` Michal Kubecek 2020-07-27 21:25 ` Andrew Lunn @ 2020-07-27 21:27 ` Jacob Keller 2020-07-27 22:00 ` Michal Kubecek 1 sibling, 1 reply; 13+ messages in thread From: Jacob Keller @ 2020-07-27 21:27 UTC (permalink / raw) To: Michal Kubecek, Andrew Lunn; +Cc: Jamie Gloudon, netdev On 7/27/2020 2:08 PM, Michal Kubecek wrote: > On Mon, Jul 27, 2020 at 11:01:41PM +0200, Andrew Lunn wrote: >>> - the exact command you ran (including arguments) >>> - expected output (or at least the relevant part) >>> - actual output (or at least the relevant part) >>> - output with dump of netlink messages, you can get it by enabling >>> debugging flags, e.g. "ethtool --debug 0x12 eth0" >> >> Hi Michal >> >> See if this helps. >> >> This is a Marvel Ethernet switch port using an Marvell PHY. > > Thank you. I think I can see the problem. Can you try the patch below? > > Michal > I think the patch below fixes part of the issue, but isn't completely correct, because NOMASK bitmaps can be sent in compact form as well. Also, we'll need something to check the NOMASK flag and do the correct thing in all of the dump functions. Thanks, Jake > > diff --git a/netlink/settings.c b/netlink/settings.c > index 35ba2f5dd6d5..60523ad6edf5 100644 > --- a/netlink/settings.c > +++ b/netlink/settings.c > @@ -280,6 +280,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset, > const struct nlattr *bit; > bool first = true; > int prev = -2; > + bool nomask; > int ret; > > ret = mnl_attr_parse_nested(bitset, attr_cb, &bitset_tb_info); > @@ -338,6 +339,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset, > goto after; > } > > + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK]; > printf("\t%s", before); > mnl_attr_for_each_nested(bit, bits) { > const struct nlattr *tb[ETHTOOL_A_BITSET_BIT_MAX + 1] = {}; > @@ -354,7 +356,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset, > if (!tb[ETHTOOL_A_BITSET_BIT_INDEX] || > !tb[ETHTOOL_A_BITSET_BIT_NAME]) > goto err; > - if (!mask && !tb[ETHTOOL_A_BITSET_BIT_VALUE]) > + if (!mask && !nomask && !tb[ETHTOOL_A_BITSET_BIT_VALUE]) > continue; > > idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]); > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Broken link partner advertised reporting in ethtool 2020-07-27 21:27 ` Jacob Keller @ 2020-07-27 22:00 ` Michal Kubecek 2020-07-27 22:15 ` Jacob Keller 0 siblings, 1 reply; 13+ messages in thread From: Michal Kubecek @ 2020-07-27 22:00 UTC (permalink / raw) To: Jacob Keller; +Cc: Andrew Lunn, Jamie Gloudon, netdev On Mon, Jul 27, 2020 at 02:27:56PM -0700, Jacob Keller wrote: > > > On 7/27/2020 2:08 PM, Michal Kubecek wrote: > > On Mon, Jul 27, 2020 at 11:01:41PM +0200, Andrew Lunn wrote: > >>> - the exact command you ran (including arguments) > >>> - expected output (or at least the relevant part) > >>> - actual output (or at least the relevant part) > >>> - output with dump of netlink messages, you can get it by enabling > >>> debugging flags, e.g. "ethtool --debug 0x12 eth0" > >> > >> Hi Michal > >> > >> See if this helps. > >> > >> This is a Marvel Ethernet switch port using an Marvell PHY. > > > > Thank you. I think I can see the problem. Can you try the patch below? > > > > Michal > > > > I think the patch below fixes part of the issue, but isn't completely > correct, because NOMASK bitmaps can be sent in compact form as well. I believe this part is correct; compact NOMASK bitmap would have no ETHTOOL_A_BITSET_MASK and ETHTOOL_A_BITSET_BIT_VALUE would contain the bits in it so that the code would generate correct output. > Also, we'll need something to check the NOMASK flag and do the correct > thing in all of the dump functions. You are right, bitset_get_bit needs the same fix, updated version is below. Michal diff --git a/netlink/bitset.c b/netlink/bitset.c index 130bcdb5b52c..67b45778692c 100644 --- a/netlink/bitset.c +++ b/netlink/bitset.c @@ -50,6 +50,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx, DECLARE_ATTR_TB_INFO(bitset_tb); const struct nlattr *bits; const struct nlattr *bit; + bool nomask; int ret; *retptr = 0; @@ -68,6 +69,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx, return bitmap[idx / 32] & (1U << (idx % 32)); } + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK]; bits = bitset_tb[ETHTOOL_A_BITSET_BITS]; if (!bits) goto err; @@ -87,7 +89,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx, my_idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]); if (my_idx == idx) - return mask || tb[ETHTOOL_A_BITSET_BIT_VALUE]; + return mask || nomask || tb[ETHTOOL_A_BITSET_BIT_VALUE]; } return false; diff --git a/netlink/settings.c b/netlink/settings.c index 35ba2f5dd6d5..60523ad6edf5 100644 --- a/netlink/settings.c +++ b/netlink/settings.c @@ -280,6 +280,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset, const struct nlattr *bit; bool first = true; int prev = -2; + bool nomask; int ret; ret = mnl_attr_parse_nested(bitset, attr_cb, &bitset_tb_info); @@ -338,6 +339,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset, goto after; } + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK]; printf("\t%s", before); mnl_attr_for_each_nested(bit, bits) { const struct nlattr *tb[ETHTOOL_A_BITSET_BIT_MAX + 1] = {}; @@ -354,7 +356,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset, if (!tb[ETHTOOL_A_BITSET_BIT_INDEX] || !tb[ETHTOOL_A_BITSET_BIT_NAME]) goto err; - if (!mask && !tb[ETHTOOL_A_BITSET_BIT_VALUE]) + if (!mask && !nomask && !tb[ETHTOOL_A_BITSET_BIT_VALUE]) continue; idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]); ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: Broken link partner advertised reporting in ethtool 2020-07-27 22:00 ` Michal Kubecek @ 2020-07-27 22:15 ` Jacob Keller 0 siblings, 0 replies; 13+ messages in thread From: Jacob Keller @ 2020-07-27 22:15 UTC (permalink / raw) To: Michal Kubecek; +Cc: Andrew Lunn, Jamie Gloudon, netdev On 7/27/2020 3:00 PM, Michal Kubecek wrote: > On Mon, Jul 27, 2020 at 02:27:56PM -0700, Jacob Keller wrote: >> >> >> On 7/27/2020 2:08 PM, Michal Kubecek wrote: >>> On Mon, Jul 27, 2020 at 11:01:41PM +0200, Andrew Lunn wrote: >>>>> - the exact command you ran (including arguments) >>>>> - expected output (or at least the relevant part) >>>>> - actual output (or at least the relevant part) >>>>> - output with dump of netlink messages, you can get it by enabling >>>>> debugging flags, e.g. "ethtool --debug 0x12 eth0" >>>> >>>> Hi Michal >>>> >>>> See if this helps. >>>> >>>> This is a Marvel Ethernet switch port using an Marvell PHY. >>> >>> Thank you. I think I can see the problem. Can you try the patch below? >>> >>> Michal >>> >> >> I think the patch below fixes part of the issue, but isn't completely >> correct, because NOMASK bitmaps can be sent in compact form as well. > > I believe this part is correct; compact NOMASK bitmap would have no > ETHTOOL_A_BITSET_MASK and ETHTOOL_A_BITSET_BIT_VALUE would contain the > bits in it so that the code would generate correct output. > >> Also, we'll need something to check the NOMASK flag and do the correct >> thing in all of the dump functions. > > You are right, bitset_get_bit needs the same fix, updated version is > below. > > Michal > > This is basically what I got except that I also applied the fix to compact bitmaps. See https://lore.kernel.org/netdev/20200727221104.GD1705504@lunn.ch/T/#t I think we're still missing something tho. Thanks, Jake > diff --git a/netlink/bitset.c b/netlink/bitset.c > index 130bcdb5b52c..67b45778692c 100644 > --- a/netlink/bitset.c > +++ b/netlink/bitset.c > @@ -50,6 +50,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx, > DECLARE_ATTR_TB_INFO(bitset_tb); > const struct nlattr *bits; > const struct nlattr *bit; > + bool nomask; > int ret; > > *retptr = 0; > @@ -68,6 +69,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx, > return bitmap[idx / 32] & (1U << (idx % 32)); > } > > + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK]; > bits = bitset_tb[ETHTOOL_A_BITSET_BITS]; > if (!bits) > goto err; > @@ -87,7 +89,7 @@ bool bitset_get_bit(const struct nlattr *bitset, bool mask, unsigned int idx, > > my_idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]); > if (my_idx == idx) > - return mask || tb[ETHTOOL_A_BITSET_BIT_VALUE]; > + return mask || nomask || tb[ETHTOOL_A_BITSET_BIT_VALUE]; > } > > return false; > diff --git a/netlink/settings.c b/netlink/settings.c > index 35ba2f5dd6d5..60523ad6edf5 100644 > --- a/netlink/settings.c > +++ b/netlink/settings.c > @@ -280,6 +280,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset, > const struct nlattr *bit; > bool first = true; > int prev = -2; > + bool nomask; > int ret; > > ret = mnl_attr_parse_nested(bitset, attr_cb, &bitset_tb_info); > @@ -338,6 +339,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset, > goto after; > } > > + nomask = bitset_tb[ETHTOOL_A_BITSET_NOMASK]; > printf("\t%s", before); > mnl_attr_for_each_nested(bit, bits) { > const struct nlattr *tb[ETHTOOL_A_BITSET_BIT_MAX + 1] = {}; > @@ -354,7 +356,7 @@ int dump_link_modes(struct nl_context *nlctx, const struct nlattr *bitset, > if (!tb[ETHTOOL_A_BITSET_BIT_INDEX] || > !tb[ETHTOOL_A_BITSET_BIT_NAME]) > goto err; > - if (!mask && !tb[ETHTOOL_A_BITSET_BIT_VALUE]) > + if (!mask && !nomask && !tb[ETHTOOL_A_BITSET_BIT_VALUE]) > continue; > > idx = mnl_attr_get_u32(tb[ETHTOOL_A_BITSET_BIT_INDEX]); > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Broken link partner advertised reporting in ethtool 2020-07-27 21:01 ` Andrew Lunn 2020-07-27 21:08 ` Michal Kubecek @ 2020-07-27 21:18 ` Jacob Keller 1 sibling, 0 replies; 13+ messages in thread From: Jacob Keller @ 2020-07-27 21:18 UTC (permalink / raw) To: Andrew Lunn, Michal Kubecek; +Cc: Jamie Gloudon, netdev On 7/27/2020 2:01 PM, Andrew Lunn wrote: > Here are the netlink messages. > > sending genetlink packet (32 bytes): > msg length 32 genl-ctrl > CTRL_CMD_GETFAMILY > CTRL_ATTR_FAMILY_NAME = "ethtool" > ... > ... > sending genetlink packet (36 bytes): > msg length 36 ethool ETHTOOL_MSG_LINKMODES_GET > ETHTOOL_MSG_LINKMODES_GET > ETHTOOL_A_LINKMODES_HEADER > ETHTOOL_A_HEADER_DEV_NAME = "green" > received genetlink packet (572 bytes): > msg length 572 ethool ETHTOOL_MSG_LINKMODES_GET_REPLY > ETHTOOL_MSG_LINKMODES_GET_REPLY > ETHTOOL_A_LINKMODES_HEADER > ETHTOOL_A_HEADER_DEV_INDEX = 8 > ETHTOOL_A_HEADER_DEV_NAME = "green" > ETHTOOL_A_LINKMODES_AUTONEG = on > ETHTOOL_A_LINKMODES_OURS > ETHTOOL_A_BITSET_SIZE = 90 > ETHTOOL_A_BITSET_BITS > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 0 > ETHTOOL_A_BITSET_BIT_NAME = "10baseT/Half" > ETHTOOL_A_BITSET_BIT_VALUE = true > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 1 > ETHTOOL_A_BITSET_BIT_NAME = "10baseT/Full" > ETHTOOL_A_BITSET_BIT_VALUE = true > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 2 > ETHTOOL_A_BITSET_BIT_NAME = "100baseT/Half" > ETHTOOL_A_BITSET_BIT_VALUE = true > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 3 > ETHTOOL_A_BITSET_BIT_NAME = "100baseT/Full" > ETHTOOL_A_BITSET_BIT_VALUE = true > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 5 > ETHTOOL_A_BITSET_BIT_NAME = "1000baseT/Full" > ETHTOOL_A_BITSET_BIT_VALUE = true > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 6 > ETHTOOL_A_BITSET_BIT_NAME = "Autoneg" > ETHTOOL_A_BITSET_BIT_VALUE = true > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 7 > ETHTOOL_A_BITSET_BIT_NAME = "TP" > ETHTOOL_A_BITSET_BIT_VALUE = true > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 9 > ETHTOOL_A_BITSET_BIT_NAME = "MII" > ETHTOOL_A_BITSET_BIT_VALUE = true > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 13 > ETHTOOL_A_BITSET_BIT_NAME = "Pause" > ETHTOOL_A_BITSET_BIT_VALUE = true > ETHTOOL_A_LINKMODES_PEER > ETHTOOL_A_BITSET_NOMASK = true > ETHTOOL_A_BITSET_SIZE = 90 > ETHTOOL_A_BITSET_BITS > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 0 > ETHTOOL_A_BITSET_BIT_NAME = "10baseT/Half" > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 1 > ETHTOOL_A_BITSET_BIT_NAME = "10baseT/Full" > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 2 > ETHTOOL_A_BITSET_BIT_NAME = "100baseT/Half" > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 3 > ETHTOOL_A_BITSET_BIT_NAME = "100baseT/Full" > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 5 > ETHTOOL_A_BITSET_BIT_NAME = "1000baseT/Full" > ETHTOOL_A_BITSET_BITS_BIT > ETHTOOL_A_BITSET_BIT_INDEX = 6 > ETHTOOL_A_BITSET_BIT_NAME = "Autoneg" > ETHTOOL_A_LINKMODES_SPEED = 1000 > ETHTOOL_A_LINKMODES_DUPLEX = 1 Based on the netlink contents here it looks like a bug at netlink/settings.c:357, where we check for ETHTOOL_A_BITSET_BIT_VALUE, but these aren't sent when you send the bitset using a individual BIT_INDEX/BIT_NAME like this. I think that's a bug. I'm working up a simple to verify this and if my suspicion is confirmed I can write up a patch. Thanks, Jake ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-07-27 22:15 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-07-27 15:47 Broken link partner advertised reporting in ethtool Jamie Gloudon 2020-07-27 19:19 ` Jacob Keller 2020-07-27 20:09 ` Jamie Gloudon 2020-07-27 20:42 ` Michal Kubecek 2020-07-27 21:01 ` Andrew Lunn 2020-07-27 21:08 ` Michal Kubecek 2020-07-27 21:25 ` Andrew Lunn 2020-07-27 21:30 ` Jacob Keller 2020-07-27 21:42 ` Jacob Keller 2020-07-27 21:27 ` Jacob Keller 2020-07-27 22:00 ` Michal Kubecek 2020-07-27 22:15 ` Jacob Keller 2020-07-27 21:18 ` Jacob Keller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).