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:     AceLan Kao <acelan.kao@canonical.com>
Cc:     Jakub Kicinski <kuba@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        "David S. Miller" <davem@davemloft.net>,
        Alexei Starovoitov <ast@kernel.org>,
        Andrii Nakryiko <andriin@fb.com>, Wei Wang <weiwan@google.com>,
        Cong Wang <cong.wang@bytedance.com>,
        Taehee Yoo <ap420073@gmail.com>,
        =?UTF-8?B?QmrDtnJuIFTDtnBlbA==?= <bjorn@kernel.org>,
        netdev <netdev@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
References: <20210420075406.64105-1-acelan.kao@canonical.com>
 <CANn89iJLSmtBNoDo8QJ6a0MzsHjdLB0Pf=cs9e4g8Y6-KuFiMQ@mail.gmail.com>
 <20210420122715.2066b537@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
 <CAFv23Q=ywiuZp7Y=bj=SAZmDdAnanAXA954hdO3GpkjmDo=RpQ@mail.gmail.com>
 <c10a6c72-9db7-18c8-6b03-1f8c40b8fd87@gmail.com>
 <CAFv23QkUsTf5M0MoUEFNYeFCtShAn3EmA3u8vXVeZyJa20Bx=g@mail.gmail.com>
From:   Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: [PATCH] net: called rtnl_unlock() before runpm resumes devices
Message-ID: <f06e0e2b-c6bb-ef5a-f629-d1ab82b7aee2@gmail.com>
Date:   Sat, 24 Apr 2021 22:07:50 +0200
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.0
MIME-Version: 1.0
In-Reply-To: <CAFv23QkUsTf5M0MoUEFNYeFCtShAn3EmA3u8vXVeZyJa20Bx=g@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 23.04.2021 05:42, AceLan Kao wrote:
> Heiner Kallweit <hkallweit1@gmail.com> 於 2021年4月22日 週四 下午3:09寫道：
>>
>> On 22.04.2021 08:30, AceLan Kao wrote:
>>> Yes, should add
>>>
>>> Fixes: 9474933caf21 ("igb: close/suspend race in netif_device_detach")
>>> and also
>>> Fixes: 9513d2a5dc7f ("igc: Add legacy power management support")
>>>
>> Please don't top-post. Apart from that:
>> If the issue was introduced with driver changes, then adding a workaround
>> in net core may not be the right approach.
> It's hard to say who introduces this issue, we probably could point
> our finger to below commit
> bd869245a3dc net: core: try to runtime-resume detached device in __dev_open
> 
> This calling path is not usual, in my case, the NIC is not plugged in
> any Ethernet cable,
> and we are doing networking tests on another NIC on the system. So,
> remove the rtnl lock from igb driver will affect other scenarios.
> 
>>
>>> Jakub Kicinski <kuba@kernel.org> 於 2021年4月21日 週三 上午3:27寫道：
>>>>
>>>> On Tue, 20 Apr 2021 10:34:17 +0200 Eric Dumazet wrote:
>>>>> On Tue, Apr 20, 2021 at 9:54 AM AceLan Kao <acelan.kao@canonical.com> wrote:
>>>>>>
>>>>>> From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com>
>>>>>>
>>>>>> The rtnl_lock() has been called in rtnetlink_rcv_msg(), and then in
>>>>>> __dev_open() it calls pm_runtime_resume() to resume devices, and in
>>>>>> some devices' resume function(igb_resum,igc_resume) they calls rtnl_lock()
>>>>>> again. That leads to a recursive lock.
>>>>>>
>>>>>> It should leave the devices' resume function to decide if they need to
>>>>>> call rtnl_lock()/rtnl_unlock(), so call rtnl_unlock() before calling
>>>>>> pm_runtime_resume() and then call rtnl_lock() after it in __dev_open().
>>>>>>
>>>>>>
>>>>>
>>>>> Hi Acelan
>>>>>
>>>>> When was the bugg added ?
>>>>> Please add a Fixes: tag
>>>>
>>>> For immediate cause probably:
>>>>
>>>> Fixes: 9474933caf21 ("igb: close/suspend race in netif_device_detach")
>>>>
>>>>> By doing so, you give more chances for reviewers to understand why the
>>>>> fix is not risky,
>>>>> and help stable teams work.
>>>>
>>>> IMO the driver lacks internal locking. Taking 看rtnl from resume is just
>>>> one example, git history shows many more places that lacked locking and
>>>> got papered over with rtnl here.
>>

You could alternatively try the following. It should avoid the deadlock,
and when runtime-resuming if __IGB_DOWN is set all we do is marking the
net_device as present (because of PCI D3 -> D0 transition).
I do basically the same in r8169 and it works as intended.

Disclaimer: I don't have an igb-driven device and therefore can't test
the proposal.

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 038a9fd1a..21436626a 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -9300,6 +9300,14 @@ static int __maybe_unused igb_runtime_suspend(struct device *dev)
 
 static int __maybe_unused igb_runtime_resume(struct device *dev)
 {
+	struct net_device *netdev = dev_get_drvdata(dev);
+	struct igb_adapter *adapter = netdev_priv(netdev);
+
+	if (test_bit(__IGB_DOWN, &adapter->state)) {
+		netif_device_attach(netdev);
+		return 0;
+	}
+
 	return igb_resume(dev);
 }
 
-- 
2.31.1