linux-kernel - Re: [PATCH 2/2] ethtool: Call begin() and complete() in __ethtool_get_link

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: <4AC365A3-3091-46A7-B938-F63A11115464@canonical.com>
Date:   Tue, 14 Jan 2020 18:06:21 +0800
From:   Kai-Heng Feng <kai.heng.feng@...onical.com>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jiri Pirko <jiri@...lanox.com>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Maxime Chevallier <maxime.chevallier@...tlin.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Li RongQing <lirongqing@...du.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] ethtool: Call begin() and complete() in
 __ethtool_get_link_ksettings()

Hi,

> On Jan 13, 2020, at 20:51, Michal Kubecek <mkubecek@...e.cz> wrote:
> 
> On Fri, Jan 10, 2020 at 03:41:59PM +0800, Kai-Heng Feng wrote:
>> Device like igb gets runtime suspended when there's no link partner. We
>> can't get correct speed under that state:
>> $ cat /sys/class/net/enp3s0/speed
>> 1000
>> 
>> In addition to that, an error can also be spotted in dmesg:
>> [  385.991957] igb 0000:03:00.0 enp3s0: PCIe link lost
>> 
>> It's because the igb device doesn't get runtime resumed before calling
>> get_link_ksettings().
>> 
>> So let's call begin() and complete() like what dev_ethtool() does, to
>> runtime resume/suspend or power up/down the device properly.
>> 
>> Once this fix is in place, igb can show the speed correctly without link
>> partner:
>> $ cat /sys/class/net/enp3s0/speed
>> -1
> 
> I agree that we definitely should make sure ->begin() and ->complete()
> are always called around ethtool_ops calls. But even if nesting should
> be safe now (for in-tree drivers, that is), I'm not really happy about
> calling them even in places where we positively know we are always
> inside a begin / complete block as e.g. vxlan or net_failover do. (And
> also linkinfo.c and linkmodes.c but it may be easier to call
> ->get_link_ksettings() directly there.)

Ok. Maybe use set_bit() to know it's inside of begin() and complete()?

> 
> How about having two helpers: one simple for "ethtool context" where we
> know we already are between ->begin() and ->complete() and one with the
> begin/complete calls for the rest?

Or I can rebase this patch on top of the refactoring work.

Kai-Heng

> Another interesting question is if
> any of the callers which do not have their own begin()/complete() wrap
> does actually need anything more than speed and duplex (I didn't do
> a full check).

For sysfs yes, I am not sure about other cases though.

Kai-Heng

> 
> Michal
> 
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>
>> ---
>> net/ethtool/ioctl.c | 15 ++++++++++++++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>> 
>> diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
>> index 182bffbffa78..c768dbf45fc4 100644
>> --- a/net/ethtool/ioctl.c
>> +++ b/net/ethtool/ioctl.c
>> @@ -423,13 +423,26 @@ struct ethtool_link_usettings {
>> int __ethtool_get_link_ksettings(struct net_device *dev,
>> 				 struct ethtool_link_ksettings *link_ksettings)
>> {
>> +	int rc;
>> +
>> 	ASSERT_RTNL();
>> 
>> 	if (!dev->ethtool_ops->get_link_ksettings)
>> 		return -EOPNOTSUPP;
>> 
>> +	if (dev->ethtool_ops->begin) {
>> +		rc = dev->ethtool_ops->begin(dev);
>> +		if (rc  < 0)
>> +			return rc;
>> +	}
>> +
>> 	memset(link_ksettings, 0, sizeof(*link_ksettings));
>> -	return dev->ethtool_ops->get_link_ksettings(dev, link_ksettings);
>> +	rc = dev->ethtool_ops->get_link_ksettings(dev, link_ksettings);
>> +
>> +	if (dev->ethtool_ops->complete)
>> +		dev->ethtool_ops->complete(dev);
>> +
>> +	return rc;
>> }
>> EXPORT_SYMBOL(__ethtool_get_link_ksettings);
>> 
>> -- 
>> 2.17.1
>>