Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751867AbdH2JDX (ORCPT ); Tue, 29 Aug 2017 05:03:23 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:48324 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751549AbdH2JDV (ORCPT ); Tue, 29 Aug 2017 05:03:21 -0400 Date: Tue, 29 Aug 2017 11:03:27 +0200 From: Greg Kroah-Hartman To: Shawn Lin Cc: Ulf Hansson , "Rafael J. Wysocki" , Heiko Stuebner , Jaehoon Chung , linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 1/2] driver core: detach device's pm_domain after devres_release_all Message-ID: <20170829090327.GC25315@kroah.com> References: <1502786217-212887-1-git-send-email-shawn.lin@rock-chips.com> <1502786217-212887-2-git-send-email-shawn.lin@rock-chips.com> <20170829064231.GE12198@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2090 Lines: 50 On Tue, Aug 29, 2017 at 04:08:52PM +0800, Shawn Lin wrote: > Hi Greg, > > On 2017/8/29 14:42, Greg Kroah-Hartman wrote: > > On Tue, Aug 15, 2017 at 04:36:56PM +0800, Shawn Lin wrote: > > > Move dev_pm_domain_detach after devres_release_all to avoid > > > accessing device's registers with genpd been powered off. > > > > So, what is this going to break that is working already today? :) > > Thanks for your comment! > > The background of this patch is that: > (1) Some SoCs, including Rockchips' SoCs, couldn't support > accessing controllers' registers w/o clk and power domain enabled. > (2) Many common drivers use devm_request_irq to request irq for either > shared irq or non-shared irq. > (3) So we rely on devres_release_all to free irq automatically. > > So the actually race condition is: > (1) Driver A probe failed or calling remove > (2) power domain is detached right now > (3) A irq triggerd cocurrently just before calling devm_irq_release.. > (4) Driver A's ISR read its register .. panic.. If a probe failed, the ISR should never be called, right? So that should not be an issue here. > The issue is exposed by enabing CONFIG_DEBUG_SHIRQ. Thus devres_free_irq > will try to call the ISR as it says: "It's a shared IRQ -- the driver > ought to be prepared for an IRQ event to happen even now it's being > freed". So it calls the driver's ISR w/o power domain enabled, which > hangup the system... This is theoretically help folks to make the code > robust enough to deal with shared case. > > But, for no matter whether the irq is shared or non-shared, the race > condition is there. So we possible have two choices that > (1) Either using request_irq and free_irq directly > (2) Or moving dev_pm_domain_detach after devres_release_all which > makes sure we free the irq before powering off power domain. > > However doesn't choice(1) imply that devm_request_irq shouldn't > exist? :) So I try to fix it like what this patch does. Ok, this makes a lot more sense, please put this kind of information in the patch changelog when you resend it. thanks, greg k-h