linux-kernel - Re: [PATCH v5] net: stmmac: fix 'ethtool -P' return -EBUSY

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <b0c93757-dd32-9d85-6f9e-12956d766661@gmail.com>
Date:   Mon, 19 Jul 2021 15:57:40 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Hao Chen <chenhaoa@...ontech.com>
Cc:     alexandre.torgue@...s.st.com, joabreu@...opsys.com,
        davem@...emloft.net, kuba@...nel.org, mcoquelin.stm32@...il.com,
        linux@...linux.org.uk, netdev@...r.kernel.org,
        linux-stm32@...md-mailman.stormreply.com,
        linux-kernel@...r.kernel.org, peppe.cavallaro@...com
Subject: Re: [PATCH v5] net: stmmac: fix 'ethtool -P' return -EBUSY

On 19.07.2021 15:08, Hao Chen wrote:
> The permanent mac address should be available for query when the device
> is not up.
> NetworkManager, the system network daemon, uses 'ethtool -P' to obtain
> the permanent address after the kernel start. When the network device
> is not up, it will return the device busy error with 'ethtool -P'. At
> that time, it is unable to access the Internet through the permanent
> address by NetworkManager.

At first a few formal aspects:
- You don't get a lot of new friends when posting a new version every
  few hours. Consolidate feedback and then post a new version,
  typically not more than one version per day. Mileage of maintainers
  may vary here.
- When posting a new version include a change log.
- Properly annotate the patch as net vs. net-next.
- If you declare something to be a fix, add a Fixes tag.

> I think that the '.begin' is not used to check if the device is up, it's
> just a pre hook for ethtool. We shouldn't check if the device is up
> there.
> 

Supposedly the check is there for a reason. Your statement leaves the
impression that you're not aware of why the check exists.
Therefore: First understand this, and then explain why it's safe to
remove the check. This drivers uses runtime pm, so device
might be in PCI D3 if interface is down (don't know this driver in
detail). Therefore registers may not be accessible what may impact
certain ethtool ops.

> Signed-off-by: Hao Chen <chenhaoa@...ontech.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index d0ce608b81c3..8901dc9f758e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -410,13 +410,6 @@ static void stmmac_ethtool_setmsglevel(struct net_device *dev, u32 level)
>  
>  }
>  
> -static int stmmac_check_if_running(struct net_device *dev)
> -{
> -	if (!netif_running(dev))
> -		return -EBUSY;
> -	return 0;
> -}
> -
>  static int stmmac_ethtool_get_regs_len(struct net_device *dev)
>  {
>  	struct stmmac_priv *priv = netdev_priv(dev);
> @@ -1073,7 +1066,6 @@ 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,
>  	.get_drvinfo = stmmac_ethtool_getdrvinfo,
>  	.get_msglevel = stmmac_ethtool_getmsglevel,
>  	.set_msglevel = stmmac_ethtool_setmsglevel,
>