Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752106AbdFUGLu (ORCPT ); Wed, 21 Jun 2017 02:11:50 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:47208 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750979AbdFUGLs (ORCPT ); Wed, 21 Jun 2017 02:11:48 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 1204B6085A Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none (p=none dis=none) header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=none smtp.mailfrom=kvalo@codeaurora.org From: Kalle Valo To: David Miller Cc: baijiaju1990@163.com, manish.chopra@cavium.com, rahul.verma@cavium.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] netxen: Fix a sleep-in-atomic bug in netxen_nic_pci_mem_access_direct References: <1497840533-4894-1-git-send-email-baijiaju1990@163.com> <20170620.133530.1607963470682255531.davem@davemloft.net> Date: Wed, 21 Jun 2017 09:11:43 +0300 In-Reply-To: <20170620.133530.1607963470682255531.davem@davemloft.net> (David Miller's message of "Tue, 20 Jun 2017 13:35:30 -0400 (EDT)") Message-ID: <87d19xooo0.fsf@purkki.adurom.net> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.4 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1484 Lines: 45 David Miller writes: > From: Jia-Ju Bai > Date: Mon, 19 Jun 2017 10:48:53 +0800 > >> The driver may sleep under a spin lock, and the function call path is: >> netxen_nic_pci_mem_access_direct (acquire the lock by spin_lock) >> ioremap --> may sleep >> >> To fix it, the lock is released before "ioremap", and the lock is >> acquired again after this function. >> >> Signed-off-by: Jia-Ju Bai > > This style of change you are making is really starting to be a > problem. > > You can't just drop locks like this, especially without explaining > why it's ok, and why the mutual exclusion this code was trying to > achieve is still going to be OK afterwards. > > In fact, I see zero analysis of the locking situation here, why > it was needed in the first place, and why your change is OK in > that context. > > Any locking change is delicate, and you must put the greatest of > care and consideration into it. > > Just putting "unlock/lock" around the sleeping operation shows a > very low level of consideration for the implications of the change > you are making. > > This isn't like making whitespace fixes, sorry... We already tried to explain this to Jia-Ju during review of a wireless patch: https://patchwork.kernel.org/patch/9756585/ Jia-Ju, you should listen to feedback. If you continue submitting random patches like this makes it hard for maintainers to trust your patches anymore. -- Kalle Valo