Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3608.60.0.2.5\))
Subject: Re: [PATCH] ethtool: Report speed and duplex as unknown when device
 is runtime suspended
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
In-Reply-To: <d6c5a1c4-e41f-d239-e0b0-15eba3e78274@gmail.com>
Date:   Fri, 27 Mar 2020 11:11:16 +0800
Cc:     "David S. Miller" <davem@davemloft.net>, kuba@kernel.org,
        alexander.duyck@gmail.com,
        Jeff Kirsher <jeffrey.t.kirsher@intel.com>,
        Aaron Brown <aaron.f.brown@intel.com>,
        Michal Kubecek <mkubecek@suse.cz>,
        Andrew Lunn <andrew@lunn.ch>,
        Maxime Chevallier <maxime.chevallier@bootlin.com>,
        "open list:NETWORKING [GENERAL]" <netdev@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: 8BIT
Message-Id: <84A954EF-4DF6-4F1C-88CE-9B4C0D1ACE5B@canonical.com>
References: <20200327024552.22170-1-kai.heng.feng@canonical.com>
 <d6c5a1c4-e41f-d239-e0b0-15eba3e78274@gmail.com>
To:     Florian Fainelli <f.fainelli@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Mar 27, 2020, at 10:56, Florian Fainelli <f.fainelli@gmail.com> wrote:
> 
> 
> 
> On 3/26/2020 7:45 PM, 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
>> 
>> Since device can only be runtime suspended when there's no link partner,
>> we can directly report the speed and duplex as unknown.
>> 
>> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> Cc: Aaron Brown <aaron.f.brown@intel.com>
>> Suggested-by: Alexander Duyck <alexander.duyck@gmail.com>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> I would push this to the responsibility of the various drivers instead
> of making this part of the standard ethtool implementation.

My original approach [1] is to ask device to runtime resume before calling __ethtool_get_link_ksettings().
Unfortunately it will cause a deadlock if the runtime resume routine wants to hold rtnl_lock.

However, it should be totally fine (and sometimes necessary) to be able to hold rtnl_lock in runtime resume routine as Alexander explained [2].
As suggested, this patch handles the situation directly in __ethtool_get_link_ksettings().

[1] https://lore.kernel.org/lkml/20200207101005.4454-2-kai.heng.feng@canonical.com/
[2] https://lkml.org/lkml/2020/3/26/989

Kai-Heng

> -- 
> Florian