lkml.org 
[lkml]   [2015]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Re: [PATCHv8 1/9] devfreq: event: Add new devfreq_event class to provide basic data for devfreq governor
> 
> Dear Myungjoo,
>
>On 01/20/2015 01:34 PM, MyungJoo Ham wrote:
>>>
[]
>>> +
>>> + mutex_lock(&edev->lock);
>>> + if (edev->desc->ops && edev->desc->ops->enable) {
>>> + ret = edev->desc->ops->enable(edev);
>>> + if (ret < 0)
>>> + goto err;
>>> + }
>>
>> Is there any reason to call enable(edev) even when enable_count is already > 0
>> while you do not call disable(edev) while enable_count > 0?
>>
>> I think this may incur errors in the related device drivers.
>> (e.g., incorrect pairing of clk/runtime-pm/regulator enable/disable
>> at the device driver side)
>
>You're right. This part has potential errors. I'll fix it as following:
>If edev is already enabled, devfreq_event_enable_edev() will just return
>without any operation because devfreq-event(edev) can handle only one event
>at the same time.
>
> mutex_lock(&edev->lock);
> if (edev->enable_count)
> dev_warn(&edev->dev, "%s is already enabled\n", edev->desc->name);
> ret = -EINVAL;
> goto err;
> }
>
> if (edev->desc->ops && edev->desc->ops->enable) {
> ret = edev->desc->ops->enable(edev);
> if (ret < 0)
> goto err;
> }
> edev->enable_count++;

No, your suggested modification creates another bug.

It should not emit "warn" when enable_count > 0 at enable().
It is a natural behavior from drivers.
- You may have multiple drivers using edev.
- You may have multiple threads using edev.

Thus, the above 12 lines should be replaced with:

if (edev->desc->ops && edev->desc->ops->enable &&
edev->enable_count == 0) {
ret = edev->desc->ops->enable(edev);
if (ret < 0)
goto err;
}
edev->enable_count++;

>
>
>>
>>> + edev->enable_count++;
>>> +err:
>>> + mutex_unlock(&edev->lock);
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(devfreq_event_enable_edev);
>>> +
>>> +/**
>>> + * devfreq_event_disable_edev() - Disable the devfreq-event dev and decrease
>>> + * the enable_count of the devfreq-event dev.
>>> + * @edev : the devfreq-event device
>>> + *
>>> + * Note that this function decrease the enable_count and disable the
>>> + * devfreq-event device. After the devfreq-event device is disabled,
>>> + * devfreq device can't use the devfreq-event device for get/set/reset
>>> + * operations.
>>> + */
>>> +int devfreq_event_disable_edev(struct devfreq_event_dev *edev)
>>> +{
>>> + int ret = 0;
>>> +
>>> + if (!edev || !edev->desc)
>>> + return -EINVAL;
>>> +
>>> + mutex_lock(&edev->lock);
>>> + if (edev->enable_count > 0) {
>>> + edev->enable_count--;
>>> + } else {
>>> + dev_warn(&edev->dev, "unbalanced enable_count\n");
>>> + ret = -EINVAL;
>>> + goto err;
>>> + }
>>> +
>>> + if (edev->desc->ops && edev->desc->ops->disable) {
>>> + ret = edev->desc->ops->disable(edev);
>>> + if (ret < 0) {
>>> + edev->enable_count++;
>>> + goto err;
>>> + }

Anyway, have you seen other subsystems doing fall-back operations as you've
done by "edev->enable_count++" here? Or is this your own idea on falling back
from errors with a disable callback?

>>> + }
>>
>> You did it correctly with disable here;
>> not calling it when it is not required.

Uh..yeah.. the original patch was incorrect..

>
>As I explained, I'll fix it as following:
>
> mutex_lock(&edev->lock);
> if (!edev->enable_count) {
> dev_warn(&edev->dev, "%s is already disabled\n", edev->desc->name);
> ret = -EINVAL;
> goto err;
> }
>
> if (edev->desc->ops && edev->desc->ops->disable) {
> ret = edev->desc->ops->disable(edev);
> if (ret < 0)
> goto err;
> }
> edev->enable_count--;

Uh.... I'd say it is still incorrect.

mutex_lock(&edev->lock);
if (!edev->enable_count) {
dev_warn(&edev->dev, "%s is already disabled\n", edev->desc->name);
ret = -EINVAL;
goto err;
}

edev->enable_count--;
if (edev->desc->ops && edev->desc->ops->disable &&
!edev->enable_count) {
ret = edev->desc->ops->disable(edev);
if (ret < 0)
goto err;
}


>
>>
>>> +err:
>>> + mutex_unlock(&edev->lock);
>>> +
>>> + return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(devfreq_event_disable_edev);
>>> +
>>
>> []
>>> +EXPORT_SYMBOL_GPL(devfreq_event_is_enabled);
>> []
>>
>>> +EXPORT_SYMBOL_GPL(devfreq_event_set_event);
>> []
>>
[]
>>> +int devfreq_event_reset_event(struct devfreq_event_dev *edev)
>>> +{
>>> + int ret = 0;
>>> +
>>> + if (!edev || !edev->desc)
>>> + return -EINVAL;
>>> +
>>> + if (!devfreq_event_is_enabled(edev))
>>> + return -EPERM;
>>> +
>>> + mutex_lock(&edev->lock);
>>> + if (edev->desc->ops && edev->desc->ops->reset)
>>> + ret = edev->desc->ops->reset(edev);
>>> + mutex_unlock(&edev->lock);
>>
>> In the context of the get_event() handling "load",
>> aren't you supposed to set total_event = event = 0; here?
>
>But, devfreq_event_reset_event() function cannot handle edata instance
>because edata is not included in edev. The edata instance is only used in devfreq_event_get_event().

Ah.. ok then.

>
[]

Cheers,
MyungJoo
\
 
 \ /
  Last update: 2015-01-20 08:21    [W:1.212 / U:0.020 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site