| Message ID | 20230331092341.268964-1-vinschen@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: stmmac: allow ethtool action on PCI devices if device is down | expand |
On 31.03.2023 11:23, Corinna Vinschen wrote: > So far stmmac is only able to handle ethtool commands if the device > is UP. However, PCI devices usually just have to be in the active > state for ethtool commands. > What do mean with "active state" here? D0? Or, as you say "connected to PCI" a few lines later, do you refer to hot-plugging? PCI being in D0 often isn't sufficient, especially if interface is down. Then required resources like clocks may be disabled by the driver. A driver may runtime-suspend a device for multiple reasons, e.g. interface down or link down. Then the device may be put to D3hot to save power. If we say that it's worth to wake a device for an ethtool command, then I wonder whether this is something that should be done in net core. E.g. calling pm_runtime_get_sync() in __dev_ethtool, similar to what we do in __dev_open() to deal with run-time suspended and therefore potentially detached devices. > Rename stmmac_check_if_running to stmmac_ethtool_begin and add a > stmmac_ethtool_complete action. Check if the device is connected > to PCI and if so, just make sure the device is active. Reset it > to idle state as complete action. > > Tested on Intel Elkhart Lake system. > > Signed-off-by: Corinna Vinschen <vinschen@redhat.com> > --- > .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 20 +++++++++++++++++-- > 1 file changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > index 35c8dd92d369..5a57970dc06a 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > @@ -14,6 +14,7 @@ > #include <linux/mii.h> > #include <linux/phylink.h> > #include <linux/net_tstamp.h> > +#include <linux/pm_runtime.h> > #include <asm/io.h> > > #include "stmmac.h" > @@ -429,13 +430,27 @@ static void stmmac_ethtool_setmsglevel(struct net_device *dev, u32 level) > > } > > -static int stmmac_check_if_running(struct net_device *dev) > +static int stmmac_ethtool_begin(struct net_device *dev) > { > + struct stmmac_priv *priv = netdev_priv(dev); > + > + if (priv->plat->pdev) { > + pm_runtime_get_sync(&priv->plat->pdev->dev); > + return 0; > + } > if (!netif_running(dev)) > return -EBUSY; > return 0; > } > > +static void stmmac_ethtool_complete(struct net_device *dev) > +{ > + struct stmmac_priv *priv = netdev_priv(dev); > + > + if (priv->plat->pdev) > + pm_runtime_put(&priv->plat->pdev->dev); > +} > + > static int stmmac_ethtool_get_regs_len(struct net_device *dev) > { > struct stmmac_priv *priv = netdev_priv(dev); > @@ -1152,7 +1167,8 @@ static int stmmac_set_tunable(struct net_device *dev, > static const struct ethtool_ops stmmac_ethtool_ops = { > .supported_coalesce_params = ETHTOOL_COALESCE_USECS | > ETHTOOL_COALESCE_MAX_FRAMES, > - .begin = stmmac_check_if_running, > + .begin = stmmac_ethtool_begin, > + .complete = stmmac_ethtool_complete, > .get_drvinfo = stmmac_ethtool_getdrvinfo, > .get_msglevel = stmmac_ethtool_getmsglevel, > .set_msglevel = stmmac_ethtool_setmsglevel,
On Mar 31 12:48, Heiner Kallweit wrote: > On 31.03.2023 11:23, Corinna Vinschen wrote: > > So far stmmac is only able to handle ethtool commands if the device > > is UP. However, PCI devices usually just have to be in the active > > state for ethtool commands. > > > What do mean with "active state" here? D0? Or, as you say "connected > to PCI" a few lines later, do you refer to hot-plugging? > > PCI being in D0 often isn't sufficient, especially if interface is down. > Then required resources like clocks may be disabled by the driver. > > A driver may runtime-suspend a device for multiple reasons, e.g. > interface down or link down. Then the device may be put to D3hot > to save power. > If we say that it's worth to wake a device for an ethtool command, > then I wonder whether this is something that should be done in net > core. E.g. calling pm_runtime_get_sync() in __dev_ethtool, similar to > what we do in __dev_open() to deal with run-time suspended and > therefore potentially detached devices. Actually, I'm not sure how to reply to your question. I replicated the behaviour of the igb and igc drivers, because that looked like the right thing to do for a PCIe device. It seems a bit awkward that the UP/DOWN state allows or denies device settings. Corinna > > > Rename stmmac_check_if_running to stmmac_ethtool_begin and add a > > stmmac_ethtool_complete action. Check if the device is connected > > to PCI and if so, just make sure the device is active. Reset it > > to idle state as complete action. > > > > Tested on Intel Elkhart Lake system. > > > > Signed-off-by: Corinna Vinschen <vinschen@redhat.com> > > --- > > .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 20 +++++++++++++++++-- > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > index 35c8dd92d369..5a57970dc06a 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > @@ -14,6 +14,7 @@ > > #include <linux/mii.h> > > #include <linux/phylink.h> > > #include <linux/net_tstamp.h> > > +#include <linux/pm_runtime.h> > > #include <asm/io.h> > > > > #include "stmmac.h" > > @@ -429,13 +430,27 @@ static void stmmac_ethtool_setmsglevel(struct net_device *dev, u32 level) > > > > } > > > > -static int stmmac_check_if_running(struct net_device *dev) > > +static int stmmac_ethtool_begin(struct net_device *dev) > > { > > + struct stmmac_priv *priv = netdev_priv(dev); > > + > > + if (priv->plat->pdev) { > > + pm_runtime_get_sync(&priv->plat->pdev->dev); > > + return 0; > > + } > > if (!netif_running(dev)) > > return -EBUSY; > > return 0; > > } > > > > +static void stmmac_ethtool_complete(struct net_device *dev) > > +{ > > + struct stmmac_priv *priv = netdev_priv(dev); > > + > > + if (priv->plat->pdev) > > + pm_runtime_put(&priv->plat->pdev->dev); > > +} > > + > > static int stmmac_ethtool_get_regs_len(struct net_device *dev) > > { > > struct stmmac_priv *priv = netdev_priv(dev); > > @@ -1152,7 +1167,8 @@ static int stmmac_set_tunable(struct net_device *dev, > > static const struct ethtool_ops stmmac_ethtool_ops = { > > .supported_coalesce_params = ETHTOOL_COALESCE_USECS | > > ETHTOOL_COALESCE_MAX_FRAMES, > > - .begin = stmmac_check_if_running, > > + .begin = stmmac_ethtool_begin, > > + .complete = stmmac_ethtool_complete, > > .get_drvinfo = stmmac_ethtool_getdrvinfo, > > .get_msglevel = stmmac_ethtool_getmsglevel, > > .set_msglevel = stmmac_ethtool_setmsglevel,
On 03.04.2023 11:43, Corinna Vinschen wrote: > On Mar 31 12:48, Heiner Kallweit wrote: >> On 31.03.2023 11:23, Corinna Vinschen wrote: >>> So far stmmac is only able to handle ethtool commands if the device >>> is UP. However, PCI devices usually just have to be in the active >>> state for ethtool commands. >>> >> What do mean with "active state" here? D0? Or, as you say "connected >> to PCI" a few lines later, do you refer to hot-plugging? >> >> PCI being in D0 often isn't sufficient, especially if interface is down. >> Then required resources like clocks may be disabled by the driver. >> >> A driver may runtime-suspend a device for multiple reasons, e.g. >> interface down or link down. Then the device may be put to D3hot >> to save power. >> If we say that it's worth to wake a device for an ethtool command, >> then I wonder whether this is something that should be done in net >> core. E.g. calling pm_runtime_get_sync() in __dev_ethtool, similar to >> what we do in __dev_open() to deal with run-time suspended and >> therefore potentially detached devices. > > Actually, I'm not sure how to reply to your question. I replicated the > behaviour of the igb and igc drivers, because that looked like the right > thing to do for a PCIe device. It seems a bit awkward that the UP/DOWN > state allows or denies device settings. > My point is, whenever boilerplate code is copied from one driver to another, then this may be something for the core. So, instead of solving an issue for one driver, it might be solved in general. > > Corinna > > > >> >>> Rename stmmac_check_if_running to stmmac_ethtool_begin and add a >>> stmmac_ethtool_complete action. Check if the device is connected >>> to PCI and if so, just make sure the device is active. Reset it >>> to idle state as complete action. >>> >>> Tested on Intel Elkhart Lake system. >>> >>> Signed-off-by: Corinna Vinschen <vinschen@redhat.com> >>> --- >>> .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 20 +++++++++++++++++-- >>> 1 file changed, 18 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c >>> index 35c8dd92d369..5a57970dc06a 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c >>> @@ -14,6 +14,7 @@ >>> #include <linux/mii.h> >>> #include <linux/phylink.h> >>> #include <linux/net_tstamp.h> >>> +#include <linux/pm_runtime.h> >>> #include <asm/io.h> >>> >>> #include "stmmac.h" >>> @@ -429,13 +430,27 @@ static void stmmac_ethtool_setmsglevel(struct net_device *dev, u32 level) >>> >>> } >>> >>> -static int stmmac_check_if_running(struct net_device *dev) >>> +static int stmmac_ethtool_begin(struct net_device *dev) >>> { >>> + struct stmmac_priv *priv = netdev_priv(dev); >>> + >>> + if (priv->plat->pdev) { >>> + pm_runtime_get_sync(&priv->plat->pdev->dev); >>> + return 0; >>> + } >>> if (!netif_running(dev)) >>> return -EBUSY; >>> return 0; >>> } >>> >>> +static void stmmac_ethtool_complete(struct net_device *dev) >>> +{ >>> + struct stmmac_priv *priv = netdev_priv(dev); >>> + >>> + if (priv->plat->pdev) >>> + pm_runtime_put(&priv->plat->pdev->dev); >>> +} >>> + >>> static int stmmac_ethtool_get_regs_len(struct net_device *dev) >>> { >>> struct stmmac_priv *priv = netdev_priv(dev); >>> @@ -1152,7 +1167,8 @@ static int stmmac_set_tunable(struct net_device *dev, >>> static const struct ethtool_ops stmmac_ethtool_ops = { >>> .supported_coalesce_params = ETHTOOL_COALESCE_USECS | >>> ETHTOOL_COALESCE_MAX_FRAMES, >>> - .begin = stmmac_check_if_running, >>> + .begin = stmmac_ethtool_begin, >>> + .complete = stmmac_ethtool_complete, >>> .get_drvinfo = stmmac_ethtool_getdrvinfo, >>> .get_msglevel = stmmac_ethtool_getmsglevel, >>> .set_msglevel = stmmac_ethtool_setmsglevel, >
Hi, is that patch still under review or is it refused, given the current behaviour is not actually incorrect? Thanks, Corinna On Apr 3 11:43, Corinna Vinschen wrote: > On Mar 31 12:48, Heiner Kallweit wrote: > > On 31.03.2023 11:23, Corinna Vinschen wrote: > > > So far stmmac is only able to handle ethtool commands if the device > > > is UP. However, PCI devices usually just have to be in the active > > > state for ethtool commands. > > > > > What do mean with "active state" here? D0? Or, as you say "connected > > to PCI" a few lines later, do you refer to hot-plugging? > > > > PCI being in D0 often isn't sufficient, especially if interface is down. > > Then required resources like clocks may be disabled by the driver. > > > > A driver may runtime-suspend a device for multiple reasons, e.g. > > interface down or link down. Then the device may be put to D3hot > > to save power. > > If we say that it's worth to wake a device for an ethtool command, > > then I wonder whether this is something that should be done in net > > core. E.g. calling pm_runtime_get_sync() in __dev_ethtool, similar to > > what we do in __dev_open() to deal with run-time suspended and > > therefore potentially detached devices. > > Actually, I'm not sure how to reply to your question. I replicated the > behaviour of the igb and igc drivers, because that looked like the right > thing to do for a PCIe device. It seems a bit awkward that the UP/DOWN > state allows or denies device settings. > > > Corinna > > > > > > > > Rename stmmac_check_if_running to stmmac_ethtool_begin and add a > > > stmmac_ethtool_complete action. Check if the device is connected > > > to PCI and if so, just make sure the device is active. Reset it > > > to idle state as complete action. > > > > > > Tested on Intel Elkhart Lake system. > > > > > > Signed-off-by: Corinna Vinschen <vinschen@redhat.com> > > > --- > > > .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 20 +++++++++++++++++-- > > > 1 file changed, 18 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > > index 35c8dd92d369..5a57970dc06a 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c > > > @@ -14,6 +14,7 @@ > > > #include <linux/mii.h> > > > #include <linux/phylink.h> > > > #include <linux/net_tstamp.h> > > > +#include <linux/pm_runtime.h> > > > #include <asm/io.h> > > > > > > #include "stmmac.h" > > > @@ -429,13 +430,27 @@ static void stmmac_ethtool_setmsglevel(struct net_device *dev, u32 level) > > > > > > } > > > > > > -static int stmmac_check_if_running(struct net_device *dev) > > > +static int stmmac_ethtool_begin(struct net_device *dev) > > > { > > > + struct stmmac_priv *priv = netdev_priv(dev); > > > + > > > + if (priv->plat->pdev) { > > > + pm_runtime_get_sync(&priv->plat->pdev->dev); > > > + return 0; > > > + } > > > if (!netif_running(dev)) > > > return -EBUSY; > > > return 0; > > > } > > > > > > +static void stmmac_ethtool_complete(struct net_device *dev) > > > +{ > > > + struct stmmac_priv *priv = netdev_priv(dev); > > > + > > > + if (priv->plat->pdev) > > > + pm_runtime_put(&priv->plat->pdev->dev); > > > +} > > > + > > > static int stmmac_ethtool_get_regs_len(struct net_device *dev) > > > { > > > struct stmmac_priv *priv = netdev_priv(dev); > > > @@ -1152,7 +1167,8 @@ static int stmmac_set_tunable(struct net_device *dev, > > > static const struct ethtool_ops stmmac_ethtool_ops = { > > > .supported_coalesce_params = ETHTOOL_COALESCE_USECS | > > > ETHTOOL_COALESCE_MAX_FRAMES, > > > - .begin = stmmac_check_if_running, > > > + .begin = stmmac_ethtool_begin, > > > + .complete = stmmac_ethtool_complete, > > > .get_drvinfo = stmmac_ethtool_getdrvinfo, > > > .get_msglevel = stmmac_ethtool_getmsglevel, > > > .set_msglevel = stmmac_ethtool_setmsglevel,
On Tue, 25 Apr 2023 10:14:01 +0200 Corinna Vinschen wrote: > is that patch still under review or is it refused, given the > current behaviour is not actually incorrect? That's not what Heiner said. Does the device not link the netdev correctly to the bus device? Core already does pm_get/pm_put around ethtool calls.
On Apr 25 06:54, Jakub Kicinski wrote: > On Tue, 25 Apr 2023 10:14:01 +0200 Corinna Vinschen wrote: > > is that patch still under review or is it refused, given the > > current behaviour is not actually incorrect? > > That's not what Heiner said. Does the device not link the netdev > correctly to the bus device? No, that's not the problem. > Core already does pm_get/pm_put > around ethtool calls. Yeah, right. I made a more thorough test now and it appears that the calls don't even have any effect. For testing I created a driver which simply skips the check for netdev_running() and compared with the driver including my patch. I tested almost all ethtool calls fetching and setting values and there's no difference in behaviour. Worse, while it's possible to change a lot of settings when skipping the netdev_running() check, , some really basic stuff doesn't work as desired. I. e., setting autoneg or a fixed speed while the interface is down is taken without complaints, but it has no effect when the interface goes up again. So we can just scratch my patch. Sorry wasting your time. Corinna
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c index 35c8dd92d369..5a57970dc06a 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c @@ -14,6 +14,7 @@ #include <linux/mii.h> #include <linux/phylink.h> #include <linux/net_tstamp.h> +#include <linux/pm_runtime.h> #include <asm/io.h> #include "stmmac.h" @@ -429,13 +430,27 @@ static void stmmac_ethtool_setmsglevel(struct net_device *dev, u32 level) } -static int stmmac_check_if_running(struct net_device *dev) +static int stmmac_ethtool_begin(struct net_device *dev) { + struct stmmac_priv *priv = netdev_priv(dev); + + if (priv->plat->pdev) { + pm_runtime_get_sync(&priv->plat->pdev->dev); + return 0; + } if (!netif_running(dev)) return -EBUSY; return 0; } +static void stmmac_ethtool_complete(struct net_device *dev) +{ + struct stmmac_priv *priv = netdev_priv(dev); + + if (priv->plat->pdev) + pm_runtime_put(&priv->plat->pdev->dev); +} + static int stmmac_ethtool_get_regs_len(struct net_device *dev) { struct stmmac_priv *priv = netdev_priv(dev); @@ -1152,7 +1167,8 @@ static int stmmac_set_tunable(struct net_device *dev, static const struct ethtool_ops stmmac_ethtool_ops = { .supported_coalesce_params = ETHTOOL_COALESCE_USECS | ETHTOOL_COALESCE_MAX_FRAMES, - .begin = stmmac_check_if_running, + .begin = stmmac_ethtool_begin, + .complete = stmmac_ethtool_complete, .get_drvinfo = stmmac_ethtool_getdrvinfo, .get_msglevel = stmmac_ethtool_getmsglevel, .set_msglevel = stmmac_ethtool_setmsglevel,
So far stmmac is only able to handle ethtool commands if the device is UP. However, PCI devices usually just have to be in the active state for ethtool commands. Rename stmmac_check_if_running to stmmac_ethtool_begin and add a stmmac_ethtool_complete action. Check if the device is connected to PCI and if so, just make sure the device is active. Reset it to idle state as complete action. Tested on Intel Elkhart Lake system. Signed-off-by: Corinna Vinschen <vinschen@redhat.com> --- .../ethernet/stmicro/stmmac/stmmac_ethtool.c | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)