Re: [PATCH 2/2] ethtool: Call begin() and complete() in __ethtool_get_link_ksettings()

From: Kai-Heng Feng
Date: Tue Jan 14 2020 - 05:06:33 EST


Hi,

> On Jan 13, 2020, at 20:51, Michal Kubecek <mkubecek@xxxxxxx> 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@xxxxxxxxxxxxx>
>> ---
>> 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
>>