Skip to content

L
linux
Commit 0f62aeec authored by David S. Miller's avatar David S. Miller
Browse files
Options

Merge branch 'ravb-sh_eth-fix-sleep-in-atomic-by-reusing-shared-ethtool-handlers' 

Vladimir Zapolskiy says:

====================
ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

For ages trivial changes to RAVB and SuperH ethernet links by means of
standard 'ethtool' trigger a 'sleeping function called from invalid
context' bug, to visualize it on r8a7795 ULCB:

  % ethtool -r eth0
  BUG: sleeping function called from invalid context at kernel/locking/mutex.c:747
  in_atomic(): 1, irqs_disabled(): 128, pid: 554, name: ethtool
  INFO: lockdep is turned off.
  irq event stamp: 0
  hardirqs last  enabled at (0): [<0000000000000000>]           (null)
  hardirqs last disabled at (0): [<ffff0000080e1d3c>] copy_process.isra.7.part.8+0x2cc/0x1918
  softirqs last  enabled at (0): [<ffff0000080e1d3c>] copy_process.isra.7.part.8+0x2cc/0x1918
  softirqs last disabled at (0): [<0000000000000000>]           (null)
  CPU: 5 PID: 554 Comm: ethtool Not tainted 4.17.0-rc4-arm64-renesas+ #33
  Hardware name: Renesas H3ULCB board based on r8a7795 ES2.0+ (DT)
  Call trace:
   dump_backtrace+0x0/0x198
   show_stack+0x24/0x30
   dump_stack+0xb8/0xf4
   ___might_sleep+0x1c8/0x1f8
   __might_sleep+0x58/0x90
   __mutex_lock+0x50/0x890
   mutex_lock_nested+0x3c/0x50
   phy_start_aneg_priv+0x38/0x180
   phy_start_aneg+0x24/0x30
   ravb_nway_reset+0x3c/0x68
   dev_ethtool+0x3dc/0x2338
   dev_ioctl+0x19c/0x490
   sock_do_ioctl+0xe0/0x238
   sock_ioctl+0x254/0x460
   do_vfs_ioctl+0xb0/0x918
   ksys_ioctl+0x50/0x80
   sys_ioctl+0x34/0x48
   __sys_trace_return+0x0/0x4

The root cause is that an attempt to modify ECMR and GECMR registers
only when RX/TX function is disabled was too overcomplicated in its
original implementation, also processing of an optional Link Change
interrupt added even more complexity, as a result the implementation
was error prone.

The new locking scheme is confirmed to be correct by dumping driver
specific and generic PHY framework function calls with aid of ftrace
while running more or less advanced tests.

Please note that sh_eth patches from the series were built-tested only.

On purpose I do not add Fixes tags, the reused PHY handlers were added
way later than the fixed problems were firstly found in the drivers.

Changes from v1 to v2:
* the original patches are split to bugfixes and enhancements only,
  both v1 and v2 series are absolutely equal in total, thus I omit
  description of changes in individual patches,
* the latter implies that there should be no strict need for retesting,
  but because formally two series are different, I have to drop the tags
  given by Geert and Andrew, please send your tags again.
====================
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parents 70ba5b6d 44f3d558
Hide whitespace changes
Inline Side-by-side
Showing with 34 additions and 153 deletions
drivers/net/ethernet/renesas/ravb_main.c
View file @ 0f62aeec
......@@ -980,6 +980,13 @@ static void ravb_adjust_link(struct net_device *ndev)
struct ravb_private *priv = netdev_priv(ndev);
struct phy_device *phydev = ndev->phydev;
bool new_state = false;
unsigned long flags;
spin_lock_irqsave(&priv->lock, flags);
/* Disable TX and RX right over here, if E-MAC change is ignored */
if (priv->no_avb_link)
ravb_rcv_snd_disable(ndev);
if (phydev->link) {
if (phydev->duplex != priv->duplex) {
......@@ -997,18 +1004,21 @@ static void ravb_adjust_link(struct net_device *ndev)
ravb_modify(ndev, ECMR, ECMR_TXF, 0);
new_state = true;
priv->link = phydev->link;
if (priv->no_avb_link)
ravb_rcv_snd_enable(ndev);
}
} else if (priv->link) {
new_state = true;
priv->link = 0;
priv->speed = 0;
priv->duplex = -1;
if (priv->no_avb_link)
ravb_rcv_snd_disable(ndev);
}
/* Enable TX and RX right over here, if E-MAC change is ignored */
if (priv->no_avb_link && phydev->link)
ravb_rcv_snd_enable(ndev);
mmiowb();
spin_unlock_irqrestore(&priv->lock, flags);
if (new_state && netif_msg_link(priv))
phy_print_status(phydev);
}
......@@ -1096,75 +1106,6 @@ static int ravb_phy_start(struct net_device *ndev)
return 0;
}
static int ravb_get_link_ksettings(struct net_device *ndev,
struct ethtool_link_ksettings *cmd)
{
struct ravb_private *priv = netdev_priv(ndev);
unsigned long flags;
if (!ndev->phydev)
return -ENODEV;
spin_lock_irqsave(&priv->lock, flags);
phy_ethtool_ksettings_get(ndev->phydev, cmd);
spin_unlock_irqrestore(&priv->lock, flags);
return 0;
}
static int ravb_set_link_ksettings(struct net_device *ndev,
const struct ethtool_link_ksettings *cmd)
{
struct ravb_private *priv = netdev_priv(ndev);
unsigned long flags;
int error;
if (!ndev->phydev)
return -ENODEV;
spin_lock_irqsave(&priv->lock, flags);
/* Disable TX and RX */
ravb_rcv_snd_disable(ndev);
error = phy_ethtool_ksettings_set(ndev->phydev, cmd);
if (error)
goto error_exit;
if (cmd->base.duplex == DUPLEX_FULL)
priv->duplex = 1;
else
priv->duplex = 0;
ravb_set_duplex(ndev);
error_exit:
mdelay(1);
/* Enable TX and RX */
ravb_rcv_snd_enable(ndev);
mmiowb();
spin_unlock_irqrestore(&priv->lock, flags);
return error;
}
static int ravb_nway_reset(struct net_device *ndev)
{
struct ravb_private *priv = netdev_priv(ndev);
int error = -ENODEV;
unsigned long flags;
if (ndev->phydev) {
spin_lock_irqsave(&priv->lock, flags);
error = phy_start_aneg(ndev->phydev);
spin_unlock_irqrestore(&priv->lock, flags);
}
return error;
}
static u32 ravb_get_msglevel(struct net_device *ndev)
{
struct ravb_private *priv = netdev_priv(ndev);
......@@ -1377,7 +1318,7 @@ static int ravb_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
}
static const struct ethtool_ops ravb_ethtool_ops = {
.nway_reset = ravb_nway_reset,
.nway_reset = phy_ethtool_nway_reset,
.get_msglevel = ravb_get_msglevel,
.set_msglevel = ravb_set_msglevel,
.get_link = ethtool_op_get_link,
......@@ -1387,8 +1328,8 @@ static const struct ethtool_ops ravb_ethtool_ops = {
.get_ringparam = ravb_get_ringparam,
.set_ringparam = ravb_set_ringparam,
.get_ts_info = ravb_get_ts_info,
.get_link_ksettings = ravb_get_link_ksettings,
.set_link_ksettings = ravb_set_link_ksettings,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
.get_wol = ravb_get_wol,
.set_wol = ravb_set_wol,
};
......
drivers/net/ethernet/renesas/sh_eth.c
View file @ 0f62aeec
......@@ -1927,8 +1927,15 @@ static void sh_eth_adjust_link(struct net_device *ndev)
{
struct sh_eth_private *mdp = netdev_priv(ndev);
struct phy_device *phydev = ndev->phydev;
unsigned long flags;
int new_state = 0;
spin_lock_irqsave(&mdp->lock, flags);
/* Disable TX and RX right over here, if E-MAC change is ignored */
if (mdp->cd->no_psr || mdp->no_ether_link)
sh_eth_rcv_snd_disable(ndev);
if (phydev->link) {
if (phydev->duplex != mdp->duplex) {
new_state = 1;
......@@ -1947,18 +1954,21 @@ static void sh_eth_adjust_link(struct net_device *ndev)
sh_eth_modify(ndev, ECMR, ECMR_TXF, 0);
new_state = 1;
mdp->link = phydev->link;
if (mdp->cd->no_psr || mdp->no_ether_link)
sh_eth_rcv_snd_enable(ndev);
}
} else if (mdp->link) {
new_state = 1;
mdp->link = 0;
mdp->speed = 0;
mdp->duplex = -1;
if (mdp->cd->no_psr || mdp->no_ether_link)
sh_eth_rcv_snd_disable(ndev);
}
/* Enable TX and RX right over here, if E-MAC change is ignored */
if ((mdp->cd->no_psr || mdp->no_ether_link) && phydev->link)
sh_eth_rcv_snd_enable(ndev);
mmiowb();
spin_unlock_irqrestore(&mdp->lock, flags);
if (new_state && netif_msg_link(mdp))
phy_print_status(phydev);
}
......@@ -2030,60 +2040,6 @@ static int sh_eth_phy_start(struct net_device *ndev)
return 0;
}
static int sh_eth_get_link_ksettings(struct net_device *ndev,
struct ethtool_link_ksettings *cmd)
{
struct sh_eth_private *mdp = netdev_priv(ndev);
unsigned long flags;
if (!ndev->phydev)
return -ENODEV;
spin_lock_irqsave(&mdp->lock, flags);
phy_ethtool_ksettings_get(ndev->phydev, cmd);
spin_unlock_irqrestore(&mdp->lock, flags);
return 0;
}
static int sh_eth_set_link_ksettings(struct net_device *ndev,
const struct ethtool_link_ksettings *cmd)
{
struct sh_eth_private *mdp = netdev_priv(ndev);
unsigned long flags;
int ret;
if (!ndev->phydev)
return -ENODEV;
spin_lock_irqsave(&mdp->lock, flags);
/* disable tx and rx */
sh_eth_rcv_snd_disable(ndev);
ret = phy_ethtool_ksettings_set(ndev->phydev, cmd);
if (ret)
goto error_exit;
if (cmd->base.duplex == DUPLEX_FULL)
mdp->duplex = 1;
else
mdp->duplex = 0;
if (mdp->cd->set_duplex)
mdp->cd->set_duplex(ndev);
error_exit:
mdelay(1);
/* enable tx and rx */
sh_eth_rcv_snd_enable(ndev);
spin_unlock_irqrestore(&mdp->lock, flags);
return ret;
}
/* If it is ever necessary to increase SH_ETH_REG_DUMP_MAX_REGS, the
* version must be bumped as well. Just adding registers up to that
* limit is fine, as long as the existing register indices don't
......@@ -2263,22 +2219,6 @@ static void sh_eth_get_regs(struct net_device *ndev, struct ethtool_regs *regs,
pm_runtime_put_sync(&mdp->pdev->dev);
}
static int sh_eth_nway_reset(struct net_device *ndev)
{
struct sh_eth_private *mdp = netdev_priv(ndev);
unsigned long flags;
int ret;
if (!ndev->phydev)
return -ENODEV;
spin_lock_irqsave(&mdp->lock, flags);
ret = phy_start_aneg(ndev->phydev);
spin_unlock_irqrestore(&mdp->lock, flags);
return ret;
}
static u32 sh_eth_get_msglevel(struct net_device *ndev)
{
struct sh_eth_private *mdp = netdev_priv(ndev);
......@@ -2429,7 +2369,7 @@ static int sh_eth_set_wol(struct net_device *ndev, struct ethtool_wolinfo *wol)
static const struct ethtool_ops sh_eth_ethtool_ops = {
.get_regs_len = sh_eth_get_regs_len,
.get_regs = sh_eth_get_regs,
.nway_reset = sh_eth_nway_reset,
.nway_reset = phy_ethtool_nway_reset,
.get_msglevel = sh_eth_get_msglevel,
.set_msglevel = sh_eth_set_msglevel,
.get_link = ethtool_op_get_link,
......@@ -2438,8 +2378,8 @@ static const struct ethtool_ops sh_eth_ethtool_ops = {
.get_sset_count = sh_eth_get_sset_count,
.get_ringparam = sh_eth_get_ringparam,
.set_ringparam = sh_eth_set_ringparam,
.get_link_ksettings = sh_eth_get_link_ksettings,
.set_link_ksettings = sh_eth_set_link_ksettings,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
.get_wol = sh_eth_get_wol,
.set_wol = sh_eth_set_wol,
};
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7