The ethtool ioctl interface used to guarantee that ethtool_ops callbacks
were always called in a block between calls to ->begin() and ->complete()
(if these are defined) and that this whole block was executed with RTNL
lock held:
rtnl_lock();
ops->begin();
/* other ethtool_ops calls */
ops->complete();
rtnl_unlock();
This prevented any nesting or crossing of the begin-complete blocks.
However, this is no longer guaranteed even for ioctl interface as at least
ethtool_phys_id() releases RTNL lock while waiting for a timer. With the
introduction of netlink ethtool interface, the begin-complete pairs are
naturally nested e.g. when a request triggers a netlink notification.
Fortunately, only minority of networking drivers implements begin() and
complete() callbacks and most of those that do, fall into three groups:
- wrappers for pm_runtime_get_sync() and pm_runtime_put()
- wrappers for clk_prepare_enable() and clk_disable_unprepare()
- begin() checks netif_running() (fails if false), no complete()
First two have their own refcounting, third is safe w.r.t. nesting of the
blocks.
Only three in-tree networking drivers need an update to deal with nesting
of begin() and complete() calls: via-velocity and epic100 perform resume
and suspend on their own and wil6210 completely serializes the calls using
its own mutex (which would lead to a deadlock if a request request
triggered a netlink notification). The series addresses these problems.
changes between v1 and v2:
- fix inverted condition in epic100 ethtool_begin() (thanks to Andrew
Lunn)
Michal Kubecek (3):
wil6210: get rid of begin() and complete() ethtool_ops
via-velocity: allow nesting of ethtool_ops begin() and complete()
epic100: allow nesting of ethtool_ops begin() and complete()
drivers/net/ethernet/smsc/epic100.c | 7 +++-
drivers/net/ethernet/via/via-velocity.c | 14 +++++--
drivers/net/ethernet/via/via-velocity.h | 1 +
drivers/net/wireless/ath/wil6210/ethtool.c | 43 ++++++++--------------
4 files changed, 32 insertions(+), 33 deletions(-)
--
2.24.1
Unlike most networking drivers using begin() and complete() ethtool_ops
callbacks to resume a device which is down and suspend it again when done,
epic100 does not use standard refcounted infrastructure but sets device
sleep state directly.
With the introduction of netlink ethtool interface, we may have nested
begin-complete blocks so that inner complete() would put the device back to
sleep for the rest of the outer block.
To avoid rewriting an old and not very actively developed driver, just add
a nesting counter and only perform resume and suspend on the outermost
level.
Signed-off-by: Michal Kubecek <[email protected]>
---
v2: fix inverted condition in ethtool_begin() (thanks to Andrew Lunn)
---
drivers/net/ethernet/smsc/epic100.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/smsc/epic100.c b/drivers/net/ethernet/smsc/epic100.c
index 912760e8514c..61ddee0c2a2e 100644
--- a/drivers/net/ethernet/smsc/epic100.c
+++ b/drivers/net/ethernet/smsc/epic100.c
@@ -280,6 +280,7 @@ struct epic_private {
signed char phys[4]; /* MII device addresses. */
u16 advertising; /* NWay media advertisement */
int mii_phy_cnt;
+ u32 ethtool_ops_nesting;
struct mii_if_info mii;
unsigned int tx_full:1; /* The Tx queue is full. */
unsigned int default_port:4; /* Last dev->if_port value. */
@@ -1435,8 +1436,10 @@ static int ethtool_begin(struct net_device *dev)
struct epic_private *ep = netdev_priv(dev);
void __iomem *ioaddr = ep->ioaddr;
+ if (ep->ethtool_ops_nesting == U32_MAX)
+ return -EBUSY;
/* power-up, if interface is down */
- if (!netif_running(dev)) {
+ if (!ep->ethtool_ops_nesting++ && !netif_running(dev)) {
ew32(GENCTL, 0x0200);
ew32(NVCTL, (er32(NVCTL) & ~0x003c) | 0x4800);
}
@@ -1449,7 +1452,7 @@ static void ethtool_complete(struct net_device *dev)
void __iomem *ioaddr = ep->ioaddr;
/* power-down, if interface is down */
- if (!netif_running(dev)) {
+ if (!--ep->ethtool_ops_nesting && !netif_running(dev)) {
ew32(GENCTL, 0x0008);
ew32(NVCTL, (er32(NVCTL) & ~0x483c) | 0x0000);
}
--
2.24.1
On Mon, Jan 06, 2020 at 07:39:26AM +0100, Michal Kubecek wrote:
> The ethtool ioctl interface used to guarantee that ethtool_ops callbacks
> were always called in a block between calls to ->begin() and ->complete()
> (if these are defined) and that this whole block was executed with RTNL
> lock held:
>
> rtnl_lock();
> ops->begin();
> /* other ethtool_ops calls */
> ops->complete();
> rtnl_unlock();
>
> This prevented any nesting or crossing of the begin-complete blocks.
> However, this is no longer guaranteed even for ioctl interface as at least
> ethtool_phys_id() releases RTNL lock while waiting for a timer. With the
> introduction of netlink ethtool interface, the begin-complete pairs are
> naturally nested e.g. when a request triggers a netlink notification.
>
> Fortunately, only minority of networking drivers implements begin() and
> complete() callbacks and most of those that do, fall into three groups:
>
> - wrappers for pm_runtime_get_sync() and pm_runtime_put()
> - wrappers for clk_prepare_enable() and clk_disable_unprepare()
> - begin() checks netif_running() (fails if false), no complete()
>
> First two have their own refcounting, third is safe w.r.t. nesting of the
> blocks.
>
> Only three in-tree networking drivers need an update to deal with nesting
> of begin() and complete() calls: via-velocity and epic100 perform resume
> and suspend on their own and wil6210 completely serializes the calls using
> its own mutex (which would lead to a deadlock if a request request
> triggered a netlink notification). The series addresses these problems.
>
> changes between v1 and v2:
> - fix inverted condition in epic100 ethtool_begin() (thanks to Andrew
> Lunn)
Reviewed-by: Simon Horman <[email protected]>
From: Michal Kubecek <[email protected]>
Date: Mon, 6 Jan 2020 07:39:26 +0100 (CET)
> The ethtool ioctl interface used to guarantee that ethtool_ops callbacks
> were always called in a block between calls to ->begin() and ->complete()
> (if these are defined) and that this whole block was executed with RTNL
> lock held:
>
> rtnl_lock();
> ops->begin();
> /* other ethtool_ops calls */
> ops->complete();
> rtnl_unlock();
>
> This prevented any nesting or crossing of the begin-complete blocks.
> However, this is no longer guaranteed even for ioctl interface as at least
> ethtool_phys_id() releases RTNL lock while waiting for a timer. With the
> introduction of netlink ethtool interface, the begin-complete pairs are
> naturally nested e.g. when a request triggers a netlink notification.
>
> Fortunately, only minority of networking drivers implements begin() and
> complete() callbacks and most of those that do, fall into three groups:
>
> - wrappers for pm_runtime_get_sync() and pm_runtime_put()
> - wrappers for clk_prepare_enable() and clk_disable_unprepare()
> - begin() checks netif_running() (fails if false), no complete()
>
> First two have their own refcounting, third is safe w.r.t. nesting of the
> blocks.
>
> Only three in-tree networking drivers need an update to deal with nesting
> of begin() and complete() calls: via-velocity and epic100 perform resume
> and suspend on their own and wil6210 completely serializes the calls using
> its own mutex (which would lead to a deadlock if a request request
> triggered a netlink notification). The series addresses these problems.
>
> changes between v1 and v2:
> - fix inverted condition in epic100 ethtool_begin() (thanks to Andrew
> Lunn)
Series applied, thanks.