From: Michal Privoznik <hidden> Date: 2014-06-02 14:25:38
The link speed is available at /sys/class/net/$nic/speed.
However, the speed is printed in unsigned integer format. This
makes userspace applications read an incorrect value (which
moreover changes through several architectures) while in fact
'-1' should be reported.
Before the change:
# cat /sys/class/net/eth0/speed
4294967295
After the change:
# cat /sys/class/net/eth0/speed
-1
Signed-off-by: Michal Privoznik <redacted>
---
net/core/net-sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Mon, Jun 02, 2014 at 04:25:15PM CEST, mprivozn@redhat.com wrote:
quoted hunk
The link speed is available at /sys/class/net/$nic/speed.
However, the speed is printed in unsigned integer format. This
makes userspace applications read an incorrect value (which
moreover changes through several architectures) while in fact
'-1' should be reported.
Before the change:
# cat /sys/class/net/eth0/speed
4294967295
After the change:
# cat /sys/class/net/eth0/speed
-1
Signed-off-by: Michal Privoznik <redacted>
---
net/core/net-sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Michal Privoznik <hidden> Date: 2014-06-02 14:43:40
On 02.06.2014 16:35, Jiri Pirko wrote:
Mon, Jun 02, 2014 at 04:25:15PM CEST, mprivozn@redhat.com wrote:
quoted
The link speed is available at /sys/class/net/$nic/speed.
However, the speed is printed in unsigned integer format. This
makes userspace applications read an incorrect value (which
moreover changes through several architectures) while in fact
'-1' should be reported.
Before the change:
# cat /sys/class/net/eth0/speed
4294967295
After the change:
# cat /sys/class/net/eth0/speed
-1
Signed-off-by: Michal Privoznik <redacted>
---
net/core/net-sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
if (netif_running(netdev)) {
struct ethtool_cmd cmd;
if (!__ethtool_get_settings(netdev, &cmd))
- ret = sprintf(buf, fmt_udec, ethtool_cmd_speed(&cmd));
+ ret = sprintf(buf, fmt_dec, ethtool_cmd_speed(&cmd));
I wonder why this should be signed. What -1 means? What driver reports
this?
This is e1000e. It seems like a default value. From e1000_get_settings()
in drivers/net/ethernet/intel/e1000e/ethtool.c:
speed = -1;
ecmd->duplex = -1;
if (netif_running(netdev)) {
if (netif_carrier_ok(netdev)) {
speed = adapter->link_speed;
ecmd->duplex = adapter->link_duplex - 1;
}
} else if (!pm_runtime_suspended(netdev->dev.parent)) {
...
}
ethtool_cmd_speed_set(ecmd, speed);
If I unplug the cord, NIC is nor running nor runtime suspended.
Michal
On Mon, Jun 02, 2014 at 04:35:57PM +0200, Jiri Pirko wrote:
Mon, Jun 02, 2014 at 04:25:15PM CEST, mprivozn@redhat.com wrote:
quoted
The link speed is available at /sys/class/net/$nic/speed.
However, the speed is printed in unsigned integer format. This
makes userspace applications read an incorrect value (which
moreover changes through several architectures) while in fact
'-1' should be reported.
Before the change:
# cat /sys/class/net/eth0/speed
4294967295
After the change:
# cat /sys/class/net/eth0/speed
-1
Signed-off-by: Michal Privoznik <redacted>
---
net/core/net-sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
if (netif_running(netdev)) {
struct ethtool_cmd cmd;
if (!__ethtool_get_settings(netdev, &cmd))
- ret = sprintf(buf, fmt_udec, ethtool_cmd_speed(&cmd));
+ ret = sprintf(buf, fmt_dec, ethtool_cmd_speed(&cmd));
I wonder why this should be signed. What -1 means? What driver reports
this?
My first thoughts were exactly this. There is SPEED_UNKOWN (along with
_10, _100, _1000 etc.) that's -1, and quite a few drivers use it/set it.
I wonder, though, if we should document it or just output "Unknown" instead
of -1.
quoted
}
rtnl_unlock();
return ret;
--
2.0.0
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 02, 2014 at 04:35:57PM +0200, Jiri Pirko wrote:
quoted
Mon, Jun 02, 2014 at 04:25:15PM CEST, mprivozn@redhat.com wrote:
quoted
The link speed is available at /sys/class/net/$nic/speed.
However, the speed is printed in unsigned integer format. This
makes userspace applications read an incorrect value (which
moreover changes through several architectures) while in fact
'-1' should be reported.
Before the change:
# cat /sys/class/net/eth0/speed
4294967295
After the change:
# cat /sys/class/net/eth0/speed
-1
Signed-off-by: Michal Privoznik <redacted>
---
net/core/net-sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
ethtool_cmd_speed(&cmd));
+ ret = sprintf(buf, fmt_dec,
ethtool_cmd_speed(&cmd));
I wonder why this should be signed. What -1 means? What driver reports
this?
My first thoughts were exactly this. There is SPEED_UNKOWN (along with
_10, _100, _1000 etc.) that's -1, and quite a few drivers use it/set it.
I wonder, though, if we should document it or just output "Unknown" instead
of -1.
I would document the special "Unkown" value in
Documentation/ABI/testing/sysfs-class-net and update speed_show() to
handle it. -1 is confusing for anyone to realize what this means.
quoted
quoted
}
rtnl_unlock();
return ret;
--
2.0.0
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
I wonder, though, if we should document it or just output "Unknown"
instead of -1.
All of this discussion means that we can't change the format string
without potentially breaking something out there interpreting the -1
value, however it gets output now.
I think we just have to leave things as-is and document them in the
sysfs ABI docs.
I'm not applying this patch.
I wonder, though, if we should document it or just output "Unknown"
instead of -1.
All of this discussion means that we can't change the format string
without potentially breaking something out there interpreting the -1
value, however it gets output now.
No, the discussion means reporting -1 as link speed is confusing.
I think we just have to leave things as-is and document them in the
sysfs ABI docs.
I wonder, though, if we should document it or just output "Unknown"
instead of -1.
All of this discussion means that we can't change the format string
without potentially breaking something out there interpreting the -1
value, however it gets output now.
One more thing. The commit that changed the behavior is 8ae6daca which
is part of the 3.0 release. So any 2.6.X kernel does report -1 while
with 3.Y kernel you'll get this meaningless value. Having said that, we
already broke the applications so how about unbreaking them?
Michal