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;
IronPort-PHdr: =?us-ascii?q?9a23=3ApNe+BhbJYDDg5HaBXBQqjAn/LSx94ef9IxIV55?=
 =?us-ascii?q?w7irlHbqWk+dH4MVfC4el21QaZD4Hc7PRLkO3Q9avmCiQM4peE5XYFdpEEFx?=
 =?us-ascii?q?oIkt4fkAFoBsmZQVb6I/jnY21ffoxCWVZp8mv9PR1TH8DzNFnVpXu99jkUXB?=
 =?us-ascii?q?75ZkJ5I+3vEdvUiMK6n+m555zUZVBOgzywKbN/JRm7t0PfrM4T1IBjMa02jB?=
 =?us-ascii?q?DOpyhF?=
From:   "Christian Benvenuti (benve)" <benve@cisco.com>
To:     Kaige Li <likaige@loongson.cn>, David Miller <davem@davemloft.net>
CC:     "_govind@gmx.com" <_govind@gmx.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "lixuefeng@loongson.cn" <lixuefeng@loongson.cn>,
        "yangtiezhu@loongson.cn" <yangtiezhu@loongson.cn>
Subject: RE: [PATCH RESEND] net/cisco: Fix a sleep-in-atomic-context bug in
 enic_init_affinity_hint()
Thread-Topic: [PATCH RESEND] net/cisco: Fix a sleep-in-atomic-context bug in
 enic_init_affinity_hint()
Thread-Index: AQHWSTYrQiYT8rgg0Ee7vd1HHqJOFKjmuV2AgAAO4ACAADrGgIAAGDMAgAAE/ACAAAGysA==
Date:   Wed, 24 Jun 2020 06:32:36 +0000
Message-ID: <BYAPR11MB37994715A3DD8259DF16A34DBA950@BYAPR11MB3799.namprd11.prod.outlook.com>
References: <20200623.143311.995885759487352025.davem@davemloft.net>
 <20200623.152626.2206118203643133195.davem@davemloft.net>
 <7533075e-0e8e-2fde-c8fa-72e2ea222176@loongson.cn>
 <20200623.202324.442008830004872069.davem@davemloft.net>
 <70519029-1cfa-5fce-52f3-cfb13bf00f7d@loongson.cn>
In-Reply-To: <70519029-1cfa-5fce-52f3-cfb13bf00f7d@loongson.cn>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: ff8657fa-9cb7-484a-1c55-08d818086268
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Jun 2020 06:32:36.1111
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 5ae1af62-9505-4097-a69a-c1553ef7840e
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: /v1vytIOfs7EZ7cBAO9yiiye6yG5unzxSiOIGdLwT/ZCarFTFQQTziwD4otf817c
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR11MB3958
X-OriginatorOrg: cisco.com
X-Outbound-SMTP-Client: 173.37.102.14, xch-rcd-004.cisco.com
X-Outbound-Node: alln-core-3.cisco.com
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org>
> On Behalf Of Kaige Li
> Sent: Tuesday, June 23, 2020 8:41 PM
> To: David Miller <davem@davemloft.net>
> Cc: Christian Benvenuti (benve) <benve@cisco.com>; _govind@gmx.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org;
> lixuefeng@loongson.cn; yangtiezhu@loongson.cn
> Subject: Re: [PATCH RESEND] net/cisco: Fix a sleep-in-atomic-context bug =
in
> enic_init_affinity_hint()
>=20
>=20
> On 06/24/2020 11:23 AM, David Miller wrote:
> > From: Kaige Li <likaige@loongson.cn>
> > Date: Wed, 24 Jun 2020 09:56:47 +0800
> >
> >> On 06/24/2020 06:26 AM, David Miller wrote:
> >>> From: David Miller <davem@davemloft.net>
> >>> Date: Tue, 23 Jun 2020 14:33:11 -0700 (PDT)
> >>>
> >>>> Calling a NIC driver open function from a context holding a
> >>>> spinlock is very much the real problem, so many operations have to
> >>>> sleep and in face that ->ndo_open() method is defined as being
> >>>> allowed to sleep and that's why the core networking never invokes
> >>>> it with spinlocks
> >>>                                                         ^^^^
> >>>
> >>> I mean "without" of course. :-)
> >>>
> >>>> held.
> >> Did you mean that open function should be out of spinlock? If so, I
> >> will send V2 patch.
> > Yes, but only if that is safe.
> >
> > You have to analyze the locking done by this driver and fix it properly=
.
> > I anticipate it is not just a matter of changing where the spinlock
> > is held, you will have to rearchitect things a bit.
>=20
> Okay, I will careful analyze this question, and make a suitable patch in =
V2.
>=20
> Thank you.

Hi David, Kaige,
I assume you are referring to the enic_api_lock spin_lock used in

  enic_reset()
  which is used to hard-reset the interface when the driver receives an err=
or interrupt

and

  enic_tx_hang_reset()
  which is used to soft-reset the interface when the stack detects the TX t=
imeout.

Both reset functions above are called in the context of a workqueue.
However, the same spin_lock (enic_api_lock) is taken by the enic_api_devcmd=
_proxy_by_index() api that is exported by the enic driver and that the usni=
c_verbs driver uses to send commands to the firmware.
This spin_lock was likely added to guarantee that no firmware command is se=
nt by usnic_verbs to an enic interface that is undergoing a reset.
Unfortunately changing that spin_lock to a mutex will likely not work on th=
e usnic_verbs side, and removing the spin_lock will require a rearchitect o=
f the code as mentioned by David.

Kaige, V2 is of course more than welcome and we can test it too.
We/Cisco will also look into it, hopefully a small code reorg will be suffi=
cient.

David, from your previous emails on this 3D I assume
- we can leave request_irq() in ndo_open (ie, no need to move it to pci/pro=
be), which is done by a number of other drivers too.
- no need to change GFP_KERNEL to GFP_ATOMIC as it was suggested in the ori=
ginal patch.

Thanks!
/Chris