On 10/22/20 11:30 AM, Dmitry Osipenko wrote:
> Hello Chanwoo,
>
> 20.10.2020 06:04, Chanwoo Choi пишет:
>> @@ -1354,55 +1365,71 @@ static ssize_t governor_store(struct device *dev,
>> struct device_attribute *attr,
>> struct devfreq *df = to_devfreq(dev);
>> int ret;
>> char str_governor[DEVFREQ_NAME_LEN + 1];
>> - const struct devfreq_governor *governor, *prev_governor;
>> + const struct devfreq_governor *new_governor, *prev_governor;
>>
>> if (!df->governor)
>> return -EINVAL;
>> + prev_governor = df->governor;
>>
>> ret = sscanf(buf, "%" __stringify(DEVFREQ_NAME_LEN) "s", str_governor);
>> if (ret != 1)
>> return -EINVAL;
>>
>> mutex_lock(&devfreq_list_lock);
>> - governor = try_then_request_governor(str_governor);
>> - if (IS_ERR(governor)) {
>> - ret = PTR_ERR(governor);
>> + new_governor = try_then_request_governor(str_governor);
>> + if (IS_ERR(new_governor)) {
>> + ret = PTR_ERR(new_governor);
>> goto out;
>> }
>> - if (df->governor == governor) {
>> + if (prev_governor == new_governor) {
>> ret = 0;
>> goto out;
>> - } else if (IS_SUPPORTED_FLAG(df->governor->flags, IMMUTABLE)
>> - || IS_SUPPORTED_FLAG(governor->flags, IMMUTABLE)) {
>> + } else if (IS_SUPPORTED_FLAG(prev_governor->flags, IMMUTABLE)
>> + || IS_SUPPORTED_FLAG(new_governor->flags, IMMUTABLE)) {
>> ret = -EINVAL;
>> goto out;
>> }
>>
>> - ret = df->governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
>> + /*
>> + * Stop the previous governor and remove the specific sysfs files
>> + * which depend on previous governor.
>> + */
>> + ret = prev_governor->event_handler(df, DEVFREQ_GOV_STOP, NULL);
>> if (ret) {
>> dev_warn(dev, "%s: Governor %s not stopped(%d)\n",
>> - __func__, df->governor->name, ret);
>> + __func__, prev_governor->name, ret);
>> goto out;
>> }
>>
>> - prev_governor = df->governor;
>> - df->governor = governor;
>> - strncpy(df->governor_name, governor->name, DEVFREQ_NAME_LEN);
>> - ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>> + remove_sysfs_files(df, prev_governor);
>> +
>> + /*
>> + * Start the new governor and create the specific sysfs files
>> + * which depend on new governor.
>> + */
>> + strncpy(df->governor_name, new_governor->name, DEVFREQ_NAME_LEN);
>> + ret = new_governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>
> The "df->governor = new_governor" needs to be set before invocation of
> the event_handler(GOV_START) because governors like a
> performance-governor need to have the df->governor being set before the
> GOV_START callback, otherwise performance governor will use
> devfreq->prev_governor->get_target_freq() in devfreq_update_target(),
> thus setting a wrong (non-max) freq.
You're right. It is my mistake. Thanks for your detailed review.
>
>> if (ret) {
>> dev_warn(dev, "%s: Governor %s not started(%d)\n",
>> - __func__, df->governor->name, ret);
>> - df->governor = prev_governor;
>> + __func__, new_governor->name, ret);
>> strncpy(df->governor_name, prev_governor->name,
>> DEVFREQ_NAME_LEN);
>> - ret = df->governor->event_handler(df, DEVFREQ_GOV_START, NULL);
>
> Same here.
OK.
>
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
