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@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
Thanks for adding Renjith as reporter, it slipped my mind.
>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
>
> Tested-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx
>
> 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
