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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 3 Apr 2023 15:13:50 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Giuseppe Cavallaro <peppe.cavallaro@...com>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu <joabreu@...opsys.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: stmmac: allow ethtool action on PCI devices
 if device is down

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@...hat.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,
>