lkml.org 
[lkml]   [2017]   [Jul]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains
    On 21 July 2017 at 11:05, Viresh Kumar <viresh.kumar@linaro.org> 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

    \
     
     \ /
      Last update: 2017-07-23 09:22    [W:3.700 / U:0.560 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site