From: Daniel Walker <danielwa@cisco.com>
To: intel-wired-lan@osuosl.org
Subject: [Intel-wired-lan] [PATCH-RFC] drivers: net: ethernet: intel: e1000e: fix ethtool autoneg off for fiber
Date: Mon, 4 Apr 2016 16:03:50 -0700 [thread overview]
Message-ID: <5702F2D6.5010105@cisco.com> (raw)
In-Reply-To: <36CDDD56DDB4D44E911123902EFC26B05B440F17@HASMSX110.ger.corp.intel.com>
On 04/03/2016 07:23 AM, Ruinskiy, Dima wrote:
> I have a couple of comments (sorry for not getting to it a bit sooner).
>
>> For fiber media, e1000_get_settings should return ETH_TP_MDI_INVALID for
>> eth_tp_mdix_ctrl instead of ETH_TP_MDI_AUTO so subsequent e1000_set_settings
>> call would not fail with -EOPNOTSUPP.
> Should this be specific to fiber? Because the code just checks media != copper. There is at least other media type (e1000_media_type_internal_serdes), which will be affected by this change as well. What is the proper behavior in this case?
Steve is shy, so here are his comments,
<Steve>
Regarding the 1st comment. The issue affects all non-copper media. In
fact, we are using e1000_media_type_internal_serdes and are affected due
to the following check in e1000_set_settings:
/* MDI setting is only allowed when autoneg enabled because
* some hardware doesn't allow MDI setting when speed or
* duplex is forced.
*/
if (ecmd->eth_tp_mdix_ctrl) {
if (hw->phy.media_type != e1000_media_type_copper)
return -EOPNOTSUPP;
if ((ecmd->eth_tp_mdix_ctrl != ETH_TP_MDI_AUTO) &&
(ecmd->autoneg != AUTONEG_ENABLE)) {
e_err("forcing MDI/MDI-X state is not supported when link speed and/or
duplex are forced\n");
return -EINVAL;
}
}
</Steve>
>> e1000_set_spd_dplx should not automatically turn autoneg back on for forced
>> 1000 Mbps full duplex settings.
> I seem to remember that this code is there because the copper-based NICs only support 1000Mbps in auto-negotiation mode, not forced (this is according to the IEEE spec, as far as I know). I believe this code is there to ensure that a user can force the speed to 1000 via the API, and still have the link resolve correctly via auto-negotiation. I am concerned that changing this code will break clients that rely on it. Maybe it is best to also limit it to fiber devices only?
>
<Steve>
Regarding the 2nd comment. Yes, will limit to non-copper media.
</Steve>
He supplied me with a second patch which I can send.. Can we remove the
RFC this time ?
Daniel
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Walker <danielwa@cisco.com>
To: "Ruinskiy, Dima" <dima.ruinskiy@intel.com>,
"Kirsher, Jeffrey T" <jeffrey.t.kirsher@intel.com>,
"Brandeburg, Jesse" <jesse.brandeburg@intel.com>,
"Nelson, Shannon" <shannon.nelson@intel.com>,
"Wyborny, Carolyn" <carolyn.wyborny@intel.com>,
"Skidmore, Donald C" <donald.c.skidmore@intel.com>,
"Allan, Bruce W" <bruce.w.allan@intel.com>,
"Ronciak, John" <john.ronciak@intel.com>,
"Williams, Mitch A" <mitch.a.williams@intel.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"xe-kernel@external.cisco.com" <xe-kernel@external.cisco.com>,
"danielwa@fifo99.com" <danielwa@fifo99.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Steve Shih <sshih@cisco.com>,
"intel-wired-lan@lists.osuosl.org"
<intel-wired-lan@lists.osuosl.org>
Subject: Re: [Intel-wired-lan] [PATCH-RFC] drivers: net: ethernet: intel: e1000e: fix ethtool autoneg off for fiber
Date: Mon, 4 Apr 2016 16:03:50 -0700 [thread overview]
Message-ID: <5702F2D6.5010105@cisco.com> (raw)
In-Reply-To: <36CDDD56DDB4D44E911123902EFC26B05B440F17@HASMSX110.ger.corp.intel.com>
On 04/03/2016 07:23 AM, Ruinskiy, Dima wrote:
> I have a couple of comments (sorry for not getting to it a bit sooner).
>
>> For fiber media, e1000_get_settings should return ETH_TP_MDI_INVALID for
>> eth_tp_mdix_ctrl instead of ETH_TP_MDI_AUTO so subsequent e1000_set_settings
>> call would not fail with -EOPNOTSUPP.
> Should this be specific to fiber? Because the code just checks media != copper. There is at least other media type (e1000_media_type_internal_serdes), which will be affected by this change as well. What is the proper behavior in this case?
Steve is shy, so here are his comments,
<Steve>
Regarding the 1st comment. The issue affects all non-copper media. In
fact, we are using e1000_media_type_internal_serdes and are affected due
to the following check in e1000_set_settings:
/* MDI setting is only allowed when autoneg enabled because
* some hardware doesn't allow MDI setting when speed or
* duplex is forced.
*/
if (ecmd->eth_tp_mdix_ctrl) {
if (hw->phy.media_type != e1000_media_type_copper)
return -EOPNOTSUPP;
if ((ecmd->eth_tp_mdix_ctrl != ETH_TP_MDI_AUTO) &&
(ecmd->autoneg != AUTONEG_ENABLE)) {
e_err("forcing MDI/MDI-X state is not supported when link speed and/or
duplex are forced\n");
return -EINVAL;
}
}
</Steve>
>> e1000_set_spd_dplx should not automatically turn autoneg back on for forced
>> 1000 Mbps full duplex settings.
> I seem to remember that this code is there because the copper-based NICs only support 1000Mbps in auto-negotiation mode, not forced (this is according to the IEEE spec, as far as I know). I believe this code is there to ensure that a user can force the speed to 1000 via the API, and still have the link resolve correctly via auto-negotiation. I am concerned that changing this code will break clients that rely on it. Maybe it is best to also limit it to fiber devices only?
>
<Steve>
Regarding the 2nd comment. Yes, will limit to non-copper media.
</Steve>
He supplied me with a second patch which I can send.. Can we remove the
RFC this time ?
Daniel
next prev parent reply other threads:[~2016-04-04 23:03 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-25 21:58 [Intel-wired-lan] [PATCH-RFC] drivers: net: ethernet: intel: e1000e: fix ethtool autoneg off for fiber Daniel Walker
2016-03-25 21:58 ` Daniel Walker
2016-03-30 19:34 ` [Intel-wired-lan] " Daniel Walker
2016-03-30 19:34 ` Daniel Walker
2016-03-30 19:44 ` [Intel-wired-lan] " Jeff Kirsher
2016-03-30 19:44 ` Jeff Kirsher
2016-04-03 14:23 ` [Intel-wired-lan] " Ruinskiy, Dima
2016-04-03 14:23 ` Ruinskiy, Dima
2016-04-04 23:03 ` Daniel Walker [this message]
2016-04-04 23:03 ` Daniel Walker
2016-04-05 13:41 ` Ruinskiy, Dima
2016-04-05 13:41 ` Ruinskiy, Dima
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=5702F2D6.5010105@cisco.com \
--to=danielwa@cisco.com \
--cc=intel-wired-lan@osuosl.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.