[PATCH v2 00/14] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[PATCH v2 00/14] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[Vladimir Zapolskiy]

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.

Vladimir Zapolskiy (14):
  sh_eth: fix invalid context bug while calling auto-negotiation by ethtool
  sh_eth: fix invalid context bug while changing link options by ethtool
  sh_eth: simplify link auto-negotiation by ethtool
  sh_eth: remove custom .nway_reset from ethtool ops
  sh_eth: remove useless serialization in sh_eth_get_link_ksettings()
  sh_eth: remove custom .get_link_ksettings from ethtool ops
  sh_eth: remove custom .set_link_ksettings from ethtool ops
  ravb: fix invalid context bug while calling auto-negotiation by ethtool
  ravb: fix invalid context bug while changing link options by ethtool
  ravb: simplify link auto-negotiation by ethtool
  ravb: remove custom .nway_reset from ethtool ops
  ravb: remove useless serialization in ravb_get_link_ksettings()
  ravb: remove custom .get_link_ksettings from ethtool ops
  ravb: remove custom .set_link_ksettings from ethtool ops

 drivers/net/ethernet/renesas/ravb_main.c | 93 +++++------------------
 drivers/net/ethernet/renesas/sh_eth.c    | 94 +++++-------------------
 2 files changed, 34 insertions(+), 153 deletions(-)

-- 
2.17.1

[PATCH v2 01/14] sh_eth: fix invalid context bug while calling auto-negotiation by ethtool

[Vladimir Zapolskiy]

Since commit 35b5f6b1a82b ("PHYLIB: Locking fixes for PHY I/O
potentially sleeping") phy_start_aneg() function utilizes a mutex
to serialize changes to phy state, however the helper function is
called in atomic context.

The bug can be reproduced by running "ethtool -r" command, the bug
is reported if CONFIG_DEBUG_ATOMIC_SLEEP build option is enabled.

Fixes: dc19e4e5e02f ("sh: sh_eth: Add support ethtool")
Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index e9007b613f17..e8aca46bb925 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2265,18 +2265,10 @@ static void sh_eth_get_regs(struct net_device *ndev, struct ethtool_regs *regs,
 
 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;
+	return phy_start_aneg(ndev->phydev);
 }
 
 static u32 sh_eth_get_msglevel(struct net_device *ndev)
-- 
2.17.1

Re: [PATCH v2 01/14] sh_eth: fix invalid context bug while calling auto-negotiation by ethtool

[Sergei Shtylyov]

Hello!

On 7/4/2018 11:12 AM, Vladimir Zapolskiy wrote:

> Since commit 35b5f6b1a82b ("PHYLIB: Locking fixes for PHY I/O
> potentially sleeping") phy_start_aneg() function utilizes a mutex
> to serialize changes to phy state, however the helper function is
> called in atomic context.
> 
> The bug can be reproduced by running "ethtool -r" command, the bug
> is reported if CONFIG_DEBUG_ATOMIC_SLEEP build option is enabled.
> 
> Fixes: dc19e4e5e02f ("sh: sh_eth: Add support ethtool")
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

    The patch obviously does what it promises, however I'm still not sure 
whether it should be placed after the next fix, not before...

MBR, Sergei

[PATCH v2 02/14] sh_eth: fix invalid context bug while changing link options by ethtool

[Vladimir Zapolskiy]

The change fixes sleep in atomic context bug, which is encountered
every time when link settings are changed by ethtool.

Since commit 35b5f6b1a82b ("PHYLIB: Locking fixes for PHY I/O
potentially sleeping") phy_start_aneg() function utilizes a mutex
to serialize changes to phy state, however that helper function is
called in atomic context under a grabbed spinlock, because
phy_start_aneg() is called by phy_ethtool_ksettings_set() and by
replaced phy_ethtool_sset() helpers from phylib.

Now duplex mode setting is enforced in sh_eth_adjust_link() only,
also now RX/TX is disabled when link is put down or modifications
to E-MAC registers ECMR and GECMR are expected for both cases of
checked and ignored link status pin state from E-MAC interrupt handler.

For reference the change is a partial rework of commit 1e1b812bbe10
("sh_eth: fix handling of no LINK signal").

Fixes: dc19e4e5e02f ("sh: sh_eth: Add support ethtool")
Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 49 ++++++++-------------------
 1 file changed, 15 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index e8aca46bb925..8e429e865552 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -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);
 }
@@ -2049,39 +2059,10 @@ static int sh_eth_get_link_ksettings(struct net_device *ndev,
 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;
+	return phy_ethtool_ksettings_set(ndev->phydev, cmd);
 }
 
 /* If it is ever necessary to increase SH_ETH_REG_DUMP_MAX_REGS, the
-- 
2.17.1

Re: [PATCH v2 02/14] sh_eth: fix invalid context bug while changing link options by ethtool

[Sergei Shtylyov]

On 07/04/2018 11:12 AM, Vladimir Zapolskiy wrote:

> The change fixes sleep in atomic context bug, which is encountered
> every time when link settings are changed by ethtool.
> 
> Since commit 35b5f6b1a82b ("PHYLIB: Locking fixes for PHY I/O
> potentially sleeping") phy_start_aneg() function utilizes a mutex
> to serialize changes to phy state, however that helper function is
> called in atomic context under a grabbed spinlock, because
> phy_start_aneg() is called by phy_ethtool_ksettings_set() and by
> replaced phy_ethtool_sset() helpers from phylib.

   So if all this boils down to phy_start_aneg(), shouldn't this patch
precede patch #1. Or even fixing both call chains with 1 patch? :-)

> Now duplex mode setting is enforced in sh_eth_adjust_link() only,
> also now RX/TX is disabled when link is put down or modifications
> to E-MAC registers ECMR and GECMR are expected for both cases of
> checked and ignored link status pin state from E-MAC interrupt handler.
> 
> For reference the change is a partial rework of commit 1e1b812bbe10
> ("sh_eth: fix handling of no LINK signal").
> 
> Fixes: dc19e4e5e02f ("sh: sh_eth: Add support ethtool")
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

[PATCH v2 03/14] sh_eth: simplify link auto-negotiation by ethtool

[Vladimir Zapolskiy]

There is no need to call a heavyweight phy_start_aneg() for phy
auto-negotiation by ethtool, the phy is already initialized and
link auto-negotiation is started by calling phy_start() from
sh_eth_phy_start() when a network device is opened.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 8e429e865552..1bed2ee4d709 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2249,7 +2249,7 @@ static int sh_eth_nway_reset(struct net_device *ndev)
 	if (!ndev->phydev)
 		return -ENODEV;
 
-	return phy_start_aneg(ndev->phydev);
+	return phy_restart_aneg(ndev->phydev);
 }
 
 static u32 sh_eth_get_msglevel(struct net_device *ndev)
-- 
2.17.1

Re: [PATCH v2 03/14] sh_eth: simplify link auto-negotiation by ethtool

[Sergei Shtylyov]

On 07/04/2018 11:12 AM, Vladimir Zapolskiy wrote:

> There is no need to call a heavyweight phy_start_aneg() for phy
> auto-negotiation by ethtool, the phy is already initialized and
> link auto-negotiation is started by calling phy_start() from
> sh_eth_phy_start() when a network device is opened.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

[PATCH v2 04/14] sh_eth: remove custom .nway_reset from ethtool ops

[Vladimir Zapolskiy]

The generic phy_ethtool_nway_reset() function from phylib can be used
instead of in-house sh_eth_nway_reset().

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 1bed2ee4d709..50ff18870be2 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2244,14 +2244,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)
-{
-	if (!ndev->phydev)
-		return -ENODEV;
-
-	return phy_restart_aneg(ndev->phydev);
-}
-
 static u32 sh_eth_get_msglevel(struct net_device *ndev)
 {
 	struct sh_eth_private *mdp = netdev_priv(ndev);
@@ -2402,7 +2394,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,
-- 
2.17.1

Re: [PATCH v2 04/14] sh_eth: remove custom .nway_reset from ethtool ops

[Sergei Shtylyov]

On 07/04/2018 11:12 AM, Vladimir Zapolskiy wrote:

> The generic phy_ethtool_nway_reset() function from phylib can be used
> instead of in-house sh_eth_nway_reset().
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

[PATCH v2 05/14] sh_eth: remove useless serialization in sh_eth_get_link_ksettings()

[Vladimir Zapolskiy]

phy_ethtool_ksettings_get() call does not modify device state or device
driver state, hence there is no need to utilize a driver specific
spinlock.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 50ff18870be2..152edd1e9a23 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2043,15 +2043,10 @@ static int sh_eth_phy_start(struct net_device *ndev)
 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;
 }
-- 
2.17.1

Re: [PATCH v2 05/14] sh_eth: remove useless serialization in sh_eth_get_link_ksettings()

[Sergei Shtylyov]

On 07/04/2018 11:14 AM, Vladimir Zapolskiy wrote:

> phy_ethtool_ksettings_get() call does not modify device state or device
> driver state, hence there is no need to utilize a driver specific
> spinlock.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

[PATCH v2 06/14] sh_eth: remove custom .get_link_ksettings from ethtool ops

[Vladimir Zapolskiy]

The generic phy_ethtool_get_link_ksettings() function from phylib can be
used instead of in-house sh_eth_get_link_ksettings().

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index 152edd1e9a23..bd4a0b9c3362 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2040,17 +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)
-{
-	if (!ndev->phydev)
-		return -ENODEV;
-
-	phy_ethtool_ksettings_get(ndev->phydev, cmd);
-
-	return 0;
-}
-
 static int sh_eth_set_link_ksettings(struct net_device *ndev,
 				     const struct ethtool_link_ksettings *cmd)
 {
@@ -2398,7 +2387,7 @@ 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,
+	.get_link_ksettings = phy_ethtool_get_link_ksettings,
 	.set_link_ksettings = sh_eth_set_link_ksettings,
 	.get_wol	= sh_eth_get_wol,
 	.set_wol	= sh_eth_set_wol,
-- 
2.17.1

Re: [PATCH v2 06/14] sh_eth: remove custom .get_link_ksettings from ethtool ops

[Sergei Shtylyov]

On 07/04/2018 11:14 AM, Vladimir Zapolskiy wrote:

> The generic phy_ethtool_get_link_ksettings() function from phylib can be
> used instead of in-house sh_eth_get_link_ksettings().
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

[PATCH v2 07/14] sh_eth: remove custom .set_link_ksettings from ethtool ops

[Vladimir Zapolskiy]

The generic phy_ethtool_set_link_ksettings() function from phylib can
be used instead of in-house sh_eth_set_link_ksettings().

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index bd4a0b9c3362..5614fd231bbe 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -2040,15 +2040,6 @@ static int sh_eth_phy_start(struct net_device *ndev)
 	return 0;
 }
 
-static int sh_eth_set_link_ksettings(struct net_device *ndev,
-				     const struct ethtool_link_ksettings *cmd)
-{
-	if (!ndev->phydev)
-		return -ENODEV;
-
-	return phy_ethtool_ksettings_set(ndev->phydev, cmd);
-}
-
 /* 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
@@ -2388,7 +2379,7 @@ static const struct ethtool_ops sh_eth_ethtool_ops = {
 	.get_ringparam	= sh_eth_get_ringparam,
 	.set_ringparam	= sh_eth_set_ringparam,
 	.get_link_ksettings = phy_ethtool_get_link_ksettings,
-	.set_link_ksettings = sh_eth_set_link_ksettings,
+	.set_link_ksettings = phy_ethtool_set_link_ksettings,
 	.get_wol	= sh_eth_get_wol,
 	.set_wol	= sh_eth_set_wol,
 };
-- 
2.17.1

Re: [PATCH v2 07/14] sh_eth: remove custom .set_link_ksettings from ethtool ops

[Sergei Shtylyov]

On 07/04/2018 11:14 AM, Vladimir Zapolskiy wrote:

> The generic phy_ethtool_set_link_ksettings() function from phylib can
> be used instead of in-house sh_eth_set_link_ksettings().
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

[PATCH v2 08/14] ravb: fix invalid context bug while calling auto-negotiation by ethtool

[Vladimir Zapolskiy]

Since commit 35b5f6b1a82b ("PHYLIB: Locking fixes for PHY I/O
potentially sleeping") phy_start_aneg() function utilizes a mutex
to serialize changes to phy state, however the helper function is
called in atomic context.

The bug can be reproduced by running "ethtool -r" command, the bug
is reported if CONFIG_DEBUG_ATOMIC_SLEEP build option is enabled.

Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 68f122140966..e7d6d1b6e7d6 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1152,15 +1152,10 @@ static int ravb_set_link_ksettings(struct net_device *ndev,
 
 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);
+	if (ndev->phydev)
 		error = phy_start_aneg(ndev->phydev);
-		spin_unlock_irqrestore(&priv->lock, flags);
-	}
 
 	return error;
 }
-- 
2.17.1

Re: [PATCH v2 08/14] ravb: fix invalid context bug while calling auto-negotiation by ethtool

[Vladimir Zapolskiy]

On 07/04/2018 11:14 AM, Vladimir Zapolskiy wrote:
> Since commit 35b5f6b1a82b ("PHYLIB: Locking fixes for PHY I/O
> potentially sleeping") phy_start_aneg() function utilizes a mutex
> to serialize changes to phy state, however the helper function is
> called in atomic context.
> 
> The bug can be reproduced by running "ethtool -r" command, the bug
> is reported if CONFIG_DEBUG_ATOMIC_SLEEP build option is enabled.
> 
> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")

Here is an invalid commit specified, the proper tag is

Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")

Re: [PATCH v2 08/14] ravb: fix invalid context bug while calling auto-negotiation by ethtool

[Vladimir Zapolskiy]

On 07/04/2018 11:14 AM, Vladimir Zapolskiy wrote:
> Since commit 35b5f6b1a82b ("PHYLIB: Locking fixes for PHY I/O
> potentially sleeping") phy_start_aneg() function utilizes a mutex
> to serialize changes to phy state, however the helper function is
> called in atomic context.
> 
> The bug can be reproduced by running "ethtool -r" command, the bug
> is reported if CONFIG_DEBUG_ATOMIC_SLEEP build option is enabled.
> 
> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")

Here is an invalid commit specified, the proper tag is

Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")

--
Best wishes,
Vladimir

Re: [PATCH v2 08/14] ravb: fix invalid context bug while calling auto-negotiation by ethtool

[Sergei Shtylyov]

Hello!

On 7/5/2018 9:09 AM, Vladimir Zapolskiy wrote:

>> Since commit 35b5f6b1a82b ("PHYLIB: Locking fixes for PHY I/O
>> potentially sleeping") phy_start_aneg() function utilizes a mutex
>> to serialize changes to phy state, however the helper function is
>> called in atomic context.
>>
>> The bug can be reproduced by running "ethtool -r" command, the bug
>> is reported if CONFIG_DEBUG_ATOMIC_SLEEP build option is enabled.
>>
>> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
> 
> Here is an invalid commit specified, the proper tag is
> 
> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")

    I was just going to tell you that. :-)
> --
> Best wishes,
> Vladimir

MBR, Sergei

Re: [PATCH v2 08/14] ravb: fix invalid context bug while calling auto-negotiation by ethtool

[Sergei Shtylyov]

On 07/04/2018 11:14 AM, Vladimir Zapolskiy wrote:

> Since commit 35b5f6b1a82b ("PHYLIB: Locking fixes for PHY I/O
> potentially sleeping") phy_start_aneg() function utilizes a mutex
> to serialize changes to phy state, however the helper function is
> called in atomic context.
> 
> The bug can be reproduced by running "ethtool -r" command, the bug
> is reported if CONFIG_DEBUG_ATOMIC_SLEEP build option is enabled.
> 
> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

   Again, the patch order question...

MBR, Sergei

[PATCH v2 09/14] ravb: fix invalid context bug while changing link options by ethtool

[Vladimir Zapolskiy]

The change fixes sleep in atomic context bug, which is encountered
every time when link settings are changed by ethtool.

Since commit 35b5f6b1a82b ("PHYLIB: Locking fixes for PHY I/O
potentially sleeping") phy_start_aneg() function utilizes a mutex
to serialize changes to phy state, however that helper function is
called in atomic context under a grabbed spinlock, because
phy_start_aneg() is called by phy_ethtool_ksettings_set() and by
replaced phy_ethtool_sset() helpers from phylib.

Now duplex mode setting is enforced in ravb_adjust_link() only, also
now RX/TX is disabled when link is put down or modifications to E-MAC
registers ECMR and GECMR are expected for both cases of checked and
ignored link status pin state from E-MAC interrupt handler.

Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 49 ++++++++----------------
 1 file changed, 15 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index e7d6d1b6e7d6..40266fe01186 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -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);
 }
@@ -1115,39 +1125,10 @@ static int ravb_get_link_ksettings(struct net_device *ndev,
 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;
+	return phy_ethtool_ksettings_set(ndev->phydev, cmd);
 }
 
 static int ravb_nway_reset(struct net_device *ndev)
-- 
2.17.1

Re: [PATCH v2 09/14] ravb: fix invalid context bug while changing link options by ethtool

[Vladimir Zapolskiy]

On 07/04/2018 11:14 AM, Vladimir Zapolskiy wrote:
> The change fixes sleep in atomic context bug, which is encountered
> every time when link settings are changed by ethtool.
> 
> Since commit 35b5f6b1a82b ("PHYLIB: Locking fixes for PHY I/O
> potentially sleeping") phy_start_aneg() function utilizes a mutex
> to serialize changes to phy state, however that helper function is
> called in atomic context under a grabbed spinlock, because
> phy_start_aneg() is called by phy_ethtool_ksettings_set() and by
> replaced phy_ethtool_sset() helpers from phylib.
> 
> Now duplex mode setting is enforced in ravb_adjust_link() only, also
> now RX/TX is disabled when link is put down or modifications to E-MAC
> registers ECMR and GECMR are expected for both cases of checked and
> ignored link status pin state from E-MAC interrupt handler.
> 
> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")

Here is an invalid commit specified, the proper tag is

Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")

Re: [PATCH v2 09/14] ravb: fix invalid context bug while changing link options by ethtool

[Vladimir Zapolskiy]

On 07/04/2018 11:14 AM, Vladimir Zapolskiy wrote:
> The change fixes sleep in atomic context bug, which is encountered
> every time when link settings are changed by ethtool.
> 
> Since commit 35b5f6b1a82b ("PHYLIB: Locking fixes for PHY I/O
> potentially sleeping") phy_start_aneg() function utilizes a mutex
> to serialize changes to phy state, however that helper function is
> called in atomic context under a grabbed spinlock, because
> phy_start_aneg() is called by phy_ethtool_ksettings_set() and by
> replaced phy_ethtool_sset() helpers from phylib.
> 
> Now duplex mode setting is enforced in ravb_adjust_link() only, also
> now RX/TX is disabled when link is put down or modifications to E-MAC
> registers ECMR and GECMR are expected for both cases of checked and
> ignored link status pin state from E-MAC interrupt handler.
> 
> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")

Here is an invalid commit specified, the proper tag is

Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")

--
Best wishes,
Vladimir

Re: [PATCH v2 09/14] ravb: fix invalid context bug while changing link options by ethtool

[Sergei Shtylyov]

On 07/04/2018 11:14 AM, Vladimir Zapolskiy wrote:

> The change fixes sleep in atomic context bug, which is encountered
> every time when link settings are changed by ethtool.
> 
> Since commit 35b5f6b1a82b ("PHYLIB: Locking fixes for PHY I/O
> potentially sleeping") phy_start_aneg() function utilizes a mutex
> to serialize changes to phy state, however that helper function is
> called in atomic context under a grabbed spinlock, because
> phy_start_aneg() is called by phy_ethtool_ksettings_set() and by
> replaced phy_ethtool_sset() helpers from phylib.
> 
> Now duplex mode setting is enforced in ravb_adjust_link() only, also
> now RX/TX is disabled when link is put down or modifications to E-MAC
> registers ECMR and GECMR are expected for both cases of checked and
> ignored link status pin state from E-MAC interrupt handler.
> 
> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")

   Wrong commit, as already noted...

> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

[PATCH v2 10/14] ravb: simplify link auto-negotiation by ethtool

[Vladimir Zapolskiy]

There is no need to call a heavyweight phy_start_aneg() for phy
auto-negotiation by ethtool, the phy is already initialized and
link auto-negotiation is started by calling phy_start() from
ravb_phy_start() when a network device is opened.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 40266fe01186..31913a469001 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1136,7 +1136,7 @@ static int ravb_nway_reset(struct net_device *ndev)
 	int error = -ENODEV;
 
 	if (ndev->phydev)
-		error = phy_start_aneg(ndev->phydev);
+		error = phy_restart_aneg(ndev->phydev);
 
 	return error;
 }
-- 
2.17.1

Re: [PATCH v2 10/14] ravb: simplify link auto-negotiation by ethtool

[Sergei Shtylyov]

On 07/04/2018 11:16 AM, Vladimir Zapolskiy wrote:

> There is no need to call a heavyweight phy_start_aneg() for phy
> auto-negotiation by ethtool, the phy is already initialized and
> link auto-negotiation is started by calling phy_start() from
> ravb_phy_start() when a network device is opened.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]
Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

[PATCH v2 11/14] ravb: remove custom .nway_reset from ethtool ops

[Vladimir Zapolskiy]

The generic phy_ethtool_nway_reset() function from phylib can be used
instead of in-house ravb_nway_reset().

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 31913a469001..6002132093cd 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1131,16 +1131,6 @@ static int ravb_set_link_ksettings(struct net_device *ndev,
 	return phy_ethtool_ksettings_set(ndev->phydev, cmd);
 }
 
-static int ravb_nway_reset(struct net_device *ndev)
-{
-	int error = -ENODEV;
-
-	if (ndev->phydev)
-		error = phy_restart_aneg(ndev->phydev);
-
-	return error;
-}
-
 static u32 ravb_get_msglevel(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -1353,7 +1343,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,
-- 
2.17.1

Re: [PATCH v2 11/14] ravb: remove custom .nway_reset from ethtool ops

[Sergei Shtylyov]

On 07/04/2018 11:16 AM, Vladimir Zapolskiy wrote:

> The generic phy_ethtool_nway_reset() function from phylib can be used
> instead of in-house ravb_nway_reset().
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

[PATCH v2 12/14] ravb: remove useless serialization in ravb_get_link_ksettings()

[Vladimir Zapolskiy]

phy_ethtool_ksettings_get() call does not modify device state or device
driver state, hence there is no need to utilize a driver specific
spinlock.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 6002132093cd..772687a5faee 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1109,15 +1109,10 @@ static int ravb_phy_start(struct net_device *ndev)
 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;
 }
-- 
2.17.1

Re: [PATCH v2 12/14] ravb: remove useless serialization in ravb_get_link_ksettings()

[Sergei Shtylyov]

On 07/04/2018 11:16 AM, Vladimir Zapolskiy wrote:

> phy_ethtool_ksettings_get() call does not modify device state or device
> driver state, hence there is no need to utilize a driver specific
> spinlock.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

[PATCH v2 13/14] ravb: remove custom .get_link_ksettings from ethtool ops

[Vladimir Zapolskiy]

The generic phy_ethtool_get_link_ksettings() function from phylib can be
used instead of in-house ravb_get_link_ksettings().

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 772687a5faee..9fe01259be6f 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1106,17 +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)
-{
-	if (!ndev->phydev)
-		return -ENODEV;
-
-	phy_ethtool_ksettings_get(ndev->phydev, cmd);
-
-	return 0;
-}
-
 static int ravb_set_link_ksettings(struct net_device *ndev,
 				   const struct ethtool_link_ksettings *cmd)
 {
@@ -1348,7 +1337,7 @@ 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,
+	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
 	.set_link_ksettings	= ravb_set_link_ksettings,
 	.get_wol		= ravb_get_wol,
 	.set_wol		= ravb_set_wol,
-- 
2.17.1

Re: [PATCH v2 13/14] ravb: remove custom .get_link_ksettings from ethtool ops

[Sergei Shtylyov]

On 07/04/2018 11:16 AM, Vladimir Zapolskiy wrote:

> The generic phy_ethtool_get_link_ksettings() function from phylib can be
> used instead of in-house ravb_get_link_ksettings().
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

[PATCH v2 14/14] ravb: remove custom .set_link_ksettings from ethtool ops

[Vladimir Zapolskiy]

The generic phy_ethtool_set_link_ksettings() function from phylib can
be used instead of in-house ravb_set_link_ksettings().

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 9fe01259be6f..0d811c02ff34 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1106,15 +1106,6 @@ static int ravb_phy_start(struct net_device *ndev)
 	return 0;
 }
 
-static int ravb_set_link_ksettings(struct net_device *ndev,
-				   const struct ethtool_link_ksettings *cmd)
-{
-	if (!ndev->phydev)
-		return -ENODEV;
-
-	return phy_ethtool_ksettings_set(ndev->phydev, cmd);
-}
-
 static u32 ravb_get_msglevel(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -1338,7 +1329,7 @@ static const struct ethtool_ops ravb_ethtool_ops = {
 	.set_ringparam		= ravb_set_ringparam,
 	.get_ts_info		= ravb_get_ts_info,
 	.get_link_ksettings	= phy_ethtool_get_link_ksettings,
-	.set_link_ksettings	= ravb_set_link_ksettings,
+	.set_link_ksettings	= phy_ethtool_set_link_ksettings,
 	.get_wol		= ravb_get_wol,
 	.set_wol		= ravb_set_wol,
 };
-- 
2.17.1

Re: [PATCH v2 14/14] ravb: remove custom .set_link_ksettings from ethtool ops

[Sergei Shtylyov]

On 07/04/2018 11:16 AM, Vladimir Zapolskiy wrote:

> The generic phy_ethtool_set_link_ksettings() function from phylib can
> be used instead of in-house ravb_set_link_ksettings().
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
[...]

Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

MBR, Sergei

Re: [PATCH v2 00/14] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[David Miller]

From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Date: Wed, 4 Jul 2018 11:12:38 +0300

> 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:
 ...
> 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.

These changes look fine to me but I want to see some reviews and/or
testing before I apply them.

Thanks.

Re: [PATCH v2 00/14] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[Vladimir Zapolskiy]

Hi David,

On 07/05/2018 03:56 AM, David Miller wrote:
> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> Date: Wed, 4 Jul 2018 11:12:38 +0300
> 
>> 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:
>  ...
>> 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.
> 
> These changes look fine to me but I want to see some reviews and/or
> testing before I apply them.
> 

Thanks to Geert for finding time to test v1 series, the sums of changes
are word for word, https://www.spinics.net/lists/linux-renesas-soc/msg28666.html

Andrew also gave a number of Reviewed-by tags, but they are not directly
applicable to new patches, unfortunately.

In any case let's wait for scrupulous review completed by Sergei, I believe
he'd like to contribute to the review process, and Sergei may highlight more
shortcomings.

Re: [PATCH v2 00/14] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[Vladimir Zapolskiy]

Hi David,

On 07/05/2018 03:56 AM, David Miller wrote:
> From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> Date: Wed, 4 Jul 2018 11:12:38 +0300
> 
>> 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:
>  ...
>> 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.
> 
> These changes look fine to me but I want to see some reviews and/or
> testing before I apply them.
> 

Thanks to Geert for finding time to test v1 series, the sums of changes
are word for word, https://www.spinics.net/lists/linux-renesas-soc/msg28666.html

Andrew also gave a number of Reviewed-by tags, but they are not directly
applicable to new patches, unfortunately.

In any case let's wait for scrupulous review completed by Sergei, I believe
he'd like to contribute to the review process, and Sergei may highlight more
shortcomings.

--
Best wishes,
Vladimir

Re: [PATCH v2 00/14] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[Sergei Shtylyov]

On 07/05/2018 08:59 AM, Vladimir Zapolskiy wrote:

>>> 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:
>>  ...
>>> 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.
>>
>> These changes look fine to me but I want to see some reviews and/or
>> testing before I apply them.
>>
> 
> Thanks to Geert for finding time to test v1 series, the sums of changes
> are word for word, https://www.spinics.net/lists/linux-renesas-soc/msg28666.html

   I might find a time to test the sh_eth patches on the R-Car V3H Starter Kit board;
I'm still on vacations, so don't have access to the R-Car gen2 boards at home...

> Andrew also gave a number of Reviewed-by tags, but they are not directly
> applicable to new patches, unfortunately.
> 
> In any case let's wait for scrupulous review completed by Sergei, I believe
> he'd like to contribute to the review process, and Sergei may highlight more
> shortcomings.

   Heh. I'm surprised DaveM haven't lectured you about not mixing fixes and cleanups
in the single series -- they go to his different 2 trees, net.git and net-next.git...
If you need your cleanups based on your fixes, you have to indicate that in the cleanups
patchset...

> --
> Best wishes,
> Vladimir

MBR, Sergei

Re: [PATCH v2 00/14] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[Geert Uytterhoeven]

Hi Vladimir,

On Wed, Jul 4, 2018 at 10:13 AM Vladimir Zapolskiy
<vladimir_zapolskiy@mentor.com> wrote:
> 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.

Thank for your series!

I've retested this on both R-Car M2-W (sh_eth) and R-Car H3 ES2.0 (ravb),
and the BUG disappeared.

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH v2 00/14] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[David Miller]

From: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Date: Wed, 4 Jul 2018 11:12:38 +0300

> 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:
 ...
> 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.

Series applied with Fixes: tags of patches 8 and 9 fixed up.

Thanks.

Re: [PATCH v2 00/14] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[Sergei Shtylyov]

On 7/7/2018 4:47 AM, David Miller wrote:

>> 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:
>   ...
>> 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.
> 
> Series applied with Fixes: tags of patches 8 and 9 fixed up.

    So you applied the whole series to net.git... that was somewhat 
unexpected, at least by me. Care to share your reasoning?

> Thanks.

MBR, Sergei

Re: [PATCH v2 00/14] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Sat, 7 Jul 2018 18:22:08 +0300

>    So you applied the whole series to net.git... that was somewhat
>    unexpected, at least by me. Care to share your reasoning?

It's fixes a sleep in atomic which is a serious bug.

Re: [PATCH v2 00/14] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[Sergei Shtylyov]

Hello!

On 7/8/2018 2:58 AM, David Miller wrote:

>>     So you applied the whole series to net.git... that was somewhat
>>     unexpected, at least by me. Care to share your reasoning?
> 
> It's fixes a sleep in atomic which is a serious bug.

    Do you realize that only patches 0, 1, 8, and 9 were real fixes, others 
were just follow-up cleanups? (Although the wording of the cover letter could 
have made people think that the whole series is the fixes...)
    OK, I understand what's done is done and it's not me that would have to 
justify the cleanups in a middle of the 4.18-rc's. But still, it was not a 
pleasant surprise (and I told Vladimir he shouldn't mix the real fixes and
cleanups in reply to his cover letter, IIRC).

MBR, Sergei

Re: [PATCH v2 00/14] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[Sergei Shtylyov]

On 7/9/2018 2:24 AM, Sergei Shtylyov wrote:

>>>     So you applied the whole series to net.git... that was somewhat
>>>     unexpected, at least by me. Care to share your reasoning?
>>
>> It's fixes a sleep in atomic which is a serious bug.
> 
>     Do you realize that only patches 0, 1, 8, and 9 were real fixes, others 

    1, 2, 8, and 9, of course. :-)

[...]

MBR, Sergei

Re: [PATCH v2 00/14] ravb/sh_eth: fix sleep in atomic by reusing shared ethtool handlers

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Mon, 9 Jul 2018 02:24:45 +0300

> Hello!
> 
> On 7/8/2018 2:58 AM, David Miller wrote:
> 
>>>     So you applied the whole series to net.git... that was somewhat
>>>     unexpected, at least by me. Care to share your reasoning?
>> It's fixes a sleep in atomic which is a serious bug.
> 
>    Do you realize that only patches 0, 1, 8, and 9 were real fixes,
>    others were just follow-up cleanups? (Although the wording of the
>    cover letter could have made people think that the whole series is the
>    fixes...)
>    OK, I understand what's done is done and it's not me that would have
>    to justify the cleanups in a middle of the 4.18-rc's. But still, it
>    was not a pleasant surprise (and I told Vladimir he shouldn't mix the
>    real fixes and
> cleanups in reply to his cover letter, IIRC).

I was disappointed the series was so large as well.

Let's try to work together to handle this better next time, ok?