[PATCH 1/8] PM / Domains: Make genpd state allocation dynamic

[Ulf Hansson]

On 6 October 2016 at 22:57, Lina Iyer <lina.iyer at linaro.org> wrote:
> On Thu, Oct 06 2016 at 13:45 -0600, Ulf Hansson wrote:
>>
>> On 6 October 2016 at 17:40, Lina Iyer <lina.iyer at linaro.org> wrote:
>>>
>>> On Thu, Oct 06 2016 at 02:37 -0600, Ulf Hansson wrote:
>>>>
>>>>
>>>> On 5 October 2016 at 22:31, Lina Iyer <lina.iyer at linaro.org> wrote:
>>>>>
>>>>>
>>>>> Allow PM Domain states to be defined dynamically by the drivers. This
>>>>> removes the limitation on the maximum number of states possible for a
>>>>> domain.
>>>>>
>>>>> Cc: Axel Haslam <ahaslam+renesas at baylibre.com>
>>>>> Suggested-by: Ulf Hansson <ulf.hansson at linaro.org>
>>>>> Signed-off-by: Lina Iyer <lina.iyer at linaro.org>
>>>
>>>
>>> <...>
>>>>>
>>>>>
>>>>> -#define GENPD_MAX_NUM_STATES   8 /* Number of possible low power
>>>>> states
>>>>> */
>>>>> -
>>>>>  enum gpd_status {
>>>>>         GPD_STATE_ACTIVE = 0,   /* PM domain is active */
>>>>>         GPD_STATE_POWER_OFF,    /* PM domain is off */
>>>>> @@ -70,7 +68,7 @@ struct generic_pm_domain {
>>>>>         void (*detach_dev)(struct generic_pm_domain *domain,
>>>>>                            struct device *dev);
>>>>>         unsigned int flags;             /* Bit field of configs for
>>>>> genpd
>>>>> */
>>>>> -       struct genpd_power_state states[GENPD_MAX_NUM_STATES];
>>>>> +       struct genpd_power_state *states;
>>>>>         unsigned int state_count; /* number of states */
>>>>>         unsigned int state_idx; /* state that genpd will go to when off
>>>>> */
>>>>>
>>>>> --
>>>>> 2.7.4
>>>>>
>>>>
>>>> In general I like the improvement, but..
>>>>
>>>> This change implies that ->states may very well be NULL. This isn't
>>>> validated by genpd's internal logic when power off/on the domain
>>>> (genpd_power_on|off(), __default_power_down_ok()). You need to fix
>>>> this, somehow.
>>>>
>>> Good point.
>>>
>>>> Perhaps the easiest solutions is, when pm_genpd_init() finds that
>>>> ->state is NULL, that we allocate a struct genpd_power_state with
>>>> array size of 1 and assign it to ->states. Although, doing this also
>>>> means you need to track that genpd was responsible for the the
>>>> allocation, so it must also free the data from within genpd_remove().
>>>>
>>>> Unless you have other ideas!?
>>>>
>>> I can think of some hacks, but they are uglier than the problem we are
>>> trying to solve. We could drop this patch. Real world situations would
>>> not have more than 8 states and if there is one, we can think about it
>>> then.
>>
>>
>> The problem with the current approach is that we waste some memory as
>> we always have an array of 8 states per genpd. In the worst case,
>> which currently is the most common case, only 1 out of 8 states is
>> being used.
>>
>> So, let's not be lazy here and instead take the opportunity to fix
>> this, and especially I think this makes sense, before we go on and add
>> the DT parsing of the domain-idle-states.
>>
> Hmm.. We are not wasting much memory in comparison, but if you insist,
> sure.
>
>> The more sophisticated method would probably be to use kobject/kref,
>> but let's not go there for now. Instead let's try an easy method of
>> just tracking whether the allocations had been made internally by
>> genpd, via adding a "bool state_allocated to the struct
>> generic_pm_domain. Would that work?
>>
> It would work.
>
> i.
> How about an additional static state by default in the domain structure,
> if the platform does not provide a state then the default structure is
> used. That way we dont have to track it. But it does waste memory
> eqivalent to a state, when there are states provided by the platform.

I thought about this, but I think it could be a bit messy as well.
Better to do an allocation when needed.

>
> ii.
> I could add a void *free to the domain structure and save the memory
> allocated by default in the *free. At domain remove, we just do a kfree
> on free.
>
> iii.
> Use a boolean flag.

Both ii) and iii) works for me!

Kind regards
Uffe

>
> Thanks,
> Lina
>