[PATCH net-next] net: stmmac: allow ethtool action on PCI devices if device is down

[PATCH net-next] net: stmmac: allow ethtool action on PCI devices if device is down

[Corinna Vinschen]

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(-)

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,
-- 
2.39.2

Re: [PATCH net-next] net: stmmac: allow ethtool action on PCI devices if device is down

[Heiner Kallweit]

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,

Re: [PATCH net-next] net: stmmac: allow ethtool action on PCI devices if device is down

[Corinna Vinschen]

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,

Re: [PATCH net-next] net: stmmac: allow ethtool action on PCI devices if device is down

[Heiner Kallweit]

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,
> 

Re: [PATCH net-next] net: stmmac: allow ethtool action on PCI devices if device is down

[Corinna Vinschen]

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,

Re: [PATCH net-next] net: stmmac: allow ethtool action on PCI devices if device is down

[Jakub Kicinski]

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.

Re: [PATCH net-next] net: stmmac: allow ethtool action on PCI devices if device is down

[Corinna Vinschen]

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