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.
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]>
---
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..b9915645412c 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
Unlike most networking drivers using begin() and complete() ethtool_ops
callbacks to resume a device which is down and suspend it again when done,
via-velocity 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]>
---
drivers/net/ethernet/via/via-velocity.c | 14 ++++++++++----
drivers/net/ethernet/via/via-velocity.h | 1 +
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/via/via-velocity.c b/drivers/net/ethernet/via/via-velocity.c
index 346e44115c4e..4b556b74541a 100644
--- a/drivers/net/ethernet/via/via-velocity.c
+++ b/drivers/net/ethernet/via/via-velocity.c
@@ -3257,12 +3257,16 @@ static struct platform_driver velocity_platform_driver = {
* @dev: network device
*
* Called before an ethtool operation. We need to make sure the
- * chip is out of D3 state before we poke at it.
+ * chip is out of D3 state before we poke at it. In case of ethtool
+ * ops nesting, only wake the device up in the outermost block.
*/
static int velocity_ethtool_up(struct net_device *dev)
{
struct velocity_info *vptr = netdev_priv(dev);
- if (!netif_running(dev))
+
+ if (vptr->ethtool_ops_nesting == U32_MAX)
+ return -EBUSY;
+ if (!vptr->ethtool_ops_nesting++ && !netif_running(dev))
velocity_set_power_state(vptr, PCI_D0);
return 0;
}
@@ -3272,12 +3276,14 @@ static int velocity_ethtool_up(struct net_device *dev)
* @dev: network device
*
* Called after an ethtool operation. Restore the chip back to D3
- * state if it isn't running.
+ * state if it isn't running. In case of ethtool ops nesting, only
+ * put the device to sleep in the outermost block.
*/
static void velocity_ethtool_down(struct net_device *dev)
{
struct velocity_info *vptr = netdev_priv(dev);
- if (!netif_running(dev))
+
+ if (!--vptr->ethtool_ops_nesting && !netif_running(dev))
velocity_set_power_state(vptr, PCI_D3hot);
}
diff --git a/drivers/net/ethernet/via/via-velocity.h b/drivers/net/ethernet/via/via-velocity.h
index cdfe7809e3c1..f196e71d2c04 100644
--- a/drivers/net/ethernet/via/via-velocity.h
+++ b/drivers/net/ethernet/via/via-velocity.h
@@ -1483,6 +1483,7 @@ struct velocity_info {
struct velocity_context context;
u32 ticks;
+ u32 ethtool_ops_nesting;
u8 rev_id;
--
2.24.1
> @@ -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);
> }
Hi Michal
In the via-velocity you added:
+ if (vptr->ethtool_ops_nesting == U32_MAX)
+ return -EBUSY;
+ if (!vptr->ethtool_ops_nesting++ && !netif_running(dev))
velocity_set_power_state(vptr, PCI_D0);
return 0;
These two fragments differ by a ! . Is that correct?
Andrew
On Sun, Jan 05, 2020 at 11:08:32PM +0100, Andrew Lunn wrote:
> > @@ -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);
> > }
>
> Hi Michal
>
> In the via-velocity you added:
>
> + if (vptr->ethtool_ops_nesting == U32_MAX)
> + return -EBUSY;
> + if (!vptr->ethtool_ops_nesting++ && !netif_running(dev))
> velocity_set_power_state(vptr, PCI_D0);
> return 0;
>
> These two fragments differ by a ! . Is that correct?
You are right, thank you for catching it. This should be
if (!ep->ethtool_ops_nesting++ && !netif_running(dev)) {
as well, we only want to wake the device up in the first (outermost)
->begin(). (It would probably do no harm to do it each time but not
doing it in the first would be wrong.)
I'll send v2 in a moment.
Michal