* [PATCH net-next 1/3] wil6210: get rid of begin() and complete() ethtool_ops
2020-01-05 21:16 [PATCH net-next 0/3] ethtool: allow nesting of begin() and complete() callbacks Michal Kubecek
@ 2020-01-05 21:17 ` Michal Kubecek
2020-01-05 21:17 ` [PATCH net-next 2/3] via-velocity: allow nesting of ethtool_ops begin() and complete() Michal Kubecek
2020-01-05 21:17 ` [PATCH net-next 3/3] epic100: " Michal Kubecek
2 siblings, 0 replies; 6+ messages in thread
From: Michal Kubecek @ 2020-01-05 21:17 UTC (permalink / raw)
To: David S. Miller, netdev
Cc: Maya Erez, Kalle Valo, linux-wireless, wil6210, Francois Romieu,
linux-kernel, Andrew Lunn, Florian Fainelli
The wil6210 driver locks a mutex in begin() ethtool_ops callback and
unlocks it in complete() so that all ethtool requests are serialized. This
is not going to work correctly with netlink interface; e.g. when ioctl
triggers a netlink notification, netlink code would call begin() again
while the mutex taken by ioctl code is still held by the same task.
Let's get rid of the begin() and complete() callbacks and move the mutex
locking into the remaining ethtool_ops handlers except get_drvinfo which
only copies strings that are not changing so that there is no need for
serialization.
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
drivers/net/wireless/ath/wil6210/ethtool.c | 43 ++++++++--------------
1 file changed, 16 insertions(+), 27 deletions(-)
diff --git a/drivers/net/wireless/ath/wil6210/ethtool.c b/drivers/net/wireless/ath/wil6210/ethtool.c
index 912c4eaf017b..fef10886ca4a 100644
--- a/drivers/net/wireless/ath/wil6210/ethtool.c
+++ b/drivers/net/wireless/ath/wil6210/ethtool.c
@@ -11,26 +11,6 @@
#include "wil6210.h"
-static int wil_ethtoolops_begin(struct net_device *ndev)
-{
- struct wil6210_priv *wil = ndev_to_wil(ndev);
-
- mutex_lock(&wil->mutex);
-
- wil_dbg_misc(wil, "ethtoolops_begin\n");
-
- return 0;
-}
-
-static void wil_ethtoolops_complete(struct net_device *ndev)
-{
- struct wil6210_priv *wil = ndev_to_wil(ndev);
-
- wil_dbg_misc(wil, "ethtoolops_complete\n");
-
- mutex_unlock(&wil->mutex);
-}
-
static int wil_ethtoolops_get_coalesce(struct net_device *ndev,
struct ethtool_coalesce *cp)
{
@@ -39,11 +19,12 @@ static int wil_ethtoolops_get_coalesce(struct net_device *ndev,
u32 rx_itr_en, rx_itr_val = 0;
int ret;
+ mutex_lock(&wil->mutex);
wil_dbg_misc(wil, "ethtoolops_get_coalesce\n");
ret = wil_pm_runtime_get(wil);
if (ret < 0)
- return ret;
+ goto out;
tx_itr_en = wil_r(wil, RGF_DMA_ITR_TX_CNT_CTL);
if (tx_itr_en & BIT_DMA_ITR_TX_CNT_CTL_EN)
@@ -57,7 +38,11 @@ static int wil_ethtoolops_get_coalesce(struct net_device *ndev,
cp->tx_coalesce_usecs = tx_itr_val;
cp->rx_coalesce_usecs = rx_itr_val;
- return 0;
+ ret = 0;
+
+out:
+ mutex_unlock(&wil->mutex);
+ return ret;
}
static int wil_ethtoolops_set_coalesce(struct net_device *ndev,
@@ -67,12 +52,14 @@ static int wil_ethtoolops_set_coalesce(struct net_device *ndev,
struct wireless_dev *wdev = ndev->ieee80211_ptr;
int ret;
+ mutex_lock(&wil->mutex);
wil_dbg_misc(wil, "ethtoolops_set_coalesce: rx %d usec, tx %d usec\n",
cp->rx_coalesce_usecs, cp->tx_coalesce_usecs);
if (wdev->iftype == NL80211_IFTYPE_MONITOR) {
wil_dbg_misc(wil, "No IRQ coalescing in monitor mode\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
/* only @rx_coalesce_usecs and @tx_coalesce_usecs supported,
@@ -88,24 +75,26 @@ static int wil_ethtoolops_set_coalesce(struct net_device *ndev,
ret = wil_pm_runtime_get(wil);
if (ret < 0)
- return ret;
+ goto out;
wil->txrx_ops.configure_interrupt_moderation(wil);
wil_pm_runtime_put(wil);
+ ret = 0;
- return 0;
+out:
+ mutex_unlock(&wil->mutex);
+ return ret;
out_bad:
wil_dbg_misc(wil, "Unsupported coalescing params. Raw command:\n");
print_hex_dump_debug("DBG[MISC] coal ", DUMP_PREFIX_OFFSET, 16, 4,
cp, sizeof(*cp), false);
+ mutex_unlock(&wil->mutex);
return -EINVAL;
}
static const struct ethtool_ops wil_ethtool_ops = {
- .begin = wil_ethtoolops_begin,
- .complete = wil_ethtoolops_complete,
.get_drvinfo = cfg80211_get_drvinfo,
.get_coalesce = wil_ethtoolops_get_coalesce,
.set_coalesce = wil_ethtoolops_set_coalesce,
--
2.24.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net-next 2/3] via-velocity: allow nesting of ethtool_ops begin() and complete()
2020-01-05 21:16 [PATCH net-next 0/3] ethtool: allow nesting of begin() and complete() callbacks Michal Kubecek
2020-01-05 21:17 ` [PATCH net-next 1/3] wil6210: get rid of begin() and complete() ethtool_ops Michal Kubecek
@ 2020-01-05 21:17 ` Michal Kubecek
2020-01-05 21:17 ` [PATCH net-next 3/3] epic100: " Michal Kubecek
2 siblings, 0 replies; 6+ messages in thread
From: Michal Kubecek @ 2020-01-05 21:17 UTC (permalink / raw)
To: David S. Miller, netdev
Cc: Maya Erez, Kalle Valo, linux-wireless, wil6210, Francois Romieu,
linux-kernel, Andrew Lunn, Florian Fainelli
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 <mkubecek@suse.cz>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH net-next 3/3] epic100: allow nesting of ethtool_ops begin() and complete()
2020-01-05 21:16 [PATCH net-next 0/3] ethtool: allow nesting of begin() and complete() callbacks Michal Kubecek
2020-01-05 21:17 ` [PATCH net-next 1/3] wil6210: get rid of begin() and complete() ethtool_ops Michal Kubecek
2020-01-05 21:17 ` [PATCH net-next 2/3] via-velocity: allow nesting of ethtool_ops begin() and complete() Michal Kubecek
@ 2020-01-05 21:17 ` Michal Kubecek
2020-01-05 22:08 ` Andrew Lunn
2 siblings, 1 reply; 6+ messages in thread
From: Michal Kubecek @ 2020-01-05 21:17 UTC (permalink / raw)
To: David S. Miller, netdev
Cc: Maya Erez, Kalle Valo, linux-wireless, wil6210, Francois Romieu,
linux-kernel, Andrew Lunn, Florian Fainelli
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 <mkubecek@suse.cz>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH net-next 3/3] epic100: allow nesting of ethtool_ops begin() and complete()
2020-01-05 21:17 ` [PATCH net-next 3/3] epic100: " Michal Kubecek
@ 2020-01-05 22:08 ` Andrew Lunn
2020-01-06 6:08 ` Michal Kubecek
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2020-01-05 22:08 UTC (permalink / raw)
To: Michal Kubecek
Cc: David S. Miller, netdev, Maya Erez, Kalle Valo, linux-wireless,
wil6210, Francois Romieu, linux-kernel, Florian Fainelli
> @@ -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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net-next 3/3] epic100: allow nesting of ethtool_ops begin() and complete()
2020-01-05 22:08 ` Andrew Lunn
@ 2020-01-06 6:08 ` Michal Kubecek
0 siblings, 0 replies; 6+ messages in thread
From: Michal Kubecek @ 2020-01-06 6:08 UTC (permalink / raw)
To: netdev
Cc: Andrew Lunn, David S. Miller, Maya Erez, Kalle Valo,
linux-wireless, wil6210, Francois Romieu, linux-kernel,
Florian Fainelli
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
^ permalink raw reply [flat|nested] 6+ messages in thread