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

[Corinna Vinschen <vinschen@redhat.com>]

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,