From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751958AbdGWHVJ (ORCPT ); Sun, 23 Jul 2017 03:21:09 -0400 Received: from mail-ua0-f177.google.com ([209.85.217.177]:33145 "EHLO mail-ua0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750871AbdGWHUn (ORCPT ); Sun, 23 Jul 2017 03:20:43 -0400 MIME-Version: 1.0 In-Reply-To: <20170721090519.GO352@vireshk-i7> References: <52daf22d2c9d92b0e61c8c5c5a88516d7a08a31a.1498026827.git.viresh.kumar@linaro.org> <20170719123751.GF352@vireshk-i7> <20170721090519.GO352@vireshk-i7> From: Ulf Hansson Date: Sun, 23 Jul 2017 09:20:42 +0200 Message-ID: Subject: Re: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains To: Viresh Kumar Cc: Rafael Wysocki , Kevin Hilman , "linux-pm@vger.kernel.org" , Vincent Guittot , Stephen Boyd , Nishanth Menon , Rob Herring , Lina Iyer , Rajendra Nayak , Sudeep Holla , "linux-kernel@vger.kernel.org" , Len Brown , Pavel Machek , Andy Gross , David Brown 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 21 July 2017 at 11:05, Viresh Kumar wrote: > On 21-07-17, 10:35, Ulf Hansson wrote: >> This depends on how drivers are dealing with runtime PM in conjunction >> with the new pm_genpd_update_performance_state(). >> >> In case you don't want to manage some of this in genpd, then each >> driver will have to drop their constraints every time they are about >> to runtime suspend its device. And restore them at runtime resume. >> >> To me, that's seems like a bad idea. Then it's better to make genpd >> deal with this - somehow. > > Right. So we should call the ->set_performance_state() from off/on as > well. Will do that. > >> Yes! >> >> On top of that change, you could also add some validation if the >> get/set callbacks is there are any constraints on how they must be >> assigned. > > I am not sure if I understood that, sorry. What other constraints are > you talking about ? Just thinking that if a genpd is about to be added as a subdomain, and it has ->get_performance_state(), but not ->set_performance_state(), perhaps we should require its master to have ->set_performance_state(). Anyway, I let you do the thinking of what is and what is not needed here. [...] >> >> My main concern is the order of how you take the looks. We never take >> a masters lock before the current domain lock. > > Right and this patch doesn't break that. > >> And when walking the topology, we use the slave links and locks the >> first master from that list. Continues with that tree, then get back >> to slave list and pick the next master. > > Again, that's how this patch does it. > >> If you change that order, we could end getting deadlocks. > > And because that order isn't changed at all, we shouldn't have > deadlocks. True. Trying to clarify more below... > >> >> A general comment is that I think you should look more closely in the >> >> code of genpd_power_off|on(). And also how it calls the >> >> ->power_on|off() callbacks. >> >> >> >> Depending whether you want to update the performance state of the >> >> master domain before the subdomain or the opposite, you will find one >> >> of them being suited for this case as well. >> > >> > Isn't it very much similar to that already ? The only major difference >> > is link->performance_state and I already explained why is it required >> > to be done that way to avoid deadlocks. >> >> No, because you walk the master lists. Thus getting a different order or locks. >> >> I did some drawing of this, using the slave links, and I don't see any >> issues why you can't use that instead. > > Damn, I am confused on which part are you talking about. Let me paste > the code here once again and clarify how this is supposed to work just fine :) I should have been more clear. Walking the master list, then checking each link without using locks - why is that safe? Then even if you think it's safe, then please explain in detail why its needed. Walking the slave list as being done for power off/on should work perfectly okay for your case as well. No? [...] Kind regards Uffe