* [PATCH] net: ethtool: remove error check for legacy setting transceiver type
@ 2017-10-19 23:32 Niklas Söderlund
2017-10-19 23:38 ` Florian Fainelli
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Niklas Söderlund @ 2017-10-19 23:32 UTC (permalink / raw)
To: David S. Miller, Florian Fainelli, netdev
Cc: linux-renesas-soc, Geert Uytterhoeven, Renjith R V,
Niklas Söderlund
Commit 9cab88726929605 ("net: ethtool: Add back transceiver type")
restores the transceiver type to struct ethtool_link_settings and
convert_link_ksettings_to_legacy_settings() but forgets to remove the
error check for the same in convert_legacy_settings_to_link_ksettings().
This prevents older versions of ethtool to change link settings.
# ethtool --version
ethtool version 3.16
# ethtool -s eth0 autoneg on speed 100 duplex full
Cannot set new settings: Invalid argument
not setting speed
not setting duplex
not setting autoneg
While newer versions of ethtool works.
# ethtool --version
ethtool version 4.10
# ethtool -s eth0 autoneg on speed 100 duplex full
[ 57.703268] sh-eth ee700000.ethernet eth0: Link is Down
[ 59.618227] sh-eth ee700000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
Fixes: 19cab88726929605 ("net: ethtool: Add back transceiver type")
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
net/core/ethtool.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
Hi,
The commit 9cab88726929605 ("net: ethtool: Add back transceiver type")
itself is not enough to trigger the error, the commit after it is also
needed ceb628134a75564d ("net: phy: Keep reporting transceiver type").
This is because the later sets cmd->base.transceiver so that the type
can be reported and the fault triggered. I hope however I did the
correct thing with the Fixes tag.
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 3228411ada0fa77e..9a9a3d77e3274fc3 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -436,7 +436,7 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
EXPORT_SYMBOL(ethtool_convert_link_mode_to_legacy_u32);
/* return false if legacy contained non-0 deprecated fields
- * transceiver/maxtxpkt/maxrxpkt. rest of ksettings always updated
+ * maxtxpkt/maxrxpkt. rest of ksettings always updated
*/
static bool
convert_legacy_settings_to_link_ksettings(
@@ -451,8 +451,7 @@ convert_legacy_settings_to_link_ksettings(
* deprecated legacy fields, and they should not use
* %ETHTOOL_GLINKSETTINGS/%ETHTOOL_SLINKSETTINGS
*/
- if (legacy_settings->transceiver ||
- legacy_settings->maxtxpkt ||
+ if (legacy_settings->maxtxpkt ||
legacy_settings->maxrxpkt)
retval = false;
--
2.14.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ethtool: remove error check for legacy setting transceiver type
2017-10-19 23:32 [PATCH] net: ethtool: remove error check for legacy setting transceiver type Niklas Söderlund
@ 2017-10-19 23:38 ` Florian Fainelli
2017-10-20 7:15 ` Geert Uytterhoeven
2017-10-22 1:15 ` David Miller
2 siblings, 0 replies; 9+ messages in thread
From: Florian Fainelli @ 2017-10-19 23:38 UTC (permalink / raw)
To: Niklas Söderlund, David S. Miller, netdev
Cc: linux-renesas-soc, Geert Uytterhoeven, Renjith R V
On 10/19/2017 04:32 PM, Niklas Söderlund wrote:
> Commit 9cab88726929605 ("net: ethtool: Add back transceiver type")
> restores the transceiver type to struct ethtool_link_settings and
> convert_link_ksettings_to_legacy_settings() but forgets to remove the
> error check for the same in convert_legacy_settings_to_link_ksettings().
> This prevents older versions of ethtool to change link settings.
>
> # ethtool --version
> ethtool version 3.16
>
> # ethtool -s eth0 autoneg on speed 100 duplex full
> Cannot set new settings: Invalid argument
> not setting speed
> not setting duplex
> not setting autoneg
>
> While newer versions of ethtool works.
>
> # ethtool --version
> ethtool version 4.10
>
> # ethtool -s eth0 autoneg on speed 100 duplex full
> [ 57.703268] sh-eth ee700000.ethernet eth0: Link is Down
> [ 59.618227] sh-eth ee700000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
>
> Fixes: 19cab88726929605 ("net: ethtool: Add back transceiver type")
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Good catch, thanks Niklas! Surprisingly I was also testing with an older
version of ethtool but I realize I was mostly testing the "get" part and
not the "set" part, so clearly an oversight here...
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> net/core/ethtool.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> Hi,
>
> The commit 9cab88726929605 ("net: ethtool: Add back transceiver type")
> itself is not enough to trigger the error, the commit after it is also
> needed ceb628134a75564d ("net: phy: Keep reporting transceiver type").
> This is because the later sets cmd->base.transceiver so that the type
> can be reported and the fault triggered. I hope however I did the
> correct thing with the Fixes tag.
Both having made it in the same release, that should be fine, thanks!
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 3228411ada0fa77e..9a9a3d77e3274fc3 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -436,7 +436,7 @@ bool ethtool_convert_link_mode_to_legacy_u32(u32 *legacy_u32,
> EXPORT_SYMBOL(ethtool_convert_link_mode_to_legacy_u32);
>
> /* return false if legacy contained non-0 deprecated fields
> - * transceiver/maxtxpkt/maxrxpkt. rest of ksettings always updated
> + * maxtxpkt/maxrxpkt. rest of ksettings always updated
> */
> static bool
> convert_legacy_settings_to_link_ksettings(
> @@ -451,8 +451,7 @@ convert_legacy_settings_to_link_ksettings(
> * deprecated legacy fields, and they should not use
> * %ETHTOOL_GLINKSETTINGS/%ETHTOOL_SLINKSETTINGS
> */
> - if (legacy_settings->transceiver ||
> - legacy_settings->maxtxpkt ||
> + if (legacy_settings->maxtxpkt ||
> legacy_settings->maxrxpkt)
> retval = false;
>
>
--
Florian
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ethtool: remove error check for legacy setting transceiver type
2017-10-19 23:32 [PATCH] net: ethtool: remove error check for legacy setting transceiver type Niklas Söderlund
2017-10-19 23:38 ` Florian Fainelli
@ 2017-10-20 7:15 ` Geert Uytterhoeven
2017-10-20 9:41 ` Niklas Söderlund
2017-10-20 13:03 ` Niklas Söderlund
2017-10-22 1:15 ` David Miller
2 siblings, 2 replies; 9+ messages in thread
From: Geert Uytterhoeven @ 2017-10-20 7:15 UTC (permalink / raw)
To: Niklas Söderlund
Cc: David S. Miller, Florian Fainelli, netdev, Linux-Renesas, Renjith R V
Hi Niklas,
On Fri, Oct 20, 2017 at 1:32 AM, Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Commit 9cab88726929605 ("net: ethtool: Add back transceiver type")
> restores the transceiver type to struct ethtool_link_settings and
> convert_link_ksettings_to_legacy_settings() but forgets to remove the
> error check for the same in convert_legacy_settings_to_link_ksettings().
> This prevents older versions of ethtool to change link settings.
>
> # ethtool --version
> ethtool version 3.16
>
> # ethtool -s eth0 autoneg on speed 100 duplex full
> Cannot set new settings: Invalid argument
> not setting speed
> not setting duplex
> not setting autoneg
>
> While newer versions of ethtool works.
>
> # ethtool --version
> ethtool version 4.10
>
> # ethtool -s eth0 autoneg on speed 100 duplex full
> [ 57.703268] sh-eth ee700000.ethernet eth0: Link is Down
> [ 59.618227] sh-eth ee700000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
>
> Fixes: 19cab88726929605 ("net: ethtool: Add back transceiver type")
Thanks for your patch!
Reported-by: Renjith R V <renjith.rv@quest-global.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
While this fixed ethtool, unfortunately this revealed another issue: several
drivers call phy_ethtool_ksettings_set() while holding a driver-specific
spinlock, which leads to phy_start_aneg_priv() calling mutex_lock() with
interrupts disabled.
Backtrace for sh_eth:
BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
[ 161.636382] in_atomic(): 1, irqs_disabled(): 128, pid: 1684, name: ethtool
[ 161.643260] CPU: 1 PID: 1684 Comm: ethtool Not tainted
4.14.0-rc5-koelsch-00441-g8545ae86337930b0 #3649
[ 161.652653] Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
[ 161.659109] [<c020efe4>] (unwind_backtrace) from [<c020aa5c>]
(show_stack+0x10/0x14)
[ 161.666859] [<c020aa5c>] (show_stack) from [<c0723444>]
(dump_stack+0x7c/0x9c)
[ 161.674090] [<c0723444>] (dump_stack) from [<c023f760>]
(___might_sleep+0x128/0x164)
[ 161.681841] [<c023f760>] (___might_sleep) from [<c073894c>]
(mutex_lock+0x18/0x60)
[ 161.689418] [<c073894c>] (mutex_lock) from [<c0537c60>]
(phy_start_aneg_priv+0x28/0x120)
[ 161.697513] [<c0537c60>] (phy_start_aneg_priv) from [<c0537ee4>]
(phy_ethtool_ksettings_set+0xc0/0xcc)
[ 161.706824] [<c0537ee4>] (phy_ethtool_ksettings_set) from
[<c053fe58>] (sh_eth_set_link_ksettings+0x3c/0xa8)
[ 161.716657] [<c053fe58>] (sh_eth_set_link_ksettings) from
[<c0676f88>] (ethtool_set_settings+0x100/0x114)
[ 161.726229] [<c0676f88>] (ethtool_set_settings) from [<c0679ea0>]
(dev_ethtool+0x400/0x2248)
[ 161.734672] [<c0679ea0>] (dev_ethtool) from [<c068f850>]
(dev_ioctl+0x424/0x774)
[ 161.742074] [<c068f850>] (dev_ioctl) from [<c02f9a24>] (vfs_ioctl+0x20/0x34)
[ 161.749128] [<c02f9a24>] (vfs_ioctl) from [<c02fa1f0>]
(do_vfs_ioctl+0x6b4/0x7b8)
[ 161.756616] [<c02fa1f0>] (do_vfs_ioctl) from [<c02fa328>]
(SyS_ioctl+0x34/0x5c)
[ 161.763930] [<c02fa328>] (SyS_ioctl) from [<c0206e60>]
(ret_fast_syscall+0x0/0x40)
There's a similar dump for ravb.
I quick grep shows that at least the Broadcom B44 driver is also affected.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ethtool: remove error check for legacy setting transceiver type
2017-10-20 7:15 ` Geert Uytterhoeven
@ 2017-10-20 9:41 ` Niklas Söderlund
2017-10-20 13:03 ` Niklas Söderlund
1 sibling, 0 replies; 9+ messages in thread
From: Niklas Söderlund @ 2017-10-20 9:41 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: David S. Miller, Florian Fainelli, netdev, Linux-Renesas, Renjith R V
Hi Geert,
On 2017-10-20 09:15:50 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
>
> On Fri, Oct 20, 2017 at 1:32 AM, Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > Commit 9cab88726929605 ("net: ethtool: Add back transceiver type")
> > restores the transceiver type to struct ethtool_link_settings and
> > convert_link_ksettings_to_legacy_settings() but forgets to remove the
> > error check for the same in convert_legacy_settings_to_link_ksettings().
> > This prevents older versions of ethtool to change link settings.
> >
> > # ethtool --version
> > ethtool version 3.16
> >
> > # ethtool -s eth0 autoneg on speed 100 duplex full
> > Cannot set new settings: Invalid argument
> > not setting speed
> > not setting duplex
> > not setting autoneg
> >
> > While newer versions of ethtool works.
> >
> > # ethtool --version
> > ethtool version 4.10
> >
> > # ethtool -s eth0 autoneg on speed 100 duplex full
> > [ 57.703268] sh-eth ee700000.ethernet eth0: Link is Down
> > [ 59.618227] sh-eth ee700000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> >
> > Fixes: 19cab88726929605 ("net: ethtool: Add back transceiver type")
>
> Thanks for your patch!
>
> Reported-by: Renjith R V <renjith.rv@quest-global.com>
Thanks for adding Renjith as reporter, it slipped my mind.
>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> While this fixed ethtool, unfortunately this revealed another issue: several
> drivers call phy_ethtool_ksettings_set() while holding a driver-specific
> spinlock, which leads to phy_start_aneg_priv() calling mutex_lock() with
> interrupts disabled.
>
> Backtrace for sh_eth:
>
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
> [ 161.636382] in_atomic(): 1, irqs_disabled(): 128, pid: 1684, name: ethtool
> [ 161.643260] CPU: 1 PID: 1684 Comm: ethtool Not tainted
> 4.14.0-rc5-koelsch-00441-g8545ae86337930b0 #3649
> [ 161.652653] Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> [ 161.659109] [<c020efe4>] (unwind_backtrace) from [<c020aa5c>]
> (show_stack+0x10/0x14)
> [ 161.666859] [<c020aa5c>] (show_stack) from [<c0723444>]
> (dump_stack+0x7c/0x9c)
> [ 161.674090] [<c0723444>] (dump_stack) from [<c023f760>]
> (___might_sleep+0x128/0x164)
> [ 161.681841] [<c023f760>] (___might_sleep) from [<c073894c>]
> (mutex_lock+0x18/0x60)
> [ 161.689418] [<c073894c>] (mutex_lock) from [<c0537c60>]
> (phy_start_aneg_priv+0x28/0x120)
> [ 161.697513] [<c0537c60>] (phy_start_aneg_priv) from [<c0537ee4>]
> (phy_ethtool_ksettings_set+0xc0/0xcc)
> [ 161.706824] [<c0537ee4>] (phy_ethtool_ksettings_set) from
> [<c053fe58>] (sh_eth_set_link_ksettings+0x3c/0xa8)
> [ 161.716657] [<c053fe58>] (sh_eth_set_link_ksettings) from
> [<c0676f88>] (ethtool_set_settings+0x100/0x114)
> [ 161.726229] [<c0676f88>] (ethtool_set_settings) from [<c0679ea0>]
> (dev_ethtool+0x400/0x2248)
> [ 161.734672] [<c0679ea0>] (dev_ethtool) from [<c068f850>]
> (dev_ioctl+0x424/0x774)
> [ 161.742074] [<c068f850>] (dev_ioctl) from [<c02f9a24>] (vfs_ioctl+0x20/0x34)
> [ 161.749128] [<c02f9a24>] (vfs_ioctl) from [<c02fa1f0>]
> (do_vfs_ioctl+0x6b4/0x7b8)
> [ 161.756616] [<c02fa1f0>] (do_vfs_ioctl) from [<c02fa328>]
> (SyS_ioctl+0x34/0x5c)
> [ 161.763930] [<c02fa328>] (SyS_ioctl) from [<c0206e60>]
> (ret_fast_syscall+0x0/0x40)
>
> There's a similar dump for ravb.
>
> I quick grep shows that at least the Broadcom B44 driver is also affected.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ethtool: remove error check for legacy setting transceiver type
@ 2017-10-20 9:41 ` Niklas Söderlund
0 siblings, 0 replies; 9+ messages in thread
From: Niklas Söderlund @ 2017-10-20 9:41 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: David S. Miller, Florian Fainelli, netdev, Linux-Renesas, Renjith R V
Hi Geert,
On 2017-10-20 09:15:50 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
>
> On Fri, Oct 20, 2017 at 1:32 AM, Niklas S�derlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > Commit 9cab88726929605 ("net: ethtool: Add back transceiver type")
> > restores the transceiver type to struct ethtool_link_settings and
> > convert_link_ksettings_to_legacy_settings() but forgets to remove the
> > error check for the same in convert_legacy_settings_to_link_ksettings().
> > This prevents older versions of ethtool to change link settings.
> >
> > # ethtool --version
> > ethtool version 3.16
> >
> > # ethtool -s eth0 autoneg on speed 100 duplex full
> > Cannot set new settings: Invalid argument
> > not setting speed
> > not setting duplex
> > not setting autoneg
> >
> > While newer versions of ethtool works.
> >
> > # ethtool --version
> > ethtool version 4.10
> >
> > # ethtool -s eth0 autoneg on speed 100 duplex full
> > [ 57.703268] sh-eth ee700000.ethernet eth0: Link is Down
> > [ 59.618227] sh-eth ee700000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> >
> > Fixes: 19cab88726929605 ("net: ethtool: Add back transceiver type")
>
> Thanks for your patch!
>
> Reported-by: Renjith R V <renjith.rv@quest-global.com>
Thanks for adding Renjith as reporter, it slipped my mind.
>
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> While this fixed ethtool, unfortunately this revealed another issue: several
> drivers call phy_ethtool_ksettings_set() while holding a driver-specific
> spinlock, which leads to phy_start_aneg_priv() calling mutex_lock() with
> interrupts disabled.
>
> Backtrace for sh_eth:
>
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
> [ 161.636382] in_atomic(): 1, irqs_disabled(): 128, pid: 1684, name: ethtool
> [ 161.643260] CPU: 1 PID: 1684 Comm: ethtool Not tainted
> 4.14.0-rc5-koelsch-00441-g8545ae86337930b0 #3649
> [ 161.652653] Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> [ 161.659109] [<c020efe4>] (unwind_backtrace) from [<c020aa5c>]
> (show_stack+0x10/0x14)
> [ 161.666859] [<c020aa5c>] (show_stack) from [<c0723444>]
> (dump_stack+0x7c/0x9c)
> [ 161.674090] [<c0723444>] (dump_stack) from [<c023f760>]
> (___might_sleep+0x128/0x164)
> [ 161.681841] [<c023f760>] (___might_sleep) from [<c073894c>]
> (mutex_lock+0x18/0x60)
> [ 161.689418] [<c073894c>] (mutex_lock) from [<c0537c60>]
> (phy_start_aneg_priv+0x28/0x120)
> [ 161.697513] [<c0537c60>] (phy_start_aneg_priv) from [<c0537ee4>]
> (phy_ethtool_ksettings_set+0xc0/0xcc)
> [ 161.706824] [<c0537ee4>] (phy_ethtool_ksettings_set) from
> [<c053fe58>] (sh_eth_set_link_ksettings+0x3c/0xa8)
> [ 161.716657] [<c053fe58>] (sh_eth_set_link_ksettings) from
> [<c0676f88>] (ethtool_set_settings+0x100/0x114)
> [ 161.726229] [<c0676f88>] (ethtool_set_settings) from [<c0679ea0>]
> (dev_ethtool+0x400/0x2248)
> [ 161.734672] [<c0679ea0>] (dev_ethtool) from [<c068f850>]
> (dev_ioctl+0x424/0x774)
> [ 161.742074] [<c068f850>] (dev_ioctl) from [<c02f9a24>] (vfs_ioctl+0x20/0x34)
> [ 161.749128] [<c02f9a24>] (vfs_ioctl) from [<c02fa1f0>]
> (do_vfs_ioctl+0x6b4/0x7b8)
> [ 161.756616] [<c02fa1f0>] (do_vfs_ioctl) from [<c02fa328>]
> (SyS_ioctl+0x34/0x5c)
> [ 161.763930] [<c02fa328>] (SyS_ioctl) from [<c0206e60>]
> (ret_fast_syscall+0x0/0x40)
>
> There's a similar dump for ravb.
>
> I quick grep shows that at least the Broadcom B44 driver is also affected.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
--
Regards,
Niklas S�derlund
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ethtool: remove error check for legacy setting transceiver type
2017-10-20 7:15 ` Geert Uytterhoeven
@ 2017-10-20 13:03 ` Niklas Söderlund
2017-10-20 13:03 ` Niklas Söderlund
1 sibling, 0 replies; 9+ messages in thread
From: Niklas Söderlund @ 2017-10-20 13:03 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: David S. Miller, Florian Fainelli, netdev, Linux-Renesas, Renjith R V
Hi Geert,
On 2017-10-20 09:15:50 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
>
> On Fri, Oct 20, 2017 at 1:32 AM, Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > Commit 9cab88726929605 ("net: ethtool: Add back transceiver type")
> > restores the transceiver type to struct ethtool_link_settings and
> > convert_link_ksettings_to_legacy_settings() but forgets to remove the
> > error check for the same in convert_legacy_settings_to_link_ksettings().
> > This prevents older versions of ethtool to change link settings.
> >
> > # ethtool --version
> > ethtool version 3.16
> >
> > # ethtool -s eth0 autoneg on speed 100 duplex full
> > Cannot set new settings: Invalid argument
> > not setting speed
> > not setting duplex
> > not setting autoneg
> >
> > While newer versions of ethtool works.
> >
> > # ethtool --version
> > ethtool version 4.10
> >
> > # ethtool -s eth0 autoneg on speed 100 duplex full
> > [ 57.703268] sh-eth ee700000.ethernet eth0: Link is Down
> > [ 59.618227] sh-eth ee700000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> >
> > Fixes: 19cab88726929605 ("net: ethtool: Add back transceiver type")
>
> Thanks for your patch!
>
> Reported-by: Renjith R V <renjith.rv@quest-global.com>
>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> While this fixed ethtool, unfortunately this revealed another issue: several
> drivers call phy_ethtool_ksettings_set() while holding a driver-specific
> spinlock, which leads to phy_start_aneg_priv() calling mutex_lock() with
> interrupts disabled.
This seems to be a old issue, I can reproduce it on v4.2, releases
earlier than that is problematic for me to compile using gcc7 so I gave
up. I don't know if this ever worked without triggering the BUG but it
is present at least from v4.2.
>
> Backtrace for sh_eth:
>
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
> [ 161.636382] in_atomic(): 1, irqs_disabled(): 128, pid: 1684, name: ethtool
> [ 161.643260] CPU: 1 PID: 1684 Comm: ethtool Not tainted
> 4.14.0-rc5-koelsch-00441-g8545ae86337930b0 #3649
> [ 161.652653] Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> [ 161.659109] [<c020efe4>] (unwind_backtrace) from [<c020aa5c>]
> (show_stack+0x10/0x14)
> [ 161.666859] [<c020aa5c>] (show_stack) from [<c0723444>]
> (dump_stack+0x7c/0x9c)
> [ 161.674090] [<c0723444>] (dump_stack) from [<c023f760>]
> (___might_sleep+0x128/0x164)
> [ 161.681841] [<c023f760>] (___might_sleep) from [<c073894c>]
> (mutex_lock+0x18/0x60)
> [ 161.689418] [<c073894c>] (mutex_lock) from [<c0537c60>]
> (phy_start_aneg_priv+0x28/0x120)
> [ 161.697513] [<c0537c60>] (phy_start_aneg_priv) from [<c0537ee4>]
> (phy_ethtool_ksettings_set+0xc0/0xcc)
> [ 161.706824] [<c0537ee4>] (phy_ethtool_ksettings_set) from
> [<c053fe58>] (sh_eth_set_link_ksettings+0x3c/0xa8)
> [ 161.716657] [<c053fe58>] (sh_eth_set_link_ksettings) from
> [<c0676f88>] (ethtool_set_settings+0x100/0x114)
> [ 161.726229] [<c0676f88>] (ethtool_set_settings) from [<c0679ea0>]
> (dev_ethtool+0x400/0x2248)
> [ 161.734672] [<c0679ea0>] (dev_ethtool) from [<c068f850>]
> (dev_ioctl+0x424/0x774)
> [ 161.742074] [<c068f850>] (dev_ioctl) from [<c02f9a24>] (vfs_ioctl+0x20/0x34)
> [ 161.749128] [<c02f9a24>] (vfs_ioctl) from [<c02fa1f0>]
> (do_vfs_ioctl+0x6b4/0x7b8)
> [ 161.756616] [<c02fa1f0>] (do_vfs_ioctl) from [<c02fa328>]
> (SyS_ioctl+0x34/0x5c)
> [ 161.763930] [<c02fa328>] (SyS_ioctl) from [<c0206e60>]
> (ret_fast_syscall+0x0/0x40)
>
> There's a similar dump for ravb.
>
> I quick grep shows that at least the Broadcom B44 driver is also affected.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ethtool: remove error check for legacy setting transceiver type
@ 2017-10-20 13:03 ` Niklas Söderlund
0 siblings, 0 replies; 9+ messages in thread
From: Niklas Söderlund @ 2017-10-20 13:03 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: David S. Miller, Florian Fainelli, netdev, Linux-Renesas, Renjith R V
Hi Geert,
On 2017-10-20 09:15:50 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
>
> On Fri, Oct 20, 2017 at 1:32 AM, Niklas S�derlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > Commit 9cab88726929605 ("net: ethtool: Add back transceiver type")
> > restores the transceiver type to struct ethtool_link_settings and
> > convert_link_ksettings_to_legacy_settings() but forgets to remove the
> > error check for the same in convert_legacy_settings_to_link_ksettings().
> > This prevents older versions of ethtool to change link settings.
> >
> > # ethtool --version
> > ethtool version 3.16
> >
> > # ethtool -s eth0 autoneg on speed 100 duplex full
> > Cannot set new settings: Invalid argument
> > not setting speed
> > not setting duplex
> > not setting autoneg
> >
> > While newer versions of ethtool works.
> >
> > # ethtool --version
> > ethtool version 4.10
> >
> > # ethtool -s eth0 autoneg on speed 100 duplex full
> > [ 57.703268] sh-eth ee700000.ethernet eth0: Link is Down
> > [ 59.618227] sh-eth ee700000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> >
> > Fixes: 19cab88726929605 ("net: ethtool: Add back transceiver type")
>
> Thanks for your patch!
>
> Reported-by: Renjith R V <renjith.rv@quest-global.com>
>
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
>
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> While this fixed ethtool, unfortunately this revealed another issue: several
> drivers call phy_ethtool_ksettings_set() while holding a driver-specific
> spinlock, which leads to phy_start_aneg_priv() calling mutex_lock() with
> interrupts disabled.
This seems to be a old issue, I can reproduce it on v4.2, releases
earlier than that is problematic for me to compile using gcc7 so I gave
up. I don't know if this ever worked without triggering the BUG but it
is present at least from v4.2.
>
> Backtrace for sh_eth:
>
> BUG: sleeping function called from invalid context at kernel/locking/mutex.c:238
> [ 161.636382] in_atomic(): 1, irqs_disabled(): 128, pid: 1684, name: ethtool
> [ 161.643260] CPU: 1 PID: 1684 Comm: ethtool Not tainted
> 4.14.0-rc5-koelsch-00441-g8545ae86337930b0 #3649
> [ 161.652653] Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
> [ 161.659109] [<c020efe4>] (unwind_backtrace) from [<c020aa5c>]
> (show_stack+0x10/0x14)
> [ 161.666859] [<c020aa5c>] (show_stack) from [<c0723444>]
> (dump_stack+0x7c/0x9c)
> [ 161.674090] [<c0723444>] (dump_stack) from [<c023f760>]
> (___might_sleep+0x128/0x164)
> [ 161.681841] [<c023f760>] (___might_sleep) from [<c073894c>]
> (mutex_lock+0x18/0x60)
> [ 161.689418] [<c073894c>] (mutex_lock) from [<c0537c60>]
> (phy_start_aneg_priv+0x28/0x120)
> [ 161.697513] [<c0537c60>] (phy_start_aneg_priv) from [<c0537ee4>]
> (phy_ethtool_ksettings_set+0xc0/0xcc)
> [ 161.706824] [<c0537ee4>] (phy_ethtool_ksettings_set) from
> [<c053fe58>] (sh_eth_set_link_ksettings+0x3c/0xa8)
> [ 161.716657] [<c053fe58>] (sh_eth_set_link_ksettings) from
> [<c0676f88>] (ethtool_set_settings+0x100/0x114)
> [ 161.726229] [<c0676f88>] (ethtool_set_settings) from [<c0679ea0>]
> (dev_ethtool+0x400/0x2248)
> [ 161.734672] [<c0679ea0>] (dev_ethtool) from [<c068f850>]
> (dev_ioctl+0x424/0x774)
> [ 161.742074] [<c068f850>] (dev_ioctl) from [<c02f9a24>] (vfs_ioctl+0x20/0x34)
> [ 161.749128] [<c02f9a24>] (vfs_ioctl) from [<c02fa1f0>]
> (do_vfs_ioctl+0x6b4/0x7b8)
> [ 161.756616] [<c02fa1f0>] (do_vfs_ioctl) from [<c02fa328>]
> (SyS_ioctl+0x34/0x5c)
> [ 161.763930] [<c02fa328>] (SyS_ioctl) from [<c0206e60>]
> (ret_fast_syscall+0x0/0x40)
>
> There's a similar dump for ravb.
>
> I quick grep shows that at least the Broadcom B44 driver is also affected.
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
--
Regards,
Niklas S�derlund
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ethtool: remove error check for legacy setting transceiver type
2017-10-19 23:32 [PATCH] net: ethtool: remove error check for legacy setting transceiver type Niklas Söderlund
@ 2017-10-22 1:15 ` David Miller
2017-10-20 7:15 ` Geert Uytterhoeven
2017-10-22 1:15 ` David Miller
2 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2017-10-22 1:15 UTC (permalink / raw)
To: niklas.soderlund+renesas
Cc: f.fainelli, netdev, linux-renesas-soc, geert, renjith.rv
From: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Date: Fri, 20 Oct 2017 01:32:08 +0200
> Commit 9cab88726929605 ("net: ethtool: Add back transceiver type")
> restores the transceiver type to struct ethtool_link_settings and
> convert_link_ksettings_to_legacy_settings() but forgets to remove the
> error check for the same in convert_legacy_settings_to_link_ksettings().
> This prevents older versions of ethtool to change link settings.
>
> # ethtool --version
> ethtool version 3.16
>
> # ethtool -s eth0 autoneg on speed 100 duplex full
> Cannot set new settings: Invalid argument
> not setting speed
> not setting duplex
> not setting autoneg
>
> While newer versions of ethtool works.
>
> # ethtool --version
> ethtool version 4.10
>
> # ethtool -s eth0 autoneg on speed 100 duplex full
> [ 57.703268] sh-eth ee700000.ethernet eth0: Link is Down
> [ 59.618227] sh-eth ee700000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
>
> Fixes: 19cab88726929605 ("net: ethtool: Add back transceiver type")
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Applied.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: ethtool: remove error check for legacy setting transceiver type
@ 2017-10-22 1:15 ` David Miller
0 siblings, 0 replies; 9+ messages in thread
From: David Miller @ 2017-10-22 1:15 UTC (permalink / raw)
To: niklas.soderlund+renesas
Cc: f.fainelli, netdev, linux-renesas-soc, geert, renjith.rv
From: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
Date: Fri, 20 Oct 2017 01:32:08 +0200
> Commit 9cab88726929605 ("net: ethtool: Add back transceiver type")
> restores the transceiver type to struct ethtool_link_settings and
> convert_link_ksettings_to_legacy_settings() but forgets to remove the
> error check for the same in convert_legacy_settings_to_link_ksettings().
> This prevents older versions of ethtool to change link settings.
>
> # ethtool --version
> ethtool version 3.16
>
> # ethtool -s eth0 autoneg on speed 100 duplex full
> Cannot set new settings: Invalid argument
> not setting speed
> not setting duplex
> not setting autoneg
>
> While newer versions of ethtool works.
>
> # ethtool --version
> ethtool version 4.10
>
> # ethtool -s eth0 autoneg on speed 100 duplex full
> [ 57.703268] sh-eth ee700000.ethernet eth0: Link is Down
> [ 59.618227] sh-eth ee700000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
>
> Fixes: 19cab88726929605 ("net: ethtool: Add back transceiver type")
> Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
Applied.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-10-22 1:15 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-19 23:32 [PATCH] net: ethtool: remove error check for legacy setting transceiver type Niklas Söderlund
2017-10-19 23:38 ` Florian Fainelli
2017-10-20 7:15 ` Geert Uytterhoeven
2017-10-20 9:41 ` Niklas Söderlund
2017-10-20 9:41 ` Niklas Söderlund
2017-10-20 13:03 ` Niklas Söderlund
2017-10-20 13:03 ` Niklas Söderlund
2017-10-22 1:15 ` David Miller
2017-10-22 1:15 ` David Miller
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.