Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Hao Chen <chenhaoa@uniontech.com>
Cc:     alexandre.torgue@foss.st.com, joabreu@synopsys.com,
        davem@davemloft.net, kuba@kernel.org, mcoquelin.stm32@gmail.com,
        linux@armlinux.org.uk, netdev@vger.kernel.org,
        linux-stm32@st-md-mailman.stormreply.com,
        linux-kernel@vger.kernel.org, peppe.cavallaro@st.com
References: <20210719130845.2102-1-chenhaoa@uniontech.com>
From:   Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: [PATCH v5] net: stmmac: fix 'ethtool -P' return -EBUSY
Message-ID: <b0c93757-dd32-9d85-6f9e-12956d766661@gmail.com>
Date:   Mon, 19 Jul 2021 15:57:40 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
MIME-Version: 1.0
In-Reply-To: <20210719130845.2102-1-chenhaoa@uniontech.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

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