linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Michal Privoznik <mprivozn@redhat.com>
To: David Miller <davem@davemloft.net>, jiri@resnulli.us
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org
Subject: Re: [PATCH] net-sysfs: Report link speed only when possible
Date: Fri, 13 Jun 2014 11:19:51 +0200	[thread overview]
Message-ID: <539AC237.40402@redhat.com> (raw)
In-Reply-To: <20140606.125447.1665616708152334692.davem@davemloft.net>

On 06.06.2014 21:54, David Miller wrote:
> From: Jiri Pirko <jiri@resnulli.us>
> Date: Fri, 6 Jun 2014 10:57:33 +0200
>
>> Fri, Jun 06, 2014 at 10:40:30AM CEST, mprivozn@redhat.com wrote:
>>> The link speed is available at /sys/class/net/$nic/speed.
>>> However, in some cases, depending on the driver, if the link is
>>> not plugged, -1 is reported (this is the case of e1000e for
>>> instance). To make things worse, the value is printed out as an
>>
>> Actually, SPEED_UNKNOWN is also -1
>> So e1000e is not any exception.
>
> And pity the person who is handling this by evaluating that unsigned
> value, we'll break them.
>
> We can't keep changing behavior for the SPEED_UNKOWN case back and
> forth.
>
> A program that wants to work with all kernels now has to handle three
> different kinds of behavior if we apply this patch, that's not making
> things better, it's making things worse.

I don't think this is a good reason to keep things broken. By the same 
token we wouldn't need to fix any bug since there already might be an 
application that learned how to deal with it.

This patch is not introducing any new class that applications need to 
learn. It just barely removes the spurious one. And it's perfectly okay 
in some ecosystems to require minimal version of some programs, 
including kernel. So if I were developing brand new application I could 
say: I'm dropping all this workaround code and have it clean and require 
say 3.16 kernel at least. Moreover, using an older application would do, 
as the workaround code will never be executed.

But okay, maybe for some reason you're hesitant to change net-sysfs.c. 
Would it be more convenient to change the broken drivers instead? Yes, 
some drivers don't report nor 0, nor -1, but rather random value. For 
instance a tap device returns '10',  igb '65535', etc.

Michal

  reply	other threads:[~2014-06-13  9:20 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-06-06  8:40 [PATCH] net-sysfs: Report link speed only when possible Michal Privoznik
2014-06-06  8:57 ` Jiri Pirko
2014-06-06 19:54   ` David Miller
2014-06-13  9:19     ` Michal Privoznik [this message]
2014-06-13 20:03       ` David Miller
2014-06-16  7:32         ` Michal Privoznik
2014-06-16  8:11           ` David Miller
2014-06-16  8:30             ` Michal Privoznik
2014-06-16  8:44               ` David Miller
2014-06-16  8:59                 ` Michal Privoznik
2014-06-16  9:01                 ` Jiri Pirko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=539AC237.40402@redhat.com \
    --to=mprivozn@redhat.com \
    --cc=davem@davemloft.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=jiri@resnulli.us \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).