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 <[email protected]>
---
net/core/net-sysfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 1cac29e..99afdea 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -173,7 +173,7 @@ static ssize_t speed_show(struct device *dev,
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));
}
rtnl_unlock();
return ret;
--
2.0.0
Mon, Jun 02, 2014 at 04:25:15PM CEST, [email protected] wrote:
>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 <[email protected]>
>---
> net/core/net-sysfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
>index 1cac29e..99afdea 100644
>--- a/net/core/net-sysfs.c
>+++ b/net/core/net-sysfs.c
>@@ -173,7 +173,7 @@ static ssize_t speed_show(struct device *dev,
> 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?
> }
> rtnl_unlock();
> return ret;
>--
>2.0.0
>
Jiri Pirko <[email protected]> writes:
> Mon, Jun 02, 2014 at 04:25:15PM CEST, [email protected] wrote:
>>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 <[email protected]>
>>---
>> net/core/net-sysfs.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
>>index 1cac29e..99afdea 100644
>>--- a/net/core/net-sysfs.c
>>+++ b/net/core/net-sysfs.c
>>@@ -173,7 +173,7 @@ static ssize_t speed_show(struct device *dev,
>> 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?
I believe many drivers will do this when the speed is unknown, e.g
because the link is down. For example:
bjorn@nemi:~$ grep . /sys/class/net/eth0/speed
4294967295
bjorn@nemi:~$ ls -l /sys/class/net/eth0/device/driver
lrwxrwxrwx 1 root root 0 Jun 2 10:48 /sys/class/net/eth0/device/driver -> ../../../bus/pci/drivers/e1000e
bjorn@nemi:~$ ethtool eth0
Settings for eth0:
Supported ports: [ TP ]
Supported link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Supported pause frame use: No
Supports auto-negotiation: Yes
Advertised link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Advertised pause frame use: No
Advertised auto-negotiation: Yes
Speed: Unknown!
Duplex: Unknown! (255)
Port: Twisted Pair
PHYAD: 2
Transceiver: internal
Auto-negotiation: on
MDI-X: Unknown
Cannot get wake-on-lan settings: Operation not permitted
Current message level: 0x00000007 (7)
drv probe link
Link detected: no
Bjørn
On 02.06.2014 16:35, Jiri Pirko wrote:
> Mon, Jun 02, 2014 at 04:25:15PM CEST, [email protected] wrote:
>> 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 <[email protected]>
>> ---
>> net/core/net-sysfs.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
>> index 1cac29e..99afdea 100644
>> --- a/net/core/net-sysfs.c
>> +++ b/net/core/net-sysfs.c
>> @@ -173,7 +173,7 @@ static ssize_t speed_show(struct device *dev,
>> 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, [email protected] wrote:
>>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 <[email protected]>
>>---
>> net/core/net-sysfs.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
>>index 1cac29e..99afdea 100644
>>--- a/net/core/net-sysfs.c
>>+++ b/net/core/net-sysfs.c
>>@@ -173,7 +173,7 @@ static ssize_t speed_show(struct device *dev,
>> 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.
>
>> }
>> rtnl_unlock();
>> return ret;
>>--
>>2.0.0
>>
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
2014-06-02 8:01 GMT-07:00 Veaceslav Falico <[email protected]>:
> On Mon, Jun 02, 2014 at 04:35:57PM +0200, Jiri Pirko wrote:
>>
>> Mon, Jun 02, 2014 at 04:25:15PM CEST, [email protected] wrote:
>>>
>>> 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 <[email protected]>
>>> ---
>>> net/core/net-sysfs.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
>>> index 1cac29e..99afdea 100644
>>> --- a/net/core/net-sysfs.c
>>> +++ b/net/core/net-sysfs.c
>>> @@ -173,7 +173,7 @@ static ssize_t speed_show(struct device *dev,
>>> 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.
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.
>
>>
>>> }
>>> rtnl_unlock();
>>> return ret;
>>> --
>>> 2.0.0
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>>
>> the body of a message to [email protected]
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Florian
From: Veaceslav Falico <[email protected]>
Date: Mon, 2 Jun 2014 17:01:50 +0200
> 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.
On 02.06.2014 20:10, David Miller wrote:
> From: Veaceslav Falico <[email protected]>
> Date: Mon, 2 Jun 2014 17:01:50 +0200
>
>> 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.
Well, that's rather unpleasant.
>
> I'm not applying this patch.
>
Michal
On 03.06.2014 09:07, Michal Privoznik wrote:
> On 02.06.2014 20:10, David Miller wrote:
>> From: Veaceslav Falico <[email protected]>
>> Date: Mon, 2 Jun 2014 17:01:50 +0200
>>
>>> 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