Re: [PATCH] PM / OPP: Implement free_opp_table function

[Inderpal Singh]

On Tue, May 20, 2014 at 12:04 AM, Nishanth Menon <nm@xxxxxx> wrote:
> On Mon, May 19, 2014 at 1:08 PM, Inderpal Singh <inderpal.s@xxxxxxxxxxx> wrote:
>> Hi Nishanth,
>>
>> Thanks for the review comments.
>>
>> On Mon, May 19, 2014 at 6:43 PM, Nishanth Menon <nm@xxxxxx> wrote:
>>> On 05/16/2014 04:09 AM, Inderpal Singh wrote:
>>>> At the driver unloading time the associated opp table may need
>>>> to be deleted. Otherwise it amounts to memory leak. The existing
>>>> OPP library does not have provison to do so.
>>>>
>>>> Hence this patch implements the function to free the opp table.
>>>>
>>>> Signed-off-by: Inderpal Singh <inderpal.s@xxxxxxxxxxx>
>>>> ---
>>>>  drivers/base/power/opp.c | 41 +++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/pm_opp.h   |  6 ++++++
>>>>  2 files changed, 47 insertions(+)
>>>>
>>>> diff --git a/drivers/base/power/opp.c b/drivers/base/power/opp.c
>>>> index d9e376a..d45ffd5 100644
>>>> --- a/drivers/base/power/opp.c
>>>> +++ b/drivers/base/power/opp.c
>>>> @@ -654,4 +654,45 @@ int of_init_opp_table(struct device *dev)
>>>>       return 0;
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(of_init_opp_table);
>>>> +
>>>> +/**
>>>> + * dev_pm_opp_free_opp_table() - free the opp table
>>>> + * @dev:     device for which we do this operation
>>>> + *
>>>> + * Free up the allocated opp table
>>>> + *
>>>> + * Locking: The internal device_opp and opp structures are RCU protected.
>>>> + * Hence this function internally uses RCU updater strategy with mutex locks to
>>>> + * keep the integrity of the internal data structures. Callers should ensure
>>>> + * that this function is *NOT* called under RCU protection or in contexts where
>>>> + * mutex locking or synchronize_rcu() blocking calls cannot be used.
>>>> + */
>>>> +void dev_pm_opp_free_opp_table(struct device *dev)
>>>> +{
>>>> +     struct device_opp *dev_opp = NULL;
>>>> +     struct dev_pm_opp *opp;
>>>> +
>>> if (!dev)
>>>         return;
>>>
>>
>> missed it. Will take care in the next version.
>>
>>>> +     /* Hold our list modification lock here */
>>>> +     mutex_lock(&dev_opp_list_lock);
>>>> +
>>>> +     /* Check for existing list for 'dev' */
>>>> +     dev_opp = find_device_opp(dev);
>>>> +     if (IS_ERR(dev_opp)) {
>>>> +             mutex_unlock(&dev_opp_list_lock);
>>>> +             return;
>>>> +     }
>>>> +
>>>> +     while (!list_empty(&dev_opp->opp_list)) {
>>>> +             opp = list_entry_rcu(dev_opp->opp_list.next,
>>>> +                                     struct dev_pm_opp, node);
>>>> +             list_del_rcu(&opp->node);
>>>> +             kfree_rcu(opp, head);
>>>> +     }
>>>
>>> How about the OPP notifiers? should'nt we add a new event
>>> OPP_EVENT_REMOVE?
>>>
>>
>> As this function is to free the whole opp table. Hence, I think,
>> notifier may not be needed. It may be required for per opp removal as
>> is the case with opp addition and enable/disable. But at present there
>> are no users of these notifiers at all. Let me know your view.
>
> umm.. we do have devfreq which depends on OPPs :).

Yes, devfreq does depend on OPPs, but no devfreq driver is registering
its notifier_block to handle OPP notifications.

>
>>> To maintain non-dt behavior coherency, should'nt we rather add a
>>> opp_remove or an opp_del function?
>>
>> Yes we should have opp_remove as well, but what's the use case ?
>> Should we go ahead and implement it Or, wait for the use-case?
>
> IMHO, if we are doing it properly, we should add the requisite
> function as well. we dont want to have differing behavior device tree
> Vs non-DT.

So we will have 2 functions then. One to remove the whole opp table
and the the other to remove the individual OPPs.
I will cover this in v2. Will also take care of the OPP_EVENT_REMOVE
notification part.


Regards,
Inder

>
> Regards,
> Nishanth Menon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/