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

[Kai-Heng Feng]

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
>>