* [PATCH] net-sysfs: Report link speed as signed integer
@ 2014-06-02 14:25 Michal Privoznik
2014-06-02 14:35 ` Jiri Pirko
0 siblings, 1 reply; 9+ messages in thread
From: Michal Privoznik @ 2014-06-02 14:25 UTC (permalink / raw)
To: davem; +Cc: gregkh, jiri, linux-kernel, netdev
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 <mprivozn@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] net-sysfs: Report link speed as signed integer
2014-06-02 14:25 [PATCH] net-sysfs: Report link speed as signed integer Michal Privoznik
@ 2014-06-02 14:35 ` Jiri Pirko
2014-06-02 14:40 ` Bjørn Mork
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jiri Pirko @ 2014-06-02 14:35 UTC (permalink / raw)
To: Michal Privoznik; +Cc: davem, gregkh, linux-kernel, netdev
Mon, Jun 02, 2014 at 04:25:15PM CEST, mprivozn@redhat.com 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 <mprivozn@redhat.com>
>---
> 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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net-sysfs: Report link speed as signed integer
2014-06-02 14:35 ` Jiri Pirko
@ 2014-06-02 14:40 ` Bjørn Mork
2014-06-02 14:43 ` Michal Privoznik
2014-06-02 15:01 ` Veaceslav Falico
2 siblings, 0 replies; 9+ messages in thread
From: Bjørn Mork @ 2014-06-02 14:40 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Michal Privoznik, davem, gregkh, linux-kernel, netdev
Jiri Pirko <jiri@resnulli.us> writes:
> Mon, Jun 02, 2014 at 04:25:15PM CEST, mprivozn@redhat.com 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 <mprivozn@redhat.com>
>>---
>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net-sysfs: Report link speed as signed integer
2014-06-02 14:35 ` Jiri Pirko
2014-06-02 14:40 ` Bjørn Mork
@ 2014-06-02 14:43 ` Michal Privoznik
2014-06-02 15:01 ` Veaceslav Falico
2 siblings, 0 replies; 9+ messages in thread
From: Michal Privoznik @ 2014-06-02 14:43 UTC (permalink / raw)
To: Jiri Pirko; +Cc: davem, gregkh, linux-kernel, netdev
On 02.06.2014 16:35, Jiri Pirko wrote:
> Mon, Jun 02, 2014 at 04:25:15PM CEST, mprivozn@redhat.com 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 <mprivozn@redhat.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net-sysfs: Report link speed as signed integer
2014-06-02 14:35 ` Jiri Pirko
2014-06-02 14:40 ` Bjørn Mork
2014-06-02 14:43 ` Michal Privoznik
@ 2014-06-02 15:01 ` Veaceslav Falico
2014-06-02 17:21 ` Florian Fainelli
2014-06-02 18:10 ` David Miller
2 siblings, 2 replies; 9+ messages in thread
From: Veaceslav Falico @ 2014-06-02 15:01 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Michal Privoznik, davem, gregkh, linux-kernel, netdev
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:
>>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 <mprivozn@redhat.com>
>>---
>> 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 majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net-sysfs: Report link speed as signed integer
2014-06-02 15:01 ` Veaceslav Falico
@ 2014-06-02 17:21 ` Florian Fainelli
2014-06-02 18:10 ` David Miller
1 sibling, 0 replies; 9+ messages in thread
From: Florian Fainelli @ 2014-06-02 17:21 UTC (permalink / raw)
To: Veaceslav Falico
Cc: Jiri Pirko, Michal Privoznik, David Miller, Greg KH,
linux-kernel, netdev
2014-06-02 8:01 GMT-07:00 Veaceslav Falico <vfalico@redhat.com>:
> 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:
>>>
>>> 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 <mprivozn@redhat.com>
>>> ---
>>> 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 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
--
Florian
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net-sysfs: Report link speed as signed integer
2014-06-02 15:01 ` Veaceslav Falico
2014-06-02 17:21 ` Florian Fainelli
@ 2014-06-02 18:10 ` David Miller
2014-06-03 7:07 ` Michal Privoznik
1 sibling, 1 reply; 9+ messages in thread
From: David Miller @ 2014-06-02 18:10 UTC (permalink / raw)
To: vfalico; +Cc: jiri, mprivozn, gregkh, linux-kernel, netdev
From: Veaceslav Falico <vfalico@redhat.com>
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net-sysfs: Report link speed as signed integer
2014-06-02 18:10 ` David Miller
@ 2014-06-03 7:07 ` Michal Privoznik
2014-06-03 12:05 ` Michal Privoznik
0 siblings, 1 reply; 9+ messages in thread
From: Michal Privoznik @ 2014-06-03 7:07 UTC (permalink / raw)
To: David Miller, vfalico; +Cc: jiri, gregkh, linux-kernel, netdev
On 02.06.2014 20:10, David Miller wrote:
> From: Veaceslav Falico <vfalico@redhat.com>
> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net-sysfs: Report link speed as signed integer
2014-06-03 7:07 ` Michal Privoznik
@ 2014-06-03 12:05 ` Michal Privoznik
0 siblings, 0 replies; 9+ messages in thread
From: Michal Privoznik @ 2014-06-03 12:05 UTC (permalink / raw)
To: David Miller, vfalico; +Cc: jiri, gregkh, linux-kernel, netdev
On 03.06.2014 09:07, Michal Privoznik wrote:
> On 02.06.2014 20:10, David Miller wrote:
>> From: Veaceslav Falico <vfalico@redhat.com>
>> 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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-06-03 12:05 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-02 14:25 [PATCH] net-sysfs: Report link speed as signed integer Michal Privoznik
2014-06-02 14:35 ` Jiri Pirko
2014-06-02 14:40 ` Bjørn Mork
2014-06-02 14:43 ` Michal Privoznik
2014-06-02 15:01 ` Veaceslav Falico
2014-06-02 17:21 ` Florian Fainelli
2014-06-02 18:10 ` David Miller
2014-06-03 7:07 ` Michal Privoznik
2014-06-03 12:05 ` Michal Privoznik
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).