From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Date: Thu, 12 Nov 2015 00:37:49 +0000 Subject: Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() Message-Id: <1611120.Ji2Kgxk0bH@vostro.rjw.lan> List-Id: References: <87h9kulkfg.wl%kuninori.morimoto.gx@renesas.com> <2060819.A5Wq9BbBOH@vostro.rjw.lan> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Ulf Hansson Cc: Geert Uytterhoeven , Kuninori Morimoto , Zhang Rui , Eduardo Valentin , Linux-SH , Linux-Kernel , Linux PM list , Cao Minh Hiep , =?utf-8?B?RHVuZ++8muS6uuOCvQ==?= , Alan Stern On Wednesday, November 11, 2015 12:03:52 PM Ulf Hansson wrote: > On 11 November 2015 at 00:57, Rafael J. Wysocki wrote: > > On Tuesday, November 10, 2015 02:00:38 PM Ulf Hansson wrote: > >> +Rafael, Alan > >> > >> On 10 November 2015 at 11:10, Geert Uytterhoeven wrote: > >> > Hi Ulf, > >> > > > > > [cut] > > > >> >> > >> >> The problem is that the runtime PM status of the device isn't > >> >> correctly updated at ->remove(). The effect is that the the > >> >> pm_runtime_get_sync() in ->probe() at re-bind will *not* trigger the > >> >> ->runtime_resume() callbacks to be invoked, as the runtime PM core > >> >> believes the device is already runtime resumed. > >> > > >> > So that's where it should be fixed? > >> > >> That would be a more generic approach, although I am not sure how the > >> driver/PM core should be able to take the correct decision in this > >> phase. Devices may be runtime PM managed also without a driver bound. > >> > >> Perhaps when __device_release_driver() finds a bounded driver for the > >> device, it could after all actions been performed to unbind the > >> driver, check if runtime PM is enabled. If it isn't, it could set the > >> runtime PM status to suspended!? > >> > >> I have no idea if that would introduce other issues as it would kind > >> of force the runtime PM status of the device to suspend, without > >> actually knowing if it's the correct thing to do. > > > > IMO, that needs to depend on the bus type. If the bus type has a way > > to manage PM for devices without drivers, it should be allowed to do so. > > By following my suggestion above, we would allow the bus/driver's > ->remove() to manage whether runtime PM should be enabled/disabled for > the device, before __device_release_driver() checks that. > Don't you think that the driver core could rely on that information? > > I realize that it would be a kind of policy decision for runtime PM, > but it's quite similar as when register/unregister devices when we set > the runtime PM status to suspended. OK If we did that, all devices that had just been unbound from their drivers and had runtime PM disabled after that would be set to "suspended" by the core, right? If that helps, I don't really have objections. Thanks, Rafael From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752598AbbKLAhf (ORCPT ); Wed, 11 Nov 2015 19:37:35 -0500 Received: from v094114.home.net.pl ([79.96.170.134]:56491 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1752511AbbKLAhe (ORCPT ); Wed, 11 Nov 2015 19:37:34 -0500 From: "Rafael J. Wysocki" To: Ulf Hansson Cc: Geert Uytterhoeven , Kuninori Morimoto , Zhang Rui , Eduardo Valentin , Linux-SH , Linux-Kernel , Linux PM list , Cao Minh Hiep , =?utf-8?B?RHVuZ++8muS6uuOCvQ==?= , Alan Stern Subject: Re: [PATCH 2/2] thermal: rcar_thermal: use pm_runtime_put_sync() Date: Thu, 12 Nov 2015 02:06:54 +0100 Message-ID: <1611120.Ji2Kgxk0bH@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/4.1.0-rc5+; KDE/4.11.5; x86_64; ; ) In-Reply-To: References: <87h9kulkfg.wl%kuninori.morimoto.gx@renesas.com> <2060819.A5Wq9BbBOH@vostro.rjw.lan> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wednesday, November 11, 2015 12:03:52 PM Ulf Hansson wrote: > On 11 November 2015 at 00:57, Rafael J. Wysocki wrote: > > On Tuesday, November 10, 2015 02:00:38 PM Ulf Hansson wrote: > >> +Rafael, Alan > >> > >> On 10 November 2015 at 11:10, Geert Uytterhoeven wrote: > >> > Hi Ulf, > >> > > > > > [cut] > > > >> >> > >> >> The problem is that the runtime PM status of the device isn't > >> >> correctly updated at ->remove(). The effect is that the the > >> >> pm_runtime_get_sync() in ->probe() at re-bind will *not* trigger the > >> >> ->runtime_resume() callbacks to be invoked, as the runtime PM core > >> >> believes the device is already runtime resumed. > >> > > >> > So that's where it should be fixed? > >> > >> That would be a more generic approach, although I am not sure how the > >> driver/PM core should be able to take the correct decision in this > >> phase. Devices may be runtime PM managed also without a driver bound. > >> > >> Perhaps when __device_release_driver() finds a bounded driver for the > >> device, it could after all actions been performed to unbind the > >> driver, check if runtime PM is enabled. If it isn't, it could set the > >> runtime PM status to suspended!? > >> > >> I have no idea if that would introduce other issues as it would kind > >> of force the runtime PM status of the device to suspend, without > >> actually knowing if it's the correct thing to do. > > > > IMO, that needs to depend on the bus type. If the bus type has a way > > to manage PM for devices without drivers, it should be allowed to do so. > > By following my suggestion above, we would allow the bus/driver's > ->remove() to manage whether runtime PM should be enabled/disabled for > the device, before __device_release_driver() checks that. > Don't you think that the driver core could rely on that information? > > I realize that it would be a kind of policy decision for runtime PM, > but it's quite similar as when register/unregister devices when we set > the runtime PM status to suspended. OK If we did that, all devices that had just been unbound from their drivers and had runtime PM disabled after that would be set to "suspended" by the core, right? If that helps, I don't really have objections. Thanks, Rafael